feat(route53): add support for grantDelegation on imported PublicHostedZone by lpizzinidev · Pull Request #26333 · aws/aws-cdk

lpizzinidev · 2023-07-12T08:37:28Z

Imported PublicHostedZone with fromPublicHostedZoneId and fromPublicHostedZoneAttributes don't have support for the grantDelegation method since they return an instance of type IPublicHostedZone.

This change adds support for grantDelegation to those instances as well.

Closes #26240.

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

…edZone

gitpod-io · 2023-07-12T08:37:40Z

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.

scanlonp

We need the build to succeed before giving more thought to the review. The build was failing because the API had changed and these changes will likely fix that. Thanks!

scanlonp · 2023-07-24T23:08:47Z

packages/aws-cdk-lib/aws-route53/lib/hosted-zone.ts

        return makeHostedZoneArn(this, this.hostedZoneId);
      }
+      public grantDelegation(grantee: iam.IGrantable) {
+        makeGrantDelegation(grantee, this.hostedZoneArn);


Looks like you need to return makeGrantDelegation

scanlonp · 2023-07-24T23:09:19Z

packages/aws-cdk-lib/aws-route53/lib/hosted-zone.ts

        return makeHostedZoneArn(this, this.hostedZoneId);
      }
+      public grantDelegation(grantee: iam.IGrantable) {
+        makeGrantDelegation(grantee, this.hostedZoneArn);


scanlonp · 2023-07-24T23:09:26Z

packages/aws-cdk-lib/aws-route53/lib/hosted-zone.ts

-    });
-
-    return g1.combine(g2);
+    makeGrantDelegation(grantee, this.hostedZoneArn);


scanlonp · 2023-07-24T23:11:22Z

packages/aws-cdk-lib/aws-route53/lib/util.ts

  });
 }
+
+export function makeGrantDelegation(grantee: iam.IGrantable, hostedZoneArn: string) {


Please specify the return type in the function definition (looks like iam.Grant or iam.IGrantable)

Pull request has been modified.

scanlonp

All of the code looks good. Small nit: we should either move the code to the existing associated cross account delegation integ test, or change the name to take route53 out of the test name to be consistent with the other integ tests.

.../@aws-cdk-testing/framework-integ/test/aws-route53/test/integ.route53-imported-delegation.ts

scanlonp · 2023-08-17T18:37:58Z

.../@aws-cdk-testing/framework-integ/test/aws-route53/test/integ.route53-imported-delegation.ts

+const role = new iam.Role(stack, 'Role', {
+  assumedBy: new iam.AccountRootPrincipal(),
+});
+
+const publicZone = PublicHostedZone.fromPublicHostedZoneId(stack, 'PublicZone', 'public-zone-id');
+publicZone.grantDelegation(role);


I think we can move these lines of code into the cross-account-zone-delegation integ test, unless you thought about this and had a reason for keeping it separate.

Maybe the name could to shift to const importedPublicZone = PublicHostedZone.from...

scanlonp

Thanks!

mergify · 2023-08-18T16:00:49Z

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).

mergify · 2023-08-18T16:05:35Z

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 · 2023-08-18T17:09:59Z

AWS CodeBuild CI Report

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

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

mergify · 2023-08-18T17:10:20Z

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).

…dZone anymore (#26888) `Identity.publicHostedZone` takes an `IPublicHostedZone`, but because of TypeScript structural typing it would also accept an `IHostedZone`. When in [this PR](#26333) the `grantDelegation` method was added to the `IPublicHostedZone` interface, this passing was no longer allowed and code that used to work on accident, no longer works. For example: ``` const zone = HostedZone.fromHostedZoneId(stack, 'Zone', 'hosted-id'); const sesIdentity = ses.Identity.publicHostedZone(zone); ``` It raises an error because the imported `zone` does not implement the `grantDelegation` method. This fix moves the `grantDelegation` method declaration into the `IHostedZone` interface and makes it available to all imported zones. Closes #26872. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

lpizzinidev added 2 commits July 12, 2023 10:31

feat(route53): add support for grantDelegation on imported PublicHost…

cfba443

…edZone

Merge branch 'main' into awsgh-26240

76fde39

aws-cdk-automation requested a review from a team July 12, 2023 08:37

github-actions bot added star-contributor [Pilot] contributed between 25-49 PRs to the CDK effort/small Small work item – less than a day of effort feature-request A feature should be added or improved. p2 labels Jul 12, 2023

aws-cdk-automation previously requested changes Jul 12, 2023

View reviewed changes

lpizzinidev added 2 commits July 17, 2023 09:01

Merge branch 'main' into awsgh-26240

1c6d8aa

added integration test

50bc8ee

lpizzinidev force-pushed the gh-26240 branch from 73b2390 to 50bc8ee Compare July 17, 2023 07:28

scanlonp previously requested changes Jul 24, 2023

View reviewed changes

lpizzinidev added 2 commits July 25, 2023 10:21

Merge branch 'main' into awsgh-26240

6690af4

fixed function return type

9594bf0

fixed README

7523cdd

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jul 25, 2023

github-actions bot mentioned this pull request Aug 1, 2023

Monthly PRs metrics report - July 2023 #26583

Closed

scanlonp reviewed Aug 17, 2023

View reviewed changes

.../@aws-cdk-testing/framework-integ/test/aws-route53/test/integ.route53-imported-delegation.ts Outdated Show resolved Hide resolved

scanlonp reviewed Aug 17, 2023

View reviewed changes

lpizzinidev added 2 commits August 18, 2023 09:34

Merge branch 'main' into awsgh-26240

f9b5daa

updated integration test

d651da1

scanlonp approved these changes Aug 18, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Aug 18, 2023

Merge branch 'main' into awsgh-26240

f7f1898

mergify bot merged commit a93af2f into aws:main Aug 18, 2023

lpizzinidev mentioned this pull request Aug 25, 2023

fix(route53): IHostedZone cannot be used for ses.Identity.publicHostedZone anymore #26888

Merged

Conversation

lpizzinidev commented Jul 12, 2023

Uh oh!

gitpod-io bot commented Jul 12, 2023

Uh oh!

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Uh oh!

scanlonp left a comment

Choose a reason for hiding this comment

Uh oh!

scanlonp Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

scanlonp Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

scanlonp Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

scanlonp Jul 24, 2023

Choose a reason for hiding this comment

Uh oh!

scanlonp left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

scanlonp Aug 17, 2023

Choose a reason for hiding this comment

Uh oh!

scanlonp left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Aug 18, 2023

Uh oh!

mergify bot commented Aug 18, 2023

Uh oh!

aws-cdk-automation commented Aug 18, 2023

AWS CodeBuild CI Report

Uh oh!

mergify bot commented Aug 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants