feat(lambda): Add LambdaRole class with helper props for common Lambda integrations #8281

flemjame-at-amazon · 2020-05-30T00:28:34Z

feat(lambda): Add LambdaRole class with helper props for common Lambda integrations

If I am providing a Role for a Lambda function, it currently isn't given the basic execution permissions, so the function cannot log anything or, in the case of a VPC Lambda, it cannot create the network interfaces. The user has to add those permissions themselves, but it isn't clear from the documentation that that needs to happen.

This commit adds a LambdaRole class, which extends the IAM Role class and which automatically handles Lambda execution permissions. It also exposes additional props which automatically set up Lambda integrations with services like Kinesis, DynamoDB, SQS, and XRay.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

aws-cdk-automation · 2020-05-30T00:50:31Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: ee5ec34
Result: FAILED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

aws-cdk-automation · 2020-05-30T01:21:58Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: 39ad590
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

nija-at · 2020-06-02T10:26:02Z

packages/@aws-cdk/aws-lambda/lib/lambda-role.ts

+/**
+ * Properties for defining a Lambda Role
+ */
+export interface LambdaRoleProps extends Omit<RoleProps, 'assumedBy'> {


Unfortunately, jsii doesn't support the Omit type.

The feature set supported in the CDK on public-facing interfaces is restricted by the feature set supported by JSII.
My guess is the Java equivalent interfaces have simply ignored the Omit and in fact, do require the 'assumedBy' property.

I've opened a bug report for this - aws/jsii#1707

If you want to achieve this, you will need to refactor RoleProps to create a new interface that holds everything in RoleProps except assumedBy and RoleProps would simply extend this.

export interface RoleProps extends RoleOptions { readonly assumedBy: IPrincipal; } export interface RoleOptions { // the rest of the properties here }

I've used RoleOptions but we might have to use a better name.

@nija-at will Omit ever be supported by jsii?

I'm fine with doing the refactor, it's just tedious.

There's no reasonable way to model this in languages outside of typescript (think: Java, C#, Python, etc.), so we don't have any plans to support this.

Fixed in recent commit

nija-at · 2020-06-02T10:29:56Z

packages/@aws-cdk/aws-lambda/lib/lambda-role.ts

+  // We omit the assumedBy property because we know what it is for a Lambda Role
+
+  /**
+   * Grants permission to manage elastic network interfaces to


Perhaps be transparent and say

Suggested change

* Grants permission to manage elastic network interfaces to

* Adds the AWS managed policy "AWSLambdaBasicExecutionRole" to this role

nija-at · 2020-06-02T10:31:40Z

packages/@aws-cdk/aws-lambda/lib/lambda-role.ts

+   *
+   * @default false
+   */
+  readonly dynamoDBExecutionAccess?: boolean;


These may be better names here - allowVpcExecution, allowDynamoExecution, allowSqsExecution

…-cdk into lambda-role

Pull request has been modified.

aws-cdk-automation · 2020-06-03T02:35:42Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
Commit ID: e7ddf13
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

nija-at

Hey @flemjame-at-amazon -

On discussion with team members, it seems like this is not the correct approach with the CDK.

I apologize for giving you the wrong idea in the first place on which this PR was based. This is my mistake. I am sorry that this has taken up your time.

The correct way to use the CDK is a resource centric approach. This would be to use the various grant APIs to associate the correct permissions with the role.
If the lambda function should have access to a Kinesis stream, then users need to call kinesisStream.grantWrite(lambdaFunction) or with DynamoDB, it will be dynamoTable.grantRead(lambdaFunction).

Let me know if there's a use case that doesn't fit with this model. It's possible that some resources don't have grant APIs but in those cases, they need to be added.

flemjame-at-amazon · 2020-06-05T02:53:18Z

Hey @flemjame-at-amazon -

On discussion with team members, it seems like this is not the correct approach with the CDK.

I apologize for giving you the wrong idea in the first place on which this PR was based. This is my mistake. I am sorry that this has taken up your time.

The correct way to use the CDK is a resource centric approach. This would be to use the various grant APIs to associate the correct permissions with the role.
If the lambda function should have access to a Kinesis stream, then users need to call kinesisStream.grantWrite(lambdaFunction) or with DynamoDB, it will be dynamoTable.grantRead(lambdaFunction).

Let me know if there's a use case that doesn't fit with this model. It's possible that some resources don't have grant APIs but in those cases, they need to be added.

It looks like the way these permissions are handled is with IEventSource:

Kinesis:

aws-cdk/packages/@aws-cdk/aws-lambda-event-sources/lib/kinesis.ts

Lines 28 to 35 in 1cd832b

    
           this.stream.grantRead(target); 
        
           // The `grantRead` API provides all the permissions recommended by the Kinesis team for reading a stream. 
        
           // `DescribeStream` permissions are not required to read a stream as it's covered by the `DescribeStreamSummary` 
        
           // and `SubscribeToShard` APIs. 
        
           // The Lambda::EventSourceMapping resource validates against the `DescribeStream` permission. So we add it explicitly. 
        
           // FIXME This permission can be removed when the event source mapping resource drops it from validation. 
        
           this.stream.grant(target, 'kinesis:DescribeStream');

DynamoDB:

aws-cdk/packages/@aws-cdk/aws-lambda-event-sources/lib/dynamodb.ts

Lines 22 to 33 in 1cd832b

    
           public bind(target: lambda.IFunction) { 
        
             if (!this.table.tableStreamArn) { 
        
               throw new Error(`DynamoDB Streams must be enabled on the table ${this.table.node.path}`); 
        
             } 
        
             const eventSourceMapping = target.addEventSourceMapping(`DynamoDBEventSource:${this.table.node.uniqueId}`, 
        
               this.enrichMappingOptions({eventSourceArn: this.table.tableStreamArn}), 
        
             ); 
        
             this._eventSourceMappingId = eventSourceMapping.eventSourceMappingId; 
        
             this.table.grantStreamRead(target); 
        
           }

SQS:

aws-cdk/packages/@aws-cdk/aws-lambda-event-sources/lib/sqs.ts

Line 36 in 1cd832b

this.queue.grantConsumeMessages(target);

(Nothing for XRay yet)

I don't see a similar bit of code for granting Vpc execution permissions.

flemjame-at-amazon · 2020-06-05T16:15:56Z

@nija-at do you have a recommendation for how to implement granting VPC execution permissions? I was thinking a static method grantExcecution(target: IFunction) in the VPC class.

nija-at · 2020-06-08T09:44:09Z

It looks like the way these permissions are handled is with IEventSource:

That is correct. The aws-lambda-event-sources package is automatically configuring the role with the permissions when an event source is configured. You can also use these grant* methods in your code directly as well.

do you have a recommendation for how to implement granting VPC execution permissions? I was thinking a static method grantExcecution(target: IFunction) in the VPC class.

Looking at the AWSLambdaVPCAccessExecutionRole managed policy, the actions being granted are ec2:CreateNetworkInterface, ec2:DescribeNetworkInterfaces, ec2:DeleteNetworkInterface.

For this, you will have to add an instance method grantNetworkInterfaceWrite(grantee: iam.Grantable): iam.Grant

flemjame-at-amazon · 2020-06-09T01:11:15Z

It looks like the way these permissions are handled is with IEventSource:

That is correct. The aws-lambda-event-sources package is automatically configuring the role with the permissions when an event source is configured. You can also use these grant* methods in your code directly as well.

do you have a recommendation for how to implement granting VPC execution permissions? I was thinking a static method grantExcecution(target: IFunction) in the VPC class.

Looking at the AWSLambdaVPCAccessExecutionRole managed policy, the actions being granted are ec2:CreateNetworkInterface, ec2:DescribeNetworkInterfaces, ec2:DeleteNetworkInterface.

For this, you will have to add an instance method grantNetworkInterfaceWrite(grantee: iam.Grantable): iam.Grant

This implies it is to be used for more than just AWS Lambda -- anyone wanting said permissions would be able to call the function, right?

A cursory inspection of the EC2 permissions list shows about 10 permissions that contain "NetworkInterface" -- should this function include them all? If so, that provides Lambda with a much broader array of permission than it needs. If it shouldn't include them all, then it's a function that provides a very specific subset of permissions, and I'm not sure what other use case it would have.

rix0rrr · 2020-06-09T08:51:45Z

I think we need a little more context on what your actual problem is before you propose the solution.

@flemjame-at-amazon, is your issue that when you do the following:

new Function(this, 'Function', {
   vpc: vpc,
   // .. stuff
});

That this fails because you also have to grant ec2:CreateNetworkInterface?

If that is the case, then can you explain to me why this:

const function = new Function(this, 'Function', {
   vpc: vpc,
   role: role,
   // ... stuff ...
});

Vpc.grantCreateDeleteEni(function);

Is more desirable than just automatically granting the VPC-bind-permissions on a Lambda when you pass a vpc argument? Doing it for you automatically seems to be more in line with the "it just works" philosophy of CDK.

nija-at · 2020-06-09T08:57:00Z

@rix0rrr - ignore the code in this PR. We are discussing now a better approach to this, and are in fact, going down the grant API route.

rix0rrr · 2020-06-09T08:59:34Z

I updated my comment. But I still feel that implicit permissions would be the more correct route to go.

nija-at · 2020-06-09T09:01:03Z

A cursory inspection of the EC2 permissions list shows about 10 permissions that contain "NetworkInterface" -- should this function include them all? If so, that provides Lambda with a much broader array of permission than it needs. If it shouldn't include them all, then it's a function that provides a very specific subset of permissions, and I'm not sure what other use case it would have.

@flemjame-at-amazon - Take a look at stream.ts in the aws-kinesis library as an example on how to do this.

You would create a method as grantCreateDeleteEni() API and a separate grant() API, the former holding the permissions you're looking for and the latter providing the user with the option to choose the list of actions instead.

Does that make sense?

Further, should we automatically call the grantCreateDeleteEni() API while creating a new Function with a VPC configured?

flemjame-at-amazon · 2020-06-10T01:42:03Z

Further, should we automatically call the grantCreateDeleteEni() API while creating a new Function with a VPC configured?

Lambda already takes care of that for you, just with managed policies. Would this replace what Lambda currently does?

aws-cdk/packages/@aws-cdk/aws-lambda/lib/function.ts

Lines 479 to 482 in 1cd832b

    
           if (props.vpc) { 
        
             // Policy that will have ENI creation permissions 
        
             managedPolicies.push(iam.ManagedPolicy.fromAwsManagedPolicyName('service-role/AWSLambdaVPCAccessExecutionRole')); 
        
           }

For a provided Role, we should do nothing, correct? That was the conclusion in another ticket:
#8041 (review)

Is more desirable than just automatically granting the VPC-bind-permissions on a Lambda when you pass a vpc argument? Doing it for you automatically seems to be more in line with the "it just works" philosophy of CDK.

@rix0rrr Lambda does this for the Role that is created with the function, but not for a Role that is provided to the function. I am trying to build a solution for the case where the user wants to provide a Role.

I initially went down the route you proposed (automatically add permissions), but the conclusion was to not automatically add permissions in the Function class. See #8041 (review)

nija-at · 2020-06-10T08:20:44Z

Further, should we automatically call the grantCreateDeleteEni() API while creating a new Function with a VPC configured?

Lambda already takes care of that for you, just with managed policies. Would this replace what Lambda currently does?

Adding the grant methods to the VPC construct is all that's needed now to achieve what you need.

nija-at · 2020-06-16T10:22:20Z

Hey @flemjame-at-amazon -

I'm closing this PR given our discussion. Open a separate PR for the grant methods whenever you're ready 😊

flemjame-at-amazon and others added 6 commits May 29, 2020 20:03

Add lambda role

5771bcb

Import ordering

6e844c8

Add tests, and use in the Function class

24e4877

Add readme and additional tests

8fdee61

Clarify readme

ee5ec34

Merge branch 'master' into lambda-role

39ad590

SomayaB assigned nija-at Jun 1, 2020

nija-at previously requested changes Jun 2, 2020

View reviewed changes

flemjame-at-amazon added 2 commits June 3, 2020 01:26

Review feedback

b66eb7d

Merge branch 'lambda-role' of ssh://github.com/flemjame-at-amazon/aws…

e7ddf13

…-cdk into lambda-role

nija-at reviewed Jun 3, 2020

View reviewed changes

nija-at closed this Jun 16, 2020

flemjame-at-amazon deleted the lambda-role branch October 27, 2020 14:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(lambda): Add LambdaRole class with helper props for common Lambda integrations #8281

feat(lambda): Add LambdaRole class with helper props for common Lambda integrations #8281

flemjame-at-amazon commented May 30, 2020 •

edited

Loading

aws-cdk-automation commented May 30, 2020

aws-cdk-automation commented May 30, 2020

nija-at Jun 2, 2020

nija-at Jun 2, 2020

flemjame-at-amazon Jun 2, 2020

nija-at Jun 2, 2020

flemjame-at-amazon Jun 3, 2020

nija-at Jun 2, 2020

flemjame-at-amazon Jun 3, 2020

nija-at Jun 2, 2020

flemjame-at-amazon Jun 3, 2020

aws-cdk-automation commented Jun 3, 2020

nija-at left a comment •

edited

Loading

flemjame-at-amazon commented Jun 5, 2020 •

edited

Loading

flemjame-at-amazon commented Jun 5, 2020

nija-at commented Jun 8, 2020

flemjame-at-amazon commented Jun 9, 2020

rix0rrr commented Jun 9, 2020 •

edited

Loading

nija-at commented Jun 9, 2020

rix0rrr commented Jun 9, 2020

nija-at commented Jun 9, 2020

flemjame-at-amazon commented Jun 10, 2020 •

edited

Loading

nija-at commented Jun 10, 2020

nija-at commented Jun 16, 2020

	* Grants permission to manage elastic network interfaces to
	* Adds the AWS managed policy "AWSLambdaBasicExecutionRole" to this role

feat(lambda): Add LambdaRole class with helper props for common Lambda integrations #8281

feat(lambda): Add LambdaRole class with helper props for common Lambda integrations #8281

Conversation

flemjame-at-amazon commented May 30, 2020 • edited Loading

aws-cdk-automation commented May 30, 2020

AWS CodeBuild CI Report

aws-cdk-automation commented May 30, 2020

AWS CodeBuild CI Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aws-cdk-automation commented Jun 3, 2020

AWS CodeBuild CI Report

nija-at left a comment • edited Loading

Choose a reason for hiding this comment

flemjame-at-amazon commented Jun 5, 2020 • edited Loading

flemjame-at-amazon commented Jun 5, 2020

nija-at commented Jun 8, 2020

flemjame-at-amazon commented Jun 9, 2020

rix0rrr commented Jun 9, 2020 • edited Loading

nija-at commented Jun 9, 2020

rix0rrr commented Jun 9, 2020

nija-at commented Jun 9, 2020

flemjame-at-amazon commented Jun 10, 2020 • edited Loading

nija-at commented Jun 10, 2020

nija-at commented Jun 16, 2020

flemjame-at-amazon commented May 30, 2020 •

edited

Loading

nija-at left a comment •

edited

Loading

flemjame-at-amazon commented Jun 5, 2020 •

edited

Loading

rix0rrr commented Jun 9, 2020 •

edited

Loading

flemjame-at-amazon commented Jun 10, 2020 •

edited

Loading