fix(s3): RETAIN removal policy does not apply to an auto created bucket policy

[go-to-k]

This PR fixes the bug that a RETAIN removal policy for the bucket does not apply to an auto created bucket policy.

Closes #27125.

---

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

[aws-cdk-automation]

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: b14e3d4
• Result: SUCCEEDED
• Build Logs (available for 30 days)

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

[go-to-k]

Focusing only on resolving this issue, it is only for RETAIN or RETAIN_ON_UPDATE_OR_DELETE.

• Since the default for RemovalPolicy is DESTROY in the S3 Bucket and BucketPolicies, there is little benefit to bother with applyRemovalPolicy even when DESTROY is used.
• If we did that, the scope of impact on existing implementations would be very large and we would need to change many more integ tests!

Also, applyRemovalPolicy is done only for BucketPolicy automatically generated here, and BucketPolicy that is not automatically generated is excluded. Therefore, I put it in if (!this.policy && this.autoCreatePolicy).

[tmokmss]

Thanks for working on this! I found this change can actually create another problem in the following scenario:

1. Deploy a bucket with a bucket policy with RETAIN removal policy

new Bucket(stack, 'Bucket', {
  enforceSSL: true,
  removalPolicy: cdk.RemovalPolicy.RETAIN,
});

2. Then remove a bucket policy and deploy

new Bucket(stack, 'Bucket', {
-  enforceSSL: true,
  removalPolicy: cdk.RemovalPolicy.RETAIN,
});

3. Finally re-create a bucket policy and deploy

new Bucket(stack, 'Bucket', {
+  enforceSSL: true,
  removalPolicy: cdk.RemovalPolicy.RETAIN,
});

We will get a CFn error on the final deployment : The bucket policy already exists on bucket xxx

Considering this, I think setting a removal policy to a bucket policy is not necessarily a good idea. What do you think?

edit) enforceSSL is just for example here. Any configuration that uses bucket policy, such as VPC flow log, ALB access logging, CloudTrail, etc will be affected.

[go-to-k]

@tmokmss

I see, indeed. But isn't that not specific to this case, but also to other cases of RETAIN of other resources?

In this case, I think there should be more cases where the stack is deleted with the bucket and bucket policy set to RETAIN than cases where the enforceSSL is added and then removed. In that case, we probably want to keep both the bucket and the policy RETAIN, so I think it would cause confusion if the bucket policy disappears.

[go-to-k]

But isn't that not specific to this case, but also to other cases of RETAIN of other resources?

I tried that other cases (set RETAIN, delete, and re-create) such as a Bucket, but it succeeded, so no duplicate errors occurred....

However, I still think we don't want the policy to disappear when the stack is deleted. Do you think there is any other way?

[go-to-k]

I had a misconception. In this case, the error is caused by the fact that Bucket can only have one bucket policy regardless of CFn.

If this is the case, we may not be able to handle it in CDK layer because of the difficulty of updatable import of the resource that is the old policy, unless we remove the old policy with custom resource Lambda like autoDeleteObjects, etc.

I think setting a removal policy to a bucket policy is not necessarily a good idea.

So I may agree with you.

[go-to-k]

Or I thought about creating a BucketPolicy from the beginning regardless of enforceSSL, but I don't think to be a good idea, because there is following description so It would change the existing behavior.

If `autoCreatePolicy` is true, a `BucketPolicy` will be created upon the
first call to addToResourcePolicy(s).

[tmokmss]

I'm still not sure how common this issue is. Also, can there be any reverse use case such that a user wants to delete a bucket policy while retaining data in a bucket (e.g. to remove unnecessary permissions)? Then it would break those use cases.

If you want to proceed further I guess you can summarize available options and ask a core team member which way to go. IMHO an escape hatch is enough to workaround this issue though.(bucket.findChild('Policy') as CfnResource).applyRemovalPolicy(RemovalPolicy.RETAIN)

[go-to-k]

Also, can there be any reverse use case such that a user wants to delete a bucket policy while retaining data in a bucket (e.g. to remove unnecessary permissions)?

I don't think that case is very likely. Because if we wanted to leave the bucket alone and rewrite the policy to something else later, we could just manually overwrite the settings.

However, I can't see where this PR solution would probably go, so I'm going to close it once and for all. If driven by the need, it will be reopened then.