Prioritize `X-Accel-Expires` than `Cache-Control` and `Expires` (#964)

[Maxim Dounin]

Hello!

On Tue, Jan 25, 2022 at 12:27:58PM +0900, Yugo Horie wrote:

> changeset:   7997:86f70e48a64a
> branch:      issue-964
> tag:         tip
> user:        Yugo Horie <yugo-horie at jocdn.co.jp>
> date:        Tue Jan 25 12:16:05 2022 +0900
> files:       src/http/ngx_http_upstream.c src/http/ngx_http_upstream.h
> description:
> Prioritize `X-Accel-Expires` than `Cache-Control` and `Expires` (#964)
> 
> We introduce 3 flags that indicate to be overwriting cache control behavior.
> 
> * The `overwrite_noncache` switches on the case of not to be cached when
> processing `Cache-Control` and `Expires` headers from upstream.
> 
> * The `overwrite_stale_xxx` flags also switch on when it's enabled to use
> stale cache behavior on processing those headers.
> 
> * `process_accel_expires` watches these flags, which invalidates their non-
> cache
> and stale behavior which had been set in other headers to prioritize
> `X-Accel-Expires`.
> 
> user: Yugo Horie <yugo-horie at jocdn.co.jp>
> changed src/http/ngx_http_upstream.c
> changed src/http/ngx_http_upstream.h
> 
> 
> diff -r 5d88e2bf92b3 -r 86f70e48a64a src/http/ngx_http_upstream.c
> --- a/src/http/ngx_http_upstream.c      Sat Jan 22 00:28:51 2022 +0300
> +++ b/src/http/ngx_http_upstream.c      Tue Jan 25 12:16:05 2022 +0900
> @@ -4747,6 +4747,7 @@
>          || ngx_strlcasestrn(start, last, (u_char *) "private", 7 - 1) !=
> NULL)
>      {
>          u->cacheable = 0;
> +        u->overwrite_noncache = 1;
>          return NGX_OK;
>      }
> 
> @@ -4772,11 +4773,13 @@
>              }
> 
>              u->cacheable = 0;
> +            u->overwrite_noncache = 1;
>              return NGX_OK;
>          }
> 
>          if (n == 0) {
>              u->cacheable = 0;
> +            u->overwrite_noncache = 1;
>              return NGX_OK;
>          }
> 
> @@ -4800,9 +4803,12 @@
>              }
> 
>              u->cacheable = 0;
> +            u->overwrite_noncache = 1;
>              return NGX_OK;
>          }
> 
> +        u->overwrite_stale_updating = 1;
> +        u->overwrite_stale_error = 1;
>          r->cache->updating_sec = n;
>          r->cache->error_sec = n;
>      }
> @@ -4822,10 +4828,12 @@
>                  continue;
>              }
> 
> +            u->overwrite_noncache = 1;
>              u->cacheable = 0;
>              return NGX_OK;
>          }
> 
> +        u->overwrite_stale_error = 1;
>          r->cache->error_sec = n;
>      }
>      }
> @@ -4863,6 +4871,7 @@
>      expires = ngx_parse_http_time(h->value.data, h->value.len);
> 
>      if (expires == NGX_ERROR || expires < ngx_time()) {
> +        u->overwrite_noncache = 1;
>          u->cacheable = 0;
>          return NGX_OK;
>      }
> @@ -4897,6 +4906,15 @@
>      if (r->cache == NULL) {
>          return NGX_OK;
>      }
> +    if (u->overwrite_noncache) {
> +        u->cacheable = 1;
> +    }
> +    if (u->overwrite_stale_updating) {
> +        r->cache->updating_sec = 0;
> +    }
> +    if (u->overwrite_stale_error) {
> +        r->cache->error_sec = 0;
> +    }
> 
>      len = h->value.len;
>      p = h->value.data;
> diff -r 5d88e2bf92b3 -r 86f70e48a64a src/http/ngx_http_upstream.h
> --- a/src/http/ngx_http_upstream.h      Sat Jan 22 00:28:51 2022 +0300
> +++ b/src/http/ngx_http_upstream.h      Tue Jan 25 12:16:05 2022 +0900
> @@ -386,6 +386,9 @@
> 
>      unsigned                         store:1;
>      unsigned                         cacheable:1;
> +    unsigned                         overwrite_noncache:1;
> +    unsigned                         overwrite_stale_updating:1;
> +    unsigned                         overwrite_stale_error:1;
>      unsigned                         accel:1;
>      unsigned                         ssl:1;
>  #if (NGX_HTTP_CACHE)

Thank you for the patch.

As already suggested in ticket #2309, the approach taken looks too 
fragile.  For example, the following set of headers will result in 
caching being incorrectly enabled (while it should be disabled due 
to Set-Cookie header):

Set-Cookie: foo=bar
Cache-Control: no-cache
X-Accel-Expires: 100

A better solution might be to save parsing results somewhere in 
u->headers_in, and apply these parsing results in a separate 
step after parsing all headers, probably somewhere in 
ngx_http_upstream_process_headers().  Similar implementation can 
be seen, for example, in Content-Length and Transfer-Encoding 
parsing.

-- 
Maxim Dounin
http://mdounin.ru/