10000 cleanup ServiceNameParser rules by bentsku · Pull Request #12358 · localstack/localstack · GitHub
[go: up one dir, main page]
Skip to content

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.

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 14, 2025
Merged

cleanup ServiceNameParser rules #12358

merged 1 commit into from
Mar 14, 2025

Conversation

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

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

  • cleanup the service name parser rules that were there to prevent them being flagged as S3. Now that those routes are matched before, we don't need them anymore

@bentsku bentsku 10000 added area: asf semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases labels Mar 7, 2025
@bentsku bentsku added this to the 4.3 milestone Mar 7, 2025
@bentsku bentsku self-assigned this Mar 7, 2025
@github-actions GitHub Actions
Copy link
github-actions bot commented Mar 7, 2025 •
edited
Loading

LocalStack Community integration with Pro

    2 files  ±0      2 suites  ±0   1h 52m 13s ⏱️ +55s
4 144 tests ±0  3 824 ✅ ±0  320 💤 ±0  0 ❌ ±0 
4 146 runs  ±0  3 824 ✅ ±0  322 💤 ±0  0 ❌ ±0 

Results for commit c033e74. ± Comparison against base commit e8907e5.

♻️ This comment has been updated with latest results.

@github-actions GitHub Actions
Copy link
github-actions bot commented Mar 7, 2025 •
edited
Loading

S3 Image Test Results (AMD64 / ARM64)

  2 files    2 suites   8m 16s ⏱️
478 tests 426 ✅  52 💤 0 ❌
956 runs  852 ✅ 104 💤 0 ❌

Results for commit c033e74.

♻️ This comment has been updated with latest results.

@bentsku bentsku marked this pull request as ready for review March 8, 2025 02:32
@bentsku bentsku requested a review from alexrashed as a code owner March 8, 2025 02:32
alexrashed
alexrashed reviewed Mar 13, 2025
View reviewed changes
Copy link
Member
@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.

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! 🚀

@bentsku bentsku force-pushed the cleanup-service-name-parser branch from f723f2a to fa3af88 Compare March 13, 2025 16:46
@bentsku bentsku force-pushed the cleanup-service-name-parser branch from fa3af88 to c033e74 Compare March 13, 2025 16:46
alexrashed
alexrashed approved these changes Mar 13, 2025
View reviewed changes
Copy link
Member
@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 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. 💯 :shipit:

@bentsku bentsku merged commit c795e31 into master Mar 14, 2025
40 checks passed
@bentsku bentsku deleted the cleanup-service-name-parser branch March 14, 2025 00:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexrashed alexrashed alexrashed approved these changes

Assignees

@bentsku bentsku

Labels
area: asf semver: minor Non-breaking changes which can be included in minor releases, but not in patch releases
Projects
None yet
Milestone
4.3
Development

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