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

feat(acm-certificatemanager): Add assume role for DNS validation record creation #23526

Closed
wants to merge 2 commits into from
Closed

feat(acm-certificatemanager): Add assume role for DNS validation record creation #23526

wants to merge 2 commits into from

Conversation

johnf
Copy link
Contributor

@johnf johnf commented Jan 2, 2023 •
edited
Loading

Add the ability to specify a role to assume to create DNS records for ACM validation.

This allows the lambda function to assume a role to make the DNS changes, which is necessary if the zone us hosted in another AWS account.

This should help address #8934, #4469, #12657, #13686

All Submissions:

Adding new Construct Runtime Dependencies:

  • This PR adds new construct runtime dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

@gitpod-io
Copy link

gitpod-io bot commented Jan 2, 2023

@github-actions github-actions bot added beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2 labels Jan 2, 2023
@aws-cdk-automation aws-cdk-automation requested a review from a team January 2, 2023 05:42
@johnf
Copy link
Contributor Author

johnf commented Jan 2, 2023

This is pending #23525

@johnf johnf force-pushed the dns-validated-cert-assume-role branch from 7fefb77 to 38945f6 Compare January 2, 2023 05:43
aws-cdk-automation
aws-cdk-automation previously requested changes Jan 2, 2023
View reviewed changes
Copy link
Collaborator

@aws-cdk-automation aws-cdk-automation left a comment

Choose a reason for hiding this comment

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

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.

@johnf johnf changed the title Dns validated cert assume role feature(acm-certificatemanager): Add assume role for DNS records Jan 2, 2023
@johnf johnf force-pushed the dns-validated-cert-assume-role branch from 38945f6 to 12b43f4 Compare January 2, 2023 22:56
@johnf johnf changed the title feature(acm-certificatemanager): Add assume role for DNS records feat(acm-certificatemanager): Add assume role for DNS validation record creation Jan 2, 2023
@johnf johnf force-pushed the dns-validated-cert-assume-role branch 3 times, most recently from bb6794b to db23f40 Compare January 3, 2023 01:01
@aws-cdk-automation aws-cdk-automation dismissed their stale review January 3, 2023 01:04

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

@zeyad001
Copy link
Contributor

Any progress on this?

@johnf johnf force-pushed the dns-validated-cert-assume-role branch from db23f40 to 000ee87 Compare January 17, 2023 20:55
@CashyAdmin
Copy link

Any way we can assist in getting this merged?

moltar
moltar reviewed Feb 1, 2023
View reviewed changes
packages/@aws-cdk/aws-certificatemanager/README.md Outdated
If the Route 53 Hosted Zone is in a separate AWS account you can pass a role to assume to create the validation records

```ts
const example.vom = route53.HostedZone.fromHostedZoneAttributes(this, 'ExampleCom', {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
const example.vom = route53.HostedZone.fromHostedZoneAttributes(this, 'ExampleCom', {
const example = route53.HostedZone.fromHostedZoneAttributes(this, 'ExampleCom', {

packages/@aws-cdk/aws-certificatemanager/README.md Outdated
service: 'iam',
account: '123456789', // Role of account with zone
resource: 'role',
resourceName: 'DNSRole', // Role that is able to make changes
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
resourceName: 'DNSRole', // Role that is able to make changes
resourceName: 'DnsRole', // Role that is able to make changes

packages/@aws-cdk/aws-certificatemanager/README.md
The role in the account with the hosted zone needs to look like

```ts
const role = new iam.Role(this, 'Role', {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there needs to be a factory for this role. This feels a bit like implementation detail leakage.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a construct I've released which handles the role creation. https://constructs.dev/packages/cdk-cross-account-route53/
I wasn't sure if it was appropriate to include it in CDK itself.

@johnf
feat(acm-certificatemanager): Add assume role for DNS validation reco…
37d468b 
…rd creation

Add the ability to specify a role to assume when creating the DNS records for
ACM validation.

This allows the lambda function to assume a role to make the DNS
changes, which is necessary if the zone is hosted in another AWS
account.
@johnf johnf force-pushed the dns-validated-cert-assume-role branch from 000ee87 to 37d468b Compare February 1, 2023 21:18
@TheRealAmazonKendra
Copy link
Contributor

@Mergifyio update

@mergify
Copy link
Contributor

mergify bot commented Feb 9, 2023

update

❌ Base branch update has failed

refusing to allow a GitHub App to create or update workflow .github/workflows/close-stale-issues.yml without workflows permission
err-code: DDC01

@TheRealAmazonKendra
Copy link
Contributor

TheRealAmazonKendra commented Feb 9, 2023 •
edited
Loading

Thank you for your contribution. I apologize for the work you put into this but we have deprecated DnsValidatedCertificate in favor of Certificate. If you would like to contribute work on that construct, we'd be more than happy to take it, but I'm going to close this PR because we aren't accepting any further changes for DnsValidatedCertificate.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
  • Commit ID: c7d06f3
  • Result: FAILED
  • Build Logs (available for 30 days)

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

@chrispaton chrispaton mentioned this pull request Apr 19, 2023
Closed
@KurtMar KurtMar mentioned this pull request Sep 27, 2023
Open
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moltar moltar moltar left review comments

@aws-cdk-automation aws-cdk-automation aws-cdk-automation left review comments

Assignees
No one assigned
Labels
beginning-contributor [Pilot] contributed between 0-2 PRs to the CDK p2
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@johnf @zeyad001 @CashyAdmin @TheRealAmazonKendra @aws-cdk-automation @moltar