10000 ESM: fix CreateESM SQS validation by bentsku · Pull Request #12338 · localstack/localstack · GitHub
[go: up one dir, main page]
Skip to content

ESM: fix CreateESM SQS validation #12338

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Mar 4, 2025
Merged

ESM: fix CreateESM SQS validation #12338

merged 1 commit into from
Mar 4, 2025

Conversation

bentsku
Copy link
Contributor
@bentsku bentsku commented Mar 4, 2025 •
edited
Loading

Motivation

Follow up from #12297, we did not pass the QueueUrl to GetQueueAttributes but rather the QueueArn. This actually still worked somehow because SQS took it as the QueueName and was able to resolve it.
However, when working with cross-account, this did not work because we were not able to resolve to the right account id.

I thought of doing an internal connect_to().sqs.get_queue_url(QueueName=name, QueueOwnerAwsAccountId=queue_account), to get the proper QueueUrl, but if the queue did not exist, it would have raised without triggering an IAM check for GetQueueAttributes, so the IAM parity would not be there... so I resolved to build a dummy value instead, as it is not very important and we only need SQS to be able to parse it.

Upstream run: ✅ 13658265278

Changes

  • build a dummy queue url from the queue ARN to pass to GetQueueAttributes.

@bentsku bentsku added aws:lambda AWS Lambda semver: patch Non-breaking changes which can be included in patch releases labels Mar 4, 2025
@bentsku bentsku added this to the 4.3 milestone Mar 4, 2025
@bentsku bentsku self-assigned this Mar 4, 2025
@github-actions GitHub Actions
Copy link
github-actions bot commented Mar 4, 2025

LocalStack Community integration with Pro

    2 files  ±  0      2 suites  ±0   1h 31m 2s ⏱️ - 20m 22s
3 115 tests  - 994  2 895 ✅  - 881  220 💤  - 113  0 ❌ ±0 
3 117 runs   - 994  2 895 ✅  - 881  222 💤  - 113  0 ❌ ±0 

Results for commit 39a7733. ± Comparison against base commit f48e6bd.

This pull request removes 994 tests. 
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_lambda_dynamodb
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_opensearch_crud
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_search_books
tests.aws.scenario.bookstore.test_bookstore.TestBookstoreApplication ‑ test_setup
tests.aws.scenario.kinesis_firehose.test_kinesis_firehose.TestKinesisFirehoseScenario ‑ test_kinesis_firehose_s3
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_destination_sns
tests.aws.scenario.lambda_destination.test_lambda_destination_scenario.TestLambdaDestinationScenario ‑ test_infra
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_prefill_dynamodb_table
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input0-SUCCEEDED]
tests.aws.scenario.loan_broker.test_loan_broker.TestLoanBrokerScenario ‑ test_stepfunctions_input_recipient_list[step_function_input1-SUCCEEDED]
…

joe4dev
joe4dev approved these changes Mar 4, 2025
View reviewed changes
Copy link
Member
@joe4dev joe4dev 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 for coming up with this innovative workaround @bentsku 🧠

what a coincidenc that it worked in the first place 😅

localstack-core/localstack/services/lambda_/provider.py
# which is not given directly. We build out a dummy `QueueUrl` which can be parsed by SQS to return
# the right value
queue_name = arn["resource"].split("/")[-1]
queue_url = f"http://sqs.{arn['region']}.domain/{arn['account']}/{queue_name}"
Copy link
Member

Choose a reason for hiding this comment

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

Interesting hack. I wonder how AWS does it internally because GetQueueUrl is not a required IAM permission: https://repost.aws/knowledge-center/lambda-sqs-event-source

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, I really wonder too.. maybe just rebuilding the queue manually like us, but it seems… weird 😅 I don’t know if there would be any other operation that sqs:getQueueAttributes permission would give access to. Thanks for the review!

@joe4dev joe4dev merged commit 4007b3f into master Mar 4, 2025
44 checks passed
@joe4dev joe4dev deleted the fix-esm-op branch March 4, 2025 20:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joe4dev joe4dev joe4dev approved these changes

@dominikschubert dominikschubert Awaiting requested review from dominikschubert dominikschubert is a code owner

@dfangl dfangl Awaiting requested review from dfangl dfangl is a code owner

@gregfurman gregfurman Awaiting requested review from gregfurman gregfurman is a code owner

Assignees

@bentsku bentsku

Labels
aws:lambda AWS Lambda semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
4.3
Development

Successfully merging this pull request may close these issues.
2 participants
@bentsku @joe4dev
0