feat(route53): support for scoping down domain names in IHostedZone.grantDelegation()

[marcogrcr]

Adds a backwards compatible parameter to IHostedZone.grantDelegation() in order to restrict the NS records with UPSERT/DELETE access.

Closes #28078.

---

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.

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

[lpizzinidev]

Thanks 👍
The implementation is good for me, I left you some comments for minor documentation improvements.

[marcogrcr]

Thanks for the review @lpizzinidev! Your suggestions make sense. I have pushed a new commit with these changes.

[lpizzinidev]

Thanks 👍

[kaizencc]

Some thoughts on implementation. Thanks for getting this started @marcogrcr

[kaizencc]

Why do we have to do this pattern? Feels like it would be easier to just go with

parentZone.grantDelegation(prodCrossAccountRole, {
  nameLike: 'beta.someexample.com',
  // or nameEquals, mutually exclusive
});

[marcogrcr]

We don't have to. I just followed what I've observed in other instances of aws-cdk (e.g. aws-cdk-lib » aws_dynamodb » Billing). Thought I could maintain consistency that way. I haven't personally come across this type pattern:

type GrantDelegationProps =
  {
    readonly nameEquals string[];
  }
  | {
    readonly nameLike:: string[];
  };

Happy to change it though if you consider it's necessary for getting the PR approved.

[kaizencc]

the pattern you are emulating, enum-like classes, i don't think is too relevant here. @marcogrcr, I responded in the other comment thread

[TheRealAmazonKendra]

So, I'm not sure that I 100% agree with this assessment. I think that it was good to treat this like an enum like class. We should be enforcing in the contract that you can't input both rather than adding a synth time validation. I think, though, I'd actually take this in a slightly different direction.

Instead of altering the current functions, why don't we create a new function that is grantScopedDelegation (or something similar) and have those functions take in GrantDelegationProps that look something like:

interface GrantDelegationProps {
  readonly grantee: IGrantable;
  readonly scope: DelegationScope;
}

DelegationScope would then have basically the two functions you created, but I would rename them nameEquals and nameLike.

The above is assuming that these two inputs really should be mutually exclusive as @kaizencc notes in another comment. If they are not, that changes my assessment here.

[kaizencc]

I think we should use a property bag. We only have once chance to add an optional prop to a public API with backwards compatibility, so lets keep the door open for future additions too. It has the additional benefit of forcing users to supply the prop name with their input, which makes things clearer.

grantDelegation(grantee: iam.IGrantable, { nameLike?: string, nameEquals?: string }): iam.Grant;

[marcogrcr]

Acknowledged. Does this look good to you?

interface GrantDelegationProps {
  readonly name?:
    {
      readonly equals: string[];
    }
    | {
      readonly like: string[];
    };
}

[kaizencc]

ideally i want

interface GrantDelegationProps {
  readonly nameEquals?: string[];
  readonly nameLike?: string[];
}

And then somewhere else make sure that nameEquals and nameLike are not both set at once.

[aws-cdk-automation]

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

[aws-cdk-automation]

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.

[marcogrcr]

@kaizencc, can we reopen the pull request? I replied to the change request comments.

Pull request has been modified.

[aws-cdk-automation]

AWS CodeBuild CI Report

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

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

[paulhcsun]

Hi @marcogrcr, are you still working on this PR? Looks like there's one more pending comment to address.

[TheRealAmazonKendra]

Removing the do-not-close label on this as it's been a while since we heard back and there are now conflicts.

I've also left a comment with a suggested change. If you would like to keep working on this but the bot closes it, I'm happy to discuss design further in the issue prior to you opening another PR so that you're not redoing the same work over and over again.

[aws-cdk-automation]

This PR has been in the CHANGES REQUESTED state for 3 weeks, and looks abandoned. To keep this PR from being closed, please continue work on it. If not, it will automatically be closed in a week.

[aws-cdk-automation]

This PR has been deemed to be abandoned, and will be automatically closed. Please create a new PR for these changes if you think this decision has been made in error.