<div dir="ltr">Thanks for reviewing. <div><div>We've reconsidered that. After all, a new patch has been pushed below this.</div><div><br></div><div>>  the following set of headers will result in caching being incorrectly enabled (while it should be disabled due to Set-Cookie header):</div><div><br></div><div>Sorry, we had lost these points. So we reconsidered that it had complex rules which include not only Set-Cookie but also Vary asterisk.<br></div><div><br></div><div>> A better solution might be to save parsing results somewhere in u->headers_in, </div><div><br>Agree with you. Thus, we've introduced several variables in our new patch which stores cache valid seconds in xxxx_n and also introduced xxxx_c flags which store the whether cacheable or not in each parsing header phases instead of u->cacheable.<br><br></div></div><div>> and apply these parsing results in a separate step after parsing all headers, probably somewhere in ngx_http_upstream_process_headers()</div><div><br></div><div>Moreover, we introduced ngx_http_upstream_cache_validate_regardless_order which applies cache behavior with the parsing result in ngx_http_upstream_process_headers. Although, we also consider that these procedures could not be more easily.<br></div><div><br></div><div>changeset:  8000:7614ced0f04d<br>branch:    issue-964<br>tag:     tip<br>user:     Yugo Horie <<a href="mailto:yugo-horie@jocdn.co.jp">yugo-horie@jocdn.co.jp</a>><br>date:     Sun Jan 30 22:11:42 2022 +0900<br>files:    src/http/ngx_http_upstream.c src/http/ngx_http_upstream.h<br>description:<br>Prioritize cache behavior headers (#964)<br>user: Yugo Horie <<a href="mailto:yugo-horie@jocdn.co.jp">yugo-horie@jocdn.co.jp</a>><br>branch 'issue-964'<br>changed src/http/ngx_http_upstream.c<br>changed src/http/ngx_http_upstream.h<br><br><br>diff -r 56ead48cfe88 -r 7614ced0f04d src/http/ngx_http_upstream.c<br>--- a/src/http/ngx_http_upstream.c    Tue Jan 25 18:03:52 2022 +0300<br>+++ b/src/http/ngx_http_upstream.c    Sun Jan 30 22:11:42 2022 +0900<br>@@ -2348,6 +2348,43 @@<br>   ngx_http_upstream_send_request(r, u, 0);<br> }<br><br>+static void<br>+ngx_http_upstream_cache_validate_regardless_order(ngx_http_request_t *r, ngx_http_upstream_t *u)<br>+{<br>+   ngx_http_upstream_headers_in_t *uh = &u->headers_in;<br>+   if (uh->x_accel_expires != NULL && uh->x_accel_expires_n >= 0) {<br>+     if (uh->cookies.elts != NULL) {<br>+       u->cacheable = uh->cookies_c;<br>+     } else if (uh->vary != NULL) {<br>+       u->cacheable = uh->vary_c;<br>+     } else {<br>+       u->cacheable = uh->x_accel_expires_c;<br>+     }<br>+     r->cache->valid_sec = ngx_time() + uh->x_accel_expires_n;<br>+     r->cache->updating_sec = 0;<br>+     r->cache->error_sec = 0;<br>+   } else if (uh->cache_control.elts != NULL) {<br>+     if (uh->cookies.elts != NULL) {<br></div><div>+       u->cacheable = uh->cookies_c;<br>+     } else if (uh->vary != NULL) {<br>+       u->cacheable = uh->vary_c;<br>+     } else {<br>+       u->cacheable = uh->cache_control_c;<br>+     }<br>+     if (uh->cache_control_n > 0) {<br>+       r->cache->valid_sec = ngx_time() + uh->cache_control_n;<br>+     }<br>+   } else if (uh->expires != NULL && uh->expires_n >= 0) {<br>+     if (uh->cookies.elts != NULL) {<br>+       u->cacheable = uh->cookies_c;<br>+     } else if (uh->vary != NULL) {<br>+       u->cacheable = uh->vary_c;<br>+     } else {<br>+       u->cacheable = uh->expires_c;<br>+     }<br>+     r->cache->valid_sec = ngx_time() + uh->expires_n;<br>+   }<br>+}<br><br> static void<br> ngx_http_upstream_process_header(ngx_http_request_t *r, ngx_http_upstream_t *u)<br>@@ -2469,6 +2506,9 @@<br>       continue;<br>     }<br></div><div>+#if (NGX_HTTP_CACHE)<br>+     ngx_http_upstream_cache_validate_regardless_order(r, u);<br>+#endif<br>     break;<br>   }<br><br>@@ -4688,8 +4728,10 @@<br>   *ph = h;<br><br> #if (NGX_HTTP_CACHE)<br>+   u->headers_in.cookies_c = u->cacheable;<br>+<br>   if (!(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_SET_COOKIE)) {<br>-     u->cacheable = 0;<br>+     u->headers_in.cookies_c = 0;<br>   }<br> #endif<br><br>@@ -4727,6 +4769,8 @@<br>   u_char   *p, *start, *last;<br>   ngx_int_t  n;<br><br>+   u->headers_in.cache_control_c = u->cacheable;<br>+<br>   if (u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_CACHE_CONTROL) {<br>     return NGX_OK;<br>   }<br>@@ -4746,7 +4790,7 @@<br>     || ngx_strlcasestrn(start, last, (u_char *) "no-store", 8 - 1) != NULL<br>     || ngx_strlcasestrn(start, last, (u_char *) "private", 7 - 1) != NULL)<br>   {<br>-     u->cacheable = 0;<br>+     u->headers_in.cache_control_c = 0;<br>     return NGX_OK;<br>   }<br></div><div>@@ -4771,16 +4815,16 @@<br>         continue;<br>       }<br><br>-       u->cacheable = 0;<br>+       u->headers_in.cache_control_c = 0;<br>       return NGX_OK;<br>     }<br><br>     if (n == 0) {<br>-       u->cacheable = 0;<br>+       u->headers_in.cache_control_c = 0;<br>       return NGX_OK;<br>     }<br>-<br>     r->cache->valid_sec = ngx_time() + n;<br>+     u->headers_in.cache_control_n = n;<br>   }<br><br>   p = ngx_strlcasestrn(start, last, (u_char *) "stale-while-revalidate=",<br>@@ -4799,7 +4843,7 @@<br>         continue;<br>       }<br><br>-       u->cacheable = 0;<br>+       u->headers_in.cache_control_c = 0;<br>       return NGX_OK;<br>     }<br></div><div>@@ -4822,7 +4866,7 @@<br>         continue;<br>       }<br><br>-       u->cacheable = 0;<br>+       u->headers_in.cache_control_c = 0;<br>       return NGX_OK;<br>     }<br><br>@@ -4848,6 +4892,8 @@<br>   {<br>   time_t  expires;<br><br>+   u->headers_in.expires_c = u->cacheable;<br>+<br>   if (u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_EXPIRES) {<br>     return NGX_OK;<br>   }<br>@@ -4863,11 +4909,10 @@<br>   expires = ngx_parse_http_time(h->value.data, h->value.len);<br><br>   if (expires == NGX_ERROR || expires < ngx_time()) {<br>-     u->cacheable = 0;<br>+     u->headers_in.expires_c = 0;<br>     return NGX_OK;<br>   }<br>-<br>-   r->cache->valid_sec = expires;<br>+   u->headers_in.expires_n = expires - ngx_time();<br>   }<br> #endif<br></div><div>@@ -4890,6 +4935,8 @@<br>   size_t    len;<br>   ngx_int_t  n;<br><br>+   u->headers_in.x_accel_expires_c = u->cacheable;<br>+<br>   if (u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_XA_EXPIRES) {<br>     return NGX_OK;<br>   }<br>@@ -4906,14 +4953,14 @@<br><br>     switch (n) {<br>     case 0:<br>-       u->cacheable = 0;<br>+       u->headers_in.x_accel_expires_c = 0;<br>       /* fall through */<br><br>     case NGX_ERROR:<br>       return NGX_OK;<br><br>     default:<br>-       r->cache->valid_sec = ngx_time() + n;<br>+       u->headers_in.x_accel_expires_n = n;<br>       return NGX_OK;<br>     }<br>   }<br>@@ -4924,7 +4971,7 @@<br>   n = ngx_atoi(p, len);<br><br>   if (n != NGX_ERROR) {<br>-     r->cache->valid_sec = n;<br>+     u->headers_in.x_accel_expires_n = n - ngx_time();<br>   }<br>   }<br> #endif<br></div><div>@@ -5055,6 +5102,8 @@<br><br> #if (NGX_HTTP_CACHE)<br><br>+   u->headers_in.vary_c = u->cacheable;<br>+<br>   if (u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_VARY) {<br>     return NGX_OK;<br>   }<br>@@ -5066,7 +5115,7 @@<br>   if (h->value.len > NGX_HTTP_CACHE_VARY_LEN<br>     || (h->value.len == 1 && h->value.data[0] == '*'))<br>   {<br>-     u->cacheable = 0;<br>+     u->headers_in.vary_c = 0;<br>   }<br><br>   r->cache->vary = h->value;<br></div><div>diff -r 56ead48cfe88 -r 7614ced0f04d src/http/ngx_http_upstream.h<br>--- a/src/http/ngx_http_upstream.h    Tue Jan 25 18:03:52 2022 +0300<br>+++ b/src/http/ngx_http_upstream.h    Sun Jan 30 22:11:42 2022 +0900<br>@@ -294,6 +294,14 @@<br><br>   off_t               content_length_n;<br>   time_t              last_modified_time;<br>+   ngx_int_t             cache_control_n;<br>+   ngx_int_t             expires_n;<br>+   ngx_int_t             x_accel_expires_n;<br>+   unsigned             cache_control_c:1;<br>+   unsigned             expires_c:1;<br>+   unsigned             vary_c:1;<br>+   unsigned             cookies_c:1;<br>+   unsigned             x_accel_expires_c:1;<br><br>   unsigned             connection_close:1;<br>   unsigned             chunked:1;<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">2022年1月27日(木) 9:10 Maxim Dounin <<a href="mailto:mdounin@mdounin.ru">mdounin@mdounin.ru</a>>:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hello!<br>
<br>
On Tue, Jan 25, 2022 at 12:27:58PM +0900, Yugo Horie wrote:<br>
<br>
> changeset:Â Â 7997:86f70e48a64a<br>
> branch:Â Â Â issue-964<br>
> tag:Â Â Â Â Â tip<br>
> user:Â Â Â Â Yugo Horie <<a href="mailto:yugo-horie@jocdn.co.jp" target="_blank">yugo-horie@jocdn.co.jp</a>><br>
> date:Â Â Â Â Tue Jan 25 12:16:05 2022 +0900<br>
> files:Â Â Â Â src/http/ngx_http_upstream.c src/http/ngx_http_upstream.h<br>
> description:<br>
> Prioritize `X-Accel-Expires` than `Cache-Control` and `Expires` (#964)<br>
> <br>
> We introduce 3 flags that indicate to be overwriting cache control behavior.<br>
> <br>
> * The `overwrite_noncache` switches on the case of not to be cached when<br>
> processing `Cache-Control` and `Expires` headers from upstream.<br>
> <br>
> * The `overwrite_stale_xxx` flags also switch on when it's enabled to use<br>
> stale cache behavior on processing those headers.<br>
> <br>
> * `process_accel_expires` watches these flags, which invalidates their non-<br>
> cache<br>
> and stale behavior which had been set in other headers to prioritize<br>
> `X-Accel-Expires`.<br>
> <br>
> user: Yugo Horie <<a href="mailto:yugo-horie@jocdn.co.jp" target="_blank">yugo-horie@jocdn.co.jp</a>><br>
> changed src/http/ngx_http_upstream.c<br>
> changed src/http/ngx_http_upstream.h<br>
> <br>
> <br>
> diff -r 5d88e2bf92b3 -r 86f70e48a64a src/http/ngx_http_upstream.c<br>
> --- a/src/http/ngx_http_upstream.c   Sat Jan 22 00:28:51 2022 +0300<br>
> +++ b/src/http/ngx_http_upstream.c   Tue Jan 25 12:16:05 2022 +0900<br>
> @@ -4747,6 +4747,7 @@<br>
>Â Â Â Â Â || ngx_strlcasestrn(start, last, (u_char *) "private", 7 - 1) !=<br>
> NULL)<br>
>Â Â Â {<br>
>Â Â Â Â Â u->cacheable = 0;<br>
> +Â Â Â Â u->overwrite_noncache = 1;<br>
>Â Â Â Â Â return NGX_OK;<br>
>Â Â Â }<br>
> <br>
> @@ -4772,11 +4773,13 @@<br>
>Â Â Â Â Â Â Â }<br>
> <br>
>Â Â Â Â Â Â Â u->cacheable = 0;<br>
> +Â Â Â Â Â Â u->overwrite_noncache = 1;<br>
>Â Â Â Â Â Â Â return NGX_OK;<br>
>Â Â Â Â Â }<br>
> <br>
>Â Â Â Â Â if (n == 0) {<br>
>Â Â Â Â Â Â Â u->cacheable = 0;<br>
> +Â Â Â Â Â Â u->overwrite_noncache = 1;<br>
>Â Â Â Â Â Â Â return NGX_OK;<br>
>Â Â Â Â Â }<br>
> <br>
> @@ -4800,9 +4803,12 @@<br>
>Â Â Â Â Â Â Â }<br>
> <br>
>Â Â Â Â Â Â Â u->cacheable = 0;<br>
> +Â Â Â Â Â Â u->overwrite_noncache = 1;<br>
>Â Â Â Â Â Â Â return NGX_OK;<br>
>Â Â Â Â Â }<br>
> <br>
> +Â Â Â Â u->overwrite_stale_updating = 1;<br>
> +Â Â Â Â u->overwrite_stale_error = 1;<br>
>Â Â Â Â Â r->cache->updating_sec = n;<br>
>Â Â Â Â Â r->cache->error_sec = n;<br>
>Â Â Â }<br>
> @@ -4822,10 +4828,12 @@<br>
>Â Â Â Â Â Â Â Â Â continue;<br>
>Â Â Â Â Â Â Â }<br>
> <br>
> +Â Â Â Â Â Â u->overwrite_noncache = 1;<br>
>Â Â Â Â Â Â Â u->cacheable = 0;<br>
>Â Â Â Â Â Â Â return NGX_OK;<br>
>Â Â Â Â Â }<br>
> <br>
> +Â Â Â Â u->overwrite_stale_error = 1;<br>
>Â Â Â Â Â r->cache->error_sec = n;<br>
>Â Â Â }<br>
>Â Â Â }<br>
> @@ -4863,6 +4871,7 @@<br>
>Â Â Â expires = ngx_parse_http_time(h->value.data, h->value.len);<br>
> <br>
>Â Â Â if (expires == NGX_ERROR || expires < ngx_time()) {<br>
> +Â Â Â Â u->overwrite_noncache = 1;<br>
>Â Â Â Â Â u->cacheable = 0;<br>
>Â Â Â Â Â return NGX_OK;<br>
>Â Â Â }<br>
> @@ -4897,6 +4906,15 @@<br>
>Â Â Â if (r->cache == NULL) {<br>
>Â Â Â Â Â return NGX_OK;<br>
>Â Â Â }<br>
> +Â Â if (u->overwrite_noncache) {<br>
> +Â Â Â Â u->cacheable = 1;<br>
> +Â Â }<br>
> +Â Â if (u->overwrite_stale_updating) {<br>
> +Â Â Â Â r->cache->updating_sec = 0;<br>
> +Â Â }<br>
> +Â Â if (u->overwrite_stale_error) {<br>
> +Â Â Â Â r->cache->error_sec = 0;<br>
> +Â Â }<br>
> <br>
>Â Â Â len = h->value.len;<br>
>Â Â Â p = h->value.data;<br>
> diff -r 5d88e2bf92b3 -r 86f70e48a64a src/http/ngx_http_upstream.h<br>
> --- a/src/http/ngx_http_upstream.h   Sat Jan 22 00:28:51 2022 +0300<br>
> +++ b/src/http/ngx_http_upstream.h   Tue Jan 25 12:16:05 2022 +0900<br>
> @@ -386,6 +386,9 @@<br>
> <br>
>   unsigned             store:1;<br>
>   unsigned             cacheable:1;<br>
> +  unsigned             overwrite_noncache:1;<br>
> +  unsigned             overwrite_stale_updating:1;<br>
> +  unsigned             overwrite_stale_error:1;<br>
>   unsigned             accel:1;<br>
>   unsigned             ssl:1;<br>
>Â #if (NGX_HTTP_CACHE)<br>
<br>
Thank you for the patch.<br>
<br>
As already suggested in ticket #2309, the approach taken looks too <br>
fragile. For example, the following set of headers will result in <br>
caching being incorrectly enabled (while it should be disabled due <br>
to Set-Cookie header):<br>
<br>
Set-Cookie: foo=bar<br>
Cache-Control: no-cache<br>
X-Accel-Expires: 100<br>
<br>
A better solution might be to save parsing results somewhere in <br>
u->headers_in, and apply these parsing results in a separate <br>
step after parsing all headers, probably somewhere in <br>
ngx_http_upstream_process_headers(). Similar implementation can <br>
be seen, for example, in Content-Length and Transfer-Encoding <br>
parsing.<br>
<br>
-- <br>
Maxim Dounin<br>
<a href="http://mdounin.ru/" rel="noreferrer" target="_blank">http://mdounin.ru/</a><br>
_______________________________________________<br>
nginx-devel mailing list -- <a href="mailto:nginx-devel@nginx.org" target="_blank">nginx-devel@nginx.org</a><br>
To unsubscribe send an email to <a href="mailto:nginx-devel-leave@nginx.org" target="_blank">nginx-devel-leave@nginx.org</a><br>
</blockquote></div>