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
This commit is contained in:
Paul Hinze 2016-06-30 18:22:20 -05:00
parent 0e3b4b4a85
commit 4a1b36ac0d
No known key found for this signature in database
GPG Key ID: B69DEDF2D55501C0
12 changed files with 489 additions and 19 deletions

View File

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

View File

@ -10,6 +10,9 @@ func Provider() terraform.ResourceProvider {
ResourcesMap: map[string]*schema.Resource{ ResourcesMap: map[string]*schema.Resource{
"test_resource": testResource(), "test_resource": testResource(),
}, },
DataSourcesMap: map[string]*schema.Resource{
"test_data_source": testDataSource(),
},
ConfigureFunc: providerConfigure, ConfigureFunc: providerConfigure,
} }
} }

View File

@ -1,6 +1,7 @@
package test package test
import ( import (
"regexp"
"strings" "strings"
"testing" "testing"
@ -82,7 +83,7 @@ resource "test_resource" "foo" {
Config: strings.TrimSpace(` Config: strings.TrimSpace(`
resource "test_resource" "foo" { resource "test_resource" "foo" {
required = "yep" required = "yep"
required_map = { required_map {
key = "value" key = "value"
} }
optional_force_new = "two" optional_force_new = "two"
@ -108,7 +109,7 @@ func TestResource_ignoreChangesForceNew(t *testing.T) {
Config: strings.TrimSpace(` Config: strings.TrimSpace(`
resource "test_resource" "foo" { resource "test_resource" "foo" {
required = "yep" required = "yep"
required_map = { required_map {
key = "value" key = "value"
} }
optional_force_new = "one" 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) { func TestResource_ignoreChangesMap(t *testing.T) {
resource.UnitTest(t, resource.TestCase{ resource.UnitTest(t, resource.TestCase{
Providers: testAccProviders, Providers: testAccProviders,

View File

@ -142,6 +142,11 @@ type TestStep struct {
// looking to verify that a diff occurs // looking to verify that a diff occurs
ExpectNonEmptyPlan bool 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 // PreventPostDestroyRefresh can be set to true for cases where data sources
// are tested alongside real resources // are tested alongside real resources
PreventPostDestroyRefresh bool PreventPostDestroyRefresh bool
@ -250,10 +255,21 @@ func Test(t TestT, c TestCase) {
// If there was an error, exit // If there was an error, exit
if err != nil { 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 // If we've never checked an id-only refresh and our state isn't

View File

@ -313,10 +313,15 @@ func (c *Context) Apply() (*State, error) {
} }
// Do the walk // Do the walk
var walker *ContextGraphWalker
if c.destroy { if c.destroy {
_, err = c.walk(graph, walkDestroy) walker, err = c.walk(graph, walkDestroy)
} else { } 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 // Clean out any unused things
@ -377,7 +382,8 @@ func (c *Context) Plan() (*Plan, error) {
} }
// Do the walk // Do the walk
if _, err := c.walk(graph, operation); err != nil { walker, err := c.walk(graph, operation)
if err != nil {
return nil, err return nil, err
} }
p.Diff = c.diff p.Diff = c.diff
@ -387,8 +393,11 @@ func (c *Context) Plan() (*Plan, error) {
if _, err := c.Graph(&ContextGraphOpts{Validate: true}); err != nil { if _, err := c.Graph(&ContextGraphOpts{Validate: true}); err != nil {
return nil, err 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 // Refresh goes through all the resources in the state and refreshes them

View File

@ -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) { func TestContext2Apply_emptyModule(t *testing.T) {
m := testModule(t, "apply-empty-module") m := testModule(t, "apply-empty-module")
p := testProvider("aws") p := testProvider("aws")

View File

@ -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) { func TestContext2Plan_dataResourceBecomesComputed(t *testing.T) {
m := testModule(t, "plan-data-resource-becomes-computed") m := testModule(t, "plan-data-resource-becomes-computed")
p := testProvider("aws") p := testProvider("aws")

View File

@ -108,11 +108,13 @@ type EvalValidateResource struct {
ResourceName string ResourceName string
ResourceType string ResourceType string
ResourceMode config.ResourceMode 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) { func (n *EvalValidateResource) Eval(ctx EvalContext) (interface{}, error) {
// TODO: test
provider := *n.Provider provider := *n.Provider
cfg := *n.Config cfg := *n.Config
var warns []string var warns []string
@ -127,16 +129,15 @@ func (n *EvalValidateResource) Eval(ctx EvalContext) (interface{}, error) {
warns, errs = provider.ValidateDataSource(n.ResourceType, cfg) 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)) { if !config.NameRegexp.Match([]byte(n.ResourceName)) {
errs = append(errs, fmt.Errorf( errs = append(errs, fmt.Errorf(
"%s: resource name can only contain letters, numbers, "+ "%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 return nil, nil
} }

View File

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

View File

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

View File

@ -0,0 +1,4 @@
data "aws_availability_zones" "azs" {}
resource "aws_instance" "foo" {
ami = "${data.aws_availability_zones.azs.names}"
}

View File

@ -328,6 +328,16 @@ func (n *graphNodeExpandedResource) managedResourceEvalNodes(resource *Resource,
Name: n.ProvidedBy()[0], Name: n.ProvidedBy()[0],
Output: &provider, 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{ &EvalReadState{
Name: n.stateId(), Name: n.stateId(),
Output: &state, Output: &state,
@ -454,7 +464,16 @@ func (n *graphNodeExpandedResource) managedResourceEvalNodes(resource *Resource,
Name: n.stateId(), Name: n.stateId(),
Output: &state, 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{ &EvalDiff{
Info: info, Info: info,
Config: &resourceConfig, Config: &resourceConfig,