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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

resource/aws_route53_record: Prevent scanning entire zone for missing record #6753

Merged
merged 2 commits into from
Dec 7, 2018

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Dec 7, 2018

Route53 only provides a sorted List-type API call with a starting name and type for reading records. If a record was deleted outside of Terraform, the resource did not previously have logic to stop reading from the zone when the record name and type no longer match the original query. In practice, this meant scanning the rest of the zone 1 record per API call until the last record of the zone.

To provide simple empirical evidence of this working (not testable without rewriting findRecord()), we count the occurances of ListResourceRecordSetsResponse for TestAccAWSRoute53Record_disappears_MultipleRecords. Before (39) / After (37).

Changes:

  • resource/aws_route53_record: Use NextRecordName and NextRecordType in ListResourceRecordSets to determine if record finding should continue
  • tests/resource/aws_route53_record: Return matching route53.ResourceRecordSet in testAccCheckRoute53RecordExists()
  • tests/resource/aws_route53_record: Add _disappears acceptance testing

Output from acceptance testing:

--- PASS: TestAccAWSRoute53Record_alias (146.56s)
--- PASS: TestAccAWSRoute53Record_AliasChange (158.36s)
--- PASS: TestAccAWSRoute53Record_aliasUppercase (108.53s)
--- PASS: TestAccAWSRoute53Record_allowOverwrite (164.81s)
--- PASS: TestAccAWSRoute53Record_basic (109.86s)
--- PASS: TestAccAWSRoute53Record_basic_fqdn (119.37s)
--- PASS: TestAccAWSRoute53Record_caaSupport (126.61s)
--- PASS: TestAccAWSRoute53Record_disappears (111.17s)
--- PASS: TestAccAWSRoute53Record_disappears_MultipleRecords (210.58s)
--- PASS: TestAccAWSRoute53Record_empty (118.09s)
--- PASS: TestAccAWSRoute53Record_failover (133.92s)
--- PASS: TestAccAWSRoute53Record_generatesSuffix (130.93s)
--- PASS: TestAccAWSRoute53Record_geolocation_basic (129.61s)
--- PASS: TestAccAWSRoute53Record_importBasic (144.73s)
--- PASS: TestAccAWSRoute53Record_importUnderscored (127.50s)
--- PASS: TestAccAWSRoute53Record_latency_basic (112.86s)
--- PASS: TestAccAWSRoute53Record_longTXTrecord (110.76s)
--- PASS: TestAccAWSRoute53Record_multivalue_answer_basic (114.46s)
--- PASS: TestAccAWSRoute53Record_s3_alias (120.63s)
--- PASS: TestAccAWSRoute53Record_SetIdentiferChange (165.49s)
--- PASS: TestAccAWSRoute53Record_spfSupport (137.76s)
--- PASS: TestAccAWSRoute53Record_txtSupport (131.46s)
--- PASS: TestAccAWSRoute53Record_TypeChange (159.15s)
--- PASS: TestAccAWSRoute53Record_weighted_alias (283.85s)
--- PASS: TestAccAWSRoute53Record_weighted_basic (138.54s)
--- PASS: TestAccAWSRoute53Record_wildcard (177.44s)

bflad added 2 commits December 6, 2018 23:56
…cordSet in testAccCheckRoute53RecordExists()
… record

Route53 only provides a sorted List-type API call with a starting name and type for reading records. If a record was deleted outside of Terraform, the resource did not previously have logic to stop reading from the zone when the record name and type no longer match the original query. In practice, this meant scanning the rest of the zone 1 record per API call until the last record of the zone.

To provide simple empirical evidence of this working (not testable without rewriting `findRecord()`), we count the occurances of `ListResourceRecordSetsResponse` for `TestAccAWSRoute53Record_disappears_MultipleRecords`. Before (39) / After (37).

Changes:

* resource/aws_route53_record: Use `NextRecordName` and `NextRecordType` in `ListResourceRecordSets` to determine if record finding should continue
* tests/resource/aws_route53_record: Add _disappears acceptance testing

Output from acceptance testing:

```
--- PASS: TestAccAWSRoute53Record_alias (146.56s)
--- PASS: TestAccAWSRoute53Record_AliasChange (158.36s)
--- PASS: TestAccAWSRoute53Record_aliasUppercase (108.53s)
--- PASS: TestAccAWSRoute53Record_allowOverwrite (164.81s)
--- PASS: TestAccAWSRoute53Record_basic (109.86s)
--- PASS: TestAccAWSRoute53Record_basic_fqdn (119.37s)
--- PASS: TestAccAWSRoute53Record_caaSupport (126.61s)
--- PASS: TestAccAWSRoute53Record_disappears (111.17s)
--- PASS: TestAccAWSRoute53Record_disappears_MultipleRecords (210.58s)
--- PASS: TestAccAWSRoute53Record_empty (118.09s)
--- PASS: TestAccAWSRoute53Record_failover (133.92s)
--- PASS: TestAccAWSRoute53Record_generatesSuffix (130.93s)
--- PASS: TestAccAWSRoute53Record_geolocation_basic (129.61s)
--- PASS: TestAccAWSRoute53Record_importBasic (144.73s)
--- PASS: TestAccAWSRoute53Record_importUnderscored (127.50s)
--- PASS: TestAccAWSRoute53Record_latency_basic (112.86s)
--- PASS: TestAccAWSRoute53Record_longTXTrecord (110.76s)
--- PASS: TestAccAWSRoute53Record_multivalue_answer_basic (114.46s)
--- PASS: TestAccAWSRoute53Record_s3_alias (120.63s)
--- PASS: TestAccAWSRoute53Record_SetIdentiferChange (165.49s)
--- PASS: TestAccAWSRoute53Record_spfSupport (137.76s)
--- PASS: TestAccAWSRoute53Record_txtSupport (131.46s)
--- PASS: TestAccAWSRoute53Record_TypeChange (159.15s)
--- PASS: TestAccAWSRoute53Record_weighted_alias (283.85s)
--- PASS: TestAccAWSRoute53Record_weighted_basic (138.54s)
--- PASS: TestAccAWSRoute53Record_wildcard (177.44s)
```
@bflad bflad added bug Addresses a defect in current functionality. service/route53 Issues and PRs that pertain to the route53 service. labels Dec 7, 2018
@bflad bflad requested a review from a team December 7, 2018 06:14
@ghost ghost added size/L Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Dec 7, 2018
Copy link
Member

@mbfrahry mbfrahry left a comment

Choose a reason for hiding this comment

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

LGTM

@bflad bflad added this to the v1.52.0 milestone Dec 7, 2018
@bflad bflad merged commit 4271e0a into master Dec 7, 2018
@bflad bflad deleted the b-aws_route53_record-no-stop branch December 7, 2018 18:48
bflad added a commit that referenced this pull request Dec 7, 2018
@bflad
Copy link
Contributor Author

bflad commented Dec 13, 2018

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

@ghost
Copy link

ghost commented Apr 1, 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!

@ghost ghost locked and limited conversation to collaborators Apr 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Addresses a defect in current functionality. service/route53 Issues and PRs that pertain to the route53 service. size/L Managed by automation to categorize the size of a PR. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants