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

AWS: Consolidates Lambda::Permission objects for cloudwatchLog events #5531

Merged
merged 14 commits into from Jan 21, 2019

Conversation

Projects
5 participants
@exoego
Copy link
Contributor

exoego commented Nov 28, 2018

What did you implement:

Closes #5357

How did you implement it:

Instead of creating Permission object separately for every cloudwatchLog events,
one-and-only Permission is created and shared with cloudwathLog-subsciber functions in one service yml.

Instead of creating Permission object separately for every cloudwatchLog events,
one-and-only Permission is created per function and shared with cloudwathLog events.

How can we verify it:

  1. Create service with many functions and cloudwatchLog subscriber
service: service-for-verification
provider:
  name: aws
  runtime: nodejs8.10
functions:
  hello1:
    handler: handler.hello1
  hello2:
    handler: handler.hello2
   ...
  hello40:
    handler: handler.hello40
  subscriber:
    handler: handler.subscriber
    events:
     - cloudwatchLog: "/aws/lambda/hello1"
     - cloudwatchLog: "/aws/lambda/hello2"
    ...
     - cloudwatchLog: "/aws/lambda/hello40"
  1. Deploy it

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?: YES

Note:

If we deploy on top of this PR, Lambda console will show warning message like below:

'cloudwatch-logs é¢ä¿ããªã¹ãããéã«ã¨ã©ã¼ãçºçãã¾ããï¼2 validation errors
 detected: Value '' at 'logGroupName' failed to satisfy constraint: Member 
must satisfy regular expression pattern: [\.\-_/#A-Za-z0-9]+; Value '' at
 'logGroupName' failed to satisfy constraint: Member must have length
 greater than or equal to 1 (Service: AWSLogs; Status Code: 400; Error Code:
 InvalidParameterException; Request ID: ....)

According to AWS support, this is cosmetic error and negligible.

exoego added some commits Nov 28, 2018

@exoego exoego closed this Nov 28, 2018

@exoego exoego reopened this Nov 28, 2018

@exoego exoego closed this Nov 28, 2018

@exoego exoego reopened this Nov 28, 2018

@dschep dschep self-assigned this Nov 29, 2018

@Jaystified

This comment has been minimized.

Copy link

Jaystified commented Dec 4, 2018

Applied this change on top of 1.32.0 and my previously undeployable stack seems to deploy properly and work well.
Hopefully this gets merged soon.

":log-group:",
"${LogGroupName}",
":*"
":log-group:*:*"

This comment has been minimized.

@dschep

dschep Dec 4, 2018

Member

Could this still be more restrictive? IE, a service foobar in stage dev has log groups that are all of the pattern /aws/lambda/foobar-dev-*.

This comment has been minimized.

@exoego

exoego Dec 5, 2018

Author Contributor

Done in 6d0cbe3 and 7dadcdd

Now it creates more restrictive pattern such as /aws/lambda/foobar-dev-* against /aws/lambda/foobar-dev-getFoo and /aws/lambda/foobar-dev-postFoo.
And such pattern and API::Lambda::Permission are consolidated per function, not per service, since logGroup are configured per subscriber-function.

@exoego exoego force-pushed the exoego:logs-compact-permission branch from 18bce37 to 99f3509 Dec 5, 2018

@exoego

This comment has been minimized.

Copy link
Contributor Author

exoego commented Dec 6, 2018

Added Note to PR summary.

@dschep

This comment has been minimized.

Copy link
Member

dschep commented Dec 6, 2018

Hey @exoego, upon reviewing this more closely. I'm not sure I like the combining of log groups with wildcard based on the longest overlap. It introduces unexpected, and more importantly unconfigurable, broader than necessary permissions. Eg, if a user subscribes to a lambda loggroup(/aws/lambda/foo-bar-baz) and custom log group(/foobar), a user unintentionally gave permission to ALL logs, and can't prevent that.

I completely understand that #5357 is a problem. What would you think about using the expanded cloudwatchLog option form and adding a permissionPattern(or similar) option and using that option if it's set and collapsing any of those that are the same into one permission? Using it would look like this:

    events:
      - cloudwatchLog:
          logGroup: '/aws/lambda/hello1'
          permissionPattern: '/aws/lambda/hello*'
      - cloudwatchLog:
          logGroup: '/aws/lambda/hello2'
          permissionPattern: '/aws/lambda/hello*'
      - cloudwatchLog:
          logGroup: '/aws/lambda/hello3'
          permissionPattern: '/aws/lambda/hello*'

it's a bit verbose, but I think it would work for your use case while avoiding introducing unexpected behavior.

@exoego

This comment has been minimized.

Copy link
Contributor Author

exoego commented Dec 6, 2018

@dschep
If I understand AWS correctly, I suppose your concern is more than necessary.

Yes, implicit wildcard may give unnecessarily-wide pattern like /* to LogGroup.
But LogGroup cannot InvokeFunction unless it has SubscriptionFilter.
And, according to AWS document, LogGroup has a hard-limit "Subscription filters | 1 per log group. This limit cannot be changed"
A SubscriptionFilter attached to LogGroup is the one which an user like me is going to deploy via serverless.yml.
And the SubscriptionFilter Lambda is allowed to perform very-limited operations via provider.iamRoleStatements.
So... I think implicit wildcard does not become a security concern.

Please correct me if I miss something.
If un-configurability is unfit to design policy of Severless Framework, I am OK to update PR with explicit wildcard via permissionPattern.

@exoego exoego changed the title Consolidates Lambda::Permission objects for cloudwatchLog events [AWS] Consolidates Lambda::Permission objects for cloudwatchLog events Dec 6, 2018

@exoego exoego changed the title [AWS] Consolidates Lambda::Permission objects for cloudwatchLog events AWS: Consolidates Lambda::Permission objects for cloudwatchLog events Dec 10, 2018

@pmuens pmuens added the pr/in-review label Jan 15, 2019

@eahefnawy eahefnawy added this to In progress in Serverless via automation Jan 17, 2019

@eahefnawy eahefnawy moved this from In progress to Needs review in Serverless Jan 17, 2019

Serverless automation moved this from Needs review to Reviewer approved Jan 21, 2019

@dschep

dschep approved these changes Jan 21, 2019

@dschep dschep merged commit 011b579 into serverless:master Jan 21, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.01%) to 90.795%
Details

Serverless automation moved this from Reviewer approved to Done Jan 21, 2019

@exoego exoego deleted the exoego:logs-compact-permission branch Jan 21, 2019

@shortjared shortjared added this to the v1.36.3 milestone Jan 22, 2019

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