fix(s3): setting autoDeleteObjects to false empties the bucket (#…

…16756) This was caused by the Custom Resource--which had previously been deployed when `autoDeleteObjects: true`--being removed when `autoDeleteObjects` is flipped off again. The custom resource would indiscriminately empty the bucket as it was being deleted. Fix by tagging the bucket to confirm that it needs to be emptied. If any deployment removes the CR but keeps the bucket, the ordering of CloudFormation updates will make sure that the untagging happens before the CR gets activated, thereby saving the bucket contents. Fixes #16603. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
aws · Oct 11, 2021 · 865abbe · 865abbe
1 parent 5d6c21a
commit 865abbe
Show file tree

Hide file tree

Showing 9 changed files with 356 additions and 52 deletions.
diff --git a/docs/CLOUDFORMATION.md b/docs/CLOUDFORMATION.md
diff --git a/packages/@aws-cdk/aws-kinesisfirehose-destinations/test/integ.s3-bucket.lit.expected.json b/packages/@aws-cdk/aws-kinesisfirehose-destinations/test/integ.s3-bucket.lit.expected.json
@@ -2,6 +2,14 @@
   "Resources": {
     "Bucket83908E77": {
       "Type": "AWS::S3::Bucket",
+      "Properties": {
+        "Tags": [
+          {
+            "Key": "aws-cdk:auto-delete-objects",
+            "Value": "true"
+          }
+        ]
+      },
       "UpdateReplacePolicy": "Delete",
       "DeletionPolicy": "Delete"
     },
@@ -102,7 +110,7 @@
       "Properties": {
         "Code": {
           "S3Bucket": {
-            "Ref": "AssetParametersfe5df38824187483182e1459db47adfae2a515aa4caedd437fc4033a0c5b3de9S3BucketD715D8B0"
+            "Ref": "AssetParameters3993fb4cd942505a050d08b09d5444e14c265cf9cd0fb8b0c5f621446b6cead9S3Bucket4673BB1A"
           },
           "S3Key": {
             "Fn::Join": [
@@ -115,7 +123,7 @@
                       "Fn::Split": [
                         "||",
                         {
-                          "Ref": "AssetParametersfe5df38824187483182e1459db47adfae2a515aa4caedd437fc4033a0c5b3de9S3VersionKey6E76822C"
+                          "Ref": "AssetParameters3993fb4cd942505a050d08b09d5444e14c265cf9cd0fb8b0c5f621446b6cead9S3VersionKey46E40510"
                         }
                       ]
                     }
@@ -128,7 +136,7 @@
                       "Fn::Split": [
                         "||",
                         {
-                          "Ref": "AssetParametersfe5df38824187483182e1459db47adfae2a515aa4caedd437fc4033a0c5b3de9S3VersionKey6E76822C"
+                          "Ref": "AssetParameters3993fb4cd942505a050d08b09d5444e14c265cf9cd0fb8b0c5f621446b6cead9S3VersionKey46E40510"
                         }
                       ]
                     }
@@ -167,6 +175,14 @@
     },
     "BackupBucket26B8E51C": {
       "Type": "AWS::S3::Bucket",
+      "Properties": {
+        "Tags": [
+          {
+            "Key": "aws-cdk:auto-delete-objects",
+            "Value": "true"
+          }
+        ]
+      },
       "UpdateReplacePolicy": "Delete",
       "DeletionPolicy": "Delete"
     },
@@ -751,17 +767,17 @@
     }
   },
   "Parameters": {
-    "AssetParametersfe5df38824187483182e1459db47adfae2a515aa4caedd437fc4033a0c5b3de9S3BucketD715D8B0": {
+    "AssetParameters3993fb4cd942505a050d08b09d5444e14c265cf9cd0fb8b0c5f621446b6cead9S3Bucket4673BB1A": {
       "Type": "String",
-      "Description": "S3 bucket for asset \"fe5df38824187483182e1459db47adfae2a515aa4caedd437fc4033a0c5b3de9\""
+      "Description": "S3 bucket for asset \"3993fb4cd942505a050d08b09d5444e14c265cf9cd0fb8b0c5f621446b6cead9\""
     },
-    "AssetParametersfe5df38824187483182e1459db47adfae2a515aa4caedd437fc4033a0c5b3de9S3VersionKey6E76822C": {
+    "AssetParameters3993fb4cd942505a050d08b09d5444e14c265cf9cd0fb8b0c5f621446b6cead9S3VersionKey46E40510": {
       "Type": "String",
-      "Description": "S3 key for asset version \"fe5df38824187483182e1459db47adfae2a515aa4caedd437fc4033a0c5b3de9\""
+      "Description": "S3 key for asset version \"3993fb4cd942505a050d08b09d5444e14c265cf9cd0fb8b0c5f621446b6cead9\""
     },
-    "AssetParametersfe5df38824187483182e1459db47adfae2a515aa4caedd437fc4033a0c5b3de9ArtifactHash9AE3702B": {
+    "AssetParameters3993fb4cd942505a050d08b09d5444e14c265cf9cd0fb8b0c5f621446b6cead9ArtifactHashBD621721": {
       "Type": "String",
-      "Description": "Artifact hash for asset \"fe5df38824187483182e1459db47adfae2a515aa4caedd437fc4033a0c5b3de9\""
+      "Description": "Artifact hash for asset \"3993fb4cd942505a050d08b09d5444e14c265cf9cd0fb8b0c5f621446b6cead9\""
     },
     "AssetParameters5ee078f2a1957fe672d6cfd84faf49e07b8460758b5cd2669b3df1212a14cd19S3BucketFEDDFB43": {
       "Type": "String",

diff --git a/packages/@aws-cdk/aws-s3/README.md b/packages/@aws-cdk/aws-s3/README.md
@@ -99,7 +99,7 @@ const result = bucket.addToResourcePolicy(new iam.PolicyStatement({
 }));
 ```
 
-If you try to add a policy statement to an existing bucket, this method will 
+If you try to add a policy statement to an existing bucket, this method will
 not do anything:
 
 ```ts
@@ -111,8 +111,8 @@ const result = bucket.addToResourcePolicy(new iam.PolicyStatement({
 }));
 ```
 
-That's because it's not possible to tell whether the bucket 
-already has a policy attached, let alone to re-use that policy to add more 
+That's because it's not possible to tell whether the bucket
+already has a policy attached, let alone to re-use that policy to add more
 statements to it. We recommend that you always check the result of the call:
 
 ```ts
@@ -445,3 +445,8 @@ const bucket = new Bucket(this, 'MyTempFileBucket', {
   autoDeleteObjects: true,
 });
 ```
+
+**Warning** if you have deployed a bucket with `autoDeleteObjects: true`,
+switching this to `false` in a CDK version *before* `1.126.0` will lead to all
+objects in the bucket being deleted. Be sure to update to version `1.126.0` or
+later before switching this value to `false`.
diff --git a/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts b/packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts
@@ -1,6 +1,8 @@
 // eslint-disable-next-line import/no-extraneous-dependencies
 import { S3 } from 'aws-sdk';
 
+const AUTO_DELETE_OBJECTS_TAG = 'aws-cdk:auto-delete-objects';
+
 const s3 = new S3();
 
 export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent) {
@@ -52,5 +54,22 @@ async function onDelete(bucketName?: string) {
   if (!bucketName) {
     throw new Error('No BucketName was provided.');
   }
+  if (!await isBucketTaggedForDeletion(bucketName)) {
+    process.stdout.write(`Bucket does not have '${AUTO_DELETE_OBJECTS_TAG}' tag, skipping cleaning.\n`);
+    return;
+  }
   await emptyBucket(bucketName);
 }
+
+/**
+ * The bucket will only be tagged for deletion if it's being deleted in the same
+ * deployment as this Custom Resource.
+ *
+ * If the Custom Resource is every deleted before the bucket, it must be because
+ * `autoDeleteObjects` has been switched to false, in which case the tag would have
+ * been removed before we get to this Delete event.
+ */
+async function isBucketTaggedForDeletion(bucketName: string) {
+  const response = await s3.getBucketTagging({ Bucket: bucketName }).promise();
+  return response.TagSet.some(tag => tag.Key === AUTO_DELETE_OBJECTS_TAG && tag.Value === 'true');
+}
diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts
@@ -5,7 +5,7 @@ import * as iam from '@aws-cdk/aws-iam';
 import * as kms from '@aws-cdk/aws-kms';
 import {
   Fn, IResource, Lazy, RemovalPolicy, Resource, ResourceProps, Stack, Token,
-  CustomResource, CustomResourceProvider, CustomResourceProviderRuntime, FeatureFlags,
+  CustomResource, CustomResourceProvider, CustomResourceProviderRuntime, FeatureFlags, Tags,
 } from '@aws-cdk/core';
 import * as cxapi from '@aws-cdk/cx-api';
 import { Construct } from 'constructs';
@@ -18,6 +18,7 @@ import { CfnBucket } from './s3.generated';
 import { parseBucketArn, parseBucketName } from './util';
 
 const AUTO_DELETE_OBJECTS_RESOURCE_TYPE = 'Custom::S3AutoDeleteObjects';
+const AUTO_DELETE_OBJECTS_TAG = 'aws-cdk:auto-delete-objects';
 
 export interface IBucket extends IResource {
   /**
@@ -460,7 +461,7 @@ export abstract class BucketBase extends Resource implements IBucket {
    * Indicates if a bucket resource policy should automatically created upon
    * the first call to `addToResourcePolicy`.
    */
-  protected abstract autoCreatePolicy = false;
+  protected abstract autoCreatePolicy: boolean;
 
   /**
    * Whether to disallow public access
@@ -1228,6 +1229,11 @@ export interface BucketProps {
    *
    * Requires the `removalPolicy` to be set to `RemovalPolicy.DESTROY`.
    *
+   * **Warning** if you have deployed a bucket with `autoDeleteObjects: true`,
+   * switching this to `false` in a CDK version *before* `1.126.0` will lead to
+   * all objects in the bucket being deleted. Be sure to update to version `1.126.0`
+   * or later before switching this value to `false`.
+   *
    * @default false
    */
   readonly autoDeleteObjects?: boolean;
@@ -1443,6 +1449,7 @@ export class Bucket extends BucketBase {
   private readonly metrics: BucketMetrics[] = [];
   private readonly cors: CorsRule[] = [];
   private readonly inventories: Inventory[] = [];
+  private readonly _resource: CfnBucket;
 
   constructor(scope: Construct, id: string, props: BucketProps = {}) {
     super(scope, id, {
@@ -1470,6 +1477,7 @@ export class Bucket extends BucketBase {
       inventoryConfigurations: Lazy.any({ produce: () => this.parseInventoryConfiguration() }),
       ownershipControls: this.parseOwnershipControls(props),
     });
+    this._resource = resource;
 
     resource.applyRemovalPolicy(props.removalPolicy);
 
@@ -1943,6 +1951,13 @@ export class Bucket extends BucketBase {
     if (this.policy) {
       customResource.node.addDependency(this.policy);
     }
+
+    // We also tag the bucket to record the fact that we want it autodeleted.
+    // The custom resource will check this tag before actually doing the delete.
+    // Because tagging and untagging will ALWAYS happen before the CR is deleted,
+    // we can set `autoDeleteObjects: false` without the removal of the CR emptying
+    // the bucket as a side effect.
+    Tags.of(this._resource).add(AUTO_DELETE_OBJECTS_TAG, 'true');
   }
 }
 

diff --git a/packages/@aws-cdk/aws-s3/test/auto-delete-objects-handler.test.ts b/packages/@aws-cdk/aws-s3/test/auto-delete-objects-handler.test.ts
@@ -1,6 +1,7 @@
 const mockS3Client = {
-  listObjectVersions: jest.fn().mockReturnThis(),
-  deleteObjects: jest.fn().mockReturnThis(),
+  listObjectVersions: jest.fn(),
+  deleteObjects: jest.fn(),
+  getBucketTagging: jest.fn(),
   promise: jest.fn(),
 };
 
@@ -13,6 +14,7 @@ jest.mock('aws-sdk', () => {
 beforeEach(() => {
   mockS3Client.listObjectVersions.mockReturnThis();
   mockS3Client.deleteObjects.mockReturnThis();
+  givenTaggedForDeletion();
 });
 
 afterEach(() => {
@@ -119,7 +121,7 @@ test('does nothing on update event when the new resource properties are absent',
 
 test('deletes all objects when the name changes on update event', async () => {
   // GIVEN
-  mockS3Client.promise.mockResolvedValue({ // listObjectVersions() call
+  mockAwsPromise(mockS3Client.listObjectVersions, {
     Versions: [
       { Key: 'Key1', VersionId: 'VersionId1' },
       { Key: 'Key2', VersionId: 'VersionId2' },
@@ -158,7 +160,7 @@ test('deletes all objects when the name changes on update event', async () => {
 
 test('deletes no objects on delete event when bucket has no objects', async () => {
   // GIVEN
-  mockS3Client.promise.mockResolvedValue({ Versions: [] }); // listObjectVersions() call
+  mockAwsPromise(mockS3Client.listObjectVersions, { Versions: [] });
 
   // WHEN
   const event: Partial<AWSLambda.CloudFormationCustomResourceDeleteEvent> = {
@@ -178,7 +180,7 @@ test('deletes no objects on delete event when bucket has no objects', async () =
 
 test('deletes all objects on delete event', async () => {
   // GIVEN
-  mockS3Client.promise.mockResolvedValue({ // listObjectVersions() call
+  mockAwsPromise(mockS3Client.listObjectVersions, {
     Versions: [
       { Key: 'Key1', VersionId: 'VersionId1' },
       { Key: 'Key2', VersionId: 'VersionId2' },
@@ -210,23 +212,46 @@ test('deletes all objects on delete event', async () => {
   });
 });
 
+test('does not empty bucket if it is not tagged', async () => {
+  // GIVEN
+  givenNotTaggedForDeletion();
+  mockAwsPromise(mockS3Client.listObjectVersions, {
+    Versions: [
+      { Key: 'Key1', VersionId: 'VersionId1' },
+      { Key: 'Key2', VersionId: 'VersionId2' },
+    ],
+  });
+
+  // WHEN
+  const event: Partial<AWSLambda.CloudFormationCustomResourceDeleteEvent> = {
+    RequestType: 'Delete',
+    ResourceProperties: {
+      ServiceToken: 'Foo',
+      BucketName: 'MyBucket',
+    },
+  };
+  await invokeHandler(event);
+
+  // THEN
+  expect(mockS3Client.listObjectVersions).not.toHaveBeenCalled();
+});
+
 test('delete event where bucket has many objects does recurse appropriately', async () => {
   // GIVEN
-  mockS3Client.promise // listObjectVersions() call
-    .mockResolvedValueOnce({
-      Versions: [
-        { Key: 'Key1', VersionId: 'VersionId1' },
-        { Key: 'Key2', VersionId: 'VersionId2' },
-      ],
-      IsTruncated: true,
-    })
-    .mockResolvedValueOnce(undefined) // deleteObjects() call
-    .mockResolvedValueOnce({ // listObjectVersions() call
-      Versions: [
-        { Key: 'Key3', VersionId: 'VersionId3' },
-        { Key: 'Key4', VersionId: 'VersionId4' },
-      ],
-    });
+  mockAwsPromise(mockS3Client.listObjectVersions, {
+    Versions: [
+      { Key: 'Key1', VersionId: 'VersionId1' },
+      { Key: 'Key2', VersionId: 'VersionId2' },
+    ],
+    IsTruncated: true,
+  }, 'once');
+  mockAwsPromise(mockS3Client.listObjectVersions, {
+    Versions: [
+      { Key: 'Key3', VersionId: 'VersionId3' },
+      { Key: 'Key4', VersionId: 'VersionId4' },
+    ],
+  }, 'once');
+  mockAwsPromise(mockS3Client.deleteObjects, {});
 
   // WHEN
   const event: Partial<AWSLambda.CloudFormationCustomResourceDeleteEvent> = {
@@ -267,3 +292,27 @@ test('delete event where bucket has many objects does recurse appropriately', as
 async function invokeHandler(event: Partial<AWSLambda.CloudFormationCustomResourceEvent>) {
   return handler(event as AWSLambda.CloudFormationCustomResourceEvent);
 }
+
+function mockAwsPromise<A>(fn: jest.Mock<any, any>, value: A, when: 'once' | 'always' = 'always') {
+  (when === 'always' ? fn.mockReturnValue : fn.mockReturnValueOnce).call(fn, {
+    promise: () => value,
+  });
+}
+
+function givenTaggedForDeletion() {
+  mockAwsPromise(mockS3Client.getBucketTagging, {
+    TagSet: [
+      {
+        Key: 'aws-cdk:auto-delete-objects',
+        Value: 'true',
+      },
+    ],
+
+  });
+}
+
+function givenNotTaggedForDeletion() {
+  mockAwsPromise(mockS3Client.getBucketTagging, {
+    TagSet: [],
+  });
+}