feat(s3): add option to auto delete objects upon bucket removal #9751

Chriscbr · 2020-08-16T21:20:51Z

Implementation

To include the custom resource needed for this feature, I initially wanted to just use a nested sub-package like I saw used in aws-cdk/aws-dynamodb-global - this would allow me to write tests, separate the dependencies out, etc. However, when I tried creating a lambda function, I quickly discovered that we can't actually use lambda as a dependency, because that would cause a dependency cycle. After further research, I found this problem had been encountered before in the BucketNotification class, and it was solved by creating an fake object that behaved like a lambda function, so I copied a lot of this code.

The second problem I found was that since I can't directly use lambda.Code.fromAsset, I can't easily include the sub-package as a zip file to the lambda. (In particular, my understanding is that lambda.Code.fromAsset works by hooking into the cdk deploy system and uploading the asset, so this would be complex to do without the aws-cdk/aws-lambda library). The solution to this is to just inline the function directly, so this is also the approach I went for.

Once we migrate to a monolithic cdk library later this year, we can probably refactor this if desired.

Testing

I've added a few unit tests so far, and I tested that this works with versioned and encrypted buckets on my personal AWS account. Looking for constructive feedback / missing blind spots as always!

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

ChenReuven · 2020-08-18T15:00:35Z

Closes #3297

Implementation

To include the custom resource needed for this feature, I initially wanted to just use a nested sub-package like I saw used in aws-cdk/aws-dynamodb-global - this would allow me to write tests, separate the dependencies out, etc. However, when I tried creating a lambda function, I quickly discovered that we can't actually use lambda as a dependency, because that would cause a dependency cycle. After further research, I found this problem had been encountered before in the BucketNotification class, and it was solved by creating an fake object that behaved like a lambda function, so I copied a lot of this code.

The second problem I found was that since I can't directly use lambda.Code.fromAsset, I can't easily include the sub-package as a zip file to the lambda. (In particular, my understanding is that lambda.Code.fromAsset works by hooking into the cdk deploy system and uploading the asset, so this would be complex to do without the aws-cdk/aws-lambda library). The solution to this is to just inline the function directly, so this is also the approach I went for.

Once we migrate to a monolithic cdk library later this year, we can probably refactor this if desired.

Testing

I've added a few unit tests so far, and I tested that this works with versioned and encrypted buckets on my personal AWS account. Looking for constructive feedback / missing blind spots as always!

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

Great suggestion

will wait for this one +1 :)

rynrn · 2020-08-19T07:43:55Z

+1

packages/@aws-cdk/aws-s3/README.md

iliapolo

Thanks @Chriscbr - this is great! :)

packages/@aws-cdk/aws-s3/README.md

packages/@aws-cdk/aws-s3/test/test.auto-delete-objects.ts

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

github-actions · 2020-10-11T06:23:33Z

This PR has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

iliapolo · 2020-10-11T09:11:03Z

Dont close

jogold · 2020-10-12T08:23:14Z

The second problem I found was that since I can't directly use lambda.Code.fromAsset, I can't easily include the sub-package as a zip file to the lambda. (In particular, my understanding is that lambda.Code.fromAsset works by hooking into the cdk deploy system and uploading the asset, so this would be complex to do without the aws-cdk/aws-lambda library). The solution to this is to just inline the function directly, so this is also the approach I went for.

How about using the CR framework from core here https://github.com/aws/aws-cdk/tree/master/packages/%40aws-cdk/core#the-corecustomresourceprovider-class? I think it will be much cleaner...

iliapolo · 2020-10-12T08:39:48Z

@jogold

How about using the CR framework from core here https://github.com/aws/aws-cdk/tree/master/packages/%40aws-cdk/core#the-corecustomresourceprovider-class? I think it will be much cleaner...

Thanks! Definitely seems cleaner.

@Chriscbr what do you think? will save a lot of boilerplate.

Chriscbr · 2020-10-13T00:01:56Z

Yeah, that sounds like a great idea - thanks for bringing this feature to my attention @jogold ! I'll try to rework the implementation as soon as I'm able.

gitpod-io · 2020-10-13T02:52:19Z

Chriscbr · 2020-10-13T03:01:14Z

Not finished, but I've begun the main porting (code cov failing). Todo:

add unit tests for handler function (see https://github.com/aws/aws-cdk/blob/master/packages/%40aws-cdk/aws-iam/test/oidc-provider.test.ts for sinon examples). try to figure out if there is a way to make an assertion about ServiceToken as suggested by iliapolo
manually test that if one bucket has autoDeleteObjects and second bucket does not, then only one of them deletes (assuming they have objects)
test if encrypted buckets work, and/or add permissions as needed

jogold · 2020-10-13T08:17:17Z

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

+      policyStatements: [
+        {
+          Effect: 'Allow',
+          Resource: '*',


Suggested change

Resource: '*',

Resource: this.bucketArn,

It turns out this policy for this lambda needs to have access to all buckets in the stack, since it is a singleton that is shared with the entire stack. So I think specifying either * or arn:aws:s3:::* should work here.

I actually think this is a very important consideration we should think about. I'm not comfortable with creating a lambda function that has permissions to delete all objects from all buckets.

I think that it's better to create a provider per bucket, which i know is currently not possible, but maybe we should add it.

@rix0rrr What do you think about either making the constructor public, or allow passing a scope?

Can't we use a bucket policy here instead? (a policy that would allow the role of the custom resource to act on the bucket)

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

Chriscbr · 2020-11-08T02:37:42Z

Hi all - sorry it took me a while to get back to this PR. I've updated the unit tests to use jest per earlier feedback, but the feature implementation has stayed the same. Right now we are providing the automatic object deletion behavior through a custom provider approach suggested by @jogold. To recap some unresolved questions:

@iliapolo mentioned:

I'm not comfortable with creating a lambda function that has permissions to delete all objects from all buckets.
I think that it's better to create a provider per bucket, which i know is currently not possible, but maybe we should add it.
@rix0rrr What do you think about either making the constructor public, or allow passing a scope?

I'm not totally sure to what degree the security risks are, but this sounds like a valid consideration. The one point I will mention is with the current implementation, even if we changed CustomResourceProvider to make this possible (since right now the getOrCreate is the only public method I can use), there would have to be more cloudformation resources constructed per bucket (one custom resource, one lambda function, and one IAM role - in the current implementation, the latter two resources get shared by all buckets in the stack).

@jogold mentioned:

Can't we use a bucket policy here instead? (a policy that would allow the role of the custom resource to act on the bucket)

I did look into this today, but because of CustomResourceProvider's restricted APIs there's no direct way to refer to the IAM role that would need to be included in the Bucket Policy's policy statement. Currently you can do something hacky like this:

const provider = Stack.of(this).node.findChild(`${AUTO_DELETE_OBJECTS_RESOURCE_TYPE}CustomResourceProvider`);
const role = Node.of(provider).children.find((x) => (x as CfnResource).cfnResourceType === 'AWS::IAM::Role')!;
const roleArn = Token.asString((role as CfnResource).getAtt('Arn'));

this.addToResourcePolicy(new iam.PolicyStatement({
  principals: [new iam.ArnPrincipal(roleArn)],
  actions: [...perms.BUCKET_READ_ACTIONS, ...perms.BUCKET_DELETE_ACTIONS],
  resources: [this.bucketArn, this.arnForObjects('*')],
}));

But even when I tried this (and removed the blanket policy statement in getOrCreateAutoDeleteObjectsResource() from my PR), I started hitting IAM permission issues with the roles not being able to perform s3 actions -- but this might just me having some kind of misunderstanding of how bucket policies work. It could also be caused the bucket policy not taking into account that the lambda assumes the role when it executes (and the principal for an assumed role looks slightly different in a policy statement). TL;DR I don't think a bucket policy would work here, but if someone else can figure out a way to do it (with the end result being more granular permissions given to the IAM roles created for the custom resource), feel free to share (or make edits). 😄

ayush987goyal

This is in a great shape now! Some small comments:

ayush987goyal · 2020-11-08T03:30:25Z

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

@@ -0,0 +1,43 @@
+export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent) {
+  // eslint-disable-next-line @typescript-eslint/no-require-imports, import/no-extraneous-dependencies
+  const s3 = new (require('aws-sdk').S3)();


Why not initialize this out of the handler scope into the global one? This would avoid passing of the instance to every function and also the usage of any type for the s3 client.

Ah right, this is a remnant of how I was mocking the s3 client previously - I'll go fix this.

You could do something like:

import * as AWS from 'aws-sdk'; const s3 = new AWS.S3();

So, for reasons I can't ascertain, when I hoist the const s3 = new (require(... line to the file's outer scope, or even perform a regular import like you suggested, then jest stops mocking the module properly, causing all my tests to run actual aws-sdk API calls (the first error it runs into a NoSuchBucket exception since the tests use dummy bucket names).

One thing to keep in mind is that the @aws-cdk/aws-s3 package doesn't actually have aws-sdk as a dependency. (That makes the above behavior odd - but the entire repo shares a single node_modules directory which does have aws-sdk so it's not that surprising).

Based on that, unless there is a way to fix this import behavior, I'll just keep passing the s3 client around. At the end of the day I think this is OK - I've tested that it works, and I'm not worried about any performance implications of instantiating the client inside the method since the "create" and "delete" operations only get once per bucket-and-custom-resource pair. But if you have any tips to fix this that is cool too. :)

The jest mocks might not be working in this new scenario because of how the actual module was imported. Previously, since the module was imported inside the handler, the jest.mock call could be placed in the describe block.

Now that the module is imported at the file level, the jest.mock should also be done at the top of the tests file (since the actual module gets imported as soon as you import the handler file).

Please give it a try once. If it works, great! Otherwise you can keep this in the current form.

Ah, that made it work! Thank you for teaching something new. :) I did have to move the mock above the handler import in order for it to fully work, but I think this is ok.

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

packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts

ayush987goyal · 2020-11-08T06:04:27Z

packages/@aws-cdk/aws-s3/test/auto-delete-objects.test.ts

+        BucketName: 'MyBucket',
+      },
+    };
+    await invokeHandler(event);


Awaiting the invokeHandler is the "WHEN" section here :)

ayush987goyal · 2020-11-08T06:09:40Z

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

@@ -0,0 +1,43 @@
+export async function handler(event: AWSLambda.CloudFormationCustomResourceEvent) {
+  // eslint-disable-next-line @typescript-eslint/no-require-imports, import/no-extraneous-dependencies
+  const s3 = new (require('aws-sdk').S3)();


The jest mocks might not be working in this new scenario because of how the actual module was imported. Previously, since the module was imported inside the handler, the jest.mock call could be placed in the describe block.

Now that the module is imported at the file level, the jest.mock should also be done at the top of the tests file (since the actual module gets imported as soon as you import the handler file).

Please give it a try once. If it works, great! Otherwise you can keep this in the current form.

ayush987goyal

Amazing! LGTM 👍

aws-cdk-automation · 2020-11-08T07:14:34Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 21f5a31
Result: SUCCEEDED
Build Logs (available for 30 days)

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

jogold · 2020-11-08T09:57:33Z

@Chriscbr can you share the resulting bucket policy? I don't see why it wouldn't work. Also if we go in this direction we should cleanly expose the role of the custom resource provider.

The policy should look like this one:

{
    "Version": "2012-10-17",
    "Statement": [
        {
            "Effect": "Allow",
            "Principal": {
                "AWS": "arn:aws:iam::123456789012:role/<CR-LAMBDA-ROLE>"
            },
            "Action": "s3:DeleteObject",
            "Resource": "arn:aws:s3:::<BUCKET>/*"
        }
    ]
}

iliapolo · 2020-12-06T22:08:30Z

@Chriscbr Wondering about the state of this PR. The code looks good, but we should address the IAM policy issue @jogold mentioned. Do you still have capacity to get back to it?

Chriscbr · 2020-12-07T05:52:16Z

@iliapolo I want to take another look, but for the next week or so I am fairly occupied with university exams so it will be hard to find time. I'm okay with moving this to a draft or closed state, or letting anyone else take a stab at this.

iliapolo · 2020-12-07T11:21:26Z

@Chriscbr cool, good luck with your exams! :)

The role ARN can then be used in resource-based policies. See aws#9751 (comment)

jogold · 2020-12-07T21:56:03Z

Also if we go in this direction we should cleanly expose the role of the custom resource provider.

#11923

aehrath-amazon · 2020-12-09T17:25:54Z

Hi, I am new to this thread. I also looked into this issue and solved it with something like this:
#3297 (comment)
Would this work as a simple solution to the problem?
Cheers, Alex

The role ARN can then be used in resource-based IAM policies. See #9751 (comment) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

@Chriscbr

Use the custom resource provider from core to delete objects in the bucket. A bucket policy gives the correct permissions to the provider's Lambda function role. Credits to @Chriscbr for starting the work on this. Closes aws#3297 Closes aws#9751

@Chriscbr

Use the custom resource provider from core to delete objects in the bucket. A bucket policy gives the correct permissions to the provider's Lambda function role. Credits to @Chriscbr for starting the work on this. Closes aws#3297 Closes aws#9751

@Chriscbr

Use the custom resource provider from core to delete objects in the bucket. A bucket policy gives the correct permissions to the provider's Lambda function role. Credits to @Chriscbr for starting the work on this. Closes aws#3297 Closes aws#9751

@Chriscbr

Use the custom resource provider from core to delete objects in the bucket. A bucket policy gives the correct permissions to the provider's Lambda function role. Credits to @Chriscbr for starting the work on this. Closes #3297 Closes #9751 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

The role ARN can then be used in resource-based IAM policies. See aws#9751 (comment) ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

@Chriscbr

Use the custom resource provider from core to delete objects in the bucket. A bucket policy gives the correct permissions to the provider's Lambda function role. Credits to @Chriscbr for starting the work on this. Closes aws#3297 Closes aws#9751 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*

Chriscbr added 2 commits August 16, 2020 17:14

feat(s3): add option to auto delete objects upon bucket removal

09642e9

Merge branch 'master' into feat/clear-s3-bucket

f3b9cd3

SomayaB assigned iliapolo Aug 17, 2020

Dzhuneyt approved these changes Oct 9, 2020

View reviewed changes

Dzhuneyt reviewed Oct 9, 2020

View reviewed changes

packages/@aws-cdk/aws-s3/README.md Show resolved Hide resolved

iliapolo reviewed Oct 10, 2020

View reviewed changes

iliapolo added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Oct 10, 2020

github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Oct 11, 2020

iliapolo removed the closing-soon This issue will automatically close in 4 days unless further comments are made. label Oct 11, 2020

github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Oct 12, 2020

iliapolo added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Oct 12, 2020

Chriscbr added 3 commits October 12, 2020 20:32

Merge branch 'master' into feat/clear-s3-bucket

6b27429

Refactor to use CustomResourceProvider

2d42b2e

Update unit tests

56a48d7

github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Oct 13, 2020

jogold reviewed Oct 13, 2020

View reviewed changes

iliapolo added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Oct 14, 2020

Chriscbr added 3 commits October 18, 2020 00:55

Add feedback from jogold

809ea22

Add unit tests for handler and ServiceToken

83463a4

Merge branch 'master' into feat/clear-s3-bucket

4018b4d

Chriscbr marked this pull request as draft October 18, 2020 06:55

Merge branch 'master' into feat/clear-s3-bucket

f399f67

ayush987goyal suggested changes Nov 8, 2020

View reviewed changes

fixup tests and other nitpicks

99ab9c1

ayush987goyal suggested changes Nov 8, 2020

View reviewed changes

Chriscbr added 2 commits November 8, 2020 01:41

hoist aws-sdk import to global scope

d35592a

reorder imports

21f5a31

ayush987goyal approved these changes Nov 8, 2020

View reviewed changes

iliapolo mentioned this pull request Nov 25, 2020

[s3] Gracefully handle failed deploys #11687

Closed

2 tasks

iliapolo added the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Dec 6, 2020

github-actions bot removed the response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. label Dec 7, 2020

jogold added a commit to jogold/aws-cdk that referenced this pull request Dec 7, 2020

feat(core): expose custom resource provider's role

390327f

The role ARN can then be used in resource-based policies. See aws#9751 (comment)

jogold mentioned this pull request Dec 7, 2020

feat(core): expose custom resource provider's role #11923

Merged

iliapolo mentioned this pull request Dec 9, 2020

Feature Request: Option to clear and delete S3 bucket on delete #3297

Closed

5 tasks

jogold mentioned this pull request Dec 15, 2020

feat(s3): option to auto delete objects upon bucket removal #12090

Merged

mergify bot closed this in #12090 Dec 26, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(s3): add option to auto delete objects upon bucket removal #9751

feat(s3): add option to auto delete objects upon bucket removal #9751

Chriscbr commented Aug 16, 2020

ChenReuven commented Aug 18, 2020

Implementation

Testing

rynrn commented Aug 19, 2020

iliapolo left a comment

github-actions bot commented Oct 11, 2020

iliapolo commented Oct 11, 2020

jogold commented Oct 12, 2020

iliapolo commented Oct 12, 2020

Chriscbr commented Oct 13, 2020

gitpod-io bot commented Oct 13, 2020 •

edited

Loading

Chriscbr commented Oct 13, 2020 •

edited

Loading

jogold Oct 13, 2020

Chriscbr Oct 18, 2020

iliapolo Oct 20, 2020

jogold Oct 22, 2020 •

edited

Loading

Chriscbr commented Nov 8, 2020

ayush987goyal left a comment

ayush987goyal Nov 8, 2020

Chriscbr Nov 8, 2020

ayush987goyal Nov 8, 2020

Chriscbr Nov 8, 2020

ayush987goyal Nov 8, 2020

Chriscbr Nov 8, 2020

ayush987goyal Nov 8, 2020

ayush987goyal Nov 8, 2020

ayush987goyal left a comment

aws-cdk-automation commented Nov 8, 2020

jogold commented Nov 8, 2020 •

edited

Loading

iliapolo commented Dec 6, 2020

Chriscbr commented Dec 7, 2020

iliapolo commented Dec 7, 2020

jogold commented Dec 7, 2020

aehrath-amazon commented Dec 9, 2020

feat(s3): add option to auto delete objects upon bucket removal #9751

feat(s3): add option to auto delete objects upon bucket removal #9751

Conversation

Chriscbr commented Aug 16, 2020

Implementation

Testing

ChenReuven commented Aug 18, 2020

Implementation

Testing

rynrn commented Aug 19, 2020

iliapolo left a comment

Choose a reason for hiding this comment

github-actions bot commented Oct 11, 2020

iliapolo commented Oct 11, 2020

jogold commented Oct 12, 2020

iliapolo commented Oct 12, 2020

Chriscbr commented Oct 13, 2020

gitpod-io bot commented Oct 13, 2020 • edited Loading

Chriscbr commented Oct 13, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jogold Oct 22, 2020 • edited Loading

Choose a reason for hiding this comment

Chriscbr commented Nov 8, 2020

ayush987goyal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ayush987goyal left a comment

Choose a reason for hiding this comment

aws-cdk-automation commented Nov 8, 2020

AWS CodeBuild CI Report

jogold commented Nov 8, 2020 • edited Loading

iliapolo commented Dec 6, 2020

Chriscbr commented Dec 7, 2020

iliapolo commented Dec 7, 2020

jogold commented Dec 7, 2020

aehrath-amazon commented Dec 9, 2020

gitpod-io bot commented Oct 13, 2020 •

edited

Loading

Chriscbr commented Oct 13, 2020 •

edited

Loading

jogold Oct 22, 2020 •

edited

Loading

jogold commented Nov 8, 2020 •

edited

Loading