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

E adding validation to dns records #16317

Closed
wants to merge 4 commits into from
Closed

E adding validation to dns records #16317

wants to merge 4 commits into from

Conversation

philnichol
Copy link
Contributor

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

This PR adds some validation to the elements of the records argument. It ensures that each element is either less than 255 chars, or that it contains escaped quote marks at least every 255 characters. The regex also allows an uneven number of escaped quote marks, which is a requirement for CAA records.

Took the opportunity to address some code duplication in the acceptance tests, so I ran them before and after changing them.

Thanks in advance for your review!

Closes #14941

Release note for CHANGELOG:

aws_route53_record - added validation to the elements of the records argument

Output from acceptance testing:

Before updating the acceptance tests:

$ TF_ACC=1 go test ./aws -v -count 1 -parallel 10 -run=TestAccAWSRoute53Record_ -timeout 60m
=== 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_basic_trailingPeriodAndZoneID
=== PAUSE TestAccAWSRoute53Record_basic_trailingPeriodAndZoneID
=== 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_HealthCheckId_SetIdentifierChange
=== PAUSE TestAccAWSRoute53Record_HealthCheckId_SetIdentifierChange
=== RUN   TestAccAWSRoute53Record_HealthCheckId_TypeChange
=== PAUSE TestAccAWSRoute53Record_HealthCheckId_TypeChange
=== 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_Alias_VpcEndpoint
=== CONT  TestAccAWSRoute53Record_TypeChange
=== CONT  TestAccAWSRoute53Record_latency_basic
=== CONT  TestAccAWSRoute53Record_NameChange
=== CONT  TestAccAWSRoute53Record_empty
=== CONT  TestAccAWSRoute53Record_AliasChange
=== CONT  TestAccAWSRoute53Record_SetIdentifierChange
=== CONT  TestAccAWSRoute53Record_allowOverwrite
=== CONT  TestAccAWSRoute53Record_multivalue_answer_basic
2020/11/19 13:38:00 [DEBUG] Expanded record name: www.notexample.com
2020/11/19 13:38:00 [DEBUG] List resource records sets for zone: Z062786036LZGRUF7PSE9, opts: {
  HostedZoneId: "Z062786036LZGRUF7PSE9",
  MaxItems: "100",
  StartRecordName: "www.notexample.com.",
  StartRecordType: "CNAME"
}
--- PASS: TestAccAWSRoute53Record_empty (118.56s)
=== CONT  TestAccAWSRoute53Record_caaSupport
--- PASS: TestAccAWSRoute53Record_multivalue_answer_basic (120.29s)
=== CONT  TestAccAWSRoute53Record_Alias_S3
--- PASS: TestAccAWSRoute53Record_basic (122.74s)
=== CONT  TestAccAWSRoute53Record_Alias_Elb
--- PASS: TestAccAWSRoute53Record_latency_basic (125.76s)
=== CONT  TestAccAWSRoute53Record_weighted_to_simple_basic
--- PASS: TestAccAWSRoute53Record_AliasChange (171.69s)
=== CONT  TestAccAWSRoute53Record_weighted_basic
--- PASS: TestAccAWSRoute53Record_TypeChange (174.12s)
=== CONT  TestAccAWSRoute53Record_failover
--- PASS: TestAccAWSRoute53Record_allowOverwrite (176.57s)
=== CONT  TestAccAWSRoute53Record_wildcard
--- PASS: TestAccAWSRoute53Record_SetIdentifierChange (188.46s)
=== CONT  TestAccAWSRoute53Record_generatesSuffix
--- PASS: TestAccAWSRoute53Record_NameChange (203.21s)
=== CONT  TestAccAWSRoute53Record_geolocation_basic
--- PASS: TestAccAWSRoute53Record_Alias_S3 (113.81s)
=== CONT  TestAccAWSRoute53Record_HealthCheckId_TypeChange
--- PASS: TestAccAWSRoute53Record_caaSupport (117.62s)
=== CONT  TestAccAWSRoute53Record_HealthCheckId_SetIdentifierChange
--- PASS: TestAccAWSRoute53Record_Alias_Elb (117.79s)
=== CONT  TestAccAWSRoute53Record_weighted_alias
--- PASS: TestAccAWSRoute53Record_weighted_to_simple_basic (160.48s)
=== CONT  TestAccAWSRoute53Record_basic_fqdn
--- PASS: TestAccAWSRoute53Record_weighted_basic (117.70s)
=== CONT  TestAccAWSRoute53Record_spfSupport
--- PASS: TestAccAWSRoute53Record_failover (117.99s)
=== CONT  TestAccAWSRoute53Record_txtSupport
--- PASS: TestAccAWSRoute53Record_generatesSuffix (117.77s)
=== CONT  TestAccAWSRoute53Record_basic_trailingPeriodAndZoneID
--- PASS: TestAccAWSRoute53Record_geolocation_basic (117.82s)
=== CONT  TestAccAWSRoute53Record_disappears
--- PASS: TestAccAWSRoute53Record_wildcard (165.37s)
=== CONT  TestAccAWSRoute53Record_disappears_MultipleRecords
2020/11/19 13:42:33 [DEBUG] Trying to get account information via sts:GetCallerIdentity
--- PASS: TestAccAWSRoute53Record_HealthCheckId_TypeChange (165.86s)
=== CONT  TestAccAWSRoute53Record_Alias_Uppercase
--- PASS: TestAccAWSRoute53Record_HealthCheckId_SetIdentifierChange (164.61s)
=== CONT  TestAccAWSRoute53Record_underscored
--- PASS: TestAccAWSRoute53Record_spfSupport (116.56s)
=== CONT  TestAccAWSRoute53Record_longTXTrecord
--- PASS: TestAccAWSRoute53Record_txtSupport (116.34s)
--- PASS: TestAccAWSRoute53Record_basic_fqdn (132.66s)
--- PASS: TestAccAWSRoute53Record_basic_trailingPeriodAndZoneID (116.04s)
=== CONT  TestAccAWSRoute53Record_disappears
    resource_aws_route53_record_test.go:165: [INFO] Got non-empty plan, as expected
--- PASS: TestAccAWSRoute53Record_disappears (110.60s)
=== CONT  TestAccAWSRoute53Record_disappears_MultipleRecords
    resource_aws_route53_record_test.go:187: [INFO] Got non-empty plan, as expected
--- PASS: TestAccAWSRoute53Record_weighted_alias (229.92s)
--- PASS: TestAccAWSRoute53Record_Alias_VpcEndpoint (488.68s)
--- PASS: TestAccAWSRoute53Record_disappears_MultipleRecords (152.13s)
--- PASS: TestAccAWSRoute53Record_Alias_Uppercase (116.86s)
--- PASS: TestAccAWSRoute53Record_underscored (120.66s)
--- PASS: TestAccAWSRoute53Record_longTXTrecord (118.77s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       524.758s

After updating the acceptance tests:

$ TF_ACC=1 go test ./aws -v -count 1 -parallel 10 -run=TestAccAWSRoute53Record_ -timeout 60m
go: downloading github.com/pquerna/otp v1.3.0
=== 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_basic_trailingPeriodAndZoneID
=== PAUSE TestAccAWSRoute53Record_basic_trailingPeriodAndZoneID
=== 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_HealthCheckId_SetIdentifierChange
=== PAUSE TestAccAWSRoute53Record_HealthCheckId_SetIdentifierChange
=== RUN   TestAccAWSRoute53Record_HealthCheckId_TypeChange
=== PAUSE TestAccAWSRoute53Record_HealthCheckId_TypeChange
=== 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_multivalue_answer_basic
=== PAUSE TestAccAWSRoute53Record_multivalue_answer_basic
=== RUN   TestAccAWSRoute53Record_allowOverwrite
=== PAUSE TestAccAWSRoute53Record_allowOverwrite
=== CONT  TestAccAWSRoute53Record_basic
=== CONT  TestAccAWSRoute53Record_Alias_VpcEndpoint
=== CONT  TestAccAWSRoute53Record_NameChange
=== CONT  TestAccAWSRoute53Record_geolocation_basic
=== CONT  TestAccAWSRoute53Record_weighted_alias
=== CONT  TestAccAWSRoute53Record_allowOverwrite
=== CONT  TestAccAWSRoute53Record_latency_basic
=== CONT  TestAccAWSRoute53Record_HealthCheckId_SetIdentifierChange
=== CONT  TestAccAWSRoute53Record_TypeChange
=== CONT  TestAccAWSRoute53Record_Alias_Uppercase
--- PASS: TestAccAWSRoute53Record_basic (133.19s)
=== CONT  TestAccAWSRoute53Record_empty
2020/11/19 15:31:17 [DEBUG] setting computed for "name_servers" from ComputedKeys
2020/11/19 15:31:17 [DEBUG] Creating Route53 hosted zone: {
  CallerReference: "terraform-2020111915311755620000000c",
  HostedZoneConfig: {
    Comment: "Managed by Terraform"
  },
  Name: "notexample.com"
}
--- PASS: TestAccAWSRoute53Record_Alias_Uppercase (146.80s)
=== CONT  TestAccAWSRoute53Record_multivalue_answer_basic
--- PASS: TestAccAWSRoute53Record_latency_basic (146.96s)
=== CONT  TestAccAWSRoute53Record_HealthCheckId_TypeChange
--- PASS: TestAccAWSRoute53Record_geolocation_basic (148.85s)
=== CONT  TestAccAWSRoute53Record_weighted_basic
--- PASS: TestAccAWSRoute53Record_allowOverwrite (180.35s)
=== CONT  TestAccAWSRoute53Record_Alias_S3
--- PASS: TestAccAWSRoute53Record_HealthCheckId_SetIdentifierChange (199.38s)
=== CONT  TestAccAWSRoute53Record_Alias_Elb
--- PASS: TestAccAWSRoute53Record_TypeChange (212.28s)
=== CONT  TestAccAWSRoute53Record_weighted_to_simple_basic
--- PASS: TestAccAWSRoute53Record_NameChange (231.26s)
=== CONT  TestAccAWSRoute53Record_AliasChange
--- PASS: TestAccAWSRoute53Record_empty (116.90s)
=== CONT  TestAccAWSRoute53Record_SetIdentifierChange
--- PASS: TestAccAWSRoute53Record_weighted_alias (262.56s)
=== CONT  TestAccAWSRoute53Record_wildcard
--- PASS: TestAccAWSRoute53Record_weighted_basic (116.17s)
=== CONT  TestAccAWSRoute53Record_generatesSuffix
--- PASS: TestAccAWSRoute53Record_multivalue_answer_basic (118.70s)
=== CONT  TestAccAWSRoute53Record_caaSupport
--- PASS: TestAccAWSRoute53Record_Alias_S3 (117.44s)
=== CONT  TestAccAWSRoute53Record_basic_fqdn
--- PASS: TestAccAWSRoute53Record_Alias_Elb (120.42s)
=== CONT  TestAccAWSRoute53Record_failover
--- PASS: TestAccAWSRoute53Record_HealthCheckId_TypeChange (179.58s)
=== CONT  TestAccAWSRoute53Record_spfSupport
--- PASS: TestAccAWSRoute53Record_weighted_to_simple_basic (175.22s)
=== CONT  TestAccAWSRoute53Record_txtSupport
--- PASS: TestAccAWSRoute53Record_caaSupport (124.91s)
=== CONT  TestAccAWSRoute53Record_basic_trailingPeriodAndZoneID
--- PASS: TestAccAWSRoute53Record_generatesSuffix (125.45s)
=== CONT  TestAccAWSRoute53Record_disappears
--- PASS: TestAccAWSRoute53Record_AliasChange (178.75s)
=== CONT  TestAccAWSRoute53Record_disappears_MultipleRecords
--- PASS: TestAccAWSRoute53Record_SetIdentifierChange (172.43s)
=== CONT  TestAccAWSRoute53Record_underscored
--- PASS: TestAccAWSRoute53Record_wildcard (175.07s)
--- PASS: TestAccAWSRoute53Record_failover (117.93s)
--- PASS: TestAccAWSRoute53Record_basic_fqdn (140.81s)
--- PASS: TestAccAWSRoute53Record_spfSupport (114.05s)
--- PASS: TestAccAWSRoute53Record_Alias_VpcEndpoint (478.60s)
--- PASS: TestAccAWSRoute53Record_disappears (108.26s)
--- PASS: TestAccAWSRoute53Record_basic_trailingPeriodAndZoneID (120.90s)
--- PASS: TestAccAWSRoute53Record_underscored (122.29s)
--- PASS: TestAccAWSRoute53Record_disappears_MultipleRecords (151.28s)
--- PASS: TestAccAWSRoute53Record_txtSupport (246.21s)
PASS
ok      github.com/terraform-providers/terraform-provider-aws/aws       633.768s

@philnichol philnichol requested a review from a team as a code owner November 19, 2020 16:11
@ghost ghost added size/M Managed by automation to categorize the size of a PR. service/route53 Issues and PRs that pertain to the route53 service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Nov 19, 2020
@github-actions github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Nov 19, 2020
Base automatically changed from master to main January 23, 2021 00:59
@breathingdust breathingdust added enhancement Requests to existing resources that expand the functionality or scope. and removed needs-triage Waiting for first response or review from a maintainer. labels Sep 8, 2021
@zhelding
Copy link
Contributor

Pull request #21306 has significantly refactored the AWS Provider codebase. As a result, most PRs opened prior to the refactor now have merge conflicts that must be resolved before proceeding.

Specifically, PR #21306 relocated the code for all AWS resources and data sources from a single aws directory to a large number of separate directories in internal/service, each corresponding to a particular AWS service. This separation of code has also allowed for us to simplify the names of underlying functions -- while still avoiding namespace collisions.

We recognize that many pull requests have been open for some time without yet being addressed by our maintainers. Therefore, we want to make it clear that resolving these conflicts in no way affects the prioritization of a particular pull request. Once a pull request has been prioritized for review, the necessary changes will be made by a maintainer -- either directly or in collaboration with the pull request author.

For a more complete description of this refactor, including examples of how old filepaths and function names correspond to their new counterparts: please refer to issue #20000.

For a quick guide on how to amend your pull request to resolve the merge conflicts resulting from this refactor and bring it in line with our new code patterns: please refer to our Service Package Refactor Pull Request Guide.

@philnichol philnichol closed this Dec 13, 2022
@philnichol philnichol deleted the e-adding-validation-to-dns-records branch December 13, 2022 07:41
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 13, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Requests to existing resources that expand the functionality or scope. service/route53 Issues and PRs that pertain to the route53 service. size/M 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.

Route53 doesn't validate TXT record lengths
3 participants