nginx: change etags for statically compressed files served from store #278380
Conversation
ofborg
bot
requested review from
emilazy,
fpletz,
Izorkin,
thoughtpolice,
dasJ,
ajs124 and
lblasc
|
Theoretically this change can be adopted in the upstream nginx? |
I'm not sure I see how they would agree to adding a prefix stripping logic for the path calculation :/. Do you have a thread where you asked them? |
Yes, there is a topic, but in a different language. |
|
https://trac.nginx.org/nginx/ticket/2351 - this variant is being considered by developers, |
That sounds like a more robust and generic solution. The Nixpkgs solution is more narrow in scope, so I don't really think it would work for upstream as is. |
I hope they make a universal variant :) |
Per RFC 9110, [section 8.8.1][1], different representations of the same resource should have different Etags: > A strong validator is unique across all versions of all > representations associated with a particular resource over time. > However, there is no implication of uniqueness across representations > of different resources (i.e., the same strong validator might be in > use for representations of multiple resources at the same time and > does not imply that those representations are equivalent) When serving statically compressed files (ie, when there is an existing corresponding .gz/.br/etc. file on disk), Nginx sends the Etag marked as strong. These tags should be different for each compressed format (as shown in an explicit example in section [8.8.3.3][2] of the RFC). Upstream Etags are composed of the file modification timestamp and content length, and the latter generally changes between these representations. Previous implementation of Nix-specific Etags for things served from store used the store hash. This is fine to share between different files, but it becomes a problem for statically compressed versions of the same file, as it means Nginx was serving different representations of the same resource with the same Etag, marked as strong. This patch addresses this by imitating the upstream Nginx behavior, and appending the value of content length to the store hash. [1]: https://www.rfc-editor.org/rfc/rfc9110.html#name-validator-fields [2]: https://www.rfc-editor.org/rfc/rfc9110.html#name-example-entity-tags-varying
DeeUnderscore
force-pushed
the
fix/nginx-etag
branch
from
da6c2e4
to
f124c73
Compare
|
Added some comments that hopefully make things clearer. |
|
Thank you @DeeUnderscore ! |
Description of changes
Per RFC 9110, section 8.8.1, different representations of the same resource should have different Etags:
When serving statically compressed files (ie, when there is an existing corresponding .gz/.br/etc. file on disk), Nginx sends the Etag marked as strong. These tags should be different for each compressed format (as shown in an explicit example in section 8.8.3.3 of the RFC). Upstream Etags are composed of the file modification timestamp and content length, and the latter generally changes between these representations.
Previous implementation of Nix-specific Etags for things served from store used the store hash. This is fine to share between different files, but it becomes a problem for statically compressed versions of the same file, as it means Nginx was serving different representations of the same resource with the same Etag, marked as strong.
This patch addresses this by imitating the upstream Nginx behavior, and appending the value of content length to the store hash.
Alternatives
I've considered appending something like
-gzipor-brto the Etag, but both the gzip and br static modules setContent-typeafterEtag, so this is more tricky.It might also be possible to mark the Etag as weak, if it is compressed (ie, prefix it with
W/). This is what the filter (dynamic compression) modules do, reusing the original file's Etag, marked as weak. I think using strong Etags where possible is preferable, though.Other options are dropping the Etag entirely for statically compressed resources, or leaving things as they are now. The former is the safer option. The latter generally seems to work, although it goes against the standard, so it might break in some non-obvious edge case somewhere.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 馃憤 reaction to pull requests you find important.