<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>