[PATCH] Proxy: Adding proxy_cache_key emedded variable

[Thomas Peterson]

# HG changeset patch
# User Thomas Peterson <***@gmail.com>
# Date 1541231609 0
# Sat Nov 03 07:53:29 2018 +0000
# Node ID 41a499230eb674b1b3ec7cfd093f3a074f9a0d09
# Parent bddacdaaec9ef174504899f1528155f84bf60e59
Proxy: Adding proxy_cache_key emedded variable

This value being able to be set as part of response headers allows for greater
debugging of caching, and to permit analytics on cache-key distribution.

diff -r bddacdaaec9e -r 41a499230eb6 src/http/modules/ngx_http_proxy_module.c
--- a/src/http/modules/ngx_http_proxy_module.c Wed Oct 31 16:49:40 2018 +0300
+++ b/src/http/modules/ngx_http_proxy_module.c Sat Nov 03 07:53:29 2018 +0000
@@ -154,6 +154,8 @@
ngx_http_variable_value_t *v, uintptr_t data);
static ngx_int_t ngx_http_proxy_internal_chunked_variable(ngx_http_request_t *r,
ngx_http_variable_value_t *v, uintptr_t data);
+static ngx_int_t ngx_http_proxy_cache_key_variable(ngx_http_request_t *r,
+ ngx_http_variable_value_t *v, uintptr_t data);
static ngx_int_t ngx_http_proxy_rewrite_redirect(ngx_http_request_t *r,
ngx_table_elt_t *h, size_t prefix);
static ngx_int_t ngx_http_proxy_rewrite_cookie(ngx_http_request_t *r,
@@ -824,6 +826,11 @@
{ ngx_string("proxy_add_x_forwarded_for"), NULL,
ngx_http_proxy_add_x_forwarded_for_variable, 0, NGX_HTTP_VAR_NOHASH, 0 },

+#if (NGX_HTTP_CACHE)
+ { ngx_string("proxy_cache_key"), NULL,
+ ngx_http_proxy_cache_key_variable, 0, NGX_HTTP_VAR_NOHASH, 0 },
+#endif
+
#if 0
{ ngx_string("proxy_add_via"), NULL, NULL, 0, NGX_HTTP_VAR_NOHASH, 0 },
#endif
@@ -2499,6 +2506,52 @@
return NGX_OK;
}

+static ngx_int_t
+ngx_http_proxy_cache_key_variable(ngx_http_request_t *r,
+ ngx_http_variable_value_t *v, uintptr_t data)
+{
+ u_char *p;
+ size_t len;
+ ngx_str_t *key;
+ ngx_uint_t i;
+ ngx_http_cache_t *c;
+ ngx_http_upstream_t *u;
+
+ u = r->upstream;
+ if(!u->cacheable) {
+ v->not_found = 1;
+ return NGX_ERROR;
+ }
+
+ c = r->cache;
+ len = 0;
+ key = c->keys.elts;
+ for (i = 0; i < c->keys.nelts; i++) {
+ len += key[i].len;
+ }
+
+ if(len == 0) {
+ return NGX_ERROR;
+ }
+
+ p = ngx_pnalloc(r->pool, len);
+ if (p == NULL) {
+ return NGX_ERROR;
+ }
+ v->data = p;
+
+ i = 0;
+ for (i = 0; i < c->keys.nelts; i++) {
+ p = ngx_cpymem(p, key[i].data, key[i].len);
+ }
+
+ v->valid = 1;
+ v->no_cacheable = 0;
+ v->not_found = 0;
+ v->len = len;
+
+ return NGX_OK;
+}

static ngx_int_t
ngx_http_proxy_rewrite_redirect(ngx_http_request_t *r, ngx_table_elt_t *h,

[Maxim Dounin]

Hello!

Post by Thomas Peterson
# HG changeset patch
# Date 1541231609 0
# Sat Nov 03 07:53:29 2018 +0000
# Node ID 41a499230eb674b1b3ec7cfd093f3a074f9a0d09
# Parent bddacdaaec9ef174504899f1528155f84bf60e59
Proxy: Adding proxy_cache_key emedded variable
This value being able to be set as part of response headers allows for greater
debugging of caching, and to permit analytics on cache-key distribution.

What's the expected difference with using the same value as in the
proxy_cache_key directive?

Also, shouldn't this be $upstream_cache_key instead, to ensure
that a single variable can be used for all protocol-specific
modules?

[...]

[Thomas Peterson]

To answer your first point around the proxy_cache_key directive, this is to cover for the scenario where a requesting client (or further upstream client) does not have access to the server's proxy_cache_key directive and in a response wants to confirm what actual key was used, not just what the configuration declares. For my own particular deployment this is not a header I would expose to the public, but between tiers in my load balancer hierarchy and when troubleshooting cache performance.

As for renaming the variable, I'm more than happy to do so if you feel that's more appropriate. I chose its current name as the change is part of the proxy codebase, not of upstream.

Regards


On 06/11/2018, 19:15, "nginx-devel on behalf of Maxim Dounin" <nginx-devel-***@nginx.org on behalf of ***@mdounin.ru> wrote:

Hello!

Post by Thomas Peterson
# HG changeset patch
# Date 1541231609 0
# Sat Nov 03 07:53:29 2018 +0000
# Node ID 41a499230eb674b1b3ec7cfd093f3a074f9a0d09
# Parent bddacdaaec9ef174504899f1528155f84bf60e59
Proxy: Adding proxy_cache_key emedded variable
This value being able to be set as part of response headers allows for greater
debugging of caching, and to permit analytics on cache-key distribution.

What's the expected difference with using the same value as in the
proxy_cache_key directive?

Also, shouldn't this be $upstream_cache_key instead, to ensure
that a single variable can be used for all protocol-specific
modules?

[...]

--
Maxim Dounin
http://mdounin.ru/
_______________________________________________
nginx-devel mailing list
nginx-***@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel

[Maxim Dounin]

Hello!

Post by Thomas Peterson
To answer your first point around the proxy_cache_key directive,
this is to cover for the scenario where a requesting client (or
further upstream client) does not have access to the server's
proxy_cache_key directive and in a response wants to confirm
what actual key was used, not just what the configuration
declares. For my own particular deployment this is not a header
I would expose to the public, but between tiers in my load
balancer hierarchy and when troubleshooting cache performance.

The point is that there shouldn't much difference between using
something like

proxy_cache_key $scheme$proxy_host$uri$is_args$args;
add_header X-Cache-Key $proxy_cache_key;

and using

proxy_cache_key $scheme$proxy_host$uri$is_args$args;
add_header X-Cache-Key $scheme$proxy_host$uri$is_args$args;

instead - that is, using the same value directly. Or you may
even do something like

set $cache_key $scheme$proxy_host$uri$is_args$args;
proxy_cache_key $cache_key;
add_header X-Cache-Key $cache_key;

so they key used will be guaranteed to match one in the variable,
also providing an easy access.

While "wants to confirm what actual key was used, not just what
the configuration declares" might be the reason, it looks more
like a development and/or debugging task.

Could you please clarify why the above methods does not work in
your case, and how often the actual key used was different from
one declared in the configuration in your practice?

Post by Thomas Peterson
As for renaming the variable, I'm more than happy to do so if
you feel that's more appropriate. I chose its current name as
the change is part of the proxy codebase, not of upstream.

The point is that this shouldn't be in the proxy codebase.

[Thomas Peterson]

Thanks for your response, I didn't consider using a configuration
variable. However upon investigating my patch in greater detail, I have
noticed a difference in behaviour. Consider the following configuration:

  proxy_cache_key    $scheme$proxy_host$request_uri;
  set $var_cache_key $scheme$proxy_host$request_uri;

  add_header Proxy-Cache-Key $proxy_cache_key;
  add_header Var-Cache-Key   $var_cache_key;

I am expecting both of these headers to contain the same value - however
in running this in the nginx test harness does not show this, it shows
the $proxy_host value being empty when interpolated into $var_cache_key.
Attached is the test which should fail against my patch.

I'd appreciate it if you could tell me where I am going wrong.


Regards

Post by Maxim Dounin
Hello!

Post by Thomas Peterson
To answer your first point around the proxy_cache_key directive,
this is to cover for the scenario where a requesting client (or
further upstream client) does not have access to the server's
proxy_cache_key directive and in a response wants to confirm
what actual key was used, not just what the configuration
declares. For my own particular deployment this is not a header
I would expose to the public, but between tiers in my load
balancer hierarchy and when troubleshooting cache performance.

The point is that there shouldn't much difference between using
something like
proxy_cache_key $scheme$proxy_host$uri$is_args$args;
add_header X-Cache-Key $proxy_cache_key;
and using
proxy_cache_key $scheme$proxy_host$uri$is_args$args;
add_header X-Cache-Key $scheme$proxy_host$uri$is_args$args;
instead - that is, using the same value directly. Or you may
even do something like
set $cache_key $scheme$proxy_host$uri$is_args$args;
proxy_cache_key $cache_key;
add_header X-Cache-Key $cache_key;
so they key used will be guaranteed to match one in the variable,
also providing an easy access.
While "wants to confirm what actual key was used, not just what
the configuration declares" might be the reason, it looks more
like a development and/or debugging task.
Could you please clarify why the above methods does not work in
your case, and how often the actual key used was different from
one declared in the configuration in your practice?

Post by Thomas Peterson
As for renaming the variable, I'm more than happy to do so if
you feel that's more appropriate. I chose its current name as
the change is part of the proxy codebase, not of upstream.

The point is that this shouldn't be in the proxy codebase.

[Maxim Dounin]

Hello!

Post by Thomas Peterson
Thanks for your response, I didn't consider using a configuration
variable. However upon investigating my patch in greater detail, I have
  proxy_cache_key    $scheme$proxy_host$request_uri;
  set $var_cache_key $scheme$proxy_host$request_uri;
  add_header Proxy-Cache-Key $proxy_cache_key;
  add_header Var-Cache-Key   $var_cache_key;
I am expecting both of these headers to contain the same value - however
in running this in the nginx test harness does not show this, it shows
the $proxy_host value being empty when interpolated into $var_cache_key.
Attached is the test which should fail against my patch.
I'd appreciate it if you could tell me where I am going wrong.

The problem is that the $proxy_host variable is only available when
proxy starts working. Before this, it is empty, and hence "set",
which is executed during rewrite phase, will use an empty value.
Sorry for the broken example.

If you want to use the $proxy_host variable, the solution would be
to use map instead (http://nginx.org/r/map). That is, something
like this:

map $scheme $cache_key {
default $scheme$proxy_host$request_uri;
}

proxy_cache_key $cache_key;
add_header X-Cache-Key $cache_key;

The map is evaluated (and cached) once proxy_cache_key is
resolved, so there will be no problem like the one with set.

[Thomas Peterson]

Thank you for the clarification.

If the map example you gave me should cover when upstream modules are
being used, I don't think my patch will be required anymore.

Regards

Post by Maxim Dounin
Hello!

Post by Thomas Peterson
Thanks for your response, I didn't consider using a configuration
variable. However upon investigating my patch in greater detail, I have
  proxy_cache_key    $scheme$proxy_host$request_uri;
  set $var_cache_key $scheme$proxy_host$request_uri;
  add_header Proxy-Cache-Key $proxy_cache_key;
  add_header Var-Cache-Key   $var_cache_key;
I am expecting both of these headers to contain the same value - however
in running this in the nginx test harness does not show this, it shows
the $proxy_host value being empty when interpolated into $var_cache_key.
Attached is the test which should fail against my patch.
I'd appreciate it if you could tell me where I am going wrong.

The problem is that the $proxy_host variable is only available when
proxy starts working. Before this, it is empty, and hence "set",
which is executed during rewrite phase, will use an empty value.
Sorry for the broken example.
If you want to use the $proxy_host variable, the solution would be
to use map instead (http://nginx.org/r/map). That is, something
map $scheme $cache_key {
default $scheme$proxy_host$request_uri;
}
proxy_cache_key $cache_key;
add_header X-Cache-Key $cache_key;
The map is evaluated (and cached) once proxy_cache_key is
resolved, so there will be no problem like the one with set.