fix(certificatemanager): increase minimum validation total timeout (#3914)

* fix(certificatemanager): add minimum backoff

Add a minimum component to the backoff and retry timer

* fix(certificatemanager): stub out sleep

Provide stub function for sleep to allow long running sleep to
cause test to fail

* fix(certificatemanager): spy on sleep

spy on sleep function to determine total time attempted to sleep

* fix(certificatemanager): Failing test

Failing test demonstrating min sleep is as low as 4.6 seconds.

* 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

Author: elhedran

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

@@ -2,13 +2,15 @@
 
 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;
+let random = Math.random;
 
 /**
  * Upload a CloudFormation response object to S3.
@@ -97,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
@@ -108,7 +110,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(random() * base * 50 + base * 150);
     }
   }
   if (!record) {
@@ -248,3 +253,31 @@ exports.withWaiter = function (w) {
 exports.resetWaiter = function () {
   waiter = undefined;
 };
+
+/**
+ * @private
+ */
+exports.withSleep = function(s) {
+  sleep = s;
+}
+
+/**
+ * @private
+ */
+exports.resetSleep = function() {
+  sleep = defaultSleep;
+}
+
+/**
+ * @private
+ */
+exports.withRandom = function(r) {
+  random = r;
+}
+
+/**
+ * @private
+ */
+exports.resetRandom = function() {
+  random = Math.random;
+}

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,13 +31,16 @@ describe('DNS Validated Certificate Handler', () => {
         }
       };
     });
+    handler.withSleep(spySleep);
     console.log = function () { };
   });
   afterEach(() => {
     // Restore waiters and logger
     handler.resetWaiter();
+    handler.resetSleep();
     AWS.restore();
     console.log = origLog;
+    spySleep.resetHistory();
   });
 
   test('Fails if the event payload is empty', () => {
@@ -171,9 +177,94 @@ describe('DNS Validated Certificate Handler', () => {
           ValidationMethod: 'DNS'
         }));
         expect(request.isDone()).toBe(true);
+        expect(spySleep.callCount).toBeLessThan(10);
+      });
+  });
+
+  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);