Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(s3): add option to auto delete objects upon bucket removal #9751
feat(s3): add option to auto delete objects upon bucket removal #9751
Changes from 5 commits
09642e9
f3b9cd3
6b27429
2d42b2e
56a48d7
809ea22
83463a4
4018b4d
6ff4bec
885b178
60c21f4
00f1b65
6762518
92e1b9c
60c16ca
2d41d98
4e2e2ff
1059325
f399f67
99ab9c1
d35592a
21f5a31
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question on the TODO, will the custom resource be able to delete objects encrypted by KMS? I think we can give the same permissions as configured in
grantReadWrite
helper on the basis of encryption configuration.If resource returned by
CustomResourceProvider
implementsIGrantable
, we can directly use the above mentioned helper:There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's the goal! Yeah I think that's probably what I will end up doing.
Hmm... so when I looked into this approach before, I wasn't able to find a way to get this to work, because I don't think
CustomResource
implementsIGrantable
(and I couldn't see a way to create a dummy/wrapper class to work around this). Likewise,CustomResourceProvider.getOrCreate
just returns a service token string unfortunately, so it's not currently possible to get access to the role that it creates (which is the principal we would hypothetically be performing grant operations on). I'm definitely open to suggestions or ideas though.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Chriscbr you can pass policies to the provider that will be added to its role. Will that be suffice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that's the prop I'm currently passing the policies into - please clarify if you had something else in mind.