Skip to content

Commit

Permalink
fix(certificatemanager): unable to set removal policy on DnsValidated…
Browse files Browse the repository at this point in the history
…Certificate (aws#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 aws#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*
  • Loading branch information
corymhall authored and madeline-k committed Oct 10, 2022
1 parent 8fe7d03 commit 7de2789
Show file tree
Hide file tree
Showing 4 changed files with 75 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down Expand Up @@ -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() }),
Expand All @@ -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
Expand Down
Original file line number Diff line number Diff line change
@@ -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', () => {
Expand Down Expand Up @@ -266,4 +266,36 @@ test('test transparency logging settings is passed to the custom resource', () =
},
CertificateTransparencyLoggingPreference: 'DISABLED',
});
});
});

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',
});
});

0 comments on commit 7de2789

Please sign in to comment.