Re: [PATCH] Added merge inheritance to proxy_set_header
Hello Roman,
Thank you for the review and feedback.
On Mon, 27 Nov 2023 16:18:21 +0400
Roman Arutyunyan wrote:
> Hi Jordan,
>
> On Thu, Nov 23, 2023 at 02:52:55PM +, J Carter wrote:
> > # HG changeset patch
> > # User J Carter
> > # Date 1700751016 0
> > # Thu Nov 23 14:50:16 2023 +
> > # Node ID cc79903ca02eff8aa87238a0f801f8070425d9ab
> > # Parent 7ec761f0365f418511e30b82e9adf80bc56681df
> > Added merge inheritance to proxy_set_header
> >
> > This patch introduces 'inherit' argument to proxy_set_header
> > directive. When marked as such it will be merge inherited into child
> > contexts regardless of the presence of other proxy_set_header
> > directives within those contexts.
> >
> > This patch also introduces the 'proxy_set_header_inherit' directive
> > which blocks the merge inheritance in receiving contexts when set to off.
> >
> > The purpose of the added mechanics is to reduce repetition within the
> > nginx configuration for universally set (or boilerplate) request
> > headers, while maintaining flexibility to set additional headers for
> > specific paths.
> >
> > There is no change in behavior for existing configurations.
> >
> > Example below:
> >
> > server {
> >
> > ...
> >
> > proxy_set_header Host $host inherit;
> > proxy_set_header Connection "" inherit;
> > proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for inherit;
> >
> > location /1/ {
> > #sets this header in addition to server context ones.
> > proxy_set_header customHeader1 "customvalue1";
> > proxy_pass http://backend1;
> > }
> >
> > location /2/ {
> > #sets this header in addition to server context ones.
> > proxy_set_header customheader2 "customValue2";
> > proxy_pass http://backend1;
> > }
> >
> > location /3/ {
> > #no location specific headers, only server context ones set.
> > proxy_pass http://backend1;
> > }
> >
> > location /special/ {
> > #Blocks merge inheritance from server context.
> > proxy_set_header_inherit off;
> > proxy_set_header Host "notserverthost.co";
> > proxy_set_header Connection "";
> > proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
> > proxy_pass http://backend2;
> > }
> >
> > }
>
> Even though the inheritance rules we have now are not convenient for some
> people, they do have one benefit - they are simple. Looking into a single
> location is enough to have a full list of headers. You introduce inheritable
> headers which break this simplicity.
>
> I also though about this issue and I think it'd be better to add a separate
> directive/parameter which would explicitly inherit all headers from the outer
> scope.
I agree, your suggested approach significantly reduces code and configuration
complexity, and still has good flexibility.
>
> server {
> proxy_set_header Host "notserverthost.co";
>
> location / {
> ...
> proxy_set_header inherit; # explicitly inherit from above
>
> proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
> }
> }
>
> [..]
>
I've kept it as a separate directive for a few reasons:
- I think it makes the config syntax a little less ambiguous (ie. harder to
misread as setting a header called 'inherit' to empty value).
- It avoids a custom handler for proxy_set_header directive parsing.
- It allows for this directive itself to be inherited (in normal fashion)
for further reduction in configuration, while still allowing for disabling
in lower contexts.
I know this last point is against your explicitness point, however the user
would be free to choose from both approaches. Personally I prefer the one
with less config lines.
Please find the revised patch below:
# HG changeset patch
# User J Carter
# Date 1701107326 0
# Mon Nov 27 17:48:46 2023 +
# Node ID 33d34dd487501932f141cb6c2124959580ce38e5
# Parent 7ec761f0365f418511e30b82e9adf80bc56681df
Proxy: proxy_set_header_inherit directive
This patch introduces proxy_set_header_inherit which enables merge inheritance
of proxy_set_header directives from higher context when set to 'on' in the
receiving context.
The purpose of the added mechanics is to reduce repetition within the
nginx configuration for universally set (or boilerplate) request
headers, while maintaining flexibility to set additional headers for
specific paths.
There is no change in behavior for existing configurations.
Example below:
server {
...
proxy_set_header Host $host;
proxy_set_header Connection "";
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_set_header_inherit on;
location /1/ {
# proxy_set_header_inherit is on here, therefore
# do merge inheritance with server headers.
proxy_set_header customHeader1 "customvalue1";
proxy_pass http://backend1;
}
...
locatio
Re: [PATCH] Added merge inheritance to proxy_set_header
Hi Jordan,
On Thu, Nov 23, 2023 at 02:52:55PM +, J Carter wrote:
> # HG changeset patch
> # User J Carter
> # Date 1700751016 0
> # Thu Nov 23 14:50:16 2023 +
> # Node ID cc79903ca02eff8aa87238a0f801f8070425d9ab
> # Parent 7ec761f0365f418511e30b82e9adf80bc56681df
> Added merge inheritance to proxy_set_header
>
> This patch introduces 'inherit' argument to proxy_set_header
> directive. When marked as such it will be merge inherited into child
> contexts regardless of the presence of other proxy_set_header
> directives within those contexts.
>
> This patch also introduces the 'proxy_set_header_inherit' directive
> which blocks the merge inheritance in receiving contexts when set to off.
>
> The purpose of the added mechanics is to reduce repetition within the
> nginx configuration for universally set (or boilerplate) request
> headers, while maintaining flexibility to set additional headers for
> specific paths.
>
> There is no change in behavior for existing configurations.
>
> Example below:
>
> server {
>
> ...
>
> proxy_set_header Host $host inherit;
> proxy_set_header Connection "" inherit;
> proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for inherit;
>
> location /1/ {
> #sets this header in addition to server context ones.
> proxy_set_header customHeader1 "customvalue1";
> proxy_pass http://backend1;
> }
>
> location /2/ {
> #sets this header in addition to server context ones.
> proxy_set_header customheader2 "customValue2";
> proxy_pass http://backend1;
> }
>
> location /3/ {
> #no location specific headers, only server context ones set.
> proxy_pass http://backend1;
> }
>
> location /special/ {
> #Blocks merge inheritance from server context.
> proxy_set_header_inherit off;
> proxy_set_header Host "notserverthost.co";
> proxy_set_header Connection "";
> proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
> proxy_pass http://backend2;
> }
>
> }
Even though the inheritance rules we have now are not convenient for some
people, they do have one benefit - they are simple. Looking into a single
location is enough to have a full list of headers. You introduce inheritable
headers which break this simplicity.
I also though about this issue and I think it'd be better to add a separate
directive/parameter which would explicitly inherit all headers from the outer
scope.
server {
proxy_set_header Host "notserverthost.co";
location / {
...
proxy_set_header inherit; # explicitly inherit from above
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
}
}
[..]
--
Roman Arutyunyan
___
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
[PATCH] Added merge inheritance to proxy_set_header
# HG changeset patch
# User J Carter
# Date 1700751016 0
# Thu Nov 23 14:50:16 2023 +
# Node ID cc79903ca02eff8aa87238a0f801f8070425d9ab
# Parent 7ec761f0365f418511e30b82e9adf80bc56681df
Added merge inheritance to proxy_set_header
This patch introduces 'inherit' argument to proxy_set_header
directive. When marked as such it will be merge inherited into child
contexts regardless of the presence of other proxy_set_header
directives within those contexts.
This patch also introduces the 'proxy_set_header_inherit' directive
which blocks the merge inheritance in receiving contexts when set to off.
The purpose of the added mechanics is to reduce repetition within the
nginx configuration for universally set (or boilerplate) request
headers, while maintaining flexibility to set additional headers for
specific paths.
There is no change in behavior for existing configurations.
Example below:
server {
...
proxy_set_header Host $host inherit;
proxy_set_header Connection "" inherit;
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for inherit;
location /1/ {
#sets this header in addition to server context ones.
proxy_set_header customHeader1 "customvalue1";
proxy_pass http://backend1;
}
location /2/ {
#sets this header in addition to server context ones.
proxy_set_header customheader2 "customValue2";
proxy_pass http://backend1;
}
location /3/ {
#no location specific headers, only server context ones set.
proxy_pass http://backend1;
}
location /special/ {
#Blocks merge inheritance from server context.
proxy_set_header_inherit off;
proxy_set_header Host "notserverthost.co";
proxy_set_header Connection "";
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_pass http://backend2;
}
}
TODO:
- Repeat this implementation for grpc_set_header
diff -r 7ec761f0365f -r cc79903ca02e src/http/modules/ngx_http_proxy_module.c
--- a/src/http/modules/ngx_http_proxy_module.c Thu Oct 26 23:35:09 2023 +0300
+++ b/src/http/modules/ngx_http_proxy_module.c Thu Nov 23 14:50:16 2023 +
@@ -69,6 +69,11 @@
ngx_str_t uri;
} ngx_http_proxy_vars_t;
+typedef struct {
+ngx_str_t key;
+ngx_str_t value;
+ngx_uint_t inherit; /* unsigned inherit:1 */
+} ngx_http_proxy_header_src_t;
typedef struct {
ngx_array_t *flushes;
@@ -117,6 +122,8 @@
ngx_uint_t headers_hash_max_size;
ngx_uint_t headers_hash_bucket_size;
+ngx_flag_t headers_inherit;
+
#if (NGX_HTTP_SSL)
ngx_uint_t ssl;
ngx_uint_t ssl_protocols;
@@ -215,6 +222,8 @@
void *conf);
static char *ngx_http_proxy_store(ngx_conf_t *cf, ngx_command_t *cmd,
void *conf);
+static char *ngx_http_proxy_set_header(ngx_conf_t *cf, ngx_command_t *cmd,
+void *conf);
#if (NGX_HTTP_CACHE)
static char *ngx_http_proxy_cache(ngx_conf_t *cf, ngx_command_t *cmd,
void *conf);
@@ -409,12 +418,19 @@
NULL },
{ ngx_string("proxy_set_header"),
- NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE2,
- ngx_conf_set_keyval_slot,
+ NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE23,
+ ngx_http_proxy_set_header,
NGX_HTTP_LOC_CONF_OFFSET,
offsetof(ngx_http_proxy_loc_conf_t, headers_source),
NULL },
+{ ngx_string("proxy_set_header_inherit"),
+ NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_FLAG,
+ ngx_conf_set_flag_slot,
+ NGX_HTTP_LOC_CONF_OFFSET,
+ offsetof(ngx_http_proxy_loc_conf_t, headers_inherit),
+ NULL },
+
{ ngx_string("proxy_headers_hash_max_size"),
NGX_HTTP_MAIN_CONF|NGX_HTTP_SRV_CONF|NGX_HTTP_LOC_CONF|NGX_CONF_TAKE1,
ngx_conf_set_num_slot,
@@ -3400,6 +3416,8 @@
conf->upstream.intercept_errors = NGX_CONF_UNSET;
+conf->headers_inherit = NGX_CONF_UNSET;
+
#if (NGX_HTTP_SSL)
conf->upstream.ssl_session_reuse = NGX_CONF_UNSET;
conf->upstream.ssl_name = NGX_CONF_UNSET_PTR;
@@ -3444,13 +3462,15 @@
ngx_http_proxy_loc_conf_t *prev = parent;
ngx_http_proxy_loc_conf_t *conf = child;
-u_char *p;
-size_t size;
-ngx_int_t rc;
-ngx_hash_init_t hash;
-ngx_http_core_loc_conf_t *clcf;
-ngx_http_proxy_rewrite_t *pr;
-ngx_http_script_compile_t sc;
+u_char *p;
+size_tsize;
+ngx_int_t rc;
+ngx_uint_ti;
+ngx_hash_init_t hash;
+ngx_http_core_loc_conf_t *clcf;
+ngx_http_proxy_rewrite_t *pr;
+ngx_http_script_compile_t sc;
+ngx_http_proxy_heade
