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

fix SQS JSON protocol support in ASF #8268

Merged
merged 22 commits into from Nov 11, 2023
Merged

fix SQS JSON protocol support in ASF #8268

merged 22 commits into from Nov 11, 2023

Conversation

alexrashed
Copy link
Member

@alexrashed alexrashed commented May 5, 2023

This PR aims at providing JSON support for SQS. AWS supports both protocols, query and json, for SQS for some time.

Initially explored solution:

As it turns out this solution is not feasible, because the JSON specification removed a lot of information from the spec which is used to properly parse and serialize the requests and responses.
This means that there is no other alternative than handling two different specifications.

New solution

  • Upgrade to the latest version of botocore internally.
  • Add the latest available version of the SQS query specifications to localstack.aws.data.
    • This is a newly created folder which is configured as extra search path in botocore when loading the specifications.
    • This will add a newly created "alias" service called sqs-query.

What's left to do:

  • Check if it is necessary to support JSON requests for SQS Queue URLs (or implement content negotiation).
    • Not necessary per se, it is not used by the json protocol based SDK. Would be nice to implement soon.
  • Check if the internal dev endpoints for SQS should be migrated to JSON (or implement content negotiation).
    • those endpoint are not targeted by SDKs, so it is not necessary right now, and can be tackled in a second iteration
  • Fix failing tests.
    • Most of them should only fail because they assume an XML response structure, but due to the upgrade of botocore a JSON request (and with that a JSON response) is sent.
  • Assert that the implementation works with some tests explicitly using XML
  • Ensure compatibility with ext (-ext is mostly green, only 4 tests to fix there with wrong assert)

@alexrashed alexrashed self-assigned this May 5, 2023
@alexrashed alexrashed added the semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases label May 5, 2023
@alexrashed alexrashed marked this pull request as draft May 5, 2023 13:23
@github-actions
Copy link

github-actions bot commented May 8, 2023

LocalStack Community integration with Pro

       2 files  ±0         2 suites  ±0   1h 25m 8s ⏱️ + 18m 49s
2 307 tests ±0  2 010 ✔️  - 1  297 💤 +1  0 ±0 
2 308 runs  ±0  2 010 ✔️  - 1  298 💤 +1  0 ±0 

Results for commit 3c5a78f. ± Comparison against base commit 74c336c.

This pull request skips 1 test.
tests.aws.services.sqs.test_sqs.TestSqsProvider ‑ test_delete_message_batch_from_lambda

♻️ This comment has been updated with latest results.

trentm added a commit to elastic/apm-agent-nodejs that referenced this pull request May 8, 2023
… (similar to s3.test.js)

This is mostly complete. There are a few XXXs to deal with.
The blocker for now is, IIUC, that localstack doesn't currently support
SQS using the JSON protocol -- see localstack/localstack#8268
@alexrashed alexrashed added this to the 3.0 milestone Jul 11, 2023
@coveralls
Copy link

Coverage Status

coverage: 84.017% (+0.002%) from 84.015%
when pulling 3c5a78f on fix-sqs-json
into 74c336c on master.

@bentsku bentsku marked this pull request as ready for review November 10, 2023 19:45
Copy link
Member Author

@alexrashed alexrashed left a comment

Choose a reason for hiding this comment

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

Thanks so much @bentsku for finalizing the PR and figuring out the nitty gritty details of the remaining test failures! 🦸🏽
From my perspective, this PR is ready to be merged as it is, but I would like to get another pair of eyes from @dominikschubert or @thrau before merging this.
I'm not happy with some of the changes in here (like the hacky "provider alias"), but I think we can iron these things out in a follow-up.

localstack/aws/protocol/serializer.py Show resolved Hide resolved
Copy link
Member

@dominikschubert dominikschubert left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks so much @alexrashed and @bentsku for getting this over the line so quickly!

It's probably not going to be the only case where they'll make the switch to JSON I guess 😞

As @bentsku said, any typos, etc. can be removed in a follow-up. First let's get this merged 🚀

localstack/aws/protocol/service_router.py Show resolved Hide resolved
localstack/utils/coverage_docs.py Show resolved Hide resolved
localstack/utils/coverage_docs.py Show resolved Hide resolved
tests/unit/aws/test_spec.py Show resolved Hide resolved
@dominikschubert dominikschubert merged commit 76483ac into master Nov 11, 2023
29 checks passed
@dominikschubert dominikschubert deleted the fix-sqs-json branch November 11, 2023 10:06
@bentsku bentsku mentioned this pull request Nov 11, 2023
1 task
@ilia-beliaev-miro
Copy link

Looks like this change has broke the localstack SQS usage for us due to the service name change from sqs to sqs-query. We receive Service 'sqs-query' is not enabled. now, was it an expected outcome and we should now switch the service name when creating a localstack instance from sqs to sqs-query?

@bentsku
Copy link
Contributor

bentsku commented Nov 13, 2023

Hello @ilia-beliaev-miro, this is an unexpected outcome and we will fix it with #9607. Sorry for the issue, you can declare both service names for now, but it should be fixed by the end of the day!

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.

None yet

5 participants