data-source/aws_route53_zone: Perform NS record lookup for private Hosted Zones #17002
Conversation
…sted Zones Reference: #10391 Reference: #16862 Changes: ``` NOTES * data-source/aws_route53_zone: The Route 53 `ListResourceRecordSets` API call has been implemented to support the `name_servers` attribute for private Hosted Zones similar to the resource implementation. Environments using restrictive IAM permissions may require updates. BUG FIXES * data-source/aws_route53_zone: Ensure `name_servers` is populated for private Hosted Zones ``` Previously after testing updates: ``` === CONT TestAccAWSRoute53ZoneDataSource_vpc data_source_aws_route53_zone_test.go:89: Step 1/1 error: Check failed: Check 3/4 error: aws_route53_zone.test: Attribute "name_servers.#" is "4", but "name_servers.#" is not set in data.aws_route53_zone.test --- FAIL: TestAccAWSRoute53ZoneDataSource_vpc (83.04s) ``` Output from acceptance testing: ``` --- PASS: TestAccAWSRoute53ZoneDataSource_id (46.24s) --- PASS: TestAccAWSRoute53ZoneDataSource_name (47.24s) --- PASS: TestAccAWSRoute53ZoneDataSource_tags (94.61s) --- PASS: TestAccAWSRoute53ZoneDataSource_vpc (94.63s) --- PASS: TestAccAWSRoute53ZoneDataSource_serviceDiscovery (110.28s) ```
ghost
added
size/S
There was a problem hiding this comment.
Optional nits but LGTM! 🎉
GovCloud:
--- PASS: TestAccAWSRoute53ZoneDataSource_tags (68.32s)
--- PASS: TestAccAWSRoute53ZoneDataSource_vpc (66.51s)
--- SKIP: TestAccAWSRoute53ZoneDataSource_id (6.86s)
--- SKIP: TestAccAWSRoute53ZoneDataSource_name (7.25s)
--- SKIP: TestAccAWSRoute53ZoneDataSource_serviceDiscovery (1.26s)
us-west-2:
--- PASS: TestAccAWSRoute53ZoneDataSource_name (45.95s)
--- PASS: TestAccAWSRoute53ZoneDataSource_id (46.08s)
--- PASS: TestAccAWSRoute53ZoneDataSource_vpc (82.93s)
--- PASS: TestAccAWSRoute53ZoneDataSource_tags (83.90s)
--- PASS: TestAccAWSRoute53ZoneDataSource_serviceDiscovery (118.57s)
| if err != nil { | ||
| return fmt.Errorf("Error finding Route 53 Hosted Zone: %v", err) | ||
| return fmt.Errorf("error getting Route 53 Hosted Zone (%s) name servers: %w", idHostedZone, err) |
There was a problem hiding this comment.
| return fmt.Errorf("error getting Route 53 Hosted Zone (%s) name servers: %w", idHostedZone, err) | |
| return fmt.Errorf("getting Route 53 Hosted Zone (%s) name servers: %w", idHostedZone, err) |
Nit: not a fan of Error: error getting... that this would produce.
There was a problem hiding this comment.
Thank you for raising this. Rather than leaving a lengthy comment that could be lost about the history of this, I have created #17314 with all the information. What is important right now is that fmt.Errorf("error creating/reading/updating/deleting Service Thing (ID): Error") is the (finally now 😖 ) documented practice for consistency that's been the goal last few years, which is showing its age in Terraform 0.12 and later. I would rather continue the existing practice for later find/replace-ability until we can decide as a team on any potential new standards.
Please feel free to check that out issue and contribute!
| if err := d.Set("name_servers", nameServers); err != nil { | ||
| return fmt.Errorf("error setting name_servers: %w", err) | ||
| } | ||
|
|
||
| tags, err = keyvaluetags.Route53ListTags(conn, idHostedZone, route53.TagResourceTypeHostedzone) | ||
|
|
||
| if err != nil { | ||
| return fmt.Errorf("Error finding Route 53 Hosted Zone: %v", err) | ||
| return fmt.Errorf("error listing Route 53 Hosted Zone (%s) tags: %w", idHostedZone, err) |
There was a problem hiding this comment.
| return fmt.Errorf("error listing Route 53 Hosted Zone (%s) tags: %w", idHostedZone, err) | |
| return fmt.Errorf("failed listing Route 53 Hosted Zone (%s) tags: %w", idHostedZone, err) |
Another approach.
| } | ||
|
|
||
| if err := d.Set("tags", tags.IgnoreAws().IgnoreConfig(ignoreTagsConfig).Map()); err != nil { | ||
| return fmt.Errorf("error setting tags: %s", err) | ||
| return fmt.Errorf("error setting tags: %w", err) |
There was a problem hiding this comment.
| return fmt.Errorf("error setting tags: %w", err) | |
| return fmt.Errorf("setting tags: %w", err) |
| if err != nil { | ||
| return []string{}, err | ||
| return nil, fmt.Errorf("error getting Route 53 Hosted Zone (%s): %w", id, err) |
There was a problem hiding this comment.
| return nil, fmt.Errorf("error getting Route 53 Hosted Zone (%s): %w", id, err) | |
| return nil, fmt.Errorf("getting Route 53 Hosted Zone (%s): %w", id, err) |
| } | ||
|
|
||
| if output == nil { | ||
| return nil, fmt.Errorf("error getting Route 53 Hosted Zone (%s): empty response", id) |
There was a problem hiding this comment.
| return nil, fmt.Errorf("error getting Route 53 Hosted Zone (%s): empty response", id) | |
| return nil, fmt.Errorf("getting Route 53 Hosted Zone (%s): empty response", id) |
| nameServers, err := getNameServers(id, name, conn) | ||
|
|
||
| if err != nil { | ||
| return nil, fmt.Errorf("error listing Route 53 Hosted Zone (%s) NS records: %w", id, err) |
There was a problem hiding this comment.
| return nil, fmt.Errorf("error listing Route 53 Hosted Zone (%s) NS records: %w", id, err) | |
| return nil, fmt.Errorf("listing Route 53 Hosted Zone (%s) NS records: %w", id, err) |
|
This has been released in version 3.26.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! |
|
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
locked as resolved and limited conversation to collaborators
Community Note
Closes #10391
Closes #16862
Release note for CHANGELOG:
Previously after testing updates:
Output from acceptance testing: