From a5c1bf1b3626aeecc5058ac09ff3173f065ff588 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 16 Jun 2016 18:23:26 -0400 Subject: [PATCH 1/3] Don't check any parts of a computed hash in Same When checking for "same" values in a computed hash, not only might some of the values differ between versions changing the hash, but there may be fields not included at all in the original map, and different overall counts. Instead of trying to match individual set fields with different hashes, remove any hashed key longer than the computed key with the same base name. --- terraform/diff.go | 27 +++++++++++---------------- 1 file changed, 11 insertions(+), 16 deletions(-) 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 } } } From a3e2c992198b08301a9816f5ca9cdee102eb3d35 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 15 Jun 2016 15:37:45 -0400 Subject: [PATCH 2/3] Add a failing acceptance test from GH-2027 --- .../aws/resource_aws_security_group_test.go | 65 +++++++++++++++++++ 1 file changed, 65 insertions(+) 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}"] + } +} +` From f4ef16a84bf612960a54ad8b336611afcc06bcc6 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 16 Jun 2016 18:37:11 -0400 Subject: [PATCH 3/3] Add a core test for InstanceDiff.Same --- terraform/diff_test.go | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) 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.)