From 57d2bf7e8b81375bb57c56cb27551eecfd92848c Mon Sep 17 00:00:00 2001 From: Otavio Macedo Date: Wed, 1 Dec 2021 11:05:19 +0000 Subject: [PATCH 1/7] feat(s3): support custom role for the bucket notifications handler --- packages/@aws-cdk/aws-s3/README.md | 17 +++++++ packages/@aws-cdk/aws-s3/lib/bucket.ts | 23 ++++++++-- .../notifications-resource-handler.ts | 44 ++++++++++++------- .../notifications-resource.ts | 13 +++++- .../@aws-cdk/aws-s3/test/notification.test.ts | 24 ++++++++++ 5 files changed, 101 insertions(+), 20 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/README.md b/packages/@aws-cdk/aws-s3/README.md index 57d9369a274a7..bf39f448ccee1 100644 --- a/packages/@aws-cdk/aws-s3/README.md +++ b/packages/@aws-cdk/aws-s3/README.md @@ -249,6 +249,23 @@ const bucket = s3.Bucket.fromBucketAttributes(this, 'ImportedBucket', { bucket.addEventNotification(s3.EventType.OBJECT_CREATED, new s3n.SnsDestination(topic)); ``` +When you add an event notification to a bucket, a custom resource is created to +manage the notifications. By default, a new role is created for the Lambda +function that implements this feature. If you want to use your own role instead, +you should provide it in the `Bucket` constructor: + +```ts +declare const importedRole: iam.IRole; +const bucket = new s3.Bucket(this, 'MyBucket', { + notificationsHandlerRole: importedRole, +}); +``` + +> NOTE: If you provide your own handler role, make sure that it has at least +>`s3:PutBucketNotification` and `s3:GetBucketNotification` permissions to the +> bucket as well as the permissions present in `AWSLambdaBasicExecutionRole` (AWS +> managed role). The lack of these permissions will cause the deployment to fail! + [S3 Bucket Notifications]: https://docs.aws.amazon.com/AmazonS3/latest/dev/NotificationHowTo.html diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index 673a1b4f5b561..30079d783f539 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -431,6 +431,19 @@ export interface BucketAttributes { readonly region?: string; } +/** + * Props common to all buckets + */ +export interface BucketBaseProps extends ResourceProps { + + /** + * The role to be used by the notifications handler + * + * @default - a new role will be created. + */ + readonly notificationsHandlerRole?: iam.IRole; +} + /** * Represents an S3 Bucket. * @@ -488,12 +501,15 @@ export abstract class BucketBase extends Resource implements IBucket { private readonly notifications: BucketNotifications; - constructor(scope: Construct, id: string, props: ResourceProps = {}) { + constructor(scope: Construct, id: string, props: BucketBaseProps = {}) { super(scope, id, props); // defines a BucketNotifications construct. Notice that an actual resource will only // be added if there are notifications added, so we don't need to condition this. - this.notifications = new BucketNotifications(this, 'Notifications', { bucket: this }); + this.notifications = new BucketNotifications(this, 'Notifications', { + bucket: this, + handlerRole: props.notificationsHandlerRole, + }); } /** @@ -1215,7 +1231,7 @@ export enum ObjectOwnership { */ OBJECT_WRITER = 'ObjectWriter', } -export interface BucketProps { +export interface BucketProps extends BucketBaseProps { /** * The kind of server-side encryption to apply to this bucket. * @@ -1555,6 +1571,7 @@ export class Bucket extends BucketBase { constructor(scope: Construct, id: string, props: BucketProps = {}) { super(scope, id, { physicalName: props.bucketName, + notificationsHandlerRole: props.notificationsHandlerRole, }); const { bucketEncryption, encryptionKey } = this.parseEncryption(props); diff --git a/packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource-handler.ts b/packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource-handler.ts index c093487a8f105..d51be12c42aab 100644 --- a/packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource-handler.ts +++ b/packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource-handler.ts @@ -7,6 +7,10 @@ import * as cdk from '@aws-cdk/core'; // eslint-disable-next-line no-duplicate-imports, import/order import { Construct } from '@aws-cdk/core'; +export class NotificationsResourceHandlerProps { + role?: iam.IRole; +} + /** * A Lambda-based custom resource handler that provisions S3 bucket * notifications for a bucket. @@ -31,14 +35,14 @@ export class NotificationsResourceHandler extends Construct { * * @returns The ARN of the custom resource lambda function. */ - public static singleton(context: Construct) { + public static singleton(context: Construct, props: NotificationsResourceHandlerProps = {}) { const root = cdk.Stack.of(context); // well-known logical id to ensure stack singletonity const logicalId = 'BucketNotificationsHandler050a0587b7544547bf325f094a3db834'; let lambda = root.node.tryFindChild(logicalId) as NotificationsResourceHandler; if (!lambda) { - lambda = new NotificationsResourceHandler(root, logicalId); + lambda = new NotificationsResourceHandler(root, logicalId, props); } return lambda; @@ -53,22 +57,12 @@ export class NotificationsResourceHandler extends Construct { /** * The role of the handler's lambda function. */ - public readonly role: iam.Role; + public readonly role: iam.IRole; - constructor(scope: Construct, id: string) { + constructor(scope: Construct, id: string, props: NotificationsResourceHandlerProps = {}) { super(scope, id); - this.role = new iam.Role(this, 'Role', { - assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com'), - managedPolicies: [ - iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AWSLambdaBasicExecutionRole'), - ], - }); - - this.role.addToPolicy(new iam.PolicyStatement({ - actions: ['s3:PutBucketNotification'], - resources: ['*'], - })); + this.role = props.role ?? this.createRole(); const resourceType = 'AWS::Lambda::Function'; class InLineLambda extends cdk.CfnResource { @@ -95,4 +89,24 @@ export class NotificationsResourceHandler extends Construct { this.functionArn = resource.getAtt('Arn').toString(); } + + public addToRolePolicy(statement: iam.PolicyStatement) { + this.role.addToPrincipalPolicy(statement); + } + + private createRole(): iam.IRole { + const role = new iam.Role(this, 'Role', { + assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com'), + managedPolicies: [ + iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AWSLambdaBasicExecutionRole'), + ], + }); + + role.addToPrincipalPolicy(new iam.PolicyStatement({ + actions: ['s3:PutBucketNotification'], + resources: ['*'], + })); + + return role; + } } diff --git a/packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource.ts b/packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource.ts index d5190f1a6a913..6bc50ec5b6064 100644 --- a/packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource.ts +++ b/packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource.ts @@ -13,6 +13,11 @@ interface NotificationsProps { * The bucket to manage notifications for. */ bucket: IBucket; + + /** + * The role to be used by the lambda handler + */ + handlerRole?: iam.IRole; } /** @@ -36,10 +41,12 @@ export class BucketNotifications extends Construct { private readonly topicNotifications = new Array(); private resource?: cdk.CfnResource; private readonly bucket: IBucket; + private readonly handlerRole?: iam.IRole; constructor(scope: Construct, id: string, props: NotificationsProps) { super(scope, id); this.bucket = props.bucket; + this.handlerRole = props.handlerRole; } /** @@ -102,12 +109,14 @@ export class BucketNotifications extends Construct { */ private createResourceOnce() { if (!this.resource) { - const handler = NotificationsResourceHandler.singleton(this); + const handler = NotificationsResourceHandler.singleton(this, { + role: this.handlerRole, + }); const managed = this.bucket instanceof Bucket; if (!managed) { - handler.role.addToPolicy(new iam.PolicyStatement({ + handler.addToRolePolicy(new iam.PolicyStatement({ actions: ['s3:GetBucketNotification'], resources: ['*'], })); diff --git a/packages/@aws-cdk/aws-s3/test/notification.test.ts b/packages/@aws-cdk/aws-s3/test/notification.test.ts index 2f3178fb42af0..7fae9f4b5b107 100644 --- a/packages/@aws-cdk/aws-s3/test/notification.test.ts +++ b/packages/@aws-cdk/aws-s3/test/notification.test.ts @@ -1,5 +1,6 @@ import '@aws-cdk/assert-internal/jest'; import { ResourcePart } from '@aws-cdk/assert-internal'; +import * as iam from '@aws-cdk/aws-iam'; import * as cdk from '@aws-cdk/core'; import * as s3 from '../lib'; @@ -33,6 +34,29 @@ describe('notification', () => { }); + test('can specify a custom role for the notifications handler', () => { + const stack = new cdk.Stack(); + + const importedRole = iam.Role.fromRoleArn(stack, 'role', 'arn:aws:iam::111111111111:role/DevsNotAllowedToTouch'); + + const bucket = new s3.Bucket(stack, 'MyBucket', { + notificationsHandlerRole: importedRole, + }); + + bucket.addEventNotification(s3.EventType.OBJECT_CREATED, { + bind: () => ({ + arn: 'ARN', + type: s3.BucketNotificationDestinationType.TOPIC, + }), + }); + + expect(stack).toHaveResource('AWS::S3::Bucket'); + expect(stack).toHaveResource('AWS::Lambda::Function', { + Description: 'AWS CloudFormation handler for "Custom::S3BucketNotifications" resources (@aws-cdk/aws-s3)', + Role: 'arn:aws:iam::111111111111:role/DevsNotAllowedToTouch', + }); + }); + test('can specify prefix and suffix filter rules', () => { const stack = new cdk.Stack(); From 95ccef14d711d9df65443afccc39e13514f63ca2 Mon Sep 17 00:00:00 2001 From: Otavio Macedo Date: Thu, 2 Dec 2021 12:55:37 +0000 Subject: [PATCH 2/7] * Added role to `Import` buckets * Kept policy management * Improved README --- .../cloudwatch/cloudwatch-logs-action.test.ts | 2 +- packages/@aws-cdk/aws-s3/README.md | 22 +++++++++++---- packages/@aws-cdk/aws-s3/lib/bucket.ts | 8 ++++++ .../notifications-resource-handler.ts | 28 ++++++++----------- .../@aws-cdk/aws-s3/test/notification.test.ts | 23 +++++++++++++++ 5 files changed, 59 insertions(+), 24 deletions(-) diff --git a/packages/@aws-cdk/aws-iot-actions/test/cloudwatch/cloudwatch-logs-action.test.ts b/packages/@aws-cdk/aws-iot-actions/test/cloudwatch/cloudwatch-logs-action.test.ts index 4499cdd35d6f1..9dfd3b290895b 100644 --- a/packages/@aws-cdk/aws-iot-actions/test/cloudwatch/cloudwatch-logs-action.test.ts +++ b/packages/@aws-cdk/aws-iot-actions/test/cloudwatch/cloudwatch-logs-action.test.ts @@ -82,7 +82,7 @@ test('can set role', () => { sql: iot.IotSql.fromStringAsVer20160323("SELECT topic(2) as device_id FROM 'device/+/data'"), }); const logGroup = logs.LogGroup.fromLogGroupArn(stack, 'my-log-group', 'arn:aws:logs:us-east-1:123456789012:log-group:my-log-group'); - const role = iam.Role.fromRoleArn(stack, 'MyRole', 'arn:aws:iam::123456789012:role/ForTest'); + const role = iam.Role.fromRoleArn(stack, 'MyRole', 'arn:aws:iam::123456789012:role/ForTest', {mutable: false}); // WHEN topicRule.addAction( diff --git a/packages/@aws-cdk/aws-s3/README.md b/packages/@aws-cdk/aws-s3/README.md index bf39f448ccee1..6bd3f19b98372 100644 --- a/packages/@aws-cdk/aws-s3/README.md +++ b/packages/@aws-cdk/aws-s3/README.md @@ -255,16 +255,26 @@ function that implements this feature. If you want to use your own role instead, you should provide it in the `Bucket` constructor: ```ts -declare const importedRole: iam.IRole; +declare const myRole: iam.IRole; const bucket = new s3.Bucket(this, 'MyBucket', { - notificationsHandlerRole: importedRole, + notificationsHandlerRole: myRole, }); ``` -> NOTE: If you provide your own handler role, make sure that it has at least ->`s3:PutBucketNotification` and `s3:GetBucketNotification` permissions to the -> bucket as well as the permissions present in `AWSLambdaBasicExecutionRole` (AWS -> managed role). The lack of these permissions will cause the deployment to fail! +Whatever role you provide, the CDK will try to modify it by adding the +permissions from `AWSLambdaBasicExecutionRole` (an AWS managed policy) as well +as the permissions `s3:PutBucketNotification` and `s3:GetBucketNotification`. +If you’re passing an imported role, and you don’t want this to happen, configure +it to be immutable: + +```ts +const importedRole = iam.Role.fromRoleArn(this, 'role', 'arn:aws:iam::123456789012:role/RoleName', { + mutable: false, +}); +``` + +> If you provide an imported immutable role, make sure that it has at least all +> the permissions mentioned above. Otherwise, the deployment will fail! [S3 Bucket Notifications]: https://docs.aws.amazon.com/AmazonS3/latest/dev/NotificationHowTo.html diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index 30079d783f539..2c8be6c90b895 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -429,6 +429,13 @@ export interface BucketAttributes { * @default - it's assumed the bucket is in the same region as the scope it's being imported into */ readonly region?: string; + + /** + * The role to be used by the notifications handler + * + * @default - a new role will be created. + */ + readonly notificationsHandlerRole?: iam.IRole; } /** @@ -1498,6 +1505,7 @@ export class Bucket extends BucketBase { return new Import(scope, id, { account: attrs.account, region: attrs.region, + notificationsHandlerRole: attrs.notificationsHandlerRole, }); } diff --git a/packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource-handler.ts b/packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource-handler.ts index d51be12c42aab..5a3955a96da9f 100644 --- a/packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource-handler.ts +++ b/packages/@aws-cdk/aws-s3/lib/notifications-resource/notifications-resource-handler.ts @@ -62,7 +62,17 @@ export class NotificationsResourceHandler extends Construct { constructor(scope: Construct, id: string, props: NotificationsResourceHandlerProps = {}) { super(scope, id); - this.role = props.role ?? this.createRole(); + this.role = props.role ?? new iam.Role(this, 'Role', { + assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com'), + }); + + this.role.addManagedPolicy( + iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AWSLambdaBasicExecutionRole'), + ); + this.role.addToPrincipalPolicy(new iam.PolicyStatement({ + actions: ['s3:PutBucketNotification'], + resources: ['*'], + })); const resourceType = 'AWS::Lambda::Function'; class InLineLambda extends cdk.CfnResource { @@ -93,20 +103,4 @@ export class NotificationsResourceHandler extends Construct { public addToRolePolicy(statement: iam.PolicyStatement) { this.role.addToPrincipalPolicy(statement); } - - private createRole(): iam.IRole { - const role = new iam.Role(this, 'Role', { - assumedBy: new iam.ServicePrincipal('lambda.amazonaws.com'), - managedPolicies: [ - iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AWSLambdaBasicExecutionRole'), - ], - }); - - role.addToPrincipalPolicy(new iam.PolicyStatement({ - actions: ['s3:PutBucketNotification'], - resources: ['*'], - })); - - return role; - } } diff --git a/packages/@aws-cdk/aws-s3/test/notification.test.ts b/packages/@aws-cdk/aws-s3/test/notification.test.ts index 7fae9f4b5b107..8e1efa4615d8f 100644 --- a/packages/@aws-cdk/aws-s3/test/notification.test.ts +++ b/packages/@aws-cdk/aws-s3/test/notification.test.ts @@ -57,6 +57,29 @@ describe('notification', () => { }); }); + test('can specify a custom role for the notifications handler of imported buckets', () => { + const stack = new cdk.Stack(); + + const importedRole = iam.Role.fromRoleArn(stack, 'role', 'arn:aws:iam::111111111111:role/DevsNotAllowedToTouch'); + + const bucket = s3.Bucket.fromBucketAttributes(stack, 'MyBucket', { + bucketName: 'foo-bar', + notificationsHandlerRole: importedRole, + }); + + bucket.addEventNotification(s3.EventType.OBJECT_CREATED, { + bind: () => ({ + arn: 'ARN', + type: s3.BucketNotificationDestinationType.TOPIC, + }), + }); + + expect(stack).toHaveResourceLike('AWS::Lambda::Function', { + Description: 'AWS CloudFormation handler for "Custom::S3BucketNotifications" resources (@aws-cdk/aws-s3)', + Role: 'arn:aws:iam::111111111111:role/DevsNotAllowedToTouch', + }); + }); + test('can specify prefix and suffix filter rules', () => { const stack = new cdk.Stack(); From 199f2963ebebc71679d179d4bfbbdf3fba2c834b Mon Sep 17 00:00:00 2001 From: Otavio Macedo Date: Thu, 2 Dec 2021 12:57:19 +0000 Subject: [PATCH 3/7] Removed change added by mistake --- .../test/cloudwatch/cloudwatch-logs-action.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-iot-actions/test/cloudwatch/cloudwatch-logs-action.test.ts b/packages/@aws-cdk/aws-iot-actions/test/cloudwatch/cloudwatch-logs-action.test.ts index 9dfd3b290895b..4499cdd35d6f1 100644 --- a/packages/@aws-cdk/aws-iot-actions/test/cloudwatch/cloudwatch-logs-action.test.ts +++ b/packages/@aws-cdk/aws-iot-actions/test/cloudwatch/cloudwatch-logs-action.test.ts @@ -82,7 +82,7 @@ test('can set role', () => { sql: iot.IotSql.fromStringAsVer20160323("SELECT topic(2) as device_id FROM 'device/+/data'"), }); const logGroup = logs.LogGroup.fromLogGroupArn(stack, 'my-log-group', 'arn:aws:logs:us-east-1:123456789012:log-group:my-log-group'); - const role = iam.Role.fromRoleArn(stack, 'MyRole', 'arn:aws:iam::123456789012:role/ForTest', {mutable: false}); + const role = iam.Role.fromRoleArn(stack, 'MyRole', 'arn:aws:iam::123456789012:role/ForTest'); // WHEN topicRule.addAction( From fcc4552cf817d9b05d44ce5e6c5914b90570b5f5 Mon Sep 17 00:00:00 2001 From: Otavio Macedo Date: Fri, 3 Dec 2021 13:01:08 +0000 Subject: [PATCH 4/7] Lazily creating the notifications object --- packages/@aws-cdk/aws-s3/lib/bucket.ts | 48 ++++++++++++-------------- 1 file changed, 23 insertions(+), 25 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index 2c8be6c90b895..6b057c818c2c5 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -438,19 +438,6 @@ export interface BucketAttributes { readonly notificationsHandlerRole?: iam.IRole; } -/** - * Props common to all buckets - */ -export interface BucketBaseProps extends ResourceProps { - - /** - * The role to be used by the notifications handler - * - * @default - a new role will be created. - */ - readonly notificationsHandlerRole?: iam.IRole; -} - /** * Represents an S3 Bucket. * @@ -506,17 +493,14 @@ export abstract class BucketBase extends Resource implements IBucket { */ protected abstract disallowPublicAccess?: boolean; - private readonly notifications: BucketNotifications; + private notifications?: BucketNotifications; - constructor(scope: Construct, id: string, props: BucketBaseProps = {}) { - super(scope, id, props); + protected notificationsHandlerRole?: iam.IRole; - // defines a BucketNotifications construct. Notice that an actual resource will only - // be added if there are notifications added, so we don't need to condition this. - this.notifications = new BucketNotifications(this, 'Notifications', { - bucket: this, - handlerRole: props.notificationsHandlerRole, - }); + protected handlerRole?: iam.IRole; + + constructor(scope: Construct, id: string, props: ResourceProps = {}) { + super(scope, id, props); } /** @@ -861,6 +845,12 @@ export abstract class BucketBase extends Resource implements IBucket { * https://docs.aws.amazon.com/AmazonS3/latest/dev/NotificationHowTo.html */ public addEventNotification(event: EventType, dest: IBucketNotificationDestination, ...filters: NotificationKeyFilter[]) { + if (!this.notifications) { + this.notifications = new BucketNotifications(this, 'Notifications', { + bucket: this, + handlerRole: this.notificationsHandlerRole, + }); + } this.notifications.addNotification(event, dest, ...filters); } @@ -1238,7 +1228,7 @@ export enum ObjectOwnership { */ OBJECT_WRITER = 'ObjectWriter', } -export interface BucketProps extends BucketBaseProps { +export interface BucketProps { /** * The kind of server-side encryption to apply to this bucket. * @@ -1434,6 +1424,13 @@ export interface BucketProps extends BucketBaseProps { * @default false */ readonly transferAcceleration?: boolean; + + /** + * The role to be used by the notifications handler + * + * @default - a new role will be created. + */ + readonly notificationsHandlerRole?: iam.IRole; } /** @@ -1493,6 +1490,7 @@ export class Bucket extends BucketBase { public policy?: BucketPolicy = undefined; protected autoCreatePolicy = false; protected disallowPublicAccess = false; + protected notificationsHandlerRole = attrs.notificationsHandlerRole; /** * Exports this bucket from the stack. @@ -1505,7 +1503,6 @@ export class Bucket extends BucketBase { return new Import(scope, id, { account: attrs.account, region: attrs.region, - notificationsHandlerRole: attrs.notificationsHandlerRole, }); } @@ -1579,9 +1576,10 @@ export class Bucket extends BucketBase { constructor(scope: Construct, id: string, props: BucketProps = {}) { super(scope, id, { physicalName: props.bucketName, - notificationsHandlerRole: props.notificationsHandlerRole, }); + this.notificationsHandlerRole = props.notificationsHandlerRole; + const { bucketEncryption, encryptionKey } = this.parseEncryption(props); Bucket.validateBucketName(this.physicalName); From fc6bdc318d0a798f7f8a8360a6ddeb12d4c1f77e Mon Sep 17 00:00:00 2001 From: Otavio Macedo Date: Thu, 13 Jan 2022 14:56:27 +0000 Subject: [PATCH 5/7] Fixed tests and adapted to the new use case of enabling event bridge notifications --- packages/@aws-cdk/aws-s3/lib/bucket.ts | 10 +++++++--- packages/@aws-cdk/aws-s3/test/notification.test.ts | 10 ++++------ 2 files changed, 11 insertions(+), 9 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index 1eb03f7465f2e..87e02a60265ac 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -845,13 +845,17 @@ export abstract class BucketBase extends Resource implements IBucket { * https://docs.aws.amazon.com/AmazonS3/latest/dev/NotificationHowTo.html */ public addEventNotification(event: EventType, dest: IBucketNotificationDestination, ...filters: NotificationKeyFilter[]) { + this.withNotifications(notifications => notifications.addNotification(event, dest, ...filters)); + } + + private withNotifications(cb: (notifications: BucketNotifications) => void) { if (!this.notifications) { this.notifications = new BucketNotifications(this, 'Notifications', { bucket: this, handlerRole: this.notificationsHandlerRole, }); } - this.notifications.addNotification(event, dest, ...filters); + cb(this.notifications); } /** @@ -879,7 +883,7 @@ export abstract class BucketBase extends Resource implements IBucket { } protected enableEventBridgeNotification() { - this.notifications.enableEventBridgeNotification(); + this.withNotifications(notifications => notifications.enableEventBridgeNotification()); } private get writeActions(): string[] { @@ -1491,7 +1495,7 @@ export interface BucketProps { * @default - a new role will be created. */ readonly notificationsHandlerRole?: iam.IRole; - + /** * Inteligent Tiering Configurations * diff --git a/packages/@aws-cdk/aws-s3/test/notification.test.ts b/packages/@aws-cdk/aws-s3/test/notification.test.ts index ee75a7d5c1171..39caea3a5db2e 100644 --- a/packages/@aws-cdk/aws-s3/test/notification.test.ts +++ b/packages/@aws-cdk/aws-s3/test/notification.test.ts @@ -1,7 +1,5 @@ -import '@aws-cdk/assert-internal/jest'; -import { ResourcePart } from '@aws-cdk/assert-internal'; -import * as iam from '@aws-cdk/aws-iam'; import { Template } from '@aws-cdk/assertions'; +import * as iam from '@aws-cdk/aws-iam'; import * as cdk from '@aws-cdk/core'; import * as s3 from '../lib'; @@ -49,8 +47,8 @@ describe('notification', () => { }), }); - expect(stack).toHaveResource('AWS::S3::Bucket'); - expect(stack).toHaveResource('AWS::Lambda::Function', { + Template.fromStack(stack).resourceCountIs('AWS::S3::Bucket', 1); + Template.fromStack(stack).hasResourceProperties('AWS::Lambda::Function', { Description: 'AWS CloudFormation handler for "Custom::S3BucketNotifications" resources (@aws-cdk/aws-s3)', Role: 'arn:aws:iam::111111111111:role/DevsNotAllowedToTouch', }); @@ -73,7 +71,7 @@ describe('notification', () => { }), }); - expect(stack).toHaveResourceLike('AWS::Lambda::Function', { + Template.fromStack(stack).hasResourceProperties('AWS::Lambda::Function', { Description: 'AWS CloudFormation handler for "Custom::S3BucketNotifications" resources (@aws-cdk/aws-s3)', Role: 'arn:aws:iam::111111111111:role/DevsNotAllowedToTouch', }); From eb98d63a897183581756627e5f0d3456d5721965 Mon Sep 17 00:00:00 2001 From: Otavio Macedo Date: Fri, 14 Jan 2022 11:42:12 +0000 Subject: [PATCH 6/7] removed unused property --- packages/@aws-cdk/aws-s3/lib/bucket.ts | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/lib/bucket.ts b/packages/@aws-cdk/aws-s3/lib/bucket.ts index 87e02a60265ac..5e37e247b2059 100644 --- a/packages/@aws-cdk/aws-s3/lib/bucket.ts +++ b/packages/@aws-cdk/aws-s3/lib/bucket.ts @@ -497,8 +497,6 @@ export abstract class BucketBase extends Resource implements IBucket { protected notificationsHandlerRole?: iam.IRole; - protected handlerRole?: iam.IRole; - constructor(scope: Construct, id: string, props: ResourceProps = {}) { super(scope, id, props); } From a7e0f0ab09b9d54acfbd730f148f4dc875fb4139 Mon Sep 17 00:00:00 2001 From: Otavio Macedo Date: Mon, 24 Jan 2022 11:19:13 +0000 Subject: [PATCH 7/7] Merged from master --- ...g.bucket-auto-delete-objects.expected.json | 36 +++++++++---------- .../test/integ.bucket-inventory.expected.json | 2 +- .../integ.bucket-sharing.lit.expected.json | 2 +- .../aws-s3/test/integ.bucket.expected.json | 2 +- .../test/integ.bucket.url.lit.expected.json | 12 +++++-- .../@aws-cdk/aws-s3/test/notification.test.ts | 23 ------------ 6 files changed, 30 insertions(+), 47 deletions(-) diff --git a/packages/@aws-cdk/aws-s3/test/integ.bucket-auto-delete-objects.expected.json b/packages/@aws-cdk/aws-s3/test/integ.bucket-auto-delete-objects.expected.json index f5cf756e8a75d..da2c8cf503fe6 100644 --- a/packages/@aws-cdk/aws-s3/test/integ.bucket-auto-delete-objects.expected.json +++ b/packages/@aws-cdk/aws-s3/test/integ.bucket-auto-delete-objects.expected.json @@ -110,7 +110,7 @@ "Properties": { "Code": { "S3Bucket": { - "Ref": "AssetParameters84e9b89449fe2573e51d08cc143e21116ed4608c6db56afffcb4ad85c8130709S3Bucket2C6C817C" + "Ref": "AssetParametersbe270bbdebe0851c887569796e3997437cca54ce86893ed94788500448e92824S3Bucket09A62232" }, "S3Key": { "Fn::Join": [ @@ -123,7 +123,7 @@ "Fn::Split": [ "||", { - "Ref": "AssetParameters84e9b89449fe2573e51d08cc143e21116ed4608c6db56afffcb4ad85c8130709S3VersionKeyFA215BD6" + "Ref": "AssetParametersbe270bbdebe0851c887569796e3997437cca54ce86893ed94788500448e92824S3VersionKeyA28118BE" } ] } @@ -136,7 +136,7 @@ "Fn::Split": [ "||", { - "Ref": "AssetParameters84e9b89449fe2573e51d08cc143e21116ed4608c6db56afffcb4ad85c8130709S3VersionKeyFA215BD6" + "Ref": "AssetParametersbe270bbdebe0851c887569796e3997437cca54ce86893ed94788500448e92824S3VersionKeyA28118BE" } ] } @@ -228,7 +228,7 @@ "Properties": { "Code": { "S3Bucket": { - "Ref": "AssetParameters618bbe9863c0edd5c4ca2e24b5063762f020fafec018cd06f57e2bd9f2f48abfS3BucketE1985B35" + "Ref": "AssetParameters31552cb1c5c4cdb0d9502dc59c3cd63cb519dcb7e320e60965c75940297ae3b6S3BucketB51EC107" }, "S3Key": { "Fn::Join": [ @@ -241,7 +241,7 @@ "Fn::Split": [ "||", { - "Ref": "AssetParameters618bbe9863c0edd5c4ca2e24b5063762f020fafec018cd06f57e2bd9f2f48abfS3VersionKey610C6DE2" + "Ref": "AssetParameters31552cb1c5c4cdb0d9502dc59c3cd63cb519dcb7e320e60965c75940297ae3b6S3VersionKey2B267DB5" } ] } @@ -254,7 +254,7 @@ "Fn::Split": [ "||", { - "Ref": "AssetParameters618bbe9863c0edd5c4ca2e24b5063762f020fafec018cd06f57e2bd9f2f48abfS3VersionKey610C6DE2" + "Ref": "AssetParameters31552cb1c5c4cdb0d9502dc59c3cd63cb519dcb7e320e60965c75940297ae3b6S3VersionKey2B267DB5" } ] } @@ -297,29 +297,29 @@ } }, "Parameters": { - "AssetParameters84e9b89449fe2573e51d08cc143e21116ed4608c6db56afffcb4ad85c8130709S3Bucket2C6C817C": { + "AssetParametersbe270bbdebe0851c887569796e3997437cca54ce86893ed94788500448e92824S3Bucket09A62232": { "Type": "String", - "Description": "S3 bucket for asset \"84e9b89449fe2573e51d08cc143e21116ed4608c6db56afffcb4ad85c8130709\"" + "Description": "S3 bucket for asset \"be270bbdebe0851c887569796e3997437cca54ce86893ed94788500448e92824\"" }, - "AssetParameters84e9b89449fe2573e51d08cc143e21116ed4608c6db56afffcb4ad85c8130709S3VersionKeyFA215BD6": { + "AssetParametersbe270bbdebe0851c887569796e3997437cca54ce86893ed94788500448e92824S3VersionKeyA28118BE": { "Type": "String", - "Description": "S3 key for asset version \"84e9b89449fe2573e51d08cc143e21116ed4608c6db56afffcb4ad85c8130709\"" + "Description": "S3 key for asset version \"be270bbdebe0851c887569796e3997437cca54ce86893ed94788500448e92824\"" }, - "AssetParameters84e9b89449fe2573e51d08cc143e21116ed4608c6db56afffcb4ad85c8130709ArtifactHash17D48178": { + "AssetParametersbe270bbdebe0851c887569796e3997437cca54ce86893ed94788500448e92824ArtifactHash76F8FCF2": { "Type": "String", - "Description": "Artifact hash for asset \"84e9b89449fe2573e51d08cc143e21116ed4608c6db56afffcb4ad85c8130709\"" + "Description": "Artifact hash for asset \"be270bbdebe0851c887569796e3997437cca54ce86893ed94788500448e92824\"" }, - "AssetParameters618bbe9863c0edd5c4ca2e24b5063762f020fafec018cd06f57e2bd9f2f48abfS3BucketE1985B35": { + "AssetParameters31552cb1c5c4cdb0d9502dc59c3cd63cb519dcb7e320e60965c75940297ae3b6S3BucketB51EC107": { "Type": "String", - "Description": "S3 bucket for asset \"618bbe9863c0edd5c4ca2e24b5063762f020fafec018cd06f57e2bd9f2f48abf\"" + "Description": "S3 bucket for asset \"31552cb1c5c4cdb0d9502dc59c3cd63cb519dcb7e320e60965c75940297ae3b6\"" }, - "AssetParameters618bbe9863c0edd5c4ca2e24b5063762f020fafec018cd06f57e2bd9f2f48abfS3VersionKey610C6DE2": { + "AssetParameters31552cb1c5c4cdb0d9502dc59c3cd63cb519dcb7e320e60965c75940297ae3b6S3VersionKey2B267DB5": { "Type": "String", - "Description": "S3 key for asset version \"618bbe9863c0edd5c4ca2e24b5063762f020fafec018cd06f57e2bd9f2f48abf\"" + "Description": "S3 key for asset version \"31552cb1c5c4cdb0d9502dc59c3cd63cb519dcb7e320e60965c75940297ae3b6\"" }, - "AssetParameters618bbe9863c0edd5c4ca2e24b5063762f020fafec018cd06f57e2bd9f2f48abfArtifactHash467DFC33": { + "AssetParameters31552cb1c5c4cdb0d9502dc59c3cd63cb519dcb7e320e60965c75940297ae3b6ArtifactHashEE982197": { "Type": "String", - "Description": "Artifact hash for asset \"618bbe9863c0edd5c4ca2e24b5063762f020fafec018cd06f57e2bd9f2f48abf\"" + "Description": "Artifact hash for asset \"31552cb1c5c4cdb0d9502dc59c3cd63cb519dcb7e320e60965c75940297ae3b6\"" } } } \ No newline at end of file diff --git a/packages/@aws-cdk/aws-s3/test/integ.bucket-inventory.expected.json b/packages/@aws-cdk/aws-s3/test/integ.bucket-inventory.expected.json index f5610756ad71e..a142de99be8b0 100644 --- a/packages/@aws-cdk/aws-s3/test/integ.bucket-inventory.expected.json +++ b/packages/@aws-cdk/aws-s3/test/integ.bucket-inventory.expected.json @@ -155,4 +155,4 @@ } } } -} +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-s3/test/integ.bucket-sharing.lit.expected.json b/packages/@aws-cdk/aws-s3/test/integ.bucket-sharing.lit.expected.json index 4197e9179b4ff..083db4157734a 100644 --- a/packages/@aws-cdk/aws-s3/test/integ.bucket-sharing.lit.expected.json +++ b/packages/@aws-cdk/aws-s3/test/integ.bucket-sharing.lit.expected.json @@ -71,4 +71,4 @@ } } } -] +] \ No newline at end of file diff --git a/packages/@aws-cdk/aws-s3/test/integ.bucket.expected.json b/packages/@aws-cdk/aws-s3/test/integ.bucket.expected.json index 58cd3c5760961..b58901930d7c1 100644 --- a/packages/@aws-cdk/aws-s3/test/integ.bucket.expected.json +++ b/packages/@aws-cdk/aws-s3/test/integ.bucket.expected.json @@ -173,4 +173,4 @@ } } } -} +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-s3/test/integ.bucket.url.lit.expected.json b/packages/@aws-cdk/aws-s3/test/integ.bucket.url.lit.expected.json index 37a7d24a40029..6ab5dcbfef26e 100644 --- a/packages/@aws-cdk/aws-s3/test/integ.bucket.url.lit.expected.json +++ b/packages/@aws-cdk/aws-s3/test/integ.bucket.url.lit.expected.json @@ -44,7 +44,10 @@ [ "https://", { - "Fn::GetAtt": ["MyBucketF68F3FF0", "RegionalDomainName"] + "Fn::GetAtt": [ + "MyBucketF68F3FF0", + "RegionalDomainName" + ] }, "/myfolder/myfile.txt" ] @@ -58,7 +61,10 @@ [ "https://", { - "Fn::GetAtt": ["MyBucketF68F3FF0", "DomainName"] + "Fn::GetAtt": [ + "MyBucketF68F3FF0", + "DomainName" + ] }, "/myfolder/myfile.txt" ] @@ -80,4 +86,4 @@ } } } -} +} \ No newline at end of file diff --git a/packages/@aws-cdk/aws-s3/test/notification.test.ts b/packages/@aws-cdk/aws-s3/test/notification.test.ts index bebf95f85dfda..411852018d081 100644 --- a/packages/@aws-cdk/aws-s3/test/notification.test.ts +++ b/packages/@aws-cdk/aws-s3/test/notification.test.ts @@ -31,29 +31,6 @@ describe('notification', () => { }); }); - test('can specify a custom role for the notifications handler', () => { - const stack = new cdk.Stack(); - - const importedRole = iam.Role.fromRoleArn(stack, 'role', 'arn:aws:iam::111111111111:role/DevsNotAllowedToTouch'); - - const bucket = new s3.Bucket(stack, 'MyBucket', { - notificationsHandlerRole: importedRole, - }); - - bucket.addEventNotification(s3.EventType.OBJECT_CREATED, { - bind: () => ({ - arn: 'ARN', - type: s3.BucketNotificationDestinationType.TOPIC, - }), - }); - - Template.fromStack(stack).resourceCountIs('AWS::S3::Bucket', 1); - Template.fromStack(stack).hasResourceProperties('AWS::Lambda::Function', { - Description: 'AWS CloudFormation handler for "Custom::S3BucketNotifications" resources (@aws-cdk/aws-s3)', - Role: 'arn:aws:iam::111111111111:role/DevsNotAllowedToTouch', - }); - }); - test('can specify a custom role for the notifications handler of imported buckets', () => { const stack = new cdk.Stack();