Add support for Zstandard to the Compression middleware #10660
Conversation
Belphemur
force-pushed
the
10557-compress-zstd
branch
2 times, most recently
from
f8a5a65
to
fb82698
Compare
Belphemur
force-pushed
the
10557-compress-zstd
branch
from
fb82698
to
849b3a5
Compare
There was a problem hiding this comment.
The package compresshandler should be merged with compress.
My suggestions will produce a non-working code because more changes are required (like the remove of the package compresshandler)
Also compression_handler.go and compression_wrapper.go should be merged in one file compression_handler.go.
pkg/middlewares/compress/compresshandler/compression_wrapper.go
Outdated
Show resolved
Hide resolved
pkg/middlewares/compress/compresshandler/compression_handler.go
Outdated
Show resolved
Hide resolved
pkg/middlewares/compress/compresshandler/compression_handler.go
Outdated
Show resolved
Hide resolved
pkg/middlewares/compress/compresshandler/compression_handler.go
Outdated
Show resolved
Hide resolved
pkg/middlewares/compress/compresshandler/compression_handler.go
Outdated
Show resolved
Hide resolved
pkg/middlewares/compress/compresshandler/compression_handler.go
Outdated
Show resolved
Hide resolved
pkg/middlewares/compress/compresshandler/compression_handler.go
Outdated
Show resolved
Hide resolved
pkg/middlewares/compress/compresshandler/compression_handler.go
Outdated
Show resolved
Hide resolved
pkg/middlewares/compress/compresshandler/compression_handler.go
Outdated
Show resolved
Hide resolved
pkg/middlewares/compress/compresshandler/compression_handler.go
Outdated
Show resolved
Hide resolved
Co-authored-by: Ludovic Fernandez <[email protected]>
Co-authored-by: Ludovic Fernandez <[email protected]>
Co-authored-by: Ludovic Fernandez <[email protected]>
Co-authored-by: Ludovic Fernandez <[email protected]>
Add support for logging compression error
Avoid duplication of setting the algo, it was forgotten in the first version of the code.
|
@ldez Ready for second round of review. For the test suite, I don't think all test case beneficiate from iterating through all the compression algo especially when they are testing that the compression isn't used. Other than that, I've merge together the handler and wrapper, used your suggested logic for it (way cleaner, thank you for it). |
This make the middleware fails. It should be logged as an error.
Co-authored-by: Ludovic Fernandez <[email protected]>
Co-authored-by: Ludovic Fernandez <[email protected]>
Co-authored-by: Ludovic Fernandez <[email protected]>
Belphemur
force-pushed
the
10557-compress-zstd
branch
from
29d9c42
to
8cfe7d5
Compare
|
I will add an extra commit to your PR, related to a refactor, but not directly related to the code of your PR. |
|
@ldez It seems GitHub still state that the PR needs some changes ? I imagine since you have added the refactor of CompressHandler, that this is just forgetting to approve the PR ? |
What does this PR do?
This PR will add support for Zstandard compression when browser state they support it.
It will take precedence on any other compression algorithm like brotli and gzip.
I also refactored the code of the Brotli compression to have only one compression handler for Brotli and Zstandard to simplify the maintenance and reduce the chance of new bug being introduced.
Motivation
Request was made for the feature, and I'm sure this will make traefik one of the first server supporting the zstd compression algorithm.
Fixes #10557
More
Additional Notes