diff --git a/builtin/providers/test/resource_test.go b/builtin/providers/test/resource_test.go index c281485a6b8c..f98759842f7c 100644 --- a/builtin/providers/test/resource_test.go +++ b/builtin/providers/test/resource_test.go @@ -27,6 +27,103 @@ 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 testAccCheckResourceDestroy(s *terraform.State) error { return nil } diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index a815f3928aaa..c07e938d9fc9 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 f1a41efb2ed8..c44c06c18032 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_diff.go b/terraform/eval_diff.go index ea6b124c3f63..06d85958f2e6 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -53,7 +53,7 @@ func (n *EvalCompareDiff) Eval(ctx EvalContext) (interface{}, error) { " Resource ID: %s\n"+ " Mismatch reason: %s\n"+ " Diff One (usually from plan): %#v\n"+ - " Diff Two (usually from apply): %#v\n"+ + " Diff Two (usually from apply): %#vv\n"+ "\n"+ "Also include as much context as you can about your config, state, "+ "and the steps you performed to trigger this error.\n", diff --git a/terraform/eval_ignore_changes.go b/terraform/eval_ignore_changes.go index cc2261313714..a012adf8efc6 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,36 @@ func (n *EvalIgnoreChanges) Eval(ctx EvalContext) (interface{}, error) { } } + // If we are replacing the resource, we need to check if we filtered out the + // RequiresNew attribute, since that could neutralize the whole diff. + if changeType == DiffDestroyCreate { + for k, v := range diff.Attributes { + if v.Empty() { + 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 diff does not requireNew") + 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 000000000000..d470660ec1cc --- /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 8efb5ec5efc7..f617d24fae19 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