move parse_service_name after serve_edge_router_rules #11800
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
LocalStack Community integration with Pro 2 files 2 suites 1h 54m 44s ⏱️ Results for commit 1e89601. ♻️ This comment has been updated with latest results. |
d675ef8 to
7254b0c
Compare
7254b0c to
1e89601
Compare
alexrashed
approved these changes
Mar 6, 2025
| handlers.validate_request_schema, # validate request schema for public LS endpoints | ||
| handlers.serve_localstack_resources, # try to serve endpoints in /_localstack | ||
| handlers.serve_edge_router_rules, | ||
| # start aws handler chain | ||
| handlers.parse_service_name, |
There was a problem hiding this comment.
praise: This absolutely minimal change is the result of a lot of work. Thanks for staying at it, absolutely fantastic to see this moving down! 💯 🥳
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation
This PR moves our ServiceNameParser to be executed after our Edge Router.
Note: this will break upstream until #11801 and its companion are merged, so should be reviewed together.
For historical reasons, we needed the service name for our security handling (we talk about CORS, but it is not about CORS, CORS is a "response" part, this is about our CRSF/firewall? security where we reject a request if not in our allowed hosts).
It however lead to some issues because every route would be considered as an S3 request, as our service name could not parse the request.
As S3 now properly manages its own CORS via a special handler executed very early in the request chain, we already know before the service name parser if an S3 request that needs to have its CORS managed is an S3 request or not via some heuristics.
I believe the burden to find solutions for CORS is on the self-managed CORS services, and not for every edge route to have to find a way not to be an S3 request via the
localstack.aws.protocol.service_router.legacy_rulesorcustom_path_addressing_rules.Currently we do have solutions for both S3 and API Gateway, so I think now is the time to finally jump the gun so we don't have CORS issues with any of external routes: they will all be managed by our default CORS/security handling.
As a side effect, we also will see less issues where the stream of a route would have been consumed by our ServiceNameParser 😄
Follow-up would be to clean the
service_router.legacy_rulesbecause most of them are there because of CORS/security issues.Changes
Testing
run #/13565727990 (failing due to APIGW, fixed with #11801 and upstream PR)