Merge pull request #7446 from hashicorp/b-jit-resource-validate

core: rerun resource validation before plan and apply

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
+}

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,
 	}
 }

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,

helper/resource/testing.go

@@ -138,6 +138,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
@@ -244,10 +249,21 @@ func Test(t TestT, c TestCase) {
 
 		// If there was an error, exit
 		if err != nil {
-			errored = true
+			// Perhaps we expected an error? Check if it matches
-			t.Error(fmt.Sprintf(
+			if step.ExpectError != nil {
-				"Step %d error: %s", i, err))
+				if !step.ExpectError.MatchString(err.Error()) {
-			break
+					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

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
 	}
-
+	var errs error
-	return p, nil
+	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

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")

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")

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
+	// If the resource name doesn't match the name regular
-	// expression, show a warning.
+	// 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."+
+				"dashes, and underscores.", n.ResourceName))
-				n.ResourceName))
 	}
 
-	if len(warns) == 0 && len(errs) == 0 {
+	if (len(warns) == 0 || n.IgnoreWarnings) && len(errs) == 0 {
 		return nil, nil
 	}
 

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)
+	}
+}

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}"
+}

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}"
+}

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,