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

docs/CLOUDFORMATION.md

@@ -0,0 +1,107 @@
+# CloudFormation internals
+
+This is documenting CloudFormation internals, that may come in useful when developing
+custom resources, working on the CLI or debugging CloudFormation operations.
+
+If you are a user of the CDK, you do not need to read this (except maybe out of interest).
+
+## CloudFormation stack lifecycle
+
+This shows the states a CloudFormation stack goes through in its lifetime.
+
+For the `_IN_PROGRESS` states, the letter `[C,U,D]` indicates whether in that
+state resource `Creates`, `Updates` or `Deletes` are performed.
+
+`GetTemplate` will return:
+
+- For `Creates`: the template that we are creating. During rollback of a `Create`, it
+  will show the template that failed to create.
+- For `Updates`: during the roll-forward it will return the template we are updating
+  to. During rollback, it will show the template we are rolling back to.
+
+```text
+      ╔══════════════════╗
+      ║                  ║
+      ║  <nonexistant>   ║──────────────────┐
+      ║                  ║                  ▼
+      ╚══════════════════╝       ┌────────────────────┐
+                │                │                    │
+                │                │ REVIEW_IN_PROGRESS │
+                │                │                    │
+                │                └────────────────────┘
+                │                           │
+                ├───────────────────────────┘
+                ▼
+     ┌────────────────────┐      ┌─────────────────────┐       ╔════════════════════╗
+     │                    │      │                     │       ║                    ║
+     │ CREATE_IN_PROGRESS │─────▶│ROLLBACK_IN_PROGRESS │──┬───▶║   CREATE_FAILED    ║
+     │        [C]         │      │         [D]         │  │    ║                    ║
+     └────────────────────┘      └─────────────────────┘  │    ╚════════════════════╝
+                │                                         ▼
+                │                              ╔════════════════════╗
+                │                              ║                    ║
+                │                              ║  ROLLBACK_FAILED   ║
+                │                              ║                    ║
+                ▼                              ╚════════════════════╝
+   ╔═════════════════════════╗
+   ║     CREATE_COMPLETE     ║
+┌──║     UPDATE_COMPLETE     ║◀─────────────────────────────────┬─────────────────┐
+│  ║UPDATE_ROLLBACK_COMPLETE ║                                  │                 │
+│  ╚═════════════════════════╝                                  │                 │
+│               │                                               │                 │
+│               │                                               │                 │
+│               ▼                                               │                 │
+│    ┌────────────────────┐                          ┌────────────────────┐       │
+│    │                    │                          │  UPDATE_COMPLETE_  │       │
+│    │ UPDATE_IN_PROGRESS │─────────────────────────▶│CLEANUP_IN_PROGRESS │       │
+│    │       [C,U]        │                          │        [D]         │       │
+│    └────────────────────┘◀────────────────────┐    └────────────────────┘       │
+│               │                               │                                 │
+│               ├──────────────────┐            │                                 │
+│               │                  ▼            │                                 │
+│               │       ╔════════════════════╗  │                                 │
+│               │       ║   (no-rollback)    ║  │                                 │
+│               │       ║   UPDATE_FAILED    ║──┘                                 │
+│               │       ║                    ║                                    │
+│               │       ╚════════════════════╝                                    │
+│               │                  │                                              │
+│               ├──────────────────┘                                              │
+│               ▼                                                                 │
+│    ┌────────────────────┐                     ┌──────────────────────────────┐  │
+│    │  UPDATE_ROLLBACK_  │                     │  UPDATE_ROLLBACK_COMPLETE_   │  │
+│    │    IN_PROGRESS     │────────────────────▶│     CLEANUP_IN_PROGRESS      │──┘
+│    │        [U]         │                     │             [D]              │
+│    └────────────────────┘                     └──────────────────────────────┘
+│    │                    ▲
+│    ▼                    │
+│    ╔════════════════════╗
+│    ║  UPDATE_ROLLBACK_  ║
+│    ║       FAILED       ║
+│    ║                    ║
+│    ╚════════════════════╝
+│               │
+│               │
+│               ▼
+│    ┌─────────────────────┐        ╔════════════════════╗
+│    │ DELETE_IN_PROGRESS  │        ║                    ║
+└───▶│         [D]         │───────▶║   DELETE_FAILED    ║
+     │                     │        ║                    ║
+     └─────────────────────┘        ╚════════════════════╝
+                │
+                │
+                ▼
+     ╔════════════════════╗
+     ║                    ║
+     ║  DELETE_COMPLETE   ║
+     ║                    ║
+     ╚════════════════════╝
+```
+
+### Rollbacks
+
+When rolling back:
+
+* `ROLLBACK_IN_PROGRESS` will exclusively do deletes.
+* `UPDATE_ROLLBACK_IN_PROGRESS` will do updates,
+  `UPDATE_ROLLBACK_COMPLETE_CLEANUP_IN_PROGRESS` will do deletes, including for
+  resources that got created in this update.

package.json

@@ -91,6 +91,8 @@
       "@aws-cdk/assertions/string-width/**",
       "@aws-cdk/assertions/table",
       "@aws-cdk/assertions/table/**",
+      "@aws-cdk/aws-s3/yaml",
+      "@aws-cdk/aws-s3/yaml/**",
       "@aws-cdk/aws-codebuild/yaml",
       "@aws-cdk/aws-codebuild/yaml/**",
       "@aws-cdk/aws-codepipeline-actions/case",

packages/@aws-cdk/aws-s3/NOTICE

@@ -1,2 +1,23 @@
 AWS Cloud Development Kit (AWS CDK)
 Copyright 2018-2021 Amazon.com, Inc. or its affiliates. All Rights Reserved.
+
+-------------------------------------------------------------------------------
+
+The AWS CDK includes the following third-party software/licensing:
+
+** yaml - https://www.npmjs.com/package/yaml
+Copyright 2018 Eemeli Aro <eemeli@gmail.com>
+
+Permission to use, copy, modify, and/or distribute this software for any purpose
+with or without fee is hereby granted, provided that the above copyright notice
+and this permission notice appear in all copies.
+
+THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES WITH
+REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF MERCHANTABILITY AND
+FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR ANY SPECIAL, DIRECT,
+INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES WHATSOEVER RESULTING FROM LOSS
+OF USE, DATA OR PROFITS, WHETHER IN AN ACTION OF CONTRACT, NEGLIGENCE OR OTHER
+TORTIOUS ACTION, ARISING OUT OF OR IN CONNECTION WITH THE USE OR PERFORMANCE OF
+THIS SOFTWARE.
+
+----------------

packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts

@@ -1,7 +1,36 @@
 // eslint-disable-next-line import/no-extraneous-dependencies
-import { S3 } from 'aws-sdk';
+import { S3, CloudFormation } from 'aws-sdk';
+import * as yaml from 'yaml';
+
+/**
+ * The custom resource should clear the bucket in one of the following cases:
+ *
+ * - The stack is deleted
+ * - The target bucket is removed from the template
+ * - The target bucket is replaced
+ * - The target bucket is created in a deployment that gets rolled back
+ *
+ * In particular, it should NOT clear the bucket in a case the custom resource is deleted
+ * but the target bucket is unaffected. This could happen in the follwoing cases:
+ *
+ * - The autoDelete feature used to be turned on, and now gets turned off (leads to removal
+ *   of the CR from the template, without affecting the bucket)
+ * - The autoDelete feature used to be turned off, now gets turned on, but the deployment
+ *   gets rolled back (leads to creation and immediate deletion of the CR, without
+ *   affecting the bucket).
+ *
+ * The only cases where we might misclassify is when the CR gets deleted. To
+ * determine whether or not we should empty the bucket, during a `Delete` event
+ * we will look at the stack state and depending on the state of the stack
+ * (rolling forward or rolling backward), compare the OLD and NEW templates to
+ * determine whether the bucket should be present in the final state:
+ *
+ * - ROLL FORWARD: delete contents if the bucket is not in the NEW template
+ * - ROLLBACK: delete contents if the bucket is not in the OLD template
+ */
 
 const s3 = new S3();
+const cfn = new CloudFormation();
 
 export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent) {
   switch (event.RequestType) {
@@ -10,7 +39,7 @@ export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent
     case 'Update':
       return onUpdate(event);
     case 'Delete':
-      return onDelete(event.ResourceProperties?.BucketName);
+      return onDelete(event.StackId, event.ResourceProperties?.BucketLogicalId, event.ResourceProperties?.BucketName);
   }
 }
 
@@ -24,7 +53,7 @@ async function onUpdate(event: AWSLambda.CloudFormationCustomResourceEvent) {
      and create a new one with the new name. So we have to delete the contents of the
      bucket so that this operation does not fail. */
   if (bucketNameHasChanged) {
-    return onDelete(oldBucketName);
+    return emptyBucket(oldBucketName);
   }
 }
 
@@ -48,9 +77,58 @@ async function emptyBucket(bucketName: string) {
   }
 }
 
-async function onDelete(bucketName?: string) {
+async function onDelete(stackId: string, logicalId?: string, bucketName?: string) {
   if (!bucketName) {
     throw new Error('No BucketName was provided.');
   }
-  await emptyBucket(bucketName);
+  if (!logicalId) {
+    throw new Error('No Logical ID was provided.');
+  }
+  if (await isBucketAboutToBeDeleted(stackId, logicalId)) {
+    await emptyBucket(bucketName);
+  }
 }
+
+/**
+ * Go and inspect CloudFormation to see if the target bucket is about to be deleted
+ */
+async function isBucketAboutToBeDeleted(stackId: string, logicalId: string) {
+  const stackResponse = await cfn.describeStacks({ StackName: stackId }).promise();
+  if (!stackResponse.Stacks?.[0]) {
+    throw new Error(`Could not find stack with ID: ${stackId}`);
+  }
+  const stackStatus = stackResponse.Stacks[0].StackStatus;
+  process.stdout.write(`Stack status: ${stackStatus}\n`);
+
+  // Case 1: the stack failed creation.
+  // Case 2: the stack is being deleted.
+  // In both cases, by definition the bucket will go bye-bye.
+  if (stackStatus === 'ROLLBACK_IN_PROGRESS' || stackStatus === 'DELETE_IN_PROGRESS') {
+    return true;
+  }
+
+  // Case 3: we're cleaning up after a successful rollforward.
+  // Case 4: we're rolling back a failed update.
+  // In both cases, either the bucket is also being deleted here, or it's just
+  // the CR that's being deleted.
+  // `GetTemplate` will show us the template we are moving to ('new' in case 3,
+  // 'old' in case 4). We will check if the bucket is in the template returned
+  // by `GetTemplate` to see if we need to clean it.
+  const destinationTemplateResponse = await cfn.getTemplate({ StackName: stackId, TemplateStage: 'Processed' }).promise();
+  let template;
+  try {
+    template = yaml.parse(destinationTemplateResponse.TemplateBody ?? '{}', {
+      schema: 'core',
+    });
+  } catch (e) {
+    throw new Error(`Unable to parse CloudFormation template (is it not YAML?): ${destinationTemplateResponse.TemplateBody}`);
+  }
+
+  if (logicalId in (template.Resources ?? {})) {
+    process.stdout.write(`Bucket ${logicalId} is in target template, so NOT cleaning.\n`);
+    return false;
+  } else {
+    process.stdout.write(`Bucket ${logicalId} is NOT in target template, so cleaning.\n`);
+    return true;
+  }
+}

packages/@aws-cdk/aws-s3/lib/bucket.ts

@@ -4,7 +4,7 @@ import * as events from '@aws-cdk/aws-events';
 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,
+  Aws, Fn, IResource, Lazy, RemovalPolicy, Resource, ResourceProps, Stack, Token,
   CustomResource, CustomResourceProvider, CustomResourceProviderRuntime, FeatureFlags,
 } from '@aws-cdk/core';
 import * as cxapi from '@aws-cdk/cx-api';
@@ -460,7 +460,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
@@ -1443,6 +1443,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 +1471,7 @@ export class Bucket extends BucketBase {
       inventoryConfigurations: Lazy.any({ produce: () => this.parseInventoryConfiguration() }),
       ownershipControls: this.parseOwnershipControls(props),
     });
+    this._resource = resource;
 
     resource.applyRemovalPolicy(props.removalPolicy);
 
@@ -1912,6 +1914,14 @@ export class Bucket extends BucketBase {
       codeDirectory: path.join(__dirname, 'auto-delete-objects-handler'),
       runtime: CustomResourceProviderRuntime.NODEJS_12_X,
       description: `Lambda function for auto-deleting objects in ${this.bucketName} S3 bucket.`,
+      policyStatements: [
+        // As a reminder: these are not `iam.PolicyStatement`s, but plain JSON IAM statements
+        {
+          Effect: 'Allow',
+          Action: ['cloudformation:DescribeStacks', 'cloudformation:GetTemplate'],
+          Resource: Aws.STACK_ID,
+        },
+      ],
     });
 
     // Use a bucket policy to allow the custom resource to delete
@@ -1934,6 +1944,7 @@ export class Bucket extends BucketBase {
       serviceToken: provider.serviceToken,
       properties: {
         BucketName: this.bucketName,
+        BucketLogicalId: this._resource.logicalId,
       },
     });
 

packages/@aws-cdk/aws-s3/package.json

@@ -55,6 +55,7 @@
   },
   "cdk-build": {
     "cloudformation": "AWS::S3",
+    "pre": ["./pre-build.sh"],
     "env": {
       "AWSLINT_BASE_CONSTRUCT": "true"
     }
@@ -79,16 +80,21 @@
     "@aws-cdk/pkglint": "0.0.0",
     "@types/aws-lambda": "^8.10.83",
     "@types/jest": "^26.0.24",
-    "jest": "^26.6.3"
+    "jest": "^26.6.3",
+    "aws-sdk": "^2"
   },
   "dependencies": {
     "@aws-cdk/aws-events": "0.0.0",
     "@aws-cdk/aws-iam": "0.0.0",
     "@aws-cdk/aws-kms": "0.0.0",
     "@aws-cdk/core": "0.0.0",
     "@aws-cdk/cx-api": "0.0.0",
-    "constructs": "^3.3.69"
+    "constructs": "^3.3.69",
+    "yaml": "1.10.2"
   },
+  "bundledDependencies": [
+    "yaml"
+  ],
   "homepage": "https://github.com/aws/aws-cdk",
   "peerDependencies": {
     "@aws-cdk/aws-events": "0.0.0",

packages/@aws-cdk/aws-s3/pre-build.sh

@@ -0,0 +1,7 @@
+#!/bin/bash
+# Bundle `node_modules/yaml` into the auto-delete-object handler
+tgt=lib/auto-delete-objects-handler/node_modules/yaml
+if [[ ! -d $tgt ]]; then
+  mkdir -p $tgt
+  cp -R node_modules/yaml/ $tgt/
+fi