From f480ae34301832a4d16c3d704f3877a44c05db0d Mon Sep 17 00:00:00 2001 From: Paul Hinze Date: Tue, 15 Mar 2016 10:03:01 -0500 Subject: [PATCH] core: Fix issues with ignore_changes The ignore_changes diff filter was stripping out attributes on Create but the diff was still making it down to the provider, so Create would end up missing attributes, causing a full failure if any required attributes were being ignored. In addition, any changes that required a replacement of the resource were causing problems with `ignore_chages`, which didn't properly filter out the replacement when the triggering attributes were filtered out. Refs #5627 --- builtin/providers/test/provider_test.go | 8 + builtin/providers/test/resource.go | 22 ++- builtin/providers/test/resource_test.go | 139 ++++++++++++++++++ terraform/context_apply_test.go | 43 ++++++ terraform/diff.go | 5 + terraform/eval_ignore_changes.go | 57 ++++++- .../apply-ignore-changes-create/main.tf | 7 + terraform/transform_resource.go | 7 +- 8 files changed, 283 insertions(+), 5 deletions(-) create mode 100644 terraform/test-fixtures/apply-ignore-changes-create/main.tf diff --git a/builtin/providers/test/provider_test.go b/builtin/providers/test/provider_test.go index 680bff119..40defefac 100644 --- a/builtin/providers/test/provider_test.go +++ b/builtin/providers/test/provider_test.go @@ -1,6 +1,8 @@ package test import ( + "testing" + "github.com/hashicorp/terraform/helper/schema" "github.com/hashicorp/terraform/terraform" ) @@ -8,6 +10,12 @@ import ( var testAccProviders map[string]terraform.ResourceProvider var testAccProvider *schema.Provider +func TestProvider(t *testing.T) { + if err := Provider().(*schema.Provider).InternalValidate(); err != nil { + t.Fatalf("err: %s", err) + } +} + func init() { testAccProvider = Provider().(*schema.Provider) testAccProviders = map[string]terraform.ResourceProvider{ diff --git a/builtin/providers/test/resource.go b/builtin/providers/test/resource.go index 93403ea92..9df02bc15 100644 --- a/builtin/providers/test/resource.go +++ b/builtin/providers/test/resource.go @@ -26,6 +26,21 @@ func testResource() *schema.Resource { Optional: true, ForceNew: true, }, + "optional_computed_map": &schema.Schema{ + Type: schema.TypeMap, + Optional: true, + Computed: true, + }, + "computed_read_only": &schema.Schema{ + Type: schema.TypeString, + Computed: true, + ForceNew: true, + }, + "computed_read_only_force_new": &schema.Schema{ + Type: schema.TypeString, + Computed: true, + ForceNew: true, + }, }, } } @@ -37,10 +52,15 @@ func testResourceCreate(d *schema.ResourceData, meta interface{}) error { if _, ok := d.GetOk("required"); !ok { return fmt.Errorf("Missing attribute 'required', but it's required!") } - return nil + return testResourceRead(d, meta) } func testResourceRead(d *schema.ResourceData, meta interface{}) error { + d.Set("computed_read_only", "value_from_api") + d.Set("computed_read_only_force_new", "value_from_api") + if _, ok := d.GetOk("optional_computed_map"); !ok { + d.Set("optional_computed_map", map[string]string{}) + } return nil } diff --git a/builtin/providers/test/resource_test.go b/builtin/providers/test/resource_test.go index c281485a6..6b12227f9 100644 --- a/builtin/providers/test/resource_test.go +++ b/builtin/providers/test/resource_test.go @@ -27,6 +27,145 @@ resource "test_resource" "foo" { }) } +// Targeted test in TestContext2Apply_ignoreChangesCreate +func TestResource_ignoreChangesRequired(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 = "yep" + lifecycle { + ignore_changes = ["required"] + } +} + `), + Check: func(s *terraform.State) error { + return nil + }, + }, + }, + }) +} + +func TestResource_ignoreChangesEmpty(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 = "yep" + optional_force_new = "one" + lifecycle { + ignore_changes = [] + } +} + `), + Check: func(s *terraform.State) error { + return nil + }, + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource" "foo" { + required = "yep" + optional_force_new = "two" + lifecycle { + ignore_changes = [] + } +} + `), + Check: func(s *terraform.State) error { + return nil + }, + }, + }, + }) +} + +func TestResource_ignoreChangesForceNew(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 = "yep" + optional_force_new = "one" + lifecycle { + ignore_changes = ["optional_force_new"] + } +} + `), + Check: func(s *terraform.State) error { + return nil + }, + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource" "foo" { + required = "yep" + optional_force_new = "two" + lifecycle { + ignore_changes = ["optional_force_new"] + } +} + `), + Check: func(s *terraform.State) error { + return nil + }, + }, + }, + }) +} + +func TestResource_ignoreChangesMap(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 = "yep" + optional_computed_map { + foo = "bar" + } + lifecycle { + ignore_changes = ["optional_computed_map"] + } +} + `), + Check: func(s *terraform.State) error { + return nil + }, + }, + resource.TestStep{ + Config: strings.TrimSpace(` +resource "test_resource" "foo" { + required = "yep" + optional_computed_map { + foo = "bar" + no = "update" + } + lifecycle { + ignore_changes = ["optional_computed_map"] + } +} + `), + Check: func(s *terraform.State) error { + return nil + }, + }, + }, + }) +} + func testAccCheckResourceDestroy(s *terraform.State) error { return nil } diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index a815f3928..c07e938d9 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -4084,3 +4084,46 @@ aws_instance.ifailedprovisioners: (1 tainted) t.Fatalf("expected state: \n%s\ngot: \n%s", expected, actual) } } + +// Higher level test exposing the bug this covers in +// TestResource_ignoreChangesRequired +func TestContext2Apply_ignoreChangesCreate(t *testing.T) { + m := testModule(t, "apply-ignore-changes-create") + p := testProvider("aws") + p.ApplyFn = testApplyFn + p.DiffFn = testDiffFn + ctx := testContext2(t, &ContextOpts{ + Module: m, + Providers: map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + }) + + if p, err := ctx.Plan(); err != nil { + t.Fatalf("err: %s", err) + } else { + t.Logf(p.String()) + } + + state, err := ctx.Apply() + if err != nil { + t.Fatalf("err: %s", err) + } + + mod := state.RootModule() + if len(mod.Resources) != 1 { + t.Fatalf("bad: %s", state) + } + + actual := strings.TrimSpace(state.String()) + // Expect no changes from original state + expected := strings.TrimSpace(` +aws_instance.foo: + ID = foo + required_field = set + type = aws_instance +`) + if actual != expected { + t.Fatalf("bad: \n%s", actual) + } +} diff --git a/terraform/diff.go b/terraform/diff.go index f1a41efb2..c44c06c18 100644 --- a/terraform/diff.go +++ b/terraform/diff.go @@ -286,6 +286,11 @@ type ResourceAttrDiff struct { Type DiffAttrType } +// Empty returns true if the diff for this attr is neutral +func (d *ResourceAttrDiff) Empty() bool { + return d.Old == d.New && !d.NewComputed && !d.NewRemoved +} + func (d *ResourceAttrDiff) GoString() string { return fmt.Sprintf("*%#v", *d) } diff --git a/terraform/eval_ignore_changes.go b/terraform/eval_ignore_changes.go index cc2261313..6c7222a8b 100644 --- a/terraform/eval_ignore_changes.go +++ b/terraform/eval_ignore_changes.go @@ -1,6 +1,7 @@ package terraform import ( + "log" "strings" "github.com/hashicorp/terraform/config" @@ -10,8 +11,9 @@ import ( // attributes if their name matches names provided by the resource's // IgnoreChanges lifecycle. type EvalIgnoreChanges struct { - Resource *config.Resource - Diff **InstanceDiff + Resource *config.Resource + Diff **InstanceDiff + WasChangeType *DiffChangeType } func (n *EvalIgnoreChanges) Eval(ctx EvalContext) (interface{}, error) { @@ -22,6 +24,22 @@ func (n *EvalIgnoreChanges) Eval(ctx EvalContext) (interface{}, error) { diff := *n.Diff ignoreChanges := n.Resource.Lifecycle.IgnoreChanges + if len(ignoreChanges) == 0 { + return nil, nil + } + + changeType := diff.ChangeType() + // Let the passed in change type pointer override what the diff currently has. + if n.WasChangeType != nil && *n.WasChangeType != DiffInvalid { + changeType = *n.WasChangeType + } + + // If we're just creating the resource, we shouldn't alter the + // Diff at all + if changeType == DiffCreate { + return nil, nil + } + for _, ignoredName := range ignoreChanges { for name := range diff.Attributes { if strings.HasPrefix(name, ignoredName) { @@ -30,5 +48,40 @@ func (n *EvalIgnoreChanges) Eval(ctx EvalContext) (interface{}, error) { } } + // If we are replacing the resource, then we expect there to be a bunch of + // extraneous attribute diffs we need to filter out for the other + // non-requires-new attributes going from "" -> "configval" or "" -> + // "". Filtering these out allows us to see if we might be able to + // skip this diff altogether. + if changeType == DiffDestroyCreate { + for k, v := range diff.Attributes { + if v.Empty() || v.NewComputed { + delete(diff.Attributes, k) + } + } + + // Here we emulate the implementation of diff.RequiresNew() with one small + // tweak, we ignore the "id" attribute diff that gets added by EvalDiff, + // since that was added in reaction to RequiresNew being true. + requiresNewAfterIgnores := false + for k, v := range diff.Attributes { + if k == "id" { + continue + } + if v.RequiresNew == true { + requiresNewAfterIgnores = true + } + } + + // Here we undo the two reactions to RequireNew in EvalDiff - the "id" + // attribute diff and the Destroy boolean field + if !requiresNewAfterIgnores { + log.Printf("[DEBUG] Removing 'id' diff and setting Destroy to false " + + "because after ignore_changes, this diff no longer requires replacement") + delete(diff.Attributes, "id") + diff.Destroy = false + } + } + return nil, nil } diff --git a/terraform/test-fixtures/apply-ignore-changes-create/main.tf b/terraform/test-fixtures/apply-ignore-changes-create/main.tf new file mode 100644 index 000000000..d470660ec --- /dev/null +++ b/terraform/test-fixtures/apply-ignore-changes-create/main.tf @@ -0,0 +1,7 @@ +resource "aws_instance" "foo" { + required_field = "set" + + lifecycle { + ignore_changes = ["required_field"] + } +} diff --git a/terraform/transform_resource.go b/terraform/transform_resource.go index 8efb5ec5e..f617d24fa 100644 --- a/terraform/transform_resource.go +++ b/terraform/transform_resource.go @@ -372,6 +372,7 @@ func (n *graphNodeExpandedResource) EvalTree() EvalNode { var err error var createNew, tainted bool var createBeforeDestroyEnabled bool + var wasChangeType DiffChangeType seq.Nodes = append(seq.Nodes, &EvalOpFilter{ Ops: []walkOperation{walkApply, walkDestroy}, Node: &EvalSequence{ @@ -393,6 +394,7 @@ func (n *graphNodeExpandedResource) EvalTree() EvalNode { return true, EvalEarlyExitError{} } + wasChangeType = diffApply.ChangeType() diffApply.Destroy = false return true, nil }, @@ -439,8 +441,9 @@ func (n *graphNodeExpandedResource) EvalTree() EvalNode { Output: &diffApply, }, &EvalIgnoreChanges{ - Resource: n.Resource, - Diff: &diffApply, + Resource: n.Resource, + Diff: &diffApply, + WasChangeType: &wasChangeType, }, // Get the saved diff