-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
cleanup ServiceNameParser rules #12358
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
Conversation
S3 Image Test Results (AMD64 / ARM64) 2 files 2 suites 8m 16s ⏱️ Results for commit c033e74. ♻️ This comment has been updated with latest results. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely great that you are following up the CORS / handler chain PRs! 🧹 🤩
I just want to make sure that this cannot have an impact on existing LocalStack users, afterwards we should be good to go! 🚀
f723f2a
to
fa3af88
Compare
fa3af88
to
c033e74
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for addressing the comments! Lookin' good! I think it's better to first cleanup as much as possible without introducing any change to the behavior, and remove the S3 specific things in a deliberate follow up PR. 💯
Motivation
Following #11800, we can remove rules that were in place so that edge router match would be assigned to a service and not to S3 (preventing CORS to be applied).
Those rules are not relevant anymore, and would not get matched as the edge routes would skip the service name parsing all together.
Edit: we will not change the current logic to fallback to S3, as it could potentially break some use-case. This PR only cleans up the other legacy rules for now, and S3 will be tackled in a follow up.
There's a small hack in place to fallback to S3 requests, something suggested by Alex at the time, because right now the "legacy rules" were using some heuristics to try to guess if the request was S3. Now, by using the OP router, we can be somewhat sure if a request is a S3 request or does not fit at all what it should look like.Not sure about the caching of the OP router, a class might be better for the
determine*
function? This is a draft to see if the pipeline works, all S3 tests even without AWS signature are still working, so it seems pretty good 👌Changes