Merge branch 'master' into njlynch/fix-private-alphas

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",

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`.

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

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

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: [],
+  });
+}