Fix: S3 bucket notification dependencies

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
  };
  }
 

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",

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[]
 }
 
 /**

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<LambdaFunctionConfiguration>();
  private readonly queueNotifications = new Array<QueueConfiguration>();
  private readonly topicNotifications = new Array<TopicConfiguration>();
- 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;
  }
 }
 

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();
  }
 };

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<IDependable>();
+
  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);
  }
 }

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
  };
  }
 }

packages/@aws-cdk/aws-sns/test/integ.sns-bucket-notifications.expected.json

@@ -127,7 +127,11 @@
  }
  ]
  }
- }
+ },
+ "DependsOn": [
+ "ObjectCreatedTopicPolicyA938ECFC",
+ "ObjectDeletedTopicPolicy026B02E6"
+ ]
  },
  "BucketNotificationsHandler050a0587b7544547bf325f094a3db834RoleB6FB88EC": {
  "Type": "AWS::IAM::Role",

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));

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<IDependable>();
+
  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);
  }
 }

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! ]
  };
  }
 }

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",

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();
  },