fix(s3): add bucket policy dependency to notification resource (#30053)

### Issue 

Closes #27600.
Closes #16811.

### Reason for this change

PutBucketPolicy and PutBucketNotification API calls are happening at the same time and causing race condition because S3 does not allow parallel bucket edits.

### Description of changes

Add dependency on bucket policy to custom resource so that PutBucketPolicy API call is always executed before PutBucketNotification.

### Description of how you validated changes

I've reproduced the bug locally and this solution fixed it.

### Checklist
- [X] 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-testing/framework-integ/test/aws-lambda-event-sources/test/integ.s3.js.snapshot/lambda-event-source-s3.template.json

@@ -174,7 +174,8 @@
     "Managed": true
    },
    "DependsOn": [
-    "BAllowBucketNotificationsTolambdaeventsources3F741608059EF9F709"
+    "BAllowBucketNotificationsTolambdaeventsources3F741608059EF9F709",
+    "BPolicy3F02723E"
    ]
   },
   "BAllowBucketNotificationsTolambdaeventsources3F741608059EF9F709": {

packages/@aws-cdk-testing/framework-integ/test/aws-s3-notifications/test/sqs/integ.bucket-notifications.js.snapshot/sqs-bucket-notifications.template.json

@@ -325,6 +325,7 @@
     "Managed": true
    },
    "DependsOn": [
+    "Bucket2Policy945B22E3",
     "MyQueuePolicy6BBEDDAC",
     "MyQueueE6CA6235"
    ]

packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket.notifications.js.snapshot/aws-cdk-s3-notifications.template.json

@@ -5,6 +5,53 @@
    "UpdateReplacePolicy": "Delete",
    "DeletionPolicy": "Delete"
   },
+  "MyEventBridgeBucketPolicy8F5346E3": {
+   "Type": "AWS::S3::BucketPolicy",
+   "Properties": {
+    "Bucket": {
+     "Ref": "MyEventBridgeBucket1ABD5C2A"
+    },
+    "PolicyDocument": {
+     "Statement": [
+      {
+       "Action": "s3:*",
+       "Condition": {
+        "Bool": {
+         "aws:SecureTransport": "false"
+        }
+       },
+       "Effect": "Deny",
+       "Principal": {
+        "AWS": "*"
+       },
+       "Resource": [
+        {
+         "Fn::GetAtt": [
+          "MyEventBridgeBucket1ABD5C2A",
+          "Arn"
+         ]
+        },
+        {
+         "Fn::Join": [
+          "",
+          [
+           {
+            "Fn::GetAtt": [
+             "MyEventBridgeBucket1ABD5C2A",
+             "Arn"
+            ]
+           },
+           "/*"
+          ]
+         ]
+        }
+       ]
+      }
+     ],
+     "Version": "2012-10-17"
+    }
+   }
+  },
   "MyEventBridgeBucketNotifications19C0453F": {
    "Type": "Custom::S3BucketNotifications",
    "Properties": {
@@ -21,7 +68,10 @@
      "EventBridgeConfiguration": {}
     },
     "Managed": true
-   }
+   },
+   "DependsOn": [
+    "MyEventBridgeBucketPolicy8F5346E3"
+   ]
   },
   "BucketNotificationsHandler050a0587b7544547bf325f094a3db834RoleB6FB88EC": {
    "Type": "AWS::IAM::Role",

packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket.notifications.ts

@@ -9,6 +9,7 @@ const stack = new cdk.Stack(app, 'aws-cdk-s3-notifications');
 
 new s3.Bucket(stack, 'MyEventBridgeBucket', {
   eventBridgeEnabled: true,
+  enforceSSL: true, // Adding dummy bucket policy for testing that bucket policy is created before bucket notification
   removalPolicy: cdk.RemovalPolicy.DESTROY,
 });
 

packages/aws-cdk-lib/aws-s3/lib/notifications-resource/notifications-resource.ts

@@ -1,4 +1,4 @@
-import { Construct } from 'constructs';
+import { Construct, IConstruct } from 'constructs';
 import { NotificationsResourceHandler } from './notifications-resource-handler';
 import * as iam from '../../../aws-iam';
 import * as cdk from '../../../core';
@@ -135,6 +135,20 @@ export class BucketNotifications extends Construct {
           Managed: managed,
         },
       });
+
+      // Add dependency on bucket policy if it exists to avoid race conditions
+      // S3 does not allow calling PutBucketPolicy and PutBucketNotification APIs at the same time
+      // See https://github.com/aws/aws-cdk/issues/27600
+      // Aspects are used here because bucket policy maybe added to construct after addition of notification resource.
+      const bucket = this.bucket;
+      const resource = this.resource;
+      cdk.Aspects.of(this).add({
+        visit(node: IConstruct) {
+          if (node === resource && bucket.policy) {
+            node.node.addDependency(bucket.policy);
+          }
+        },
+      });
     }
 
     return this.resource;

packages/aws-cdk-lib/aws-s3/test/notification.test.ts

@@ -1,4 +1,4 @@
-import { Template } from '../../assertions';
+import { Match, Template } from '../../assertions';
 import * as iam from '../../aws-iam';
 import * as cdk from '../../core';
 import * as s3 from '../lib';
@@ -121,6 +121,68 @@ describe('notification', () => {
     });
   });
 
+  test('custom resource must not depend on bucket policy if it bucket policy does not exists', () => {
+    const stack = new cdk.Stack();
+
+    const bucket = new s3.Bucket(stack, 'MyBucket');
+
+    bucket.addEventNotification(s3.EventType.OBJECT_CREATED, {
+      bind: () => ({
+        arn: 'ARN',
+        type: s3.BucketNotificationDestinationType.TOPIC,
+      }),
+    });
+
+    Template.fromStack(stack).hasResource('Custom::S3BucketNotifications', {
+      Type: 'Custom::S3BucketNotifications',
+      DependsOn: Match.absent(),
+    });
+  });
+
+  test('custom resource must depend on bucket policy to prevent executing too early', () => {
+    const stack = new cdk.Stack();
+
+    const bucket = new s3.Bucket(stack, 'MyBucket', {
+      enforceSSL: true, // adds bucket policy for test
+    });
+
+    bucket.addEventNotification(s3.EventType.OBJECT_CREATED, {
+      bind: () => ({
+        arn: 'ARN',
+        type: s3.BucketNotificationDestinationType.TOPIC,
+      }),
+    });
+
+    Template.fromStack(stack).hasResource('Custom::S3BucketNotifications', {
+      Type: 'Custom::S3BucketNotifications',
+      DependsOn: ['MyBucketPolicyE7FBAC7B'],
+    });
+  });
+
+  test('custom resource must depend on bucket policy even if bucket policy is added after notification', () => {
+    const stack = new cdk.Stack();
+
+    const bucket = new s3.Bucket(stack, 'MyBucket');
+
+    bucket.addEventNotification(s3.EventType.OBJECT_CREATED, {
+      bind: () => ({
+        arn: 'ARN',
+        type: s3.BucketNotificationDestinationType.TOPIC,
+      }),
+    });
+
+    bucket.addToResourcePolicy(new iam.PolicyStatement({
+      resources: [bucket.bucketArn],
+      actions: ['s3:GetBucketAcl'],
+      principals: [new iam.AnyPrincipal()],
+    }));
+
+    Template.fromStack(stack).hasResource('Custom::S3BucketNotifications', {
+      Type: 'Custom::S3BucketNotifications',
+      DependsOn: ['MyBucketPolicyE7FBAC7B'],
+    });
+  });
+
   test('throws with multiple prefix rules in a filter', () => {
     const stack = new cdk.Stack();