Merge pull request #7205 from hashicorp/jbardin/GH-2027

core: don't check any parts of a computed set in InstanceDiff.Same

builtin/providers/aws/resource_aws_security_group_test.go

@@ -968,6 +968,24 @@ func testAccCheckAWSSecurityGroupExistsWithoutDefault(n string) resource.TestChe
 	}
 }
 
+func TestAccAWSSecurityGroup_failWithDiffMismatch(t *testing.T) {
+	var group ec2.SecurityGroup
+
+	resource.Test(t, resource.TestCase{
+		PreCheck:     func() { testAccPreCheck(t) },
+		Providers:    testAccProviders,
+		CheckDestroy: testAccCheckAWSSecurityGroupDestroy,
+		Steps: []resource.TestStep{
+			resource.TestStep{
+				Config: testAccAWSSecurityGroupConfig_failWithDiffMismatch,
+				Check: resource.ComposeTestCheckFunc(
+					testAccCheckAWSSecurityGroupExists("aws_security_group.nat", &group),
+				),
+			},
+		},
+	})
+}
+
 const testAccAWSSecurityGroupConfig = `
 resource "aws_vpc" "foo" {
   cidr_block = "10.1.0.0/16"
@@ -1529,3 +1547,50 @@ resource "aws_security_group" "web" {
   }
 }
 `
+
+// fails to apply in one pass with the error "diffs didn't match during apply"
+// GH-2027
+const testAccAWSSecurityGroupConfig_failWithDiffMismatch = `
+resource "aws_vpc" "main" {
+  cidr_block = "10.0.0.0/16"
+
+  tags {
+    Name = "tf-test"
+  }
+}
+
+resource "aws_security_group" "ssh_base" {
+  name   = "test-ssh-base"
+  vpc_id = "${aws_vpc.main.id}"
+}
+
+resource "aws_security_group" "jump" {
+  name   = "test-jump"
+  vpc_id = "${aws_vpc.main.id}"
+}
+
+resource "aws_security_group" "provision" {
+  name   = "test-provision"
+  vpc_id = "${aws_vpc.main.id}"
+}
+
+resource "aws_security_group" "nat" {
+  vpc_id      = "${aws_vpc.main.id}"
+  name        = "nat"
+  description = "For nat servers "
+
+  ingress {
+    from_port       = 22
+    to_port         = 22
+    protocol        = "tcp"
+    security_groups = ["${aws_security_group.jump.id}"]
+  }
+
+  ingress {
+    from_port       = 22
+    to_port         = 22
+    protocol        = "tcp"
+    security_groups = ["${aws_security_group.provision.id}"]
+  }
+}
+`

terraform/diff.go

@@ -443,35 +443,30 @@ func (d *InstanceDiff) Same(d2 *InstanceDiff) (bool, string) {
 			// values. So check if there is an approximate hash in the key
 			// and if so, try to match the key.
 			if strings.Contains(k, "~") {
-				// TODO (SvH): There should be a better way to do this...
 				parts := strings.Split(k, ".")
-				parts2 := strings.Split(k, ".")
+				parts2 := append([]string(nil), parts...)
+
 				re := regexp.MustCompile(`^~\d+$`)
 				for i, part := range parts {
 					if re.MatchString(part) {
+						// we're going to consider this the base of a
+						// computed hash, and remove all longer matching fields
+						ok = true
+
 						parts2[i] = `\d+`
+						parts2 = parts2[:i+1]
+						break
 					}
 				}
-				re, err := regexp.Compile("^" + strings.Join(parts2, `\.`) + "$")
+
+				re, err := regexp.Compile("^" + strings.Join(parts2, `\.`))
 				if err != nil {
 					return false, fmt.Sprintf("regexp failed to compile; err: %#v", err)
 				}
+
 				for k2, _ := range checkNew {
 					if re.MatchString(k2) {
 						delete(checkNew, k2)
-
-						if diffOld.NewComputed && strings.HasSuffix(k, ".#") {
-							// This is a computed list or set, so remove any keys with this
-							// prefix from the check list.
-							prefix := k2[:len(k2)-1]
-							for k2, _ := range checkNew {
-								if strings.HasPrefix(k2, prefix) {
-									delete(checkNew, k2)
-								}
-							}
-						}
-						ok = true
-						break
 					}
 				}
 			}

terraform/diff_test.go

@@ -525,6 +525,47 @@ func TestInstanceDiffSame(t *testing.T) {
 			"",
 		},
 
+		// Computed sets may not contain all fields in the original diff, and
+		// because multiple entries for the same set can compute to the same
+		// hash before the values are computed or interpolated, the overall
+		// count can change as well.
+		{
+			&InstanceDiff{
+				Attributes: map[string]*ResourceAttrDiff{
+					"foo.#": &ResourceAttrDiff{
+						Old: "0",
+						New: "1",
+					},
+					"foo.~35964334.bar": &ResourceAttrDiff{
+						Old: "",
+						New: "${var.foo}",
+					},
+				},
+			},
+			&InstanceDiff{
+				Attributes: map[string]*ResourceAttrDiff{
+					"foo.#": &ResourceAttrDiff{
+						Old: "0",
+						New: "2",
+					},
+					"foo.87654323.bar": &ResourceAttrDiff{
+						Old: "",
+						New: "12",
+					},
+					"foo.87654325.bar": &ResourceAttrDiff{
+						Old: "",
+						New: "12",
+					},
+					"foo.87654325.baz": &ResourceAttrDiff{
+						Old: "",
+						New: "12",
+					},
+				},
+			},
+			true,
+			"",
+		},
+
 		// In a DESTROY/CREATE scenario, the plan diff will be run against the
 		// state of the old instance, while the apply diff will be run against an
 		// empty state (because the state is cleared when the destroy runs.)