Skip to content
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

Add support for Zstandard to the Compression middleware #10660

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

Belphemur
Copy link

@Belphemur Belphemur commented Apr 24, 2024

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

  • Added/updated tests
  • Added/updated documentation

Additional Notes

Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@Belphemur
Copy link
Author

@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).

@Belphemur Belphemur requested a review from ldez April 25, 2024 15:01
pkg/middlewares/compress/compress_test.go Outdated Show resolved Hide resolved
pkg/middlewares/compress/compression_handler.go Outdated Show resolved Hide resolved
pkg/middlewares/compress/compression_handler.go Outdated Show resolved Hide resolved
pkg/middlewares/compress/compression_handler.go Outdated Show resolved Hide resolved
pkg/middlewares/compress/compression_handler_test.go Outdated Show resolved Hide resolved
pkg/middlewares/compress/compression_handler_test.go Outdated Show resolved Hide resolved
pkg/middlewares/compress/compression_handler_test.go Outdated Show resolved Hide resolved
pkg/middlewares/compress/compression_handler_test.go Outdated Show resolved Hide resolved
@ldez
Copy link
Member

ldez commented Apr 25, 2024

I will add an extra commit to your PR, related to a refactor, but not directly related to the code of your PR.

@ldez ldez added this to the next milestone Apr 25, 2024
@ldez ldez added the kind/enhancement a new or improved feature. label Apr 25, 2024
@Belphemur
Copy link
Author

@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 ?

Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kevinpollet kevinpollet self-assigned this May 13, 2024
@Belphemur
Copy link
Author

Belphemur commented Jun 3, 2024

@ldez I'd like to confirm your latest change.
By making the br > zstd > gzip, this will basically make zstd never used. Since every browser that support zstd do also support br.

Unless the browser decides to drop br which I don't think there is any plan for.

My understanding is zstd has better performance, compression and decompression than br hence why it made sense to give it priority.

@ldez
Copy link
Member

ldez commented Jun 3, 2024

The compression depends on the Accept-Encoding header, I create another PR to support the weight. See #10777.

The weight allows the client to specify the compression type "priority/quality".

By making the br > zstd > gzip,

The default order is br > zstd > gzip for compatibility.

The browser's usage is one of the possible use cases.
Browsers can define the quality and so the order will be changed.

When PR will be merged, I update this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

zstandard compression
5 participants