Merge pull request #12897 from hashicorp/jbardin/ignore-changes

ignore_changes causes keys in other flatmapped objects to be lost from diff

terraform/context_plan_test.go

@@ -3095,3 +3095,54 @@ func TestContext2Plan_listOrder(t *testing.T) {
 		t.Fatal("aws_instance.a and aws_instance.b diffs should match:\n", plan)
 	}
 }
+
+// Make sure ignore-changes doesn't interfere with set/list/map diffs.
+// If a resource was being replaced by a RequiresNew attribute that gets
+// ignored, we need to filter the diff properly to properly update rather than
+// replace.
+func TestContext2Plan_ignoreChangesWithFlatmaps(t *testing.T) {
+	m := testModule(t, "plan-ignore-changes-with-flatmaps")
+	p := testProvider("aws")
+	p.DiffFn = testDiffFn
+	s := &State{
+		Modules: []*ModuleState{
+			&ModuleState{
+				Path: rootModulePath,
+				Resources: map[string]*ResourceState{
+					"aws_instance.foo": &ResourceState{
+						Type: "aws_instance",
+						Primary: &InstanceState{
+							ID: "bar",
+							Attributes: map[string]string{
+								"user_data":   "x",
+								"require_new": "",
+								"set.#":       "1",
+								"set.0.a":     "1",
+								"lst.#":       "1",
+								"lst.0":       "j",
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+	ctx := testContext2(t, &ContextOpts{
+		Module: m,
+		Providers: map[string]ResourceProviderFactory{
+			"aws": testProviderFuncFixed(p),
+		},
+		State: s,
+	})
+
+	plan, err := ctx.Plan()
+	if err != nil {
+		t.Fatalf("err: %s", err)
+	}
+
+	actual := strings.TrimSpace(plan.Diff.String())
+	expected := strings.TrimSpace(testTFPlanDiffIgnoreChangesWithFlatmaps)
+	if actual != expected {
+		t.Fatalf("bad:\n%s\n\nexpected\n\n%s", actual, expected)
+	}
+}

terraform/context_test.go

@@ -262,6 +262,11 @@ func testDiffFn(
 			if _, ok := c.Raw["__"+k+"_requires_new"]; ok {
 				attrDiff.RequiresNew = true
 			}
+
+			if attr, ok := s.Attributes[k]; ok {
+				attrDiff.Old = attr
+			}
+
 			diff.Attributes[k] = attrDiff
 		}
 	}

terraform/diff.go

@@ -25,6 +25,9 @@ const (
 	DiffDestroyCreate
 )
 
+// multiVal matches the index key to a flatmapped set, list or map
+var multiVal = regexp.MustCompile(`\.(#|%)$`)
+
 // Diff trackes the changes that are necessary to apply a configuration
 // to an existing infrastructure.
 type Diff struct {
@@ -808,7 +811,6 @@ func (d *InstanceDiff) Same(d2 *InstanceDiff) (bool, string) {
 		}
 
 		// search for the suffix of the base of a [computed] map, list or set.
-		multiVal := regexp.MustCompile(`\.(#|~#|%)$`)
 		match := multiVal.FindStringSubmatch(k)
 
 		if diffOld.NewComputed && len(match) == 2 {

terraform/eval_diff.go

@@ -152,6 +152,7 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) {
 		})
 	}
 
+	// filter out ignored resources
 	if err := n.processIgnoreChanges(diff); err != nil {
 		return nil, err
 	}
@@ -190,62 +191,72 @@ func (n *EvalDiff) processIgnoreChanges(diff *InstanceDiff) error {
 		return nil
 	}
 
-	changeType := diff.ChangeType()
-
 	// If we're just creating the resource, we shouldn't alter the
 	// Diff at all
-	if changeType == DiffCreate {
+	if diff.ChangeType() == DiffCreate {
 		return nil
 	}
 
 	// If the resource has been tainted then we don't process ignore changes
 	// since we MUST recreate the entire resource.
-	if diff.DestroyTainted {
+	if diff.GetDestroyTainted() {
 		return nil
 	}
 
+	attrs := diff.CopyAttributes()
+
+	// get the complete set of keys we want to ignore
 	ignorableAttrKeys := make(map[string]bool)
 	for _, ignoredKey := range ignoreChanges {
-		for k := range diff.CopyAttributes() {
+		for k := range attrs {
 			if ignoredKey == "*" || 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.CopyAttributes() {
+	// If the resource was being destroyed, check to see if we can ignore the
+	// reason for it being destroyed.
+	if diff.GetDestroy() {
+		for k, v := range attrs {
+			if k == "id" {
+				// id will always be changed if we intended to replace this instance
+				continue
+			}
 			if v.Empty() || v.NewComputed {
+				continue
+			}
+
+			// If any RequiresNew attribute isn't ignored, we need to keep the diff
+			// as-is to be able to replace the resource.
+			if v.RequiresNew && !ignorableAttrKeys[k] {
+				return nil
+			}
+		}
+
+		// Now that we know that we aren't replacing the instance, we can filter
+		// out all the empty and computed attributes. There may be a bunch of
+		// extraneous attribute diffs for the other non-requires-new attributes
+		// going from "" -> "configval" or "" -> "<computed>".
+		// We must make sure any flatmapped containers are filterred (or not) as a
+		// whole.
+		containers := groupContainers(diff)
+		keep := map[string]bool{}
+		for _, v := range containers {
+			if v.keepDiff() {
+				// At least one key has changes, so list all the sibling keys
+				// to keep in the diff.
+				for k := range v {
+					keep[k] = true
+				}
+			}
+		}
+
+		for k, v := range attrs {
+			if (v.Empty() || v.NewComputed) && !keep[k] {
 				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.CopyAttributes() {
-			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"
@@ -254,7 +265,6 @@ func (n *EvalDiff) processIgnoreChanges(diff *InstanceDiff) error {
 		"because after ignore_changes, this diff no longer requires replacement")
 	diff.DelAttribute("id")
 	diff.SetDestroy(false)
-	}
 
 	// If we didn't hit any of our early exit conditions, we can filter the diff.
 	for k := range ignorableAttrKeys {
@@ -266,6 +276,46 @@ func (n *EvalDiff) processIgnoreChanges(diff *InstanceDiff) error {
 	return nil
 }
 
+// a group of key-*ResourceAttrDiff pairs from the same flatmapped container
+type flatAttrDiff map[string]*ResourceAttrDiff
+
+// we need to keep all keys if any of them have a diff
+func (f flatAttrDiff) keepDiff() bool {
+	for _, v := range f {
+		if !v.Empty() && !v.NewComputed {
+			return true
+		}
+	}
+	return false
+}
+
+// sets, lists and maps need to be compared for diff inclusion as a whole, so
+// group the flatmapped keys together for easier comparison.
+func groupContainers(d *InstanceDiff) map[string]flatAttrDiff {
+	isIndex := multiVal.MatchString
+	containers := map[string]flatAttrDiff{}
+	attrs := d.CopyAttributes()
+	// we need to loop once to find the index key
+	for k := range attrs {
+		if isIndex(k) {
+			// add the key, always including the final dot to fully qualify it
+			containers[k[:len(k)-1]] = flatAttrDiff{}
+		}
+	}
+
+	// loop again to find all the sub keys
+	for prefix, values := range containers {
+		for k, attrDiff := range attrs {
+			// we include the index value as well, since it could be part of the diff
+			if strings.HasPrefix(k, prefix) {
+				values[k] = attrDiff
+			}
+		}
+	}
+
+	return containers
+}
+
 // EvalDiffDestroy is an EvalNode implementation that returns a plain
 // destroy diff.
 type EvalDiffDestroy struct {

terraform/terraform_test.go

@@ -1500,7 +1500,7 @@ DIFF:
 
 DESTROY/CREATE: aws_instance.foo
   type: "" => "aws_instance"
-  vars: "" => "foo"
+  vars: "foo" => "foo"
 
 STATE:
 
@@ -1570,6 +1570,17 @@ aws_instance.foo:
   ami = ami-abcd1234
 `
 
+const testTFPlanDiffIgnoreChangesWithFlatmaps = `
+UPDATE: aws_instance.foo
+  lst.#:   "1" => "2"
+  lst.0:   "j" => "j"
+  lst.1:   "" => "k"
+  set.#:   "1" => "1"
+  set.0.a: "1" => "1"
+  set.0.b: "" => "2"
+  type:    "" => "aws_instance"
+`
+
 const testTerraformPlanIgnoreChangesWildcardStr = `
 DIFF:
 

terraform/test-fixtures/plan-ignore-changes-with-flatmaps/main.tf

@@ -0,0 +1,16 @@
+resource "aws_instance" "foo" {
+  id = "bar"
+  user_data = "x"
+  require_new = "yes"
+
+  set = {
+	  a = "1"
+	  b = "2"
+  }
+
+  lst = ["j", "k"]
+
+  lifecycle {
+    ignore_changes = ["require_new"]
+  }
+}