Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lambda props does not allow you to bring your own Role #205

Closed
rclark opened this issue Jun 30, 2018 · 11 comments · Fixed by #263
Closed

Lambda props does not allow you to bring your own Role #205

rclark opened this issue Jun 30, 2018 · 11 comments · Fixed by #263
Labels
package/awscl Cross-cutting issues related to the AWS Construct Library

Comments

@rclark
Copy link

rclark commented Jun 30, 2018

I don't know how to attach my own role to a Lambda function -- the props won't accept one, and the .role attribute is read-only. I usually write a dedicated role to associate with any Lambda function, or reference one that has been created independently. '@aws-cdk/lambda' Lambda seems to be hard-wired to provide its own service role?

@eladb
Copy link
Contributor

eladb commented Jun 30, 2018

Definitely a missing feature. I’ve also added this to our design guidelines so we will ensure that any construct that needs a role can accept one via props.

Out of curiosity, could you share a few more details about your use case for defining the role separately instead of just adding policy statements to the auto-created role?

@eladb eladb added the package/awscl Cross-cutting issues related to the AWS Construct Library label Jun 30, 2018
@rclark
Copy link
Author

rclark commented Jun 30, 2018

Honestly I hadn't recognized that you could add policies to the role. That'd work as follows?

const lambda = new Lambda(stack, 'MyFunction', { ... });
lambda.role.addPolicy()

That could work, although I don't like the fact that this would bring a mandatory attachment to a Managed Policy. I think it'd be better to allow the user to completely define their role if they so desire.

@eladb
Copy link
Contributor

eladb commented Jun 30, 2018

Yes, either lambda.role.addToPolicy(policyStatement) or `lambda.addToRolePolicy(policyStatement>‘ will do.

And I agree - it should be possible to specify a role that you define elsewhere.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jun 30, 2018

Allow me to disagree.

The reason you want to pass in your own role is because CloudFormation required you to do that, and so you're used to it. Semantically, idiomatically, every Lambda should have their own Role. This way this is even enforced.

In fact, people who don't read docs (all of them) will be tempted to write CloudFormation-style CDK apps and write worse code.

There's nothing you can't do now that you could do before, maybe with the exception of having Lambda's share a role, but if they need to share permissions, some shared code can take care of that.

@rclark
Copy link
Author

rclark commented Jun 30, 2018

Semantically, idiomatically, every Lambda should have their own Role

Much of the time yes, but in other cases no. Perhaps an organization does not delegate the ability to design roles to a subset of its employees, but it does let them generate lambda functions and reuse a controlled set of roles. Maybe there's tight control over who can and who can't attach managed policies. The fact that the CDK builds parts of a CloudFormation template in a "black box" fashion will inevitably run up against scenarios where those parts need to be removed.

@rix0rrr I appreciate your position. Just keep in mind a few things about the opinionated decisions that the CDK design implies:

  • There are "more right" and "more wrong" ways to write CDK applications. Is it clear and simple for users to learn what does and does not follow CDK conventions / opinions? And why those opinionated decisions have been made on their behalf?
  • The more opinionated a spin you put on top of CloudFormation, the more resistance to adoption you'll see from users who are already accustomed to using CloudFormation directly. New users most of the time will not care, but organizations with history and process around CloudFormation won't have a lot of tolerance for opinionated decisions that they disagree with.
  • The number of people who will read all of the docs is very very close to zero. If that's what it takes to develop a firm understanding of the subtleties of CDK application design, then most people will develop appreciation for those subtleties very, very slowly.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jul 1, 2018

The preauthored role is a good argument.

We could solve that concern a bunch of different ways, but not so clearly as with a trusted team writing the roles.

@rix0rrr
Copy link
Contributor

rix0rrr commented Jul 1, 2018

Although thinking about that a bit more just gave me another idea.

Precreating roles would still be a hassle, because if you'd want to write a new Lambda, or make a change to your existing Lambda that would require additional permissions, you'd have to go to the security-vetted team and ask them to update your IAM role, only after which you could start testing.

Presumably, a process rule like this is a mechanism to enforce the actual requirement: all IAM changes go through a central, specialized team.

Something that might also enforce that requirement, but in a way that's probably more comfortable for everyone involved, is if we would generate all IAM-related resources to a different stack. That stack would then be deployed by the security team after manual review, while the other resources are deployed as usual.

This would require us to be able to decouple application definition from stack definition (something which is strongly coupled today). But a different feature that would also require us to do that is bidirectional references between elements that come from different stacks. I think it makes sense in the grand scheme of things that you define your application in CDK without regard to what, or how many, CloudFormation stacks would be necessary to finally deploy all those resources. It's in line with the abstraction that we're trying to provide.

On the other hand, doing this implies moving farther away from CloudFormation proper which might hurt adoption with teams who already know CloudFormation as you mentioned Ryan.

Tricky to know what the right balance is...

@rix0rrr
Copy link
Contributor

rix0rrr commented Jul 1, 2018

All this is not to say we shouldn't allow people to specify their own Role, it's a low effort change that will address a definite need people have.

Just wondering out loud how we can nudge or help them into even more productive patterns...

rix0rrr pushed a commit that referenced this issue Jul 9, 2018
This is as opposed to having the Function generate a role automatically.

We still recommend you do not specify a role manually.

This fixes #205.
rix0rrr pushed a commit that referenced this issue Jul 9, 2018
Add a `role` parameter so a role can be specified externally.  This is as
opposed to having the Function generate a role automatically.

This fixes #205.
rix0rrr pushed a commit that referenced this issue Jul 9, 2018
Add a `role` parameter so a role can be specified externally.  This is as
opposed to having the Function generate a role automatically.

This fixes #205.
rix0rrr added a commit that referenced this issue Jul 10, 2018
Add a `role` parameter so a role can be specified externally. Specifying a role will prevent the Lambda from generating a role automatically; otherwise it works the same.

This fixes #205.
@bgdnlp
Copy link

bgdnlp commented Jun 21, 2019

For others that arrive here looking for how to create the IRole object needed, use the from_role_arn method of the Role class in aws_cdk.aws_iam.

Example Python code:

from aws_cdk import aws_iam

role = aws_iam.Role.from_role_arn(self, 'role_id', 'arn:aws:iam::123456789012:role/myrole')

will create an IRole object that can be passed as a parameter to Lambda Function.

@vertex451
Copy link

vertex451 commented Mar 21, 2021

I didn't managed to add role during lambda creation, but next code gives to lambda access to external role, created in another CDK:

lambdaFn.addToRolePolicy(new iam.PolicyStatement({
      effect: iam.Effect.ALLOW,
      actions: [ "sts:AssumeRole" ],
      resources: [externalRoleArn]
    }))

@fatihaslantas
Copy link

whoever come to solve circular reference issue, Role.FromRoleArn function will solve your issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package/awscl Cross-cutting issues related to the AWS Construct Library
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants