Skip to content

Commit

Permalink
fix(acm): DnsValidatedCertificate intermittently fails with "Cannot r…
Browse files Browse the repository at this point in the history
…ead 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*
  • Loading branch information
njlynch authored Dec 16, 2021
1 parent ed19828 commit 2b6c2da
Show file tree
Hide file tree
Showing 2 changed files with 123 additions and 1 deletion.
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ describe('DNS Validated Certificate Handler', () => {
handler.resetSleep();
handler.resetMaxAttempts();
AWS.restore();
nock.cleanAll();
console.log = origLog;
spySleep.resetHistory();
});
Expand Down Expand Up @@ -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({
Expand Down

0 comments on commit 2b6c2da

Please sign in to comment.