fix(DnsValidatedCertificate): add support for subjectAlternativeNames by touzoku · Pull Request #6516 · aws/aws-cdk

touzoku · 2020-02-29T04:47:18Z

Fix requestCertificate function in packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/lib/index.js
Add a test to packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/test/handler.test.js
Run eslint --fix (sorry for a messy PR, but why the code was not adhering to .eslintrc.js rules in the first place?)
Bumped up node runtime for the certificate requestor Lambda function to lambda.Runtime.NODEJS_12_X in packages/@aws-cdk/aws-certificatemanager/lib/dns-validated-certificate.ts

Fixes #4659

cc @skinny85

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

…urce

… eslint --fix

aws-cdk-automation · 2020-02-29T05:01:08Z

AWS CodeBuild CI Report

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

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

… version

aws-cdk-automation · 2020-02-29T06:34:35Z

AWS CodeBuild CI Report

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

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

aws-cdk-automation · 2020-02-29T15:32:56Z

AWS CodeBuild CI Report

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

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

aws-cdk-automation · 2020-03-01T07:04:15Z

AWS CodeBuild CI Report

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

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

aws-cdk-automation · 2020-03-02T03:53:10Z

AWS CodeBuild CI Report

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

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

aws-cdk-automation · 2020-03-03T12:35:43Z

AWS CodeBuild CI Report

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

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

aws-cdk-automation · 2020-03-08T10:58:32Z

AWS CodeBuild CI Report

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

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

aws-cdk-automation · 2020-03-10T07:36:37Z

AWS CodeBuild CI Report

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

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

aws-cdk-automation · 2020-03-11T16:30:07Z

AWS CodeBuild CI Report

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

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

skinny85

Thanks for the contribution @touzoku !

But I have to say, I don't follow the change here that well. Can you write a short paragraph explaining what needed changing to support SubjectAlternativeNames? From what I can tell, code dealing with them was there already, so I'm not sure what was it that needed changing.

skinny85 · 2020-03-12T18:23:42Z

packages/@aws-cdk/aws-certificatemanager/lib/dns-validated-certificate.ts

            code: lambda.Code.fromAsset(path.resolve(__dirname, '..', 'lambda-packages', 'dns_validated_certificate_handler', 'lib')),
            handler: 'index.certificateRequestHandler',
-            runtime: lambda.Runtime.NODEJS_10_X,
+            runtime: lambda.Runtime.NODEJS_12_X,


Why this change?

Updated nodejs version to the most recent runtime, since this is the only 10.x function in my stacks now. But yes, irrelevant to the PR, side fix.

Can you bring it back to NODEJS_10_X then? I will also accept a property that allows you to set it to NODEJS_12_X (but the default should be NODEJS_10_X) 🙂.

skinny85 · 2020-03-12T18:26:04Z

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

          event.ResourceProperties.SubjectAlternativeNames,
          event.ResourceProperties.HostedZoneId,
-          event.ResourceProperties.Region,
+          event.ResourceProperties.Region


Please do not remove this trailing comma.

@skinny85 I did not! The eslint config in the package did!

skinny85 · 2020-03-12T18:27:18Z

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

+            }]
+          }
+        };
+      })


Can you explain why this change in logic is needed in this PR?

As @saltman424 mentioned below and as it is pointed out in #4659, current logic only creates a single Route53 validation record and misses creating all other records for subjectAlternativeNames.

...ws-certificatemanager/lambda-packages/dns_validated_certificate_handler/test/handler.test.js

skinny85 · 2020-03-12T18:31:28Z

...ws-certificatemanager/lambda-packages/dns_validated_certificate_handler/test/handler.test.js

+    describeCertificateFake.resolves({
+      Certificate: {
+        CertificateArn: testCertificateArn,
+        DomainValidationOptions: [{


Indent this like the following:

DomainValidationOptions: [ { ValidationStatus: 'SUCCESS', ResourceRecord: { Name: testRRName, Type: 'CNAME', Value: testRRValue, }, }, { ValidationStatus: 'SUCCESS', ResourceRecord: { Name: testAltRRName, Type: 'CNAME', Value: testAltRRValue, }, }, ],

@skinny85 I have added a rule config to the .eslintrc.json of the lambda-package as follows:

"comma-dangle": ["error", { "arrays": "always-multiline", "objects": "always-multiline", "imports": "never", "exports": "never", "functions": "never" }], "array-bracket-newline": ["error", { "multiline": true }]

And ran eslint --fix on both lib/index.js and test/handler.test.js. Let me know if this is what you want. This created a lot of mess, because test/handler.test.js did not follow the convention that you have requested that much....

Let me know if this is what you've wanted. Otherwise, tell me what the eslint config should be.

I'd rather the .eslintrc.json file not be modified, and to minimize incidental changes to the lambda code not related to the logic changes.

saltman424 · 2020-03-12T18:48:04Z

Thanks for the contribution @touzoku !

But I have to say, I don't follow the change here that well. Can you write a short paragraph explaining what needed changing to support SubjectAlternativeNames? From what I can tell, code dealing with them was there already, so I'm not sure what was it that needed changing.

@skinny85 Since I ran into the same issue and had actually also made my own fix, I can give you a quick answer. If you look at lines 110 and 127 - 137 of what is being changed by:
https://github.com/aws/aws-cdk/pull/6516/files#diff-60e5eb022ef3dcfba44e73fe7643fe7f
you can see that at line 110 the only ResourceRecord that was being kept was the one for the first domain, and thus that was the only one for which a CNAME validation record was being upserted in lines 127 - 137. What @touzoku added was logic to wait for all ResourceRecords, and then upsert all required validation records (i.e. the validation record for the first domain as well as all subjectAlternativeNames).

Long story short, the implementation before this pull request only added a single validation record for the domainName, not any of the subjectAlternativeNames. Thus, any DnsValidatedCertificates that specified subjectAlternativeNames would fail to deploy because the certificate would never be completely validated.

Pull request has been modified.

aws-cdk-automation · 2020-03-14T09:08:22Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 5efb0b2
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-14T09:26:51Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 9a26a0e
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-14T10:52:00Z

AWS CodeBuild CI Report

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

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

aws-cdk-automation · 2020-03-17T00:03:46Z

AWS CodeBuild CI Report

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

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

skinny85

@touzoku can you revert the .eslintrc.json changes?

mergify · 2020-03-25T15:52:51Z

Once all the requested changes have been addressed, and the PR is ready for another review, remember to dismiss the review.

rrrix · 2020-06-11T22:41:07Z

@skinny85 it looks like this PR will solve #8282 and #7509

I'm thinking perhaps I can fork this PR, make the requested changes (revert .eslintrc.json) and make a new PR - unless you know of a way I can update this PR directly? Are there any other changes needed?

skinny85 · 2020-06-11T22:55:21Z

@skinny85 it looks like this PR will solve #8282 and #7509

I'm thinking perhaps I can fork this PR, make the requested changes (revert .eslintrc.json) and make a new PR - unless you know of a way I can update this PR directly? Are there any other changes needed?

Yes, this PR looks quite good, and I really didn't have comments beyond the .eslintrc.json changes which make the diff so messy.

Feel free to take it over. Unfortunately, I don't think you can update this PR directly, but we can close it after yours is opened.

rrrix · 2020-06-11T23:27:01Z

I figured out why he changed the .eslintrc.json - because your requested modifications (to add trailing commas) conflicts with the existing .eslintrc.json :)

~/dev/git/aws/aws-cdk/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/lib $ eslint index.js

/Users/rbowen/dev/git/aws/aws-cdk/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/lib/index.js
   39:25  error  Unexpected trailing comma  comma-dangle
   50:46  error  Unexpected trailing comma  comma-dangle
   51:8   error  Unexpected trailing comma  comma-dangle
   95:28  error  Unexpected trailing comma  comma-dangle
  106:53  error  Unexpected trailing comma  comma-dangle
  133:55  error  Unexpected trailing comma  comma-dangle
  134:12  error  Unexpected trailing comma  comma-dangle
  136:9   error  Unexpected trailing comma  comma-dangle
  138:31  error  Unexpected trailing comma  comma-dangle
  146:22  error  Unexpected trailing comma  comma-dangle
  148:34  error  Unexpected trailing comma  comma-dangle
  156:22  error  Unexpected trailing comma  comma-dangle
  158:51  error  Unexpected trailing comma  comma-dangle
  179:28  error  Unexpected trailing comma  comma-dangle
  202:26  error  Unexpected trailing comma  comma-dangle

✖ 15 problems (15 errors, 0 warnings)
  15 errors and 0 warnings potentially fixable with the `--fix` option.

It seems to me the changes @touzoku made in .eslintrc.json (which is only for the Lambda function and its tests) are quite reasonable, given your requested formatting changes.

Shouldn't the .eslintrc.json configuration match the desired format? It will probably cause more confusion in the future if they're not aligned.

Anyways... new PR incoming! 👍

diff --git b/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/.eslintrc.json a/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/.eslintrc.json
index 12a6e88f4..483a623f6 100644
--- b/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/.eslintrc.json
+++ a/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/.eslintrc.json
@@ -1,10 +1,20 @@
 {
     "extends": "standard",
     "rules": {
-        "semi": ["error", "always"]
+        "semi": ["error", "always"],
+        "comma-dangle": ["error", {
+            "arrays": "always-multiline",
+            "objects": "always-multiline",
+            "imports": "never",
+            "exports": "never",
+            "functions": "never"
+        }],
+        "array-bracket-newline": ["error", {
+            "multiline": true
+        }]
     },
     "env": {
         "jest": true,
         "node": true
     }
-}
+}
\ No newline at end of file

skinny85 · 2020-06-12T00:53:44Z

I figured out why he changed the .eslintrc.json - because your requested modifications (to add trailing commas) conflicts with the existing .eslintrc.json :)

Then ignore my comments, and keep the style that passes ESLint :)

touzoku · 2020-06-12T07:07:01Z

I think this PR is obsolete now, somebody else fixed the issue in a different PR.

rrrix · 2020-06-12T23:11:45Z

#6516 (comment)

I think this PR is obsolete now, somebody else fixed the issue in a different PR.

Hi @touzoku! The issue isn't fixed yet (at least, for me). I still get an error when I attempt to create a DNS Validated Certificate with too many subject alternative names. Looking at the latest version of the master branch, it looks like the same problematic lambda code that you updated in this PR is still there.

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

Lines 108 to 119 in 86d84e6

    
           if (options.length > 0 && options[0].ResourceRecord) { 
        
             // some alternative names will produce the same validation record 
        
             // as the main domain (eg. example.com + *.example.com) 
        
             // filtering duplicates to avoid errors with adding the same record 
        
             // to the route53 zone twice 
        
             const unique = options 
        
               .map((val) => val.ResourceRecord) 
        
               .reduce((acc, cur) => { 
        
                 acc[cur.Name] = cur; 
        
                 return acc; 
        
               }, {}); 
        
             records = Object.keys(unique).sort().map(key => unique[key]);

rrrix · 2020-06-13T06:14:43Z

Looks like CloudFormation natively supports DNS Validation for ACM Certificates now.

aws-cloudformation/cloudformation-coverage-roadmap#31 (comment)

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/aws-properties-certificatemanager-certificate-domainvalidationoption.html#cfn-certificatemanager-certificate-domainvalidationoption-hostedzoneid

I think perhaps a better PR at this point would be to add the native CFN support to the L2 Construct :) Gets rid of one more pesky CustomResource and Lambda Function.

…icate Automatically adding Amazon Route 53 CNAME records for DNS validation is now natively supported by CloudFormation. Add a `validation` prop to `Certificate` to handle both email and DNS validation. Deprecate `DnsValidatedCertificate`. The default remains email validation (non-breaking). Closes aws#5831 Closes aws#5835 Closes aws#6081 Closes aws#6516 Closes aws#7150 Closes aws#7941 Closes aws#7995 Closes aws#7996

…cate Automatically adding Amazon Route 53 CNAME records for DNS validation is now natively supported by CloudFormation. Add a `validation` prop to `Certificate` to handle both email and DNS validation. Deprecate `DnsValidatedCertificate`. The default remains email validation (non-breaking). Closes aws#5831 Closes aws#5835 Closes aws#6081 Closes aws#6516 Closes aws#7150 Closes aws#7941 Closes aws#7995 Closes aws#7996

rrrix · 2020-06-17T01:03:12Z

The "native" (a.k.a. CloudFormation) DNS validated certificate totally worked with SAN's and everything, first try, boom!

Here's how for anyone looking for a workaround instead of the above DnsValidatedCertificate custom Lambda fix.

import { Certificate } from '@aws-cdk/aws-certificatemanager';
import { HostedZone } from '@aws-cdk/aws-route53';
import { CfnResource, Lazy } from '@aws-cdk/core';

// ... snip

    this.hostedZone = HostedZone.fromLookup(this, 'hostedZone', {
      domainName: this.envDomainName,
    });

    this.cfnCertificate = new CfnResource(this, 'cfnCertificate', {
      type: 'AWS::CertificateManager::Certificate',
      properties: {
        DomainName: this.envDomainName,
        SubjectAlternativeNames: [`*.${this.envDomainName}`],
        ValidationMethod: 'DNS',
        DomainValidationOptions: [
          {
            DomainName: this.envDomainName,
            HostedZoneId: this.hostedZone.hostedZoneId,
          },
        ],
      },
    });

    this.tlsCertificate = Certificate.fromCertificateArn(
      this,
      'certificateArn',
      Lazy.stringValue({ produce: () => this.cfnCertificate.ref }),
    );

…cate (#8552) Automatically adding Amazon Route 53 CNAME records for DNS validation is now natively supported by CloudFormation. Add a `validation` prop to `Certificate` to handle both email and DNS validation. `DnsValidatedCertificate` is now only useful for cross-region certificate creation. The default remains email validation (non-breaking). Closes #5831 Closes #5835 Closes #6081 Closes #6516 Closes #7150 Closes #7941 Closes #7995 Closes #7996 Closes #8282 Closes #8659 Closes #8783 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Marat Vyshegorodtsev added 2 commits February 29, 2020 13:34

chore(aws-certificatemanager) bump nodejs runtime for the custom reso…

ea26e98

…urce

fix(aws-certificatemanager): add SubjectAlternativeNames support, run…

36813b1

… eslint --fix

touzoku mentioned this pull request Feb 29, 2020

aws-certificatemanager DnsValidatedCertificateHandler does not properly handle certs with SubjectAlternativeNames #4659

Closed

chore(aws-certificatemanager): update test for bumped up node runtime…

ef7a4d1

… version

Marat Vyshegorodtsev added 2 commits February 29, 2020 23:46

fix(aws-ecs-patterns): update fixture due to acm custom resource change

8da1426

fix(acm-certificatemanager): checks that all ResourceRecords are present

3eed922

Merge branch 'master' into master

b4a6b14

Merge branch 'master' into master

d00f289

SomayaB assigned skinny85 Mar 2, 2020

Merge branch 'master' into master

6434da3

Merge branch 'master' into master

b9f9150

Merge branch 'master' into master

bdfd786

Merge branch 'master' into master

b4f9499

skinny85 changed the title ~~fix(DnsValidatedCertificate): add support for subjectAlternativeNames Fixes #4659~~ fix(DnsValidatedCertificate): add support for subjectAlternativeNames Mar 12, 2020

skinny85 previously requested changes Mar 12, 2020

View reviewed changes

Align dangling commas rule with @skinny85's requirements

5efb0b2

Marat Vyshegorodtsev added 2 commits March 14, 2020 18:04

Merge remote-tracking branch 'upstream/master'

d0cdd60

Fix dangling commas and arrays style per @skinny85's comment

9a26a0e

Fix asset hashes in aws-ecs-patterns

b23c77b

Merge branch 'master' into master

9f342b1

skinny85 suggested changes Mar 19, 2020

View reviewed changes

Dzhuneyt approved these changes Mar 19, 2020

View reviewed changes

rrrix mentioned this pull request Jun 11, 2020

DNS Validated Certificate Error: Failed to create resource. Cannot read property 'Name' of undefined #8282

Closed

jogold mentioned this pull request Jun 15, 2020

feat(certificatemanager): native CloudFormation DNS validated certificate #8552

Merged

mergify bot closed this in #8552 Jul 10, 2020

Conversation

touzoku commented Feb 29, 2020 • edited by skinny85 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aws-cdk-automation commented Feb 29, 2020

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Feb 29, 2020

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Feb 29, 2020

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Mar 1, 2020

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Mar 2, 2020

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Mar 3, 2020

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Mar 8, 2020

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Mar 10, 2020

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Mar 11, 2020

AWS CodeBuild CI Report

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

touzoku Mar 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

saltman424 commented Mar 12, 2020

Uh oh!

aws-cdk-automation commented Mar 14, 2020

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Mar 14, 2020

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Mar 14, 2020

AWS CodeBuild CI Report

Uh oh!

aws-cdk-automation commented Mar 17, 2020

AWS CodeBuild CI Report

Uh oh!

skinny85 left a comment

Choose a reason for hiding this comment

Uh oh!

mergify bot commented Mar 25, 2020

Uh oh!

rrrix commented Jun 11, 2020

Uh oh!

skinny85 commented Jun 11, 2020

touzoku commented Feb 29, 2020 •

edited by skinny85

Loading

touzoku Mar 14, 2020 •

edited

Loading