Skip to content

Conversation

@thrau
Copy link
Member

@thrau thrau commented Oct 22, 2024

Motivation

There was a long-standing TODO for the sqs developer endpoints /_aws/sqs/messages to also support the modern x-amz-json-1.0 protocol introduced in #8268. This PR adds this functionality.

The implementation is not pretty but the best I could come up with without reworking the entire serialization mechanism. Basically through the use of the @aws_response_serializer decorator the protocol used to serialize the response was hard-coded, but needed to be dynamic per-request.

It's still a bit awkward because we documented that when calling this endpoint with application/json content type, it will return a JSON serialization of the query protocol response. So basically we have three protocols now: query (XML), x-amz-json-1.0 (for modern clients), and our weird/wrong json protocol that just returns the XML responses as json. That needs to be addressed at some point but is technically a breaking change.

Changes

  • When making AWS SQS client requests in x-amz-json-1.0 format to the endpoint localhost:4566/_aws/sqs/messages , the endpoint now correctly returns the JSON response.

TODO

What's left to do:

  • Update OpenAPI spec to contain the new input and output shape for x-amz-json-1.0

@thrau thrau changed the title Sqs dev endpoints json add AWS json protocol support for SQS developer endpoints Oct 22, 2024
@thrau thrau added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label Oct 22, 2024
@github-actions
Copy link

github-actions bot commented Oct 22, 2024

LocalStack Community integration with Pro

    2 files  ±    0      2 suites  ±0   1h 5m 31s ⏱️ - 36m 45s
2 490 tests  - 1 019  2 267 ✅  - 829  223 💤  - 190  0 ❌ ±0 
2 492 runs   - 1 019  2 267 ✅  - 829  225 💤  - 190  0 ❌ ±0 

Results for commit 131770e. ± Comparison against base commit 56e0b77.

This pull request removes 1037 and adds 18 tests. Note that renamed tests count towards both.
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]
…
tests.aws.services.sqs.test_sqs_backdoor.TestSqsDeveloperEndpoints ‑ test_fifo_list_messages_as_botocore_endpoint_url[json-domain]
tests.aws.services.sqs.test_sqs_backdoor.TestSqsDeveloperEndpoints ‑ test_fifo_list_messages_as_botocore_endpoint_url[json-path]
tests.aws.services.sqs.test_sqs_backdoor.TestSqsDeveloperEndpoints ‑ test_fifo_list_messages_as_botocore_endpoint_url[json-standard]
tests.aws.services.sqs.test_sqs_backdoor.TestSqsDeveloperEndpoints ‑ test_fifo_list_messages_as_botocore_endpoint_url[query-domain]
tests.aws.services.sqs.test_sqs_backdoor.TestSqsDeveloperEndpoints ‑ test_fifo_list_messages_as_botocore_endpoint_url[query-path]
tests.aws.services.sqs.test_sqs_backdoor.TestSqsDeveloperEndpoints ‑ test_fifo_list_messages_as_botocore_endpoint_url[query-standard]
tests.aws.services.sqs.test_sqs_backdoor.TestSqsDeveloperEndpoints ‑ test_list_messages_as_botocore_endpoint_url[json-domain]
tests.aws.services.sqs.test_sqs_backdoor.TestSqsDeveloperEndpoints ‑ test_list_messages_as_botocore_endpoint_url[json-path]
tests.aws.services.sqs.test_sqs_backdoor.TestSqsDeveloperEndpoints ‑ test_list_messages_as_botocore_endpoint_url[json-standard]
tests.aws.services.sqs.test_sqs_backdoor.TestSqsDeveloperEndpoints ‑ test_list_messages_as_botocore_endpoint_url[query-domain]
…

♻️ This comment has been updated with latest results.

Copy link
Contributor

@bentsku bentsku left a comment

Choose a reason for hiding this comment

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

I'll only comment as I've updated the PR a bit, let me know what you think @thrau! I've tried to remove the logic of the protocol outside of the method to avoid having nested functions, we still need it for the parser though. Happy to get feedback!

@@ -652,20 +690,14 @@ def list_messages(self, request: Request) -> ReceiveMessageResult:
return self._get_and_serialize_messages(request, region, account_id, queue_name)

@route("/_aws/sqs/messages/<region>/<account_id>/<queue_name>")
Copy link
Contributor

Choose a reason for hiding this comment

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

q: not sure if it's normal that this method accept all HTTP methods?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we make this the same as the /_aws/sqs/messages route?

Suggested change
@route("/_aws/sqs/messages/<region>/<account_id>/<queue_name>")
@route("/_aws/sqs/messages/<region>/<account_id>/<queue_name>", methods=["GET", "POST"])

# only parse the request using a parser if it comes from an AWS client
protocol = get_sqs_protocol(request)
operation, service_request = create_parser(
load_service("sqs", protocol=protocol)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: I've updated it, as we're moving away from the sqs-query service name to the "service name" + "protocol" way of creating/selecting serializer and parser.

return "json" if content_type == "application/x-amz-json-1.0" else "query"


def sqs_auto_protocol_aws_response_serializer(service_name: str, operation: str):
Copy link
Contributor

Choose a reason for hiding this comment

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

this is a bit verbose and copies the logic of aws_response_serializer to get the request out of it, I'd like to make it cleaner but not sure how 🤔 there's no way of getting the proper protocol before we're inside the method itself.

@thrau thrau marked this pull request as ready for review November 3, 2024 18:08
@thrau thrau requested a review from baermat as a code owner November 3, 2024 18:08
@thrau thrau merged commit b5823f8 into master Nov 3, 2024
@thrau thrau deleted the sqs-dev-endpoints-json branch November 3, 2024 18:08
@thrau
Copy link
Member Author

thrau commented Nov 3, 2024

thanks for your changes @bentsku! merging this PR to unblock @webdev51 for the SQS resource browser improvements. we still need to add the x-amz-json format to the openapi spec so we can re-enable this check here

# @pytest.mark.usefixtures("openapi_validate")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants