diff --git a/builtin/providers/test/data_source.go b/builtin/providers/test/data_source.go new file mode 100644 index 000000000..206b8fb84 --- /dev/null +++ b/builtin/providers/test/data_source.go @@ -0,0 +1,28 @@ +package test + +import ( + "time" + + "github.com/hashicorp/terraform/helper/schema" +) + +func testDataSource() *schema.Resource { + return &schema.Resource{ + Read: testDataSourceRead, + + Schema: map[string]*schema.Schema{ + "list": &schema.Schema{ + Type: schema.TypeList, + Computed: true, + Elem: &schema.Schema{Type: schema.TypeString}, + }, + }, + } +} + +func testDataSourceRead(d *schema.ResourceData, meta interface{}) error { + d.SetId(time.Now().UTC().String()) + d.Set("list", []interface{}{"one", "two", "three"}) + + return nil +} diff --git a/builtin/providers/test/provider.go b/builtin/providers/test/provider.go index a8e8c2470..fc85ad4a4 100644 --- a/builtin/providers/test/provider.go +++ b/builtin/providers/test/provider.go @@ -10,6 +10,9 @@ func Provider() terraform.ResourceProvider { ResourcesMap: map[string]*schema.Resource{ "test_resource": testResource(), }, + DataSourcesMap: map[string]*schema.Resource{ + "test_data_source": testDataSource(), + }, ConfigureFunc: providerConfigure, } } diff --git a/builtin/providers/test/resource_test.go b/builtin/providers/test/resource_test.go index 32a501acf..35878e0b1 100644 --- a/builtin/providers/test/resource_test.go +++ b/builtin/providers/test/resource_test.go @@ -1,6 +1,7 @@ package test import ( + "regexp" "strings" "testing" @@ -82,7 +83,7 @@ resource "test_resource" "foo" { Config: strings.TrimSpace(` resource "test_resource" "foo" { required = "yep" - required_map = { + required_map { key = "value" } optional_force_new = "two" @@ -108,7 +109,7 @@ func TestResource_ignoreChangesForceNew(t *testing.T) { Config: strings.TrimSpace(` resource "test_resource" "foo" { required = "yep" - required_map = { + required_map { key = "value" } optional_force_new = "one" @@ -189,6 +190,61 @@ resource "test_resource" "foo" { }) } +// Reproduces plan-time panic described in GH-7170 +func TestResource_dataSourceListPlanPanic(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +data "test_data_source" "foo" {} +resource "test_resource" "foo" { + required = "${data.test_data_source.foo.list}" + required_map = { + key = "value" + } +} + `), + ExpectError: regexp.MustCompile(`must be a single value, not a list`), + Check: func(s *terraform.State) error { + return nil + }, + }, + }, + }) +} + +// Reproduces apply-time panic described in GH-7170 +func TestResource_dataSourceListApplyPanic(t *testing.T) { + resource.UnitTest(t, resource.TestCase{ + Providers: testAccProviders, + CheckDestroy: testAccCheckResourceDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource" "foo" { + required = "ok" + required_map = { + key = "value" + } +} +resource "test_resource" "bar" { + required = "${test_resource.foo.computed_list}" + required_map = { + key = "value" + } +} + `), + ExpectError: regexp.MustCompile(`must be a single value, not a list`), + Check: func(s *terraform.State) error { + return nil + }, + }, + }, + }) +} + func TestResource_ignoreChangesMap(t *testing.T) { resource.UnitTest(t, resource.TestCase{ Providers: testAccProviders, diff --git a/helper/resource/testing.go b/helper/resource/testing.go index e1f1caa9c..89128a1dd 100644 --- a/helper/resource/testing.go +++ b/helper/resource/testing.go @@ -142,6 +142,11 @@ type TestStep struct { // looking to verify that a diff occurs ExpectNonEmptyPlan bool + // ExpectError allows the construction of test cases that we expect to fail + // with an error. The specified regexp must match against the error for the + // test to pass. + ExpectError *regexp.Regexp + // PreventPostDestroyRefresh can be set to true for cases where data sources // are tested alongside real resources PreventPostDestroyRefresh bool @@ -250,10 +255,21 @@ func Test(t TestT, c TestCase) { // If there was an error, exit if err != nil { - errored = true - t.Error(fmt.Sprintf( - "Step %d error: %s", i, err)) - break + // Perhaps we expected an error? Check if it matches + if step.ExpectError != nil { + if !step.ExpectError.MatchString(err.Error()) { + errored = true + t.Error(fmt.Sprintf( + "Step %d, expected error:\n\n%s\n\nTo match:\n\n%s\n\n", + i, err, step.ExpectError)) + break + } + } else { + errored = true + t.Error(fmt.Sprintf( + "Step %d error: %s", i, err)) + break + } } // If we've never checked an id-only refresh and our state isn't diff --git a/terraform/context.go b/terraform/context.go index 667ee0473..324cbef70 100644 --- a/terraform/context.go +++ b/terraform/context.go @@ -313,10 +313,15 @@ func (c *Context) Apply() (*State, error) { } // Do the walk + var walker *ContextGraphWalker if c.destroy { - _, err = c.walk(graph, walkDestroy) + walker, err = c.walk(graph, walkDestroy) } else { - _, err = c.walk(graph, walkApply) + walker, err = c.walk(graph, walkApply) + } + + if len(walker.ValidationErrors) > 0 { + err = multierror.Append(err, walker.ValidationErrors...) } // Clean out any unused things @@ -377,7 +382,8 @@ func (c *Context) Plan() (*Plan, error) { } // Do the walk - if _, err := c.walk(graph, operation); err != nil { + walker, err := c.walk(graph, operation) + if err != nil { return nil, err } p.Diff = c.diff @@ -387,8 +393,11 @@ func (c *Context) Plan() (*Plan, error) { if _, err := c.Graph(&ContextGraphOpts{Validate: true}); err != nil { return nil, err } - - return p, nil + var errs error + if len(walker.ValidationErrors) > 0 { + errs = multierror.Append(errs, walker.ValidationErrors...) + } + return p, errs } // Refresh goes through all the resources in the state and refreshes them diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index a287abdd8..fa0817821 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -223,6 +223,79 @@ aws_instance.foo: } } +// Higher level test at TestResource_dataSourceListApplyPanic +func TestContext2Apply_computedAttrRefTypeMismatch(t *testing.T) { + m := testModule(t, "apply-computed-attr-ref-type-mismatch") + p := testProvider("aws") + p.DiffFn = testDiffFn + p.ValidateResourceFn = func(t string, c *ResourceConfig) (ws []string, es []error) { + // Emulate the type checking behavior of helper/schema based validation + if t == "aws_instance" { + ami, _ := c.Get("ami") + switch a := ami.(type) { + case string: + // ok + default: + es = append(es, fmt.Errorf("Expected ami to be string, got %T", a)) + } + } + return + } + p.DiffFn = func( + info *InstanceInfo, + state *InstanceState, + c *ResourceConfig) (*InstanceDiff, error) { + switch info.Type { + case "aws_ami_list": + // Emulate a diff that says "we'll create this list and ids will be populated" + return &InstanceDiff{ + Attributes: map[string]*ResourceAttrDiff{ + "ids.#": &ResourceAttrDiff{NewComputed: true}, + }, + }, nil + case "aws_instance": + // If we get to the diff for instance, we should be able to assume types + ami, _ := c.Get("ami") + _ = ami.(string) + } + return nil, nil + } + p.ApplyFn = func(info *InstanceInfo, s *InstanceState, d *InstanceDiff) (*InstanceState, error) { + if info.Type != "aws_ami_list" { + t.Fatalf("Reached apply for unexpected resource type! %s", info.Type) + } + // Pretend like we make a thing and the computed list "ids" is populated + return &InstanceState{ + ID: "someid", + Attributes: map[string]string{ + "ids.#": "2", + "ids.0": "ami-abc123", + "ids.1": "ami-bcd345", + }, + }, nil + } + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + }) + + if _, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } + + _, err := ctx.Apply() + + if err == nil { + t.Fatalf("Expected err, got none!") + } + expected := "Expected ami to be string" + if !strings.Contains(err.Error(), expected) { + t.Fatalf("expected:\n\n%s\n\nto contain:\n\n%s", err, expected) + } +} + func TestContext2Apply_emptyModule(t *testing.T) { m := testModule(t, "apply-empty-module") p := testProvider("aws") diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index 3b6defb94..18cd7efc6 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -911,6 +911,73 @@ func TestContext2Plan_computedDataResource(t *testing.T) { } } +// Higher level test at TestResource_dataSourceListPlanPanic +func TestContext2Plan_dataSourceTypeMismatch(t *testing.T) { + m := testModule(t, "plan-data-source-type-mismatch") + p := testProvider("aws") + p.ValidateResourceFn = func(t string, c *ResourceConfig) (ws []string, es []error) { + // Emulate the type checking behavior of helper/schema based validation + if t == "aws_instance" { + ami, _ := c.Get("ami") + switch a := ami.(type) { + case string: + // ok + default: + es = append(es, fmt.Errorf("Expected ami to be string, got %T", a)) + } + } + return + } + p.DiffFn = func( + info *InstanceInfo, + state *InstanceState, + c *ResourceConfig) (*InstanceDiff, error) { + if info.Type == "aws_instance" { + // If we get to the diff, we should be able to assume types + ami, _ := c.Get("ami") + _ = ami.(string) + } + return nil, nil + } + ctx := testContext2(t, &ContextOpts{ + Module: m, + // Pretend like we ran a Refresh and the AZs data source was populated. + State: &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: rootModulePath, + Resources: map[string]*ResourceState{ + "data.aws_availability_zones.azs": &ResourceState{ + Type: "aws_availability_zones", + Primary: &InstanceState{ + ID: "i-abc123", + Attributes: map[string]string{ + "names.#": "2", + "names.0": "us-east-1a", + "names.1": "us-east-1b", + }, + }, + }, + }, + }, + }, + }, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + }) + + _, err := ctx.Plan() + + if err == nil { + t.Fatalf("Expected err, got none!") + } + expected := "Expected ami to be string" + if !strings.Contains(err.Error(), expected) { + t.Fatalf("expected:\n\n%s\n\nto contain:\n\n%s", err, expected) + } +} + func TestContext2Plan_dataResourceBecomesComputed(t *testing.T) { m := testModule(t, "plan-data-resource-becomes-computed") p := testProvider("aws") diff --git a/terraform/eval_validate.go b/terraform/eval_validate.go index ed5c167cf..9ae221aa1 100644 --- a/terraform/eval_validate.go +++ b/terraform/eval_validate.go @@ -108,11 +108,13 @@ type EvalValidateResource struct { ResourceName string ResourceType string ResourceMode config.ResourceMode + + // IgnoreWarnings means that warnings will not be passed through. This allows + // "just-in-time" passes of validation to continue execution through warnings. + IgnoreWarnings bool } func (n *EvalValidateResource) Eval(ctx EvalContext) (interface{}, error) { - // TODO: test - provider := *n.Provider cfg := *n.Config var warns []string @@ -127,16 +129,15 @@ func (n *EvalValidateResource) Eval(ctx EvalContext) (interface{}, error) { warns, errs = provider.ValidateDataSource(n.ResourceType, cfg) } - // If the resouce name doesn't match the name regular - // expression, show a warning. + // If the resource name doesn't match the name regular + // expression, show an error. if !config.NameRegexp.Match([]byte(n.ResourceName)) { errs = append(errs, fmt.Errorf( "%s: resource name can only contain letters, numbers, "+ - "dashes, and underscores."+ - n.ResourceName)) + "dashes, and underscores.", n.ResourceName)) } - if len(warns) == 0 && len(errs) == 0 { + if (len(warns) == 0 || n.IgnoreWarnings) && len(errs) == 0 { return nil, nil } diff --git a/terraform/eval_validate_test.go b/terraform/eval_validate_test.go new file mode 100644 index 000000000..d34439781 --- /dev/null +++ b/terraform/eval_validate_test.go @@ -0,0 +1,184 @@ +package terraform + +import ( + "errors" + "strings" + "testing" + + "github.com/hashicorp/terraform/config" +) + +func TestEvalValidateResource_managedResource(t *testing.T) { + mp := testProvider("aws") + mp.ValidateResourceFn = func(rt string, c *ResourceConfig) (ws []string, es []error) { + expected := "aws_instance" + if rt != expected { + t.Fatalf("expected: %s, got: %s", expected, rt) + } + expected = "bar" + val, _ := c.Get("foo") + if val != expected { + t.Fatalf("expected: %s, got: %s", expected, val) + } + return + } + + p := ResourceProvider(mp) + rc := &ResourceConfig{ + Raw: map[string]interface{}{"foo": "bar"}, + } + node := &EvalValidateResource{ + Provider: &p, + Config: &rc, + ResourceName: "foo", + ResourceType: "aws_instance", + ResourceMode: config.ManagedResourceMode, + } + + _, err := node.Eval(&MockEvalContext{}) + if err != nil { + t.Fatalf("err: %s", err) + } + + if !mp.ValidateResourceCalled { + t.Fatal("Expected ValidateResource to be called, but it was not!") + } +} + +func TestEvalValidateResource_dataSource(t *testing.T) { + mp := testProvider("aws") + mp.ValidateDataSourceFn = func(rt string, c *ResourceConfig) (ws []string, es []error) { + expected := "aws_ami" + if rt != expected { + t.Fatalf("expected: %s, got: %s", expected, rt) + } + expected = "bar" + val, _ := c.Get("foo") + if val != expected { + t.Fatalf("expected: %s, got: %s", expected, val) + } + return + } + + p := ResourceProvider(mp) + rc := &ResourceConfig{ + Raw: map[string]interface{}{"foo": "bar"}, + } + node := &EvalValidateResource{ + Provider: &p, + Config: &rc, + ResourceName: "foo", + ResourceType: "aws_ami", + ResourceMode: config.DataResourceMode, + } + + _, err := node.Eval(&MockEvalContext{}) + if err != nil { + t.Fatalf("err: %s", err) + } + + if !mp.ValidateDataSourceCalled { + t.Fatal("Expected ValidateDataSource to be called, but it was not!") + } +} + +func TestEvalValidateResource_validReturnsNilError(t *testing.T) { + mp := testProvider("aws") + mp.ValidateResourceFn = func(rt string, c *ResourceConfig) (ws []string, es []error) { + return + } + + p := ResourceProvider(mp) + rc := &ResourceConfig{} + node := &EvalValidateResource{ + Provider: &p, + Config: &rc, + ResourceName: "foo", + ResourceType: "aws_instance", + ResourceMode: config.ManagedResourceMode, + } + + _, err := node.Eval(&MockEvalContext{}) + if err != nil { + t.Fatalf("Expected nil error, got: %s", err) + } +} + +func TestEvalValidateResource_warningsAndErrorsPassedThrough(t *testing.T) { + mp := testProvider("aws") + mp.ValidateResourceFn = func(rt string, c *ResourceConfig) (ws []string, es []error) { + ws = append(ws, "warn") + es = append(es, errors.New("err")) + return + } + + p := ResourceProvider(mp) + rc := &ResourceConfig{} + node := &EvalValidateResource{ + Provider: &p, + Config: &rc, + ResourceName: "foo", + ResourceType: "aws_instance", + ResourceMode: config.ManagedResourceMode, + } + + _, err := node.Eval(&MockEvalContext{}) + if err == nil { + t.Fatal("Expected an error, got none!") + } + + verr := err.(*EvalValidateError) + if len(verr.Warnings) != 1 || verr.Warnings[0] != "warn" { + t.Fatalf("Expected 1 warning 'warn', got: %#v", verr.Warnings) + } + if len(verr.Errors) != 1 || verr.Errors[0].Error() != "err" { + t.Fatalf("Expected 1 error 'err', got: %#v", verr.Errors) + } +} + +func TestEvalValidateResource_checksResourceName(t *testing.T) { + mp := testProvider("aws") + p := ResourceProvider(mp) + rc := &ResourceConfig{} + node := &EvalValidateResource{ + Provider: &p, + Config: &rc, + ResourceName: "bad*name", + ResourceType: "aws_instance", + ResourceMode: config.ManagedResourceMode, + } + + _, err := node.Eval(&MockEvalContext{}) + if err == nil { + t.Fatal("Expected an error, got none!") + } + expectErr := "resource name can only contain" + if !strings.Contains(err.Error(), expectErr) { + t.Fatalf("Expected err: %s to contain %s", err, expectErr) + } +} + +func TestEvalValidateResource_ignoreWarnings(t *testing.T) { + mp := testProvider("aws") + mp.ValidateResourceFn = func(rt string, c *ResourceConfig) (ws []string, es []error) { + ws = append(ws, "warn") + return + } + + p := ResourceProvider(mp) + rc := &ResourceConfig{} + node := &EvalValidateResource{ + Provider: &p, + Config: &rc, + ResourceName: "foo", + ResourceType: "aws_instance", + ResourceMode: config.ManagedResourceMode, + + IgnoreWarnings: true, + } + + _, err := node.Eval(&MockEvalContext{}) + if err != nil { + t.Fatalf("Expected no error, got: %s", err) + } +} diff --git a/terraform/test-fixtures/apply-computed-attr-ref-type-mismatch/main.tf b/terraform/test-fixtures/apply-computed-attr-ref-type-mismatch/main.tf new file mode 100644 index 000000000..98bc4d3b2 --- /dev/null +++ b/terraform/test-fixtures/apply-computed-attr-ref-type-mismatch/main.tf @@ -0,0 +1,10 @@ +resource "aws_ami_list" "foo" { + # assume this has a computed attr called "ids" +} + +resource "aws_instance" "foo" { + # this is erroneously referencing the list of all ids, but + # since it is a computed attr, the Validate walk won't catch + # it. + ami = "${aws_ami_list.foo.ids}" +} diff --git a/terraform/test-fixtures/plan-data-source-type-mismatch/main.tf b/terraform/test-fixtures/plan-data-source-type-mismatch/main.tf new file mode 100644 index 000000000..d0782f261 --- /dev/null +++ b/terraform/test-fixtures/plan-data-source-type-mismatch/main.tf @@ -0,0 +1,4 @@ +data "aws_availability_zones" "azs" {} +resource "aws_instance" "foo" { + ami = "${data.aws_availability_zones.azs.names}" +} diff --git a/terraform/transform_resource.go b/terraform/transform_resource.go index 81c5bae56..78f62be33 100644 --- a/terraform/transform_resource.go +++ b/terraform/transform_resource.go @@ -328,6 +328,16 @@ func (n *graphNodeExpandedResource) managedResourceEvalNodes(resource *Resource, Name: n.ProvidedBy()[0], Output: &provider, }, + // Re-run validation to catch any errors we missed, e.g. type + // mismatches on computed values. + &EvalValidateResource{ + Provider: &provider, + Config: &resourceConfig, + ResourceName: n.Resource.Name, + ResourceType: n.Resource.Type, + ResourceMode: n.Resource.Mode, + IgnoreWarnings: true, + }, &EvalReadState{ Name: n.stateId(), Output: &state, @@ -454,7 +464,16 @@ func (n *graphNodeExpandedResource) managedResourceEvalNodes(resource *Resource, Name: n.stateId(), Output: &state, }, - + // Re-run validation to catch any errors we missed, e.g. type + // mismatches on computed values. + &EvalValidateResource{ + Provider: &provider, + Config: &resourceConfig, + ResourceName: n.Resource.Name, + ResourceType: n.Resource.Type, + ResourceMode: n.Resource.Mode, + IgnoreWarnings: true, + }, &EvalDiff{ Info: info, Config: &resourceConfig,