From 4af2c5f5dd4f1601b782d2a140943ddbf59d53f9 Mon Sep 17 00:00:00 2001
From: Paul Hinze <phinze@phinze.com>
Date: Tue, 19 Jan 2016 15:28:48 -0600
Subject: [PATCH] core: fix diff mismatch when RequiresNew field and list both
 change

fixes #1752

Includes AccTest reproducing example from the issue as well as a bunch
of explanatory comments in the tests and impls.
---
 .../aws/resource_aws_instance_test.go         | 85 +++++++++++++++++++
 terraform/diff.go                             |  7 ++
 terraform/diff_test.go                        | 37 ++++++++
 3 files changed, 129 insertions(+)

diff --git a/builtin/providers/aws/resource_aws_instance_test.go b/builtin/providers/aws/resource_aws_instance_test.go
index 9b1e3b8e7..11df73a8c 100644
--- a/builtin/providers/aws/resource_aws_instance_test.go
+++ b/builtin/providers/aws/resource_aws_instance_test.go
@@ -513,6 +513,41 @@ func TestAccAWSInstance_rootBlockDeviceMismatch(t *testing.T) {
 	})
 }
 
+// This test reproduces the bug here:
+//   https://github.com/hashicorp/terraform/issues/1752
+//
+// I wish there were a way to exercise resources built with helper.Schema in a
+// unit context, in which case this test could be moved there, but for now this
+// will cover the bugfix.
+//
+// The following triggers "diffs didn't match during apply" without the fix in to
+// set NewRemoved on the .# field when it changes to 0.
+func TestAccAWSInstance_forceNewAndTagsDrift(t *testing.T) {
+	var v ec2.Instance
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckInstanceDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccInstanceConfigForceNewAndTagsDrift,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckInstanceExists("aws_instance.foo", &v),
+					driftTags(&v),
+				),
+				ExpectNonEmptyPlan: true,
+			},
+			resource.TestStep{
+				Config: testAccInstanceConfigForceNewAndTagsDrift_Update,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckInstanceExists("aws_instance.foo", &v),
+				),
+			},
+		},
+	})
+}
+
 func testAccCheckInstanceDestroy(s *terraform.State) error {
 	return testAccCheckInstanceDestroyWithProvider(s, testAccProvider)
 }
@@ -622,6 +657,22 @@ func TestInstanceTenancySchema(t *testing.T) {
 	}
 }
 
+func driftTags(instance *ec2.Instance) resource.TestCheckFunc {
+	return func(s *terraform.State) error {
+		conn := testAccProvider.Meta().(*AWSClient).ec2conn
+		_, err := conn.CreateTags(&ec2.CreateTagsInput{
+			Resources: []*string{instance.InstanceId},
+			Tags: []*ec2.Tag{
+				&ec2.Tag{
+					Key:   aws.String("Drift"),
+					Value: aws.String("Happens"),
+				},
+			},
+		})
+		return err
+	}
+}
+
 const testAccInstanceConfig_pre = `
 resource "aws_security_group" "tf_test_foo" {
 	name = "tf_test_foo"
@@ -988,3 +1039,37 @@ resource "aws_instance" "foo" {
 	}
 }
 `
+
+const testAccInstanceConfigForceNewAndTagsDrift = `
+resource "aws_vpc" "foo" {
+	cidr_block = "10.1.0.0/16"
+}
+
+resource "aws_subnet" "foo" {
+	cidr_block = "10.1.1.0/24"
+	vpc_id = "${aws_vpc.foo.id}"
+}
+
+resource "aws_instance" "foo" {
+	ami = "ami-22b9a343"
+	instance_type = "t2.nano"
+	subnet_id = "${aws_subnet.foo.id}"
+}
+`
+
+const testAccInstanceConfigForceNewAndTagsDrift_Update = `
+resource "aws_vpc" "foo" {
+	cidr_block = "10.1.0.0/16"
+}
+
+resource "aws_subnet" "foo" {
+	cidr_block = "10.1.1.0/24"
+	vpc_id = "${aws_vpc.foo.id}"
+}
+
+resource "aws_instance" "foo" {
+	ami = "ami-22b9a343"
+	instance_type = "t2.micro"
+	subnet_id = "${aws_subnet.foo.id}"
+}
+`
diff --git a/terraform/diff.go b/terraform/diff.go
index 87ae84ccf..f1a41efb2 100644
--- a/terraform/diff.go
+++ b/terraform/diff.go
@@ -466,6 +466,13 @@ func (d *InstanceDiff) Same(d2 *InstanceDiff) (bool, string) {
 				ok = true
 			}
 
+			// Similarly, in a RequiresNew scenario, a list that shows up in the plan
+			// diff can disappear from the apply diff, which is calculated from an
+			// empty state.
+			if d.RequiresNew() && strings.HasSuffix(k, ".#") {
+				ok = true
+			}
+
 			if !ok {
 				return false, fmt.Sprintf("attribute mismatch: %s", k)
 			}
diff --git a/terraform/diff_test.go b/terraform/diff_test.go
index 4eeb8d387..6dbdd505e 100644
--- a/terraform/diff_test.go
+++ b/terraform/diff_test.go
@@ -553,6 +553,43 @@ func TestInstanceDiffSame(t *testing.T) {
 			true,
 			"",
 		},
+
+		// Another thing that can occur in DESTROY/CREATE scenarios is that list
+		// values that are going to zero have diffs that show up at plan time but
+		// are gone at apply time. The NewRemoved handling catches the fields and
+		// treats them as OK, but it also needs to treat the .# field itself as
+		// okay to be present in the old diff but not in the new one.
+		{
+			&InstanceDiff{
+				Attributes: map[string]*ResourceAttrDiff{
+					"reqnew": &ResourceAttrDiff{
+						Old:         "old",
+						New:         "new",
+						RequiresNew: true,
+					},
+					"somemap.#": &ResourceAttrDiff{
+						Old: "1",
+						New: "0",
+					},
+					"somemap.oldkey": &ResourceAttrDiff{
+						Old:        "long ago",
+						New:        "",
+						NewRemoved: true,
+					},
+				},
+			},
+			&InstanceDiff{
+				Attributes: map[string]*ResourceAttrDiff{
+					"reqnew": &ResourceAttrDiff{
+						Old:         "",
+						New:         "new",
+						RequiresNew: true,
+					},
+				},
+			},
+			true,
+			"",
+		},
 	}
 
 	for i, tc := range cases {