feat(route53): support restricting delegated zone names when using grantDelegation#35129
Conversation
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
pahud
left a comment
There was a problem hiding this comment.
A few minor suggestions for consideration:
-
Input Validation: Consider adding validation that the provided record names are valid DNS names to catch errors early rather than at IAM policy evaluation time.
-
Documentation Enhancement: It might be helpful to clarify in the README that the record names should be the actual NS record names that will be created (e.g., for delegating
beta.example.com, you'd specify['beta.example.com']). -
Property Naming: While
nameEqualsis clear,recordNamesorallowedRecordNamesmight be slightly more intuitive for users.
These are minor suggestions - the core implementation is solid. Thanks for contributing this useful feature to the CDK!
This comment was marked as outdated.
This comment was marked as outdated.
|
Maybe the property should be named Update: so according to RFC 9499 DNS Terminology there is no uniform name, but child zone is definitely the most common name. |
This comment was marked as outdated.
This comment was marked as outdated.
88ea9b1 to
005bb30
Compare
This comment was marked as outdated.
This comment was marked as outdated.
|
Additionally, the integ test is using conditions to limit access |
|
Tokens in Additionally we now handle the octal code conversion in the delegated zone name which is something I missed on the first iteration. Names with tokens in them are not encoded.
|
|
@Mergifyio update |
❌ Mergify doesn't have permission to updateDetailsFor security reasons, Mergify can't update this pull request. Try updating locally. |
|
Noticed opportunity for a small improvement in the input validation order, went ahead and updated it |
|
Thank you for contributing! Your pull request will be automatically updated and merged (do not update manually, and be sure to allow changes to be pushed to your fork). |
|
This pull request has been removed from the queue for the following reason: The pull request can't be updated
You should update or rebase your pull request manually. If you do, this pull request will automatically be requeued once the queue conditions match again. |
Pull request has been modified.
|
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). |
|
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). |
|
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
Closes #28078.
Reason for this change
Allowing the option to restrict the hosted zone names the delegation role can create records for encourages minimum permissions setup. The linked issue establishes a fairly common usecase - different roles for
dev.example.comeandprod.example.com,Description of changes
Adds the interface
GrantDelegationOptions, with optional readonly propdelegatedZoneNames. This interface is used as an optional prop tohostedZone.grantDelegation().Example usage:
Added some validation that ensures each of the
delegatedZoneNamesis a valid subdomain of the parent hosted zone.Additionally, updated the README with usage instructions and fixed an outdated code example for how to use
grantDelegation. This code example was giving too broad permissions that what was necessary.Describe any new or updated permissions being added
when
delegatedZoneNamesis provided with[a.example.com], the following condition is added:"ForAllValues:StringEquals": { "route53:ChangeResourceRecordSetsRecordTypes": [ "NS" ], "route53:ChangeResourceRecordSetsActions": [ "UPSERT", "DELETE" ], + "route53:ChangeResourceRecordSetsNormalizedRecordNames": [ + "a.example.com" + ]Description of how you validated changes
Updated Integ and unit tests
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license