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

feat(bus-sqs): use aws-sdk v3, allow passing ARNs #199

Open
wants to merge 3 commits into
base: aws-sdk-3
Choose a base branch
from

Conversation

shellscape
Copy link

@shellscape shellscape commented Mar 11, 2023

This PR proposes the following:

  • Update bus-sqs to use AWS SDK v3
  • Allow passing the full queueArn instead of awsAccountId and awsRegion
  • Allow passing the full deadLetterQueueArn

This update has the advantage of producing smaller bundles, as v3 is tree-shakable, as well as automatic discovery of AWS environment and credentials data (a built-in with v3). This PR also makes this package much smaller as the latest v2 aws-sdk weighs in at a massive 77mb https://packagephobia.com/result?p=aws-sdk.

I had opened #198 to discuss, but as we're in immediate need on our end, I figured it wouldn't hurt to open a PR as well.

@adenhertog
Copy link
Contributor

hi @shellscape - thanks for the PR! just letting you know I'm reviewing this now. The proposal's really smart and I'll merge this through if there are no issues

@@ -157,7 +202,7 @@ export class SqsTransport implements Transport<SQS.Message> {
)
await Promise.allSettled(
result.Messages
.map(async message => this.makeMessageVisible(message))
.map(async (message: any) => this.makeMessageVisible(message))
Copy link
Contributor

Choose a reason for hiding this comment

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

does this need to be cast to any? It looks like the implict type is correct

@@ -224,7 +269,7 @@ export class SqsTransport implements Transport<SQS.Message> {
}
)

const serviceQueueAttributes: QueueAttributeMap = {
const serviceQueueAttributes: Record<string, string> = {
Copy link
Contributor

Choose a reason for hiding this comment

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

is it possible to align this with the target type? something like:

Suggested change
const serviceQueueAttributes: Record<string, string> = {
const serviceQueueAttributes: CreateQueueCommandInput['Attributes'] = {

@@ -265,17 +310,17 @@ export class SqsTransport implements Transport<SQS.Message> {
*/
private async assertSqsQueue (
queueName: string,
queueAttributes?: QueueAttributeMap
queueAttributes?: Record<string, string>
Copy link
Contributor

@adenhertog adenhertog Mar 20, 2023

Choose a reason for hiding this comment

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

ditto from above 🙂

Suggested change
queueAttributes?: Record<string, string>
queueAttributes?: CreateQueueCommandInput['Attributes']

@adenhertog
Copy link
Contributor

hey @shellscape the changes look great! I've left a couple of nit-picky comments, but the structure is all fine.

One small blocker is the sqs-transport.integration.ts is out of sync with these changes. If you're comfortable updating that then feel free, otherwise I'll wait to merge it back and update the tests there.

@shellscape
Copy link
Author

hey @adenhertog thanks for taking a look. yes, integration tests didn't work out of the box on a fresh clone, and docs didn't seem to address the appropriate setup for getting them to run, so in the spirit of providing code for aws-sdk v3 to the community, we opened this PR as is.

since I opened this up we've come to find out that much of the mechanisms of the default sqs bus require blanket permissions in the running context to both SQS and SNS, and assume that those permissions are available (e.g. sns:CreateTopic and sqs:CreateQueue, among others). while useful in a RabbitMQ scenario (and others) this is counter-intuitive for AWS and not the case for most business environments, as these permissions are restricted to IAM roles that have deployment capabilities. as well, SNS isn't required to interact with and send/receive messages to a queue and is only really useful in fan-out scenarios.

as a result, we've mostly ripped out the SNS functionality and the automatic creation of queues. that ends up being quite the divergence from the transports in this repo. you can view our changes along those lines here if you're curious https://github.com/Moment-Wealth/bus/tree/%40superadvisor. given the assumptions about AWS that were made in the exisiting sqs bus and the large divergence of the code in our branch, we're not going to continue work on master so please feel free to close this PR or use it as a reference for adding v3 support.

@adenhertog adenhertog changed the base branch from master to aws-sdk-3 April 30, 2023 06:34
@adenhertog
Copy link
Contributor

hey @shellscape thanks for the debrief and for raising this MR to demonstrate the aws-sdk-v3 changes. I'll still get that part merged into master as it's certainly useful. I have had to reapply the changes in a separate branch as the repo has changed a bit in the meantime.

It's been a month since you commented, but if you still remember the context I'd be keen to learn a bit more about two of your points:

much of the mechanisms of the default sqs bus require blanket permissions in the running context to both SQS and SNS

To confirm, this is saying that the IAM policies needed to bootstrap the app is different to those needed to run the app (ie: creation/subscription of queues vs publishing & deleting messages). I do agree here and the current approach is more of a short-cut.

Would something like a db-migration style approach fit your use-case here? eg: the build server can have the bootstrap IAM policy and then run a cli-command to create/subscribe queues; meaning the runtime policy can be more restrictive.

SNS isn't required to interact with and send/receive messages to a queue and is only really useful in fan-out scenarios

This approach was mainly to decouple the sender from having knowledge of the receiver. For a more general library like this I can't see much benefit to dropping the SNS support, but I'm guessing you have a more optimised use case. Keen to learn what this is if you can share :)

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

Successfully merging this pull request may close these issues.

2 participants