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

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

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