S3: fix CORS handler + GZIP handling #12855
Conversation
S3 Image Test Results (AMD64 / ARM64) 2 files 2 suites 8m 46s ⏱️ Results for commit c646af8. |
Test Results (amd64) - Integration, Bootstrap 5 files ± 0 5 suites ±0 1h 26m 31s ⏱️ - 53m 59s Results for commit c646af8. ± Comparison against base commit f0f8045. This pull request removes 3004 and adds 1 tests. Note that renamed tests count towards both. |
LocalStack Community integration with Pro 2 files ± 0 2 suites ±0 1h 3m 25s ⏱️ - 39m 40s Results for commit c646af8. ± Comparison against base commit f0f8045. This pull request removes 2669 and adds 1 tests. Note that renamed tests count towards both. |
There was a problem hiding this comment.
Nice catch! I added a comment just to verify that I did grasp the implications of this PR correctly, but in my opinion they are minimal and this PR is good to go.
On the broader topic: I think the problem here is just that this is a conceptional problem we are having with our one-dimensional handler chain, in combination with multiple different routing mechanisms (internal endpoints, dynamic edge routes, service routes), and a clean solution to this issue would mean that we would have to have a generalized routing which then invokes service specific handler chains... 🤔
| context.service = self._service | ||
|
|
||
| if config.DISABLE_CUSTOM_CORS_S3: |
There was a problem hiding this comment.
thought: I'll just ramble around here, just to make sure that we are on the same page when it comes to the (performance) implications of this PR. Please correct me if any of this is wrong 😅
So this config is false by default. This means in the vast majority of cases, we already already did execute the minimum S3 service detection, and for this (default) case, this PR does not have any implications.
However, for users who actively set DISABLE_CUSTOM_CORS_S3 in their configuration, this PR will change the behavior such that this simplified service routing heuristic is now being executed.
If this is the case, I would say that this change is absolutely reasonable, and the implications are pretty much minimal.
There was a problem hiding this comment.
Exactly, this is exactly it. Thanks for making it clearer 👌
| context.service = self._service | ||
|
|
||
| if config.DISABLE_CUSTOM_CORS_S3: |
There was a problem hiding this comment.
Exactly, this is exactly it. Thanks for making it clearer 👌
Motivation
As reported by #12854, we had an issue in S3 with GZIP handling when disabling S3 CORS.
This is because the GZIP handler depends on the service name being set in the context. However, we were only attaching the S3 service name if S3 CORS was not disabled.
This is a consequence of the ServiceNameParser being moved further down the chain with #11800
As a quick fix and bandaid, I've moved the logic around to always try to determine if it's an S3 request, even if S3 CORS is disabled.
However, we're muddling a bit the responsibility of the "CORS" handler with a mini service name parser. I'd suggest to get this first PR as a quick fix for the issue, because the first part will always need to be executed anyway.
As a follow-up, we could either split this in 2 handlers: one "S3 Detection" and one "S3 CORS", however the order will need to be respected closely, and the ordering of handlers inside a
CompositeHandleris not something that is defined. It is rather implicit depending on the order you are appending your handlers, but it could be modified somewhere else.My problem is the naming of the "S3CORSHandler", which has more responsibilities than just CORS
So for now I'm proposing to do it this way to quickly solve the issue, which does not really have a cost anyway, but we'll need to address the naming + responsibility issue soon 👀
Changes
fixes #12854