encode: Add zstd compression level support

[dev-polymer]

Its changes adds support zstd encoder ability to spicify compression level like is able for Gzip (and for brotli via ueffel/caddy-brotli module).

The solution appeared as a continuation of this conversation on the caddy community forum/

Notices:

• Only for levels 1,2,3,4 supported beacuse its only supported in used zstd library and focusing on the fact that in encode gzip and brotli we can set up a different number of levels, I consider a suitable solution with the ability to configure only 4 levels
• Without explicitly specifying a level or specifying 0, the default level 2 will be selected
• Buffer size zstd.WithWindowSize(128<<10) is hadrcoded as before and when i try delete and trust the automatic buffer selection in the library algorithm — i have compression error in Chrome: ERR_CONTENT_DECODING on CSS files. I think no hardcode in buffer size could be logical, but if it not stabe working, it seems like a good idea to move buffer size feature to a separate task, but i make test with hardcoded zstd.WithWindowSize(512<<10) value just for intrest, and it is working ok:
  
  ...and compress even better, but my knowledge of compression is insufficient for an informed choice of best level.

I have been running tests of this solution on a fully functional multipage website in the Chrome v122 browser.

[CLAassistant]

All committers have signed the CLA.

[francislavoie]

I think we should add a godoc comment here to explain the possible options.

[WeidiDeng]

Generally we should built-in or caddy defined type to configure a plugin. We should use string to specify the level and check if it's one of the possible values provided by zstd.

[WeidiDeng]

Level should be just string. We can use a private member to specify the actual level used with type zstd.EncoderLevel.

[francislavoie]

Please remember to sign the CLA - you might need to update your emails on your github account to match your commit, or you should update your local git config to use the correct email, and rewrite the commit's email.

[WeidiDeng]

To convert string to levels we can use this function.

[WeidiDeng]

Although their values are from 1 to 4, we shouldn't rely on this behaviour. They are defined as iota.

[WeidiDeng]

The reason why gzip can check for number range is because the level is given explicitly as a number. But zstd only provides 4 constants.

[francislavoie]

How would you suggest we do it then @WeidiDeng?

[dev-polymer]

@WeidiDeng , you are mostly right, but let me some explaination. I mean usage of text constants not matches caddyfile's code-style and other encoder algo's compression levels (1-9 for gzip and 1-11 for brotli).

I think about another realization, and propose for appeal to original zstd levels, not library approximation. It is 1 to 22 (or 1 to 19, excludes --ultra levels which recommended to carefull usage).

But if we implement library's way to transform caddyfile's levels to zstd levels, we will have the option to specify the level 6,7,8,9 and it will be the same value (for the current version of the used library). And it will surprise the user and it will seem that changing the level does not work (in some ranges).

Perhaps such a solution will be more universal for the long term, but now it will introduce a little confusion and crutchiness in the configuration. And even more then little: if I specify level 9 today, I will get a conditionally N% CPU load, then if the library implements level 9 to more powerfull compression, perhaps the CPU load will increase by a multiple, for example 3x N%, which may be an unpleasant surprise for a user who has updated the caddy in the future.

This is reason, why i chose to offer a version with 4 levels...

Finally, i think the best solution would be allow user to specify levels from 1 to 22 (refer to original Zstandart levels), convert it with library's method and may be we may need to specify in the documentation (or even caddy warning logs) that the recommended and future safe operating zstd levels should be 1, 3, 6 and 10 now.

WDYT?

[francislavoie]

I think we can use the strings/named options for zstd, I don't think that's weird. We can simply document them. We use named options in many places in the Caddyfile.

[dev-polymer]

I pushed new commit now, but my tests show error when level is not set in CaddyFile:

//Test results (with default 128kb zstd buffer) of 585kb minified uncompressed CSS-file
zstd (level not set) → ERR_HTTP2_PROTOCOL_ERROR (Chrome v122 browser error)
zstd fastest → 337kb 
zstd default → 294kb
zstd better → 290kb
zstd best → 286kb

Please friends suggest how to fix 😳

[francislavoie]

Probably need to set a default in Provision

[mholt]

I can do a review as soon as the CLA is signed 😊

[francislavoie]

The CLA still isn't happy, because your first commit used an email address not attached to your GitHub account.

Please rewrite the commits to change the email address on the first commit (google it, it's a simple enough operation to perform) then force-push your branch.

[francislavoie]

Thanks! We can merge once @WeidiDeng's feedback is addressed

[WeidiDeng]

Level should be just string. We can use a private member to specify the actual level used with type zstd.EncoderLevel.

[mholt]

@WeidiDeng How about now? I'm good with this if you are. 👍

[polarathene]

• Only for levels 1,2,3,4 supported beacuse its only supported in used zstd library and focusing on the fact that in encode gzip and brotli we can set up a different number of levels, I consider a suitable solution with the ability to configure only 4 levels

Will that documentation be visible/linked for the equivalent Caddy docs on the values? For those that are familiar with zstd elsewhere it's useful to have some context on what they map to without having to hunt for the information on Github :)

---

• Buffer size zstd.WithWindowSize(128<<10) is hadrcoded as before and when i try delete and trust the automatic buffer selection in the library algorithm — i have compression error in Chrome: ERR_CONTENT_DECODING on CSS files. I think no hardcode in buffer size could be logical, but if it not stabe working, it seems like a good idea to move buffer size feature to a separate task, but i make test with hardcoded zstd.WithWindowSize(512<<10) value just for intrest, and it is working ok

This may vary by client, but Chrome is following guidance from RFC 8878 to limit the buffer window size to 8MB: https://issues.chromium.org/issues/40196713#comment31

Prevent OoM condition by restricting maximum window size

Unlike gzip or Brotli where the maximum window size is fixed (32KB
for zlib and 16MB for brotli, see RFC1952 and RFC7932), zstd
allows much bigger window sizes (up to 3.75TB, see RFC8878).

This potentially exposes a user agent if the response to a request
leverages a payload with a request for a very large window descriptor.

Fortunately, the RFC8878 authors already addressed the potential issue
by allowing a user agent to 'reject a compressed frame that requires
a memory size beyond the decoder's authorized range.

The current change remedies this issue by setting a maximum allowed
window size, limiting the buffer size to 8MB as suggested by RFC8878.

https://issues.chromium.org/issues/41493659#comment4

I reproduced the issue, and it seems the decompression fails with the error code 16, which is "Frame requires too much memory for decoding".
For memory usage reasons, Chromium limits the window size to 8MB whereas curl and directly using zstd does not have that constraint.
You can most likely use https://pkg.go.dev/github.com/klauspost/compress/zstd#WithWindowSize with anything less than 1<<23, and it should fix the issue.

[mholt]

@polarathene

Will that documentation be visible/linked for the equivalent Caddy docs on the values? For those that are familiar with zstd elsewhere it's useful to have some context on what they map to without having to hunt for the information on Github :)

Yes, our docs will explain what the possible values are, but the zstd lib we use recommends we use only those constants as the underlying meaning may change 🤷‍♂️

[polarathene]

zstd lib we use recommends we use only those constants as the underlying meaning may change 🤷‍♂️

Ah ok I see that after looking into the package docs. It supports int levels that are more familiar for zstd config but maps to the nearest supported constant mode the package offers instead.

This was already discussed earlier above, but I disagree with the conclusion since the constant modes are also subject to change just as much, so the ordinal mapping which can be more familiar should be roughly equivalent if it's maintained alongside those changes? 🤷‍♂️

It's true that the ordinal mapping would be a bit confusing without that knowledge, due to the mapped ranges producing the same output and potential for future updates to shift those range bounds. I think linking to this part of the package README might be relevant to get an approximation of the ordinal mapping that each constant mode is roughly equivalent to (assuming it's kept up to date).

---

The README reference also follows with a performance comparison to the CGO package (datadog/zstd) which shows notable overhead from klauspost/compress zstd implementation.

I've seen for brotli there are two external packages/modules, one that is pure Go and another that requires CGO enabled during build, but I'm not sure how feasible / compatible a zstd CGO alternative would be since unlike brotli support Caddy has official zstd already with the encode directive 😅

[PoneyClairDeLune]

https://caddyserver.com/docs/caddyfile/directives/encode

The Caddyfile documentation for the encode directive wasn't updated.

[mholt]

Thanks for the heads up. Just pushed the docs for it.