From 84701d62bea78cfa5971b988dc06ab9eaa906c95 Mon Sep 17 00:00:00 2001
From: GZ <yuanhaoz@amazon.com>
Date: Fri, 13 Sep 2024 16:22:51 -0700
Subject: [PATCH] chore(s3): readme update with mixing L1 and L2 bucket policy
 (#31437)

### Issue # (if applicable)

Closes https://github.com/aws/aws-cdk/issues/30148

### Reason for this change

Users using L1 and L2 bucket policy with `serverAccessLogsBucket` would cause bucket policy overwrite instead of append.

### Description of changes

No behavioural change, only readme update to explain the issues and the workaround.

### Description of how you validated changes

No behavioural change.

### Checklist
- [ ] My code adheres to the [CONTRIBUTING GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and [DESIGN GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
---
 packages/aws-cdk-lib/aws-s3/README.md | 111 ++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)

diff --git a/packages/aws-cdk-lib/aws-s3/README.md b/packages/aws-cdk-lib/aws-s3/README.md
index f7d35ba94dc79..f1670876e5e4a 100644
--- a/packages/aws-cdk-lib/aws-s3/README.md
+++ b/packages/aws-cdk-lib/aws-s3/README.md
@@ -492,6 +492,117 @@ const bucket = new s3.Bucket(this, 'MyBucket', {
 });
 ```
 
+The above code will create a new bucket policy if none exists or update the
+existing bucket policy to allow access log delivery.
+
+However, there could be an edge case if the `accessLogsBucket` also defines a bucket
+policy resource using the L1 Construct. Although the mixing of L1 and L2 Constructs is not
+recommended, there are no mechanisms in place to prevent users from doing this at the moment.
+
+```ts
+const bucketName = "my-favorite-bucket-name";
+const accessLogsBucket = new s3.Bucket(this, 'AccessLogsBucket', {
+  objectOwnership: s3.ObjectOwnership.BUCKET_OWNER_ENFORCED,
+  bucketName,
+});
+
+// Creating a bucket policy using L1
+const bucketPolicy = new s3.CfnBucketPolicy(this, "BucketPolicy", {
+  bucket: bucketName,
+  policyDocument: {
+    Statement: [
+      {
+        Action: 's3:*',
+        Effect: 'Deny',
+        Principal: {
+          AWS: '*',
+        },
+        Resource: [
+          accessLogsBucket.bucketArn,
+          `${accessLogsBucket.bucketArn}/*`
+        ],
+      },
+    ],
+    Version: '2012-10-17',
+  },
+});
+
+// 'serverAccessLogsBucket' will create a new L2 bucket policy
+// to allow log delivery and overwrite the L1 bucket policy.
+const bucket = new s3.Bucket(this, 'MyBucket', {
+  serverAccessLogsBucket: accessLogsBucket,
+  serverAccessLogsPrefix: 'logs',
+});
+```
+
+The above example uses the L2 Bucket Construct with the L1 CfnBucketPolicy Construct. However,
+when `serverAccessLogsBucket` is set, a new L2 Bucket Policy resource will be created
+which overwrites the permissions defined in the L1 Bucket Policy causing unintended
+behaviours.
+
+As noted above, we highly discourage the mixed usage of L1 and L2 Constructs. The recommended
+approach would to define the bucket policy using `addToResourcePolicy` method.
+
+```ts
+const accessLogsBucket = new s3.Bucket(this, 'AccessLogsBucket', {
+  objectOwnership: s3.ObjectOwnership.BUCKET_OWNER_ENFORCED,
+});
+
+accessLogsBucket.addToResourcePolicy(
+  new iam.PolicyStatement({
+    actions: ['s3:*'],
+    resources: [accessLogsBucket.bucketArn, accessLogsBucket.arnForObjects('*')],
+    principals: [new iam.AnyPrincipal()],
+  })
+)
+
+const bucket = new s3.Bucket(this, 'MyBucket', {
+  serverAccessLogsBucket: accessLogsBucket,
+  serverAccessLogsPrefix: 'logs',
+});
+```
+
+Alternatively, users can use the L2 Bucket Policy Construct
+`BucketPolicy.fromCfnBucketPolicy` to wrap around `CfnBucketPolicy` Construct. This will allow the subsequent bucket policy generated by `serverAccessLogsBucket` usage to append to the existing bucket policy instead of overwriting.
+
+```ts
+const bucketName = "my-favorite-bucket-name";
+const accessLogsBucket = new s3.Bucket(this, 'AccessLogsBucket', {
+  objectOwnership: s3.ObjectOwnership.BUCKET_OWNER_ENFORCED,
+  bucketName,
+});
+
+const bucketPolicy = new s3.CfnBucketPolicy(this, "BucketPolicy", {
+  bucket: bucketName,
+  policyDocument: {
+    Statement: [
+      {
+        Action: 's3:*',
+        Effect: 'Deny',
+        Principal: {
+          AWS: '*',
+        },
+        Resource: [
+          accessLogsBucket.bucketArn,
+          `${accessLogsBucket.bucketArn}/*`
+        ],
+      },
+    ],
+    Version: '2012-10-17',
+  },
+});
+
+// Wrap L1 Construct with L2 Bucket Policy Construct. Subsequent 
+// generated bucket policy to allow access log delivery would append 
+// to the current policy.
+s3.BucketPolicy.fromCfnBucketPolicy(bucketPolicy);
+
+const bucket = new s3.Bucket(this, 'MyBucket', {
+  serverAccessLogsBucket: accessLogsBucket,
+  serverAccessLogsPrefix: 'logs',
+});
+```
+
 ## S3 Inventory
 
 An [inventory](https://docs.aws.amazon.com/AmazonS3/latest/dev/storage-inventory.html) contains a list of the objects in the source bucket and metadata for each object. The inventory lists are stored in the destination bucket as a CSV file compressed with GZIP, as an Apache optimized row columnar (ORC) file compressed with ZLIB, or as an Apache Parquet (Parquet) file compressed with Snappy.