Thanks for reviewing.
We've reconsidered that. After all, a new patch has been pushed below this.

> the following set of headers will result in caching being incorrectly
enabled (while it should be disabled due to Set-Cookie header):

Sorry, we had lost these points. So we reconsidered that it had complex
rules which include not only Set-Cookie but also Vary asterisk.

> A better solution might be to save parsing results somewhere in
u->headers_in,

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.

> and apply these parsing results in a separate step after parsing all
headers, probably somewhere in ngx_http_upstream_process_headers()

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.

changeset: 8000:7614ced0f04d
branch: issue-964
tag: tip
user: Yugo Horie <yugo-horie@jocdn.co.jp>
date: Sun Jan 30 22:11:42 2022 +0900
files: src/http/ngx_http_upstream.c src/http/ngx_http_upstream.h
description:
Prioritize cache behavior headers (#964)
user: Yugo Horie <yugo-horie@jocdn.co.jp>
branch 'issue-964'
changed src/http/ngx_http_upstream.c
changed src/http/ngx_http_upstream.h


diff -r 56ead48cfe88 -r 7614ced0f04d src/http/ngx_http_upstream.c
--- a/src/http/ngx_http_upstream.c Tue Jan 25 18:03:52 2022 +0300
+++ b/src/http/ngx_http_upstream.c Sun Jan 30 22:11:42 2022 +0900
@@ -2348,6 +2348,43 @@
ngx_http_upstream_send_request(r, u, 0);
}

+static void
+ngx_http_upstream_cache_validate_regardless_order(ngx_http_request_t *r,
ngx_http_upstream_t *u)
+{
+ ngx_http_upstream_headers_in_t *uh = &u->headers_in;
+ if (uh->x_accel_expires != NULL && uh->x_accel_expires_n >= 0) {
+ if (uh->cookies.elts != NULL) {
+ u->cacheable = uh->cookies_c;
+ } else if (uh->vary != NULL) {
+ u->cacheable = uh->vary_c;
+ } else {
+ u->cacheable = uh->x_accel_expires_c;
+ }
+ r->cache->valid_sec = ngx_time() + uh->x_accel_expires_n;
+ r->cache->updating_sec = 0;
+ r->cache->error_sec = 0;
+ } else if (uh->cache_control.elts != NULL) {
+ if (uh->cookies.elts != NULL) {
+ u->cacheable = uh->cookies_c;
+ } else if (uh->vary != NULL) {
+ u->cacheable = uh->vary_c;
+ } else {
+ u->cacheable = uh->cache_control_c;
+ }
+ if (uh->cache_control_n > 0) {
+ r->cache->valid_sec = ngx_time() + uh->cache_control_n;
+ }
+ } else if (uh->expires != NULL && uh->expires_n >= 0) {
+ if (uh->cookies.elts != NULL) {
+ u->cacheable = uh->cookies_c;
+ } else if (uh->vary != NULL) {
+ u->cacheable = uh->vary_c;
+ } else {
+ u->cacheable = uh->expires_c;
+ }
+ r->cache->valid_sec = ngx_time() + uh->expires_n;
+ }
+}

static void
ngx_http_upstream_process_header(ngx_http_request_t *r,
ngx_http_upstream_t *u)
@@ -2469,6 +2506,9 @@
continue;
}
+#if (NGX_HTTP_CACHE)
+ ngx_http_upstream_cache_validate_regardless_order(r, u);
+#endif
break;
}

@@ -4688,8 +4728,10 @@
*ph = h;

#if (NGX_HTTP_CACHE)
+ u->headers_in.cookies_c = u->cacheable;
+
if (!(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_SET_COOKIE)) {
- u->cacheable = 0;
+ u->headers_in.cookies_c = 0;
}
#endif

@@ -4727,6 +4769,8 @@
u_char *p, *start, *last;
ngx_int_t n;

+ u->headers_in.cache_control_c = u->cacheable;
+
if (u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_CACHE_CONTROL) {
return NGX_OK;
}
@@ -4746,7 +4790,7 @@
|| ngx_strlcasestrn(start, last, (u_char *) "no-store", 8 - 1) !=
NULL
|| ngx_strlcasestrn(start, last, (u_char *) "private", 7 - 1) !=
NULL)
{
- u->cacheable = 0;
+ u->headers_in.cache_control_c = 0;
return NGX_OK;
}
@@ -4771,16 +4815,16 @@
continue;
}

- u->cacheable = 0;
+ u->headers_in.cache_control_c = 0;
return NGX_OK;
}

if (n == 0) {
- u->cacheable = 0;
+ u->headers_in.cache_control_c = 0;
return NGX_OK;
}
-
r->cache->valid_sec = ngx_time() + n;
+ u->headers_in.cache_control_n = n;
}

p = ngx_strlcasestrn(start, last, (u_char *) "stale-while-revalidate=",
@@ -4799,7 +4843,7 @@
continue;
}

- u->cacheable = 0;
+ u->headers_in.cache_control_c = 0;
return NGX_OK;
}
@@ -4822,7 +4866,7 @@
continue;
}

- u->cacheable = 0;
+ u->headers_in.cache_control_c = 0;
return NGX_OK;
}

@@ -4848,6 +4892,8 @@
{
time_t expires;

+ u->headers_in.expires_c = u->cacheable;
+
if (u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_EXPIRES) {
return NGX_OK;
}
@@ -4863,11 +4909,10 @@
expires = ngx_parse_http_time(h->value.data, h->value.len);

if (expires == NGX_ERROR || expires < ngx_time()) {
- u->cacheable = 0;
+ u->headers_in.expires_c = 0;
return NGX_OK;
}
-
- r->cache->valid_sec = expires;
+ u->headers_in.expires_n = expires - ngx_time();
}
#endif
@@ -4890,6 +4935,8 @@
size_t len;
ngx_int_t n;

+ u->headers_in.x_accel_expires_c = u->cacheable;
+
if (u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_XA_EXPIRES) {
return NGX_OK;
}
@@ -4906,14 +4953,14 @@

switch (n) {
case 0:
- u->cacheable = 0;
+ u->headers_in.x_accel_expires_c = 0;
/* fall through */

case NGX_ERROR:
return NGX_OK;

default:
- r->cache->valid_sec = ngx_time() + n;
+ u->headers_in.x_accel_expires_n = n;
return NGX_OK;
}
}
@@ -4924,7 +4971,7 @@
n = ngx_atoi(p, len);

if (n != NGX_ERROR) {
- r->cache->valid_sec = n;
+ u->headers_in.x_accel_expires_n = n - ngx_time();
}
}
#endif
@@ -5055,6 +5102,8 @@

#if (NGX_HTTP_CACHE)

+ u->headers_in.vary_c = u->cacheable;
+
if (u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_VARY) {
return NGX_OK;
}
@@ -5066,7 +5115,7 @@
if (h->value.len > NGX_HTTP_CACHE_VARY_LEN
|| (h->value.len == 1 && h->value.data[0] == '*'))
{
- u->cacheable = 0;
+ u->headers_in.vary_c = 0;
}

r->cache->vary = h->value;
diff -r 56ead48cfe88 -r 7614ced0f04d src/http/ngx_http_upstream.h
--- a/src/http/ngx_http_upstream.h Tue Jan 25 18:03:52 2022 +0300
+++ b/src/http/ngx_http_upstream.h Sun Jan 30 22:11:42 2022 +0900
@@ -294,6 +294,14 @@

off_t content_length_n;
time_t last_modified_time;
+ ngx_int_t cache_control_n;
+ ngx_int_t expires_n;
+ ngx_int_t x_accel_expires_n;
+ unsigned cache_control_c:1;
+ unsigned expires_c:1;
+ unsigned vary_c:1;
+ unsigned cookies_c:1;
+ unsigned x_accel_expires_c:1;

unsigned connection_close:1;
unsigned chunked:1;

2022年1月27日(木) 9:10 Maxim Dounin <mdounin@mdounin.ru>:

> Hello!
>
> On Tue, Jan 25, 2022 at 12:27:58PM +0900, Yugo Horie wrote:
>
> > changeset: 7997:86f70e48a64a
> > branch: issue-964
> > tag: tip
> > user: Yugo Horie <yugo-horie@jocdn.co.jp>
> > date: Tue Jan 25 12:16:05 2022 +0900
> > files: src/http/ngx_http_upstream.c src/http/ngx_http_upstream.h
> > description:
> > Prioritize `X-Accel-Expires` than `Cache-Control` and `Expires` (#964)
> >
> > We introduce 3 flags that indicate to be overwriting cache control
> behavior.
> >
> > * The `overwrite_noncache` switches on the case of not to be cached when
> > processing `Cache-Control` and `Expires` headers from upstream.
> >
> > * The `overwrite_stale_xxx` flags also switch on when it's enabled to use
> > stale cache behavior on processing those headers.
> >
> > * `process_accel_expires` watches these flags, which invalidates their
> non-
> > cache
> > and stale behavior which had been set in other headers to prioritize
> > `X-Accel-Expires`.
> >
> > user: Yugo Horie <yugo-horie@jocdn.co.jp>
> > changed src/http/ngx_http_upstream.c
> > changed src/http/ngx_http_upstream.h
> >
> >
> > diff -r 5d88e2bf92b3 -r 86f70e48a64a src/http/ngx_http_upstream.c
> > --- a/src/http/ngx_http_upstream.c Sat Jan 22 00:28:51 2022 +0300
> > +++ b/src/http/ngx_http_upstream.c Tue Jan 25 12:16:05 2022 +0900
> > @@ -4747,6 +4747,7 @@
> > || ngx_strlcasestrn(start, last, (u_char *) "private", 7 - 1) !=
> > NULL)
> > {
> > u->cacheable = 0;
> > + u->overwrite_noncache = 1;
> > return NGX_OK;
> > }
> >
> > @@ -4772,11 +4773,13 @@
> > }
> >
> > u->cacheable = 0;
> > + u->overwrite_noncache = 1;
> > return NGX_OK;
> > }
> >
> > if (n == 0) {
> > u->cacheable = 0;
> > + u->overwrite_noncache = 1;
> > return NGX_OK;
> > }
> >
> > @@ -4800,9 +4803,12 @@
> > }
> >
> > u->cacheable = 0;
> > + u->overwrite_noncache = 1;
> > return NGX_OK;
> > }
> >
> > + u->overwrite_stale_updating = 1;
> > + u->overwrite_stale_error = 1;
> > r->cache->updating_sec = n;
> > r->cache->error_sec = n;
> > }
> > @@ -4822,10 +4828,12 @@
> > continue;
> > }
> >
> > + u->overwrite_noncache = 1;
> > u->cacheable = 0;
> > return NGX_OK;
> > }
> >
> > + u->overwrite_stale_error = 1;
> > r->cache->error_sec = n;
> > }
> > }
> > @@ -4863,6 +4871,7 @@
> > expires = ngx_parse_http_time(h->value.data, h->value.len);
> >
> > if (expires == NGX_ERROR || expires < ngx_time()) {
> > + u->overwrite_noncache = 1;
> > u->cacheable = 0;
> > return NGX_OK;
> > }
> > @@ -4897,6 +4906,15 @@
> > if (r->cache == NULL) {
> > return NGX_OK;
> > }
> > + if (u->overwrite_noncache) {
> > + u->cacheable = 1;
> > + }
> > + if (u->overwrite_stale_updating) {
> > + r->cache->updating_sec = 0;
> > + }
> > + if (u->overwrite_stale_error) {
> > + r->cache->error_sec = 0;
> > + }
> >
> > len = h->value.len;
> > p = h->value.data;
> > diff -r 5d88e2bf92b3 -r 86f70e48a64a src/http/ngx_http_upstream.h
> > --- a/src/http/ngx_http_upstream.h Sat Jan 22 00:28:51 2022 +0300
> > +++ b/src/http/ngx_http_upstream.h Tue Jan 25 12:16:05 2022 +0900
> > @@ -386,6 +386,9 @@
> >
> > unsigned store:1;
> > unsigned cacheable:1;
> > + unsigned overwrite_noncache:1;
> > + unsigned overwrite_stale_updating:1;
> > + unsigned overwrite_stale_error:1;
> > unsigned accel:1;
> > unsigned ssl:1;
> > #if (NGX_HTTP_CACHE)
>
> Thank you for the patch.
>
> As already suggested in ticket #2309, the approach taken looks too
> fragile. For example, the following set of headers will result in
> caching being incorrectly enabled (while it should be disabled due
> to Set-Cookie header):
>
> Set-Cookie: foo=bar
> Cache-Control: no-cache
> X-Accel-Expires: 100
>
> A better solution might be to save parsing results somewhere in
> u->headers_in, and apply these parsing results in a separate
> step after parsing all headers, probably somewhere in
> ngx_http_upstream_process_headers(). Similar implementation can
> be seen, for example, in Content-Length and Transfer-Encoding
> parsing.
>
> --
> Maxim Dounin
> http://mdounin.ru/
> _______________________________________________
> nginx-devel mailing list -- nginx-devel@nginx.org
> To unsubscribe send an email to nginx-devel-leave@nginx.org
>
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org