Possible error on revalidate in ngx_http_upstream

[Maxim Dounin]

Hello!

On Thu, Oct 07, 2021 at 06:37:11PM +0200, Jiří Setnička wrote:

> I use nginx as a proxy with enabled cache. I encountered strange 
> behavior on revalidate.
> 
> When upstream does not return any caching headers it is ok - file is 
> cached with default cachetime and on revalidate the r->cache->valid_sec 
> is updated to now + default cachetime.
> 
> Also when upstream consistently returns caching headers it is still ok - 
> file is cached according to caching headers and on revalidate the 
> r->cache->valid_sec is updated by value from 304 response caching headers.
> 
> Problem is when upstream previously returned absolute caching headers on 
> 200 response (so the file is cached according to these headers and these 
> headers are saved into cache file on disk) but later it changed its 
> behavior and on 304 response it does not return any caching headers.
> In such case, I would expect that now + default cachetime would be used 
> as the new r->cache->valid_sec, but old absolute time is used instead 
> and this yields in revalidate on each request.

Per RFC 2616
(https://datatracker.ietf.org/doc/html/rfc2616#section-10.3.5):

:    The response MUST include the following header fields:
: 
:       ...
: 
:       - Expires, Cache-Control, and/or Vary, if the field-value might
:         differ from that sent in any previous response for the same
:         variant

That is, as long as Expires is not present in the 304 response, 
per RFC 2616 it is correct to assume that the Expires in the 
original response still applies.  This is what nginx currently 
does: if there are no Expires/Cache-Control in the 304 response, 
it uses Expires/Cache-Control from the original response, and 
there are none either, tries to use proxy_cache_valid.  With this 
behaviour it is basically not possible to "remove" cache control 
headers, it is only possible to redefine them to something new 
explicitly returned.

In RFC 7232 this was changed to
(https://datatracker.ietf.org/doc/html/rfc7232#section-4.1):

:    The server generating a 304 response MUST generate any of the
:    following header fields that would have been sent in a 200 (OK)
:    response to the same request: Cache-Control, Content-Location, Date,
:    ETag, Expires, and Vary.

This implies that the original response Expires and Cache-Control 
headers should not be used at all.  With this approach, Expires 
and Cache-Control headers can be safely removed (in theory).

Switching to RFC 7232 logic might be considered, though this 
should be done with care, as it can affect various upstream 
servers which follow RFC 2616 and does not generate 
Expire/Cache-Control if these are expected to match headers 
returned in the original response.

On the other hand, I'm not sure this will help in your case, since 
Expires headers in the past are already automatically ignored, see 
below.

> In ngx_http_upstream_test_next(...) in revalidate part there is firstly 
> cache time from upstream 304 response saved to temporal variable (valid 
> = r->cache->valid_sec) and then request is reinited and 
> r->cache->valid_sec is set according to headers in the cached file.
> Problem is when value == 0 (no caching info from upstream) and there is 
> an absolute time in the cached file headers.
> 
> This patch should fix this behavior - time computed from cached file is 
> used only when it is in the future otherwise, time calculated by 
> ngx_http_file_cache_valid(...) is used.

As long as Expires is in the past, r->cache->valid_sec is not set 
and remains 0, see ngx_http_upstream_process_expires().  As such, 
suggested patch is a nop as long as standard Expires and 
Cache-Control headers are used: nginx will ignore Expires from the 
original response automatically, and will use proxy_cache_valid 
instead.

Are you trying to address X-Accel-Expires with an absolute time in 
the past?  Note that it is known to be specifically used to 
achieve the "revalidate on each request" behaviour, and the 
suggested change will break this.  (Also, changing the 
X-Accel-Expires behaviour is better to be done in 
ngx_http_upstream_process_accel_expires(), rather than indirectly, 
in 304 response handling code.)

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