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: Support maximum concurrency of Lambda with SQS as an event source #402

Merged
merged 4 commits into from
Apr 3, 2023

Conversation

davidegiunchi
Copy link
Contributor

Description

Aws recently introduced maximum concurrency of lambda functions when using sqs as an event source ( https://aws.amazon.com/it/blogs/compute/introducing-maximum-concurrency-of-aws-lambda-functions-when-using-amazon-sqs-as-an-event-source/ ), i've added the support of this feature to this module.
This is my first contribution to this repo, please check the code since i might be wrong.

Motivation and Context

Limiting lambda concurrency with SQS without setting reserved concurrency.

Breaking Changes

If you want to set maximum_concurrency_sqs_scaling you need the aws provider >= 4.51.0, because the support for scaling_config has been introduced in 4.51.0: https://github.com/hashicorp/terraform-provider-aws/blob/main/CHANGELOG.md#4510-january-19-2023

How Has This Been Tested?

  • I have updated at least one of the examples/* to demonstrate and validate my change(s)
  • I have tested and validated these changes using one or more of the provided examples/* projects
  • I have executed pre-commit run -a on my pull request

@bryantbiggs bryantbiggs changed the title support maximum concurrency of lambda with sqs as event_source feat: Support maximum concurrency of Lambda with SQS as an event source Jan 22, 2023
main.tf Outdated
@@ -280,6 +280,14 @@ resource "aws_lambda_event_source_mapping" "this" {
}
}

dynamic "scaling_config" {
for_each = try(each.value.maximum_concurrency_sqs_scaling, null) != null ? [true] : []
Copy link
Member

@bryantbiggs bryantbiggs Jan 22, 2023

Choose a reason for hiding this comment

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

Suggested change
for_each = try(each.value.maximum_concurrency_sqs_scaling, null) != null ? [true] : []
for_each = try([each.value.scaling_config], [])

Copy link
Member

Choose a reason for hiding this comment

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

resolved in 748c6c2

main.tf Outdated
dynamic "scaling_config" {
for_each = try(each.value.maximum_concurrency_sqs_scaling, null) != null ? [true] : []
content {
maximum_concurrency = maximum_concurrency_sqs_scaling.value
Copy link
Member

@bryantbiggs bryantbiggs Jan 22, 2023

Choose a reason for hiding this comment

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

Suggested change
maximum_concurrency = maximum_concurrency_sqs_scaling.value
maximum_concurrency = try(scaling_config.value.maximum_concurrency, null)

Copy link
Member

Choose a reason for hiding this comment

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

resolved in 748c6c2

function_response_types = ["ReportBatchItemFailures"]
event_source_arn = aws_sqs_queue.this.arn
function_response_types = ["ReportBatchItemFailures"]
maximum_concurrency_sqs_scaling = "2"
Copy link
Member

Choose a reason for hiding this comment

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

this would become

      scaling_config = {
        maximum_concurrency = 2
      }

Copy link
Member

Choose a reason for hiding this comment

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

resolved in 748c6c2

@github-actions
Copy link

This PR has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this PR will be closed in 10 days

@github-actions github-actions bot added the stale label Feb 22, 2023
@davidegiunchi
Copy link
Contributor Author

not stale

@github-actions github-actions bot removed the stale label Feb 23, 2023
@DStranger
Copy link

@davidegiunchi this is a welcome change! Curious, what is the status of this PR?

@davidegiunchi
Copy link
Contributor Author

@DStranger the PR ha already been checked and corrected by Bryant, we are waiting for the PR to be merged (i think by Anton)

@jomast
Copy link

jomast commented Mar 30, 2023

are we simply waiting for Bryant's suggestions to be implemented?

@jomast
Copy link

jomast commented Mar 30, 2023

is this feature already implemented? https://registry.terraform.io/providers/hashicorp/aws/latest/docs/resources/lambda_event_source_mapping#maximum_concurrency

@antonbabenko
Copy link
Member

@davidegiunchi Please fix this PR as Bryant suggested, and I will be able to merge it.

@jonathangueedes
Copy link

@davidegiunchi help us with this please.

@bryantbiggs
Copy link
Member

let me see if I can push changes - if I can, I can probably get it updated and released - one sec

…g and remove warning on deprecated EC2 platform attribute
@@ -2,7 +2,6 @@ provider "aws" {
region = "eu-west-1"

# Make it faster by skipping something
skip_get_ec2_platforms = true
Copy link
Member

Choose a reason for hiding this comment

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

removed due to
image

@@ -20,12 +27,15 @@ module "lambda_function" {
handler = "index.lambda_handler"
runtime = "python3.8"

source_path = "${path.module}/../fixtures/python3.8-app1"
source_path = "${path.module}/../fixtures/python3.8-app1/index.py"
Copy link
Member

Choose a reason for hiding this comment

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

this was throwing an error around a specific version of python being required. Changing to point directly to the individual file fixed that. I don't believe that affects the intent of this example

data "aws_vpc" "default" {
default = true
}
module "vpc" {
Copy link
Member

Choose a reason for hiding this comment

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

I don't have any default VPCs, so added a bare bones one to make it work for the RabbitMQ requirements

}

resource "aws_mq_broker" "this" {
broker_name = random_pet.this.id
engine_type = "RabbitMQ"
engine_version = "3.8.11"
Copy link
Member

Choose a reason for hiding this comment

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

this is almost at EOS so updated to latest recommended by AWS

Copy link
Member

@bryantbiggs bryantbiggs left a comment

Choose a reason for hiding this comment

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

thank you!

@bryantbiggs bryantbiggs merged commit 268975c into terraform-aws-modules:master Apr 3, 2023
antonbabenko pushed a commit that referenced this pull request Apr 3, 2023
## [4.13.0](v4.12.1...v4.13.0) (2023-04-03)

### Features

* Support maximum concurrency of Lambda with SQS as an event source ([#402](#402)) ([268975c](268975c))
@antonbabenko
Copy link
Member

This PR is included in version 4.13.0 🎉

@github-actions
Copy link

github-actions bot commented May 4, 2023

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants