Hello Roman,
Thank you for the review and feedback.
On Mon, 27 Nov 2023 16:18:21 +0400
Roman Arutyunyan <arut@nginx.com> wrote:
> Hi Jordan,
>
> On Thu, Nov 23, 2023 at 02:52:55PM +0000, J Carter wrote:
> > # HG changeset patch
> > # User J Carter <jordan.carter@outlook.com>
> > # Date 1700751016 0
> > # Thu Nov 23 14:50:16 2023 +0000
> > # 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 <jordanc.carter@outlook.com>
# Date 1701107326 0
# Mon Nov 27 17:48:46 2023 +0000
# 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;
}
...
location /special/ {
# Blocks merge inheritance from server context.
# Standard nginx inheritence rules apply.
proxy_set_header_inherit off;
proxy_set_header Host "notserverhost.co";
proxy_set_header Connection "";
proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for;
proxy_pass http://backend2;
}
}
TODO:
- repeat this for grpc module.
diff -r 7ec761f0365f -r 33d34dd48750 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 Mon Nov 27 17:48:46 2023 +0000
@@ -69,7 +69,6 @@
ngx_str_t uri;
} ngx_http_proxy_vars_t;
-
typedef struct {
ngx_array_t *flushes;
ngx_array_t *lengths;
@@ -117,6 +116,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;
@@ -415,6 +416,13 @@
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 +3408,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;
@@ -3447,6 +3457,8 @@
u_char *p;
size_t size;
ngx_int_t rc;
+ ngx_uint_t i;
+ ngx_keyval_t *src, *h;
ngx_hash_init_t hash;
ngx_http_core_loc_conf_t *clcf;
ngx_http_proxy_rewrite_t *pr;
@@ -3724,6 +3736,9 @@
ngx_conf_merge_value(conf->upstream.intercept_errors,
prev->upstream.intercept_errors, 0);
+ ngx_conf_merge_value(conf->headers_inherit,
+ prev->headers_inherit, 0);
+
#if (NGX_HTTP_SSL)
if (ngx_http_proxy_merge_ssl(cf, conf, prev) != NGX_OK) {
@@ -3905,6 +3920,24 @@
#if (NGX_HTTP_CACHE)
conf->headers_cache = prev->headers_cache;
#endif
+ } else if (conf->headers_source
+ && prev->headers_source
+ && prev->headers_source != NGX_CONF_UNSET_PTR)
+ {
+ src = prev->headers_source->elts;
+
+ for (i = 0; i < prev->headers_source->nelts; i++) {
+ if (!conf->headers_inherit) {
+ continue;
+ }
+
+ h = ngx_array_push(conf->headers_source);
+ if (h == NULL) {
+ return NGX_CONF_ERROR;
+ }
+
+ *h = src[i];
+ }
}
rc = ngx_http_proxy_init_headers(cf, conf, &conf->headers,
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel
