Hello!

On Fri, Jun 10, 2022 at 03:37:34PM +0400, Sergey Kandaurov wrote:

> > On 8 Jun 2022, at 04:57, Maxim Dounin <mdounin@mdounin.ru> wrote:
> >
> > On Tue, Jun 07, 2022 at 07:58:36PM +0400, Sergey Kandaurov wrote:

[...]

> > 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;
> > }
>
> If I got you right, it refers to earlier patches in January
> with erroneously missed Set-Cookie handling.
> I reproduced this with Expires in the past and X-Accel-Expires:
>
> location /set-cookie {
> add_header Set-Cookie foo;
> add_header Expires "Thu, 01 Jan 1970 00:00:01 GMT";
> add_header X-Accel-Expires 60;
> return 204;
> }

I believe I've seen more than one patch which missed Set-Cookie
(and Vary) handling and tried to simply set cacheable back to 1.

Using "Cache-Control: max-age=60" might be better, as RFC
explicitly says to ignore Expires if "Cache-Control: max-age" is
present, and does not define X-Accel-Expires handling.

While it is highly unlikely that at some point we'll decide that
X-Accel-Expires should be used regardless of other factors, such
as Set-Cookie and Vary, I don't see specific reasons to state the
opposite in the tests.

[...]

> >> +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.
>
> While it's better to avoid math, this one is somewhat synthetic.
> Though, I have no objection in principle, good enough for tests.
>
> >
> > Not sure if "max-age=60" is at all needed here, though probably
> > it can catch some potential bugs.
> >
>
> Since "stale-while-revalidate" is an extension, I assume that
> it should be given with some base parameter, such as "max-age".
> The assumption is supported by examples in RFC 5861.

The "stale-while-revalidate" is an extension as it extends the
Cache-Control header with an additional directive. Freshness
lifetime might be determined by other means, not just by max-age
directive in the same header field, see RFC 9111, "4.2.1.
Calculating Freshness Lifetime"
(https://datatracker.ietf.org/doc/html/rfc9111#section-4.2.1).

In this particular case, X-Accel-Expires is certainly enough to
provide freshness lifetime. In general, I see no reasons why even
a heuristic freshness lifetime wouldn't be good enough.

> This brought me a thought that we don't respect Cache-Control
> preference over Expires if given with only extensions:
>
> Expires: Thu, 31 Dec 2037 23:55:55 GMT
> Cache-Control: stale-while-revalidate=1
>
> In the above, Cache-Control doesn't set/override valid_sec,
> and as such, it is set with Expires on its own.

That's the expected behaviour. With the current code, Expires is
equivalent to the max-age time as set by "Cache-Control: max-age".
This is in line with what RFC recommends, see the link above.
Further, section "5.3. Expires" explicitly talks about
"Cache-Control: max-age", not just Cache-Control
(https://datatracker.ietf.org/doc/html/rfc9111#section-5.3).

> # HG changeset patch
> # User Sergey Kandaurov <pluknet@nginx.com>
> # Date 1654860778 -14400
> # Fri Jun 10 15:32:58 2022 +0400
> # Node ID f8e5ab69e6edd379b2ea9e0e8f1ebbab01fd9075
> # Parent 4f238efded81e00c635c146df6ecb585fa0907ec
> Tests: added proxy cache tests with upstream cache headers.
>
> diff --git a/proxy_cache_control.t b/proxy_cache_control.t
> new file mode 100644
> --- /dev/null
> +++ b/proxy_cache_control.t
> @@ -0,0 +1,276 @@
> +#!/usr/bin/perl
> +
> +# (C) Sergey Kandaurov
> +# (C) Nginx, Inc.
> +
> +# Tests for proxy Expires / Cache-Control / X-Accel-Expires.
> +
> +###############################################################################
> +
> +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/)->plan(19);
> +
> +$t->write_file_expand('nginx.conf', <<'EOF');
> +
> +%%TEST_GLOBALS%%
> +
> +daemon off;
> +
> +events {
> +}
> +
> +http {
> + %%TEST_GLOBALS_HTTP%%
> +
> + proxy_cache_path %%TESTDIR%%/cache keys_zone=NAME:1m;
> +
> + server {
> + listen 127.0.0.1:8080;
> + server_name localhost;
> +
> + add_header X-Cache-Status $upstream_cache_status;
> +
> + location / {
> + proxy_pass http://127.0.0.1:8081;
> + proxy_cache NAME;
> +
> + proxy_cache_background_update on;
> + }
> +
> + location /ignore {
> + proxy_pass http://127.0.0.1:8081;
> + proxy_cache NAME;
> +
> + proxy_ignore_headers Cache-Control Expires;
> + proxy_ignore_headers X-Accel-Expires;
> + }
> + }
> +
> + server {
> + listen 127.0.0.1:8081;
> + server_name localhost;
> +
> + location /expires {
> + add_header Expires "Thu, 31 Dec 2037 23:55:55 GMT";
> + return 204;
> + }
> +
> + location /cache-control {
> + add_header Cache-Control max-age=60;
> + return 204;

I tend to think that it would be better to remove extra spaces.
It looks weird, especially combined with "return 204;" without
extra spaces.

> + }
> +
> + location /x-accel-expires {
> + add_header X-Accel-Expires 60;
> + return 204;
> + }
> +
> + location /x-accel-expires-at {
> + add_header X-Accel-Expires @60;
> + return 204;
> + }
> +
> + location /x-accel-expires-duplicate {
> + add_header X-Accel-Expires 60;
> + add_header X-Accel-Expires 0;
> + return 204;
> + }
> +
> + location /ignore {
> + add_header Expires "Thu, 31 Dec 2037 23:55:55 GMT";
> + add_header Cache-Control max-age=60;
> + add_header X-Accel-Expires 60;
> + return 204;
> + }
> +
> + 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;
> + }

See below about before/after order.

> +
> + 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;
> + }
> +
> + 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;
> + }
> +
> + location /cache-control-no-cache-one {
> + add_header Cache-Control "no-cache, max-age=60";
> + return 204;
> + }
> +
> + location /cache-control-no-cache-multi {
> + add_header Cache-Control no-cache;
> + add_header Cache-Control max-age=60;
> + return 204;
> + }
> +
> + location /extension-before-x-accel-expires {
> + add_header Cache-Control
> + "max-age=60, stale-while-revalidate=2145902155";
> + add_header X-Accel-Expires @1;
> + return 204;

See above about "max-age=".

> + }
> +
> + location /extension-after-x-accel-expires {
> + add_header X-Accel-Expires @1;
> + add_header Cache-Control
> + "max-age=60, stale-while-revalidate=2145902155";
> + return 204;
> + }
> +
> + location /set-cookie {
> + add_header Set-Cookie foo;
> + add_header Expires "Thu, 01 Jan 1970 00:00:01 GMT";
> + add_header X-Accel-Expires 60;
> + return 204;
> + }
> + }
> +}
> +
> +EOF
> +
> +$t->run();
> +
> +###############################################################################
> +
> +# upstream cache headers work
> +
> +like(get('/expires'), qr/HIT/, 'expires');
> +like(get('/cache-control'), qr/HIT/, 'cache-control');
> +like(get('/x-accel-expires'), qr/HIT/, 'x-accel-expires');
> +like(get('/x-accel-expires-at'), qr/EXPIRED/, 'x-accel-expires at');
> +
> +TODO: {
> +local $TODO = 'not yet' unless $t->has_version('1.23.0');
> +
> +# the second header to disable cache is duplicate and ignored
> +
> +like(get('/x-accel-expires-duplicate'), qr/HIT/, 'x-accel-expires duplicate');
> +
> +}
> +
> +# with cache headers ignored, the response will be fresh
> +
> +like(get('/ignore'), qr/MISS/, 'cache headers ignored');
> +
> +# Cache-Control is preferred over Expires
> +
> +TODO: {
> +local $TODO = 'not yet' unless $t->has_version('1.23.0');
> +
> +like(get('/cache-control-after-expires'), qr/HIT/,
> + 'cache-control after expires');
> +
> +}
> +
> +like(get('/cache-control-before-expires'), qr/HIT/,
> + 'cache-control before expires');

It might make sense to use the opposite order for before/after, to
make sure TODO tests are after corresponding tests which pass in
the previous versions. YMMV though.

> +
> +like(get('/cache-control-no-cache-after-expires'), qr/MISS/,
> + 'cache-control no-cache after expires');
> +like(get('/cache-control-no-cache-before-expires'), qr/MISS/,
> + 'cache-control no-cache before expires');
> +
> +# X-Accel-Expires is preferred over both Cache-Control and Expires
> +
> +TODO: {
> +local $TODO = 'not yet' unless $t->has_version('1.23.0');
> +
> +like(get('/x-accel-expires-after'), qr/HIT/, 'x-accel-expires after');
> +
> +}
> +
> +like(get('/x-accel-expires-before'), qr/HIT/, 'x-accel-expires before');
> +
> +like(get('/x-accel-expires-0-after'), qr/MISS/, 'x-accel-expires 0 after');
> +like(get('/x-accel-expires-0-before'), qr/MISS/, 'x-accel-expires 0 before');
> +
> +# "Cache-Control: no-cache" disables caching, no matter of "max-age".

Style nit: extra dot.

> +
> +like(get('/cache-control-no-cache-one'), qr/MISS/,
> + 'cache-control no-cache');
> +like(get('/cache-control-no-cache-multi'), qr/MISS/,
> + 'cache-control no-cache multi line');
> +
> +# Set-Cookie is considered when caching with X-Accel-Expires
> +
> +like(get('/set-cookie'), qr/MISS/, 'set-cookie not cached');
> +
> +# Cache-Control extensions are preserved with X-Accel-Expires
> +
> +like(get('/extension-before-x-accel-expires'),
> + qr/STALE/, 'cache-control extensions before x-accel-expires');
> +
> +TODO: {
> +local $TODO = 'not yet' unless $t->has_version('1.23.0');
> +
> +like(get('/extension-after-x-accel-expires'),
> + qr/STALE/, 'cache-control extensions after x-accel-expires');
> +
> +}
> +
> +###############################################################################
> +
> +sub get {
> + my ($uri) = @_;
> + http_get($uri);
> + http_get($uri);
> +}
> +
> +###############################################################################

Overall looks good, see minor comments above.

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org