terraform: another set of ignore_changes fixes

This set of changes addresses two bug scenarios:

(1) When an ignored change canceled a resource replacement, any
downstream resources referencing computer attributes on that resource
would get "diffs didn't match" errors. This happened because the
`EvalDiff` implementation was calling `state.MergeDiff(diff)` on the
unfiltered diff. Generally this is what you want, so that downstream
references catch the "incoming" values. When there's a potential for the
diff to change, thought, this results in problems w/ references.

Here we solve this by doing away with the separate `EvalNode` for
`ignore_changes` processing and integrating it into `EvalDiff`. This
allows us to only call `MergeDiff` with the final, filtered diff.

(2) When a resource had an ignored change but was still being replaced
anyways, the diff was being improperly filtered. This would cause
problems during apply when not all attributes were available to perform
the replacement.

We solve that by deferring actual attribute removal until after we've
decided that we do not have to replace the resource.

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{