[PATCH] reset r->cache->valid_sec for following the redirect

[Maxim Dounin]

Hello!

On Wed, May 08, 2019 at 02:30:28PM +0900, Sangdeuk Kwon wrote:

> # HG changeset patch
> # User Sangdeuk Kwon <dark264sh at gmail.com>
> # Date 1557288704 -32400
> #      Wed May 08 13:11:44 2019 +0900
> # Node ID c4e1be885640a8302dcbf97162b3cbdc21710a84
> # Parent  16a1adadf43751f59257ba419f6bacd530dd19d3
> reset r->cache->valid_sec for following the redirect
> 
> this config:
> 
> http {
>     proxy_intercept_errors on;
> 
>     proxy_cache_path /cache0 levels=1:2 keys_zone=STATIC:10m max_size=10g
> inactive=60m use_temp_path=off;
>     proxy_cache_key "$host$uri";
>     proxy_cache STATIC;
> 
>     upstream up_servers {
>         server 127.0.0.2:8080;
>     }
> 
>     server {
>         listen 127.0.0.1:8000;
> 
>         location / {
>             proxy_pass http://up_servers;
>             proxy_set_header Host $host;
>             proxy_http_version 1.1;
>             add_header X-Cached $upstream_cache_status;
> 
>             proxy_intercept_errors on;
>             error_page 301 302 303 307 308 = @handle_redirects;
>         }
> 
>         location @handle_redirects {
>             set $orig_loc $upstream_http_location;
>             proxy_pass $orig_loc;
>         }
>     }
> }
> 
> When origin server does not send cache time header (max-age or expires),
> nginx can follow the redirect with location header.
> But, if origin server sends cache time header,
> nginx can't follow the redirect and nginx send 302 response without
> location header to a client.
> So the client also can't follow the redirect because of absenting location
> header.
> 
> diff -r 16a1adadf437 -r c4e1be885640 src/http/ngx_http_upstream.c
> --- a/src/http/ngx_http_upstream.c  Wed Apr 24 16:38:56 2019 +0300
> +++ b/src/http/ngx_http_upstream.c  Wed May 08 13:11:44 2019 +0900
> @@ -2645,6 +2645,15 @@
>                      if (valid) {
>                          r->cache->error = status;
>                      }
> +
> +                    if (status == NGX_HTTP_MOVED_PERMANENTLY
> +                        || status == NGX_HTTP_MOVED_TEMPORARILY
> +                        || status == NGX_HTTP_SEE_OTHER
> +                        || status == NGX_HTTP_TEMPORARY_REDIRECT
> +                        || status == NGX_HTTP_PERMANENT_REDIRECT)
> +                    {
> +                        r->cache->valid_sec = 0;
> +                    }
>                  }
> 
>                  ngx_http_file_cache_free(r->cache, u->pipe->temp_file);

Thanks for the patch.

This seems to be a negative effect of dbd77a638eb7 (nginx 1.13.6, 
http://hg.nginx.org/nginx/rev/dbd77a638eb7), which manifests 
itself in the configuration in question.  As long as the response 
is cached, the particular configuration cannot work, since 
$upstream_http_location is not available.

An obvious workaround is to ensure that such configuration do not 
allow caching of responses, for example, by using "proxy_cache 
off;" in such locations.  

Using "r->cache->valid_sec = 0;" based on the status code, 
however, looks wrong to me.  First of all, it doesn't actually 
disable caching, but rather preserves the existing response 
validity as is - and this may result in more or less arbitrary 
behaviour.  Second, it doesn't clear to me if this is something to 
be done unconditionally - there can be configuration when caching 
is desired, despite the fact that $upstream_http_location will not 
be available.

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