Correctly filter flatmapped values in diff

When transforming a diff from DestroyCreate to a simple Update,
ignore_changes can cause keys from flatmapped objects to be filtered
form the diff. We need to filter each flatmapped container as a whole to
ensure that unchanged keys aren't lost in the update.

terraform/context_plan_test.go

@@ -3098,7 +3098,8 @@ func TestContext2Plan_listOrder(t *testing.T) {
 
 // Make sure ignore-changes doesn't interfere with set/list/map diffs.
 // If a resource was being replaced by a RequiresNew attribute that gets
-// ignores, we need to filter out the diff properly.
+// 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")

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,72 +191,81 @@ 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"
-		// 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")
-		diff.DelAttribute("id")
-		diff.SetDestroy(false)
 	}
 
+	// 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")
+	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 {
 		log.Printf("[DEBUG] [EvalIgnoreChanges] %s - Ignoring diff attribute: %s",
@@ -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 {