Hello!

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.
> >
> > Previously, if caching was disabled due to Expires in the past, nginx
> > failed to cache the response even if it was cacheable as per subsequently
> > parsed Cache-Control header (ticket #964).
> >
> > Similarly, if caching was disabled due to Expires in the past,
> > "Cache-Control: no-cache" or "Cache-Control: max-age=0", caching was not
> > used if it was cacheable as per subsequently parsed X-Accel-Expires header.
> >
> > Fix is to avoid disabling caching immediately after parsing Expires in
> > the past or Cache-Control, but rather set flags which are later checked by
> > ngx_http_upstream_process_headers() (and cleared by "Cache-Control: max-age"
> > and X-Accel-Expires).
> >
> > Additionally, now X-Accel-Expires does not prevent parsing of cache control
> > extensions, notably stale-while-revalidate and stale-if-error. This
> > ensures that order of the X-Accel-Expires and Cache-Control headers is not
> > important.
> >
> > Prodded by Vadim Fedorenko and Yugo Horie.
> >
> > diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
> > --- a/src/http/ngx_http_upstream.c
> > +++ b/src/http/ngx_http_upstream.c
> > @@ -2697,6 +2697,10 @@ ngx_http_upstream_intercept_errors(ngx_h
> >
> > if (r->cache) {
> >
> > + if (u->headers_in.no_cache || u->headers_in.expired) {
> > + u->cacheable = 0;
> > + }
> > +
> > if (u->cacheable) {
> > time_t valid;
> >
> > @@ -2791,6 +2795,10 @@ ngx_http_upstream_process_headers(ngx_ht
> >
> > umcf = ngx_http_get_module_main_conf(r, ngx_http_upstream_module);
> >
> > + if (u->headers_in.no_cache || u->headers_in.expired) {
> > + u->cacheable = 0;
> > + }
> > +
> > if (u->headers_in.x_accel_redirect
> > && !(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_XA_REDIRECT))
> > {
> > @@ -4735,18 +4743,18 @@ ngx_http_upstream_process_cache_control(
> > return NGX_OK;
> > }
> >
> > - if (r->cache->valid_sec != 0 && u->headers_in.x_accel_expires != NULL) {
> > - return NGX_OK;
> > - }
> > -
> > start = h->value.data;
> > last = start + h->value.len;
> >
> > + if (r->cache->valid_sec != 0 && u->headers_in.x_accel_expires != NULL) {
> > + goto extensions;
> > + }
> > +
> > if (ngx_strlcasestrn(start, last, (u_char *) "no-cache", 8 - 1) != NULL
> > || ngx_strlcasestrn(start, last, (u_char *) "no-store", 8 - 1) != NULL
> > || ngx_strlcasestrn(start, last, (u_char *) "private", 7 - 1) != NULL)
> > {
> > - u->cacheable = 0;
> > + u->headers_in.no_cache = 1;
> > return NGX_OK;
> > }
> >
> > @@ -4776,12 +4784,15 @@ ngx_http_upstream_process_cache_control(
> > }
> >
> > if (n == 0) {
> > - u->cacheable = 0;
> > + u->headers_in.no_cache = 1;
> > return NGX_OK;
> > }
> >
> > r->cache->valid_sec = ngx_time() + n;
> > - }
> > + u->headers_in.expired = 0;
> > + }
> > +
> > +extensions:
> >
> > p = ngx_strlcasestrn(start, last, (u_char *) "stale-while-revalidate=",
> > 23 - 1);
> > @@ -4863,7 +4874,7 @@ ngx_http_upstream_process_expires(ngx_ht
> > expires = ngx_parse_http_time(h->value.data, h->value.len);
> >
> > if (expires == NGX_ERROR || expires < ngx_time()) {
> > - u->cacheable = 0;
> > + u->headers_in.expired = 1;
> > return NGX_OK;
> > }
> >
> > @@ -4914,6 +4925,8 @@ ngx_http_upstream_process_accel_expires(
> >
> > default:
> > r->cache->valid_sec = ngx_time() + n;
> > + u->headers_in.no_cache = 0;
> > + u->headers_in.expired = 0;
> > return NGX_OK;
> > }
> > }
> > @@ -4925,6 +4938,8 @@ ngx_http_upstream_process_accel_expires(
> >
> > if (n != NGX_ERROR) {
> > r->cache->valid_sec = n;
> > + u->headers_in.no_cache = 0;
> > + u->headers_in.expired = 0;
>
> Handling of "X-Accel-Redirect: @0" is still missed. Per the docs,
> it should set caching time to Epoch resulting in the expired response.
> Instead, such header is handled different, and it can be overridden
> with Cache-Control/Expires headers. Probably, it should be handled
> separately and disable caching, similar to "X-Accel-Redirect: 0".
>
> 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

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