fix(acm-certificatemanager): DnsValidatedCertificateHandler support for `SubjectAlternativeNames` #7050

nlang · 2020-03-27T22:42:58Z

First, sorry to create another PR for that issue. I did just find out that this issue already exists and has an open PR when I was about to share the fix I created for myself.

Here is a clean PR that only addresses the issue (#nohousekeeping) and also handles the case that a certificate exposes multiple DomainValidationOptions with the same CNAME record. That happens when you add subdomains of the main domain (e.g. example.com as domain and *.example.com in alt names). Of course I also added some tests ;)

And btw: Thank you for making CDK and your overall awsomeness!

Commit Message

added support for certificates with SubjectAlternativNames by creating a Dns Record in Route53 for distinct entries in DomainValidationOptions (packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/lib/index.js)
added unittests

fixes #4659

End Commit Message

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

…icates with `SubjectAlternativeNames` fixes aws#4659

aws-cdk-automation · 2020-03-27T23:04:16Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 95f4aac
Result: FAILED
Build Logs (available for 30 days)

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

aws-cdk-automation · 2020-03-28T17:48:27Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 3839efd
Result: FAILED
Build Logs (available for 30 days)

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

skinny85

Thank you for the contribution @nlang , it's very high quality!

One small thing though, and the reason the build is failing. The @aws-cdk/aws-ecs-patterns package depends on the aws-certificatemanager package, and in particular uses the DnsValidatedCertificate construct in a few of its classes like ApplicationMultipleTargetGroupsEc2/FargateService, ApplicationLoadBalancedEc/Fargate2Service, etc. Because you changed the source of the custom resource, the parameter names (which include the hash of the source changed as well), and the snapshot tests in @aws-cdk/aws-ecs-patterns are failing:

@aws-cdk/aws-ecs-patterns: [-] Parameter AssetParameters01b2187f99280c53b7d58040d494b5d051e1e253601fc32dee62ba56712db247S3Bucket3747EA0C: {"Type":"String","Description":"S3 bucket for asset \"01b2187f99280c53b7d58040d494b5d051e1e253601fc32dee62ba56712db247\""}
@aws-cdk/aws-ecs-patterns: [-] Parameter AssetParameters01b2187f99280c53b7d58040d494b5d051e1e253601fc32dee62ba56712db247S3VersionKey13E25E1F: {"Type":"String","Description":"S3 key for asset version \"01b2187f99280c53b7d58040d494b5d051e1e253601fc32dee62ba56712db247\""}
@aws-cdk/aws-ecs-patterns: [-] Parameter AssetParameters01b2187f99280c53b7d58040d494b5d051e1e253601fc32dee62ba56712db247ArtifactHashFB4438F1: {"Type":"String","Description":"Artifact hash for asset \"01b2187f99280c53b7d58040d494b5d051e1e253601fc32dee62ba56712db247\""}
@aws-cdk/aws-ecs-patterns: [+] Parameter AssetParameters32949dc2510e9dcfb770033a67b684a0ed49f86fb8a6ed93e2ccafbb4217641dS3Bucket7A5FA028: {"Type":"String","Description":"S3 bucket for asset \"32949dc2510e9dcfb770033a67b684a0ed49f86fb8a6ed93e2ccafbb4217641d\""}
@aws-cdk/aws-ecs-patterns: [+] Parameter AssetParameters32949dc2510e9dcfb770033a67b684a0ed49f86fb8a6ed93e2ccafbb4217641dS3VersionKeyD02759A5: {"Type":"String","Description":"S3 key for asset version \"32949dc2510e9dcfb770033a67b684a0ed49f86fb8a6ed93e2ccafbb4217641d\""}
@aws-cdk/aws-ecs-patterns: [+] Parameter AssetParameters32949dc2510e9dcfb770033a67b684a0ed49f86fb8a6ed93e2ccafbb4217641dArtifactHashC10E7B43: {"Type":"String","Description":"Artifact hash for asset \"32949dc2510e9dcfb770033a67b684a0ed49f86fb8a6ed93e2ccafbb4217641d\""}
@aws-cdk/aws-ecs-patterns: Resources
@aws-cdk/aws-ecs-patterns: [~] AWS::Lambda::Function myServiceCertificateCertificateRequestorFunctionC16CEAAF 
@aws-cdk/aws-ecs-patterns:  â””â”€ [~] Code
@aws-cdk/aws-ecs-patterns:      â”œâ”€ [~] .S3Bucket:
@aws-cdk/aws-ecs-patterns:      â”‚   â””â”€ [~] .Ref:
@aws-cdk/aws-ecs-patterns:      â”‚       â”œâ”€ [-] AssetParameters01b2187f99280c53b7d58040d494b5d051e1e253601fc32dee62ba56712db247S3Bucket3747EA0C
@aws-cdk/aws-ecs-patterns:      â”‚       â””â”€ [+] AssetParameters32949dc2510e9dcfb770033a67b684a0ed49f86fb8a6ed93e2ccafbb4217641dS3Bucket7A5FA028
@aws-cdk/aws-ecs-patterns:      â””â”€ [~] .S3Key:
@aws-cdk/aws-ecs-patterns:          â””â”€ [~] .Fn::Join:
@aws-cdk/aws-ecs-patterns:              â””â”€ @@ -8,7 +8,7 @@
@aws-cdk/aws-ecs-patterns:                 [ ]   "Fn::Split": [
@aws-cdk/aws-ecs-patterns:                 [ ]     "||",
@aws-cdk/aws-ecs-patterns:                 [ ]     {
@aws-cdk/aws-ecs-patterns:                 [-]       "Ref": "AssetParameters01b2187f99280c53b7d58040d494b5d051e1e253601fc32dee62ba56712db247S3VersionKey13E25E1F"
@aws-cdk/aws-ecs-patterns:                 [+]       "Ref": "AssetParameters32949dc2510e9dcfb770033a67b684a0ed49f86fb8a6ed93e2ccafbb4217641dS3VersionKeyD02759A5"
@aws-cdk/aws-ecs-patterns:                 [ ]     }
@aws-cdk/aws-ecs-patterns:                 [ ]   ]
@aws-cdk/aws-ecs-patterns:                 [ ] }
@aws-cdk/aws-ecs-patterns:                 @@ -21,7 +21,7 @@
@aws-cdk/aws-ecs-patterns:                 [ ]   "Fn::Split": [
@aws-cdk/aws-ecs-patterns:                 [ ]     "||",
@aws-cdk/aws-ecs-patterns:                 [ ]     {
@aws-cdk/aws-ecs-patterns:                 [-]       "Ref": "AssetParameters01b2187f99280c53b7d58040d494b5d051e1e253601fc32dee62ba56712db247S3VersionKey13E25E1F"
@aws-cdk/aws-ecs-patterns:                 [+]       "Ref": "AssetParameters32949dc2510e9dcfb770033a67b684a0ed49f86fb8a6ed93e2ccafbb4217641dS3VersionKeyD02759A5"
@aws-cdk/aws-ecs-patterns:                 [ ]     }
@aws-cdk/aws-ecs-patterns:                 [ ]   ]
@aws-cdk/aws-ecs-patterns:                 [ ] }

Can you please update the expectations in @aws-cdk/aws-ecs-patterns? Thanks!

skinny85 · 2020-03-31T19:40:01Z

...ws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/lib/index.js

+          acc[cur.Name] = cur;
+          return acc;
+        }, {});
+      records = Object.keys(unique).sort().map((key) => unique[key]);


I believe the parens around key are not required, so this can be: .map(key => unique[key]).

You are right, they are not needed. Omitting them is usually considered bad coding style though, but that's probably arguable and since it's your rules here I'm happy to change it ;)

…g enough fixes aws#5831 changed parameter references

Pull request has been modified.

aws-cdk-automation · 2020-03-31T21:17:34Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 4120a34
Result: FAILED
Build Logs (available for 30 days)

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

…g enough fixes aws#5831 changed parameter references

aws-cdk-automation · 2020-03-31T21:45:35Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 2f92e3e
Result: FAILED
Build Logs (available for 30 days)

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

nlang · 2020-03-31T21:51:10Z

Thank you @skinny85. I made the updates as requested. Build is still failing, but I'm not sure if that is still because of my changes?

skinny85 · 2020-03-31T22:08:25Z

Thank you @skinny85. I made the updates as requested. Build is still failing, but I'm not sure if that is still because of my changes?

Nope, still failing in @aws-cdk/aws-ecs-patterns:

@aws-cdk/aws-ecs-patterns: Verifying fargate/integ.load-balanced-fargate-service.js against fargate/integ.load-balanced-fargate-service.expected.json ... CHANGED.
@aws-cdk/aws-ecs-patterns: Parameters
@aws-cdk/aws-ecs-patterns: [-] Parameter AssetParameters01b2187f99280c53b7d58040d494b5d051e1e253601fc32dee62ba56712db247ArtifactHashFB4438F1: {"Type":"String","Description":"Artifact hash for asset \"01b2187f99280c53b7d58040d494b5d051e1e253601fc32dee62ba56712db247\""}
@aws-cdk/aws-ecs-patterns: [+] Parameter AssetParameters19e461d2ff1a5b90438fed6ceee4c197d7efee8712a6f76d85b501ab20bfb1a2ArtifactHash652C125C: {"Type":"String","Description":"Artifact hash for asset \"19e461d2ff1a5b90438fed6ceee4c197d7efee8712a6f76d85b501ab20bfb1a2\""}
@aws-cdk/aws-ecs-patterns: [~] Parameter AssetParameters19e461d2ff1a5b90438fed6ceee4c197d7efee8712a6f76d85b501ab20bfb1a2S3BucketFCCD3A76: {"Type":"String","Description":"S3 bucket for asset \"01b2187f99280c53b7d58040d494b5d051e1e253601fc32dee62ba56712db247\""} to {"Type":"String","Description":"S3 bucket for asset \"19e461d2ff1a5b90438fed6ceee4c197d7efee8712a6f76d85b501ab20bfb1a2\""}
@aws-cdk/aws-ecs-patterns: [~] Parameter AssetParameters19e461d2ff1a5b90438fed6ceee4c197d7efee8712a6f76d85b501ab20bfb1a2S3VersionKey07AF06B6: {"Type":"String","Description":"S3 key for asset version \"01b2187f99280c53b7d58040d494b5d051e1e253601fc32dee62ba56712db247\""} to {"Type":"String","Description":"S3 key for asset version \"19e461d2ff1a5b90438fed6ceee4c197d7efee8712a6f76d85b501ab20bfb1a2\""}

(BTW, you have access to the build log - it is posted as a comment on this PR once it finishes, for example here's a comment for your last build)

…g enough fixes aws#5831 changed parameter references

aws-cdk-automation · 2020-03-31T23:15:22Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 8ff9932
Result: SUCCEEDED
Build Logs (available for 30 days)

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

nlang · 2020-04-01T08:14:25Z

@skinny85 ok, so now the build went through :)
Thanks a bunch!

skinny85

Awesome, thanks @nlang !

mergify · 2020-04-01T21:42:27Z

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 · 2020-04-01T22:43:09Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 09f27fc
Result: SUCCEEDED
Build Logs (available for 30 days)

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

mergify · 2020-04-01T22:43:29Z

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

fix(acm-certificatemanager): add support for DNS validation of certif…

95f4aac

…icates with `SubjectAlternativeNames` fixes aws#4659

Merge remote-tracking branch 'upstream/master'

3839efd

SomayaB assigned skinny85 Mar 30, 2020

skinny85 previously requested changes Mar 31, 2020

View reviewed changes

fix(acm-certificatemanager): DnsValidatedCertificate doesn't wait lon…

4120a34

…g enough fixes aws#5831 changed parameter references

fix(acm-certificatemanager): DnsValidatedCertificate doesn't wait lon…

2f92e3e

…g enough fixes aws#5831 changed parameter references

fix(acm-certificatemanager): DnsValidatedCertificate doesn't wait lon…

8ff9932

…g enough fixes aws#5831 changed parameter references

skinny85 approved these changes Apr 1, 2020

View reviewed changes

Merge branch 'master' into master

09f27fc

mergify bot merged commit a711c01 into aws:master Apr 1, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(acm-certificatemanager): DnsValidatedCertificateHandler support for `SubjectAlternativeNames` #7050

fix(acm-certificatemanager): DnsValidatedCertificateHandler support for `SubjectAlternativeNames` #7050

nlang commented Mar 27, 2020 •

edited

Loading

aws-cdk-automation commented Mar 27, 2020

aws-cdk-automation commented Mar 28, 2020

skinny85 left a comment

skinny85 Mar 31, 2020

nlang Mar 31, 2020

aws-cdk-automation commented Mar 31, 2020

aws-cdk-automation commented Mar 31, 2020

nlang commented Mar 31, 2020

skinny85 commented Mar 31, 2020 •

edited

Loading

aws-cdk-automation commented Mar 31, 2020

nlang commented Apr 1, 2020

skinny85 left a comment

mergify bot commented Apr 1, 2020

aws-cdk-automation commented Apr 1, 2020

mergify bot commented Apr 1, 2020

fix(acm-certificatemanager): DnsValidatedCertificateHandler support for SubjectAlternativeNames #7050

fix(acm-certificatemanager): DnsValidatedCertificateHandler support for SubjectAlternativeNames #7050

Conversation

nlang commented Mar 27, 2020 • edited Loading

Commit Message

End Commit Message

aws-cdk-automation commented Mar 27, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Mar 28, 2020

AWS CodeBuild CI Report

skinny85 left a comment

Choose a reason for hiding this comment

skinny85 Mar 31, 2020

Choose a reason for hiding this comment

nlang Mar 31, 2020

Choose a reason for hiding this comment

aws-cdk-automation commented Mar 31, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented Mar 31, 2020

AWS CodeBuild CI Report

nlang commented Mar 31, 2020

skinny85 commented Mar 31, 2020 • edited Loading

aws-cdk-automation commented Mar 31, 2020

AWS CodeBuild CI Report

nlang commented Apr 1, 2020

skinny85 left a comment

Choose a reason for hiding this comment

mergify bot commented Apr 1, 2020

aws-cdk-automation commented Apr 1, 2020

AWS CodeBuild CI Report

mergify bot commented Apr 1, 2020

fix(acm-certificatemanager): DnsValidatedCertificateHandler support for `SubjectAlternativeNames` #7050

fix(acm-certificatemanager): DnsValidatedCertificateHandler support for `SubjectAlternativeNames` #7050

nlang commented Mar 27, 2020 •

edited

Loading

skinny85 commented Mar 31, 2020 •

edited

Loading