Add Vary: Accept-Encoding to responses that negotiate compression

José M. Requena Plensrequested to merge
gitlab-community/gitlab-org/gitlab-pages:jmrp-pages-vary-accept-encoding into master

What does this MR do?

Adds Vary: Accept-Encoding to the two kinds of Pages responses whose body depends on the request's Accept-Encoding. Until now the only Vary they carried was Origin, from the CORS middleware, so a shared cache or CDN in front of Pages could store the gzip or brotli body and serve it to a client that did not ask for it, or the reverse. Sites without access control are served with Cache-Control: max-age=600 and public artifacts with max-age=3600, so those responses are cacheable.

  • Static files and custom 404 pages (first commit). handleContentEncoding in internal/serving/disk/helpers.go picks the .br or .gz sibling of a file when one exists and the client accepts that encoding. It now adds Vary: Accept-Encoding before it looks, which covers both of its callers, serveFile and serveCustomFile.
  • Proxied artifacts (second commit). The proxy in internal/artifact/artifact.go forwards the client's Accept-Encoding to the GitLab API and passes on the Content-Encoding of the answer, so the body it relays can depend on that header as well: with the default Omnibus GitLab configuration, the NGINX in front of the API gzips HTML and other text responses of 250 bytes or more when the client accepts it. It now adds Vary: Accept-Encoding to every response it relays.

Both use Header().Add rather than Set, so the Vary: Origin entry stays.

Behaviour change: every static file, custom 404 page and relayed artifact now carries Vary: Accept-Encoding, including files with no .br or .gz sibling and artifacts the API did not compress. Sending it only when a sibling exists would add sibling lookups to the requests that make none today (those without Accept-Encoding, and Range requests), whose uncompressed response needs Vary too, and a second branch to test. For artifacts there is no such test at all: Pages cannot tell whether the API would have compressed the answer for another Accept-Encoding. The cost of the simpler rule is that a shared cache keys those responses by Accept-Encoding as well. I am happy to switch static files to the conditional variant if you prefer it.

Tests
  • internal/serving/disk/zip/serving_test.go: TestZip_ServeFileHTTP_VaryAcceptEncoding serves index.html from the group.gitlab-example.com fixture archive, which holds index.html, index.html.gz and index.html.br, with Accept-Encoding: gzip, br, none, and a Range request (served uncompressed with a 206), and expects exactly one Vary: Accept-Encoding. TestZip_ServeNotFoundHTTP_VaryAcceptEncoding checks the same header on the custom 404 page of the zip.gitlab.io fixture (one request with Accept-Encoding: gzip; that page has no compressed sibling).
  • test/acceptance/encodings_test.go: TestCompressedEncoding now expects Vary to hold both Origin and Accept-Encoding, which also checks that the CORS entry is kept.
  • internal/artifact/artifact_test.go: every case of TestTryMakeRequest now expects Vary: Accept-Encoding.
  • test/acceptance/artifacts_test.go: the successful cases of TestArtifactProxyRequest now expect Vary to hold Origin and Accept-Encoding.

Without the fixes, on master (8d3ddba6), the new assertions fail: the unit tests get no Vary at all and the acceptance tests get only Origin. The status and Content-Encoding checks before them already pass. With each commit, its tests pass.

Replacing Add with Set makes both acceptance tests fail (the Origin entry is lost), and adding the artifact header only when the API sent a Content-Encoding makes TestTryMakeRequest fail.

Headers before and after, and the checks I ran

gitlab-pages started against the GitLab API stub the acceptance tests use (test/gitlabstub/cmd/server) and the repository's shared/pages fixtures, then curl -sI -H 'Host: group.gitlab-example.com' -H 'Accept-Encoding: gzip' http://127.0.0.1:8090/index.html, trimmed to the headers that matter:

master                                  this branch
HTTP/1.1 200 OK                         HTTP/1.1 200 OK
Cache-Control: max-age=600              Cache-Control: max-age=600
Content-Encoding: gzip                  Content-Encoding: gzip
Vary: Origin                            Vary: Origin
                                        Vary: Accept-Encoding

The same request without Accept-Encoding returns the uncompressed body, with Vary: Origin alone on master and both entries on this branch.

On this branch, in golang:1.26 and golang:1.27, the images the CI matrix uses (go1.26.9 and go1.27.2 today), unless noted:

  • go test -short -count=1 ./... and go build -o gitlab-pages . && go test -count=1 ./test/acceptance: ok
  • go test -race -count=1 ./... with the binary built, as the race job runs make race (in golang:1.26, the job's image): ok
  • golangci-lint run ./... in golangci/golangci-lint:v2.12.2, as code_quality runs it: 0 issues.
  • the new tests on golang:1.27.1, the version in .tool-versions: ok

Closes #1214 (closed)

TODO

Merge request reports