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

Add support for Kinesis Data Streams Enhanced Fan-out (Consumers) #7320

Merged
merged 5 commits into from
Feb 14, 2020

Conversation

zaccharles
Copy link
Contributor

What did you implement

Closes #5510

I added the a new configuration property for Kinesis data stream events which configures the event source mapping to use a stream consumer instead of shard iterator.

It creates a new stream consumer when set to true and uses an existing consumer if given the ARN of one.

It manages the necessary IAM permissions and supports functions such as Ref and ImportValue (essential since stream consumer ARNs are AWS generated with timestamps in them).

This is my first PR of this size to this project, so apologies if I didn't quite get some conventions right.

How can we verify it

service: dct

provider:
  name: aws
  runtime: nodejs12.x

functions:
  hello:
    handler: handler.hello
    events:
      - stream:
          batchSize: 1
          type: kinesis
          arn:
            Fn::GetAtt:
              - ExampleStream
              - Arn
          consumer: true
      - stream:
          batchSize: 1
          type: kinesis
          arn:
            Fn::GetAtt:
              - ExampleStream
              - Arn
          consumer:
            Ref: ExampleStreamConsumer

resources:
  Resources:
    ExampleStream: 
      Type: AWS::Kinesis::Stream 
      Properties: 
          ShardCount: 1
    ExampleStreamConsumer: 
      Type: AWS::Kinesis::StreamConsumer
      Properties: 
          StreamARN:
            Fn::GetAtt:
              - ExampleStream
              - Arn
          ConsumerName: whatever

Todos

No todos at the moment.

  • Write and run all tests
  • Write documentation
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

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

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great thanks @zaccharles It looks really good.

I've proposed few improvements, let me know what you think

lib/plugins/aws/package/compile/events/stream/index.js Outdated Show resolved Hide resolved
lib/plugins/aws/package/compile/events/stream/index.js Outdated Show resolved Hide resolved
lib/plugins/aws/package/compile/events/stream/index.js Outdated Show resolved Hide resolved
lib/plugins/aws/package/compile/events/stream/index.js Outdated Show resolved Hide resolved
lib/plugins/aws/package/compile/events/stream/index.js Outdated Show resolved Hide resolved
@zaccharles
Copy link
Contributor Author

@medikoo Hi, thanks for the review. I've made some more changes. Specifically, I've relied more on CloudFormation validating things, which is probably a good approach. This has allowed me to simplify things a bit. I'm happy to accept more feedback.

@codecov-io
Copy link

codecov-io commented Feb 12, 2020

Codecov Report

Merging #7320 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #7320   +/-   ##
=======================================
  Coverage   87.91%   87.91%           
=======================================
  Files         240      240           
  Lines        8888     8888           
=======================================
  Hits         7814     7814           
  Misses       1074     1074
Impacted Files Coverage Δ
lib/plugins/aws/package/lib/mergeIamTemplates.js 100% <100%> (ø) ⬆️
...ckage/compile/events/apiGateway/lib/permissions.js 88.23% <100%> (ø) ⬆️
.../package/compile/events/apiGateway/lib/validate.js 96.94% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 73fc5a3...f8fb2a5. Read the comment docs.

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @zaccharles looks really good now, just added one extra suggestion.

lib/plugins/aws/package/compile/events/stream/index.js Outdated Show resolved Hide resolved
@zaccharles
Copy link
Contributor Author

It's quite a lot simpler now without any of the validation 😄

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, thank you @zaccharles !

@xharsin
Copy link

xharsin commented Jun 16, 2020

@zaccharles Is it possible to use the function name defined with name in serverless under function.

 functions:
   preprocess:
     name: preprocess-test
     handler: handler.preprocess
     events:
       - stream:
           arn: arn:aws:kinesis:region:XXXXXX:stream/foo
           consumer: true

The Kinesis consumer is created with functionNamestreamNameConsumer as naming but it is getting the functionName as preprocess and not preprocess-test

I my use case I need to deploy multiple functions with stages name so I am adding stage name in functions names using name

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

Successfully merging this pull request may close these issues.

Add support for AWS Kinesis enhanced stream consumer for Lambda
4 participants