From 2ee5ed62fe2c749b9b38954f45fe1f5591f62f88 Mon Sep 17 00:00:00 2001 From: Antoine Rouaze Date: Tue, 9 Jan 2018 11:31:37 -0500 Subject: [PATCH 1/3] Added option to avoid DNS record overwrite The current behavior of the aws_route53_record resource is to overwrite the record if one already exists. For example, we suppose that the notexample.com CNAME to foo.bar is added by hand via the CLI. If the new Terraform resource with the same name but with a different CNAME value is added, the record is going to be overridden. This might be a big problem: a record might be changed by mistake. So this PR proposes to add an option named allow_overwrite on the resource to protect overwrite. When the option is true the CREATE mode is used instead of UPSERT in the change batch. This means that if an added record already exists on route53, the application will fail. By default, the overwrite is permitted to avoid compatibility issue. --- aws/resource_aws_route53_record.go | 18 +++++++++++++++++- website/docs/r/route53_record.html.markdown | 1 + 2 files changed, 18 insertions(+), 1 deletion(-) diff --git a/aws/resource_aws_route53_record.go b/aws/resource_aws_route53_record.go index 719825ffec5..77a3bf96ca2 100644 --- a/aws/resource_aws_route53_record.go +++ b/aws/resource_aws_route53_record.go @@ -232,6 +232,12 @@ func resourceAwsRoute53Record() *schema.Resource { Optional: true, Set: schema.HashString, }, + + "allow_overwrite": { + Type: schema.TypeBool, + Optional: true, + Default: true, + }, }, } } @@ -379,6 +385,16 @@ func resourceAwsRoute53RecordCreate(d *schema.ResourceData, meta interface{}) er return err } + // Protect existing DNS records which might be managed in another way + // Use UPSERT only if the overwrite flag is true or if the current action is an update + // Else CREATE is used and fail if the same record exists + var action string + if d.Get("allow_overwrite").(bool) || !d.IsNewResource() { + action = "UPSERT" + } else { + action = "CREATE" + } + // Create the new records. We abuse StateChangeConf for this to // retry for us since Route53 sometimes returns errors about another // operation happening at the same time. @@ -386,7 +402,7 @@ func resourceAwsRoute53RecordCreate(d *schema.ResourceData, meta interface{}) er Comment: aws.String("Managed by Terraform"), Changes: []*route53.Change{ { - Action: aws.String("UPSERT"), + Action: aws.String(action), ResourceRecordSet: rec, }, }, diff --git a/website/docs/r/route53_record.html.markdown b/website/docs/r/route53_record.html.markdown index 46406071fa4..5e4aba8bac5 100644 --- a/website/docs/r/route53_record.html.markdown +++ b/website/docs/r/route53_record.html.markdown @@ -108,6 +108,7 @@ The following arguments are supported: * `latency_routing_policy` - (Optional) A block indicating a routing policy based on the latency between the requestor and an AWS region. Conflicts with any other routing policy. Documented below. * `weighted_routing_policy` - (Optional) A block indicating a weighted routing policy. Conflicts with any other routing policy. Documented below. * `multivalue_answer_routing_policy` - (Optional) A block indicating a multivalue answer routing policy. Conflicts with any other routing policy. +* `allow_overwrite` - (Optional) Allow overwrite existing records on route53. True by default. Exactly one of `records` or `alias` must be specified: this determines whether it's an alias record. From d20082494d6795323958f61038afec58362917e7 Mon Sep 17 00:00:00 2001 From: Antoine Rouaze Date: Thu, 15 Feb 2018 11:28:18 -0500 Subject: [PATCH 2/3] Added Acc test --- aws/resource_aws_route53_record_test.go | 102 ++++++++++++++++++++++++ 1 file changed, 102 insertions(+) diff --git a/aws/resource_aws_route53_record_test.go b/aws/resource_aws_route53_record_test.go index 996f31bd081..57d15ba60d9 100644 --- a/aws/resource_aws_route53_record_test.go +++ b/aws/resource_aws_route53_record_test.go @@ -12,6 +12,9 @@ import ( "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/service/route53" + "regexp" + "strconv" + "time" ) func TestCleanRecordName(t *testing.T) { @@ -488,6 +491,88 @@ func TestAccAWSRoute53Record_multivalue_answer_basic(t *testing.T) { }) } +func TestAccAWSRoute53Record_allowOverwrite(t *testing.T) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { testAccPreCheck(t) }, + IDRefreshName: "aws_route53_record.default", + Providers: testAccProviders, + CheckDestroy: testAccAWSRoute53RecordDestroy_allowOverwrite, + PreventPostDestroyRefresh: true, + Steps: []resource.TestStep{ + { + PreConfig: func() { testAccRoute53RecordCreate_allowOverwrite(t) }, + Config: testAccRoute53RecordConfig_allowOverwrite(false), + ExpectError: regexp.MustCompile("Tried to create resource record set \\[name='www.notexample.com.', type='A'] but it already exists"), + }, + { + Config: testAccRoute53RecordConfig_allowOverwrite(true), + Check: resource.ComposeTestCheckFunc(testAccCheckRoute53RecordExists("aws_route53_record.default")), + }, + }, + }) +} + +func testAccRoute53RecordCreate_allowOverwrite(t *testing.T) { + conn := testAccProvider.Meta().(*AWSClient).r53conn + zone, err := conn.CreateHostedZone(&route53.CreateHostedZoneInput{ + Name: aws.String("notexample.com"), + CallerReference: aws.String(strconv.FormatInt(time.Now().Unix(), 10)), + }) + if err == nil { + _, err = conn.ChangeResourceRecordSets(&route53.ChangeResourceRecordSetsInput{ + ChangeBatch: &route53.ChangeBatch{ + Changes: []*route53.Change{ + { + Action: aws.String("CREATE"), + ResourceRecordSet: &route53.ResourceRecordSet{ + Name: aws.String("www.notexample.com"), + Type: aws.String("A"), + TTL: aws.Int64(30), + ResourceRecords: []*route53.ResourceRecord{ + { + Value: aws.String("127.0.0.1"), + }, + }, + }, + }, + }, + }, + HostedZoneId: zone.HostedZone.Id, + }) + if err != nil { + t.Error(err) + } + } else { + t.Error(err) + } +} + +// Delete the hosted zone created manually +func testAccAWSRoute53RecordDestroy_allowOverwrite(s *terraform.State) error { + err := testAccCheckRoute53RecordDestroy(s) + if err == nil { + conn := testAccProvider.Meta().(*AWSClient).r53conn + hostedZone, ok := s.RootModule().Resources["data.aws_route53_zone.main"] + if !ok { + return fmt.Errorf("unable to find the hosted zone from the state") + } + zoneId := hostedZone.Primary.Attributes["id"] + _, err = conn.DeleteHostedZone(&route53.DeleteHostedZoneInput{ + Id: aws.String(zoneId), + }) + if err != nil { + if awsErr, ok := err.(awserr.Error); ok { + // if the zone is not found then everything is destroyed + if awsErr.Code() == "NoSuchHostedZone" { + return nil + } + } + return err + } + } + return nil +} + func testAccCheckRoute53RecordDestroy(s *terraform.State) error { conn := testAccProvider.Meta().(*AWSClient).r53conn for _, rs := range s.RootModule().Resources { @@ -573,6 +658,23 @@ func testAccCheckRoute53RecordExists(n string) resource.TestCheckFunc { } } +func testAccRoute53RecordConfig_allowOverwrite(allowOverwrite bool) string { + return fmt.Sprintf(` +data "aws_route53_zone" "main" { + name = "notexample.com." +} + +resource "aws_route53_record" "default" { + allow_overwrite = %v + zone_id = "${data.aws_route53_zone.main.zone_id}" + name = "www.notexample.com" + type = "A" + ttl = "30" + records = ["127.0.0.1"] +} +`, allowOverwrite) +} + const testAccRoute53RecordConfig = ` resource "aws_route53_zone" "main" { name = "notexample.com" From 87c1c68f8105813bfee871ff2e0b389db764ae09 Mon Sep 17 00:00:00 2001 From: Brian Flad Date: Thu, 15 Feb 2018 14:26:36 -0500 Subject: [PATCH 3/3] test/resource/aws_route53_record: Ensure allowOverwrite works when running parallelized testing --- aws/resource_aws_route53_record_test.go | 91 ++++----------------- website/docs/r/route53_record.html.markdown | 2 +- 2 files changed, 19 insertions(+), 74 deletions(-) diff --git a/aws/resource_aws_route53_record_test.go b/aws/resource_aws_route53_record_test.go index 57d15ba60d9..1e0d87d3a4d 100644 --- a/aws/resource_aws_route53_record_test.go +++ b/aws/resource_aws_route53_record_test.go @@ -9,12 +9,11 @@ import ( "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/terraform" + "regexp" + "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/service/route53" - "regexp" - "strconv" - "time" ) func TestCleanRecordName(t *testing.T) { @@ -493,86 +492,22 @@ func TestAccAWSRoute53Record_multivalue_answer_basic(t *testing.T) { func TestAccAWSRoute53Record_allowOverwrite(t *testing.T) { resource.Test(t, resource.TestCase{ - PreCheck: func() { testAccPreCheck(t) }, - IDRefreshName: "aws_route53_record.default", - Providers: testAccProviders, - CheckDestroy: testAccAWSRoute53RecordDestroy_allowOverwrite, - PreventPostDestroyRefresh: true, + PreCheck: func() { testAccPreCheck(t) }, + Providers: testAccProviders, + CheckDestroy: testAccCheckRoute53RecordDestroy, Steps: []resource.TestStep{ { - PreConfig: func() { testAccRoute53RecordCreate_allowOverwrite(t) }, Config: testAccRoute53RecordConfig_allowOverwrite(false), ExpectError: regexp.MustCompile("Tried to create resource record set \\[name='www.notexample.com.', type='A'] but it already exists"), }, { Config: testAccRoute53RecordConfig_allowOverwrite(true), - Check: resource.ComposeTestCheckFunc(testAccCheckRoute53RecordExists("aws_route53_record.default")), + Check: resource.ComposeTestCheckFunc(testAccCheckRoute53RecordExists("aws_route53_record.overwriting")), }, }, }) } -func testAccRoute53RecordCreate_allowOverwrite(t *testing.T) { - conn := testAccProvider.Meta().(*AWSClient).r53conn - zone, err := conn.CreateHostedZone(&route53.CreateHostedZoneInput{ - Name: aws.String("notexample.com"), - CallerReference: aws.String(strconv.FormatInt(time.Now().Unix(), 10)), - }) - if err == nil { - _, err = conn.ChangeResourceRecordSets(&route53.ChangeResourceRecordSetsInput{ - ChangeBatch: &route53.ChangeBatch{ - Changes: []*route53.Change{ - { - Action: aws.String("CREATE"), - ResourceRecordSet: &route53.ResourceRecordSet{ - Name: aws.String("www.notexample.com"), - Type: aws.String("A"), - TTL: aws.Int64(30), - ResourceRecords: []*route53.ResourceRecord{ - { - Value: aws.String("127.0.0.1"), - }, - }, - }, - }, - }, - }, - HostedZoneId: zone.HostedZone.Id, - }) - if err != nil { - t.Error(err) - } - } else { - t.Error(err) - } -} - -// Delete the hosted zone created manually -func testAccAWSRoute53RecordDestroy_allowOverwrite(s *terraform.State) error { - err := testAccCheckRoute53RecordDestroy(s) - if err == nil { - conn := testAccProvider.Meta().(*AWSClient).r53conn - hostedZone, ok := s.RootModule().Resources["data.aws_route53_zone.main"] - if !ok { - return fmt.Errorf("unable to find the hosted zone from the state") - } - zoneId := hostedZone.Primary.Attributes["id"] - _, err = conn.DeleteHostedZone(&route53.DeleteHostedZoneInput{ - Id: aws.String(zoneId), - }) - if err != nil { - if awsErr, ok := err.(awserr.Error); ok { - // if the zone is not found then everything is destroyed - if awsErr.Code() == "NoSuchHostedZone" { - return nil - } - } - return err - } - } - return nil -} - func testAccCheckRoute53RecordDestroy(s *terraform.State) error { conn := testAccProvider.Meta().(*AWSClient).r53conn for _, rs := range s.RootModule().Resources { @@ -660,13 +595,23 @@ func testAccCheckRoute53RecordExists(n string) resource.TestCheckFunc { func testAccRoute53RecordConfig_allowOverwrite(allowOverwrite bool) string { return fmt.Sprintf(` -data "aws_route53_zone" "main" { +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"] +} + +resource "aws_route53_record" "overwriting" { + depends_on = ["aws_route53_record.default"] + allow_overwrite = %v - zone_id = "${data.aws_route53_zone.main.zone_id}" + zone_id = "${aws_route53_zone.main.zone_id}" name = "www.notexample.com" type = "A" ttl = "30" diff --git a/website/docs/r/route53_record.html.markdown b/website/docs/r/route53_record.html.markdown index 5e4aba8bac5..cb2a6e40b52 100644 --- a/website/docs/r/route53_record.html.markdown +++ b/website/docs/r/route53_record.html.markdown @@ -108,7 +108,7 @@ The following arguments are supported: * `latency_routing_policy` - (Optional) A block indicating a routing policy based on the latency between the requestor and an AWS region. Conflicts with any other routing policy. Documented below. * `weighted_routing_policy` - (Optional) A block indicating a weighted routing policy. Conflicts with any other routing policy. Documented below. * `multivalue_answer_routing_policy` - (Optional) A block indicating a multivalue answer routing policy. Conflicts with any other routing policy. -* `allow_overwrite` - (Optional) Allow overwrite existing records on route53. True by default. +* `allow_overwrite` - (Optional) Allow creation of this record in Terraform to overwrite an existing record, if any. This does not prevent other resources within Terraform or manual Route53 changes from overwriting this record. `true` by default. Exactly one of `records` or `alias` must be specified: this determines whether it's an alias record.