From 42ee519a316c759c74a7eef88a5ae2f823f3c3d5 Mon Sep 17 00:00:00 2001 From: clint shryock Date: Mon, 9 May 2016 16:48:16 -0500 Subject: [PATCH] provider/aws: Update Route53 Record to schema v1, normalizing name The `name` attribute will always be normalized to a FQDN, with a trailing "dot" at the end when returned from the API. We store the name as it's provided in the configuration, so "www" stays as "www" and "www.terraformtesting.io." stays as "www.terraformtesting.io.". The problem here is that if we use a full name as above, and the configuraiton does *not* include the trailing dot, the API will return a version that does, and we'll have a conflict. This is particularly bad when we have a lifecycle block with `create_before_destroy`; the record will get an update posted (which ends up being a no-op on AWS's side), but then we'll delete the same record immediately after, resulting in no record at all. This PR addresses that by trimming the trailing dot from the `name` when saving to state. We migrate existing state to match, to avoid false-positive diffs. --- .../aws/resource_aws_route53_record.go | 6 +- .../resource_aws_route53_record_migrate.go | 33 ++++++++++ ...esource_aws_route53_record_migrate_test.go | 59 +++++++++++++++++ .../aws/resource_aws_route53_record_test.go | 65 +++++++++++++++++++ 4 files changed, 162 insertions(+), 1 deletion(-) create mode 100644 builtin/providers/aws/resource_aws_route53_record_migrate.go create mode 100644 builtin/providers/aws/resource_aws_route53_record_migrate_test.go diff --git a/builtin/providers/aws/resource_aws_route53_record.go b/builtin/providers/aws/resource_aws_route53_record.go index cd8cbf7c81..8a94e1dce0 100644 --- a/builtin/providers/aws/resource_aws_route53_record.go +++ b/builtin/providers/aws/resource_aws_route53_record.go @@ -27,13 +27,16 @@ func resourceAwsRoute53Record() *schema.Resource { Update: resourceAwsRoute53RecordUpdate, Delete: resourceAwsRoute53RecordDelete, + SchemaVersion: 1, + MigrateState: resourceAwsRoute53RecordMigrateState, + Schema: map[string]*schema.Schema{ "name": &schema.Schema{ Type: schema.TypeString, Required: true, ForceNew: true, StateFunc: func(v interface{}) string { - value := v.(string) + value := strings.TrimSuffix(v.(string), ".") return strings.ToLower(value) }, }, @@ -330,6 +333,7 @@ func findRecord(d *schema.ResourceData, meta interface{}) (*route53.ResourceReco } return nil, err } + en := expandRecordName(d.Get("name").(string), *zoneRecord.HostedZone.Name) log.Printf("[DEBUG] Expanded record name: %s", en) d.Set("fqdn", en) diff --git a/builtin/providers/aws/resource_aws_route53_record_migrate.go b/builtin/providers/aws/resource_aws_route53_record_migrate.go new file mode 100644 index 0000000000..3e73b0c2a0 --- /dev/null +++ b/builtin/providers/aws/resource_aws_route53_record_migrate.go @@ -0,0 +1,33 @@ +package aws + +import ( + "fmt" + "log" + "strings" + + "github.com/hashicorp/terraform/terraform" +) + +func resourceAwsRoute53RecordMigrateState( + v int, is *terraform.InstanceState, meta interface{}) (*terraform.InstanceState, error) { + switch v { + case 0: + log.Println("[INFO] Found AWS Route53 Record State v0; migrating to v1") + return migrateRoute53RecordStateV0toV1(is) + default: + return is, fmt.Errorf("Unexpected schema version: %d", v) + } +} + +func migrateRoute53RecordStateV0toV1(is *terraform.InstanceState) (*terraform.InstanceState, error) { + if is.Empty() { + log.Println("[DEBUG] Empty InstanceState; nothing to migrate.") + return is, nil + } + + log.Printf("[DEBUG] Attributes before migration: %#v", is.Attributes) + newName := strings.TrimSuffix(is.Attributes["name"], ".") + is.Attributes["name"] = newName + log.Printf("[DEBUG] Attributes after migration: %#v, new name: %s", is.Attributes, newName) + return is, nil +} diff --git a/builtin/providers/aws/resource_aws_route53_record_migrate_test.go b/builtin/providers/aws/resource_aws_route53_record_migrate_test.go new file mode 100644 index 0000000000..69e1bd2579 --- /dev/null +++ b/builtin/providers/aws/resource_aws_route53_record_migrate_test.go @@ -0,0 +1,59 @@ +package aws + +import ( + "testing" + + "github.com/hashicorp/terraform/terraform" +) + +func TestAWSRoute53RecordMigrateState(t *testing.T) { + cases := map[string]struct { + StateVersion int + ID string + Attributes map[string]string + Expected string + Meta interface{} + }{ + "v0_0": { + StateVersion: 0, + ID: "some_id", + Attributes: map[string]string{ + "name": "www", + }, + Expected: "www", + }, + "v0_1": { + StateVersion: 0, + ID: "some_id", + Attributes: map[string]string{ + "name": "www.notdomain.com.", + }, + Expected: "www.notdomain.com", + }, + "v0_2": { + StateVersion: 0, + ID: "some_id", + Attributes: map[string]string{ + "name": "www.notdomain.com", + }, + Expected: "www.notdomain.com", + }, + } + + for tn, tc := range cases { + is := &terraform.InstanceState{ + ID: tc.ID, + Attributes: tc.Attributes, + } + is, err := resourceAwsRoute53RecordMigrateState( + tc.StateVersion, is, tc.Meta) + + if err != nil { + t.Fatalf("bad: %s, err: %#v", tn, err) + } + + if is.Attributes["name"] != tc.Expected { + t.Fatalf("bad Route 53 Migrate: %s\n\n expected: %s", is.Attributes["name"], tc.Expected) + } + } +} diff --git a/builtin/providers/aws/resource_aws_route53_record_test.go b/builtin/providers/aws/resource_aws_route53_record_test.go index 32acf9abfd..259cb3dce6 100644 --- a/builtin/providers/aws/resource_aws_route53_record_test.go +++ b/builtin/providers/aws/resource_aws_route53_record_test.go @@ -68,6 +68,36 @@ func TestAccAWSRoute53Record_basic(t *testing.T) { }) } +func TestAccAWSRoute53Record_basic_fqdn(t *testing.T) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + IDRefreshName: "aws_route53_record.default", + Providers: testAccProviders, + CheckDestroy: testAccCheckRoute53RecordDestroy, + Steps: []resource.TestStep{ + resource.TestStep{ + Config: testAccRoute53RecordConfig_fqdn, + Check: resource.ComposeTestCheckFunc( + testAccCheckRoute53RecordExists("aws_route53_record.default"), + ), + }, + + // Ensure that changing the name to include a trailing "dot" results in + // nothing happening, because the name is stripped of trailing dots on + // save. Otherwise, an update would occur and due to the + // create_before_destroy, the record would actually be destroyed, and a + // non-empty plan would appear, and the record will fail to exist in + // testAccCheckRoute53RecordExists + resource.TestStep{ + Config: testAccRoute53RecordConfig_fqdn_no_op, + Check: resource.ComposeTestCheckFunc( + testAccCheckRoute53RecordExists("aws_route53_record.default"), + ), + }, + }, + }) +} + func TestAccAWSRoute53Record_txtSupport(t *testing.T) { resource.Test(t, resource.TestCase{ PreCheck: func() { testAccPreCheck(t) }, @@ -416,6 +446,41 @@ resource "aws_route53_record" "default" { records = ["127.0.0.1", "127.0.0.27"] } ` +const testAccRoute53RecordConfig_fqdn = ` +resource "aws_route53_zone" "main" { + name = "notexample.com" +} + +resource "aws_route53_record" "default" { + zone_id = "${aws_route53_zone.main.zone_id}" + name = "www.NOTexamplE.com" + type = "A" + ttl = "30" + records = ["127.0.0.1", "127.0.0.27"] + + lifecycle { + create_before_destroy = true + } +} +` + +const testAccRoute53RecordConfig_fqdn_no_op = ` +resource "aws_route53_zone" "main" { + name = "notexample.com" +} + +resource "aws_route53_record" "default" { + zone_id = "${aws_route53_zone.main.zone_id}" + name = "www.NOTexamplE.com." + type = "A" + ttl = "30" + records = ["127.0.0.1", "127.0.0.27"] + + lifecycle { + create_before_destroy = true + } +} +` const testAccRoute53RecordNoConfig = ` resource "aws_route53_zone" "main" {