From 206af35ac7957587d8e2cd30ba8fd0729d55a4ca Mon Sep 17 00:00:00 2001 From: Yerzhan Mazhkenov <20302932+yerzhan7@users.noreply.github.com> Date: Fri, 3 May 2024 16:58:23 +0100 Subject: [PATCH 1/3] fix(s3): add bucket policy dependency to notification resource Closes #27600 --- .../notifications-resource.ts | 16 ++++- .../aws-s3/test/notification.test.ts | 64 ++++++++++++++++++- 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/packages/aws-cdk-lib/aws-s3/lib/notifications-resource/notifications-resource.ts b/packages/aws-cdk-lib/aws-s3/lib/notifications-resource/notifications-resource.ts index d97ca9bb1498a..b144baa3006c6 100644 --- a/packages/aws-cdk-lib/aws-s3/lib/notifications-resource/notifications-resource.ts +++ b/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; diff --git a/packages/aws-cdk-lib/aws-s3/test/notification.test.ts b/packages/aws-cdk-lib/aws-s3/test/notification.test.ts index fb5b037d74c1d..b0efb5cbe847b 100644 --- a/packages/aws-cdk-lib/aws-s3/test/notification.test.ts +++ b/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(); From c81ad1b17849a0dc3496478701edb57eefa92f84 Mon Sep 17 00:00:00 2001 From: Yerzhan Mazhkenov <20302932+yerzhan7@users.noreply.github.com> Date: Fri, 3 May 2024 18:05:36 +0100 Subject: [PATCH 2/3] Fix snapshots --- .../integ.s3.js.snapshot/lambda-event-source-s3.template.json | 3 ++- .../sqs-bucket-notifications.template.json | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-lambda-event-sources/test/integ.s3.js.snapshot/lambda-event-source-s3.template.json b/packages/@aws-cdk-testing/framework-integ/test/aws-lambda-event-sources/test/integ.s3.js.snapshot/lambda-event-source-s3.template.json index 0288a64914e5c..86b32b9b5104c 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-lambda-event-sources/test/integ.s3.js.snapshot/lambda-event-source-s3.template.json +++ b/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": { diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-s3-notifications/test/sqs/integ.bucket-notifications.js.snapshot/sqs-bucket-notifications.template.json b/packages/@aws-cdk-testing/framework-integ/test/aws-s3-notifications/test/sqs/integ.bucket-notifications.js.snapshot/sqs-bucket-notifications.template.json index 1c5e5194391e6..fa8536c4037e4 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-s3-notifications/test/sqs/integ.bucket-notifications.js.snapshot/sqs-bucket-notifications.template.json +++ b/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" ] From 6f42c4c8a63db9a1110f88cdbb5e190781d615c1 Mon Sep 17 00:00:00 2001 From: Yerzhan Mazhkenov <20302932+yerzhan7@users.noreply.github.com> Date: Sat, 4 May 2024 17:22:28 +0100 Subject: [PATCH 3/3] Update integ test --- .../aws-cdk-s3-notifications.assets.json | 4 +- .../aws-cdk-s3-notifications.template.json | 52 ++++++++++++++- .../manifest.json | 8 ++- .../tree.json | 65 +++++++++++++++++++ .../aws-s3/test/integ.bucket.notifications.ts | 1 + 5 files changed, 126 insertions(+), 4 deletions(-) diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket.notifications.js.snapshot/aws-cdk-s3-notifications.assets.json b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket.notifications.js.snapshot/aws-cdk-s3-notifications.assets.json index c1a1d66de37fd..0b2caef08b8cb 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket.notifications.js.snapshot/aws-cdk-s3-notifications.assets.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket.notifications.js.snapshot/aws-cdk-s3-notifications.assets.json @@ -1,7 +1,7 @@ { "version": "36.0.0", "files": { - "e8c1260052c00a8b1063424232e950e1ca94727f321c62c61cc9ab83f0b31b7b": { + "baecfaa557432e2a3a97967fcd59a796a9fd3426035d03ec0122d8dc3dc96185": { "source": { "path": "aws-cdk-s3-notifications.template.json", "packaging": "file" @@ -9,7 +9,7 @@ "destinations": { "current_account-current_region": { "bucketName": "cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}", - "objectKey": "e8c1260052c00a8b1063424232e950e1ca94727f321c62c61cc9ab83f0b31b7b.json", + "objectKey": "baecfaa557432e2a3a97967fcd59a796a9fd3426035d03ec0122d8dc3dc96185.json", "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-file-publishing-role-${AWS::AccountId}-${AWS::Region}" } } diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket.notifications.js.snapshot/aws-cdk-s3-notifications.template.json b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket.notifications.js.snapshot/aws-cdk-s3-notifications.template.json index af086c7449a66..5e4dce9b0244e 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket.notifications.js.snapshot/aws-cdk-s3-notifications.template.json +++ b/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", diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket.notifications.js.snapshot/manifest.json b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket.notifications.js.snapshot/manifest.json index 5c8e54d43ce08..e3db84eee860e 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket.notifications.js.snapshot/manifest.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket.notifications.js.snapshot/manifest.json @@ -18,7 +18,7 @@ "validateOnSynth": false, "assumeRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-deploy-role-${AWS::AccountId}-${AWS::Region}", "cloudFormationExecutionRoleArn": "arn:${AWS::Partition}:iam::${AWS::AccountId}:role/cdk-hnb659fds-cfn-exec-role-${AWS::AccountId}-${AWS::Region}", - "stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/e8c1260052c00a8b1063424232e950e1ca94727f321c62c61cc9ab83f0b31b7b.json", + "stackTemplateAssetObjectUrl": "s3://cdk-hnb659fds-assets-${AWS::AccountId}-${AWS::Region}/baecfaa557432e2a3a97967fcd59a796a9fd3426035d03ec0122d8dc3dc96185.json", "requiresBootstrapStackVersion": 6, "bootstrapStackVersionSsmParameter": "/cdk-bootstrap/hnb659fds/version", "additionalDependencies": [ @@ -40,6 +40,12 @@ "data": "MyEventBridgeBucket1ABD5C2A" } ], + "/aws-cdk-s3-notifications/MyEventBridgeBucket/Policy/Resource": [ + { + "type": "aws:cdk:logicalId", + "data": "MyEventBridgeBucketPolicy8F5346E3" + } + ], "/aws-cdk-s3-notifications/MyEventBridgeBucket/Notifications/Resource": [ { "type": "aws:cdk:logicalId", diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket.notifications.js.snapshot/tree.json b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket.notifications.js.snapshot/tree.json index 7491cd134c058..01b8d8c23b5a5 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket.notifications.js.snapshot/tree.json +++ b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket.notifications.js.snapshot/tree.json @@ -24,6 +24,71 @@ "version": "0.0.0" } }, + "Policy": { + "id": "Policy", + "path": "aws-cdk-s3-notifications/MyEventBridgeBucket/Policy", + "children": { + "Resource": { + "id": "Resource", + "path": "aws-cdk-s3-notifications/MyEventBridgeBucket/Policy/Resource", + "attributes": { + "aws:cdk:cloudformation:type": "AWS::S3::BucketPolicy", + "aws:cdk:cloudformation:props": { + "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" + } + } + }, + "constructInfo": { + "fqn": "aws-cdk-lib.aws_s3.CfnBucketPolicy", + "version": "0.0.0" + } + } + }, + "constructInfo": { + "fqn": "aws-cdk-lib.aws_s3.BucketPolicy", + "version": "0.0.0" + } + }, "Notifications": { "id": "Notifications", "path": "aws-cdk-s3-notifications/MyEventBridgeBucket/Notifications", diff --git a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket.notifications.ts b/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket.notifications.ts index 53b21dc556028..cc207cdb81a48 100644 --- a/packages/@aws-cdk-testing/framework-integ/test/aws-s3/test/integ.bucket.notifications.ts +++ b/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, });