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

nginx: if root is in Nix store, use path's hash as ETag #48337

Merged
merged 6 commits into from Apr 18, 2019

Conversation

@yegortimoshenko
Copy link
Member

commented Oct 13, 2018

Motivation for this change

Resolves #25485. Usage example:

$ realpath /var/www
/nix/store/wnrhnnpdj3x50j5xz38zp1qxs1ygwccw-site
$ curl --head localhost
HTTP/1.1 200 OK
Server: nginx
Date: Fri, 28 Sep 2018 06:09:25 GMT
Content-Type: text/html
Content-Length: 50
Last-Modified: Thu, 01 Jan 1970 00:00:01 GMT
Connection: keep-alive
ETag: "wnrhnnpdj3x50j5xz38zp1qxs1ygwccw"
Accept-Ranges: bytes

cc @Akii @chris-martin @domenkozar

P.S. I didn't commit patch into Nixpkgs repo because that seems to be the preference, but it would be nice to have the patch reviewed. Tell me if I should commit it anyway.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

@yegortimoshenko yegortimoshenko force-pushed the transumption:201810/nginx-etag branch Oct 13, 2018

Show resolved Hide resolved pkgs/servers/http/nginx/generic.nix Outdated
@GrahamcOfBorg

This comment has been minimized.

Copy link

commented Oct 13, 2018

Success on aarch64-linux (full log)

Attempted: nginx

Partial log (click to expand)

test -d '/nix/store/r4bzjgc4y1k1f27p2g4a3660pydg6m11-nginx-1.14.0/logs' \
        || mkdir -p '/nix/store/r4bzjgc4y1k1f27p2g4a3660pydg6m11-nginx-1.14.0/logs'
make[1]: Leaving directory '/build/nginx-1.14.0'
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/r4bzjgc4y1k1f27p2g4a3660pydg6m11-nginx-1.14.0
shrinking /nix/store/r4bzjgc4y1k1f27p2g4a3660pydg6m11-nginx-1.14.0/bin/nginx
strip is /nix/store/p9akxn2sfy4wkhqdqa3li97pc6jaz3r1-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/r4bzjgc4y1k1f27p2g4a3660pydg6m11-nginx-1.14.0/bin
patching script interpreter paths in /nix/store/r4bzjgc4y1k1f27p2g4a3660pydg6m11-nginx-1.14.0
checking for references to /build in /nix/store/r4bzjgc4y1k1f27p2g4a3660pydg6m11-nginx-1.14.0...

@GrahamcOfBorg

This comment has been minimized.

Copy link

commented Oct 13, 2018

Success on x86_64-linux (full log)

Attempted: nginx

Partial log (click to expand)

test -d '/nix/store/vkdh0nnyfg6j9fdmky7x2img0vgx0jzz-nginx-1.14.0/logs' \
        || mkdir -p '/nix/store/vkdh0nnyfg6j9fdmky7x2img0vgx0jzz-nginx-1.14.0/logs'
make[1]: Leaving directory '/build/nginx-1.14.0'
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/vkdh0nnyfg6j9fdmky7x2img0vgx0jzz-nginx-1.14.0
shrinking /nix/store/vkdh0nnyfg6j9fdmky7x2img0vgx0jzz-nginx-1.14.0/bin/nginx
strip is /nix/store/vcc4svb8gy29g4pam2zja6llkbcwsyiq-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/vkdh0nnyfg6j9fdmky7x2img0vgx0jzz-nginx-1.14.0/bin
patching script interpreter paths in /nix/store/vkdh0nnyfg6j9fdmky7x2img0vgx0jzz-nginx-1.14.0
checking for references to /build in /nix/store/vkdh0nnyfg6j9fdmky7x2img0vgx0jzz-nginx-1.14.0...

@GrahamcOfBorg

This comment has been minimized.

Copy link

commented Oct 13, 2018

Success on x86_64-darwin (full log)

Attempted: nginx

Partial log (click to expand)

        || mkdir -p '/nix/store/nc4hcpmi05q8wb0f4hp56q5cn72lip4d-nginx-1.14.0/logs'
test -d '/nix/store/nc4hcpmi05q8wb0f4hp56q5cn72lip4d-nginx-1.14.0/html' \
        || cp -R html '/nix/store/nc4hcpmi05q8wb0f4hp56q5cn72lip4d-nginx-1.14.0'
test -d '/nix/store/nc4hcpmi05q8wb0f4hp56q5cn72lip4d-nginx-1.14.0/logs' \
        || mkdir -p '/nix/store/nc4hcpmi05q8wb0f4hp56q5cn72lip4d-nginx-1.14.0/logs'
make[1]: Leaving directory '/private/tmp/nix-build-nginx-1.14.0.drv-0/nginx-1.14.0'
post-installation fixup
strip is /nix/store/53nysl8bqwxihwzs1pgwka20nf8mbvlp-cctools-binutils-darwin/bin/strip
stripping (with command strip and flags -S) in /nix/store/nc4hcpmi05q8wb0f4hp56q5cn72lip4d-nginx-1.14.0/bin
patching script interpreter paths in /nix/store/nc4hcpmi05q8wb0f4hp56q5cn72lip4d-nginx-1.14.0

@grahamc

This comment has been minimized.

Copy link
Member

commented Oct 14, 2018

@grahamc

This comment has been minimized.

Copy link
Member

commented Oct 14, 2018

@yegortimoshenko

This comment has been minimized.

Copy link
Member Author

commented Oct 14, 2018

Yes, I believe so. Browsers will use ETag as a validation token to ensure content didn't change, and that maps onto Nix store semantics very well. There is also some syntax involved and that's conformant too (double quotes, optionally weak prefix but it doesn't apply here).

This patch seems to generate RFC 7232 compliant tags.

Additionally, Nginx will also properly handle this ETag and respond with 304 Not Modified if client sends HEAD request with If-None-Match: "<ETag>" header.

@yegortimoshenko yegortimoshenko force-pushed the transumption:201810/nginx-etag branch to 971f8e2 Oct 14, 2018

@yegortimoshenko

This comment has been minimized.

Copy link
Member Author

commented Oct 14, 2018

Included patch into the PR.

@yegortimoshenko

This comment has been minimized.

Copy link
Member Author

commented Oct 14, 2018

Come to think of it, there is a concern: if Nix store path contains impure symlinks outside of Nix store, and Nginx has disable_symlinks off; set, this patch will cause such symlinked path to be cached indefinitely.

Meaning, my assumption was that if root is in Nix store, it will only contain symlinks (if any) that point inside of Nix store. Is that a valid assumption to make?

Situation is complicated by the fact that ngx_http_set_etag function doesn't really have access to path of the file that it's going to serve, only to its last modified time and content length.

If this is undesirable, I think I have an idea how to make it work while keeping patch relatively small and contained: make sure that last modified time is 1970-01-01 00:00:00 before checking if root is in Nix store.

@GrahamcOfBorg

This comment has been minimized.

Copy link

commented Oct 14, 2018

Success on x86_64-linux (full log)

Attempted: nginx

Partial log (click to expand)

test -d '/nix/store/cc32dm6gg32a5wcy9lhbv6r9fvr9wmn7-nginx-1.14.0/logs' \
        || mkdir -p '/nix/store/cc32dm6gg32a5wcy9lhbv6r9fvr9wmn7-nginx-1.14.0/logs'
make[1]: Leaving directory '/build/nginx-1.14.0'
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/cc32dm6gg32a5wcy9lhbv6r9fvr9wmn7-nginx-1.14.0
shrinking /nix/store/cc32dm6gg32a5wcy9lhbv6r9fvr9wmn7-nginx-1.14.0/bin/nginx
strip is /nix/store/vcc4svb8gy29g4pam2zja6llkbcwsyiq-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/cc32dm6gg32a5wcy9lhbv6r9fvr9wmn7-nginx-1.14.0/bin
patching script interpreter paths in /nix/store/cc32dm6gg32a5wcy9lhbv6r9fvr9wmn7-nginx-1.14.0
checking for references to /build in /nix/store/cc32dm6gg32a5wcy9lhbv6r9fvr9wmn7-nginx-1.14.0...

@GrahamcOfBorg

This comment has been minimized.

Copy link

commented Oct 14, 2018

Success on x86_64-darwin (full log)

Attempted: nginx

Partial log (click to expand)

        || mkdir -p '/nix/store/cj2njbxkxqmk4j10iz0pd18maav79n8y-nginx-1.14.0/logs'
test -d '/nix/store/cj2njbxkxqmk4j10iz0pd18maav79n8y-nginx-1.14.0/html' \
        || cp -R html '/nix/store/cj2njbxkxqmk4j10iz0pd18maav79n8y-nginx-1.14.0'
test -d '/nix/store/cj2njbxkxqmk4j10iz0pd18maav79n8y-nginx-1.14.0/logs' \
        || mkdir -p '/nix/store/cj2njbxkxqmk4j10iz0pd18maav79n8y-nginx-1.14.0/logs'
make[1]: Leaving directory '/private/tmp/nix-build-nginx-1.14.0.drv-0/nginx-1.14.0'
post-installation fixup
strip is /nix/store/53nysl8bqwxihwzs1pgwka20nf8mbvlp-cctools-binutils-darwin/bin/strip
stripping (with command strip and flags -S) in /nix/store/cj2njbxkxqmk4j10iz0pd18maav79n8y-nginx-1.14.0/bin
patching script interpreter paths in /nix/store/cj2njbxkxqmk4j10iz0pd18maav79n8y-nginx-1.14.0

@GrahamcOfBorg

This comment has been minimized.

Copy link

commented Oct 14, 2018

Success on aarch64-linux (full log)

Attempted: nginx

Partial log (click to expand)

        || mkdir -p '/nix/store/f6zgnxw75r98yr723kix1a3x8v7nmdr9-nginx-1.14.0/logs'
make[1]: Leaving directory '/build/nginx-1.14.0'
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/f6zgnxw75r98yr723kix1a3x8v7nmdr9-nginx-1.14.0
shrinking /nix/store/f6zgnxw75r98yr723kix1a3x8v7nmdr9-nginx-1.14.0/bin/nginx
strip is /nix/store/p9akxn2sfy4wkhqdqa3li97pc6jaz3r1-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/f6zgnxw75r98yr723kix1a3x8v7nmdr9-nginx-1.14.0/bin
patching script interpreter paths in /nix/store/f6zgnxw75r98yr723kix1a3x8v7nmdr9-nginx-1.14.0
checking for references to /build in /nix/store/f6zgnxw75r98yr723kix1a3x8v7nmdr9-nginx-1.14.0...
/nix/store/f6zgnxw75r98yr723kix1a3x8v7nmdr9-nginx-1.14.0

@domenkozar

This comment has been minimized.

Copy link
Member

commented Oct 16, 2018

I think whatever we do here, it shouldn't be the default. We can make it convenient to turn on Nix+Nginx behaviour for those that understand what it does.

@yegortimoshenko

This comment has been minimized.

Copy link
Member Author

commented Oct 16, 2018

Why? If Nix store symlinks are handled properly, this shouldn't cause any regressions whatsoever. Since this Nginx is Nixpkgs-distributed, it seems that it would make sense for it to correctly work with Nix.

@GrahamcOfBorg

This comment has been minimized.

Copy link

commented Oct 16, 2018

Success on x86_64-linux (full log)

Attempted: nginx

Partial log (click to expand)

test -d '/nix/store/3hlqw53imrs5l7v34gj4i6f0xi2sjq09-nginx-1.14.0/logs' \
        || mkdir -p '/nix/store/3hlqw53imrs5l7v34gj4i6f0xi2sjq09-nginx-1.14.0/logs'
make[1]: Leaving directory '/build/nginx-1.14.0'
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/3hlqw53imrs5l7v34gj4i6f0xi2sjq09-nginx-1.14.0
shrinking /nix/store/3hlqw53imrs5l7v34gj4i6f0xi2sjq09-nginx-1.14.0/bin/nginx
strip is /nix/store/vcc4svb8gy29g4pam2zja6llkbcwsyiq-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/3hlqw53imrs5l7v34gj4i6f0xi2sjq09-nginx-1.14.0/bin
patching script interpreter paths in /nix/store/3hlqw53imrs5l7v34gj4i6f0xi2sjq09-nginx-1.14.0
checking for references to /build in /nix/store/3hlqw53imrs5l7v34gj4i6f0xi2sjq09-nginx-1.14.0...

@GrahamcOfBorg

This comment has been minimized.

Copy link

commented Oct 16, 2018

Success on aarch64-linux (full log)

Attempted: nginx

Partial log (click to expand)

test -d '/nix/store/gcgzsqbr0shcp55p7lcp02wm24hwwg07-nginx-1.14.0/logs' \
        || mkdir -p '/nix/store/gcgzsqbr0shcp55p7lcp02wm24hwwg07-nginx-1.14.0/logs'
make[1]: Leaving directory '/build/nginx-1.14.0'
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/gcgzsqbr0shcp55p7lcp02wm24hwwg07-nginx-1.14.0
shrinking /nix/store/gcgzsqbr0shcp55p7lcp02wm24hwwg07-nginx-1.14.0/bin/nginx
strip is /nix/store/p9akxn2sfy4wkhqdqa3li97pc6jaz3r1-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/gcgzsqbr0shcp55p7lcp02wm24hwwg07-nginx-1.14.0/bin
patching script interpreter paths in /nix/store/gcgzsqbr0shcp55p7lcp02wm24hwwg07-nginx-1.14.0
checking for references to /build in /nix/store/gcgzsqbr0shcp55p7lcp02wm24hwwg07-nginx-1.14.0...

@GrahamcOfBorg

This comment has been minimized.

Copy link

commented Oct 16, 2018

Success on x86_64-darwin (full log)

Attempted: nginx

Partial log (click to expand)

        || mkdir -p '/nix/store/pys7fcv41sbyj3qqr27lmab3k9p2cyvk-nginx-1.14.0/logs'
test -d '/nix/store/pys7fcv41sbyj3qqr27lmab3k9p2cyvk-nginx-1.14.0/html' \
        || cp -R html '/nix/store/pys7fcv41sbyj3qqr27lmab3k9p2cyvk-nginx-1.14.0'
test -d '/nix/store/pys7fcv41sbyj3qqr27lmab3k9p2cyvk-nginx-1.14.0/logs' \
        || mkdir -p '/nix/store/pys7fcv41sbyj3qqr27lmab3k9p2cyvk-nginx-1.14.0/logs'
make[1]: Leaving directory '/private/tmp/nix-build-nginx-1.14.0.drv-0/nginx-1.14.0'
post-installation fixup
strip is /nix/store/53nysl8bqwxihwzs1pgwka20nf8mbvlp-cctools-binutils-darwin/bin/strip
stripping (with command strip and flags -S) in /nix/store/pys7fcv41sbyj3qqr27lmab3k9p2cyvk-nginx-1.14.0/bin
patching script interpreter paths in /nix/store/pys7fcv41sbyj3qqr27lmab3k9p2cyvk-nginx-1.14.0

@yegortimoshenko

This comment has been minimized.

Copy link
Member Author

commented Oct 16, 2018

I've pushed two commits: one adds missing realpath error check (@gebner, thanks!) and another one fixes impure symlink handling (see #48337 (comment)).

This patch shouldn't cause any regressions anymore.

@GrahamcOfBorg

This comment has been minimized.

Copy link

commented Oct 16, 2018

Success on aarch64-linux (full log)

Attempted: nginx

Partial log (click to expand)

test -d '/nix/store/d1mpq32i9qyvpiw1x4qq3dj2k3w6r0w6-nginx-1.14.0/logs' \
        || mkdir -p '/nix/store/d1mpq32i9qyvpiw1x4qq3dj2k3w6r0w6-nginx-1.14.0/logs'
make[1]: Leaving directory '/build/nginx-1.14.0'
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/d1mpq32i9qyvpiw1x4qq3dj2k3w6r0w6-nginx-1.14.0
shrinking /nix/store/d1mpq32i9qyvpiw1x4qq3dj2k3w6r0w6-nginx-1.14.0/bin/nginx
strip is /nix/store/p9akxn2sfy4wkhqdqa3li97pc6jaz3r1-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/d1mpq32i9qyvpiw1x4qq3dj2k3w6r0w6-nginx-1.14.0/bin
patching script interpreter paths in /nix/store/d1mpq32i9qyvpiw1x4qq3dj2k3w6r0w6-nginx-1.14.0
checking for references to /build in /nix/store/d1mpq32i9qyvpiw1x4qq3dj2k3w6r0w6-nginx-1.14.0...

@GrahamcOfBorg

This comment has been minimized.

Copy link

commented Oct 16, 2018

Success on x86_64-linux (full log)

Attempted: nginx

Partial log (click to expand)

test -d '/nix/store/7nsm2sz5q5xilp0kqhzqa33a99n2nnr7-nginx-1.14.0/logs' \
        || mkdir -p '/nix/store/7nsm2sz5q5xilp0kqhzqa33a99n2nnr7-nginx-1.14.0/logs'
make[1]: Leaving directory '/build/nginx-1.14.0'
post-installation fixup
shrinking RPATHs of ELF executables and libraries in /nix/store/7nsm2sz5q5xilp0kqhzqa33a99n2nnr7-nginx-1.14.0
shrinking /nix/store/7nsm2sz5q5xilp0kqhzqa33a99n2nnr7-nginx-1.14.0/bin/nginx
strip is /nix/store/vcc4svb8gy29g4pam2zja6llkbcwsyiq-binutils-2.30/bin/strip
stripping (with command strip and flags -S) in /nix/store/7nsm2sz5q5xilp0kqhzqa33a99n2nnr7-nginx-1.14.0/bin
patching script interpreter paths in /nix/store/7nsm2sz5q5xilp0kqhzqa33a99n2nnr7-nginx-1.14.0
checking for references to /build in /nix/store/7nsm2sz5q5xilp0kqhzqa33a99n2nnr7-nginx-1.14.0...

@GrahamcOfBorg

This comment has been minimized.

Copy link

commented Oct 16, 2018

Success on x86_64-darwin (full log)

Attempted: nginx

Partial log (click to expand)

        || mkdir -p '/nix/store/cpq2ca2652p6icr2xxprlk49qqgnqj8d-nginx-1.14.0/logs'
test -d '/nix/store/cpq2ca2652p6icr2xxprlk49qqgnqj8d-nginx-1.14.0/html' \
        || cp -R html '/nix/store/cpq2ca2652p6icr2xxprlk49qqgnqj8d-nginx-1.14.0'
test -d '/nix/store/cpq2ca2652p6icr2xxprlk49qqgnqj8d-nginx-1.14.0/logs' \
        || mkdir -p '/nix/store/cpq2ca2652p6icr2xxprlk49qqgnqj8d-nginx-1.14.0/logs'
make[1]: Leaving directory '/private/tmp/nix-build-nginx-1.14.0.drv-0/nginx-1.14.0'
post-installation fixup
strip is /nix/store/53nysl8bqwxihwzs1pgwka20nf8mbvlp-cctools-binutils-darwin/bin/strip
stripping (with command strip and flags -S) in /nix/store/cpq2ca2652p6icr2xxprlk49qqgnqj8d-nginx-1.14.0/bin
patching script interpreter paths in /nix/store/cpq2ca2652p6icr2xxprlk49qqgnqj8d-nginx-1.14.0

@gebner

gebner approved these changes Oct 16, 2018

@domenkozar

This comment has been minimized.

Copy link
Member

commented Oct 17, 2018

@yegortimoshenko because we're changing expected behavior in nginx, it could have implications we can't predict. Although to be fair, current "cache forever" is pretty bad so maybe it's not that bad :)

@grahamc

This comment has been minimized.

Copy link
Member

commented Nov 30, 2018

We should probably merge this.

@aszlig

This comment has been minimized.

Copy link
Member

commented Mar 7, 2019

@domenkozar: Well, more or less, the memory leak would be great to fix.

@aszlig

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

@domenkozar: Addressed the mentioned issue, but I'll also add a test for this, so please don't merge yet :-)

yegortimoshenko and others added some commits Oct 13, 2018

nginx: if root is in Nix store, use path's hash as ETag
Resolves #25485. Usage example:

$ realpath /var/www
/nix/store/wnrhnnpdj3x50j5xz38zp1qxs1ygwccw-site
$ curl --head localhost
HTTP/1.1 200 OK
Server: nginx
Date: Fri, 28 Sep 2018 06:09:25 GMT
Content-Type: text/html
Content-Length: 50
Last-Modified: Thu, 01 Jan 1970 00:00:01 GMT
Connection: keep-alive
ETag: "wnrhnnpdj3x50j5xz38zp1qxs1ygwccw"
Accept-Ranges: bytes
nginx: Fix memleak in nix-etag patch
The original patch introduced a new "real" variable which gets populated
(and allocated) via ngx_realpath(). It's properly freed in error
conditions but it won't be freed if ngx_http_set_etag returns
successfully.

Adding another ngx_free() just before returning fixes that memory leak.

I also fixed a small indentation issue along the way.

Signed-off-by: aszlig <aszlig@nix.build>
nixos/tests/nginx: Add subtest for Nix ETag patch
This is to make sure that we get different ETag values whenever we
switch to a different store path but with the same file contents.

I've checked this against the old behaviour without the patch and it
fails as expected.

Signed-off-by: aszlig <aszlig@nix.build>

@aszlig aszlig force-pushed the transumption:201810/nginx-etag branch from ff78760 to d533285 Apr 18, 2019

@aszlig

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

@domenkozar: Added a subtest to the nginx NixOS VM test and rebased against master, because of a conflict in the test expression.

@aszlig

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

@GrahamcOfBorg test nginx

nginx/etag-patch: Use Nix store dir from build env
So far, the Nix store directory was hardcoded and if someone uses a
different Nix store directory the patch won't work. Of course, this is
pretty uncommon, but by not only substituting the store directory but
also the length of it we also save a few calls to ngx_strlen(), which
should save us a few cycles.

Signed-off-by: aszlig <aszlig@nix.build>
@aszlig

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

@domenkozar: Instead of hardcoding the Nix store directory, it's now looked up from the builder environment.

@yegortimoshenko: I've hijacked your patch by only changing the hunks necessary for the changes, please ack if you're okay with that.

@GrahamcOfBorg test nginx

So from my point of view, this should be ready to be merged after the ack from Yegor 😃

@aszlig

aszlig approved these changes Apr 18, 2019

@aszlig aszlig requested a review from gebner Apr 18, 2019

@gebner

gebner approved these changes Apr 18, 2019

@yegortimoshenko

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

@aszlig Ack! Thank you for working on this.

@domenkozar domenkozar merged commit 9bc23f3 into NixOS:master Apr 18, 2019

16 checks passed

grahamcofborg-eval ^.^!
Details
grahamcofborg-eval-check-maintainers matching changed paths to changed attrs...
Details
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A darwin-tested
Details
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A manual
Details
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./nixos/release.nix -A options
Details
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A manual
Details
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A tarball
Details
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="ofborg"; } ./pkgs/top-level/release.nix -A unstable
Details
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
nginx on aarch64-linux Success
Details
nginx on x86_64-darwin Success
Details
nginx on x86_64-linux Success
Details
tests.nginx on aarch64-linux Success
Details
tests.nginx on x86_64-linux Success
Details

yorickvP added a commit to serokell/nixpkgs that referenced this pull request Apr 23, 2019

Merge pull request NixOS#48337 from transumption/201810/nginx-etag
nginx: if root is in Nix store, use path's hash as ETag

yorickvP added a commit to serokell/nixpkgs that referenced this pull request Apr 23, 2019

Merge pull request NixOS#48337 from transumption/201810/nginx-etag
nginx: if root is in Nix store, use path's hash as ETag

aszlig added a commit to headcounter/shabitica that referenced this pull request May 15, 2019

Only disable nginx ETags if etag patch is missing
Since NixOS/nixpkgs#48337 we no longer have the
issue that we get the same ETags for different store paths but the hash
of the store path is now the ETag.

This means, that we no longer need to disable caching if the patch is
applied to nginx.

Unfortunately, the patch is only in NixOS Unstable at the moment, hence
the conditional on whether the patch exists. Even if the patch is
backported to 19.03, we'd still need it in 18.09, which we currently
still support as well.

In order to apply the patch for eg. NixOS 19.03, something like this
needs to be put in the system configuration:

  { pkgs, lib, ... }:

  {
    # ... other configuration definitions

    services.nginx.package = pkgs.nginx.overrideAttrs (drv: {
      patches = (drv.patches or []) ++ lib.singleton (fetchurl {
        url = "https://raw.githubusercontent.com/NixOS/nixpkgs/master/"
            + "pkgs/servers/http/nginx/nix-etag-1.15.4.patch";
        sha256 = "0w7sbvfrf0s20lyfr99r5d13rd97nd3c4n569n9ldy7a1r7nx019";
      });
    });

    # ... other configuration definitions
  }

Signed-off-by: aszlig <aszlig@nix.build>
@aszlig

This comment has been minimized.

Copy link
Member

commented May 23, 2019

Heads-up everyone: After testing this in a few production instances, it seems that some browsers still get cache hits for new store paths (and changed contents) for some reason. I highly suspect that it might be due to the last-modified header (as mentioned in #48337 (comment)).

Going to test this with last-modified disabled for a little while and if this is the case I think we should improve that patch by disabling last-modified if serving from a store path.

@yegortimoshenko

This comment has been minimized.

Copy link
Member Author

commented May 23, 2019

@aszlig

This comment has been minimized.

Copy link
Member

commented May 23, 2019

@yegortimoshenko: Yep, in theory it should, see also #48337 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.