encode: Enhancements to content negotiation, serving precompressed content #4045

ueffel · 2021-03-02T19:10:21Z

This pull request partly addresses #2665

It adds:

the ability to configure the minimum_length of a response for encoding to happen via caddyfile
the ability to configure encoding preference on the server-side (prefer setting for encode)
a new interface Precompress, which modules can implement to provide the file extension suffix for the file_server to search for precompressed files

Differences to the previous iteration of the changes (master...ueffel:file_server-precompressed):
ueffel/caddy@file_server-precompressed...ueffel:file_server-precompressed_v2

module namespace for precompress modules changed to http.precompressed.*
new precompress modules for gzip, zstd, br
caddyfile directive and interface name changed to Precompressed

Edit:
Instead of introducing a new dependency to "github.com/gobwas/glob" now the content-types matching uses filepath.Match. Maybe the "validation" by matching with an empty string does not produce a ErrBadPattern for every bad patttern 🤔

CLAassistant · 2021-03-02T19:10:24Z

All committers have signed the CLA.

mholt

Awesome, thanks so much for working on this.

This is my first pass -- see what you think!

modules/caddyhttp/encode/br/brotli_precompressed.go

mholt · 2021-03-02T20:35:34Z

modules/caddyhttp/encode/encode.go


 	// Only encode responses that are at least this many bytes long.
 	MinLength int `json:"minimum_length,omitempty"`

+	// Only encode responses that match at least one of the content-types.
+	Types []string `json:"types,omitempty"`


It occurred to me, would there be any value in letting the user configure a ResponseMatcher here? This could let them choose based on other headers (and status code) too, but I'm not sure how useful that is.

(To be clear, what you have is fine, but before we introduce a non-breakable change I want to get your thoughts on this. If the ResponseMatcher approach is feasible, that might be better since it gives more flexibility.)

I didn't know a ResponseMatcher exists. I will have a look.

#4021 is something you could reference re response matchers. Not sure if that's quite the direction we want to go, but it could be.

encode zstd gzip { minimum_length 256 prefer zstd gzip @text { status 2xx 4xx header Content-Type text/* header Content-Type application/json header Content-Type application/javascript header Content-Type application/xhtml+xml header Content-Type application/atom+xml header Content-Type application/rss+xml header Content-Type image/svg+xml } match @text }

something like this is possible now

Hmm. I'm thinking whether it would work better like this, in this case, since it only makes sense to define match once for encode, unlike reverse_proxy's handle_response which can appear more than once:

encode [<encoders...>] { match [<matcher> [<args...]] { <matcher> <args...> } }

Basically skip the named matcher stuff and directly read the args on the same line and/or block for the matcher. This shouldn't change the code much, and should still be reusable.

Awesome! Looks good 😄

Small thing, I think it might be better to make parseNamedResponseMatcher return the response matcher rather than shoving it into responseMatchers and read it back out.

I haven't touched parseNamedResponseMatcher intentionally. If it will be reusable in that state from your pull request, the changes required to use the hopefully soon merged implementation will be minimal.
Also it ensures right now, that match is only defined once in a encode block.

mholt · 2021-03-02T20:35:49Z

modules/caddyhttp/encode/encode.go

+	if len(enc.Types) == 0 {
+		enc.Types = []string{"*"} // backwards compatibility
+
+		// sane default for text-based content ?


We like sane defaults

with the implementation via ResponseMatcher the default is:

header Content-Type text/* header Content-Type application/json header Content-Type application/javascript header Content-Type application/xhtml+xml header Content-Type application/atom+xml header Content-Type application/rss+xml header Content-Type image/svg+xml

Changed the default to:

header Content-Type text/* header Content-Type application/json* header Content-Type application/javascript* header Content-Type application/xhtml+xml* header Content-Type application/atom+xml* header Content-Type application/rss+xml* header Content-Type image/svg+xml*

to account for any ;charset=xxx that may be appended to the header.

Are there any MIME types that could match these globs except for the ;charset=... addition?
Could we use a regexp instead (or is it too much of a performance hit, maybe?)

Regexps aren't supported by the ResponseMatcher type, yet. It would be nice the have a header_regexp directive from RequestMatchers as well, but I think this would be out of the scope of this PR. Maybe an additional one?

I agree we should have a header_regexp matcher. If you want to do a follow-up with that, that'd be great 👍
I am a bit worried that it would be detrimental to performance for the encode defaults though, so a bit of benchmarking may be beneficial to see if it makes any appreciable difference.

modules/caddyhttp/encode/br/brotli_precompressed.go

modules/caddyhttp/encode/gzip/gzip_precompressed.go

modules/caddyhttp/fileserver/caddyfile.go

modules/caddyhttp/fileserver/staticfiles.go

mholt · 2021-03-02T20:40:46Z

modules/caddyhttp/fileserver/staticfiles.go

+	PrecompressRaw caddy.ModuleMap `json:"encodings,omitempty" caddy:"namespace=http.precompressed"`
+
+	// If the client has no strong preference, choose these encodings in order.
+	PrecompressOrder []string


Maybe explain what the default is.

modules/caddyhttp/fileserver/staticfiles.go

ueffel · 2021-03-02T22:21:16Z

Would you like changes to be in additional commit or should I rebase-edit changes into the existing commits?

francislavoie · 2021-03-02T22:29:01Z

We squash-merge at the end, so do whatever works best for you for now 👍

modules/caddyhttp/encode/caddyfile.go

also change the namespace of the encoders back, I accidently changed to precompressed >.> default matchers include a * to match to any charset, that may be appended

francislavoie · 2021-03-07T18:08:18Z

Could you add an adapt test as well for this? See https://github.com/caddyserver/caddy/tree/master/caddytest/integration/caddyfile_adapt, both for encode and your file_server changes. Just for the coverage.

mholt

Awesome, this is looking so good!! Nice work.

Just a couple nits. But I think we're good to go. Caddyfile tests that Francis mentioned can come either in this PR or a later one. Also, if you want to make sure godoc comments end with periods (or other proper punctuation), that'd be good for legibility when they are read on the docs website.

Almost there!

mholt · 2021-03-11T22:15:07Z

modules/caddyhttp/encode/encode.go


 	// Only encode responses that are at least this many bytes long.
 	MinLength int `json:"minimum_length,omitempty"`

+	Matcher *caddyhttp.ResponseMatcher


This needs a JSON tag 😊

mholt · 2021-03-11T22:16:07Z

modules/caddyhttp/encode/encode.go

@@ -332,6 +392,13 @@ type Encoding interface {
 	NewEncoder() Encoder
 }

+// Precompressed is a type which returns filename suffix of precomressed


Suggested change

// Precompressed is a type which returns filename suffix of precomressed

// Precompressed is a type which returns filename suffix of precompressed

ueffel · 2021-03-13T15:32:15Z

Could you add an adapt test as well for this? See https://github.com/caddyserver/caddy/tree/master/caddytest/integration/caddyfile_adapt, both for encode and your file_server changes. Just for the coverage.

The file_server adapt test is giving me a headache. It automatically adds the "Caddyfile" with a OS-specific path separator as hidden file. So the test fails either on unix/mac or on windows, because in the adapt-txt file can only be one specific path separator. Should I use a placeholder for the path separator and added a replacement step in the step before the actual comparison is done?

francislavoie · 2021-03-13T15:39:40Z

Right... Dangit. Yeah, I think a string replacement for ".\\Caddyfile" to "./Caddyfile" should be enough, in caddyfile_adapt_test.go just before calling CompareAdapt. We already replace windows newlines with unix ones, so there's precedent for doing that.

Edit: Sorry - I think the replacement might need to happen inside CompareAdapt cause what you actually need to replace is the string in the adapt output, cause that's the variable. It's a bit less clean to do it in there, but 🤷‍♂️ good enough I think.

* added integration tests for new caddyfile directives * improved various doc strings (punctuation and typos) * added json tag for file_server precompress order and encode matcher

ueffel · 2021-03-13T18:35:35Z

Edit: Sorry - I think the replacement might need to happen inside CompareAdapt cause what you actually need to replace is the string in the adapt output, cause that's the variable. It's a bit less clean to do it in there, but 🤷‍♂️ good enough I think.

I just did it here, if that's ok? It seems to work, tests are not failing anymore.

francislavoie · 2021-03-13T18:37:07Z

Yeah that seems okay to me 👍

…mpressed files

ueffel · 2021-03-19T19:23:41Z

So, I would consider this PR ready-to-merge or at least ready-for-final-review.

francislavoie

This is excellent work! Thank you!

If you find the time, it would be great if you could update the docs for https://github.com/caddyserver/website/blob/master/src/docs/markdown/caddyfile/directives/encode.md and https://github.com/caddyserver/website/blob/master/src/docs/markdown/caddyfile/directives/file_server.md to cover the changes 📝

modules/caddyhttp/encode/gzip/gzip.go

mholt

Awesome, this looks good -- let's give it a try. Thanks for your persistence in helping to get this ready! Please contribute again sometime.

ueffel force-pushed the file_server-precompressed_v2 branch from 3379bb5 to 7bc0930 Compare March 2, 2021 19:50

mholt requested changes Mar 2, 2021

View reviewed changes

mholt added the under review 🧐 Review is pending before merging label Mar 2, 2021

mholt added this to the 2.x milestone Mar 2, 2021

ueffel force-pushed the file_server-precompressed_v2 branch from 7bc0930 to b1fdbf0 Compare March 3, 2021 19:27

francislavoie reviewed Mar 3, 2021

View reviewed changes

modules/caddyhttp/encode/caddyfile.go Show resolved Hide resolved

ueffel added 7 commits March 7, 2021 17:22

encode: implement prefer setting

4d26a36

encode: minimum_length configurable via caddyfile

a519fec

encode: configurable content-types which to encode

b71cdaf

file_server: support precompressed files

a05ee3b

encode: use ReponseMatcher for conditional encoding of content

2e32c01

linting error & documentation of encode.PrecompressedOrder

fbca316

encode: allow just one response matcher

67ab2d0

also change the namespace of the encoders back, I accidently changed to precompressed >.> default matchers include a * to match to any charset, that may be appended

ueffel force-pushed the file_server-precompressed_v2 branch from 8673868 to 67ab2d0 Compare March 7, 2021 16:26

mholt requested changes Mar 11, 2021

View reviewed changes

ueffel force-pushed the file_server-precompressed_v2 branch from 9cd73eb to d8154e7 Compare March 13, 2021 14:50

rounding of the PR

10a4f2c

* added integration tests for new caddyfile directives * improved various doc strings (punctuation and typos) * added json tag for file_server precompress order and encode matcher

ueffel force-pushed the file_server-precompressed_v2 branch from d8154e7 to 10a4f2c Compare March 13, 2021 16:11

file_server: add vary header, remove accept-ranges when serving preco…

e2ac31f

…mpressed files

francislavoie requested a review from mholt March 19, 2021 19:39

francislavoie previously approved these changes Mar 19, 2021

View reviewed changes

mholt reviewed Mar 19, 2021

View reviewed changes

modules/caddyhttp/encode/gzip/gzip.go Outdated Show resolved Hide resolved

francislavoie changed the title ~~Enhancements to content negotiation and encoding~~ encode: Enhancements to content negotiation, serving precompressed content Mar 20, 2021

francislavoie added a commit to francislavoie/caddy that referenced this pull request Mar 20, 2021

Borrow test change from caddyserver#4045

79af89f

encode: move Suffix implementation to precompressed modules

797af73

ueffel dismissed francislavoie’s stale review via 797af73 March 21, 2021 23:06

ueffel mentioned this pull request Mar 28, 2021

Add documentation for encode and file_server enhancements caddyserver/website#148

Merged

mholt approved these changes Mar 30, 2021

View reviewed changes

mholt merged commit f35a7fa into caddyserver:master Mar 30, 2021

mholt removed the under review 🧐 Review is pending before merging label Mar 30, 2021

mholt modified the milestones: 2.x, v2.4.0 Mar 30, 2021

ueffel mentioned this pull request Apr 26, 2021

Prefer order for encodings and minimum length via Caddyfile #3692

Closed

mholt mentioned this pull request Jun 7, 2021

v2: Content negotiation #2665

Closed

ueffel deleted the file_server-precompressed_v2 branch March 12, 2022 21:41

francislavoie mentioned this pull request Mar 20, 2024

Caddy seems to prefer Accept-Encodings left-to-right (prefers gzip over zstd) #6179

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

encode: Enhancements to content negotiation, serving precompressed content #4045

encode: Enhancements to content negotiation, serving precompressed content #4045

ueffel commented Mar 2, 2021 •

edited by mholt

CLAassistant commented Mar 2, 2021 •

edited

mholt left a comment

mholt Mar 2, 2021

ueffel Mar 2, 2021

francislavoie Mar 2, 2021

ueffel Mar 3, 2021

francislavoie Mar 5, 2021 •

edited

francislavoie Mar 7, 2021

ueffel Mar 7, 2021

mholt Mar 2, 2021

ueffel Mar 3, 2021

ueffel Mar 7, 2021

xdevs23 Mar 11, 2021 •

edited

ueffel Mar 13, 2021

francislavoie Mar 13, 2021

mholt Mar 2, 2021

ueffel commented Mar 2, 2021

francislavoie commented Mar 2, 2021

francislavoie commented Mar 7, 2021 •

edited

mholt left a comment

mholt Mar 11, 2021

mholt Mar 11, 2021

ueffel commented Mar 13, 2021

francislavoie commented Mar 13, 2021 •

edited

ueffel commented Mar 13, 2021 •

edited

francislavoie commented Mar 13, 2021

ueffel commented Mar 19, 2021

francislavoie left a comment

mholt left a comment

	// Precompressed is a type which returns filename suffix of precomressed
	// Precompressed is a type which returns filename suffix of precompressed

encode: Enhancements to content negotiation, serving precompressed content #4045

encode: Enhancements to content negotiation, serving precompressed content #4045

Conversation

ueffel commented Mar 2, 2021 • edited by mholt

CLAassistant commented Mar 2, 2021 • edited

mholt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francislavoie Mar 5, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xdevs23 Mar 11, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ueffel commented Mar 2, 2021

francislavoie commented Mar 2, 2021

francislavoie commented Mar 7, 2021 • edited

mholt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ueffel commented Mar 13, 2021

francislavoie commented Mar 13, 2021 • edited

ueffel commented Mar 13, 2021 • edited

francislavoie commented Mar 13, 2021

ueffel commented Mar 19, 2021

francislavoie left a comment

Choose a reason for hiding this comment

mholt left a comment

Choose a reason for hiding this comment

ueffel commented Mar 2, 2021 •

edited by mholt

CLAassistant commented Mar 2, 2021 •

edited

francislavoie Mar 5, 2021 •

edited

xdevs23 Mar 11, 2021 •

edited

francislavoie commented Mar 7, 2021 •

edited

francislavoie commented Mar 13, 2021 •

edited

ueffel commented Mar 13, 2021 •

edited