From 35a7dd15341b87c1fce1ad4f2a7a630719b4dbea Mon Sep 17 00:00:00 2001 From: Nick Lynch Date: Thu, 16 Dec 2021 14:18:09 +0000 Subject: [PATCH] fix(acm): DnsValidatedCertificate intermittently fails with "Cannot read property 'Name' of undefined" (#18033) There have been about a dozen reports of "Cannot read property 'Name' of undefined" errors from the `DnsValidatedCertificate` over the last two years. The most likely culprit seems to be a partial response from the ACM DescribeCertificates API, where one ResourceRecord entry is present, but not the others. Updated the wait condition to verify that all records are present. fixes #8282 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license* --- .../lib/index.js | 3 +- .../test/handler.test.js | 121 ++++++++++++++++++ 2 files changed, 123 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/lib/index.js b/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/lib/index.js index d34f7922f4e7e..528dd55eb460a 100644 --- a/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/lib/index.js +++ b/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/lib/index.js @@ -116,7 +116,8 @@ const requestCertificate = async function (requestId, domainName, subjectAlterna CertificateArn: reqCertResponse.CertificateArn }).promise(); const options = Certificate.DomainValidationOptions || []; - if (options.length > 0 && options[0].ResourceRecord) { + // Ensure all records are ready; there is (at least a theory there's) a chance of a partial response here in rare cases. + if (options.length > 0 && options.every(opt => opt && !!opt.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 diff --git a/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/test/handler.test.js b/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/test/handler.test.js index 5b53bc0a46301..9f180f20211e9 100644 --- a/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/test/handler.test.js +++ b/packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/test/handler.test.js @@ -47,6 +47,7 @@ describe('DNS Validated Certificate Handler', () => { handler.resetSleep(); handler.resetMaxAttempts(); AWS.restore(); + nock.cleanAll(); console.log = origLog; spySleep.resetHistory(); }); @@ -380,6 +381,126 @@ describe('DNS Validated Certificate Handler', () => { }); }); + test('Create operation with `SubjectAlternativeNames` gracefully handles partial results from DescribeCertificate', () => { + const requestCertificateFake = sinon.fake.resolves({ + CertificateArn: testCertificateArn, + }); + + const describeCertificateFake = sinon.stub(); + describeCertificateFake.onFirstCall().resolves({ + Certificate: { + CertificateArn: testCertificateArn, + DomainValidationOptions: [ + { + ValidationStatus: 'PENDING_VALIDATION', + ResourceRecord: { + Name: testRRName, + Type: 'CNAME', + Value: testRRValue + } + }, + { + ValidationStatus: 'PENDING_VALIDATION', + }, + ] + } + }); + describeCertificateFake.resolves({ + Certificate: { + CertificateArn: testCertificateArn, + DomainValidationOptions: [ + { + ValidationStatus: 'SUCCESS', + ResourceRecord: { + Name: testRRName, + Type: 'CNAME', + Value: testRRValue + } + }, + { + ValidationStatus: 'SUCCESS', + ResourceRecord: { + Name: testAltRRName, + Type: 'CNAME', + Value: testAltRRValue + } + }, + ] + } + }); + + const addTagsToCertificateFake = sinon.fake.resolves({ + Certificate: testCertificateArn, + Tags: testTags, + }); + + const changeResourceRecordSetsFake = sinon.fake.resolves({ + ChangeInfo: { + Id: 'bogus' + } + }); + + AWS.mock('ACM', 'requestCertificate', requestCertificateFake); + AWS.mock('ACM', 'describeCertificate', describeCertificateFake); + AWS.mock('ACM', 'addTagsToCertificate', addTagsToCertificateFake); + AWS.mock('Route53', 'changeResourceRecordSets', changeResourceRecordSetsFake); + + const request = nock(ResponseURL).put('/', body => { + return body.Status === 'SUCCESS'; + }).reply(200); + + return LambdaTester(handler.certificateRequestHandler) + .event({ + RequestType: 'Create', + RequestId: testRequestId, + ResourceProperties: { + DomainName: testDomainName, + HostedZoneId: testHostedZoneId, + Region: 'us-east-1', + Tags: testTags, + } + }) + .expectResolve(() => { + sinon.assert.calledWith(requestCertificateFake, sinon.match({ + DomainName: testDomainName, + ValidationMethod: 'DNS' + })); + sinon.assert.calledWith(changeResourceRecordSetsFake, sinon.match({ + ChangeBatch: { + Changes: [ + { + Action: 'UPSERT', + ResourceRecordSet: { + Name: testRRName, + Type: 'CNAME', + TTL: 60, + ResourceRecords: [{ + Value: testRRValue + }] + } + }, { + Action: 'UPSERT', + ResourceRecordSet: { + Name: testAltRRName, + Type: 'CNAME', + TTL: 60, + ResourceRecords: [{ + Value: testAltRRValue + }] + } + } + ] + }, + HostedZoneId: testHostedZoneId + })); + sinon.assert.calledWith(addTagsToCertificateFake, sinon.match({ + "CertificateArn": testCertificateArn, + "Tags": testTagsValue, + })); + expect(request.isDone()).toBe(true); + }); + }); + test('Create operation fails after more than 60s if certificate has no DomainValidationOptions', () => { handler.withRandom(() => 0); const requestCertificateFake = sinon.fake.resolves({