> On 8 Jun 2022, at 04:57, Maxim Dounin <mdounin@mdounin.ru> wrote:
>
> 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.

I didn't concentrate on Expires and Cache-Control originally,
but rather on the preference order vs. X-Accel-Expires.
Extending it with generic tests may be a good idea, though.

>
>> @@ -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"?

Adjusted all the above, thnx.

>
>> + 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.

It's based on proxy_xar.t where headers start with capital letter.
I don't mind to replace them with lowercase though and think
it's generally good to have a simple description 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;
> }
>

Explicit headers will require rather large configuration,
but let's try and see.

> 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.

Added.

>
> 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;
}

>
>> +# 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.

Sure, don't like it as well.

>
>> +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;
> }

Indeed, this results in pretty the same number of tests,
even though the configuration is expanded quite a bit
compared to request based configuration with variables,
so it looks promising to implement that way.

Updated patch is at the end.

>
>> +
>> +# 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.

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.

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.


# 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;
+ }
+
+ 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;
+ }
+
+ 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;
+ }
+
+ 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');
+
+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".
+
+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);
+}
+
+###############################################################################

--
Sergey Kandaurov

_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org