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

Using Fn:ImportValue for role should create empty DependsOn for strea… #4084

Conversation

@tobyhede
Copy link
Contributor

commented Aug 14, 2017

What did you implement:

Closes #4083

How did you implement it:

Add test for use of Fn:ImportValue in a function role, and set DependsOn to be an empty array in that case.

How can we verify it:

Please see the bug report for examples of config.
It should be possible to use Fn:ImportValue for a role when using a kinesis event.

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

…m event source mapping

Resolves #4083
@pmuens pmuens added the pr/in-review label Aug 14, 2017
@pmuens pmuens self-requested a review Aug 14, 2017
@pmuens pmuens modified the milestone: 1.20 Aug 14, 2017
Copy link
Member

left a comment

Thanks for PRing this fix @tobyhede 👍

Just looked over the code and it looks good! Could you please provide a simple service(s) setup so that we can copy-and-paste it and validate this real quick?

Thanks in advance!

@tobyhede

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2017

Given a CloudFormation stack with an exported role:

---
Resources:
  ExtendedLambdaRole:
    Type: AWS::IAM::Role
    Properties:
      AssumeRolePolicyDocument:
        Statement:
          - Effect: Allow
            Principal:
              Service:
                - lambda.amazonaws.com
            Action: sts:AssumeRole
      Policies:
        - PolicyName: LambdaKinesisRole
          PolicyDocument:
            Statement:
              - Effect: Allow
                Action:
                  - ec2:CreateNetworkInterface
                  - ec2:DescribeNetworkInterfaces
                  - ec2:DetachNetworkInterface
                  - ec2:DeleteNetworkInterface
                Resource: "*"
              - Effect: Allow
                Resource:
                  !Sub arn:aws:logs:${AWS::Region}:*:log-group:/aws/lambda/*:*:*
                Action:
                  - logs:CreateLogGroup
                  - logs:CreateLogStream
                  - logs:PutLogEvents
              - Effect: Allow
                Resource:
                  !Sub arn:aws:kinesis:${AWS::Region}:*:stream/*
                Action:
                  - kinesis:*

Outputs:
  ExtendedLambdaRoleArn:
    Description: ExtendedLambdaRoleArn
    Value:
      Fn::GetAtt:
      - ExtendedLambdaRole
      - Arn
    Export:
      Name:
        Fn::Sub: "${AWS::StackName}-ExtendedLambdaRoleArn"

A hello world serverless project with the following config should be deployable:

service: validation

provider:
  name: aws
  runtime: nodejs6.10

functions:
  hello:
    handler: handler.hello
    role:
      Fn::ImportValue: Validation-ExtendedLambdaRoleArn
    events:
      - stream:
          type: kinesis
          arn:
            Fn::GetAtt:
              - Kinesis
              - Arn
resources:
  Resources:
    Kinesis:
      Type: AWS::Kinesis::Stream
      Properties:
        Name: ValidationStream
        ShardCount: 1            

In the above I have assumed that the Role stack was called Validation and the role is available for import as Validation-ExtendedLambdaRoleArn.

@tobyhede

This comment has been minimized.

Copy link
Contributor Author

commented Sep 5, 2017

Previously the deploy would fail with Unresolved resource dependencies.

@pmuens

This comment has been minimized.

Copy link
Member

commented Sep 6, 2017

Thanks for providing the templates to validate this enhancement 👍

Just tested it today and it works great!

Thanks for PRing this change 🎉 --> Merging :shipit:

@pmuens
pmuens approved these changes Sep 6, 2017
@pmuens pmuens merged commit 1f2d811 into serverless:master Sep 6, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@pmuens pmuens added this to the 1.22 milestone Sep 6, 2017
@eahefnawy eahefnawy referenced this pull request Sep 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.