fix(s3): bucket notification dependencies (#584)

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 · Aug 16, 2018 · 5164365 · 5164365
1 parent 5812a14
commit 5164365
Show file tree

Hide file tree

Showing 13 changed files with 117 additions and 31 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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;
     }
 }
 

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