diff --git a/builtin/providers/aws/resource_aws_security_group_test.go b/builtin/providers/aws/resource_aws_security_group_test.go index 23bdb0622944..58555618232c 100644 --- a/builtin/providers/aws/resource_aws_security_group_test.go +++ b/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}"] + } +} +` diff --git a/terraform/diff.go b/terraform/diff.go index 8c26e16ff5ad..088c09c5a097 100644 --- a/terraform/diff.go +++ b/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 } } } diff --git a/terraform/diff_test.go b/terraform/diff_test.go index 926a093d40fe..d2bb2f81087a 100644 --- a/terraform/diff_test.go +++ b/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.)