Hello! On Tue, Jun 07, 2022 at 07:58:36PM +0400, Sergey Kandaurov wrote:
> > On 7 Jun 2022, at 01:09, Maxim Dounin <mdou...@mdounin.ru> wrote: > > > > On Mon, Jun 06, 2022 at 05:42:20PM +0400, Sergey Kandaurov wrote: > > > >> On Sun, Apr 24, 2022 at 06:42:47PM +0300, Maxim Dounin wrote: > >>> Hello! > >>> > >>> On Sun, Apr 24, 2022 at 07:55:17AM +0300, Maxim Dounin wrote: > >>> > >>> [...] > >>> > >>>> As far as I can tell, proper behaviour, assuming we parse cache > >>>> control extensions independently of X-Accel-Expires, can be > >>>> implemented by using just one flag. > >>> > >>> No, that's wrong, as with just one flag it wouldn't be possible to > >>> correctly disable caching of responses with: > >>> > >>> Cache-Control: private > >>> Cache-Control: max-age=10 > >>> > >>> So it needs at least two flags. Updated patch below, review and > >>> testing appreciated. > >>> > >>> # HG changeset patch > >>> # User Maxim Dounin <mdou...@mdounin.ru> > >>> # Date 1650814681 -10800 > >>> # Sun Apr 24 18:38:01 2022 +0300 > >>> # Node ID 940ba4317a97c72d1ee6700cbf58a543fee04c7a > >>> # Parent a736a7a613ea6e182ff86fbadcb98bb0f8891c0b > >>> Upstream: fixed X-Accel-Expires/Cache-Control/Expires handling. > >>> [..] > >> [..] > >> As the above note covers a corner case of X-Accel-Expires, > >> I believe it's fine to commit as is. > > > > This patch doesn't try to change handling of X-Accel-Expires in > > the past. > > > > And, honestly, the only documented way to _disable_ caching is to > > use 'X-Accel-Expires: 0'. The 'X-Accel-Expires: @0' only > > documented to set the time up to which the response may be cached > > to 0 seconds the Epoch. Expires and Cache-Control handling used > > to disable caching in similar situations, yet this behaviour was > > questioned more than once. X-Accel-Expires never disabled caching > > for absolute times in the past, and this is known to be used as a > > workaround to cache a pre-expires response[1][2]. > > > > Pushed to http://mdounin.ru/hg/nginx. > > > > [1] https://trac.nginx.org/nginx/ticket/1182 > > [2] https://mailman.nginx.org/pipermail/nginx-ru/2013-November/052614.html > > > > Tests: > > > # HG changeset patch > # User Sergey Kandaurov <pluk...@nginx.com> > # Date 1654617432 -14400 > # Tue Jun 07 19:57:12 2022 +0400 > # Node ID ed5fe35807f6c4853be1eb8ac429524e04ac5d67 > # Parent 4f238efded81e00c635c146df6ecb585fa0907ec > Tests: added X-Accel-Expires tests. This probably needs to be adjusted. > > diff --git a/proxy_xae.t b/proxy_xae.t > new file mode 100644 > --- /dev/null > +++ b/proxy_xae.t Should be "proxy_cache_*.t", probably "proxy_cache_control.t" would be a good enough name. Related existing tests are in proxy_cache_valid.t, though a separate file is probably more readable given the number of tests. > @@ -0,0 +1,160 @@ > +#!/usr/bin/perl > + > +# (C) Sergey Kandaurov > +# (C) Nginx, Inc. > + > +# Tests for proxy Expires / Cache-Control / X-Accel-Expires preference. > + > +############################################################################### > + > +use warnings; > +use strict; > + > +use Test::More; > + > +BEGIN { use FindBin; chdir($FindBin::Bin); } > + > +use lib 'lib'; > +use Test::Nginx; > + > +############################################################################### > + > +select STDERR; $| = 1; > +select STDOUT; $| = 1; > + > +my $t = Test::Nginx->new()->has(qw/http proxy cache rewrite map/)->plan(19); > + > +$t->write_file_expand('nginx.conf', <<'EOF'); > + > +%%TEST_GLOBALS%% > + > +daemon off; > + > +events { > +} > + > +http { > + %%TEST_GLOBALS_HTTP%% > + > + proxy_cache_path %%TESTDIR%%/cache levels=1:2 > + keys_zone=NAME:1m; > + > + map $arg_e $e { > + epoch "Thu, 01 Jan 1970 00:00:01 GMT"; > + max "Thu, 31 Dec 2037 23:55:55 GMT"; > + } > + > + server { > + listen 127.0.0.1:8080; > + server_name localhost; > + > + location / { > + proxy_pass http://127.0.0.1:8081; > + proxy_cache NAME; > + > + proxy_cache_background_update on; > + > + add_header X-Cache-Status $upstream_cache_status; This probably can be moved to server level. > + } > + > + location /ignore { > + proxy_pass http://127.0.0.1:8081/return-xae; Any reasons for "/return-xae"? > + proxy_cache NAME; > + > + proxy_ignore_headers X-Accel-Expires; > + > + add_header X-Cache-Status $upstream_cache_status; > + } > + } > + > + server { > + listen 127.0.0.1:8081; > + server_name localhost; > + > + location / { > + add_header Expires $e; > + add_header Cache-Control $arg_c; > + add_header X-Accel-Expires $arg_x; > + add_header X-Accel-Expires $arg_xx; > + > + return 204; > + } > + > + location /rev { > + add_header X-Accel-Expires $arg_x; > + add_header Cache-Control $arg_c; > + add_header Expires $e; > + > + return 204; > + } > + } > +} > + > +EOF > + > +$t->run(); > + > +############################################################################### > + > +# Cache-Control is preferred over Expires, and > +# X-Accel-Expires is preferred over both Cache-Control and Expires, > + > +like(get('/?e=max'), qr/HIT/, 'Expires'); > +like(get('/?c=max-age=60'), qr/HIT/, 'Cache-Control'); > +like(get('/?x=60'), qr/HIT/, 'X-Accel-Expires'); > +like(get('/?x=@2145902155'), qr/HIT/, 'X-Accel-Expires at'); It would be great to have test names in lowercase unless there are specific reasons to write them differently. Header names are usually written in lowercase. > + > +like(get('/ignore?x=60&ign=1'), qr/MISS/, 'X-Accel-Expires ignored'); > + > +TODO: { > +local $TODO = 'not yet' unless $t->has_version('1.23.0'); > + > +like(get('/?x=60&xx=0'), qr/HIT/, 'X-Accel-Expires duplicate'); > + > +} > + It might be better to use explicitly returned headers in appropriately named locations, at least for some specific tests like the one with duplicate X-Accel-Expires, e.g.: location /duplicate-accel-expires { add_header X-Accel-Expires 60; add_header X-Accel-Expires 0; return 204; } Another test with multiple headers to consider, given that this specific case requires two flags instead of just one: location /cache-control-no-cache-multi { add_header Cache-Control no-cache; add_header Cache-Control max-age=60; return 204; } Probably along with the simple variant, location /cache-control-no-cache-one { add_header Cache-Control "no-cache, max-age=60"; return 204; } which is expected to be exactly equivalent. Also, it might be a good idea to make sure that various other factors to disable cache, such as Set-Cookie, are not ignored when a resource is returned with Expires in the past and valid Cache-Control max-age. There were multiple attempts to submit patches which failed to handle this properly. E.g.: location /set-cookie { add_header Set-Cookie foo; add_header Cache-Control max-age=60; return 204; } > +# handling of caching parameters in Expires / Cache-Control / X-Accel-Expires > + > +like(get('/?e=max&c=no-cache'), qr/MISS/, 'Expires on CC off'); I would really like to see Cache-Control non-abbreviated, even if it requires some line wrapping. > +like(get('/?e=max&x=0'), qr/MISS/, 'Expires on X-Accel-Expires off'); > +like(get('/?c=max-age=60&x=0'), qr/MISS/, 'CC on X-Accel-Expires off'); > + > +TODO: { > +local $TODO = 'not yet' unless $t->has_version('1.23.0'); > + > +like(get('/?e=epoch&c=max-age=60'), qr/HIT/, 'Expires off CC on'); > +like(get('/?e=epoch&x=60'), qr/HIT/, 'Expires off X-Accel-Expires on'); > +like(get('/?c=no-cache&x=60'), qr/HIT/, 'CC off X-Accel-Expires on'); > + > +} Any tests with all three headers? Overall, we need to test the following basic things: - Expires is ignored when Cache-Control is present. This implies that "Cache-Control: max-age=60" enables caching even if the response has Expires header in the past, regardless of the order, i.e.: location /cache-control-after-expires { add_header Expires "Thu, 01 Jan 1970 00:00:01 GMT"; add_header Cache-Control max-age=60; return 204; } location /cache-control-before-expires { add_header Cache-Control max-age=60; add_header Expires "Thu, 01 Jan 1970 00:00:01 GMT"; return 204; } location /cache-control-no-cache-after-expires { add_header Expires "Thu, 31 Dec 2037 23:55:55 GMT"; add_header Cache-Control no-cache; return 204; } location /cache-control-no-cache-before-expires { add_header Cache-Control no-cache; add_header Expires "Thu, 31 Dec 2037 23:55:55 GMT"; return 204; } - Expires and Cache-Control are ignored when X-Accel-Expires is present. Quite similar to the above, but with Expires / Cache-Control vs. X-Accel-Expires. location /x-accel-expires-after { add_header Expires "Thu, 01 Jan 1970 00:00:01 GMT"; add_header Cache-Control no-cache; add_header X-Accel-Expires 60; return 204; } location /x-accel-expires-before { add_header X-Accel-Expires 60; add_header Expires "Thu, 01 Jan 1970 00:00:01 GMT"; add_header Cache-Control no-cache; return 204; } location /x-accel-expires-0-after { add_header Cache-Control max-age=60; add_header Expires "Thu, 31 Dec 2037 23:55:55 GMT"; add_header X-Accel-Expires 0; return 204; } location /x-accel-expires-0-before { add_header X-Accel-Expires 0; add_header Cache-Control max-age=60; add_header Expires "Thu, 31 Dec 2037 23:55:55 GMT"; return 204; } > + > +# and in the reversed order: X-Accel-Expires / Cache-Control / Expires > + > +like(get('/rev/?e=max&c=no-cache'), qr/MISS/, 'CC off Expires on'); > +like(get('/rev?e=max&x=0'), qr/MISS/, 'X-Accel-Expires off Expires on'); > +like(get('/rev?c=max-age=60&x=0'), qr/MISS/, 'X-Accel-Expires off CC on'); > + > +like(get('/rev?e=epoch&c=max-age=60'), qr/HIT/, 'CC on Expires off'); > +like(get('/rev?e=epoch&x=60'), qr/HIT/, 'X-Accel-Expires on Expires off'); > +like(get('/rev?c=no-cache&x=60'), qr/HIT/, 'X-Accel-Expires on CC off'); > + > +# Cache-Control caching extensions preserved after X-Accel-Expires > + > +TODO: { > +local $TODO = 'not yet' unless $t->has_version('1.23.0'); > + > +my $time = time() - 1; > +like(get("/rev?c=max-age=60,stale-while-revalidate=60&x=\@$time"), > + qr/STALE/, 'Cache-Control extensions after X-Accel-Expires'); Something like: X-Accel-Expires: @1 Cache-Control: stale-while-revalidate=2145902155 shouldn't require any calculations (and can be placed in an explicit location). And the opposite order probably worth checking too. Not sure if "max-age=60" is at all needed here, though probably it can catch some potential bugs. [...] -- Maxim Dounin http://mdounin.ru/ _______________________________________________ nginx-devel mailing list -- nginx-devel@nginx.org To unsubscribe send an email to nginx-devel-le...@nginx.org
