From b3c9464d0e0d333db132daec96cdd283145a6ce5 Mon Sep 17 00:00:00 2001 From: Cory Hall <43035978+corymhall@users.noreply.github.com> Date: Thu, 15 Sep 2022 11:11:34 -0400 Subject: [PATCH] fix(certificatemanager): unable to set removal policy on DnsValidatedCertificate (#22040) 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` fixes #20649 ---- ### 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* --- .../lib/index.js | 3 ++ .../test/handler.test.js | 32 +++++++++++++++++ .../lib/dns-validated-certificate.ts | 6 ++++ .../test/dns-validated-certificate.test.ts | 36 +++++++++++++++++-- 4 files changed, 75 insertions(+), 2 deletions(-) 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 48261e12d82e5..32b676820b865 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 @@ -300,6 +300,9 @@ exports.certificateRequestHandler = async function (event, context) { responseData.Arn = physicalResourceId = certificateArn; break; case 'Delete': + if (event.ResourceProperties.RemovalPolicy === 'retain') { + break; + } physicalResourceId = event.PhysicalResourceId; // 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. 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 37697f69b6e2e..5c627c0e6f27c 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 @@ -835,6 +835,38 @@ describe('DNS Validated Certificate Handler', () => { }); }); + test('Delete operation does not delete the certificate if RemovalPolicy===retain', () => { + const describeCertificateFake = sinon.fake.resolves({ + Certificate: { + CertificateArn: testCertificateArn, + } + }); + AWS.mock('ACM', 'describeCertificate', describeCertificateFake); + + const deleteCertificateFake = sinon.fake.resolves({}); + AWS.mock('ACM', 'deleteCertificate', deleteCertificateFake); + + const request = nock(ResponseURL).put('/', body => { + return body.Status === 'SUCCESS'; + }).reply(200); + + return LambdaTester(handler.certificateRequestHandler) + .event({ + RequestType: 'Delete', + RequestId: testRequestId, + PhysicalResourceId: testCertificateArn, + ResourceProperties: { + Region: 'us-east-1', + RemovalPolicy: 'retain', + } + }) + .expectResolve(() => { + sinon.assert.notCalled(describeCertificateFake); + sinon.assert.notCalled(deleteCertificateFake); + expect(request.isDone()).toBe(true); + }); + }); + test('Delete operation is idempotent', () => { const error = new Error(); error.name = 'ResourceNotFoundException'; diff --git a/packages/@aws-cdk/aws-certificatemanager/lib/dns-validated-certificate.ts b/packages/@aws-cdk/aws-certificatemanager/lib/dns-validated-certificate.ts index eb4044cb7833f..b01062021fb2a 100644 --- a/packages/@aws-cdk/aws-certificatemanager/lib/dns-validated-certificate.ts +++ b/packages/@aws-cdk/aws-certificatemanager/lib/dns-validated-certificate.ts @@ -79,6 +79,7 @@ export class DnsValidatedCertificate extends CertificateBase implements ICertifi private normalizedZoneName: string; private hostedZoneId: string; private domainName: string; + private _removalPolicy?: cdk.RemovalPolicy; constructor(scope: Construct, id: string, props: DnsValidatedCertificateProps) { super(scope, id); @@ -132,6 +133,7 @@ export class DnsValidatedCertificate extends CertificateBase implements ICertifi HostedZoneId: this.hostedZoneId, Region: props.region, Route53Endpoint: props.route53Endpoint, + RemovalPolicy: cdk.Lazy.any({ produce: () => this._removalPolicy }), // Custom resources properties are always converted to strings; might as well be explict here. CleanupRecords: props.cleanupRoute53Records ? 'true' : undefined, Tags: cdk.Lazy.list({ produce: () => this.tags.renderTags() }), @@ -143,6 +145,10 @@ export class DnsValidatedCertificate extends CertificateBase implements ICertifi this.node.addValidation({ validate: () => this.validateDnsValidatedCertificate() }); } + public applyRemovalPolicy(policy: cdk.RemovalPolicy): void { + this._removalPolicy = policy; + } + private validateDnsValidatedCertificate(): string[] { const errors: string[] = []; // Ensure the zone name is a parent zone of the certificate domain name diff --git a/packages/@aws-cdk/aws-certificatemanager/test/dns-validated-certificate.test.ts b/packages/@aws-cdk/aws-certificatemanager/test/dns-validated-certificate.test.ts index 5ed77764de122..688a17ef25a69 100644 --- a/packages/@aws-cdk/aws-certificatemanager/test/dns-validated-certificate.test.ts +++ b/packages/@aws-cdk/aws-certificatemanager/test/dns-validated-certificate.test.ts @@ -1,7 +1,7 @@ import { Template } from '@aws-cdk/assertions'; import * as iam from '@aws-cdk/aws-iam'; import { HostedZone, PublicHostedZone } from '@aws-cdk/aws-route53'; -import { App, Stack, Token, Tags } from '@aws-cdk/core'; +import { App, Stack, Token, Tags, RemovalPolicy } from '@aws-cdk/core'; import { DnsValidatedCertificate } from '../lib/dns-validated-certificate'; test('creates CloudFormation Custom Resource', () => { @@ -266,4 +266,36 @@ test('test transparency logging settings is passed to the custom resource', () = }, CertificateTransparencyLoggingPreference: 'DISABLED', }); -}); \ No newline at end of file +}); + +test('can set removal policy', () => { + const stack = new Stack(); + + const exampleDotComZone = new PublicHostedZone(stack, 'ExampleDotCom', { + zoneName: 'example.com', + }); + + const cert = new DnsValidatedCertificate(stack, 'Certificate', { + domainName: 'test.example.com', + hostedZone: exampleDotComZone, + subjectAlternativeNames: ['test2.example.com'], + cleanupRoute53Records: true, + }); + cert.applyRemovalPolicy(RemovalPolicy.RETAIN); + + Template.fromStack(stack).hasResourceProperties('AWS::CloudFormation::CustomResource', { + DomainName: 'test.example.com', + SubjectAlternativeNames: ['test2.example.com'], + RemovalPolicy: 'retain', + ServiceToken: { + 'Fn::GetAtt': [ + 'CertificateCertificateRequestorFunction5E845413', + 'Arn', + ], + }, + HostedZoneId: { + Ref: 'ExampleDotCom4D1B83AA', + }, + CleanupRecords: 'true', + }); +});