fix(logs): cannot use same Lambda for multiple SubscriptionFilters (#…

…4975)

We used to scope the permission for calling it from CloudWatch Logs
under the Lambda. However, when it was used multiple times there
would be a name conflict.

Instead, scope the Permission under the SubscriptionFilter.

Fixes #4951.

packages/@aws-cdk/aws-logs-destinations/lib/kinesis.ts

@@ -15,8 +15,8 @@ export class KinesisDestination implements logs.ILogSubscriptionDestination {
     // Create a role to be assumed by CWL that can write to this stream and pass itself.
     const id = 'CloudWatchLogsCanPutRecords';
     const role = scope.node.tryFindChild(id) as iam.IRole || new iam.Role(scope, id, {
-      assumedBy: new iam.ServicePrincipal(`logs.amazonaws.com`)
-    });
+        assumedBy: new iam.ServicePrincipal(`logs.amazonaws.com`)
+      });
     this.stream.grantWrite(role);
     role.grantPassRole(role);
     return { arn: this.stream.streamArn, role };

packages/@aws-cdk/aws-logs-destinations/lib/lambda.ts

@@ -10,18 +10,16 @@ export class LambdaDestination implements logs.ILogSubscriptionDestination {
   constructor(private readonly fn: lambda.IFunction) {
   }
 
-  public bind(_scope: Construct, logGroup: logs.ILogGroup): logs.LogSubscriptionDestinationConfig {
+  public bind(scope: Construct, logGroup: logs.ILogGroup): logs.LogSubscriptionDestinationConfig {
     const arn = logGroup.logGroupArn;
 
-    const logSubscriptionDestinationPolicyAddedFor: string[] = [];
-
-    if (logSubscriptionDestinationPolicyAddedFor.indexOf(arn) === -1) {
-      this.fn.addPermission('InvokedByCloudWatchLogs', {
-        principal: new iam.ServicePrincipal(`logs.amazonaws.com`),
-        sourceArn: arn
-      });
-      logSubscriptionDestinationPolicyAddedFor.push(arn);
-    }
+    this.fn.addPermission('CanInvokeLambda', {
+      principal: new iam.ServicePrincipal(`logs.amazonaws.com`),
+      sourceArn: arn,
+      // Using SubScription Filter as scope is okay, since every Subscription Filter has only
+      // one destination.
+      scope
+    });
     return { arn: this.fn.functionArn };
   }
-}
+}

packages/@aws-cdk/aws-logs-destinations/test/kinesis.test.ts

@@ -23,7 +23,78 @@ test('stream can be subscription destination', () => {
     RoleArn: { "Fn::GetAtt": [ "SubscriptionCloudWatchLogsCanPutRecords9C1223EC", "Arn" ] },
   });
 
-  // THEN: we have a role to write to the Lambda
+  // THEN: we have a role to write to the Stream
+  expect(stack).toHaveResource('AWS::IAM::Role', {
+    AssumeRolePolicyDocument: {
+      Version: '2012-10-17',
+      Statement: [{
+        Action: "sts:AssumeRole",
+        Effect: 'Allow',
+        Principal: {
+          Service: {
+            "Fn::Join": [ "", [
+                "logs.",
+                { Ref: "AWS::Region" },
+                ".",
+                { Ref: "AWS::URLSuffix" }
+              ]
+            ]
+          }
+        }
+      }],
+    }
+  });
+
+  expect(stack).toHaveResource('AWS::IAM::Policy', {
+    PolicyDocument: {
+      Version: '2012-10-17',
+      Statement: [
+        {
+          Action: [
+            "kinesis:DescribeStream",
+            "kinesis:PutRecord",
+            "kinesis:PutRecords",
+          ],
+          Effect: "Allow",
+          Resource: { "Fn::GetAtt": [ "MyStream5C050E93", "Arn" ] }
+        },
+        {
+          Action: "iam:PassRole",
+          Effect: "Allow",
+          Resource: { "Fn::GetAtt": [ "SubscriptionCloudWatchLogsCanPutRecords9C1223EC", "Arn" ] }
+        }
+      ],
+    }
+  });
+});
+
+test('stream can be subscription destination twice, without duplicating permissions', () => {
+  // GIVEN
+  const stack = new cdk.Stack();
+  const stream = new kinesis.Stream(stack, 'MyStream');
+  const logGroup1 = new logs.LogGroup(stack, 'LogGroup');
+  const logGroup2 = new logs.LogGroup(stack, 'LogGroup2');
+
+  // WHEN
+  new logs.SubscriptionFilter(stack, 'Subscription', {
+    logGroup: logGroup1,
+    destination: new dests.KinesisDestination(stream),
+    filterPattern: logs.FilterPattern.allEvents()
+  });
+
+  new logs.SubscriptionFilter(stack, 'Subscription2', {
+    logGroup: logGroup2,
+    destination: new dests.KinesisDestination(stream),
+    filterPattern: logs.FilterPattern.allEvents()
+  });
+
+  // THEN: subscription target is Stream
+  expect(stack).toHaveResource('AWS::Logs::SubscriptionFilter', {
+    DestinationArn: { "Fn::GetAtt": [ "MyStream5C050E93", "Arn" ] },
+    RoleArn: { "Fn::GetAtt": [ "SubscriptionCloudWatchLogsCanPutRecords9C1223EC", "Arn" ] },
+  });
+
+  // THEN: we have a role to write to the Stream
   expect(stack).toHaveResource('AWS::IAM::Role', {
     AssumeRolePolicyDocument: {
       Version: '2012-10-17',

packages/@aws-cdk/aws-logs-destinations/test/lambda.test.ts

@@ -4,16 +4,22 @@ import logs = require('@aws-cdk/aws-logs');
 import cdk = require('@aws-cdk/core');
 import dests = require('../lib');
 
-test('lambda can be used as metric subscription destination', () => {
+let stack: cdk.Stack;
+let fn: lambda.Function;
+let logGroup: logs.LogGroup;
+
+beforeEach(() => {
   // GIVEN
-  const stack = new cdk.Stack();
-  const fn = new lambda.Function(stack, 'MyLambda', {
+  stack = new cdk.Stack();
+  fn = new lambda.Function(stack, 'MyLambda', {
     code: new lambda.InlineCode('foo'),
     handler: 'index.handler',
     runtime: lambda.Runtime.NODEJS_8_10,
   });
-  const logGroup = new logs.LogGroup(stack, 'LogGroup');
+  logGroup = new logs.LogGroup(stack, 'LogGroup');
+});
 
+test('lambda can be used as metric subscription destination', () => {
   // WHEN
   new logs.SubscriptionFilter(stack, 'Subscription', {
     logGroup,
@@ -29,12 +35,37 @@ test('lambda can be used as metric subscription destination', () => {
   // THEN: Lambda has permissions to be invoked by CWL
   expect(stack).toHaveResource('AWS::Lambda::Permission', {
     Action: "lambda:InvokeFunction",
-    FunctionName: {
-      "Fn::GetAtt": [
-        "MyLambdaCCE802FB",
-        "Arn"
-      ]
-    },
+    FunctionName: { "Fn::GetAtt": [ "MyLambdaCCE802FB", "Arn" ] },
+    Principal: "logs.amazonaws.com",
+  });
+});
+
+test('can have multiple subscriptions use the same Lambda', () => {
+  // WHEN
+  new logs.SubscriptionFilter(stack, 'Subscription', {
+    logGroup,
+    destination: new dests.LambdaDestination(fn),
+    filterPattern: logs.FilterPattern.allEvents()
+  });
+
+  new logs.SubscriptionFilter(stack, 'Subscription2', {
+    logGroup: new logs.LogGroup(stack, 'LG2'),
+    destination: new dests.LambdaDestination(fn),
+    filterPattern: logs.FilterPattern.allEvents()
+  });
+
+  // THEN: Lambda has permissions to be invoked by CWL from both Source Arns
+  expect(stack).toHaveResource('AWS::Lambda::Permission', {
+    Action: "lambda:InvokeFunction",
+    FunctionName: { "Fn::GetAtt": [ "MyLambdaCCE802FB", "Arn" ] },
+    SourceArn: { "Fn::GetAtt": [ "LogGroupF5B46931", "Arn" ] },
+    Principal: "logs.amazonaws.com",
+  });
+
+  expect(stack).toHaveResource('AWS::Lambda::Permission', {
+    Action: "lambda:InvokeFunction",
+    FunctionName: { "Fn::GetAtt": [ "MyLambdaCCE802FB", "Arn" ] },
+    SourceArn: { "Fn::GetAtt": [ "LG224A94C8F", "Arn" ] },
     Principal: "logs.amazonaws.com",
   });
 });