feat(event-source): add SQS as Lambda event source #451

brettstack · 2018-05-30T17:08:54Z

Issue #, if available:

Description of changes:

Adds support for SQS as an event source. This feature is not yet supported by AWS (pre-announced earlier this year) and will not be merged to master until it is available.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

sanathkr

LGTM in general. I have a few comments you can choose to ignore ;)

sanathkr · 2018-05-31T17:17:11Z

docs/internals/generated_resources.rst

+        MyTrigger:
+          Type: SQS
+          Properties:
+            Queue: arn:aws:sqs:us-east-1:123456789012:my-queue


should we call this property QueueArn instead? I wonder if users might get confused and specify a queue name here

I went with Queue to be consistent with Stream. Though I agree, QueueArn is more descriptive. If we want in the future we can add QueueArn (and StreamArn) leave Queue as an alias (and possibly deprecate?).

Fair enough. Let's stick with Queue to keep the language consistent.

sanathkr · 2018-05-31T17:19:06Z

tests/translator/input/streams.yaml

@@ -25,3 +25,15 @@ Resources:
            Stream: arn:aws:dynamodb:us-west-2:012345678901:table/TestTable/stream/2015-05-11T21:21:33.291
            BatchSize: 200
            StartingPosition: LATEST
+  SQSFunction:


Consider moving SQS test to a separate file. This file is already getting overloaded.

Yes we should consider moving this to a separate file. And this file is explicitly named Streams and hence SQS doesn't belong here.

muthiah90 · 2018-06-01T00:11:11Z

samtranslator/model/eventsources/pull.py

-    The pull events are the streams--Kinesis and DynamoDB Streams. Both of these correspond to an EventSourceMapping in
-    Lambda, and require that the execution role be given to Kinesis or DynamoDB Streams, respectively.
+    The pull events are Kinesis Streams, DynamoDB Streams, and SQS Queues. All of these correspond to an EventSourceMapping in
+    Lambda, and require that the execution role be given to Kinesis Streams, DynamoDB Streams, or SQS Streams, respectively.


I suppose we shouldn't be calling SQS as streams. SQS in itself should do. Thoughts ?

Good catch; that was a miss. I refer to it as SQS Queues in the first sentence.

muthiah90 · 2018-06-01T00:52:19Z

samtranslator/model/eventsources/pull.py

+        'Stream': PropertyType(False, is_str()),
+        'Queue': PropertyType(False, is_str()),
+        'BatchSize': PropertyType(False, is_type(int)),
+        'StartingPosition': PropertyType(False, is_str())


I suppose all properties are changed to not required (False) so as to support event source mapping for both Streams (DynamoDB and Kinesis) and SQS. But aren't we actually missing on validation ? I can create an Event Source mapping for DDB streams without the StartingPosition specified, it will pass this check but will end up in an error from CFN.

Good point. We do need to validate them separately now that these properties are not required

muthiah90 · 2018-06-01T00:55:21Z

samtranslator/model/lambda_.py

@@ -61,7 +61,7 @@ class LambdaEventSourceMapping(Resource):
            'Enabled': PropertyType(False, is_type(bool)),
            'EventSourceArn': PropertyType(True, is_str()),
            'FunctionName': PropertyType(True, is_str()),
-            'StartingPosition': PropertyType(True, is_str())
+            'StartingPosition': PropertyType(False, is_str())


Same as the previous comment. Aren't we letting go the validation of StartingPosition for streams because of SQS which doesn't require it.

JSON Schema validation will pick this up once we turn it on (it's currently not doing anything with failed JSON Schemas as we're not yet ready). Would prefer to not add additional validation right now.

What if someone forgot to set the Queue property? SAM Translator will crash instead of giving them a validation error. After turning on JSON Schema, we do need a deeper scrub of code. This extra validation won't increase the surface area of the scrub significantly IMO

muthiah90 · 2018-06-01T00:59:30Z

tests/translator/input/streams.yaml

@@ -25,3 +25,15 @@ Resources:
            Stream: arn:aws:dynamodb:us-west-2:012345678901:table/TestTable/stream/2015-05-11T21:21:33.291
            BatchSize: 200
            StartingPosition: LATEST
+  SQSFunction:


Yes we should consider moving this to a separate file. And this file is explicitly named Streams and hence SQS doesn't belong here.

muthiah90 · 2018-06-01T01:00:10Z

tests/translator/input/streams.yaml

+          Type: SQS
+          Properties:
+            Queue: arn:aws:sqs:us-west-2:012345678901:my-queue
+            BatchSize: 200


Batch size for SQS is only between 1 and 10

muthiah90 · 2018-06-01T06:02:16Z

tests/translator/output/aws-cn/streams.json

+    "SQSFunctionMySqsQueue": {
+      "Type": "AWS::Lambda::EventSourceMapping",
+      "Properties": {
+        "BatchSize": 200,


Batch size for SQS is between 1 and 10

muthiah90 · 2018-06-01T06:04:18Z

tests/translator/output/aws-us-gov/streams.json

+    "SQSFunctionMySqsQueue": {
+      "Type": "AWS::Lambda::EventSourceMapping",
+      "Properties": {
+        "BatchSize": 200,


BatchSize between 1 and 10

muthiah90 · 2018-06-01T06:05:02Z

tests/translator/output/streams.json

+    "SQSFunctionMySqsQueue": {
+      "Type": "AWS::Lambda::EventSourceMapping",
+      "Properties": {
+        "BatchSize": 200,


Batch size between 1 and 10

muthiah90 · 2018-06-01T06:07:08Z

versions/2016-10-31.md

+```yaml
+Type: SQS
+Properties:
+  Stream: arn:aws:sqs:us-west-2:012345678901:my-queue # NOTE: FIFO SQS Queues are not yet supported


This should be Queue: and not Stream:

…tchSize from 200 to 10 (max)

sanathkr

Can we add validation for the Queue and Stream property? In the current code, if Queue or Stream property is not specified, customers will get an invalid CFN whichi will fail deployment with a weird error.

sanathkr

This works, but I think you can do better :)

sanathkr · 2018-06-01T19:11:08Z

samtranslator/model/eventsources/pull.py

@@ -48,6 +49,10 @@ def to_cloudformation(self, **kwargs):
        except NotImplementedError:
            function_name_or_arn = function.get_runtime_attr("arn")

+        if not self.Stream and not self.Queue:


Can you instead add a validate functino to each subclass that gets called here? Each subclass can then validate and return a customized error message. What do you think?

I deliberately went with this simpler approach since this is temporary code and I think the error message is clear enough even when they are combined for both cases.

sanathkr

LGTM assuring json schema will do better validation in future

muthiah90

Looks good to me !

* doc: update DESIGN.md to include an Overview and entry point for SAM (#370) * Update DESIGN.md to include an Overview and entry point for SAM * Updated wording in DESIGN.md based on PR feedback * docs(example): improve microservice-http-endpoint-python3 example (#415) * Remove unnecessary index.js" * Moved common config to template Globals and uncomment debug log. * Add sample payload and commands for testing. * Rename test script as a script. * Exit from script after command. * Simplify use case for easier startup. - Move table creation into template. - Eliminates need to provide table name in every REST call. - Fix GET with no params. - Split test.sh script from output log. * Address requested changes. * doc(example): update api_backend example to use ES6 arrow function immediate return (#446) Since this code is using ES6 we can use the short version of returning a param * feat(event-source): add SQS as Lambda event source (#451) * feat(cli): add lightweight transformer cli (#459) * bug: fix ANY method ARN generation using wildcard (#449) * fix(event-source): fix SQS Queue schema to allow intrinsic functions (#465) * fix: log a warning on invalid schema validation (#466)

brettstack added 7 commits April 18, 2018 10:54

release: 1.5.1

bcb74c0

Merge remote-tracking branch 'upstream/develop' into develop

95aafd7

Merge remote-tracking branch 'upstream/develop' into develop

a5fc1b1

feat(event-source): add SQS event source

01451e1

Merge remote-tracking branch 'upstream/develop' into feature/sqs-event

062194b

uncomment code coverage

9bbee00

test: fix aws-cn output for streams with SQS

7f186ce

brettstack requested review from sanathkr and jfuss May 30, 2018 17:10

brettstack self-assigned this May 30, 2018

sanathkr previously approved these changes May 31, 2018

View reviewed changes

muthiah90 reviewed Jun 1, 2018

View reviewed changes

fix comment 'SQS Streams' -> 'SQS Queues'

bb325a1

brettstack dismissed sanathkr’s stale review via bb325a1 June 1, 2018 00:26

muthiah90 reviewed Jun 1, 2018

View reviewed changes

move sqs tests into separate file; fix doc (Stream=>Queue); reduce Ba…

c100723

…tchSize from 200 to 10 (max)

digioak-zz previously approved these changes Jun 1, 2018

View reviewed changes

sanathkr suggested changes Jun 1, 2018

View reviewed changes

add validation for missing Queue or Stream

a26e50c

brettstack dismissed digioak-zz’s stale review via a26e50c June 1, 2018 18:54

sanathkr suggested changes Jun 1, 2018

View reviewed changes

sanathkr previously approved these changes Jun 1, 2018

View reviewed changes

add validation for StartingPosition when Stream is specified

e6d4910

brettstack dismissed sanathkr’s stale review via e6d4910 June 1, 2018 21:58

muthiah90 approved these changes Jun 1, 2018

View reviewed changes

sanathkr approved these changes Jun 1, 2018

View reviewed changes

digioak-zz approved these changes Jun 1, 2018

View reviewed changes

brettstack merged commit a5a3845 into aws:develop Jun 1, 2018

alexdebrie mentioned this pull request Jun 25, 2018

Add SQS event integration serverless/serverless#5074

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(event-source): add SQS as Lambda event source #451

feat(event-source): add SQS as Lambda event source #451

brettstack commented May 30, 2018

sanathkr left a comment

sanathkr May 31, 2018

brettstack May 31, 2018

sanathkr May 31, 2018

sanathkr May 31, 2018

muthiah90 Jun 1, 2018

muthiah90 Jun 1, 2018

brettstack Jun 1, 2018

muthiah90 Jun 1, 2018

sanathkr Jun 1, 2018

muthiah90 Jun 1, 2018

brettstack Jun 1, 2018

sanathkr Jun 1, 2018

muthiah90 Jun 1, 2018

muthiah90 Jun 1, 2018

muthiah90 Jun 1, 2018

muthiah90 Jun 1, 2018

muthiah90 Jun 1, 2018

muthiah90 Jun 1, 2018

sanathkr left a comment

sanathkr left a comment

sanathkr Jun 1, 2018

brettstack Jun 1, 2018

sanathkr left a comment

muthiah90 left a comment

feat(event-source): add SQS as Lambda event source #451

feat(event-source): add SQS as Lambda event source #451

Conversation

brettstack commented May 30, 2018

sanathkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanathkr left a comment

Choose a reason for hiding this comment

sanathkr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanathkr left a comment

Choose a reason for hiding this comment

muthiah90 left a comment

Choose a reason for hiding this comment