-
Notifications
You must be signed in to change notification settings - Fork 2
feat: Add support for SQS #11
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
base: main
Are you sure you want to change the base?
Changes from 1 commit
f35139b
73ed2e8
0cbd6c5
ac6afb2
858646d
c123c3f
9ee4dd6
884bc6f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| locals { | ||
| # 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" : "" | ||
| full_queue_name = "${var.stack}-${var.env}${local.given_queue_name}${local.queue_suffix}" | ||
| } | ||
|
|
||
| resource "aws_sqs_queue" "this" { | ||
| name = local.full_queue_name | ||
| fifo_queue = var.is_fifo | ||
| content_based_deduplication = var.content_based_deduplication | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe we should enforce that this is
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| receive_wait_time_seconds = var.receive_wait_time_seconds | ||
| visibility_timeout_seconds = var.visibility_timeout_seconds | ||
| } | ||
|
|
||
| resource "aws_sqs_queue_policy" "this" { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| queue_url = aws_sqs_queue.this.id | ||
|
|
||
| policy = jsonencode( | ||
| { | ||
| Version : "2008-10-17" | ||
| Id : "__default_policy_ID" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this line seems unnecessary |
||
| Statement : [ | ||
| { | ||
| Sid : "__owner_statement" | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this line also seems unnecessary |
||
| Effect : "Allow" | ||
| Principal : "*" | ||
| Action : "sqs:*" | ||
| Resource : "${aws_sqs_queue.this.arn}" | ||
| } | ||
| ] | ||
| } | ||
| ) | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| output "arn" { | ||
| value = aws_sqs_queue.this.arn | ||
| } | ||
|
|
||
| output "full_queue_name" { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit, i think calling it
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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.nameand those not being the same thing. |
||
| value = aws_sqs_queue.this.name | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,3 @@ | ||
| terraform { | ||
| experiments = [module_variable_optional_attrs] | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this file doesn't seem necessary (I don't see any use of the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was using |
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| variable "stack" { | ||
| description = "The name of the stack" | ||
| type = string | ||
| } | ||
|
|
||
| variable "env" { | ||
| description = "The name of the environment" | ||
| type = string | ||
| } | ||
|
|
||
| variable "queue_name" { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit, i've been calling these |
||
| description = "The shorthand name of the queue. The full queue name can be retrieved as an output. Note that an empty string is still a valid queue name." | ||
| type = string | ||
| } | ||
|
|
||
| variable "visibility_timeout_seconds" { | ||
| description = "The amount of time allowed to the processor to process a message before it is declared failed. Defaults to 30 seconds." | ||
| type = number | ||
| default = 30 | ||
| } | ||
|
|
||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why shorten this from 20?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| } | ||
|
|
||
| variable "is_fifo" { | ||
| description = "Specifies if this queue should be a FIFO queue, which would preserve message ordering. Defaults to true." | ||
| type = bool | ||
| default = true | ||
| } | ||
|
|
||
| variable "content_based_deduplication" { | ||
| description = "Specifies if this queue should use content-based deduplication. Must be false if using a standard (non-fifo) queue. Defaults to true" | ||
| type = bool | ||
| default = true | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| terraform { | ||
| required_version = ">= 1.0" | ||
|
|
||
| required_providers { | ||
| aws = { | ||
| source = "hashicorp/aws" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
var.is_fifo ? ".fifo" : ""There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, we have the
== is truein a couple of other places, I think? I thought I just needed it. I'll fix that.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you find any others, feel free to lump in a cleanup of 'em 😁