Opened 7 years ago

Closed 6 years ago

Last modified 6 years ago

#1382 closed defect (fixed)

proxy_cache doesn't respect no-cache from error_page

Reported by: piotrprz@… Owned by:
Priority: minor Milestone:
Component: nginx-module Version: 1.13.x
Keywords: Cc:
uname -a: Linux nginx 4.9.41-moby #1 SMP Wed Sep 6 00:05:16 UTC 2017 x86_64 GNU/Linux
nginx -V: nginx version: nginx/1.13.5
built by gcc 6.3.0 20170516 (Debian 6.3.0-18)
built with OpenSSL 1.1.0f 25 May 2017
TLS SNI support enabled
configure arguments: --prefix=/etc/nginx --sbin-path=/usr/sbin/nginx --modules-path=/usr/lib/nginx/modules --conf-path=/etc/nginx/nginx.conf --error-log-path=/var/log/nginx/error.log --http-log-path=/var/log/nginx/access.log --pid-path=/var/run/nginx.pid --lock-path=/var/run/nginx.lock --http-client-body-temp-path=/var/cache/nginx/client_temp --http-proxy-temp-path=/var/cache/nginx/proxy_temp --http-fastcgi-temp-path=/var/cache/nginx/fastcgi_temp --http-uwsgi-temp-path=/var/cache/nginx/uwsgi_temp --http-scgi-temp-path=/var/cache/nginx/scgi_temp --user=nginx --group=nginx --with-compat --with-file-aio --with-threads --with-http_addition_module --with-http_auth_request_module --with-http_dav_module --with-http_flv_module --with-http_gunzip_module --with-http_gzip_static_module --with-http_mp4_module --with-http_random_index_modue --with-http_realip_module --with-http_secure_link_module --with-http_slice_module --with-http_ssl_module --with-http_stub_status_module --with-http_sub_module --with-http_v2_module --with-mail --with-mail_ssl_module --with-stream --with-stream_realip_module --with-stream_ssl_module --with-stream_ssl_preread_module --with-cc-opt='-g -O2 -fdebug-prefix-map=/data/builder/debuild/nginx-1.13.5/debian/debuild-base/nginx-1.13.5=. -specs=/usr/share/dpkg/no-pie-compile.specs -fstack-protector-strong -Wformat -Werror=format-security -Wp,-D_FORTIFY_SOURCE=2 -fPIC' --with-ld-opt='-specs=/usr/share/dpkg/no-pie-link.specs -Wl,-z,relro -Wl,-z,now -Wl,--as-needed -pie'

Description

I am trying to cache some 404 upstream responses, but not all of them. What's more, I want to use custom 404 error pages. So I have two kinds of requests:

  • normal requests: client -> nginx -> proxy_cache -> upstream (normal processing) - standard caching proxy with proxy_intercept_errors
  • conditional requests: client -> nginx -> proxy_cache -> upstream (always 404) - get data from cache only when already cached; return 404 when data is not in cache (in that case response body doesn't matter)

It's a bit simplified - I removed all custom logic and upstream balancer code that I add in OpenResty, and put add_header to simulate what I do for 404 responses that should not be cached.

My config:

http {
    proxy_cache_path cache_temp keys_zone=cache:10m;

    server {
        listen      80;

        location = /test {
            proxy_cache cache;
            proxy_cache_key $uri;
            proxy_pass http://localhost:8080;
            proxy_cache_valid 404 5s;

            add_header X-Cache-Status $upstream_cache_status always;
            add_header X-Upstream-Status $upstream_status always;

            proxy_intercept_errors on;
            error_page 404 /404.htm; # comment out this line and caching will be properly skipped
        }

        location = /404.htm {
            add_header X-Cache-Status $upstream_cache_status always;
            add_header X-Upstream-Status $upstream_status always;

            add_header X-Accel-Expires "0" always;
            add_header Cache-Control "no-cache" always;
            add_header Expires "0" always;
            add_header Via "*" always;

            return 404;
        }
    }

    server {
        listen 8080;

        add_header X-Accel-Expires "0" always;
        add_header Cache-Control "no-cache" always;
        add_header Expires "0" always;
        add_header Via "*" always;
        return 404;
    }
}

Now, according to my knowledge, when I issue GET http://localhost/test the request should go like this:

  1. Request is sent to :8080, which returns 404 error
  2. proxy_intercept_errors + error_page replace response with error page defined in /404.htm, which is fetched via internal subrequest
  3. Cache-Control etc. should be respected and response should not be cached.

I already moved my special 404 processing to another internal location, so I went around this issue, but I'm reporting it anyway because it seems like a bug to me.

Change History (5)

comment:1 by Maxim Dounin, 7 years ago

Status: newaccepted

Thanks for reporting this. Caching of intercepted error responses uses a special code path, and it uses proxy_cache_valid time configured regardless of any cache control parameters returned in the response. This is how it was introduced in revision ea908f6ae499. I don't think the behaviour is intentional though, this looks like a bug.

The following patch should fix it:

# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1505835337 -10800
#      Tue Sep 19 18:35:37 2017 +0300
# Node ID 48899ab9d89d654b1664233b9fa907f12667dd77
# Parent  bd2f5843e8cd968715d711b757a7210e26d24465
Cache: fixed caching of intercepted errors (ticket #1382).

When caching intercepted errors, previous behaviour was to use
proxy_cache_valid times specified, regardless of various cache control
headers present in the response.  Fix is to check u->cacheable and
use u->cache->valid_sec as set by various cache control response headers,
similar to how we do this in the normal caching code path.

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
@@ -2518,13 +2518,23 @@ ngx_http_upstream_intercept_errors(ngx_h
 #if (NGX_HTTP_CACHE)
 
             if (r->cache) {
-                time_t  valid;
-
-                valid = ngx_http_file_cache_valid(u->conf->cache_valid, status);
-
-                if (valid) {
-                    r->cache->valid_sec = ngx_time() + valid;
-                    r->cache->error = status;
+
+                if (u->cacheable) {
+                    time_t  valid;
+
+                    valid = r->cache->valid_sec;
+
+                    if (valid == 0) {
+                        valid = ngx_http_file_cache_valid(u->conf->cache_valid,
+                                                          status);
+                        if (valid) {
+                            r->cache->valid_sec = ngx_time() + valid;
+                        }
+                    }
+
+                    if (valid) {
+                        r->cache->error = status;
+                    }
                 }
 
                 ngx_http_file_cache_free(r->cache, u->pipe->temp_file);

comment:2 by piotrprz@…, 6 years ago

Thanks, with this patch it works correctly.

comment:3 by Maxim Dounin <mdounin@…>, 6 years ago

In 7117:dbd77a638eb7/nginx:

Cache: fixed caching of intercepted errors (ticket #1382).

When caching intercepted errors, previous behaviour was to use
proxy_cache_valid times specified, regardless of various cache control
headers present in the response. Fix is to check u->cacheable and
use u->cache->valid_sec as set by various cache control response headers,
similar to how we do this in the normal caching code path.

comment:4 by Maxim Dounin, 6 years ago

Resolution: fixed
Status: acceptedclosed

Patch committed, thanks.

comment:5 by Sergey Kandaurov <pluknet@…>, 6 years ago

In 1225:66426ca24671/nginx-tests:

Tests: intercepted errors with cache control (ticket #1382).

Note: See TracTickets for help on using tickets.