feat(AWS Lambda): Add support for self-managed `kafka` event #8784

lewgordon · 2021-01-19T13:37:28Z

Closes: #8718

codecov · 2021-01-19T13:44:39Z

Codecov Report

Merging #8784 (066a196) into master (cd5a739) will increase coverage by 0.02%.
The diff coverage is 98.33%.

@@            Coverage Diff             @@
##           master    #8784      +/-   ##
==========================================
+ Coverage   87.74%   87.77%   +0.02%     
==========================================
  Files         264      265       +1     
  Lines        9853     9913      +60     
==========================================
+ Hits         8646     8701      +55     
- Misses       1207     1212       +5

Impacted Files	Coverage Δ
lib/plugins/index.js	`100.00% <ø> (ø)`
lib/plugins/aws/package/compile/events/kafka.js	`98.24% <98.24%> (ø)`
lib/plugins/aws/lib/naming.js	`97.67% <100.00%> (+0.04%)`	⬆️
lib/Serverless.js	`92.24% <0.00%> (-3.11%)`	⬇️

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 cd5a739...066a196. Read the comment docs.

pgrzesik

Thank you @lewgordon 👍 Overall it's a great work, I have a few comments, please check them out and let me know what you think. I'll also ask @medikoo for an extra review to asses the chosen approach as correct.

lib/plugins/aws/lib/naming.js

lib/plugins/aws/package/compile/events/msk/index.js

test/unit/lib/plugins/aws/package/compile/events/msk/index.test.js

pgrzesik

Thank you @lewgordon, in general it looks good, I have a few questions though, please see my comments and let me know what you think

pgrzesik · 2021-01-21T19:29:14Z

lib/plugins/aws/package/compile/events/kafka.js

+      const functionObj = this.serverless.service.getFunction(functionName);
+      const cfTemplate = this.serverless.service.provider.compiledCloudFormationTemplate;
+
+      // It is required to add the following statement in order to be able to connect to MSK cluster


Just to confirm, is it still required at all times or only when VPC-based accessConfiguration is used?

Hmm, not sure, but will double check.

I believe it's only required for VPC-based accessConfiguration.

pgrzesik · 2021-01-21T19:36:19Z

docs/providers/aws/events/kafka.md

+    handler: handler.compute
+    events:
+      - kafka:
+          accessConfigurations:


Which approach is the valid one in the end? I see in the documentation here that accessConfigurations are supposed to be an Object, however, in tests and in processing logic it expects it to be an array. Based on recent discussion here #8718 I got the impression that we could go with object-based approach.

Yep, good catch. I think my documentation is right, but the code may need some fixes.

Thanks for clarification 🙌

pgrzesik · 2021-01-21T19:36:37Z

lib/plugins/aws/package/compile/events/kafka.js

+      type: 'object',
+      properties: {
+        accessConfigurations: {
+          type: 'array',


Should be an array or an object?

pgrzesik · 2021-01-21T19:38:13Z

test/unit/lib/plugins/aws/package/compile/events/kafka.test.js

+
+chai.use(require('chai-as-promised'));
+
+describe('AwsCompileKafkaEvents', () => {


I'm not sure if this is documented, but it's recommended to name new describe blocks after the name of the test file, so in this case it will be test/unit/lib/plugins/aws/package/compile/events/kafka.test.js

lewgordon · 2021-01-22T17:01:54Z

I also just verified that BASIC_AUTH is not supported for self-managed Apache Kafka, so I removed it.

pgrzesik

Looks good overall, I have two minor comments, please let me know what you think. I'd also like to wait with final merge until @medikoo has a chance to re-review it 👍

pgrzesik · 2021-01-24T14:25:06Z

lib/plugins/aws/package/compile/events/kafka.js

+          type: 'boolean',
+        },
+        bootstrapServers: {
+          type: 'array',


Should we also specify the type of item that's allowed here?

pgrzesik · 2021-01-24T14:28:53Z

lib/plugins/aws/package/compile/events/kafka.js

+        if (secretsManagerStatement.Resource.length) {
+          statement.push(secretsManagerStatement);
+        }
+        if (needsEc2Permissions) {


pgrzesik · 2021-01-24T14:32:31Z

test/unit/lib/plugins/aws/package/compile/events/kafka.test.js

+      expect(eventSourceMappingResource.Properties).to.deep.equal(eventConfig.resource(awsNaming));
+    };
+
+    const requirePropertyTest = async (event) => {


Our standard approach is not to test schema validation in such manner - we instead test all supported scenarios which you already have. I believe these tests could be removed

medikoo

@lewgordon that's outstanding! Thank you.

In addition to @pgrzesik comments, I have just few minor improvement suggestions from me.

Let me know if anything is not clear

medikoo · 2021-01-25T12:06:05Z

lib/plugins/aws/package/compile/events/kafka.js

+          type: 'object',
+          minProperties: 1,
+          properties: {
+            vpcSubnet: arrayOrStringSchema('subnet-[a-z0-9]+'),


We don't need to use arrayOrStringSchema, as in schema validator we have to array coercion configured to be on (https://ajv.js.org/docs/coercion.html)

So we should simply configure those properties as arrays. Singular values will automatically be allowed, and in code we can unconditionally expect arrays (as coercion will ensure all values are converted to arrays during validation phase)

Thanks for bringing that up @medikoo - I was not aware of that 👍

@medikoo after changing this, should I inline the lambda function or keep as is? I wasn't sure what was more readable.

I would inline the schema (when inspecting the schema, then it's easier to acknowledge what's defined)

medikoo · 2021-01-25T12:06:57Z

lib/plugins/aws/package/compile/events/kafka.js

+            saslScram256Auth: arrayOrStringSchema(secretsManagerArnRegex),
+            saslScram512Auth: arrayOrStringSchema(secretsManagerArnRegex),
+          },
+        },


Misses additionalProperties: false

medikoo · 2021-01-25T12:16:49Z

lib/plugins/aws/package/compile/events/kafka.js

+      let needsEc2Permissions = false;
+
+      functionObj.events.forEach((event) => {
+        if (event.kafka == null) {


Let's improve it to simply if (!event.kafka) return;

medikoo · 2021-01-25T12:18:48Z

lib/plugins/aws/package/compile/events/kafka.js

+        kafkaResource.Properties.SourceAccessConfigurations = [];
+        Object.entries(event.kafka.accessConfigurations).forEach(
+          ([accessConfigurationType, value]) => {
+            const accessConfigurationValues = typeof value === 'string' ? [value] : value;


After we simplify schema, we can unconditionally expect an array

lewgordon · 2021-01-25T13:45:03Z

In addition to the requested changes in the PR, I made a few changes to the documentation that I had forgot about. I removed references to using basicAuth, as it is not valid for this event type. I also removed an example that uses an accessConfiguration that is not in the schema.

medikoo

Thank you @lewgordon ! Looks great, I have just few comments

medikoo · 2021-01-25T14:08:44Z

lib/plugins/aws/package/compile/events/kafka.js

+                pattern: secretsManagerArnRegex,
+              },
+            },
+            additionalProperties: false,


This should be set in same level as properties (currently seems as an extra property definition)

medikoo · 2021-01-25T14:09:21Z

lib/plugins/aws/package/compile/events/kafka.js

+'use strict';
+class AwsCompileKafkaEvents {


Let's add a line space between 'use strict' and code (sorry we should have a lint rule for that)

medikoo · 2021-01-25T14:11:06Z

test/unit/lib/plugins/aws/package/compile/events/kafka.test.js

+      ).to.be.eventually.rejectedWith(ServerlessError);
+    };
+
+    it('should require access configurations', async () => {


As @pgrzesik pointed, let's not introduce tests that confirm that schema validation errors are thrown on invalid values

We have some sanity tests for it in other place, and otherwise schema validation engine is extensively tested in its own repository.

medikoo

Thank you @lewgordon Looks great to me!

I'll leave it for final look to @pgrzesik

lewgordon · 2021-01-25T14:44:38Z

Thank you for your help and patience @medikoo and @pgrzesik !

pgrzesik

Thank you @lewgordon, outstanding work 👍

lewgordon closed this Jan 19, 2021

lewgordon reopened this Jan 19, 2021

lewgordon marked this pull request as ready for review January 19, 2021 14:02

lewgordon force-pushed the event-kafka-smaa branch from f83af11 to b0f29fa Compare January 19, 2021 14:13

pgrzesik suggested changes Jan 20, 2021

View reviewed changes

pgrzesik requested a review from medikoo January 20, 2021 08:26

lewgordon force-pushed the event-kafka-smaa branch 4 times, most recently from 86554ce to b62028f Compare January 21, 2021 17:00

lewgordon requested a review from pgrzesik January 21, 2021 17:05

pgrzesik suggested changes Jan 21, 2021

View reviewed changes

lewgordon force-pushed the event-kafka-smaa branch from b62028f to b63dbc1 Compare January 22, 2021 17:00

lewgordon force-pushed the event-kafka-smaa branch 2 times, most recently from c819ab2 to 89c1083 Compare January 22, 2021 17:19

lewgordon requested a review from pgrzesik January 22, 2021 17:29

pgrzesik suggested changes Jan 24, 2021

View reviewed changes

medikoo suggested changes Jan 25, 2021

View reviewed changes

lewgordon force-pushed the event-kafka-smaa branch 2 times, most recently from 894d432 to e1ed35b Compare January 25, 2021 13:43

lewgordon requested review from medikoo and pgrzesik January 25, 2021 13:55

medikoo suggested changes Jan 25, 2021

View reviewed changes

lewgordon force-pushed the event-kafka-smaa branch 2 times, most recently from b059837 to 75b4af9 Compare January 25, 2021 14:22

Lew Gordon added 2 commits January 25, 2021 09:27

feat (AWS): Allow support for self managed Kafka source.

c66b713

docs: Add documentation for self-managed Apache Kafka.

5fee10e

test (AWS): Test self managed Kafka config

066a196

lewgordon force-pushed the event-kafka-smaa branch from 75b4af9 to 066a196 Compare January 25, 2021 14:27

lewgordon requested a review from medikoo January 25, 2021 14:34

medikoo assigned lewgordon Jan 25, 2021

medikoo added enhancement cat/aws-event-kafka labels Jan 25, 2021

medikoo approved these changes Jan 25, 2021

View reviewed changes

pgrzesik approved these changes Jan 25, 2021

View reviewed changes

pgrzesik changed the title ~~Event kafka smaa~~ feat(AWS Lambda): Add support for self-managed kafka event Jan 25, 2021

pgrzesik merged commit ff60501 into serverless:master Jan 25, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(AWS Lambda): Add support for self-managed `kafka` event #8784

feat(AWS Lambda): Add support for self-managed `kafka` event #8784

lewgordon commented Jan 19, 2021

codecov bot commented Jan 19, 2021 •

edited

pgrzesik left a comment

pgrzesik left a comment

pgrzesik Jan 21, 2021

lewgordon Jan 21, 2021

lewgordon Jan 22, 2021

pgrzesik Jan 21, 2021

lewgordon Jan 21, 2021

pgrzesik Jan 21, 2021

pgrzesik Jan 21, 2021

lewgordon Jan 21, 2021

pgrzesik Jan 21, 2021

lewgordon commented Jan 22, 2021

pgrzesik left a comment

pgrzesik Jan 24, 2021

pgrzesik Jan 24, 2021

pgrzesik Jan 24, 2021

medikoo left a comment

medikoo Jan 25, 2021

pgrzesik Jan 25, 2021

lewgordon Jan 25, 2021

medikoo Jan 25, 2021

medikoo Jan 25, 2021

medikoo Jan 25, 2021

medikoo Jan 25, 2021

lewgordon commented Jan 25, 2021

medikoo left a comment

medikoo Jan 25, 2021

medikoo Jan 25, 2021

medikoo Jan 25, 2021

medikoo left a comment

lewgordon commented Jan 25, 2021

pgrzesik left a comment


		chai.use(require('chai-as-promised'));

		describe('AwsCompileKafkaEvents', () => {

		'use strict';
		class AwsCompileKafkaEvents {

feat(AWS Lambda): Add support for self-managed kafka event #8784

feat(AWS Lambda): Add support for self-managed kafka event #8784

Conversation

lewgordon commented Jan 19, 2021

codecov bot commented Jan 19, 2021 • edited

Codecov Report

pgrzesik left a comment

Choose a reason for hiding this comment

pgrzesik 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

lewgordon commented Jan 22, 2021

pgrzesik 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

medikoo 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

lewgordon commented Jan 25, 2021

medikoo 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

medikoo left a comment

Choose a reason for hiding this comment

lewgordon commented Jan 25, 2021

pgrzesik left a comment

Choose a reason for hiding this comment

feat(AWS Lambda): Add support for self-managed `kafka` event #8784

feat(AWS Lambda): Add support for self-managed `kafka` event #8784

codecov bot commented Jan 19, 2021 •

edited