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

rix0rrr · 2021-10-01T16:00:51Z

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

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 having the custom resource inspect the ongoing CloudFormation deployment: if the bucket would not be deleted as part of the ongoing deployment, also do not empty it. Fixes #16603.

gitpod-io · 2021-10-01T16:00:54Z

ryparker

Nice fix 🚀

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

ryparker · 2021-10-01T16:30:37Z

Looks like CI found an integration test that needs an update:
yarn integ integ.bucket-auto-delete-objects.js

Co-authored-by: Ryan Parker <ryan.parker3@outlook.com>

…nto huijbers/fix-s3-autodelete

rittneje · 2021-10-01T19:15:57Z

@rix0rrr @ryparker FWIW, as a workaround, we were investigating using CDK metadata on the S3 bucket as a means of telling the lambda whether it should actually clear the bucket or not. (The metadata would be set when auto_delete_objects is true, and unset otherwise.) I'm not sure how airtight that is though.

jogold · 2021-10-02T08:12:24Z

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

+  const destinationTemplateResponse = await cfn.getTemplate({ StackName: stackId, TemplateStage: 'Processed' }).promise();
+  let template;
+  try {
+    template = yaml.parse(destinationTemplateResponse.TemplateBody ?? '{}', {


If you describe the changeset here instead of analyzing the template then you don't need the yaml dependency (and the script to include it in the handler). Better?

Maybe. It artificially limits the template's use to cases where we deploy using change sets. Granted, the CDK always does that, but I hate unnecessarily limiting ourselves in that way.

jogold · 2021-10-02T08:16:01Z

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

+tgt=lib/auto-delete-objects-handler/node_modules/yaml
+if [[ ! -d $tgt ]]; then
+  mkdir -p $tgt
+  cp -R node_modules/yaml/ $tgt/


what if yaml gets updated and comes with dependencies? (tests will continue to pass)

iliapolo

@rix0rrr Have you considered coupling the custom resource with the bucket, instead of the autoDeleteObjects property?

I.e the CR will always exist, and the lambda will either no-op if autoDeleteObjects is false (or undefined), or will delete otherwise.

rix0rrr · 2021-10-04T07:37:32Z

@rix0rrr Have you considered coupling the custom resource with the bucket, instead of the autoDeleteObjects property?

We have considered it. It means that every single one of the 1243891 templates out there that contains an S3 bucket will automatically gain 3 more resources:

Role
Lambda
CR

And so be "polluted", not to mention the diff we'd be introducing to all customers everywhere. I don't think it would be looked upon graciously

rix0rrr · 2021-10-04T07:45:59Z

@rix0rrr @ryparker FWIW, as a workaround, we were investigating using CDK metadata on the S3 bucket as a means of telling the lambda whether it should actually clear the bucket or not. (The metadata would be set when auto_delete_objects is true, and unset otherwise.) I'm not sure how airtight that is though.

You know what -- I rejected that initially because of assumptions I made on the CFN lifecycle. Specifically: I thought the following use case would be broken:

Bucket already exists with data
New deployment activates autoDeleteObjects: true, but gets rolled back.
In the process of applying autoDeleteObjects, we would set a tag on the bucket:

My assumption was that the rollback would happen in reverse topological order, and so the CR deletion would happen before the untagging:

UPDATE_IN_PROGRESS	UPDATE_ROLLBACK_IN_PROGRESS
Tag bucket -> create custom resource -> (ROLLBACK)	-> delete custom resource -> untag

However, after documenting the lifecycle, I think the order would actually be:

UPDATE_IN_PROGRESS	UPDATE_ROLLBACK_IN_PROGRESS	UPDATE_ROLLBACK_COMPLETE_CLEANUP_IN_PROGRESS
Tag bucket -> create custom resource -> (ROLLBACK)	-> untag	-> delete custom resource

In which case the tag-based approach is actually completely valid, and requires a smaller code change.

I think I will pivot to that. Thanks for reminding me!

rix0rrr · 2021-10-04T11:58:45Z

monocdk: FATAL ERROR: NewSpace::Rebalance Allocation failed - JavaScript heap out of memory

mergify · 2021-10-04T12:03:17Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

jogold · 2021-10-04T12:10:19Z

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

@@ -0,0 +1,7 @@
+#!/bin/bash


Remove this file?

aws-cdk-automation · 2021-10-04T15:25:57Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
Commit ID: cfa9a41
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2021-10-04T15:26:16Z

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

* '15588' of https://github.com/xykkong/aws-cdk: (47 commits) chore: rollback `GenericSSMParameterImage` deprecation (backport aws#16798) (aws#16800) chore(deps): bump actions/setup-node from 2.4.0 to 2.4.1 (aws#16778) Update CHANGELOG.md chore(release): 1.126.0 feat(assertions): matcher support for `templateMatches()` API (aws#16789) feat(stepfunctions-tasks): add step concurrency level to EmrCreateCluster (aws#15242) docs(s3): correct heading levels Object Ownership / Bucket deletion (aws#16790) chore(individual-pkg-gen): fix bug in setting alpha package visibility (aws#16787) fix(s3): setting `autoDeleteObjects` to `false` empties the bucket (aws#16756) fix(iam): `User.fromUserArn` does not work for ARNs that include a path (aws#16269) fix(cli): progress bar overshoots count by 1 for stack updates (aws#16168) fix(config): add SourceAccount condition to Lambda permission (aws#16617) docs(events): add a note about not using `EventPattern` with `CfnRule` (aws#16715) docs(core): fix reference to nonexistant enum value (aws#16716) chore(s3-deployments): update python version on BucketDeployment handler (aws#16771) chore: set response-requested length to 2 and closing-soon to 5 (aws#16763) fix(revert): "fix: CDK does not honor NO_PROXY settings (aws#16751)" (aws#16761) docs(GitHub issue templates): Upgrade to GitHub Issues v2 (aws#16592) chore: reset jsii-rosetta worker count to default (aws#16755) fix: CDK does not honor NO_PROXY settings (aws#16751) ...

…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*

…ws#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 aws#16603. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

The AutoDeleteObjects Custom Resource should pass when the bucket doesn't exist. With #16756 we introduced a safety check to only delete buckets that are marked for object-deletion. This check would unintentionally bypass the special case to mark the CR deletion as successful when the bucket doesn't exist. Additionally, with the upgrade to SDK v3 we need to change the check from `error.code` to check for the actual error instance. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

The AutoDeleteObjects Custom Resource should pass when the bucket doesn't exist. With aws#16756 we introduced a safety check to only delete buckets that are marked for object-deletion. This check would unintentionally bypass the special case to mark the CR deletion as successful when the bucket doesn't exist. Additionally, with the upgrade to SDK v3 we need to change the check from `error.code` to check for the actual error instance. ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

rix0rrr requested a review from a team October 1, 2021 16:00

rix0rrr self-assigned this Oct 1, 2021

mergify bot added the contribution/core This is a PR that came from AWS. label Oct 1, 2021

Dependency should have been under devDependencies

39b0304

ryparker approved these changes Oct 1, 2021

View reviewed changes

packages/@aws-cdk/aws-s3/lib/auto-delete-objects-handler/index.ts Outdated Show resolved Hide resolved

ryparker added the p0 label Oct 1, 2021

rix0rrr and others added 2 commits October 1, 2021 19:06

Update snapshot

c44c19f

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

90e3ad7

Co-authored-by: Ryan Parker <ryan.parker3@outlook.com>

ryparker mentioned this pull request Oct 1, 2021

feat(ecr): add option to auto delete images upon ECR repo removal #15932

Closed

rix0rrr added 2 commits October 1, 2021 20:37

Ignore asset hashes

669c9c9

Merge branch 'huijbers/fix-s3-autodelete' of github.com:aws/aws-cdk i…

7e5698b

…nto huijbers/fix-s3-autodelete

ryparker approved these changes Oct 1, 2021

View reviewed changes

jogold reviewed Oct 2, 2021

View reviewed changes

iliapolo reviewed Oct 3, 2021

View reviewed changes

rix0rrr added 4 commits October 4, 2021 09:56

Undo all my hard work

d220b94

Switch to tag-based solution

3ce3ab4

Thanks eslint

b5a7234

Update expectation

2e8a5e0

jogold reviewed Oct 4, 2021

View reviewed changes

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

@@ -0,0 +1,7 @@

#!/bin/bash

Copy link

Contributor

jogold Oct 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Remove this file?

rix0rrr added 2 commits October 4, 2021 14:27

Remove useless file

827be46

Merge branch 'master' into huijbers/fix-s3-autodelete

cfa9a41

mergify bot merged commit 21836f2 into master Oct 4, 2021

mergify bot deleted the huijbers/fix-s3-autodelete branch October 4, 2021 15:26

shah-smit mentioned this pull request Oct 9, 2021

Upgrade CDK to 1.126.0 or later shah-smit/cdk-cloud-formation-appreciation-dashboard#1

Closed

mrgrain mentioned this pull request Jul 19, 2023

fix(s3): auto-delete-objects fails when bucket doesn't exist #26433

Merged

tmokmss mentioned this pull request Aug 2, 2023

feat(lambda): allow to delete log group of a lambda function when it is removed #21820

Closed

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

rix0rrr commented Oct 1, 2021 •

edited

Loading

gitpod-io bot commented Oct 1, 2021

ryparker left a comment

ryparker commented Oct 1, 2021

rittneje commented Oct 1, 2021

jogold Oct 2, 2021

rix0rrr Oct 4, 2021 •

edited

Loading

jogold Oct 2, 2021 •

edited

Loading

iliapolo left a comment •

edited

Loading

rix0rrr commented Oct 4, 2021

rix0rrr commented Oct 4, 2021

rix0rrr commented Oct 4, 2021

mergify bot commented Oct 4, 2021

jogold Oct 4, 2021

aws-cdk-automation commented Oct 4, 2021

mergify bot commented Oct 4, 2021

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

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

Conversation

rix0rrr commented Oct 1, 2021 • edited Loading

gitpod-io bot commented Oct 1, 2021

ryparker left a comment

Choose a reason for hiding this comment

ryparker commented Oct 1, 2021

rittneje commented Oct 1, 2021

jogold Oct 2, 2021

Choose a reason for hiding this comment

rix0rrr Oct 4, 2021 • edited Loading

Choose a reason for hiding this comment

jogold Oct 2, 2021 • edited Loading

Choose a reason for hiding this comment

iliapolo left a comment • edited Loading

Choose a reason for hiding this comment

rix0rrr commented Oct 4, 2021

rix0rrr commented Oct 4, 2021

rix0rrr commented Oct 4, 2021

mergify bot commented Oct 4, 2021

jogold Oct 4, 2021

Choose a reason for hiding this comment

aws-cdk-automation commented Oct 4, 2021

AWS CodeBuild CI Report

mergify bot commented Oct 4, 2021

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

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

rix0rrr commented Oct 1, 2021 •

edited

Loading

rix0rrr Oct 4, 2021 •

edited

Loading

jogold Oct 2, 2021 •

edited

Loading

iliapolo left a comment •

edited

Loading