fix(core): ACM Import remove race condition (#77)

Removes a race condition where certificates would attempt to be deleted while ACM thinks the certificates are still in use.
aws · Aug 24, 2020 · ac6b419 · ac6b419
1 parent 39c3b07
commit ac6b419
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 1 deletion.
diff --git a/packages/aws-rfdk/lib/core/lambdas/nodejs/x509-certificate/acm-handlers.ts b/packages/aws-rfdk/lib/core/lambdas/nodejs/x509-certificate/acm-handlers.ts
@@ -19,6 +19,10 @@ import {
   implementsIAcmImportCertProps,
 } from './types';
 
+const defaultSleep = function(ms: number) {
+  return new Promise(resolve => setTimeout(resolve, ms));
+};
+
 const ACM_VERSION = '2015-12-08';
 const DYNAMODB_VERSION = '2012-08-10';
 const SECRETS_MANAGER_VERSION = '2017-10-17';
@@ -76,9 +80,34 @@ export class AcmCertificateImporter extends DynamoBackedCustomResource {
       this.databasePermissionsCheck(resourceTable),
     ]);
     const resources = await resourceTable.query(physicalId);
+
+    const maxAttempts = 10;
     for (const [key, resource] of Object.entries(resources)) {
-      console.log(`Deleting resource for '${key}'`);
       const arn: string = resource.ARN;
+
+      let inUseByResources = [];
+      for (let attempt = 0; attempt < maxAttempts; attempt++) {
+        const { Certificate: cert } = await this.acmClient.describeCertificate({
+          CertificateArn: arn,
+        }).promise();
+
+        inUseByResources = cert!.InUseBy || [];
+
+        if (inUseByResources.length) {
+          // Exponential backoff with jitter based on 200ms base
+          // component of backoff fixed to ensure minimum total wait time on
+          // slow targets.
+          const base = Math.pow(2, attempt);
+          await defaultSleep(Math.random() * base * 50 + base * 150);
+        } else {
+          break;
+        }
+      }
+
+      if (inUseByResources.length) {
+        throw new Error(`Response from describeCertificate did not contain an empty InUseBy list after ${maxAttempts} attempts.`);
+      }
+      console.log(`Deleting resource for '${key}'`);
       try {
         await this.acmClient.deleteCertificate({ CertificateArn: arn }).promise();
       } catch (e) {

diff --git a/packages/aws-rfdk/lib/core/lib/imported-acm-certificate.ts b/packages/aws-rfdk/lib/core/lib/imported-acm-certificate.ts
@@ -159,6 +159,7 @@ export class ImportedAcmCertificate extends Construct implements ICertificate {
     lambda.addToRolePolicy(new PolicyStatement({
       actions: [
         'acm:DeleteCertificate',
+        'acm:DescribeCertificate',
         'acm:GetCertificate',
       ],
       resources: ['*'],

diff --git a/packages/aws-rfdk/lib/core/test/imported-acm-certificate.test.ts b/packages/aws-rfdk/lib/core/test/imported-acm-certificate.test.ts
@@ -129,6 +129,7 @@ test('Import cert', () => {
         {
           Action: [
             'acm:DeleteCertificate',
+            'acm:DescribeCertificate',
             'acm:GetCertificate',
           ],
           Resource: '*',