From 4a1b36ac0da7ff191cdb4b33500325f2ad4edbaa Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Thu, 30 Jun 2016 18:22:20 -0500 Subject: [PATCH] core: rerun resource validation before plan and apply In #7170 we found two scenarios where the type checking done during the `context.Validate()` graph walk was circumvented, and the subsequent assumption of type safety in the provider's `Diff()` implementation caused panics. Both scenarios have to do with interpolations that reference Computed values. The sentinel we use to indicate that a value is Computed does not carry any type information with it yet. That means that an incorrect reference to a list or a map in a string attribute can "sneak through" validation only to crop up... 1. ...during Plan for Data Source References 2. ...during Apply for Resource references In order to address this, we: * add high-level tests for each of these two scenarios in `provider/test` * add context-level tests for the same two scenarios in `terraform` (these tests proved _really_ tricky to write!) * place an `EvalValidateResource` just before `EvalDiff` and `EvalApply` to catch these errors * add some plumbing to `Plan()` and `Apply()` to return validation errors, which were previously only generated during `Validate()` * wrap unit-tests around `EvalValidateResource` * add an `IgnoreWarnings` option to `EvalValidateResource` to prevent active warnings from halting execution on the second-pass validation Eventually, we might be able to attach type information to Computed values, which would allow for these errors to be caught earlier. For now, this solution keeps us safe from panics and raises the proper errors to the user. Fixes #7170 --- builtin/providers/test/data_source.go | 28 +++ builtin/providers/test/provider.go | 3 + builtin/providers/test/resource_test.go | 60 +++++- helper/resource/testing.go | 24 ++- terraform/context.go | 19 +- terraform/context_apply_test.go | 73 +++++++ terraform/context_plan_test.go | 67 +++++++ terraform/eval_validate.go | 15 +- terraform/eval_validate_test.go | 184 ++++++++++++++++++ .../main.tf | 10 + .../plan-data-source-type-mismatch/main.tf | 4 + terraform/transform_resource.go | 21 +- 12 files changed, 489 insertions(+), 19 deletions(-) create mode 100644 builtin/providers/test/data_source.go create mode 100644 terraform/eval_validate_test.go create mode 100644 terraform/test-fixtures/apply-computed-attr-ref-type-mismatch/main.tf create mode 100644 terraform/test-fixtures/plan-data-source-type-mismatch/main.tf 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,