Merge pull request #7563 from hashicorp/b-ignore-changes-dependency

terraform: another set of ignore_changes fixes

builtin/providers/test/resource.go

@@ -35,6 +35,12 @@ func testResource() *schema.Resource {
 				Optional: true,
 				Computed: true,
 			},
+			"optional_computed_force_new": {
+				Type:     schema.TypeString,
+				Optional: true,
+				Computed: true,
+				ForceNew: true,
+			},
 			"computed_read_only": {
 				Type:     schema.TypeString,
 				Computed: true,

builtin/providers/test/resource_test.go

@@ -293,6 +293,106 @@ resource "test_resource" "foo" {
 	})
 }
 
+func TestResource_ignoreChangesDependent(t *testing.T) {
+	resource.UnitTest(t, resource.TestCase{
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckResourceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource" "foo" {
+	count = 2
+	required = "yep"
+	required_map { key = "value" }
+
+	optional_force_new = "one"
+	lifecycle {
+		ignore_changes = ["optional_force_new"]
+	}
+}
+resource "test_resource" "bar" {
+	count = 2
+	required = "yep"
+	required_map { key = "value" }
+	optional = "${element(test_resource.foo.*.id, count.index)}"
+}
+				`),
+				Check: func(s *terraform.State) error {
+					return nil
+				},
+			},
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource" "foo" {
+	count = 2
+	required = "yep"
+	required_map { key = "value" }
+
+	optional_force_new = "two"
+	lifecycle {
+		ignore_changes = ["optional_force_new"]
+	}
+}
+resource "test_resource" "bar" {
+	count = 2
+	required = "yep"
+	required_map { key = "value" }
+	optional = "${element(test_resource.foo.*.id, count.index)}"
+}
+				`),
+				Check: func(s *terraform.State) error {
+					return nil
+				},
+			},
+		},
+	})
+}
+
+func TestResource_ignoreChangesStillReplaced(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"
+  required_map = {
+    key = "value"
+  }
+  optional_force_new = "one"
+  optional_bool      = true
+  lifecycle {
+    ignore_changes = ["optional_bool"]
+  }
+}
+				`),
+				Check: func(s *terraform.State) error {
+					return nil
+				},
+			},
+			resource.TestStep{
+				Config: strings.TrimSpace(`
+resource "test_resource" "foo" {
+  required     = "yep"
+  required_map = {
+    key = "value"
+  }
+  optional_force_new = "two"
+  optional_bool      = false
+  lifecycle {
+    ignore_changes = ["optional_bool"]
+  }
+}
+				`),
+				Check: func(s *terraform.State) error {
+					return nil
+				},
+			},
+		},
+	})
+}
+
 func testAccCheckResourceDestroy(s *terraform.State) error {
 	return nil
 }

terraform/context_apply_test.go

@@ -4700,6 +4700,101 @@ aws_instance.foo:
   required_field = set
   type = aws_instance
 `)
+	if actual != expected {
+		t.Fatalf("expected:\n%s\ngot:\n%s", expected, actual)
+	}
+}
+
+func TestContext2Apply_ignoreChangesWithDep(t *testing.T) {
+	m := testModule(t, "apply-ignore-changes-dep")
+	p := testProvider("aws")
+	p.ApplyFn = testApplyFn
+	p.DiffFn = func(i *InstanceInfo, s *InstanceState, c *ResourceConfig) (*InstanceDiff, error) {
+		switch i.Type {
+		case "aws_instance":
+			newAmi, _ := c.Get("ami")
+			return &InstanceDiff{
+				Attributes: map[string]*ResourceAttrDiff{
+					"ami": &ResourceAttrDiff{
+						Old:         s.Attributes["ami"],
+						New:         newAmi.(string),
+						RequiresNew: true,
+					},
+				},
+			}, nil
+		case "aws_eip":
+			return testDiffFn(i, s, c)
+		default:
+			t.Fatalf("Unexpected type: %s", i.Type)
+			return nil, nil
+		}
+	}
+	s := &State{
+		Modules: []*ModuleState{
+			&ModuleState{
+				Path: rootModulePath,
+				Resources: map[string]*ResourceState{
+					"aws_instance.foo.0": &ResourceState{
+						Primary: &InstanceState{
+							ID: "i-abc123",
+							Attributes: map[string]string{
+								"ami": "ami-abcd1234",
+								"id":  "i-abc123",
+							},
+						},
+					},
+					"aws_instance.foo.1": &ResourceState{
+						Primary: &InstanceState{
+							ID: "i-bcd234",
+							Attributes: map[string]string{
+								"ami": "ami-abcd1234",
+								"id":  "i-bcd234",
+							},
+						},
+					},
+					"aws_eip.foo.0": &ResourceState{
+						Primary: &InstanceState{
+							ID: "eip-abc123",
+							Attributes: map[string]string{
+								"id":       "eip-abc123",
+								"instance": "i-abc123",
+							},
+						},
+					},
+					"aws_eip.foo.1": &ResourceState{
+						Primary: &InstanceState{
+							ID: "eip-bcd234",
+							Attributes: map[string]string{
+								"id":       "eip-bcd234",
+								"instance": "i-bcd234",
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+	ctx := testContext2(t, &ContextOpts{
+		Module: m,
+		Providers: map[string]ResourceProviderFactory{
+			"aws": testProviderFuncFixed(p),
+		},
+		State: s,
+	})
+
+	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)
+	}
+
+	actual := strings.TrimSpace(state.String())
+	expected := strings.TrimSpace(s.String())
 	if actual != expected {
 		t.Fatalf("bad: \n%s", actual)
 	}

terraform/eval_diff.go

@@ -3,6 +3,9 @@ package terraform
 import (
 	"fmt"
 	"log"
+	"strings"
+
+	"github.com/hashicorp/terraform/config"
 )
 
 // EvalCompareDiff is an EvalNode implementation that compares two diffs
@@ -73,6 +76,10 @@ type EvalDiff struct {
 	State       **InstanceState
 	OutputDiff  **InstanceDiff
 	OutputState **InstanceState
+
+	// Resource is needed to fetch the ignore_changes list so we can
+	// filter user-requested ignored attributes from the diff.
+	Resource *config.Resource
 }
 
 // TODO: test
@@ -132,6 +139,10 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) {
 		}
 	}
 
+	if err := n.processIgnoreChanges(diff); err != nil {
+		return nil, err
+	}
+
 	// Call post-refresh hook
 	err = ctx.Hook(func(h Hook) (HookAction, error) {
 		return h.PostDiff(n.Info, diff)
@@ -156,6 +167,86 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) {
 	return nil, nil
 }
 
+func (n *EvalDiff) processIgnoreChanges(diff *InstanceDiff) error {
+	if diff == nil || n.Resource == nil || n.Resource.Id() == "" {
+		return nil
+	}
+	ignoreChanges := n.Resource.Lifecycle.IgnoreChanges
+
+	if len(ignoreChanges) == 0 {
+		return nil
+	}
+
+	changeType := diff.ChangeType()
+
+	// If we're just creating the resource, we shouldn't alter the
+	// Diff at all
+	if changeType == DiffCreate {
+		return nil
+	}
+
+	ignorableAttrKeys := make(map[string]bool)
+	for _, ignoredKey := range ignoreChanges {
+		for k := range diff.Attributes {
+			if strings.HasPrefix(k, ignoredKey) {
+				ignorableAttrKeys[k] = true
+			}
+		}
+	}
+
+	// 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 "" ->
+	// "<computed>". 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 {
+				ignorableAttrKeys[k] = true
+			}
+		}
+
+		// 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 _, ok := ignorableAttrKeys[k]; ok {
+				continue
+			}
+			if v.RequiresNew == true {
+				requiresNewAfterIgnores = true
+			}
+		}
+
+		// If we still require resource replacement after ignores, we
+		// can't touch the diff, as all of the attributes will be
+		// required to process the replacement.
+		if requiresNewAfterIgnores {
+			return nil
+		}
+
+		// Here we undo the two reactions to RequireNew in EvalDiff - the "id"
+		// attribute diff and the Destroy boolean field
+		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
+	}
+
+	// If we didn't hit any of our early exit conditions, we can filter the diff.
+	for k := range ignorableAttrKeys {
+		log.Printf("[DEBUG] [EvalIgnoreChanges] %s - Ignoring diff attribute: %s",
+			n.Resource.Id(), k)
+		delete(diff.Attributes, k)
+	}
+
+	return nil
+}
+
 // EvalDiffDestroy is an EvalNode implementation that returns a plain
 // destroy diff.
 type EvalDiffDestroy struct {

terraform/eval_ignore_changes.go

@@ -1,87 +0,0 @@
-package terraform
-
-import (
-	"log"
-	"strings"
-
-	"github.com/hashicorp/terraform/config"
-)
-
-// EvalIgnoreChanges is an EvalNode implementation that removes diff
-// attributes if their name matches names provided by the resource's
-// IgnoreChanges lifecycle.
-type EvalIgnoreChanges struct {
-	Resource      *config.Resource
-	Diff          **InstanceDiff
-	WasChangeType *DiffChangeType
-}
-
-func (n *EvalIgnoreChanges) Eval(ctx EvalContext) (interface{}, error) {
-	if n.Diff == nil || *n.Diff == nil || n.Resource == nil || n.Resource.Id() == "" {
-		return nil, nil
-	}
-
-	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) {
-				delete(diff.Attributes, name)
-			}
-		}
-	}
-
-	// 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 "" ->
-	// "<computed>". 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
-}

terraform/test-fixtures/apply-ignore-changes-dep/main.tf

@@ -0,0 +1,12 @@
+resource "aws_instance" "foo" {
+  count         = 2
+  ami           = "ami-bcd456"
+  lifecycle {
+    ignore_changes = ["ami"]
+  }
+}
+
+resource "aws_eip" "foo" {
+  count    = 2
+  instance = "${element(aws_instance.foo.*.id, count.index)}"
+}

terraform/transform_resource.go

@@ -345,6 +345,7 @@ func (n *graphNodeExpandedResource) managedResourceEvalNodes(resource *Resource,
 				&EvalDiff{
 					Info:        info,
 					Config:      &resourceConfig,
+					Resource:    n.Resource,
 					Provider:    &provider,
 					State:       &state,
 					OutputDiff:  &diff,
@@ -354,10 +355,6 @@ func (n *graphNodeExpandedResource) managedResourceEvalNodes(resource *Resource,
 					Resource: n.Resource,
 					Diff:     &diff,
 				},
-				&EvalIgnoreChanges{
-					Resource: n.Resource,
-					Diff:     &diff,
-				},
 				&EvalWriteState{
 					Name:         n.stateId(),
 					ResourceType: n.Resource.Type,
@@ -404,7 +401,6 @@ func (n *graphNodeExpandedResource) managedResourceEvalNodes(resource *Resource,
 	var err error
 	var createNew bool
 	var createBeforeDestroyEnabled bool
-	var wasChangeType DiffChangeType
 	nodes = append(nodes, &EvalOpFilter{
 		Ops: []walkOperation{walkApply, walkDestroy},
 		Node: &EvalSequence{
@@ -426,7 +422,6 @@ func (n *graphNodeExpandedResource) managedResourceEvalNodes(resource *Resource,
 							return true, EvalEarlyExitError{}
 						}
 
-						wasChangeType = diffApply.ChangeType()
 						diffApply.Destroy = false
 						return true, nil
 					},
@@ -477,16 +472,12 @@ func (n *graphNodeExpandedResource) managedResourceEvalNodes(resource *Resource,
 				&EvalDiff{
 					Info:       info,
 					Config:     &resourceConfig,
+					Resource:   n.Resource,
 					Provider:   &provider,
 					Diff:       &diffApply,
 					State:      &state,
 					OutputDiff: &diffApply,
 				},
-				&EvalIgnoreChanges{
-					Resource:      n.Resource,
-					Diff:          &diffApply,
-					WasChangeType: &wasChangeType,
-				},
 
 				// Get the saved diff
 				&EvalReadDiff{