fix(s3-notifications): lambda destination creates a circular dependen…

…cy when bucket and lambda are in different stacks (#10426) When the bucket and function are in two different stacks, the following stacks are created: ### Bucket Stack - `s3.Bucket` - `s3.BucketNotificationHandler` (creates a dependency on **lambda stack** since it configures the target of the trigger) ### Lambda Stack - `lambda.Function` - `lambda.Permission` (creates a dependency on the **bucket stack** since it configures the lambda to allow invocations from that specific bucket) The solution is to switch up the `lambda.Permission` scope and use the bucket instead of the function, so that it is added to the bucket stack, leaving the lambda stack independent. Fixes #5760 ---- *By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
aws · Sep 21, 2020 · 7222b5d · 7222b5d
1 parent 84b9d5e
commit 7222b5d
Show file tree

Hide file tree

Showing 4 changed files with 179 additions and 74 deletions.
diff --git a/packages/@aws-cdk/aws-lambda-event-sources/test/integ.s3.expected.json b/packages/@aws-cdk/aws-lambda-event-sources/test/integ.s3.expected.json
@@ -50,28 +50,6 @@
         "FServiceRole3AC82EE1"
       ]
     },
-    "FAllowBucketNotificationsFromlambdaeventsources3B101C8CFBA8EBB2AA": {
-      "Type": "AWS::Lambda::Permission",
-      "Properties": {
-        "Action": "lambda:InvokeFunction",
-        "FunctionName": {
-          "Fn::GetAtt": [
-            "FC4345940",
-            "Arn"
-          ]
-        },
-        "Principal": "s3.amazonaws.com",
-        "SourceAccount": {
-          "Ref": "AWS::AccountId"
-        },
-        "SourceArn": {
-          "Fn::GetAtt": [
-            "B08E7C7AF",
-            "Arn"
-          ]
-        }
-      }
-    },
     "B08E7C7AF": {
       "Type": "AWS::S3::Bucket",
       "UpdateReplacePolicy": "Delete",
@@ -116,9 +94,31 @@
         }
       },
       "DependsOn": [
-        "FAllowBucketNotificationsFromlambdaeventsources3B101C8CFBA8EBB2AA"
+        "BAllowBucketNotificationsTolambdaeventsources3F741608059EF9F709"
       ]
     },
+    "BAllowBucketNotificationsTolambdaeventsources3F741608059EF9F709": {
+      "Type": "AWS::Lambda::Permission",
+      "Properties": {
+        "Action": "lambda:InvokeFunction",
+        "FunctionName": {
+          "Fn::GetAtt": [
+            "FC4345940",
+            "Arn"
+          ]
+        },
+        "Principal": "s3.amazonaws.com",
+        "SourceAccount": {
+          "Ref": "AWS::AccountId"
+        },
+        "SourceArn": {
+          "Fn::GetAtt": [
+            "B08E7C7AF",
+            "Arn"
+          ]
+        }
+      }
+    },
     "BucketNotificationsHandler050a0587b7544547bf325f094a3db834RoleB6FB88EC": {
       "Type": "AWS::IAM::Role",
       "Properties": {

diff --git a/packages/@aws-cdk/aws-s3-notifications/lib/lambda.ts b/packages/@aws-cdk/aws-s3-notifications/lib/lambda.ts
@@ -11,19 +11,28 @@ export class LambdaDestination implements s3.IBucketNotificationDestination {
   }
 
   public bind(_scope: Construct, bucket: s3.IBucket): s3.BucketNotificationDestinationConfig {
-    const permissionId = `AllowBucketNotificationsFrom${bucket.node.uniqueId}`;
+    const permissionId = `AllowBucketNotificationsTo${this.fn.permissionsNode.uniqueId}`;
 
-    if (this.fn.permissionsNode.tryFindChild(permissionId) === undefined) {
+    if (!Construct.isConstruct(bucket)) {
+      throw new Error(`LambdaDestination for function ${this.fn.permissionsNode.uniqueId} can only be configured on a
+        bucket construct (Bucket ${bucket.bucketName})`);
+    }
+
+    if (bucket.node.tryFindChild(permissionId) === undefined) {
       this.fn.addPermission(permissionId, {
         sourceAccount: Stack.of(bucket).account,
         principal: new iam.ServicePrincipal('s3.amazonaws.com'),
         sourceArn: bucket.bucketArn,
+        // Placing the permissions node in the same scope as the s3 bucket.
+        // Otherwise, there is a circular dependency when the s3 bucket
+        // and lambda functions declared in different stacks.
+        scope: bucket,
       });
     }
 
     // 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.fn.permissionsNode.tryFindChild(permissionId) as CfnResource | undefined;
+    const permission = bucket.node.tryFindChild(permissionId) as CfnResource | undefined;
 
     return {
       type: s3.BucketNotificationDestinationType.LAMBDA,

diff --git a/packages/@aws-cdk/aws-s3-notifications/test/lambda/integ.bucket-notifications.expected.json b/packages/@aws-cdk/aws-s3-notifications/test/lambda/integ.bucket-notifications.expected.json
@@ -44,9 +44,31 @@
         }
       },
       "DependsOn": [
-        "MyFunctionAllowBucketNotificationsFromlambdabucketnotificationsMyBucket0F0FC402189522F6"
+        "MyBucketAllowBucketNotificationsTolambdabucketnotificationsMyFunction4086861C1BF13476"
       ]
     },
+    "MyBucketAllowBucketNotificationsTolambdabucketnotificationsMyFunction4086861C1BF13476": {
+      "Type": "AWS::Lambda::Permission",
+      "Properties": {
+        "Action": "lambda:InvokeFunction",
+        "FunctionName": {
+          "Fn::GetAtt": [
+            "MyFunction3BAA72D1",
+            "Arn"
+          ]
+        },
+        "Principal": "s3.amazonaws.com",
+        "SourceAccount": {
+          "Ref": "AWS::AccountId"
+        },
+        "SourceArn": {
+          "Fn::GetAtt": [
+            "MyBucketF68F3FF0",
+            "Arn"
+          ]
+        }
+      }
+    },
     "MyFunctionServiceRole3C357FF2": {
       "Type": "AWS::IAM::Role",
       "Properties": {
@@ -97,50 +119,6 @@
         "MyFunctionServiceRole3C357FF2"
       ]
     },
-    "MyFunctionAllowBucketNotificationsFromlambdabucketnotificationsMyBucket0F0FC402189522F6": {
-      "Type": "AWS::Lambda::Permission",
-      "Properties": {
-        "Action": "lambda:InvokeFunction",
-        "FunctionName": {
-          "Fn::GetAtt": [
-            "MyFunction3BAA72D1",
-            "Arn"
-          ]
-        },
-        "Principal": "s3.amazonaws.com",
-        "SourceAccount": {
-          "Ref": "AWS::AccountId"
-        },
-        "SourceArn": {
-          "Fn::GetAtt": [
-            "MyBucketF68F3FF0",
-            "Arn"
-          ]
-        }
-      }
-    },
-    "MyFunctionAllowBucketNotificationsFromlambdabucketnotificationsYourBucket307F72F245F2C5AE": {
-      "Type": "AWS::Lambda::Permission",
-      "Properties": {
-        "Action": "lambda:InvokeFunction",
-        "FunctionName": {
-          "Fn::GetAtt": [
-            "MyFunction3BAA72D1",
-            "Arn"
-          ]
-        },
-        "Principal": "s3.amazonaws.com",
-        "SourceAccount": {
-          "Ref": "AWS::AccountId"
-        },
-        "SourceArn": {
-          "Fn::GetAtt": [
-            "YourBucketC6A57364",
-            "Arn"
-          ]
-        }
-      }
-    },
     "YourBucketC6A57364": {
       "Type": "AWS::S3::Bucket",
       "UpdateReplacePolicy": "Delete",
@@ -175,9 +153,31 @@
         }
       },
       "DependsOn": [
-        "MyFunctionAllowBucketNotificationsFromlambdabucketnotificationsYourBucket307F72F245F2C5AE"
+        "YourBucketAllowBucketNotificationsTolambdabucketnotificationsMyFunction4086861C8FE2B89D"
       ]
     },
+    "YourBucketAllowBucketNotificationsTolambdabucketnotificationsMyFunction4086861C8FE2B89D": {
+      "Type": "AWS::Lambda::Permission",
+      "Properties": {
+        "Action": "lambda:InvokeFunction",
+        "FunctionName": {
+          "Fn::GetAtt": [
+            "MyFunction3BAA72D1",
+            "Arn"
+          ]
+        },
+        "Principal": "s3.amazonaws.com",
+        "SourceAccount": {
+          "Ref": "AWS::AccountId"
+        },
+        "SourceArn": {
+          "Fn::GetAtt": [
+            "YourBucketC6A57364",
+            "Arn"
+          ]
+        }
+      }
+    },
     "BucketNotificationsHandler050a0587b7544547bf325f094a3db834RoleB6FB88EC": {
       "Type": "AWS::IAM::Role",
       "Properties": {

diff --git a/packages/@aws-cdk/aws-s3-notifications/test/lambda/lambda.test.ts b/packages/@aws-cdk/aws-s3-notifications/test/lambda/lambda.test.ts
@@ -3,9 +3,105 @@ import { ResourcePart } from '@aws-cdk/assert';
 import '@aws-cdk/assert/jest';
 import * as lambda from '@aws-cdk/aws-lambda';
 import * as s3 from '@aws-cdk/aws-s3';
-import { Stack } from '@aws-cdk/core';
+import { Stack, App } from '@aws-cdk/core';
 import * as s3n from '../../lib';
 
+test('add notifications to multiple functions', () => {
+
+  const stack = new Stack();
+  const bucket = new s3.Bucket(stack, 'MyBucket');
+  const fn1 = new lambda.Function(stack, 'MyFunction1', {
+    runtime: lambda.Runtime.NODEJS_10_X,
+    handler: 'index.handler',
+    code: lambda.Code.fromInline('foo'),
+  });
+
+  const fn2 = new lambda.Function(stack, 'MyFunction2', {
+    runtime: lambda.Runtime.NODEJS_10_X,
+    handler: 'index.handler',
+    code: lambda.Code.fromInline('foo'),
+  });
+
+  const lambdaDestination1 = new s3n.LambdaDestination(fn1);
+  const lambdaDestination2 = new s3n.LambdaDestination(fn2);
+
+  bucket.addEventNotification(s3.EventType.OBJECT_CREATED, lambdaDestination1, { prefix: 'v1/' });
+  bucket.addEventNotification(s3.EventType.OBJECT_CREATED, lambdaDestination2, { prefix: 'v2/' });
+
+  // expecting notification configuration to have both events
+  expect(stack).toHaveResourceLike('Custom::S3BucketNotifications', {
+    NotificationConfiguration: {
+      LambdaFunctionConfigurations: [
+        { Filter: { Key: { FilterRules: [{ Name: 'prefix', Value: 'v1/' }] } } },
+        { Filter: { Key: { FilterRules: [{ Name: 'prefix', Value: 'v2/' }] } } },
+      ],
+    },
+  });
+
+  // expecting one permission for each function
+  expect(stack).toCountResources('AWS::Lambda::Permission', 2);
+
+  // make sure each permission points to the correct function
+  expect(stack).toHaveResourceLike('AWS::Lambda::Permission', {
+    FunctionName: {
+      'Fn::GetAtt': [
+        'MyFunction12A744C2E',
+        'Arn',
+      ],
+    },
+    SourceArn: {
+      'Fn::GetAtt': [
+        'MyBucketF68F3FF0',
+        'Arn',
+      ],
+    },
+  });
+  expect(stack).toHaveResourceLike('AWS::Lambda::Permission', {
+    FunctionName: {
+      'Fn::GetAtt': [
+        'MyFunction2F2A964CA',
+        'Arn',
+      ],
+    },
+    SourceArn: {
+      'Fn::GetAtt': [
+        'MyBucketF68F3FF0',
+        'Arn',
+      ],
+    },
+  });
+
+});
+
+test('lambda in a different stack as notification target', () => {
+
+  const app = new App();
+  const lambdaStack = new Stack(app, 'stack1');
+  const bucketStack = new Stack(app, 'stack2');
+
+  const lambdaFunction = new lambda.Function(lambdaStack, 'lambdaFunction', {
+    code: lambda.Code.fromInline('whatever'),
+    handler: 'index.handler',
+    runtime: lambda.Runtime.NODEJS_10_X,
+  });
+
+  const bucket = new s3.Bucket(bucketStack, 'bucket');
+  bucket.addObjectCreatedNotification(new s3n.LambdaDestination(lambdaFunction));
+
+  // permission should be in the bucket stack
+  expect(bucketStack).toHaveResourceLike('AWS::Lambda::Permission', {
+    FunctionName: {
+      'Fn::ImportValue': 'stack1:ExportsOutputFnGetAttlambdaFunction940E68ADArn6B2878AF',
+    },
+    SourceArn: {
+      'Fn::GetAtt': [
+        'bucket43879C71',
+        'Arn',
+      ],
+    },
+  });
+});
+
 test('lambda as notification target', () => {
   // GIVEN
   const stack = new Stack();
@@ -88,7 +184,7 @@ test('permissions are added as a dependency to the notifications resource when u
   bucket.addEventNotification(s3.EventType.OBJECT_CREATED, lambdaDestination, { prefix: 'v1/' });
 
   expect(stack).toHaveResource('Custom::S3BucketNotifications', {
-    DependsOn: ['SingletonLambdauuidAllowBucketNotificationsFromMyBucket1464DCBA'],
+    DependsOn: ['MyBucketAllowBucketNotificationsToSingletonLambdauuid28C96883'],
   }, ResourcePart.CompleteDefinition);
 });