Hello!

On Fri, Apr 22, 2022 at 08:06:08PM +0100, Vadim Fedorenko via nginx-devel wrote:

> Hi Maxim!
>
> Thanks for the feedback. As you might have already seen, I sent another version
> of this patch which addresses some of style and readability issues as well as
> X-Accel-Expires issue in v3.

Ah, sorry, I've missed that you've removed the
"u->headers_in.x_accel_expires != NULL" check in the
ngx_http_upstream_process_cache_control(). So the
stale-while-revalidate and stale-if-error Cache-Control directive
are always applied now, whenever X-Accel-Expires is present or
not.

Not sure this is a good approach: simply ignoring Cache-Control if
X-Accel-Expires is present might be a better and easier to
understand solution.

The basic idea behind X-Accel-Expires is that site administrators
might want to provide distinct caching parameters to caches they
control (and can purge or force-update when needed), and these
caching parameters are different from caching parameters sent to
external caches. Still using some caching parameters from
Cache-Control does not seem to align well with this usage.

On the other hand, as per RFC 2616/7234, the Expires header is
only expected to be ignored when "Cache-Control: max-age" is
present, but not other Cache-Control directives. So probably you
are right and this is correct approach to use.

> Regarding RFC issue. RFC 7234 Section 5.1 explicitly states that Expires MUST be
> ignored if reponse contains Cache-Control header with max-age or s-maxage
> directives. That's why I said that Nginx server configured to cache responses
> and to rely on headers to determine the freshness violates RFC. And this is true
> for a subset of absolutely legitimate responses, and it was done intended to
> actually optimize the internal processing but not because of configuration
> optimizations.

Sure. Though it is also completely legitimate to don't cache a
response, regardless of the particular headers. Switching of
caching for any reason is generally safe, and makes further RFC
7234 requirements void.

Rather, I would say that nginx violates RFC by caching a response
based on "Cache-Control: max-age" when there is an Expires header
which prevents caching, as nginx does not support the Age header
and therefore can incorrectly cache an expired response.

> Hope you will have some time to review version 3 of my patch even though it
> might not clearly apply on top of the patchset you sent couple of days ago.

Unfortunately, style and readability issues are still there.
Review below.

(Note that it might be a good idea to keep patches in a single
thread. When sending with "hg email", the "--in-reply-to" option
ensures proper threading.)

: # HG changeset patch
: # User Vadim Fedorenko <vadim.fedorenko@cdnnow.ru>
: # Date 1650406016 -10800
: # Wed Apr 20 01:06:56 2022 +0300
: # Node ID e04dac22328020cf8d8abcc4863b982b513b0c80
: # Parent a736a7a613ea6e182ff86fbadcb98bb0f8891c0b
: Upstream: prioritise X-Accel-Expire over Cache-Control and Expire

Note missing "s" in "Expires".
Style: should be a trailing dot.

: RFC7234 explicitly says that cache recipient MUST ignore Expires
: header if response includes Cache-Control header with max-age or
: s-maxage directives. Previously Cache-Control was ignored if it
: was after Expires in reply headers.

Note that this is an inaccurate and misleading statement:
Cache-Control wasn't ignored. The only specific case to address
is when the Expires header comes first and disables caching.

: At the same time documentation states that there is special header
: X-Accel-Expires that controls the behavior of nginx and should be
: prioritized over other cache-specific headers and this patch
: implements this priority.

Again, the X-Accel-Expires header is already prioritized over any
other cache control headers.

: More informantion could be found in ticket #964.

Typo: "information".

References in the "(ticket #964)" form are automatically linked in
Trac and generally recommended unless there are specific reasons
to avoid linking.

: ---
: src/http/ngx_http_upstream.c | 62 +++++++++++++++++++++++++-----------
: src/http/ngx_http_upstream.h | 6 ++++
: 2 files changed, 50 insertions(+), 18 deletions(-)
:
: diff -r a736a7a613ea -r e04dac223280 src/http/ngx_http_upstream.c
: --- a/src/http/ngx_http_upstream.c Tue Feb 08 17:35:27 2022 +0300
: +++ b/src/http/ngx_http_upstream.c Wed Apr 20 01:06:56 2022 +0300
: @@ -2350,6 +2350,32 @@
:
:
: static void
: +ngx_http_upstream_validate_cache_headers(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 &&
: + !(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_XA_EXPIRES)) {

Style issues: more than 80 characters in a line, no empty line
between variable definition and code, less than two spaces between
type and variable name, variable initialization should be on a
separate line, two spaces before "&&", operator should be on the
continuation line, "{" should be on its own line, no function
declaration, function definition should follow first usage unless
there are specific reasons to place it elsewhere.

Also, we don't generally use ngx_http_upstream_headers_in_t local
variables, but rather use direct references via u->headers_in.

: + u->cacheable = uh->x_accel_expires_c;
: + r->cache->valid_sec = uh->x_accel_expires_n;

This uses r->cache without appropriate conditional compilation
directives, so will result in compilation errors without HTTP
cache compiled in ("./configure --without-http-cache").

: + return;
: + }
: +
: + if (uh->cache_control.elts != NULL &&
: + !(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_CACHE_CONTROL)) {
: + u->cacheable = uh->cache_control_c;
: + r->cache->valid_sec = uh->cache_control_n;

This seems to be incorrect, given that something like

Cache-Control: public
Expires: <date>

is exactly equivalent to just "Expires: <date>", without
Cache-Control at all.

: + return;
: + }
: +
: + if (uh->expires != NULL &&
: + !(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_EXPIRES)) {
: + u->cacheable = uh->expires_c;
: + r->cache->valid_sec = uh->expires_n;
: + }
: +}
: +
: +
: +static void
: ngx_http_upstream_process_header(ngx_http_request_t *r, ngx_http_upstream_t *u)
: {
: ssize_t n;
: @@ -2468,6 +2494,11 @@
:
: continue;
: }
: +#if (NGX_HTTP_CACHE)

Conditional compilation here would result in a function which is
never used without HTTP cache compiled in, and hence "unused
function" warnings.

: + if (u->cacheable) {
: + ngx_http_upstream_validate_cache_headers(r, u);
: + }
: +#endif

Caching also needs to be properly adjusted somewhere in
ngx_http_upstream_intercept_errors(), as it does not call
ngx_http_upstream_process_headers().

:
: break;
: }
: @@ -4735,10 +4766,6 @@
: return NGX_OK;
: }
:
: - if (r->cache->valid_sec != 0 && u->headers_in.x_accel_expires != NULL) {
: - return NGX_OK;
: - }
: -
: start = h->value.data;
: last = start + h->value.len;
:
: @@ -4746,7 +4773,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 +4798,15 @@
: continue;
: }
:
: - u->cacheable = 0;
: return NGX_OK;
: }
:
: if (n == 0) {
: - u->cacheable = 0;
: return NGX_OK;
: }
:
: - r->cache->valid_sec = ngx_time() + n;
: + u->headers_in.cache_control_c = 1;
: + u->headers_in.cache_control_n = ngx_time() + n;
: }
:
: p = ngx_strlcasestrn(start, last, (u_char *) "stale-while-revalidate=",
: @@ -4856,18 +4882,18 @@
: return NGX_OK;
: }
:
: - if (r->cache->valid_sec != 0) {
: + expires = ngx_parse_http_time(h->value.data, h->value.len);
: +
: + if (expires == NGX_ERROR || expires < ngx_time()) {
: return NGX_OK;
: }
:
: - expires = ngx_parse_http_time(h->value.data, h->value.len);
: -
: - if (expires == NGX_ERROR || expires < ngx_time()) {
: - u->cacheable = 0;
: - return NGX_OK;
: - }
: -
: - r->cache->valid_sec = expires;
: + if (u->headers_in.expires_c) {
: + expires = ngx_min(expires, u->headers_in.expires_n);
: + }

The patch series recently posted will take care of duplicate
headers, there is no need to introduce additional ad-hoc
solutions.

: + u->headers_in.expires_n = expires;
: + u->headers_in.expires_c = 1;
: +
: }
: #endif
:
: @@ -4906,14 +4932,12 @@
:
: switch (n) {
: case 0:
: - u->cacheable = 0;
: - /* fall through */
: -
: case NGX_ERROR:
: return NGX_OK;
:
: default:
: - r->cache->valid_sec = ngx_time() + n;
: + u->headers_in.x_accel_expires_c = 1;
: + u->headers_in.x_accel_expires_n = ngx_time() + n;
: return NGX_OK;
: }
: }
: @@ -4924,7 +4948,8 @@
: n = ngx_atoi(p, len);
:
: if (n != NGX_ERROR) {
: - r->cache->valid_sec = n;
: + u->headers_in.x_accel_expires_c = 1;
: + u->headers_in.x_accel_expires_n = ngx_time() + n;
: }
: }
: #endif
: diff -r a736a7a613ea -r e04dac223280 src/http/ngx_http_upstream.h
: --- a/src/http/ngx_http_upstream.h Tue Feb 08 17:35:27 2022 +0300
: +++ b/src/http/ngx_http_upstream.h Wed Apr 20 01:06:56 2022 +0300
: @@ -294,9 +294,15 @@
:
: 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 connection_close:1;
: unsigned chunked:1;
: + unsigned cache_control_c;
: + unsigned expires_c;
: + unsigned x_accel_expires_c;
: } ngx_http_upstream_headers_in_t;
:
:

As far as I can tell, proper behaviour, assuming we parse cache
control extensions independently of X-Accel-Expires, can be
implemented by using just one flag.

Patch below, review and testing appreciated:

# HG changeset patch
# User Maxim Dounin <mdounin@mdounin.ru>
# Date 1650775261 -10800
# Sun Apr 24 07:41:01 2022 +0300
# Node ID f9c6d561b510e6008bfb4d7989f358dba33b38cd
# Parent a736a7a613ea6e182ff86fbadcb98bb0f8891c0b
Upstream: fixed X-Accel-Expires/Cache-Control/Expires handling.

Previously, if caching was disabled due to Expires in the past, nginx
failed to cache the response even if it was cacheable as per subsequently
parsed Cache-Control header (ticket #964).

Similarly, if caching was disabled due to Expires in the past,
"Cache-Control: no-cache" or "Cache-Control: max-age=0", caching was not
used if it was cacheable as per subsequently parsed X-Accel-Expires header.

Fix is to avoid disabling caching immediately after parsing Expires in
the past, but rather set a flag which is later checked by
ngx_http_upstream_process_headers() (and cleared by "Cache-Control: max-age"
and X-Accel-Expires).

Additionally, now X-Accel-Expires does not prevent parsing of cache control
extensions, notably stale-while-revalidate and stale-if-error. This
ensures that order of the X-Accel-Expires and Cache-Control headers is not
important.

Prodded by Vadim Fedorenko and Yugo Horie.

diff --git a/src/http/ngx_http_upstream.c b/src/http/ngx_http_upstream.c
--- a/src/http/ngx_http_upstream.c
+++ b/src/http/ngx_http_upstream.c
@@ -2697,6 +2697,10 @@ ngx_http_upstream_intercept_errors(ngx_h

if (r->cache) {

+ if (u->headers_in.no_cache) {
+ u->cacheable = 0;
+ }
+
if (u->cacheable) {
time_t valid;

@@ -2791,6 +2795,10 @@ ngx_http_upstream_process_headers(ngx_ht

umcf = ngx_http_get_module_main_conf(r, ngx_http_upstream_module);

+ if (u->headers_in.no_cache) {
+ u->cacheable = 0;
+ }
+
if (u->headers_in.x_accel_redirect
&& !(u->conf->ignore_headers & NGX_HTTP_UPSTREAM_IGN_XA_REDIRECT))
{
@@ -4735,18 +4743,18 @@ ngx_http_upstream_process_cache_control(
return NGX_OK;
}

- if (r->cache->valid_sec != 0 && u->headers_in.x_accel_expires != NULL) {
- return NGX_OK;
- }
-
start = h->value.data;
last = start + h->value.len;

+ if (r->cache->valid_sec != 0 && u->headers_in.x_accel_expires != NULL) {
+ goto extensions;
+ }
+
if (ngx_strlcasestrn(start, last, (u_char *) "no-cache", 8 - 1) != NULL
|| 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.no_cache = 1;
return NGX_OK;
}

@@ -4776,12 +4784,15 @@ ngx_http_upstream_process_cache_control(
}

if (n == 0) {
- u->cacheable = 0;
+ u->headers_in.no_cache = 1;
return NGX_OK;
}

r->cache->valid_sec = ngx_time() + n;
- }
+ u->headers_in.no_cache = 0;
+ }
+
+extensions:

p = ngx_strlcasestrn(start, last, (u_char *) "stale-while-revalidate=",
23 - 1);
@@ -4863,7 +4874,7 @@ ngx_http_upstream_process_expires(ngx_ht
expires = ngx_parse_http_time(h->value.data, h->value.len);

if (expires == NGX_ERROR || expires < ngx_time()) {
- u->cacheable = 0;
+ u->headers_in.no_cache = 1;
return NGX_OK;
}

@@ -4914,6 +4925,7 @@ ngx_http_upstream_process_accel_expires(

default:
r->cache->valid_sec = ngx_time() + n;
+ u->headers_in.no_cache = 0;
return NGX_OK;
}
}
@@ -4925,6 +4937,7 @@ ngx_http_upstream_process_accel_expires(

if (n != NGX_ERROR) {
r->cache->valid_sec = n;
+ u->headers_in.no_cache = 0;
}
}
#endif
diff --git a/src/http/ngx_http_upstream.h b/src/http/ngx_http_upstream.h
--- a/src/http/ngx_http_upstream.h
+++ b/src/http/ngx_http_upstream.h
@@ -297,6 +297,7 @@ typedef struct {

unsigned connection_close:1;
unsigned chunked:1;
+ unsigned no_cache:1;
} ngx_http_upstream_headers_in_t;



--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org