mapbox / cloudfriend

Helper functions for assembling CloudFormation templates in JavaScript
ISC License
69 stars 9 forks source link

Modify Queue shortcut SNS behavior #125

Open drboyer opened 3 years ago

drboyer commented 3 years ago

Currently, the Queue shortcut will also create a SNS topic that the created queue is subscribed to, unless one of two conditions are met:

  1. The queue is a FIFO queue
  2. An ExistingTopicArn property is specified, in which case the queue will be subscribed to that topic instead

There are two things that could be improved about this behavior:

  1. In some cases you may not want to send messages via an intermediate SNS topic - it may be fine to just directly send messages to SQS. In that case, it might be nice to skip creation of the SNS topic. This is mostly to prevent unnecessary infrastructure from being created - there's really no cost benefit to this, as you're only charged for message processing with SNS.
  2. With the recent addition of SNS FIFO topics, you now can subscribe a FIFO queue to an SNS topic, as long as it's a FIFO topic. I'm already aware of one internal use case where this'd be a nice feature to support.

What I'd propose is adding a new property CreateSnsTopic with a default value of true (to prevent a breaking change). If it was set to false, we'd skip creation of the SNS topic. Then instead of ignoring TopicName or ExistingTopicArn if FifoQueue: true, we should accept an existing topic ARN (and let CloudFormation validate that it's a FIFO topic), or create a FIFO Topic if it's not and CreateSnsTopic is not false.

Priority

Not super high, as there are workarounds to both of these, and as I said, even if you don't use the Topic that's created, it doesn't really cost you anything.

rclark commented 3 years ago

ignoring TopicName or ExistingTopicArn if FifoQueue: true

We'd be trading this off for

ignoring TopicName or ExistingTopicArn if CreateSnsTopic: true

Right? I wonder if what we really need is a breaking change.

drboyer commented 3 years ago

A breaking changes might make the interface a bit clearer, yeah. So if you're willing to allow that, we can go that route. In that case, would it make sense to change the behavior to:

rclark commented 3 years ago

If QueueName is provided, create a new queue and subscribe the topic to it (create FIFO topic if a FIFO queue)

Meant TopicName I think? If so, I think this sounds right.

drboyer commented 3 years ago

Found another reason this behavior is somewhat problematic today - the Queue will automatically create a Queue Policy that grants the SNS topic access to send events to the queue. Unfortunately, there's no way to modify that autogenerated policy, which can cause challenges if you need to, such as if you're trying to subscribe EventBridge events to an SQS queue. Again there's a few different ways we could fix, but just eliminating the SNS-related generated code in some cases would probably be sufficient to solve.