From e9558cb7ca22e5dc5d34521b36017873770e3a8d Mon Sep 17 00:00:00 2001 From: Ian Walters Date: Tue, 3 Sep 2019 20:51:54 +1000 Subject: [PATCH 1/5] fix(certificatemanager): add minimum backoff Add a minimum component to the backoff and retry timer --- .../dns_validated_certificate_handler/lib/index.js | 5 ++++- 1 file changed, 4 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 5fd298b21e5fa..89be821ddb0ec 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 @@ -108,7 +108,10 @@ const requestCertificate = async function (requestId, domainName, subjectAlterna record = options[0].ResourceRecord; } else { // Exponential backoff with jitter based on 200ms base - await sleep(Math.random() * (Math.pow(2, attempt) * 200)); + // component of backoff fixed to ensure minimum total wait time on + // slow targets. + const base = Math.pow(2, attempt); + await sleep(Math.random() * base * 50 + base * 150); } } if (!record) { From f0cc195935215f9125f5599c52f430e552a056e5 Mon Sep 17 00:00:00 2001 From: Ian Walters Date: Tue, 3 Sep 2019 21:10:50 +1000 Subject: [PATCH 2/5] fix(certificatemanager): stub out sleep Provide stub function for sleep to allow long running sleep to cause test to fail --- .../lib/index.js | 17 ++++++++++++++++- .../test/handler.test.js | 2 ++ 2 files changed, 18 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 89be821ddb0ec..1392e17338cf9 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 @@ -2,13 +2,14 @@ const aws = require('aws-sdk'); -const sleep = function (ms) { +const defaultSleep = function (ms) { return new Promise(resolve => setTimeout(resolve, ms)); }; // These are used for test purposes only let defaultResponseURL; let waiter; +let sleep = defaultSleep; /** * Upload a CloudFormation response object to S3. @@ -251,3 +252,17 @@ exports.withWaiter = function (w) { exports.resetWaiter = function () { waiter = undefined; }; + +/** + * @private + */ +exports.withSleep = function(s) { + sleep = s; +} + +/** + * @private + */ +exports.resetSleep = function() { + sleep = defaultSleep; +} \ No newline at end of file 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 66c254ae295df..12bd52b684217 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 @@ -28,11 +28,13 @@ describe('DNS Validated Certificate Handler', () => { } }; }); + handler.withSleep(() => Promise.resolve()); console.log = function () { }; }); afterEach(() => { // Restore waiters and logger handler.resetWaiter(); + handler.resetSleep(); AWS.restore(); console.log = origLog; }); From 1bfdbb2024154213aa51ca5c76f9d47c3a318b3f Mon Sep 17 00:00:00 2001 From: Ian Walters Date: Tue, 3 Sep 2019 21:26:19 +1000 Subject: [PATCH 3/5] fix(certificatemanager): spy on sleep spy on sleep function to determine total time attempted to sleep --- .../dns_validated_certificate_handler/test/handler.test.js | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) 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 12bd52b684217..84cc678b3166c 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 @@ -15,6 +15,9 @@ describe('DNS Validated Certificate Handler', () => { const testCertificateArn = 'arn:aws:acm:region:123456789012:certificate/12345678-1234-1234-1234-123456789012'; const testRRName = '_3639ac514e785e898d2646601fa951d5.example.com'; const testRRValue = '_x2.acm-validations.aws'; + const spySleep = sinon.spy(function(ms) { + return Promise.resolve(); + }); beforeEach(() => { handler.withDefaultResponseURL(ResponseURL); @@ -28,7 +31,7 @@ describe('DNS Validated Certificate Handler', () => { } }; }); - handler.withSleep(() => Promise.resolve()); + handler.withSleep(spySleep); console.log = function () { }; }); afterEach(() => { @@ -37,6 +40,7 @@ describe('DNS Validated Certificate Handler', () => { handler.resetSleep(); AWS.restore(); console.log = origLog; + spySleep.resetHistory(); }); test('Fails if the event payload is empty', () => { @@ -173,6 +177,7 @@ describe('DNS Validated Certificate Handler', () => { ValidationMethod: 'DNS' })); expect(request.isDone()).toBe(true); + expect(spySleep.callCount).toBeLessThan(10); }); }); From 2115b52084fd976ab5004fc98318481c1f4ea594 Mon Sep 17 00:00:00 2001 From: Ian Walters Date: Tue, 3 Sep 2019 21:34:37 +1000 Subject: [PATCH 4/5] fix(certificatemanager): Failing test Failing test demonstrating min sleep is as low as 4.6 seconds. --- .../lib/index.js | 17 +++- .../test/handler.test.js | 84 +++++++++++++++++++ 2 files changed, 100 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 1392e17338cf9..f530e8b37b099 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 @@ -10,6 +10,7 @@ const defaultSleep = function (ms) { let defaultResponseURL; let waiter; let sleep = defaultSleep; +let random = Math.random; /** * Upload a CloudFormation response object to S3. @@ -112,7 +113,7 @@ const requestCertificate = async function (requestId, domainName, subjectAlterna // component of backoff fixed to ensure minimum total wait time on // slow targets. const base = Math.pow(2, attempt); - await sleep(Math.random() * base * 50 + base * 150); + await sleep(random() * base * 50 + base * 150); } } if (!record) { @@ -265,4 +266,18 @@ exports.withSleep = function(s) { */ exports.resetSleep = function() { sleep = defaultSleep; +} + +/** + * @private + */ +exports.withRandom = function(r) { + random = r; +} + +/** + * @private + */ +exports.resetRandom = function() { + random = Math.random; } \ No newline at end of file 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 84cc678b3166c..f29ceb41bfc8f 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 @@ -181,6 +181,90 @@ describe('DNS Validated Certificate Handler', () => { }); }); + test('Fails after at more than 60 seconds', () => { + handler.withRandom(() => 0); + const requestCertificateFake = sinon.fake.resolves({ + CertificateArn: testCertificateArn, + }); + + const describeCertificateFake = sinon.fake.resolves({ + CertificateArn: testCertificateArn, + Certificate: { + } + }); + + AWS.mock('ACM', 'requestCertificate', requestCertificateFake); + AWS.mock('ACM', 'describeCertificate', describeCertificateFake); + + const request = nock(ResponseURL).put('/', body => { + return body.Status === 'FAILED' && + body.Reason.startsWith('Response from describeCertificate did not contain DomainValidationOptions'); + }).reply(200); + + return LambdaTester(handler.certificateRequestHandler) + .event({ + RequestType: 'Create', + RequestId: testRequestId, + ResourceProperties: { + DomainName: testDomainName, + HostedZoneId: testHostedZoneId, + Region: 'us-east-1', + } + }) + .expectResolve(() => { + sinon.assert.calledWith(requestCertificateFake, sinon.match({ + DomainName: testDomainName, + ValidationMethod: 'DNS' + })); + expect(request.isDone()).toBe(true); + const totalSleep = spySleep.getCalls().map(call => call.args[0]).reduce((p, n) => p + n, 0); + expect(totalSleep).toBeGreaterThan(60 * 1000); + }); + }); + + test('Fails after at less than 360 seconds', () => { + handler.withRandom(() => 1); + const requestCertificateFake = sinon.fake.resolves({ + CertificateArn: testCertificateArn, + }); + + const describeCertificateFake = sinon.fake.resolves({ + CertificateArn: testCertificateArn, + Certificate: { + } + }); + + AWS.mock('ACM', 'requestCertificate', requestCertificateFake); + AWS.mock('ACM', 'describeCertificate', describeCertificateFake); + + const request = nock(ResponseURL).put('/', body => { + return body.Status === 'FAILED' && + body.Reason.startsWith('Response from describeCertificate did not contain DomainValidationOptions'); + }).reply(200); + + return LambdaTester(handler.certificateRequestHandler) + .event({ + RequestType: 'Create', + RequestId: testRequestId, + ResourceProperties: { + DomainName: testDomainName, + HostedZoneId: testHostedZoneId, + Region: 'us-east-1', + } + }) + .expectResolve(() => { + sinon.assert.calledWith(requestCertificateFake, sinon.match({ + DomainName: testDomainName, + ValidationMethod: 'DNS' + })); + expect(request.isDone()).toBe(true); + expect(spySleep.callCount).toBeLessThan(10); + const totalSleep = spySleep.getCalls().map(call => call.args[0]).reduce((p, n) => p + n, 0); + expect(totalSleep).toBeLessThan(360 *1000); + }); + }); + + test('Deletes a certificate if RequestType is Delete', () => { const deleteCertificateFake = sinon.fake.resolves({}); AWS.mock('ACM', 'deleteCertificate', deleteCertificateFake); From 98e0e1c401d4e6e781ac36d331d5ca70644282bb Mon Sep 17 00:00:00 2001 From: Ian Walters Date: Tue, 3 Sep 2019 21:35:23 +1000 Subject: [PATCH 5/5] fix(certificatemanager): increase attempts Increase max attempts in validation, still under 10, but such that minimum sleep duration of one minute is reached (but not more than 3 minutes). Can adjust tests if window is not suitable, but I find that at least 30 seconds is needed in my region --- .../dns_validated_certificate_handler/lib/index.js | 2 +- 1 file changed, 1 insertion(+), 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 f530e8b37b099..ae0115ddee4d6 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 @@ -99,7 +99,7 @@ const requestCertificate = async function (requestId, domainName, subjectAlterna console.log('Waiting for ACM to provide DNS records for validation...'); let record; - const maxAttempts = 6; + const maxAttempts = 10; for (let attempt = 0; attempt < maxAttempts - 1 && !record; attempt++) { const { Certificate } = await acm.describeCertificate({ CertificateArn: reqCertResponse.CertificateArn