feat: Add support for SQS by JoelAtDeluxe · Pull Request #11 · Originate/terraform-modules

JoelAtDeluxe · 2021-11-09T00:47:21Z

This PR adds support for SQS deployments. This tries to be opinionated in the following ways:

Defaults to long polling (10 seconds)
Sets the visibility timeout to 30 seconds (which is the normal AWS default, and seems sensible for most usecases)
Tries to enforce a shorthand queue name
Prefers fifo queues that use content-based deduplication

This also provides the generated name of the queue as an output, which will typically be needed by any deployment.

chrizkim · 2021-11-09T01:01:36Z

aws/sqs/main.tf

+  # This helps avoid queue names ending in "-" or "-.fifo"
+  given_queue_name = var.queue_name == "" ? "" : "-${var.queue_name}"
+  # All fifo queues must end in .fifo, per AWS rules
+  queue_suffix    = var.is_fifo == true ? ".fifo" : ""


var.is_fifo ? ".fifo" : ""

Oh, we have the == is true in a couple of other places, I think? I thought I just needed it. I'll fix that.

If you find any others, feel free to lump in a cleanup of 'em 😁

chrizkim · 2021-11-09T01:02:31Z

aws/sqs/main.tf

+  policy = jsonencode(
+    {
+      Version : "2008-10-17"
+      Id : "__default_policy_ID"


this line seems unnecessary

chrizkim · 2021-11-09T01:02:42Z

aws/sqs/main.tf

+      Id : "__default_policy_ID"
+      Statement : [
+        {
+          Sid : "__owner_statement"


this line also seems unnecessary

chrizkim · 2021-11-09T01:06:10Z

aws/sqs/main.tf

+  visibility_timeout_seconds  = var.visibility_timeout_seconds
+}
+
+resource "aws_sqs_queue_policy" "this" {


On a more general note, we probably shouldn't have a default policy, especially one so permissive (this one makes any action on the queue open to anyone in the world i believe). At best, we could have a policy statement variable that can be passed in. For most cases, permission to the queue should be granted through an IAM role rather than a queue policy

Good call. I started this a month ago based on what we had in perfected. I had to make a number of minor tweaks, so I'm not surprised the policy isn't perfect. I think cognito does a policy passing, or something close to that. i'll review and see if I can follow the same thing.

chrizkim · 2021-11-09T01:07:09Z

aws/sqs/outputs.tf

+  value = aws_sqs_queue.this.arn
+}
+
+output "full_queue_name" {


nit, i think calling it name is good enough

Fair. I thought it was a bit weird that you'd have something like:

module "sqs" { queue_name = "blah" }

and then later:

queue_name = module.sqs.name

and those not being the same thing.

chrizkim · 2021-11-09T01:07:46Z

aws/sqs/terraform.tf

@@ -0,0 +1,3 @@
+terraform {
+  experiments = [module_variable_optional_attrs]
+}


this file doesn't seem necessary (I don't see any use of the optional type

I was using defaults earlier. I'll remove that.

chrizkim · 2021-11-09T01:08:42Z

aws/sqs/variables.tf

+  type        = string
+}
+
+variable "queue_name" {


nit, i've been calling these identifier to be less ambiguous, though your description does do a decent job clarifying, so.. 🤷‍♂️

chrizkim · 2021-11-09T01:09:42Z

aws/sqs/variables.tf

+variable "receive_wait_time_seconds" {
+  description = "The time to wait when polling for new messages. Use 0 for immediate response. Longer values are preferred. AWS recommends a maximum of 20 seconds."
+  type        = number
+  default     = 5


why shorten this from 20?

I shortened it because 20 is the maximum, and I think the configuration we were looking at in perfected was 10 seconds. Thinking about this a bit more, and doing a bit more reading, I think it makes sense to set it to a large value, and have those that need to adjust downward.

chrizkim · 2021-11-09T01:14:16Z

aws/sqs/main.tf

+resource "aws_sqs_queue" "this" {
+  name                        = local.full_queue_name
+  fifo_queue                  = var.is_fifo
+  content_based_deduplication = var.content_based_deduplication


Maybe we should enforce that this is false for non-fifo queues (with a note in the variable description)?

content_based_deduplication = var.is_fifo && var.content_based_deduplication

Ah, I was trying to do something like that, while still allowing for a fifo: true / dedupe: false, and I was having problems doing that. && should do it though.

add support for sqs

f35139b

JoelAtDeluxe requested review from chrizkim and hallettj November 9, 2021 00:47

chrizkim reviewed Nov 9, 2021

View reviewed changes

Joel Smith added 7 commits November 8, 2021 19:49

remove unused terraform experiment file

73ed2e8

Set default receive_wait to maximum val

0cbd6c5

siimplify output name

ac6afb2

only set content_dedupe if using fifo

858646d

use standard variable name for "name"

c123c3f

add the url to the output

9ee4dd6

remove security policy

884bc6f

Conversation

JoelAtDeluxe commented Nov 9, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants