[PATCH] Upstream: prioritise Cache-Control over Expires

[Vadim Fedorenko]

Hi Maxim!

Thanks for the update, I will make some production tests soon but for now I can 
say that tests from test-suit are passing which is great!

Best wishes,
Vadim

On 24.04.2022 16:42, 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 at 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;
>       }
>       }
>   #endif
> diff --git a/src/http/ngx_http_upstream.h b/src/http/ngx_http_upstream.h
> --- a/src/http/ngx_http_upstream.h
> +++ b/src/http/ngx_http_upstream.h
> @@ -297,6 +297,8 @@ typedef struct {
>   
>       unsigned                         connection_close:1;
>       unsigned                         chunked:1;
> +    unsigned                         no_cache:1;
> +    unsigned                         expired:1;
>   } ngx_http_upstream_headers_in_t;
>   
>   
>