Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delete old Route53 record when updating name #11335



Copy link

@jbergknoff-rival jbergknoff-rival commented Dec 17, 2019

If one updates the name of a Route53 record, it will show up in the plan as if the resource will be destroyed and then a new resource created in its place, but it actually leaves the old resource alone. This makes it difficult to refactor Route53 record resources if there will be any name collisions, because the old records can violate uniqueness constraints.

This PR adds a test which fails on the master branch, demonstrating the buggy behavior, and it updates the R53 findRecord function to handle this case correctly and make the test pass.

Community Note

  • Please vote on this pull request by adding a 馃憤 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for pull request followers and do not help prioritize the request

Closes #9024

Release note for CHANGELOG:

aws_route53_record: properly delete old record when updating name

Output from acceptance testing:

$ AWS_DEFAULT_REGION=us-east-1 AWS_ACCESS_KEY_ID=... AWS_SECRET_ACCESS_KEY=... make testacc TEST=./aws TESTARGS='-run=TestAccAWSRoute53Record_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./aws -v -count 1 -parallel 20 -run=TestAccAWSRoute53Record_ -timeout 120m
=== RUN   TestAccAWSRoute53Record_basic
=== PAUSE TestAccAWSRoute53Record_basic
=== RUN   TestAccAWSRoute53Record_underscored
=== PAUSE TestAccAWSRoute53Record_underscored
=== RUN   TestAccAWSRoute53Record_disappears
=== PAUSE TestAccAWSRoute53Record_disappears
=== RUN   TestAccAWSRoute53Record_disappears_MultipleRecords
=== PAUSE TestAccAWSRoute53Record_disappears_MultipleRecords
=== RUN   TestAccAWSRoute53Record_basic_fqdn
=== PAUSE TestAccAWSRoute53Record_basic_fqdn
=== RUN   TestAccAWSRoute53Record_txtSupport
=== PAUSE TestAccAWSRoute53Record_txtSupport
=== RUN   TestAccAWSRoute53Record_spfSupport
=== PAUSE TestAccAWSRoute53Record_spfSupport
=== RUN   TestAccAWSRoute53Record_caaSupport
=== PAUSE TestAccAWSRoute53Record_caaSupport
=== RUN   TestAccAWSRoute53Record_generatesSuffix
=== PAUSE TestAccAWSRoute53Record_generatesSuffix
=== RUN   TestAccAWSRoute53Record_wildcard
=== PAUSE TestAccAWSRoute53Record_wildcard
=== RUN   TestAccAWSRoute53Record_failover
=== PAUSE TestAccAWSRoute53Record_failover
=== RUN   TestAccAWSRoute53Record_weighted_basic
=== PAUSE TestAccAWSRoute53Record_weighted_basic
=== RUN   TestAccAWSRoute53Record_weighted_to_simple_basic
=== PAUSE TestAccAWSRoute53Record_weighted_to_simple_basic
=== RUN   TestAccAWSRoute53Record_Alias_Elb
=== PAUSE TestAccAWSRoute53Record_Alias_Elb
=== RUN   TestAccAWSRoute53Record_Alias_S3
=== PAUSE TestAccAWSRoute53Record_Alias_S3
=== RUN   TestAccAWSRoute53Record_Alias_VpcEndpoint
=== PAUSE TestAccAWSRoute53Record_Alias_VpcEndpoint
=== RUN   TestAccAWSRoute53Record_Alias_Uppercase
=== PAUSE TestAccAWSRoute53Record_Alias_Uppercase
=== RUN   TestAccAWSRoute53Record_weighted_alias
=== PAUSE TestAccAWSRoute53Record_weighted_alias
=== RUN   TestAccAWSRoute53Record_geolocation_basic
=== PAUSE TestAccAWSRoute53Record_geolocation_basic
=== RUN   TestAccAWSRoute53Record_latency_basic
=== PAUSE TestAccAWSRoute53Record_latency_basic
=== RUN   TestAccAWSRoute53Record_TypeChange
=== PAUSE TestAccAWSRoute53Record_TypeChange
=== RUN   TestAccAWSRoute53Record_NameChange
=== PAUSE TestAccAWSRoute53Record_NameChange
=== RUN   TestAccAWSRoute53Record_SetIdentifierChange
=== PAUSE TestAccAWSRoute53Record_SetIdentifierChange
=== RUN   TestAccAWSRoute53Record_AliasChange
=== PAUSE TestAccAWSRoute53Record_AliasChange
=== RUN   TestAccAWSRoute53Record_empty
=== PAUSE TestAccAWSRoute53Record_empty
=== RUN   TestAccAWSRoute53Record_longTXTrecord
=== PAUSE TestAccAWSRoute53Record_longTXTrecord
=== RUN   TestAccAWSRoute53Record_multivalue_answer_basic
=== PAUSE TestAccAWSRoute53Record_multivalue_answer_basic
=== RUN   TestAccAWSRoute53Record_allowOverwrite
=== PAUSE TestAccAWSRoute53Record_allowOverwrite
=== CONT  TestAccAWSRoute53Record_basic
=== CONT  TestAccAWSRoute53Record_longTXTrecord
=== CONT  TestAccAWSRoute53Record_multivalue_answer_basic
=== CONT  TestAccAWSRoute53Record_NameChange
=== CONT  TestAccAWSRoute53Record_geolocation_basic
=== CONT  TestAccAWSRoute53Record_empty
=== CONT  TestAccAWSRoute53Record_AliasChange
=== CONT  TestAccAWSRoute53Record_allowOverwrite
=== CONT  TestAccAWSRoute53Record_SetIdentifierChange
=== CONT  TestAccAWSRoute53Record_weighted_basic
=== CONT  TestAccAWSRoute53Record_TypeChange
=== CONT  TestAccAWSRoute53Record_latency_basic
=== CONT  TestAccAWSRoute53Record_weighted_alias
=== CONT  TestAccAWSRoute53Record_Alias_Uppercase
=== CONT  TestAccAWSRoute53Record_Alias_VpcEndpoint
=== CONT  TestAccAWSRoute53Record_Alias_S3
=== CONT  TestAccAWSRoute53Record_Alias_Elb
=== CONT  TestAccAWSRoute53Record_weighted_to_simple_basic
=== CONT  TestAccAWSRoute53Record_spfSupport
=== CONT  TestAccAWSRoute53Record_failover
--- PASS: TestAccAWSRoute53Record_longTXTrecord (138.32s)
=== CONT  TestAccAWSRoute53Record_wildcard
--- PASS: TestAccAWSRoute53Record_failover (147.67s)
=== CONT  TestAccAWSRoute53Record_generatesSuffix
--- PASS: TestAccAWSRoute53Record_Alias_Elb (151.26s)
=== CONT  TestAccAWSRoute53Record_caaSupport
--- PASS: TestAccAWSRoute53Record_empty (169.00s)
=== CONT  TestAccAWSRoute53Record_disappears_MultipleRecords
--- PASS: TestAccAWSRoute53Record_multivalue_answer_basic (177.89s)
=== CONT  TestAccAWSRoute53Record_txtSupport
--- PASS: TestAccAWSRoute53Record_Alias_Uppercase (180.78s)
=== CONT  TestAccAWSRoute53Record_basic_fqdn
--- PASS: TestAccAWSRoute53Record_spfSupport (184.73s)
=== CONT  TestAccAWSRoute53Record_disappears
--- PASS: TestAccAWSRoute53Record_latency_basic (192.39s)
=== CONT  TestAccAWSRoute53Record_underscored
--- PASS: TestAccAWSRoute53Record_Alias_S3 (197.97s)
--- PASS: TestAccAWSRoute53Record_geolocation_basic (204.19s)
--- PASS: TestAccAWSRoute53Record_basic (211.41s)
--- PASS: TestAccAWSRoute53Record_TypeChange (221.02s)
--- PASS: TestAccAWSRoute53Record_SetIdentifierChange (229.11s)
--- PASS: TestAccAWSRoute53Record_weighted_to_simple_basic (239.16s)
--- PASS: TestAccAWSRoute53Record_AliasChange (253.06s)
--- PASS: TestAccAWSRoute53Record_allowOverwrite (260.47s)
--- PASS: TestAccAWSRoute53Record_weighted_basic (281.95s)
--- PASS: TestAccAWSRoute53Record_generatesSuffix (136.26s)
--- PASS: TestAccAWSRoute53Record_NameChange (288.70s)
--- PASS: TestAccAWSRoute53Record_caaSupport (137.73s)
--- PASS: TestAccAWSRoute53Record_weighted_alias (312.53s)
--- PASS: TestAccAWSRoute53Record_txtSupport (139.52s)
--- PASS: TestAccAWSRoute53Record_disappears (136.58s)
--- PASS: TestAccAWSRoute53Record_underscored (138.32s)
--- PASS: TestAccAWSRoute53Record_wildcard (194.25s)
--- PASS: TestAccAWSRoute53Record_basic_fqdn (154.01s)
--- PASS: TestAccAWSRoute53Record_disappears_MultipleRecords (176.53s)
--- PASS: TestAccAWSRoute53Record_Alias_VpcEndpoint (537.54s)
ok   537.599s

@jbergknoff-rival jbergknoff-rival requested a review from Dec 17, 2019
@ghost ghost added needs-triage size/M service/route53 tests labels Dec 17, 2019
@@ -1394,7 +1470,7 @@ resource "aws_route53_record" "alias" {

resource "aws_elb" "main" {
name = "foobar-terraform-elb-%s"
availability_zones = ["us-west-2a"]
availability_zones = slice(data.aws_availability_zones.available.names, 0, 1)
Copy link
Contributor Author

@jbergknoff-rival jbergknoff-rival Dec 17, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This handful of tests was hardcoded to us-west-2 and can only pass in that region. This updates them to work in other regions, which I needed in order to post the full acceptance test output.

@bflad bflad added bug and removed needs-triage labels Dec 18, 2019
Copy link

danieladams456 commented May 11, 2020

@bflad bump when you have time. This would be helpful so we don't have to manually clean up records. Thank you!

Copy link

gawbul commented Jun 5, 2020

Is this a fix for #9024?

Copy link

gawbul commented Jun 5, 2020

Is this a fix for #9024?

Ah, yes, have seen it referenced on that issue 馃憤

Copy link
Contributor Author

jbergknoff-rival commented Jun 5, 2020

@gawbul this PR is #11335, did you mean to ask about a different issue?

Copy link

gawbul commented Jun 5, 2020

@gawbul this PR is #11335, did you mean to ask about a different issue?

Yes, sorry, meant #9024! Have updated above 馃憤

bflad approved these changes Jul 1, 2020
Copy link

@bflad bflad left a comment

Oh goodness, this resource needs to be refactored to make more sense. Thank you for fixing this, @jbergknoff-rival! 馃殌

Output from acceptance testing:

--- PASS: TestAccAWSRoute53Record_Alias_Elb (180.35s)
--- PASS: TestAccAWSRoute53Record_Alias_S3 (171.16s)
--- PASS: TestAccAWSRoute53Record_Alias_Uppercase (215.76s)
--- PASS: TestAccAWSRoute53Record_Alias_VpcEndpoint (523.96s)
--- PASS: TestAccAWSRoute53Record_AliasChange (260.40s)
--- PASS: TestAccAWSRoute53Record_allowOverwrite (226.43s)
--- PASS: TestAccAWSRoute53Record_basic (144.66s)
--- PASS: TestAccAWSRoute53Record_basic_fqdn (146.16s)
--- PASS: TestAccAWSRoute53Record_caaSupport (150.70s)
--- PASS: TestAccAWSRoute53Record_disappears (130.35s)
--- PASS: TestAccAWSRoute53Record_disappears_MultipleRecords (183.50s)
--- PASS: TestAccAWSRoute53Record_empty (201.91s)
--- PASS: TestAccAWSRoute53Record_failover (185.00s)
--- PASS: TestAccAWSRoute53Record_generatesSuffix (173.50s)
--- PASS: TestAccAWSRoute53Record_geolocation_basic (255.41s)
--- PASS: TestAccAWSRoute53Record_latency_basic (146.36s)
--- PASS: TestAccAWSRoute53Record_longTXTrecord (158.31s)
--- PASS: TestAccAWSRoute53Record_multivalue_answer_basic (247.80s)
--- PASS: TestAccAWSRoute53Record_NameChange (306.27s)
--- PASS: TestAccAWSRoute53Record_SetIdentifierChange (219.03s)
--- PASS: TestAccAWSRoute53Record_spfSupport (169.50s)
--- PASS: TestAccAWSRoute53Record_txtSupport (137.39s)
--- PASS: TestAccAWSRoute53Record_TypeChange (200.09s)
--- PASS: TestAccAWSRoute53Record_underscored (147.19s)
--- PASS: TestAccAWSRoute53Record_weighted_alias (330.82s)
--- PASS: TestAccAWSRoute53Record_weighted_basic (165.71s)
--- PASS: TestAccAWSRoute53Record_weighted_to_simple_basic (222.40s)
--- PASS: TestAccAWSRoute53Record_wildcard (193.78s)

@@ -1376,6 +1448,10 @@ resource "aws_route53_record" "ap-northeast-1" {

const testAccRoute53RecordConfigAliasElb = `
data "aws_availability_zones" "available" {
Copy link

@bflad bflad Jul 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will add Local Zone filtering post-merge for all these.

@bflad bflad merged commit bc51e46 into hashicorp:master Jul 1, 2020
1 check passed
bflad added a commit that referenced this issue Jul 1, 2020
Copy link

ghost commented Jul 3, 2020

This has been released in version 2.69.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

Copy link

ghost commented Jul 31, 2020

I'm going to lock this issue because it has been closed for 30 days . This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@hashicorp hashicorp locked and limited conversation to collaborators Jul 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
bug service/route53 size/M tests
None yet

Successfully merging this pull request may close these issues.

5 participants