Inconsistency between url encoding/decoder when using proxy_cache_background_update

[Amotz Terem]

Description

It seems that there is some inconsistency with proxy_cache_background_update in regard to encoded "/". Which results in first request (MISS) to pass through encoded (not parsed) to the remote server. But later after expiration of the cache document, while ngnix is doing proxy_cache_background_update this time the remote server will get the request decoded (i.e different from the original request when it had a cache MISS ). This will results in cache never refreshing and STALE will be returned until the cache is cleared

Reproduce:

Use "proxy_cache_background_update on" in nginx
In remove sever set headers with max-age=30,stale-while-revalidate=300
make a request with character "%2F" (curl localhost:8000/myserver/at%2Faa) - this call will be cache-status MISS and will pass to server.
make the same call again - cache-status should be HIT
Wait for 30 seconds (same as max-age) and make the call again.

Expected:

The exact same call (as original) should go to remote server.
cache status should return STALE
on following call cache status should be HIT

Observed:

The call in remote server is different from the original call (this time the %2F is parsed to / on the remote server)
cache status return STALE -same as before
on following call cache status continue to return STALE

I'm not going into the discussion of whether nginx should or should not decode the url when proxying. I'm saying that the call of proxy_cache_background_update should be 100% identical to the original call that went through the first time ( when it was a cache MISS).

I'm sure that there are many ways to workaround if I want to. But I'm interested if you would consider this a real bug because if so I'm interested to try and fix it.

Investigation

Looking at the log I can see that in the first try it is using "http cache key" the is not decoded. But in the second try (when returning from call) it is using "http cache key" that is decoding, this demonstrate the issue from ngnix POV. (from remote server it is different request path).

I tried playing with the function ngx_http_upstream_cache_background_update and so the final call will be as original, but didn't quite manage to fix it.
Please share you opinion

[Amotz Terem]

nginx debug log

[Maxim Dounin]

Just for the record, likely the reason is r->valid_unparsed_uri being set to 0 for subrequests, and URI processing done as a result. Also, there is r == r->main check in the relevant code, probably needs to be removed (though I'm not sure why it is at all there). I've asked Roman to take a look at this.

[Roman Arutyunyan]

Please try the following patch.

# HG changeset patch
# User Roman Arutyunyan <arut@nginx.com>
# Date 1510941863 -10800
#      Fri Nov 17 21:04:23 2017 +0300
# Node ID 0b222e53f7860ef6b56afbb194a4188a2e8b470d
# Parent  9ef704d8563af4aff6817ab1c694fb40591f20b3
Inherit valid_unparsed_uri in cloned subrequests.

The value of unparsed_uri is already inherited from the parent when creating a
subrequest.  Inheriting the valid_unparsed_uri flag will make the cloned
subrequest behave consistently with the parent.  Specifically, the upstream HTTP
request and cache key, created by the proxy module, may depend directly on
unparsed_uri if valid_unparsed_uri flag is set.  Resetting the flag affects
cache background update, which may proxy a request different than its parent
and cache the result with a different key.  Similar problems exist in the slice
module.

This also requires removing the obsolete check for the main request in the
proxy module when formatting the upstream HTTP request and creating the cache
key.

diff --git a/src/http/modules/ngx_http_proxy_module.c b/src/http/modules/ngx_http_proxy_module.c
--- a/src/http/modules/ngx_http_proxy_module.c
+++ b/src/http/modules/ngx_http_proxy_module.c
@@ -1086,8 +1086,7 @@ ngx_http_proxy_create_key(ngx_http_reque
 
         return NGX_OK;
 
-    } else if (ctx->vars.uri.len == 0 && r->valid_unparsed_uri && r == r->main)
-    {
+    } else if (ctx->vars.uri.len == 0 && r->valid_unparsed_uri) {
         *key = r->unparsed_uri;
         u->uri = r->unparsed_uri;
 
@@ -1201,8 +1200,7 @@ ngx_http_proxy_create_request(ngx_http_r
     if (plcf->proxy_lengths && ctx->vars.uri.len) {
         uri_len = ctx->vars.uri.len;
 
-    } else if (ctx->vars.uri.len == 0 && r->valid_unparsed_uri && r == r->main)
-    {
+    } else if (ctx->vars.uri.len == 0 && r->valid_unparsed_uri) {
         unparsed_uri = 1;
         uri_len = r->unparsed_uri.len;
 
diff --git a/src/http/ngx_http_core_module.c b/src/http/ngx_http_core_module.c
--- a/src/http/ngx_http_core_module.c
+++ b/src/http/ngx_http_core_module.c
@@ -2365,6 +2365,7 @@ ngx_http_subrequest(ngx_http_request_t *
         sr->valid_location = r->valid_location;
         sr->content_handler = r->content_handler;
         sr->phase_handler = r->phase_handler;
+        sr->valid_unparsed_uri = r->valid_unparsed_uri;
         sr->write_event_handler = ngx_http_core_run_phases;
 
         ngx_http_update_location_config(sr);

[Amotz Terem]

Thanks Roman.
Yep, that seems to fix it :)
Do you intent to push this fix to master?

Thanks,
Amotz

[Roman Arutyunyan]

Replying to amotzte@…:

Thanks Roman.
Yep, that seems to fix it :)
Do you intent to push this fix to master?

Yes

[Roman Arutyunyan <arut@…>]

In 7168:46ebff8c6396/nginx:

Inherit valid_unparsed_uri in cloned subrequests (ticket #1430).

Inheriting this flag will make the cloned subrequest behave consistently with
the parent. Specifically, the upstream HTTP request and cache key created by
the proxy module may depend directly on unparsed_uri if valid_unparsed_uri flag
is set. Previously, the flag was zero for cloned requests, which could make
background update proxy a request different than its parent and cache the result
with a different key. For example, if client URI contained the escaped slash
character %2F, it was used as is by the proxy module in the main request, but
was unescaped in the subrequests.

Similar problems exist in the slice module.

[Roman Arutyunyan]

Thanks for reporting this.