fix(certificatemanager): unable to set removal policy on DnsValidated…

…Certificate (aws#22122)

This PR adds a method override for applyRemovalPolicy which allows the user to specify a removal policy for the DnsValidatedCertificate construct. Since this construct is backed by a custom resource, the lambda handler was updated to no longer delete the certificate if the RemovalPolicy is set to retain.

This is also needed to allow for an easier migration from DnsValidatedCertificate -> Certificate

reroll of aws#22040
This has the same changes as aws#22040 with the addition of some logic to handle only processing updates for certain parameters. If `RemovalPolicy` is changed for example, the update will not be processed.

I also added an integration test with some manual instructions. In order to test ACM certificates I also updated the integ-runner to handle some additional special env variables.

fixes aws#20649, fixes aws#14519


----

### All Submissions:

* [ ] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [ ] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [ ] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

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

@@ -65,6 +65,23 @@ let report = function (event, context, responseStatus, physicalResourceId, respo
   });
 };
 
+/**
+ * Adds tags to an existing certificate
+ *
+ * @param {string} certificateArn the ARN of the certificate to add tags to
+ * @param {string} region the region the certificate exists in
+ * @param {map} tags Tags to add to the requested certificate
+ */
+const addTags = async function(certificateArn, region, tags) {
+  const result = Array.from(Object.entries(tags)).map(([Key, Value]) => ({ Key, Value }))
+  const acm = new aws.ACM({ region });
+
+  await acm.addTagsToCertificate({
+    CertificateArn: certificateArn,
+    Tags: result,
+  }).promise();
+}
+
 /**
  * Requests a public certificate from AWS Certificate Manager, using DNS validation.
  * The hosted zone ID must refer to a **public** Route53-managed DNS zone that is authoritative
@@ -75,10 +92,9 @@ let report = function (event, context, responseStatus, physicalResourceId, respo
  * @param {string} requestId the CloudFormation request ID
  * @param {string} domainName the Common Name (CN) field for the requested certificate
  * @param {string} hostedZoneId the Route53 Hosted Zone ID
- * @param {map} tags Tags to add to the requested certificate
  * @returns {string} Validated certificate ARN
  */
-const requestCertificate = async function (requestId, domainName, subjectAlternativeNames, certificateTransparencyLoggingPreference, hostedZoneId, region, route53Endpoint, tags) {
+const requestCertificate = async function (requestId, domainName, subjectAlternativeNames, certificateTransparencyLoggingPreference, hostedZoneId, region, route53Endpoint) {
   const crypto = require('crypto');
   const acm = new aws.ACM({ region });
   const route53 = route53Endpoint ? new aws.Route53({ endpoint: route53Endpoint }) : new aws.Route53();
@@ -101,16 +117,6 @@ const requestCertificate = async function (requestId, domainName, subjectAlterna
 
   console.log(`Certificate ARN: ${reqCertResponse.CertificateArn}`);
 
-
-  if (!!tags) {
-    const result = Array.from(Object.entries(tags)).map(([Key, Value]) => ({ Key, Value }))
-
-    await acm.addTagsToCertificate({
-      CertificateArn: reqCertResponse.CertificateArn,
-      Tags: result,
-    }).promise();
-  }
-
   console.log('Waiting for ACM to provide DNS records for validation...');
 
   let records = [];
@@ -275,35 +281,69 @@ async function commitRoute53Records(route53, records, hostedZoneId, action = 'UP
   }).promise();
 }
 
+/**
+ * Determines whether an update request should request a new certificate
+ *
+ * @param {map} oldParams the previously process request parameters
+ * @param {map} newParams the current process request parameters
+ * @param {string} physicalResourceId the physicalResourceId
+ * @returns {boolean} whether or not to request a new certificate
+ */
+function shouldUpdate(oldParams, newParams, physicalResourceId) {
+  if (!oldParams) return true;
+  if (oldParams.DomainName !== newParams.DomainName) return true;
+  if (oldParams.SubjectAlternativeNames !== newParams.SubjectAlternativeNames) return true;
+  if (oldParams.CertificateTransparencyLoggingPreference !== newParams.CertificateTransparencyLoggingPreference) return true;
+  if (oldParams.HostedZoneId !== newParams.HostedZoneId) return true;
+  if (oldParams.Region !== newParams.Region) return true;
+  if (!physicalResourceId || !physicalResourceId.startsWith('arn:')) return true;
+  return false;
+}
+
 /**
  * Main handler, invoked by Lambda
  */
 exports.certificateRequestHandler = async function (event, context) {
   var responseData = {};
   var physicalResourceId;
   var certificateArn;
+  async function processRequest() {
+    certificateArn = await requestCertificate(
+      event.RequestId,
+      event.ResourceProperties.DomainName,
+      event.ResourceProperties.SubjectAlternativeNames,
+      event.ResourceProperties.CertificateTransparencyLoggingPreference,
+      event.ResourceProperties.HostedZoneId,
+      event.ResourceProperties.Region,
+      event.ResourceProperties.Route53Endpoint,
+    );
+    responseData.Arn = physicalResourceId = certificateArn;
+  }
 
   try {
     switch (event.RequestType) {
       case 'Create':
+        await processRequest();
+        if (event.ResourceProperties.Tags && physicalResourceId.startsWith('arn:')) {
+          await addTags(physicalResourceId, event.ResourceProperties.Region, event.ResourceProperties.Tags);
+        }
+        break;
       case 'Update':
-        certificateArn = await requestCertificate(
-          event.RequestId,
-          event.ResourceProperties.DomainName,
-          event.ResourceProperties.SubjectAlternativeNames,
-          event.ResourceProperties.CertificateTransparencyLoggingPreference,
-          event.ResourceProperties.HostedZoneId,
-          event.ResourceProperties.Region,
-          event.ResourceProperties.Route53Endpoint,
-          event.ResourceProperties.Tags,
-        );
-        responseData.Arn = physicalResourceId = certificateArn;
+        if (shouldUpdate(event.OldResourceProperties, event.ResourceProperties, event.PhysicalResourceId)) {
+          await processRequest();
+        } else {
+          responseData.Arn = physicalResourceId = event.PhysicalResourceId;
+        }
+        if (event.ResourceProperties.Tags && physicalResourceId.startsWith('arn:')) {
+          await addTags(physicalResourceId, event.ResourceProperties.Region, event.ResourceProperties.Tags);
+        }
         break;
       case 'Delete':
         physicalResourceId = event.PhysicalResourceId;
+        const removalPolicy = event.ResourceProperties.RemovalPolicy ?? 'destroy';
         // If the resource didn't create correctly, the physical resource ID won't be the
         // certificate ARN, so don't try to delete it in that case.
-        if (physicalResourceId.startsWith('arn:')) {
+        if (physicalResourceId.startsWith('arn:') && removalPolicy === 'destroy') {
           await deleteCertificate(
             physicalResourceId,
             event.ResourceProperties.Region,

packages/@aws-cdk/aws-certificatemanager/lambda-packages/dns_validated_certificate_handler/test/handler.test.js

@@ -869,6 +869,243 @@ describe('DNS Validated Certificate Handler', () => {
       });
   });
 
+  test('Update operation requests a certificate', () => {
+    const requestCertificateFake = sinon.fake.resolves({
+      CertificateArn: testCertificateArn,
+    });
+
+    const describeCertificateFake = sinon.stub();
+    describeCertificateFake.onFirstCall().resolves({
+      Certificate: {
+        CertificateArn: testCertificateArn
+      }
+    });
+    describeCertificateFake.resolves({
+      Certificate: {
+        CertificateArn: testCertificateArn,
+        DomainValidationOptions: [{
+          ValidationStatus: 'SUCCESS',
+          ResourceRecord: {
+            Name: testRRName,
+            Type: 'CNAME',
+            Value: testRRValue
+          }
+        }]
+      }
+    });
+
+    const addTagsToCertificateFake = sinon.fake.resolves({});
+
+    const changeResourceRecordSetsFake = sinon.fake.resolves({
+      ChangeInfo: {
+        Id: 'bogus'
+      }
+    });
+
+    AWS.mock('ACM', 'requestCertificate', requestCertificateFake);
+    AWS.mock('ACM', 'describeCertificate', describeCertificateFake);
+    AWS.mock('Route53', 'changeResourceRecordSets', changeResourceRecordSetsFake);
+    AWS.mock('ACM', 'addTagsToCertificate', addTagsToCertificateFake);
+
+    const request = nock(ResponseURL).put('/', body => {
+      return body.Status === 'SUCCESS';
+    }).reply(200);
+
+    return LambdaTester(handler.certificateRequestHandler)
+      .event({
+        RequestType: 'Update',
+        RequestId: testRequestId,
+        OldResourceProperties: {
+          DomainName: 'example.com',
+          HostedZoneId: testHostedZoneId,
+          Region: 'us-east-1',
+          Tags: testTags
+        },
+        ResourceProperties: {
+          DomainName: testDomainName,
+          HostedZoneId: testHostedZoneId,
+          Region: 'us-east-1',
+          Tags: testTags
+        }
+      })
+      .expectResolve(() => {
+        sinon.assert.calledWith(requestCertificateFake, sinon.match({
+          DomainName: testDomainName,
+          ValidationMethod: 'DNS',
+          Options: {
+            CertificateTransparencyLoggingPreference: undefined
+          }
+        }));
+        sinon.assert.calledWith(changeResourceRecordSetsFake, sinon.match({
+          ChangeBatch: {
+            Changes: [{
+              Action: 'UPSERT',
+              ResourceRecordSet: {
+                Name: testRRName,
+                Type: 'CNAME',
+                TTL: 60,
+                ResourceRecords: [{
+                  Value: testRRValue
+                }]
+              }
+            }]
+          },
+          HostedZoneId: testHostedZoneId
+        }));
+        sinon.assert.calledWith(addTagsToCertificateFake, sinon.match({
+          "CertificateArn": testCertificateArn,
+          "Tags": testTagsValue,
+        }));
+        expect(request.isDone()).toBe(true);
+      });
+  });
+
+  test('Update operation updates tags only', () => {
+    const requestCertificateFake = sinon.fake.resolves({
+      CertificateArn: testCertificateArn,
+    });
+
+    const describeCertificateFake = sinon.stub();
+    describeCertificateFake.onFirstCall().resolves({
+      Certificate: {
+        CertificateArn: testCertificateArn
+      }
+    });
+    describeCertificateFake.resolves({
+      Certificate: {
+        CertificateArn: testCertificateArn,
+        DomainValidationOptions: [{
+          ValidationStatus: 'SUCCESS',
+          ResourceRecord: {
+            Name: testRRName,
+            Type: 'CNAME',
+            Value: testRRValue
+          }
+        }]
+      }
+    });
+
+    const addTagsToCertificateFake = sinon.fake.resolves({});
+
+    const changeResourceRecordSetsFake = sinon.fake.resolves({
+      ChangeInfo: {
+        Id: 'bogus'
+      }
+    });
+
+    AWS.mock('ACM', 'requestCertificate', requestCertificateFake);
+    AWS.mock('ACM', 'describeCertificate', describeCertificateFake);
+    AWS.mock('Route53', 'changeResourceRecordSets', changeResourceRecordSetsFake);
+    AWS.mock('ACM', 'addTagsToCertificate', addTagsToCertificateFake);
+
+    const request = nock(ResponseURL).put('/', body => {
+      return body.Status === 'SUCCESS';
+    }).reply(200);
+
+    return LambdaTester(handler.certificateRequestHandler)
+      .event({
+        RequestType: 'Update',
+        RequestId: testRequestId,
+        PhysicalResourceId: testCertificateArn,
+        OldResourceProperties: {
+          DomainName: testDomainName,
+          HostedZoneId: testHostedZoneId,
+          Region: 'us-east-1',
+          Tags: testTags,
+        },
+        ResourceProperties: {
+          DomainName: testDomainName,
+          HostedZoneId: testHostedZoneId,
+          Region: 'us-east-1',
+          Tags: {
+            ...testTags,
+            Tag4: 'Value4',
+          },
+        }
+      })
+      .expectResolve(() => {
+        sinon.assert.notCalled(requestCertificateFake);
+        sinon.assert.notCalled(changeResourceRecordSetsFake);
+        sinon.assert.calledWith(addTagsToCertificateFake, sinon.match({
+          "CertificateArn": testCertificateArn,
+          "Tags": [{ Key: 'Tag1', Value: 'Test1' }, { Key: 'Tag2', Value: 'Test2' }, { Key: 'Tag4', Value: 'Value4' }],
+        }));
+        expect(request.isDone()).toBe(true);
+      });
+  });
+
+  test('Update operation does not request certificate if removal policy is changed', () => {
+    const requestCertificateFake = sinon.fake.resolves({
+      CertificateArn: testCertificateArn,
+    });
+
+    const describeCertificateFake = sinon.stub();
+    describeCertificateFake.onFirstCall().resolves({
+      Certificate: {
+        CertificateArn: testCertificateArn
+      }
+    });
+    describeCertificateFake.resolves({
+      Certificate: {
+        CertificateArn: testCertificateArn,
+        DomainValidationOptions: [{
+          ValidationStatus: 'SUCCESS',
+          ResourceRecord: {
+            Name: testRRName,
+            Type: 'CNAME',
+            Value: testRRValue
+          }
+        }]
+      }
+    });
+
+    const addTagsToCertificateFake = sinon.fake.resolves({});
+
+    const changeResourceRecordSetsFake = sinon.fake.resolves({
+      ChangeInfo: {
+        Id: 'bogus'
+      }
+    });
+
+    AWS.mock('ACM', 'requestCertificate', requestCertificateFake);
+    AWS.mock('ACM', 'describeCertificate', describeCertificateFake);
+    AWS.mock('Route53', 'changeResourceRecordSets', changeResourceRecordSetsFake);
+    AWS.mock('ACM', 'addTagsToCertificate', addTagsToCertificateFake);
+
+    const request = nock(ResponseURL).put('/', body => {
+      return body.Status === 'SUCCESS';
+    }).reply(200);
+
+    return LambdaTester(handler.certificateRequestHandler)
+      .event({
+        RequestType: 'Update',
+        RequestId: testRequestId,
+        PhysicalResourceId: testCertificateArn,
+        OldResourceProperties: {
+          DomainName: testDomainName,
+          HostedZoneId: testHostedZoneId,
+          Region: 'us-east-1',
+          Tags: testTags,
+        },
+        ResourceProperties: {
+          DomainName: testDomainName,
+          HostedZoneId: testHostedZoneId,
+          Region: 'us-east-1',
+          Tags: testTags,
+          RemovalPolicy: 'retain',
+        }
+      })
+      .expectResolve(() => {
+        sinon.assert.notCalled(requestCertificateFake);
+        sinon.assert.notCalled(changeResourceRecordSetsFake);
+        sinon.assert.calledWith(addTagsToCertificateFake, sinon.match({
+          "CertificateArn": testCertificateArn,
+          "Tags": testTagsValue,
+        }));
+        expect(request.isDone()).toBe(true);
+      });
+  });
+
   test('Delete operation succeeds if certificate becomes not-in-use', () => {
     const usedByArn = 'arn:aws:cloudfront::123456789012:distribution/d111111abcdef8';