refactor: clean up API for removal policy across the library (#2893)

- Normalize how deletion policy is configured for stateful resources across the AWS Construct Library. - Simplify the model by removing the future-proofing of "RemovalPolicy.Forbid". - When `RemovalPolicy.Destory` is used, explicitly specify `DeletionPolicy: Delete` in CloudFormation (as oppose to relying on the fact the "Delete" is the default). BREAKING CHANGE: multiple modules have been changed to use `cdk.RemovalPolicy` to configure the resource's removal policy. * **core:** `applyRemovalPolicy` is now `CfnResource.applyRemovalPolicy`. * **core:** `RemovalPolicy.Orphan` has been renamed to `Retain`. * **core:** `RemovalPolicy.Forbid` has been removed, use `Retain`. * **ecr:** `RepositoryProps.retain` is now `removalPolicy`, and defaults to `Retain` instead of remove since ECR is a stateful resource * **kms:** `KeyProps.retain` is now `removalPolicy` * **logs:** `LogGroupProps.retainLogGroup` is now `removalPolicy` * **logs:** `LogStreamProps.retainLogStream` is now `removalPolicy` * **rds:** `DatabaseClusterProps.deleteReplacePolicy` is now `removalPolicy` * **rds:** `DatabaseInstanceNewProps.deleteReplacePolicy` is now `removalPolicy`
aws · Jun 18, 2019 · 65014ab · 65014ab
1 parent f4c8dcd
commit 65014ab
Show file tree

Hide file tree

Showing 53 changed files with 196 additions and 110 deletions.
diff --git a/packages/@aws-cdk/app-delivery/test/integ.cicd.expected.json b/packages/@aws-cdk/app-delivery/test/integ.cicd.expected.json
@@ -1,6 +1,7 @@
 {
   "Resources": {
     "ArtifactBucket7410C9EF": {
+      "DeletionPolicy": "Delete",
       "Type": "AWS::S3::Bucket"
     },
     "CodePipelineRoleB3A660B4": {
@@ -281,4 +282,4 @@
       }
     }
   }
-}
+}
diff --git a/packages/@aws-cdk/aws-cloudfront/test/integ.cloudfront-bucket-logging.expected.json b/packages/@aws-cdk/aws-cloudfront/test/integ.cloudfront-bucket-logging.expected.json
@@ -1,6 +1,7 @@
 {
   "Resources": {
     "Bucket83908E77": {
+      "DeletionPolicy": "Delete",
       "Type": "AWS::S3::Bucket"
     },
     "AnAmazingWebsiteProbablyCFDistribution47E3983B": {

diff --git a/packages/@aws-cdk/aws-cloudfront/test/integ.cloudfront-ipv6-disabled.expected.json b/packages/@aws-cdk/aws-cloudfront/test/integ.cloudfront-ipv6-disabled.expected.json
@@ -1,6 +1,7 @@
 {
   "Resources": {
     "Bucket83908E77": {
+      "DeletionPolicy": "Delete",
       "Type": "AWS::S3::Bucket"
     },
     "MyDistributionCFDistributionDE147309": {

diff --git a/packages/@aws-cdk/aws-cloudfront/test/integ.cloudfront-lambda-association.expected.json b/packages/@aws-cdk/aws-cloudfront/test/integ.cloudfront-lambda-association.expected.json
@@ -1,6 +1,7 @@
 {
   "Resources": {
     "Bucket83908E77": {
+      "DeletionPolicy": "Delete",
       "Type": "AWS::S3::Bucket"
     },
     "LambdaServiceRoleA8ED4D3B": {

diff --git a/packages/@aws-cdk/aws-cloudfront/test/integ.cloudfront-s3.expected.json b/packages/@aws-cdk/aws-cloudfront/test/integ.cloudfront-s3.expected.json
@@ -1,6 +1,7 @@
 {
   "Resources": {
     "Bucket83908E77": {
+      "DeletionPolicy": "Delete",
       "Type": "AWS::S3::Bucket"
     },
     "BucketPolicyE9A3008A": {

diff --git a/packages/@aws-cdk/aws-cloudfront/test/integ.cloudfront.expected.json b/packages/@aws-cdk/aws-cloudfront/test/integ.cloudfront.expected.json
@@ -1,6 +1,7 @@
 {
   "Resources": {
     "Bucket83908E77": {
+      "DeletionPolicy": "Delete",
       "Type": "AWS::S3::Bucket"
     },
     "MyDistributionCFDistributionDE147309": {

diff --git a/packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail.lit.expected.json b/packages/@aws-cdk/aws-cloudtrail/test/integ.cloudtrail.lit.expected.json
@@ -1,7 +1,8 @@
 {
   "Resources": {
     "Bucket83908E77": {
-      "Type": "AWS::S3::Bucket"
+      "Type": "AWS::S3::Bucket",
+      "DeletionPolicy": "Delete"
     },
     "TrailS30071F172": {
       "Type": "AWS::S3::Bucket",
@@ -124,4 +125,4 @@
       ]
     }
   }
-}
+}
diff --git a/packages/@aws-cdk/aws-codebuild/test/integ.caching.expected.json b/packages/@aws-cdk/aws-codebuild/test/integ.caching.expected.json
@@ -1,6 +1,7 @@
 {
   "Resources": {
     "CacheBucket41D9D0B0": {
+      "DeletionPolicy": "Delete",
       "Type": "AWS::S3::Bucket"
     },
     "MyProjectRole9BBE5233": {

diff --git a/packages/@aws-cdk/aws-codebuild/test/integ.ecr.lit.expected.json b/packages/@aws-cdk/aws-codebuild/test/integ.ecr.lit.expected.json
@@ -1,6 +1,7 @@
 {
   "Resources": {
     "MyRepoF4F48043": {
+      "DeletionPolicy": "Retain",
       "Type": "AWS::ECR::Repository",
       "Properties": {
         "RepositoryPolicyText": {

diff --git a/packages/@aws-cdk/aws-codebuild/test/integ.project-bucket.expected.json b/packages/@aws-cdk/aws-codebuild/test/integ.project-bucket.expected.json
@@ -1,6 +1,7 @@
 {
   "Resources": {
     "MyBucketF68F3FF0": {
+      "DeletionPolicy": "Delete",
       "Type": "AWS::S3::Bucket"
     },
     "MyProjectRole9BBE5233": {

diff --git a/packages/@aws-cdk/aws-codebuild/test/integ.project-secondary-sources-artifacts.expected.json b/packages/@aws-cdk/aws-codebuild/test/integ.project-secondary-sources-artifacts.expected.json
@@ -1,6 +1,7 @@
 {
   "Resources": {
     "MyBucketF68F3FF0": {
+      "DeletionPolicy": "Delete",
       "Type": "AWS::S3::Bucket"
     },
     "MyProjectRole9BBE5233": {

diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.lambda-pipeline.expected.json b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.lambda-pipeline.expected.json
@@ -378,6 +378,7 @@
     },
     "PipelineBucketB967BD35": {
       "Type": "AWS::S3::Bucket",
+      "DeletionPolicy": "Delete",
       "Properties": {
         "VersioningConfiguration": {
           "Status": "Enabled"

diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-alexa-deploy.expected.json b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-alexa-deploy.expected.json
@@ -68,6 +68,7 @@
     },
     "PipelineBucketB967BD35": {
       "Type": "AWS::S3::Bucket",
+      "DeletionPolicy": "Delete",
       "Properties": {
         "VersioningConfiguration": {
           "Status": "Enabled"

diff --git a/...ages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-cfn-cross-region.expected.json b/...ages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-cfn-cross-region.expected.json
@@ -2,6 +2,7 @@
   "Resources": {
     "MyBucketF68F3FF0": {
       "Type": "AWS::S3::Bucket",
+      "DeletionPolicy": "Delete",
       "Properties": {
         "VersioningConfiguration": {
           "Status": "Enabled"

diff --git a/.../@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-cfn-wtih-action-role.expected.json b/.../@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-cfn-wtih-action-role.expected.json
@@ -2,6 +2,7 @@
   "Resources": {
     "MyBucketF68F3FF0": {
       "Type": "AWS::S3::Bucket",
+      "DeletionPolicy": "Delete",
       "Properties": {
         "VersioningConfiguration": {
           "Status": "Enabled"

diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-cfn.expected.json b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-cfn.expected.json
@@ -373,6 +373,7 @@
     },
     "PipelineBucketB967BD35": {
       "Type": "AWS::S3::Bucket",
+      "DeletionPolicy": "Delete",
       "Properties": {
         "VersioningConfiguration": {
           "Status": "Enabled"

diff --git a/...codepipeline-actions/test/integ.pipeline-code-build-multiple-inputs-outputs.expected.json b/...codepipeline-actions/test/integ.pipeline-code-build-multiple-inputs-outputs.expected.json
@@ -74,6 +74,7 @@
     },
     "MyBucketF68F3FF0": {
       "Type": "AWS::S3::Bucket",
+      "DeletionPolicy": "Delete",
       "Properties": {
         "VersioningConfiguration": {
           "Status": "Enabled"

diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-code-deploy.expected.json b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-code-deploy.expected.json
@@ -87,6 +87,7 @@
     },
     "CodeDeployPipelineIntegTest9F618D61": {
       "Type": "AWS::S3::Bucket",
+      "DeletionPolicy": "Delete",
       "Properties": {
         "VersioningConfiguration": {
           "Status": "Enabled"

diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-ecr-source.expected.json b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-ecr-source.expected.json
@@ -1,6 +1,7 @@
 {
   "Resources": {
     "MyBucketF68F3FF0": {
+      "DeletionPolicy": "Delete",
       "Type": "AWS::S3::Bucket"
     },
     "MyPipelineRoleC0D47CA4": {
@@ -226,7 +227,8 @@
       }
     },
     "MyEcrRepo767466D0": {
-      "Type": "AWS::ECR::Repository"
+      "Type": "AWS::ECR::Repository",
+      "DeletionPolicy": "Retain"
     },
     "MyEcrRepoawscdkcodepipelineecrsourceMyPipeline63CF3194SourceEventRule911FDB6D": {
       "Type": "AWS::Events::Rule",

diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-ecs-deploy.expected.json b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-ecs-deploy.expected.json
@@ -194,7 +194,8 @@
       "Type": "AWS::ECS::Cluster"
     },
     "EcrRepoBB83A592": {
-      "Type": "AWS::ECR::Repository"
+      "Type": "AWS::ECR::Repository",
+      "DeletionPolicy": "Retain"
     },
     "TaskDefTaskRole1EDB4A67": {
       "Type": "AWS::IAM::Role",
@@ -310,6 +311,7 @@
     },
     "MyBucketF68F3FF0": {
       "Type": "AWS::S3::Bucket",
+      "DeletionPolicy": "Delete",
       "Properties": {
         "VersioningConfiguration": {
           "Status": "Enabled"

diff --git a/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-s3-deploy.expected.json b/packages/@aws-cdk/aws-codepipeline-actions/test/integ.pipeline-s3-deploy.expected.json
@@ -67,6 +67,7 @@
       }
     },
     "PipelineBucketB967BD35": {
+      "DeletionPolicy": "Delete",
       "Type": "AWS::S3::Bucket",
       "Properties": {
         "VersioningConfiguration": {

diff --git a/packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts b/packages/@aws-cdk/aws-codepipeline/lib/pipeline.ts
@@ -231,7 +231,7 @@ export class Pipeline extends PipelineBase {
         bucketName: PhysicalName.auto({ crossEnvironment: true }),
         encryptionKey,
         encryption: s3.BucketEncryption.Kms,
-        removalPolicy: RemovalPolicy.Orphan
+        removalPolicy: RemovalPolicy.Retain
       });
     }
     this.artifactBucket = propsBucket;

diff --git a/packages/@aws-cdk/aws-ecr/lib/repository.ts b/packages/@aws-cdk/aws-ecr/lib/repository.ts
@@ -1,6 +1,6 @@
 import events = require('@aws-cdk/aws-events');
 import iam = require('@aws-cdk/aws-iam');
-import { Construct, DeletionPolicy, IConstruct, IResource, Lazy, Resource, Stack, Token } from '@aws-cdk/cdk';
+import { Construct, IConstruct, IResource, Lazy, RemovalPolicy, Resource, Stack, Token } from '@aws-cdk/cdk';
 import { CfnRepository } from './ecr.generated';
 import { CountType, LifecycleRule, TagStatus } from './lifecycle';
 
@@ -248,14 +248,11 @@ export interface RepositoryProps {
   readonly lifecycleRegistryId?: string;
 
   /**
-   * Retain the repository on stack deletion
+   * Determine what happens to the repository when the resource/stack is deleted.
    *
-   * If you don't set this to true, the registry must be empty, otherwise
-   * your stack deletion will fail.
-   *
-   * @default false
+   * @default RemovalPolicy.Retain
    */
-  readonly retain?: boolean;
+  readonly removalPolicy?: RemovalPolicy;
 }
 
 export interface RepositoryAttributes {
@@ -347,9 +344,7 @@ export class Repository extends RepositoryBase {
       lifecyclePolicy: Lazy.anyValue({ produce: () => this.renderLifecyclePolicy() }),
     });
 
-    if (props.retain) {
-      resource.options.deletionPolicy = DeletionPolicy.Retain;
-    }
+    resource.applyRemovalPolicy(props.removalPolicy);
 
     this.registryId = props.lifecycleRegistryId;
     if (props.lifecycleRules) {

diff --git a/packages/@aws-cdk/aws-ecr/test/integ.basic.expected.json b/packages/@aws-cdk/aws-ecr/test/integ.basic.expected.json
@@ -2,6 +2,7 @@
   "Resources": {
     "Repo02AC86CF": {
       "Type": "AWS::ECR::Repository",
+      "DeletionPolicy": "Retain",
       "Properties": {
         "LifecyclePolicy": {
           "LifecyclePolicyText": "{\"rules\":[{\"rulePriority\":1,\"selection\":{\"tagStatus\":\"any\",\"countType\":\"imageCountMoreThan\",\"countNumber\":5},\"action\":{\"type\":\"expire\"}}]}"

diff --git a/packages/@aws-cdk/aws-ecr/test/test.repository.ts b/packages/@aws-cdk/aws-ecr/test/test.repository.ts
@@ -1,6 +1,7 @@
 import { expect, haveResource, haveResourceLike, ResourcePart } from '@aws-cdk/assert';
 import iam = require('@aws-cdk/aws-iam');
 import cdk = require('@aws-cdk/cdk');
+import { RemovalPolicy } from '@aws-cdk/cdk';
 import { Test } from 'nodeunit';
 import ecr = require('../lib');
 
@@ -18,7 +19,8 @@ export = {
     expect(stack).toMatch({
       Resources: {
         Repo02AC86CF: {
-          Type: "AWS::ECR::Repository"
+          Type: "AWS::ECR::Repository",
+          DeletionPolicy: "Retain"
         }
       }
     });
@@ -320,18 +322,36 @@ export = {
     }
   },
 
-  '"retain" can be used to retain the repo when the resource is deleted'(test: Test) {
+  'removal policy is "Retain" by default'(test: Test) {
     // GIVEN
     const stack = new cdk.Stack();
 
     // WHEN
-    new ecr.Repository(stack, 'Repo', { retain: true });
+    new ecr.Repository(stack, 'Repo');
 
     // THEN
     expect(stack).to(haveResource('AWS::ECR::Repository', {
       "Type": "AWS::ECR::Repository",
       "DeletionPolicy": "Retain"
     }, ResourcePart.CompleteDefinition));
     test.done();
+  },
+
+  '"Delete" removal policy can be set explicitly'(test: Test) {
+    // GIVEN
+    const stack = new cdk.Stack();
+
+    // WHEN
+    new ecr.Repository(stack, 'Repo', {
+      removalPolicy: RemovalPolicy.Destroy
+    });
+
+    // THEN
+    expect(stack).to(haveResource('AWS::ECR::Repository', {
+      "Type": "AWS::ECR::Repository",
+      "DeletionPolicy": "Delete"
+    }, ResourcePart.CompleteDefinition));
+    test.done();
   }
+
 };
diff --git a/packages/@aws-cdk/aws-kms/lib/key.ts b/packages/@aws-cdk/aws-kms/lib/key.ts
@@ -1,6 +1,6 @@
 import iam = require('@aws-cdk/aws-iam');
 import { PolicyDocument, PolicyStatement } from '@aws-cdk/aws-iam';
-import { Construct, DeletionPolicy, IResource, Resource, Stack } from '@aws-cdk/cdk';
+import { Construct, IResource, RemovalPolicy, Resource, Stack } from '@aws-cdk/cdk';
 import { Alias } from './alias';
 import { CfnKey } from './kms.generated';
 
@@ -177,9 +177,9 @@ export interface KeyProps {
    * Whether the encryption key should be retained when it is removed from the Stack. This is useful when one wants to
    * retain access to data that was encrypted with a key that is being retired.
    *
-   * @default true
+   * @default RemovalPolicy.Retain
    */
-  readonly retain?: boolean;
+  readonly removalPolicy?: RemovalPolicy;
 }
 
 /**
@@ -225,9 +225,7 @@ export class Key extends KeyBase {
     });
 
     this.keyArn = resource.attrArn;
-    resource.options.deletionPolicy = props.retain === false
-                                    ? DeletionPolicy.Delete
-                                    : DeletionPolicy.Retain;
+    resource.applyRemovalPolicy(props.removalPolicy);
   }
 
   /**

diff --git a/packages/@aws-cdk/aws-kms/test/integ.key-sharing.lit.ts b/packages/@aws-cdk/aws-kms/test/integ.key-sharing.lit.ts
@@ -1,5 +1,6 @@
 /// !cdk-integ *
 import cdk = require('@aws-cdk/cdk');
+import { RemovalPolicy } from '@aws-cdk/cdk';
 import kms = require('../lib');
 
 const app = new cdk.App();
@@ -14,7 +15,7 @@ class KeyStack extends cdk.Stack {
 
   constructor(scope: cdk.App, id: string, props?: cdk.StackProps) {
     super(scope, id, props);
-    this.key = new kms.Key(this, 'MyKey', { retain: false });
+    this.key = new kms.Key(this, 'MyKey', { removalPolicy: RemovalPolicy.Destroy });
   }
 }
 

diff --git a/packages/@aws-cdk/aws-kms/test/integ.key.ts b/packages/@aws-cdk/aws-kms/test/integ.key.ts
@@ -1,13 +1,13 @@
 import { PolicyStatement } from '@aws-cdk/aws-iam';
 import iam = require('@aws-cdk/aws-iam');
-import { App, Stack } from '@aws-cdk/cdk';
+import { App, RemovalPolicy, Stack } from '@aws-cdk/cdk';
 import { Key } from '../lib';
 
 const app = new App();
 
 const stack = new Stack(app, `aws-cdk-kms-1`);
 
-const key = new Key(stack, 'MyKey', { retain: false });
+const key = new Key(stack, 'MyKey', { removalPolicy: RemovalPolicy.Destroy });
 
 key.addToResourcePolicy(new PolicyStatement({
   resources: ['*'],

diff --git a/packages/@aws-cdk/aws-kms/test/test.key.ts b/packages/@aws-cdk/aws-kms/test/test.key.ts
@@ -1,6 +1,6 @@
 import { exactlyMatchTemplate, expect, haveResource, ResourcePart } from '@aws-cdk/assert';
 import { PolicyStatement, User } from '@aws-cdk/aws-iam';
-import { App, Stack, Tag } from '@aws-cdk/cdk';
+import { App, RemovalPolicy, Stack, Tag } from '@aws-cdk/cdk';
 import { Test } from 'nodeunit';
 import { Key } from '../lib';
 
@@ -68,7 +68,7 @@ export = {
     const app = new App();
     const stack = new Stack(app, 'TestStack');
 
-    new Key(stack, 'MyKey', { retain: false });
+    new Key(stack, 'MyKey', { removalPolicy: RemovalPolicy.Destroy });
 
     expect(stack).to(haveResource('AWS::KMS::Key', { DeletionPolicy: "Delete" }, ResourcePart.CompleteDefinition));
     test.done();