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..f229f9d307bcd 100644 --- a/packages/@aws-cdk/aws-s3/test/test.notifications.ts +++ b/packages/@aws-cdk/aws-s3/test/test.notifications.ts @@ -1,6 +1,7 @@ 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'; @@ -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(); },