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

Allow Custom Per-Function Roles #2073

Closed

Conversation

@erikerikson
Copy link
Member

erikerikson commented Sep 8, 2016

What did you implement:

Closes Issue[s]: #1807 and #1895 by allowing per-function role declarations with fallback to provider or default policy/role

How did you implement it:

Allow each function to declare the ARN of the role that it is to execute under.
If any function has no role ARN but provider role ARN is defined then use that role for such functions.
If any function does not have a specified role (even by falling back to a provider-wide role) then add the default policy and role so that it can be used for such functions.
Break out the portions of the function assignment to CFT logic into their discrete units so that the registered plugin code is a readable summary. Add comments to the discrete units.

How can we verify it:

Added tests that check that the default role and policy are not added if every function has a role.
Added tests that check that every function that has a declared role gets it assigned.
Added tests that check that every function that has no declared role but where a provider role is declared gets the provider role assigned.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config/commands/resources
  • Leave a comment that this is ready for review once you've finished the implementation
erikerikson added 2 commits Sep 8, 2016
Resolves matter 1 of #1895

Allow each function to declare the ARN of the role that it is to execute within.  If any function has no role ARN but provider role ARN is defined then use that role for such functions.  If any function does not have a specified role (even by falling back to a provider-wide role) then add the default policy and role so that it can be used for such functions.
Break out the portions of the logic into their discrete units so that the plugin code is a readable summary.  Add comments to the discrete units.

Add tests that check that the default role and policy are not added if every function has a role.
Add tests that check that every function that has a declared role gets it assigned.
Add tests that check that every function that has no declared role but where a provider role is declared gets the provider role assigned.
Adds documentation about how the feature works and examples of how to use it.
@erikerikson

This comment has been minimized.

Copy link
Member Author

erikerikson commented Sep 8, 2016

This should be ready for review.

@andymac4182

This comment has been minimized.

Copy link
Contributor

andymac4182 commented Sep 9, 2016

Does this work ok when using the VPC config? If there is a custom role declared and assigned as part of the Cloudformation template it would be good if it verified that you have the correct ec2 permissions assigned so it doesn't fail.

@erikerikson

This comment has been minimized.

Copy link
Member Author

erikerikson commented Sep 9, 2016

Hi @andymac4182, this change does not impact the current behavior of SLS. That said, the code in this change is not doing any checking for or verification of any rights on the role; it only cares to allow a role to be specified at an individual function level (with fall back to any provider declared role). That, of course, means it is not checking for VPC rights assignments on any of the assigned roles.

I can see how verifying VPC rights would be useful if VPC configuration exists on any of the functions but that seems beyond the scope of this change. I've seen PR #1934 but that regards the default role that SLS creates (of course, I'll happily merge that code into this PR if it gets accepted prior to this one). The code related to this PR is about not using the default role/policy (and not having them created at all) for cases where you want to do something more fine-grained, nuanced, or where your account configuration will not accept the default approach (because of assigned developer rights). More correctly, this PR allows you to do this on an individual function level (contrary to the default and current bypass rights assignment approach).

Given that this change increases available flexibility in order to support a bit more of a "do-the-heavy-lifting-yourself" and "advanced/responsibility and knowledge needed" use case, I think we can depend on users to properly configure their roles; at least to start out. If a "verify VPC rights" capability were to be implemented in the future, I would hope that it would produce a warning by default that could potentially be escalated to an error if a command line flag (or some other configuration) was used. For use cases like these, it seems important to not make any assumptions about how users would like to implement their custom rights management: are they using an inline policy, a custom policy, or a managed policy? Seems to me that there's a bit of complexity in how to implement such a sufficient rights verification feature properly.

@erikerikson

This comment has been minimized.

Copy link
Member Author

erikerikson commented Sep 9, 2016

Thanks for asking the question though @andymac4182, I think I noticed an issue with that PR assumes the default exists although it will not (even prior to this PR) if a custom provider role is declared.

* @returns {boolean} Whether any of the declared functions did not have an iamRoleARN.
*/
anyFunctionHasNoRole() {
let ret = false;
if (typeof this.serverless.service.provider.iamRoleARN !== 'string') {

This comment has been minimized.

Copy link
@erikerikson

erikerikson Sep 12, 2016

Author Member

I believe there is a bug in this. if iamRoleArn is an object (i.e. { 'Fn::GetAtt': ['IamRoleLambdaExecution', 'Arn'] }) then this doesn't detect the declaration of that arn declaration.

Switch from an all lambdas logging resource IAM policy to one that targets specifically and only those CloudWatch logs produces by the lambdas declared by the service.
Modify tests to ensure this is properly done.

Introduce a `role` property that specifies a role defined within the service.
Update tests to ensure this is properly used
Update documentation to describe this

Replace `iamRoleARN` with `roleArn`
Update tests and documentation to reflect this

Add Decision Trees describing the decision points and considerations between individual function rights and shared rights models
@flomotlik

This comment has been minimized.

Copy link
Contributor

flomotlik commented Sep 15, 2016

Thanks again @erikerikson for this very extensive PR. I've looked over it and it looks good, but I want to do a larger deep dive on it as well tomorrow.

I love the roleARN and the ability to expand role to a custom resource. This also gives us the ability to add an object notation in the future where users don't need to define custom resources, but could do this in the function config if they want to create a role that is purely for one specific function. Great stuff!

Copy link
Member Author

erikerikson left a comment

I am assuming that I will have to merge the changes from #1944 into this before we're done

If you want to give permission to your functions to access certain resources on your AWS account, you can add custom IAM role statements to your service by adding the statements in the `iamRoleStatements` array in the `provider` object. As those statements will be merged into the CloudFormation template you can use Join, Ref or any other CloudFormation method or feature. You're also able to either use YAML for defining the statement (including the methods) or use embedded JSON if you prefer it. Here's an example that uses all of the above:
# Defining IAM Rights

Serverless provides no-configuration rights provisioning by default. You are welcome. Yet, if you ungratefully require greater control over rights provisioning it graciously gets out of your way too. Custom provider or function level roles may be specified either by named reference or by role ARN.

This comment has been minimized.

Copy link
@erikerikson

erikerikson Sep 15, 2016

Author Member

Apologies for the silliness. I once read that even vaguely interesting docs are enjoyed and read with a much greater probability. Plus, I was in a good mood.

This comment has been minimized.

Copy link
@flomotlik

flomotlik Sep 23, 2016

Contributor

thats alright, I might go over it at some point, but fine with me for now

erikerikson added 2 commits Sep 26, 2016
# Conflicts:
#	lib/plugins/aws/deploy/compile/functions/index.js
- ec2:DescribeNetworkInterfaces
- ec2:DetachNetworkInterface
- ec2:DeleteNetworkInterface
Resource: *

This comment has been minimized.

Copy link
@pmuens

pmuens Sep 30, 2016

Member

Need to be wrapped in "" so that YAML won't throw an exception --> Resource: "*"

This comment has been minimized.

Copy link
@erikerikson

erikerikson Sep 30, 2016

Author Member

Thanks for noticing this mistake!

- ec2:DescribeNetworkInterfaces
- ec2:DetachNetworkInterface
- ec2:DeleteNetworkInterface
Resource: *

This comment has been minimized.

Copy link
@pmuens

pmuens Sep 30, 2016

Member

Need to be wrapped in "" so that YAML won't throw an exception --> Resource: "*"

if (typeof this.serverless.service.provider.iamRoleARN === 'string') {
newFunction.Properties.Role = this.serverless.service.provider.iamRoleARN;
} else {
// translate role to roleArn

This comment has been minimized.

Copy link
@pmuens

pmuens Sep 30, 2016

Member

Could you add tests for this functionality? Thanks!

This comment has been minimized.

Copy link
@erikerikson

erikerikson Sep 30, 2016

Author Member

that's embarassing. yes. definitely. sorry! :)

This comment has been minimized.

Copy link
@pmuens

pmuens Sep 30, 2016

Member

No problem! Really appreciate all the efforts you've put in this PR! 🎸

@pmuens

This comment has been minimized.

Copy link
Member

pmuens commented Sep 30, 2016

First of all: Thank you very much @erikerikson for this PR!
It's a really great addition!

Tried it out today in a bunch of different combination and it works really great!

The only thing I was not able to get running was to set the role based on the name (e.g. in the provider property).
Here's my serverless.yml which fails to deploy. Am I missing something?

service: new-test-service

provider:
  name: aws
  runtime: nodejs4.3
  role: test123

functions:
  hello:
    handler: handler.hello

resources:
  Resources:
    test123:
      Type: AWS::IAM::Role
      Properties:
        RoleName: test123
        Path: "/"
        AssumeRolePolicyDocument:
          Version: '2012-10-17'
          Statement:
            - Effect: Allow
              Principal:
                Service:
                  - lambda.amazonaws.com
              Action: sts:AssumeRole
        Policies:
          - PolicyName: myPolicyName
            PolicyDocument:
              Version: '2012-10-17'
              Statement:
                - Effect: Allow
                  Action:
                    - logs:CreateLogGroup
                    - logs:CreateLogStream
                    - logs:PutLogEvents
                  Resource: arn:aws:logs:us-east-1:XXXXXXXX:log-group:/aws/lambda/*:*:*

But all in all a really great PR with an awesome documentation! 👯

@eahefnawy could you also do a deep dive into this and test out different combinations? Thanks!

@pmuens pmuens added this to the v1.0-ideas milestone Sep 30, 2016
1. fix docs that would lead to an error for users via copy-paste
2. add tests about adding roleArn to functions given role declared on provider and/or function
3. fix bug discovered due to lack of tests
4. add test to ensure preference for function declared roleArn over provider declared roleArn
@erikerikson

This comment has been minimized.

Copy link
Member Author

erikerikson commented Sep 30, 2016

I believe I have addressed all requested changes. Thanks @pmuens!

@erikerikson

This comment has been minimized.

Copy link
Member Author

erikerikson commented Sep 30, 2016

Oh yeah... And:

Am I missing something?

No, not at all, I f-d up (see commit changes to functions/index.js) but your asking me to write tests for the role=>roleArn case helped me catch it! Sorry, that case should totally work now.

@erikerikson

This comment has been minimized.

Copy link
Member Author

erikerikson commented Sep 30, 2016

I'll be in a position to actually push this into the official accounts soon. I apologize for the obvious errors due to a lack of use testing.

Resources:
myDefaultRole:
Type: AWS::IAM::Role
RoleName: MyDefaultRole

This comment has been minimized.

Copy link
@pmuens

pmuens Sep 30, 2016

Member

I think this should be nested in the Properties section.

This comment has been minimized.

Copy link
@erikerikson

erikerikson Oct 6, 2016

Author Member

Thank you for noticing this error too!

This comment has been minimized.

Copy link
@erikerikson

erikerikson Oct 6, 2016

Author Member

And it's like

Resources:
myCustRole0:
Type: AWS::IAM::Role
RoleName: MyCustRole0

This comment has been minimized.

Copy link
@pmuens

pmuens Sep 30, 2016

Member

I think this should be nested in the Properties section.

Resource: "*"
myCustRole1:
Type: AWS::IAM::Role
RoleName: MyCustRole1

This comment has been minimized.

Copy link
@pmuens

pmuens Sep 30, 2016

Member

I think this should be nested in the Properties section.

- "Ref" : "ServerlessDeploymentBucket"
myCustRole0:
Type: AWS::IAM::Role
RoleName: MyCustRole0

This comment has been minimized.

Copy link
@pmuens

pmuens Sep 30, 2016

Member

I think this should be nested in the Properties section.

@pmuens

This comment has been minimized.

Copy link
Member

pmuens commented Sep 30, 2016

Thank you very much @erikerikson

No worries you totally rock here 🎸 . Thanks for putting that much effort into it! 👍
Really appreciate it!

I've just added some minor change requests (seems like the RoleName property needs to be nested in side of `Properties).

Furthermore I've retried to deploy the following service:

service: new-test-service

provider:
  name: aws
  runtime: nodejs4.3
  role: test123

functions:
  hello:
    handler: handler.hello

resources:
  Resources:
    test123:
      Type: AWS::IAM::Role
      Properties:
        RoleName: test123
        Path: "/"
        AssumeRolePolicyDocument:
          Version: 2012-10-17
          Statement:
            - Effect: Allow
              Principal:
                Service:
                  - lambda.amazonaws.com
                Action: sts:AssumeRole
        Policies:
          - PolicyName: myPolicyName
            PolicyDocument:
              Version: 2012-10-17
              Statement:
                - Effect: Allow
                  Action:
                    - logs:CreateLogGroup
                    - logs:CreateLogStream
                    - logs:PutLogEvents
                  Resource: arn:aws:logs:us-east-1:XXXXXXXX:log-group:/aws/lambda/*:*:*

But I get this error when I want to deploy it:

   Requires capabilities : [CAPABILITY_NAMED_IAM]

Thanks in advance! And thanks again for your time you've put into this!

EDIT: Tried to add comments to all the RoleName usages. Somehow GitHub messed the comments up a little bit. Sorry for that 😒

erikerikson added 2 commits Oct 4, 2016
1. Make changes
2. Change tests
3. Change Docs
@erikerikson

This comment has been minimized.

Copy link
Member Author

erikerikson commented Oct 4, 2016

With this last commit (change roleArn to role), this should be ready for review once again.

@eahefnawy

This comment has been minimized.

Copy link
Member

eahefnawy commented Oct 5, 2016

thanks @erikerikson ... not sure if you've noticed @pmuens comments, but there seems to be an issue with the docs where the Properties parent property is missing from the example CF templates

erikerikson added 2 commits Oct 6, 2016
To the "createStack" call.  This allows for custom named IAM resources to be created within the stack that is sent.  I'm not sure that it won't create issues in some user's cases where they have very locked down rights.
@erikerikson

This comment has been minimized.

Copy link
Member Author

erikerikson commented Oct 6, 2016

@eahefnawy I had failed to notice @pmuens comments on the RoleNames needing to move under the Properties. Thank you for pointing this out. It is fixed now.

I also looked up @pmuens comments about the Requires capabilities : [CAPABILITY_NAMED_IAM] error and it appears that the API call must have an additional argument added to it.
http://docs.aws.amazon.com/AWSCloudFormation/latest/APIReference/API_CreateStack.html
http://docs.aws.amazon.com/AWSJavaScriptSDK/latest/AWS/CloudFormation.html#createStack-property
Specifically (from the last):

var params = {
  ...
  Capabilities: [
    'CAPABILITY_IAM | CAPABILITY_NAMED_IAM',
  ],
  ...
}

These docs follow:

Capabilities — (Array<String>)
A list of values that you must specify before AWS CloudFormation can create certain stacks. Some stack templates might include resources that can affect permissions in your AWS account, for example, by creating new AWS Identity and Access Management (IAM) users. For those stacks, you must explicitly acknowledge their capabilities by specifying this parameter.

The only valid values are CAPABILITY_IAM and CAPABILITY_NAMED_IAM. The following resources require you to specify this parameter: AWS::IAM::AccessKey, AWS::IAM::Group, AWS::IAM::InstanceProfile, AWS::IAM::Policy, AWS::IAM::Role, AWS::IAM::User, and AWS::IAM::UserToGroupAddition. If your stack template contains these resources, we recommend that you review all permissions associated with them and edit their permissions if necessary.

If you have IAM resources, you can specify either capability. If you have IAM resources with custom names, you must specify CAPABILITY_NAMED_IAM. If you don't specify this parameter, this action returns an InsufficientCapabilities error.

For more information, see Acknowledging IAM Resources in AWS CloudFormation Templates.

I have added the 'CAPABILITY_NAMED_IAM' flag to the create call. As you are surely all painfully aware (very sorry) I am not in a position to exercise this. I really have been trying to get myself in such a position but things have been coming up. I will continue to endeavour to get set up and stop leaning on you guys.
An unanswered question is whether this will cause issues if users don't have any IAM resources in the CFT. The use case for that is that all roles/policies/etc. are being generated and deployed independently and then ARNs are being provided for all roles. This is the only thing I'm concerned be tested.

@pmuens
pmuens approved these changes Oct 6, 2016
.Properties
.PolicyDocument
.Statement[0]
.Resource = `arn:aws:logs:${this.serverless.config.region}:*:*`;

This comment has been minimized.

Copy link
@flomotlik

flomotlik Oct 7, 2016

Contributor

We can't use this.serverless.config.region as it might be unset, e.g. when running serverless deploy --noDeploy you get an undefined in the CF template.

iamPolicyLambdaExecutionTemplate
.IamPolicyLambdaExecution
.Properties
.PolicyName = `${this.serverless.config.stage}-${this.serverless.service.service}-lambda`;

This comment has been minimized.

Copy link
@flomotlik

flomotlik Oct 7, 2016

Contributor

We can't use this.serverless.config.region as it might be unset, e.g. when running serverless deploy --noDeploy you get an undefined in the CF template.

This comment has been minimized.

Copy link
@erikerikson

erikerikson Oct 7, 2016

Author Member

Definitely thank you for find this. It is interesting.

I will have to change this on the naming PR too. I have been passing the options through a lot of Serverless constructors there in order to have access to them in the naming module. I'll comment there in order to discuss.

This comment has been minimized.

Copy link
@erikerikson

erikerikson Oct 7, 2016

Author Member

Also, thanks for inlining policies!

{
Bucket: bucketName,
},
this.serverless.config.stage,

This comment has been minimized.

Copy link
@flomotlik

flomotlik Oct 7, 2016

Contributor

We can't use this.serverless.config.region as it might be unset, e.g. when running serverless deploy --noDeploy you get an undefined in the CF template.

Bucket: bucketName,
},
this.serverless.config.stage,
this.serverless.config.region

This comment has been minimized.

Copy link
@flomotlik

flomotlik Oct 7, 2016

Contributor

We can't use this.serverless.config.region as it might be unset, e.g. when running serverless deploy --noDeploy you get an undefined in the CF template.

this.serverless.config.region
))
.then(result => {
if (result.LocationConstraint !== this.serverless.config.region) {

This comment has been minimized.

Copy link
@flomotlik

flomotlik Oct 7, 2016

Contributor

We can't use this.serverless.config.region as it might be unset, e.g. when running serverless deploy --noDeploy you get an undefined in the CF template.

@flomotlik

This comment has been minimized.

Copy link
Contributor

flomotlik commented Oct 7, 2016

To give some context on my comments:

"IamPolicyLambdaExecution": {
      "Type": "AWS::IAM::Policy",
      "Properties": {
        "PolicyName": "undefined-sls-test-aws-nodejs-6344-lambda",
        "PolicyDocument": {
          "Version": "2012-10-17",
          "Statement": [
            {
              "Effect": "Allow",
              "Action": [
                "logs:CreateLogGroup",
                "logs:CreateLogStream",
                "logs:PutLogEvents"
              ],
              "Resource": "arn:aws:logs:undefined:*:*"
            }
          ]
        },
        "Roles": [
          {
            "Ref": "IamRoleLambdaExecution"
          }
        ]
      }
    },
@flomotlik flomotlik assigned pmuens and unassigned eahefnawy Oct 10, 2016
@flomotlik flomotlik modified the milestones: V1.0, v1.0-ideas Oct 10, 2016
@erikerikson erikerikson mentioned this pull request Oct 10, 2016
5 of 5 tasks complete
@erikerikson

This comment has been minimized.

Copy link
Member Author

erikerikson commented Oct 11, 2016

Commit 77ab041 resolved @flomotlik's recent comments.

# Conflicts:
#	docs/02-providers/aws/02-iam.md
@flomotlik flomotlik modified the milestones: 1.1, V1.0 Oct 18, 2016
@pmuens

This comment has been minimized.

Copy link
Member

pmuens commented Oct 31, 2016

Just tested it once again with the --noDeploy switch and inspected the output.

Works as expected. Big 👍 and 🙌 for this feature @erikerikson Good job.

However if I want to deploy it it fails with the error message

  Serverless Error ---------------------------------------

     Invalid template resource property 'RoleName'

  Get Support --------------------------------------------
     Docs:          docs.serverless.com
     Bugs:          github.com/serverless/serverless/issues

  Your Environment Infomation -----------------------------
     OS:                 linux
     Node Version:       6.6.0
     Serverless Version: 1.0.2

Could you maybe rebase the PR once again as this bug can be related to the wrong resource merging (in the create stack step) which was resolved recently. After that I'll do another code review so that it can be merged ASAP.

Again. Thanks for your time and effort @erikerikson 💯

@erikerikson

This comment has been minimized.

Copy link
Member Author

erikerikson commented Oct 31, 2016

Absolutely @pmuens, although it will be a bit until I return from holiday

@pmuens pmuens mentioned this pull request Nov 1, 2016
6 of 6 tasks complete
@pmuens

This comment has been minimized.

Copy link
Member

pmuens commented Nov 1, 2016

@erikerikson thanks for your effort here 💯 .
Great addition to Serverless!

No hurry! We've got you covered. Resolved all the conflicts and fixed the branch in #2584 (I took your PR / branch and worked on top of it).

Thanks for everything and enjoy your holidays! 🙌 🌴

@pmuens pmuens closed this Nov 1, 2016
@flomotlik flomotlik removed this from the 1.1 milestone Nov 1, 2016
@erikerikson

This comment has been minimized.

Copy link
Member Author

erikerikson commented Nov 1, 2016

You rock @pmuens! Thank you!

@erikerikson erikerikson deleted the erikerikson:add-per-function-custom-roles branch Nov 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.