Hello!

On Tue, Jun 07, 2022 at 07:58:36PM +0400, Sergey Kandaurov wrote:

> > On 7 Jun 2022, at 01:09, Maxim Dounin <mdounin@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 <mdounin@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 <pluknet@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-leave@nginx.org