Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(route53): support multiple cross account DNS delegations #17837

Merged
merged 5 commits into from Jan 5, 2022
Merged

fix(route53): support multiple cross account DNS delegations #17837

merged 5 commits into from Jan 5, 2022

Conversation

phoefflin
Copy link
Contributor

@phoefflin phoefflin commented Dec 3, 2021 •
edited

the custom resource lambda function's role is only created once. To support multiple zone delegations the role creation and policy management needs to be decoupled so each CrossAccountZoneDelegationRecord instance can add an individual policy to the role.

Fixes #17836

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

@phoefflin
fix(route53): support multiple cross account DNS delegations
da00c11 
create individual policies for each delegated zone and add them
individually to the singleton lambda function role.

fixes aws#17836
@gitpod-io
Copy link

gitpod-io bot commented Dec 3, 2021

@github-actions github-actions bot added the @aws-cdk/aws-route53 Related to Amazon Route 53 label Dec 3, 2021
njlynch
njlynch previously requested changes Dec 29, 2021
View reviewed changes
Copy link
Contributor

@njlynch njlynch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this contribution!

The change as-is looks great. Can you just add a unit test for the new behavior?

@mergify mergify bot dismissed njlynch’s stale review December 30, 2021 15:18

Pull request has been modified.

njlynch
njlynch approved these changes Jan 4, 2022
View reviewed changes
Copy link
Contributor

@njlynch njlynch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mergify
Copy link
Contributor

mergify bot commented Jan 5, 2022

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

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 54e381b
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

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

@mergify mergify bot merged commit 76b5c0d into aws:master Jan 5, 2022
@mergify
Copy link
Contributor

mergify bot commented Jan 5, 2022

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

@phoefflin phoefflin deleted the route53-crossaccount-delegation branch January 5, 2022 10:45
@ckifer
Copy link
Contributor

ckifer commented Feb 17, 2022 •
edited

@phoefflin @njlynch upon upgrading to monocdk v1.139 and beyond with no other code changes I am unable to assume my cross-account delegation role in the parent account with access denied. I am doing more research as I am not sure how much of an issue this is yet but commenting for visibility and that there might be something up.

@phoefflin
Copy link
Contributor Author

phoefflin commented Feb 17, 2022 •
edited

@ckifer I also have some sporadic issue in v2.12 when adding additional zones. I have the feeling it could be a dependency issue that the custom resource lambda is running before the additional policy is added to the lambda role (see https://github.com/phoefflin/aws-cdk/blob/master/packages/%40aws-cdk/aws-route53/lib/record-set.ts#L699). I'm trying to verify.

@ckifer
Copy link
Contributor

ckifer commented Feb 17, 2022 •
edited

@phoefflin I have two uses of this Construct in different child accounts with the same parent account but different roles.

  1. Api domain being delegated to an existing HZ in a parent account: myservice.api.mydomain.com -> api.mydomain.com - this fails
  2. Website domain being delegation to an existing HZ in a parent account: mysite.app.mydomain.com -> app.mydomain.com - this works

The setup is exactly the same for them both so I'm not sure whats going on here but its definitely being flaky.

Pre-change we have an inline policy on the custom resource with my parent account role Arn, post-change we have a ref to a role but I think you're right it doesn't resolve on time.

@phoefflin phoefflin mentioned this pull request Feb 18, 2022
Closed
@phoefflin
Copy link
Contributor Author

@ckifer see #19041

TikiTDO pushed a commit to TikiTDO/aws-cdk that referenced this pull request Feb 21, 2022
@phoefflin @TikiTDO
fix(route53): support multiple cross account DNS delegations (aws#17837)
b70636c 
the custom resource lambda function's role is only created once. To support multiple zone delegations the role creation and policy management needs to be decoupled so each CrossAccountZoneDelegationRecord instance can add an individual policy to the  role.

Fixes aws#17836

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@njlynch njlynch njlynch approved these changes

Assignees

@njlynch njlynch

Labels
@aws-cdk/aws-route53 Related to Amazon Route 53
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

(route53): cross account zone delegations of more than one zone fail
4 participants
@phoefflin @aws-cdk-automation @ckifer @njlynch