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