From fe32f7b189afefdb3f476e6bbe7c8ac5f3fa9712 Mon Sep 17 00:00:00 2001
From: Mitchell Hashimoto <mitchell.hashimoto@gmail.com>
Date: Mon, 6 Feb 2017 17:40:44 -0800
Subject: [PATCH] terraform: ignore RequiresNew for collection removal in
 diff.Same

Fixes #11349

I tracked this bug back to the early 0.7 days so this has been around a
really long time. I wanted to confirm that this wasn't introduced by any
new graph changes and it appears to predate all of that. I couldn't find
a single 0.7.x release where this worked, and I didn't want to go back
to 0.6.x since it was pre-vendoring.

The test case shows the logic the best, but the basic idea is: for
collections that go to zero elements, the "RequiresNew" sameness check
should be ignored, since the new diff can choose to not have that at all
in the diff.
---
 terraform/diff.go      | 40 ++++++++++++++++++++++++-
 terraform/diff_test.go | 66 ++++++++++++++++++++++++++++++++++++------
 2 files changed, 96 insertions(+), 10 deletions(-)

diff --git a/terraform/diff.go b/terraform/diff.go
index d50ec5b10..5cf1b78ce 100644
--- a/terraform/diff.go
+++ b/terraform/diff.go
@@ -644,7 +644,45 @@ func (d *InstanceDiff) Same(d2 *InstanceDiff) (bool, string) {
 	newNew := d2.RequiresNew()
 	if oldNew && !newNew {
 		oldNew = false
-		for _, rd := range d.Attributes {
+
+		// This section builds a list of ignorable attributes for requiresNew
+		// by removing off any elements of collections going to zero elements.
+		// For collections going to zero, they may not exist at all in the
+		// new diff (and hence RequiresNew == false).
+		ignoreAttrs := make(map[string]struct{})
+		for k, diffOld := range d.Attributes {
+			if !strings.HasSuffix(k, ".%") && !strings.HasSuffix(k, ".#") {
+				continue
+			}
+
+			// This case is in here as a protection measure. The bug that this
+			// code originally fixed (GH-11349) didn't have to deal with computed
+			// so I'm not 100% sure what the correct behavior is. Best to leave
+			// the old behavior.
+			if diffOld.NewComputed {
+				continue
+			}
+
+			// We're looking for the case a map goes to exactly 0.
+			if diffOld.New != "0" {
+				continue
+			}
+
+			// Found it! Ignore all of these. The prefix here is stripping
+			// off the "%" so it is just "k."
+			prefix := k[:len(k)-1]
+			for k2, _ := range d.Attributes {
+				if strings.HasPrefix(k2, prefix) {
+					ignoreAttrs[k2] = struct{}{}
+				}
+			}
+		}
+
+		for k, rd := range d.Attributes {
+			if _, ok := ignoreAttrs[k]; ok {
+				continue
+			}
+
 			// If the field is requires new and NOT computed, then what
 			// we have is a diff mismatch for sure. We set that the old
 			// diff does REQUIRE a ForceNew.
diff --git a/terraform/diff_test.go b/terraform/diff_test.go
index 8f29ad08d..a74cbbecd 100644
--- a/terraform/diff_test.go
+++ b/terraform/diff_test.go
@@ -1,6 +1,7 @@
 package terraform
 
 import (
+	"fmt"
 	"reflect"
 	"strings"
 	"testing"
@@ -1085,18 +1086,65 @@ func TestInstanceDiffSame(t *testing.T) {
 			true,
 			"",
 		},
+
+		// When removing all collection items, the diff is allowed to contain
+		// nothing when re-creating the resource. This should be the "Same"
+		// since we said we were going from 1 to 0.
+		{
+			&InstanceDiff{
+				Attributes: map[string]*ResourceAttrDiff{
+					"foo.%": &ResourceAttrDiff{
+						Old:         "1",
+						New:         "0",
+						RequiresNew: true,
+					},
+					"foo.bar": &ResourceAttrDiff{
+						Old:         "baz",
+						New:         "",
+						NewRemoved:  true,
+						RequiresNew: true,
+					},
+				},
+			},
+			&InstanceDiff{},
+			true,
+			"",
+		},
+
+		{
+			&InstanceDiff{
+				Attributes: map[string]*ResourceAttrDiff{
+					"foo.#": &ResourceAttrDiff{
+						Old:         "1",
+						New:         "0",
+						RequiresNew: true,
+					},
+					"foo.0": &ResourceAttrDiff{
+						Old:         "baz",
+						New:         "",
+						NewRemoved:  true,
+						RequiresNew: true,
+					},
+				},
+			},
+			&InstanceDiff{},
+			true,
+			"",
+		},
 	}
 
 	for i, tc := range cases {
-		same, reason := tc.One.Same(tc.Two)
-		if same != tc.Same {
-			t.Fatalf("%d: expected same: %t, got %t (%s)\n\n one: %#v\n\ntwo: %#v",
-				i, tc.Same, same, reason, tc.One, tc.Two)
-		}
-		if reason != tc.Reason {
-			t.Fatalf(
-				"%d: bad reason\n\nexpected: %#v\n\ngot: %#v", i, tc.Reason, reason)
-		}
+		t.Run(fmt.Sprintf("%d", i), func(t *testing.T) {
+			same, reason := tc.One.Same(tc.Two)
+			if same != tc.Same {
+				t.Fatalf("%d: expected same: %t, got %t (%s)\n\n one: %#v\n\ntwo: %#v",
+					i, tc.Same, same, reason, tc.One, tc.Two)
+			}
+			if reason != tc.Reason {
+				t.Fatalf(
+					"%d: bad reason\n\nexpected: %#v\n\ngot: %#v", i, tc.Reason, reason)
+			}
+		})
 	}
 }