fix: route53 CrossAccountZoneDelegationRecord fails at deployment time with imported `delegatedZone` #30440

samson-keung · 2024-06-03T17:14:29Z

Issue # (if applicable)

Reason for this change

An imported delegatedZone will not have info about the Name Servers. When it is passed to CrossAccountZoneDelegationRecord, the handler will see undefined when trying to retrieve the Name Servers info on delegatedZone, then throw exception during deployment.

This change throws the exception at build time for a faster feedback loop.

Description of changes

CrossAccountZoneDelegationRecord throws exception if delegatedZone.hostedZoneNameServers is undefined.

Description of how you validated changes

Add unit test to cover the case of passing an imported HostedZone to CrossAccountZoneDelegationRecord

Checklist

My code adheres to the CONTRIBUTING GUIDE and DESIGN GUIDELINES

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

samson-keung · 2024-06-03T18:38:36Z

Exemption Request

Reason: Added build time validation. No changes to deployment behaviour, hence, no integ test update

packages/aws-cdk-lib/aws-route53/lib/record-set.ts

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

mergify · 2024-06-03T23:38:39Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2024-06-04T00:08:25Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: dde1d3f
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2024-06-04T00:08:50Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

…e with imported `delegatedZone` (aws#30440) ### Issue # (if applicable) Closes aws#28581. ### Reason for this change An imported `delegatedZone` will not have info about the Name Servers. When it is passed to `CrossAccountZoneDelegationRecord`, the handler will see `undefined` when trying to retrieve the Name Servers info on `delegatedZone`, then throw exception during deployment. This change throws the exception at build time for a faster feedback loop. ### Description of changes `CrossAccountZoneDelegationRecord` throws exception if `delegatedZone.hostedZoneNameServers` is undefined. ### Description of how you validated changes Add unit test to cover the case of passing an imported HostedZone to `CrossAccountZoneDelegationRecord` ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

vhelke · 2024-06-20T07:19:35Z

This implementation is problematic for us. We are using the imported delegatedZone method, and since the nameservers are missing we are fetching them separately using a customResource and then by using the add_property_override method we are updating the nameserver info.

Is it possible to add an attribute which would bypass the !props.delegatedZone.hostedZoneNameServers validation?

samson-keung · 2024-06-20T17:59:15Z

This implementation is problematic for us. We are using the imported delegatedZone method, and since the nameservers are missing we are fetching them separately using a customResource and then by using the add_property_override method we are updating the nameserver info.

Is it possible to add an attribute which would bypass the !props.delegatedZone.hostedZoneNameServers validation?

Thanks for reporting the issue. I have created #30600 to track and will be pushing a fix to replace the error with a warning instead.

…ment time with imported `delegatedZone` (aws#30440)" This reverts commit a3d9b10.

…time with imported `delegatedZone` (#30440)" (#30606) This reverts commit a3d9b10. ### Issue # (if applicable) Closes #30600. ### Reason for this change Reverting #30440 so that `cdk synth` succeed when using imported `delegatedZone` ### Description of changes Removed the `throw new Error()` ### Description of how you validated changes ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

…e with imported `delegatedZone` (aws#30440) ### Issue # (if applicable) Closes aws#28581. ### Reason for this change An imported `delegatedZone` will not have info about the Name Servers. When it is passed to `CrossAccountZoneDelegationRecord`, the handler will see `undefined` when trying to retrieve the Name Servers info on `delegatedZone`, then throw exception during deployment. This change throws the exception at build time for a faster feedback loop. ### Description of changes `CrossAccountZoneDelegationRecord` throws exception if `delegatedZone.hostedZoneNameServers` is undefined. ### Description of how you validated changes Add unit test to cover the case of passing an imported HostedZone to `CrossAccountZoneDelegationRecord` ### Checklist - [x] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

aws-cdk-automation · 2024-07-25T20:49:04Z

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.

samson-keung added 2 commits June 3, 2024 10:09

CrossAccountZoneDelegationRecord: throw if delegatedZone is imported

a6a7480

Merge branch 'main' into validate-hosted-zone-ns

0e6601e

aws-cdk-automation requested a review from a team June 3, 2024 17:14

github-actions bot added bug This issue is a bug. effort/medium Medium work item – several days of effort p1 labels Jun 3, 2024

mergify bot added the contribution/core This is a PR that came from AWS. label Jun 3, 2024

aws-cdk-automation previously requested changes Jun 3, 2024

View reviewed changes

samson-keung changed the title ~~fix: CrossAccountZoneDelegationRecord fails at deployment time with imported delegatedZone~~ fix: route53 CrossAccountZoneDelegationRecord fails at deployment time with imported delegatedZone Jun 3, 2024

samson-keung added 2 commits June 3, 2024 11:47

linting fix: missing comma

45efba5

Merge branch 'main' into validate-hosted-zone-ns

6489185

samson-keung marked this pull request as ready for review June 3, 2024 20:22

aaythapa reviewed Jun 3, 2024

View reviewed changes

packages/aws-cdk-lib/aws-route53/lib/record-set.ts Show resolved Hide resolved

packages/aws-cdk-lib/aws-route53/lib/record-set.ts Show resolved Hide resolved

aaythapa added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Jun 3, 2024

aws-cdk-automation added the pr/needs-maintainer-review This PR needs a review from a Core Team Member label Jun 3, 2024

Update Route53 README

5f048b2

aaythapa approved these changes Jun 3, 2024

View reviewed changes

Merge branch 'main' into validate-hosted-zone-ns

dde1d3f

mergify bot merged commit a3d9b10 into aws:main Jun 4, 2024
9 checks passed

github-actions bot mentioned this pull request Jun 10, 2024

Weekly PR metrics report #30506

Closed

samson-keung mentioned this pull request Jun 20, 2024

aws-route53: Not able to retrieve Name Servers for fake-name due to it being imported. CrossAccountZoneDelegationRecord delegatedZone validation on NameServers blocks using Custom Resource to retrieve NameServers #30600

Closed

samson-keung mentioned this pull request Jun 20, 2024

fix(route53): warn CrossAccountZoneDelegationRecord delegatedZone Name Server undefined #30602

Closed

1 task

samson-keung added a commit to samson-keung/aws-cdk that referenced this pull request Jun 21, 2024

Revert "fix: route53 CrossAccountZoneDelegationRecord fails at deploy…

0d40387

…ment time with imported `delegatedZone` (aws#30440)" This reverts commit a3d9b10.

samson-keung mentioned this pull request Jun 21, 2024

revert: route53 CrossAccountZoneDelegationRecord fails at deployment time with imported delegatedZone (#30440)" #30606

Merged

1 task

github-actions bot mentioned this pull request Jun 24, 2024

Weekly PR metrics report #30637

Closed

kazimanzurrashid mentioned this pull request Jun 29, 2024

[Snyk] Upgrade aws-cdk-lib from 2.144.0 to 2.145.0 kazimanzurrashid/aws-scheduler-go#821

Open

NOUIY mentioned this pull request Jun 29, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#106

Open

github-actions bot mentioned this pull request Jul 1, 2024

Monthly PR metrics report #30715

Closed

This was referenced Jul 5, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#107

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#108

Open

This was referenced Jul 16, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#109

Open

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#110

Open

NOUIY mentioned this pull request Jul 23, 2024

[Snyk] Upgrade: , aws-cdk NOUIY/aws-solutions-constructs#111

Open

aws locked as resolved and limited conversation to collaborators Jul 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: route53 CrossAccountZoneDelegationRecord fails at deployment time with imported `delegatedZone` #30440

fix: route53 CrossAccountZoneDelegationRecord fails at deployment time with imported `delegatedZone` #30440

samson-keung commented Jun 3, 2024

aws-cdk-automation left a comment

samson-keung commented Jun 3, 2024

mergify bot commented Jun 3, 2024

aws-cdk-automation commented Jun 4, 2024

mergify bot commented Jun 4, 2024

vhelke commented Jun 20, 2024

samson-keung commented Jun 20, 2024

aws-cdk-automation commented Jul 25, 2024

fix: route53 CrossAccountZoneDelegationRecord fails at deployment time with imported delegatedZone #30440

fix: route53 CrossAccountZoneDelegationRecord fails at deployment time with imported delegatedZone #30440

Conversation

samson-keung commented Jun 3, 2024

Issue # (if applicable)

Reason for this change

Description of changes

Description of how you validated changes

Checklist

aws-cdk-automation left a comment

Choose a reason for hiding this comment

samson-keung commented Jun 3, 2024

mergify bot commented Jun 3, 2024

aws-cdk-automation commented Jun 4, 2024

AWS CodeBuild CI Report

mergify bot commented Jun 4, 2024

vhelke commented Jun 20, 2024

samson-keung commented Jun 20, 2024

aws-cdk-automation commented Jul 25, 2024

fix: route53 CrossAccountZoneDelegationRecord fails at deployment time with imported `delegatedZone` #30440

fix: route53 CrossAccountZoneDelegationRecord fails at deployment time with imported `delegatedZone` #30440