feat(AWS Lambda): Support for Amazon MQ RabbitMQ events #9919

liegeandlief · 2021-09-06T14:35:50Z

@pgrzesik This should add support for Amazon MQ RabbitMQ events. It is very much based upon the recently merged PR for ActiveMQ events. I have added integration tests but have been unable to actually run them as I don't have access to an AWS account with the necessary permissions. If you could take a look then that would be great. Thanks :)

codecov · 2021-09-06T14:42:06Z

Codecov Report

Merging #9919 (34571ec) into master (8401ff7) will increase coverage by 0.03%.
The diff coverage is 100.00%.

❗ Current head 34571ec differs from pull request most recent head bd82017. Consider uploading reports for the commit bd82017 to get more accurate results

@@            Coverage Diff             @@
##           master    #9919      +/-   ##
==========================================
+ Coverage   85.26%   85.30%   +0.03%     
==========================================
  Files         334      335       +1     
  Lines       13638    13675      +37     
==========================================
+ Hits        11628    11665      +37     
  Misses       2010     2010

Impacted Files	Coverage Δ
lib/plugins/aws/provider.js	`94.27% <ø> (ø)`
lib/plugins/index.js	`100.00% <ø> (ø)`
lib/utils/tokenize-exception.js	`100.00% <ø> (ø)`
scripts/serverless.js	`56.07% <ø> (ø)`
lib/Serverless.js	`69.46% <100.00%> (ø)`
lib/cli/handle-error.js	`84.84% <100.00%> (ø)`
lib/plugins/aws/lib/monitorStack.js	`96.29% <100.00%> (+0.04%)`	⬆️
lib/plugins/aws/lib/naming.js	`97.35% <100.00%> (+0.04%)`	⬆️
lib/plugins/aws/package/compile/events/rabbitmq.js	`100.00% <100.00%> (ø)`
lib/plugins/aws/package/index.js	`100.00% <100.00%> (ø)`
... and 1 more

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 8401ff7...bd82017. Read the comment docs.

liegeandlief · 2021-09-07T11:27:56Z

Closes: #9924

pgrzesik · 2021-09-09T14:59:57Z

Hello @liegeandlief - I've tried it out and after fixing few minor issues in integration tests they are failing, looks like the Lambda function cannot access rabbitmq at all. Did you have a chance to run the tests itself or did you perform similar testing to ensure it's working?

liegeandlief · 2021-09-09T15:07:21Z

Hi @pgrzesik, thanks for taking a look. Is that the Lambda function for putting a message into the queue? Unfortunately I have not been able to run the integration tests as I don't have access to an AWS account with suitable permissions to create all the necessary infrastructure. Is there a particular error you are getting?

pgrzesik · 2021-09-09T15:12:25Z

Hey @liegeandlief - yes, this it's the producer Lambda that cannot access the broker, it errors out with "errorMessage":"read ECONNRESET" - I'm guessing it might be caused by misconfigured SSL or something similar, I'm not really familiar with the amqplib

liegeandlief · 2021-09-09T15:37:40Z

@pgrzesik There was a spelling mistake in one of the variable names which may have caused this error. I've have just pushed up a fix.

pgrzesik · 2021-09-09T16:13:47Z

@pgrzesik There was a spelling mistake in one of the variable names which may have caused this error. I've have just pushed up a fix.

Yeah, that one I've spotted and fixed on my side as it was simply trying to connect to localhost, but after fixing it it's still refusing to connect :(

liegeandlief · 2021-09-10T10:29:53Z

@pgrzesik I'm just trying to build the infrastructure stack so I can take a look at this test error. I have fixed the spelling mistake problem and also corrected the RabbitMQ instance type to mq.t3.micro. But the AWS::MSK::Cluster seems to be stuck in the CREATE_IN_PROGRESS state. Did you encounter this or have to make any other changes to get the dependency stack running?

pgrzesik · 2021-09-10T10:36:03Z

Hey @liegeandlief, the MSK Cluster takes really long to provision, but I would encourage you to just simplify the script to not provision ActiveMQ-related, EFS-related and MSK-related resources - just stuff that is needed for RabbitMQ - it took 10-15 minutes for me to provision such simplified stack (I simply commented a bunch of stuff in the template and script that provisions the infra).

liegeandlief · 2021-09-13T10:38:32Z

@pgrzesik The RabbitMQ integration test should now be working (at least i worked for me!)

pgrzesik · 2021-09-13T15:14:49Z

Thanks @liegeandlief - I see that it was about SSL in the end :D I've tried to test, but I see the integration test failing for me - there are not messages to be consumed by the consumer function that include the expected message. Is that test green for you?

liegeandlief · 2021-09-13T16:14:13Z

@pgrzesik It's weird that it isn't working for you. It is green for me. It is quite slow as it seems to take quite a long time to create the Lambda functions in the VPC.

pgrzesik · 2021-09-14T11:14:25Z

@liegeandlief That is surprising - for me it always fail with the following output

Do you have any local changes that are not pushed here that might be affecting it?

liegeandlief · 2021-09-14T12:24:34Z

@pgrzesik No local changes, everything is pushed

liegeandlief · 2021-09-14T12:32:11Z

@pgrzesik When the test is running can you see the consumer function log group in CloudWatch and are you able to inspect what gets logged?

liegeandlief · 2021-09-14T21:00:08Z

@pgrzesik a colleague and I have both managed to run the test successfully. We both used brand new AWS accounts when doing so and all the resources were created in the us-east-1 region. The AWS user credentials had full permissions on each account. When creating the integration test dependency stack we didn't comment anything out, but allowed it to create all infrastructure even though only some of it was required for the test. I don't know if any of this context is helpful in determining why the test is failing for you?

pgrzesik · 2021-09-15T18:31:58Z

Hello @liegeandlief - I was trying it out once again today multiple times, with the exact same environment and the tests are not succeeding for me. Unfortunately, I won't be available for the next two weeks - I'm handing off the tasks to @medikoo, who based on availability might be able to help.

pgrzesik · 2021-10-07T11:27:44Z

Hello @liegeandlief 👋 Sorry for the delay but as I've mentioned I've been off for two weeks. I'll be checking your branch and tests out once again on a fully fresh environment to see if I can get these tests to pass on my side. Thanks for you patience on this one 🙇

pgrzesik · 2021-10-14T08:45:39Z

Hello @liegeandlief - I've tried multiple different approaches and I cannot get this test to work - it does not pass and errors out without finding the expected item in the logs - is there anything else different with your setup when you're running this test by any chance?

Additionally, could you rebase your PR on top of current master whenever you have a chance?

Update - I've managed to finally resolve the issue on my side - it turns out the error was with my setup, where the current logic for setting up infrastructure does not update the passwords in Secrets Manager and I ran it the first time by mistake and then it wasn't using the updated password - all is good now, I'll review the PR and I imagine we should be good to go soon.

pgrzesik

Thank you @liegeandlief - it looks great in general. I only have small suggestions related to async/await and changes to ActiveMQ. I understand the reasoning behind "cleanup" for activemq, but I would keep it out of this PR to make it focused on one thing only and submit the cleanup part separately if you'd like. What do you think?

Thanks a lot for your patience on this one 🙇

pgrzesik · 2021-10-14T12:14:51Z

test/fixtures/programmatic/functionRabbitmq/core.js

+}
+
+async function producer() {
+  await new Promise((resolve) => {


Is there a reason why it needs to be wrapped in a Promise? Can we just use simpler async/await across the whole function?

I tried using asyc/await and it caused the test to fail. I think it may have something to do with the fact that amqplib isn't using native promises but is using Bluebird instead.

How was it failing? I don't think there should be an issue with await-ing Bluebird promises as we're doing that in our codebase in a lot of places

I can't quite remember now, but I think it either wouldn't create the queue or wouldn't send the message to the queue. I fiddled about with it for ages with no success and then changed it to not use async/await which worked first time.

I've tried changing it and went with:

const connectOptions = { protocol: 'amqps', hostname: process.env.RABBITMQ_HOST, port: 5671, username: process.env.RABBITMQ_USERNAME, password: process.env.RABBITMQ_PASSWORD, }; const connection = await amqp.connect(connectOptions); const channel = await connection.createChannel(); const queueName = process.env.QUEUE_NAME; await channel.assertQueue(queueName); await channel.sendToQueue(queueName, Buffer.from('Hello from RabbitMQ Integration test!')); return { statusCode: 200, };

and it looks like it works just fine. Could you change the implementation to async/await-based one? I think it's the last thing before the merge 🙌

That's now been updated to use async/await

pgrzesik · 2021-10-14T12:15:46Z

test/integration/aws/infra-dependent/activemq.test.js

@@ -30,9 +30,9 @@ describe('AWS - Active MQ Integration Test', function () {

    const outputMap = await getDependencyStackOutputMap();

-    log.notice('Getting Active MQ Credentials ARN');
+    log.notice('Getting ActiveMQ Credentials ARN');


I would leave out the adjustments to ActiveMQ tests, even though the change might be valid to keep this PR focused on RabbitMQ

I've removed those changes to the ActiveMQ tests

liegeandlief · 2021-10-15T12:38:39Z

@pgrzesik I've brought my fork up to date with the upstream master branch

pgrzesik

Looks good overall, I just have a few minor things and we're good to ship it

pgrzesik · 2021-10-15T14:57:13Z

test/fixtures/programmatic/functionRabbitmq/core.js

+}
+
+async function producer() {
+  await new Promise((resolve) => {


How was it failing? I don't think there should be an issue with await-ing Bluebird promises as we're doing that in our codebase in a lot of places

pgrzesik · 2021-10-15T14:57:44Z

test/utils/cloudformation.js

@@ -4,7 +4,9 @@ const awsRequest = require('@serverless/test/aws-request');

 const SHARED_INFRA_TESTS_CLOUDFORMATION_STACK = 'integration-tests-deps-stack';
 const SHARED_INFRA_TESTS_ACTIVE_MQ_CREDENTIALS_NAME =
-  'integration-tests-active-mq-broker-credentials';
+  'integration-tests-activemq-broker-credentials';


let's keep the old value for this const

pgrzesik · 2021-10-15T14:58:31Z

scripts/test/integration-setup/index.js

@@ -130,17 +170,33 @@ async function handleInfrastructureUpdate() {
 (async () => {
  log.notice('Starting setup of integration infrastructure...');

-  if (!process.env.SLS_INTEGRATION_TESTS_ACTIVE_MQ_USER) {
+  if (!process.env.SLS_INTEGRATION_TESTS_ACTIVEMQ_USER) {


let's keep these env vars as they were - let's limit changes to existing logic in this PR

pgrzesik · 2021-10-19T12:24:30Z

test/fixtures/programmatic/functionRabbitmq/core.js

+}
+
+async function producer() {
+  await new Promise((resolve) => {


I've tried changing it and went with:

const connectOptions = { protocol: 'amqps', hostname: process.env.RABBITMQ_HOST, port: 5671, username: process.env.RABBITMQ_USERNAME, password: process.env.RABBITMQ_PASSWORD, }; const connection = await amqp.connect(connectOptions); const channel = await connection.createChannel(); const queueName = process.env.QUEUE_NAME; await channel.assertQueue(queueName); await channel.sendToQueue(queueName, Buffer.from('Hello from RabbitMQ Integration test!')); return { statusCode: 200, };

and it looks like it works just fine. Could you change the implementation to async/await-based one? I think it's the last thing before the merge 🙌

pgrzesik

awesome job 👏

liegeandlief · 2021-10-19T15:03:42Z

@pgrzesik Thanks for all of your help on this! When is it likely to be included in a release?

pgrzesik · 2021-10-19T15:10:03Z

If nothing unexpected pops up, I was planning to prepare a release tomorrow morning CEST time

Support for Amazon MQ Rabbit MQ queues

a3a14ab

liegeandlief changed the title ~~Support for Amazon MQ Rabbit MQ queues~~ Support for Amazon MQ Rabbit MQ events Sep 6, 2021

liegeandlief changed the title ~~Support for Amazon MQ Rabbit MQ events~~ Support for Amazon MQ RabbitMQ events Sep 6, 2021

Michael added 3 commits September 6, 2021 16:41

Corrected titles in readme files

cdae695

Consistency in naming of ActiveMQ

dbd2ea6

Consistency in naming of ActiveMQ

57a123b

liegeandlief mentioned this pull request Sep 7, 2021

Add support for Amazon MQ RabbitMQ event source #9924

Closed

pgrzesik assigned liegeandlief Sep 8, 2021

pgrzesik added the enhancement label Sep 8, 2021

Integration test fixes

fed0352

Fixed integration test

dba1b63

Merge branch 'serverless:master' into rabbitmq

493609a

pgrzesik suggested changes Oct 14, 2021

View reviewed changes

Merge branch 'serverless:master' into rabbitmq

5349344

Michael added 3 commits October 15, 2021 13:45

Removed changes to activemq tests

5cb25a9

Removed changes to activemq tests

4ec8e6b

Removed changes to activemq tests

94016da

pgrzesik suggested changes Oct 15, 2021

View reviewed changes

Removed ActiveMQ renaming changes, fixed unit tests

ecd16bd

pgrzesik suggested changes Oct 19, 2021

View reviewed changes

Use async/await in test producer function

bd82017

pgrzesik approved these changes Oct 19, 2021

View reviewed changes

pgrzesik changed the title ~~Support for Amazon MQ RabbitMQ events~~ feat(AWS Lambda): Support for Amazon MQ RabbitMQ events Oct 19, 2021

pgrzesik merged commit a3edecf into serverless:master Oct 19, 2021

liegeandlief deleted the rabbitmq branch October 19, 2021 13:57

liegeandlief mentioned this pull request Oct 21, 2021

Added types for RabbitMQ and ActiveMQ events DefinitelyTyped/DefinitelyTyped#56673

Merged

8 tasks

feat(AWS Lambda): Support for Amazon MQ RabbitMQ events #9919

feat(AWS Lambda): Support for Amazon MQ RabbitMQ events #9919

Conversation

liegeandlief commented Sep 6, 2021 • edited

codecov bot commented Sep 6, 2021 • edited

Codecov Report

liegeandlief commented Sep 7, 2021

pgrzesik commented Sep 9, 2021

liegeandlief commented Sep 9, 2021

pgrzesik commented Sep 9, 2021

liegeandlief commented Sep 9, 2021

pgrzesik commented Sep 9, 2021

liegeandlief commented Sep 10, 2021

pgrzesik commented Sep 10, 2021

liegeandlief commented Sep 13, 2021

pgrzesik commented Sep 13, 2021

liegeandlief commented Sep 13, 2021 • edited

pgrzesik commented Sep 14, 2021 • edited

liegeandlief commented Sep 14, 2021

liegeandlief commented Sep 14, 2021

liegeandlief commented Sep 14, 2021

pgrzesik commented Sep 15, 2021

pgrzesik commented Oct 7, 2021

pgrzesik commented Oct 14, 2021 • edited

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

liegeandlief commented Oct 15, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgrzesik left a comment

Choose a reason for hiding this comment

liegeandlief commented Oct 19, 2021

pgrzesik commented Oct 19, 2021

liegeandlief commented Sep 6, 2021 •

edited

codecov bot commented Sep 6, 2021 •

edited

liegeandlief commented Sep 13, 2021 •

edited

pgrzesik commented Sep 14, 2021 •

edited

pgrzesik commented Oct 14, 2021 •

edited