From c182690ea5131b536b71dc89b1e6a1cd2e1b7664 Mon Sep 17 00:00:00 2001 From: Elad Ben-Israel Date: Thu, 16 Aug 2018 17:19:49 +0300 Subject: [PATCH 1/2] fix(s3): bucket notification dependencies Allow bucket notification destinations to specify dependency elements that are added to the bucket notification resource. This is to ensure that S3 will be able to validate the subscription when notifications are configured. --- .../@aws-cdk/aws-lambda/lib/lambda-ref.ts | 7 +++- .../integ.bucket-notifications.expected.json | 10 ++++-- .../aws-s3-notifications/lib/destination.ts | 10 ++++-- .../notifications-resource.ts | 36 +++++++++++-------- .../aws-s3/test/test.notifications.ts | 29 +++++++++++++++ packages/@aws-cdk/aws-sns/lib/policy.ts | 13 +++++-- packages/@aws-cdk/aws-sns/lib/topic-ref.ts | 3 +- ...teg.sns-bucket-notifications.expected.json | 6 +++- packages/@aws-cdk/aws-sns/test/test.sns.ts | 3 ++ packages/@aws-cdk/aws-sqs/lib/policy.ts | 13 +++++-- packages/@aws-cdk/aws-sqs/lib/queue-ref.ts | 3 +- .../integ.bucket-notifications.expected.json | 11 ++++-- packages/@aws-cdk/aws-sqs/test/test.sqs.ts | 4 +++ 13 files changed, 117 insertions(+), 31 deletions(-) diff --git a/packages/@aws-cdk/aws-lambda/lib/lambda-ref.ts b/packages/@aws-cdk/aws-lambda/lib/lambda-ref.ts index 9bf961f808fbf..505562b050785 100644 --- a/packages/@aws-cdk/aws-lambda/lib/lambda-ref.ts +++ b/packages/@aws-cdk/aws-lambda/lib/lambda-ref.ts @@ -278,9 +278,14 @@ export abstract class FunctionRef extends cdk.Construct }); } + // if we have a permission resource for this relationship, add it as a dependency + // to the bucket notifications resource, so it will be created first. + const permission = this.tryFindChild(permissionId) as cdk.Resource; + return { type: s3n.BucketNotificationDestinationType.Lambda, - arn: this.functionArn + arn: this.functionArn, + dependencies: permission ? [ permission ] : undefined }; } diff --git a/packages/@aws-cdk/aws-lambda/test/integ.bucket-notifications.expected.json b/packages/@aws-cdk/aws-lambda/test/integ.bucket-notifications.expected.json index edbf24da3f3f2..67b30b027eb0f 100644 --- a/packages/@aws-cdk/aws-lambda/test/integ.bucket-notifications.expected.json +++ b/packages/@aws-cdk/aws-lambda/test/integ.bucket-notifications.expected.json @@ -40,7 +40,10 @@ } ] } - } + }, + "DependsOn": [ + "MyFunctionAllowBucketNotificationsFromlambdabucketnotificationsMyBucket0F0FC402189522F6" + ] }, "MyFunctionServiceRole3C357FF2": { "Type": "AWS::IAM::Role", @@ -170,7 +173,10 @@ } ] } - } + }, + "DependsOn": [ + "MyFunctionAllowBucketNotificationsFromlambdabucketnotificationsYourBucket307F72F245F2C5AE" + ] }, "BucketNotificationsHandler050a0587b7544547bf325f094a3db834RoleB6FB88EC": { "Type": "AWS::IAM::Role", diff --git a/packages/@aws-cdk/aws-s3-notifications/lib/destination.ts b/packages/@aws-cdk/aws-s3-notifications/lib/destination.ts index d48960a02c82d..286aa4a80e56d 100644 --- a/packages/@aws-cdk/aws-s3-notifications/lib/destination.ts +++ b/packages/@aws-cdk/aws-s3-notifications/lib/destination.ts @@ -22,12 +22,18 @@ export interface BucketNotificationDestinationProps { /** * The notification type. */ - readonly type: BucketNotificationDestinationType; + type: BucketNotificationDestinationType; /** * The ARN of the destination (i.e. Lambda, SNS, SQS). */ - readonly arn: cdk.Arn; + arn: cdk.Arn; + + /** + * Any additional dependencies that should be resolved before the bucket notification + * can be configured (for example, the SNS Topic Policy resource). + */ + dependencies?: cdk.IDependable[] } /** 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 3dec683c961a5..fd26e13a71044 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 @@ -32,7 +32,7 @@ export class BucketNotifications extends cdk.Construct { private readonly lambdaNotifications = new Array(); private readonly queueNotifications = new Array(); private readonly topicNotifications = new Array(); - private customResourceCreated = false; + private resource?: cdk.Resource; private readonly bucket: Bucket; constructor(parent: cdk.Construct, id: string, props: NotificationsProps) { @@ -49,7 +49,7 @@ export class BucketNotifications extends cdk.Construct { * @param filters A set of S3 key filters */ public addNotification(event: EventType, target: IBucketNotificationDestination, ...filters: NotificationKeyFilter[]) { - this.createResourceOnce(); + const resource = this.createResourceOnce(); // resolve target. this also provides an opportunity for the target to e.g. update // policies to allow this notification to happen. @@ -59,6 +59,13 @@ export class BucketNotifications extends cdk.Construct { Filter: renderFilters(filters), }; + // if the target specifies any dependencies, add them to the custom resource. + // for example, the SNS topic policy must be created /before/ the notification resource. + // otherwise, S3 won't be able to confirm the subscription. + if (targetProps.dependencies) { + resource.addDependency(...targetProps.dependencies); + } + // based on the target type, add the the correct configurations array switch (targetProps.type) { case BucketNotificationDestinationType.Lambda: @@ -92,21 +99,20 @@ export class BucketNotifications extends cdk.Construct { * there is no notifications resource. */ private createResourceOnce() { - if (this.customResourceCreated) { - return; + if (!this.resource) { + const handlerArn = NotificationsResourceHandler.singleton(this); + + this.resource = new cdk.Resource(this, 'Resource', { + type: 'Custom::S3BucketNotifications', + properties: { + ServiceToken: handlerArn, + BucketName: this.bucket.bucketName, + NotificationConfiguration: new cdk.Token(() => this.renderNotificationConfiguration()) + } + }); } - const handlerArn = NotificationsResourceHandler.singleton(this); - new cdk.Resource(this, 'Resource', { - type: 'Custom::S3BucketNotifications', - properties: { - ServiceToken: handlerArn, - BucketName: this.bucket.bucketName, - NotificationConfiguration: new cdk.Token(() => this.renderNotificationConfiguration()) - } - }); - - this.customResourceCreated = true; + return this.resource; } } diff --git a/packages/@aws-cdk/aws-s3/test/test.notifications.ts b/packages/@aws-cdk/aws-s3/test/test.notifications.ts index d8e210fe4fa30..a7235581e726e 100644 --- a/packages/@aws-cdk/aws-s3/test/test.notifications.ts +++ b/packages/@aws-cdk/aws-s3/test/test.notifications.ts @@ -4,6 +4,7 @@ import cdk = require('@aws-cdk/cdk'); import { Test } from 'nodeunit'; import s3 = require('../lib'); import { Topic } from './notification-dests'; +import { Stack } from '@aws-cdk/cdk'; // tslint:disable:object-literal-key-quotes // tslint:disable:max-line-length @@ -268,6 +269,34 @@ export = { } })); + test.done(); + }, + + 'a notification destination can specify a set of dependencies that must be resolved before the notifications resource is created'(test: Test) { + const stack = new Stack(); + + const bucket = new s3.Bucket(stack, 'Bucket'); + const dependent = new cdk.Resource(stack, 'Dependent', { type: 'DependOnMe' }); + const dest: s3n.IBucketNotificationDestination = { + asBucketNotificationDestination: () => ({ + arn: new cdk.Arn('arn'), + type: s3n.BucketNotificationDestinationType.Queue, + dependencies: [ dependent ] + }) + }; + + bucket.onObjectCreated(dest); + + test.deepEqual(stack.toCloudFormation().Resources.BucketNotifications8F2E257D, { + Type: 'Custom::S3BucketNotifications', + Properties: { + ServiceToken: { 'Fn::GetAtt': [ 'BucketNotificationsHandler050a0587b7544547bf325f094a3db8347ECC3691', 'Arn' ] }, + BucketName: { Ref: 'Bucket83908E77' }, + NotificationConfiguration: { QueueConfigurations: [ { Events: [ 's3:ObjectCreated:*' ], QueueArn: 'arn' } ] } + }, + DependsOn: [ 'Dependent' ] + }); + test.done(); } }; diff --git a/packages/@aws-cdk/aws-sns/lib/policy.ts b/packages/@aws-cdk/aws-sns/lib/policy.ts index ac14d700e867f..4a22ee7a2975b 100644 --- a/packages/@aws-cdk/aws-sns/lib/policy.ts +++ b/packages/@aws-cdk/aws-sns/lib/policy.ts @@ -1,4 +1,4 @@ -import { Construct, PolicyDocument } from '@aws-cdk/cdk'; +import { Construct, IDependable, PolicyDocument } from '@aws-cdk/cdk'; import { cloudformation } from './sns.generated'; import { TopicRef } from './topic-ref'; @@ -12,18 +12,25 @@ export interface TopicPolicyProps { /** * Applies a policy to SNS topics. */ -export class TopicPolicy extends Construct { +export class TopicPolicy extends Construct implements IDependable { /** * The IAM policy document for this policy. */ public readonly document = new PolicyDocument(); + /** + * Allows topic policy to be added as a dependency. + */ + public readonly dependencyElements = new Array(); + constructor(parent: Construct, name: string, props: TopicPolicyProps) { super(parent, name); - new cloudformation.TopicPolicyResource(this, 'Resource', { + const resource = new cloudformation.TopicPolicyResource(this, 'Resource', { policyDocument: this.document, topics: props.topics.map(t => t.topicArn) }); + + this.dependencyElements.push(resource); } } diff --git a/packages/@aws-cdk/aws-sns/lib/topic-ref.ts b/packages/@aws-cdk/aws-sns/lib/topic-ref.ts index 563915b5cc65c..ec16ea348d5a3 100644 --- a/packages/@aws-cdk/aws-sns/lib/topic-ref.ts +++ b/packages/@aws-cdk/aws-sns/lib/topic-ref.ts @@ -302,7 +302,8 @@ export abstract class TopicRef extends cdk.Construct implements events.IEventRul return { arn: this.topicArn, - type: s3n.BucketNotificationDestinationType.Topic + type: s3n.BucketNotificationDestinationType.Topic, + dependencies: [ this.policy! ] // make sure the topic policy resource is created before the notification config }; } } diff --git a/packages/@aws-cdk/aws-sns/test/integ.sns-bucket-notifications.expected.json b/packages/@aws-cdk/aws-sns/test/integ.sns-bucket-notifications.expected.json index 011685cf9af87..f282f4ce68fad 100644 --- a/packages/@aws-cdk/aws-sns/test/integ.sns-bucket-notifications.expected.json +++ b/packages/@aws-cdk/aws-sns/test/integ.sns-bucket-notifications.expected.json @@ -127,7 +127,11 @@ } ] } - } + }, + "DependsOn": [ + "ObjectCreatedTopicPolicyA938ECFC", + "ObjectDeletedTopicPolicy026B02E6" + ] }, "BucketNotificationsHandler050a0587b7544547bf325f094a3db834RoleB6FB88EC": { "Type": "AWS::IAM::Role", diff --git a/packages/@aws-cdk/aws-sns/test/test.sns.ts b/packages/@aws-cdk/aws-sns/test/test.sns.ts index 1b44251efb67c..e30672b03fa53 100644 --- a/packages/@aws-cdk/aws-sns/test/test.sns.ts +++ b/packages/@aws-cdk/aws-sns/test/test.sns.ts @@ -703,6 +703,9 @@ export = { test.deepEqual(resolve(dest1.arn), resolve(topic.topicArn)); test.deepEqual(dest1.type, s3n.BucketNotificationDestinationType.Topic); + const dep: cdk.Construct = dest1.dependencies![0] as any; + test.deepEqual((dep.children[0] as any).logicalId, 'MyTopicPolicy12A5EC17', 'verify topic policy is added as dependency'); + // calling again on the same bucket yields is idempotent const dest2 = topic.asBucketNotificationDestination(bucketArn, bucketId); test.deepEqual(resolve(dest2.arn), resolve(topic.topicArn)); diff --git a/packages/@aws-cdk/aws-sqs/lib/policy.ts b/packages/@aws-cdk/aws-sqs/lib/policy.ts index 28c9cff03ecf9..22fd03c361872 100644 --- a/packages/@aws-cdk/aws-sqs/lib/policy.ts +++ b/packages/@aws-cdk/aws-sqs/lib/policy.ts @@ -1,4 +1,4 @@ -import { Construct, PolicyDocument } from '@aws-cdk/cdk'; +import { Construct, IDependable, PolicyDocument } from '@aws-cdk/cdk'; import { QueueRef } from './queue-ref'; import { cloudformation } from './sqs.generated'; @@ -12,18 +12,25 @@ export interface QueuePolicyProps { /** * Applies a policy to SQS queues. */ -export class QueuePolicy extends Construct { +export class QueuePolicy extends Construct implements IDependable { /** * The IAM policy document for this policy. */ public readonly document = new PolicyDocument(); + /** + * Allows adding QueuePolicy as a dependency. + */ + public readonly dependencyElements = new Array(); + constructor(parent: Construct, name: string, props: QueuePolicyProps) { super(parent, name); - new cloudformation.QueuePolicyResource(this, 'Resource', { + const resource = new cloudformation.QueuePolicyResource(this, 'Resource', { policyDocument: this.document, queues: props.queues.map(q => q.queueUrl) }); + + this.dependencyElements.push(resource); } } diff --git a/packages/@aws-cdk/aws-sqs/lib/queue-ref.ts b/packages/@aws-cdk/aws-sqs/lib/queue-ref.ts index df9493566fd3f..047abfa325222 100644 --- a/packages/@aws-cdk/aws-sqs/lib/queue-ref.ts +++ b/packages/@aws-cdk/aws-sqs/lib/queue-ref.ts @@ -106,7 +106,8 @@ export abstract class QueueRef extends cdk.Construct implements s3n.IBucketNotif return { arn: this.queueArn, - type: s3n.BucketNotificationDestinationType.Queue + type: s3n.BucketNotificationDestinationType.Queue, + dependencies: [ this.policy! ] }; } } diff --git a/packages/@aws-cdk/aws-sqs/test/integ.bucket-notifications.expected.json b/packages/@aws-cdk/aws-sqs/test/integ.bucket-notifications.expected.json index 93adc81f02f79..9bca049124ab9 100644 --- a/packages/@aws-cdk/aws-sqs/test/integ.bucket-notifications.expected.json +++ b/packages/@aws-cdk/aws-sqs/test/integ.bucket-notifications.expected.json @@ -41,7 +41,11 @@ } ] } - } + }, + "DependsOn": [ + "MyQueuePolicy6BBEDDAC", + "EncryptedQueuePolicy8AEB1708" + ] }, "MyQueueE6CA6235": { "Type": "AWS::SQS::Queue" @@ -227,7 +231,10 @@ } ] } - } + }, + "DependsOn": [ + "MyQueuePolicy6BBEDDAC" + ] }, "EncryptedQueueKey6F4FD304": { "Type": "AWS::KMS::Key", diff --git a/packages/@aws-cdk/aws-sqs/test/test.sqs.ts b/packages/@aws-cdk/aws-sqs/test/test.sqs.ts index 00aca29685afc..135d37d76a6d2 100644 --- a/packages/@aws-cdk/aws-sqs/test/test.sqs.ts +++ b/packages/@aws-cdk/aws-sqs/test/test.sqs.ts @@ -315,6 +315,10 @@ export = { } })); + // make sure the queue policy is added as a dependency to the bucket + // notifications resource so it will be created first. + test.deepEqual(stack.toCloudFormation().Resources.BucketNotifications8F2E257D.DependsOn, [ 'QueuePolicy25439813' ]); + test.done(); }, From 4d18cc8b4e047e45dee6b2f7eb0a4432317ccd37 Mon Sep 17 00:00:00 2001 From: Elad Ben-Israel Date: Thu, 16 Aug 2018 17:45:40 +0300 Subject: [PATCH 2/2] Fix tslint error --- packages/@aws-cdk/aws-s3/test/test.notifications.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@aws-cdk/aws-s3/test/test.notifications.ts b/packages/@aws-cdk/aws-s3/test/test.notifications.ts index a7235581e726e..f229f9d307bcd 100644 --- a/packages/@aws-cdk/aws-s3/test/test.notifications.ts +++ b/packages/@aws-cdk/aws-s3/test/test.notifications.ts @@ -1,10 +1,10 @@ import { expect, haveResource } from '@aws-cdk/assert'; import s3n = require('@aws-cdk/aws-s3-notifications'); import cdk = require('@aws-cdk/cdk'); +import { Stack } from '@aws-cdk/cdk'; import { Test } from 'nodeunit'; import s3 = require('../lib'); import { Topic } from './notification-dests'; -import { Stack } from '@aws-cdk/cdk'; // tslint:disable:object-literal-key-quotes // tslint:disable:max-line-length