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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add custom per function roles #2584

Merged
merged 33 commits into from Nov 8, 2016

Conversation

Projects
None yet
4 participants
@pmuens
Member

pmuens commented Nov 1, 2016

What did you implement:

Closes #1807, #1895 and #2073

Updated version of @erikerikson PR #2073 (thanks mate 馃檶 ).

How did you implement it:

See #2073 (comment)

How can we verify it:

service: aws-nodejs

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

functions:
  hello:
    handler: handler.hello
    # role: role1
  world:
    handler: handler.hello
    # role: role1

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

Things to test

  • Custom IamRoleStatements merging
  • provider level role
  • function level role
  • Combination of all mentioned above

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config/commands/resources
  • Change ready for review message below

Is this ready for review?: YES

erikerikson and others added some commits Sep 8, 2016

Allow Custom Per-Function Roles
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.
Add Custom Roles Documentation
Adds documentation about how the feature works and examples of how to use it.
Tighten Log Rights/iamRoleARN=>role,roleArn/Add Docs
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
Merge branch 'master' into add-per-function-custom-roles
# Conflicts:
#	lib/plugins/aws/deploy/compile/functions/index.js
Review Fixes
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
Merge branch 'master' into add-per-function-custom-roles
# Conflicts:
#	lib/plugins/aws/deploy/compile/functions/index.js
#	lib/plugins/aws/deploy/compile/functions/tests/index.js
Collapse role & roleArn Down To role
1. Make changes
2. Change tests
3. Change Docs
Add 'CAPABILITY_NAMED_IAM'
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.
Merge branch 'master' into add-per-function-custom-roles
# Conflicts:
#	docs/02-providers/aws/02-iam.md

pmuens added some commits Nov 1, 2016

@pmuens pmuens referenced this pull request Nov 1, 2016

Closed

Allow Custom Per-Function Roles #2073

6 of 6 tasks complete

@flomotlik flomotlik added this to the 1.1 milestone Nov 1, 2016

@eahefnawy

Looking good @pmuens ... just have some minor docs improvements ... the implementation is simpler than I thought. Testing it a bit more and will get back to you 馃憤

- Effect: "Allow"
Action:
- "s3:ListBucket"
Resource: { "Fn::Join" : ["", ["arn:aws:s3:::", { "Ref" : "ServerlessDeploymentBucket"} ] ] }

This comment has been minimized.

@eahefnawy

eahefnawy Nov 2, 2016

Member

@pmuens I know this is valid yaml and probably not related to this PR, but imo the examples should be in yaml syntax for consistency and simplicity. So I'd ditch the brackets, quotes...etc

This comment has been minimized.

@pmuens

pmuens Nov 2, 2016

Member

Good catch! Will update it! 馃憤

name: aws
# declare one of the following...
role: myDefaultRole # must validly reference a role defined in the service
role: arn:aws:iam::0123456789:role//my/default/path/myDefaultRole # must validly reference a role defined in your account

This comment has been minimized.

@eahefnawy

eahefnawy Nov 2, 2016

Member

are you sure that would work? you're passing the ARN directly even though the resource might not be created yet because it's defined as a custom resource

This comment has been minimized.

@eahefnawy

eahefnawy Nov 2, 2016

Member

if you're referencing roles created outside of Serverless, it's best if we change the myDefaultRole name because it gives the impression that you're referencing the custom resources defined below

This comment has been minimized.

@pmuens

pmuens Nov 2, 2016

Member

Yep. Will update it 馃憤

@@ -20,6 +20,7 @@ module.exports = {
OnFailure: 'ROLLBACK',
Capabilities: [
'CAPABILITY_IAM',
'CAPABILITY_NAMED_IAM',

This comment has been minimized.

@eahefnawy

eahefnawy Nov 2, 2016

Member

why do we need that?

This comment has been minimized.

@pmuens

pmuens Nov 2, 2016

Member

That's @erikerikson commit with description

Add 'CAPABILITY_NAMED_IAM'

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.

@flomotlik flomotlik modified the milestones: 1.1, 1.2 Nov 2, 2016

@flomotlik flomotlik removed this from the 1.1 milestone Nov 2, 2016

@pmuens

This comment has been minimized.

Member

pmuens commented Nov 2, 2016

Ok. So I updated the docs and gave it a deep dive.

I tested many different scenarios with --noDeploy and a real deployment. It works as expected and is GTM from my side.

Would be great if others can also play around with it a little bit and provide feedback.

Here's the serverless.yml I've used (just comment / uncomment) the specific blocks to use different role setups):

service: custom-per-function-roles

provider:
  name: aws
  runtime: nodejs4.3
  #role: role1
  #iamRoleStatements:
  #  - Effect: Allow
  #    Action:
  #      - s3:*
  #      - kinesis:*
  #      - dynamodb:*
  #    Resource: "*"

functions:
  hello:
    handler: handler.hello
    #role: role1
  world:
    handler: handler.hello
    #role: role2

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

This comment has been minimized.

Member

pmuens commented Nov 3, 2016

@eahefnawy is the iamRoleArn property for the provider still relevant in this PR?

I think it can be replaced with the role property, right? If that's the case I'll update the PR and remove the usage.

Should we add a deprecation message if we remove iamRoleArn?

@flomotlik

This comment has been minimized.

Contributor

flomotlik commented Nov 4, 2016

@pmuens please remove iamRoleARN from the codebase. It wasn't documented afterwards as I knew this PR was coming, so we can drop it.

@pmuens

This comment has been minimized.

Member

pmuens commented Nov 4, 2016

Just updated the PR and remove the iamRoleARN usage. Tested it thoroughly. GTM from my side.

@eahefnawy

This comment has been minimized.

Member

eahefnawy commented Nov 7, 2016

@pmuens If a user provides a custom role and a stream event, serverless deploy would fail with error can not read property "Properties" of undefined because the default policy document is not present anymore. We need to handle that case otherwise deployment is not possible at all!

Could you add an if statement that checks whether the default role/policy template exists and only add the default permissions in that case? if not, don't add permissions and just deploy the stream event source mapping. This insures serverless deploy would always work, but in the latter case the user will just need to provide the permissions himself, which you already documented 馃憤

@pmuens

This comment has been minimized.

Member

pmuens commented Nov 7, 2016

@eahefnawy nice catch! Updated the stream event compilation accordingly.

@pmuens pmuens force-pushed the add-custom-per-function-roles branch from 0afe51f to 66be13d Nov 8, 2016

@pmuens

This comment has been minimized.

Member

pmuens commented Nov 8, 2016

@eahefnawy just added the test for the stream events! Let me know if there are any other updates I should add!

@pmuens pmuens modified the milestone: 1.2 Nov 8, 2016

@eahefnawy eahefnawy merged commit dce44fc into master Nov 8, 2016

3 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 94.746%
Details

@eahefnawy eahefnawy deleted the add-custom-per-function-roles branch Nov 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment