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