Hello!
On Mon, Apr 03, 2017 at 02:32:13AM -0700, Piotr Sikora via nginx-devel wrote:
> # HG changeset patch
> # User Piotr Sikora <piotrsik...@google.com>
> # Date 1490351854 25200
> # Fri Mar 24 03:37:34 2017 -0700
> # Node ID 488c59bd49dcb1503144fe4d712165b69d1a5945
> # Parent 5bab17ebe2b1f8ec42cf069bf484489c2a92c7a8
> Upstream: add support for trailers in HTTP responses.
>
> Please note that due to how upstream module terminates processing of
> responses that cannot have message body (responses to HEAD requests,
> and responses with 1xx, 204 and 304 status codes), trailers of those
> responses won't be passed to the downstream.
See comments to previous patches, from the HTTP logic there should
be no trailers in such responses. This seems to be the problem
with the "treat trailers separately from headers" approach.
>
> This change also adds $upstream_trailer_* variables.
>
> Signed-off-by: Piotr Sikora <piotrsik...@google.com>
Overral, this patch looks at most half-ready, as it doesn't even
try to implement sending trailers (r->expect_trailers is never
set), lacks any support for trailers in the cache, and so on.
If the goal were to add support for $upstream_trailer_* variables,
I would recommend to seriously simplify the patch, and only do
that instead - without trying to introduce any trailer copying,
and/or sending them to client. This should be enough for various
use cases involving upstream status information / logging, yet
won't introduce various issues with responses without bodies,
caching, and so on. I understand this is not your goal though.
See some additional comments below.
>
> diff -r 5bab17ebe2b1 -r 488c59bd49dc
> src/http/modules/ngx_http_fastcgi_module.c
> --- a/src/http/modules/ngx_http_fastcgi_module.c
> +++ b/src/http/modules/ngx_http_fastcgi_module.c
> @@ -2784,10 +2784,10 @@ ngx_http_fastcgi_create_loc_conf(ngx_con
>
> conf->upstream.intercept_errors = NGX_CONF_UNSET;
>
> - /* "fastcgi_cyclic_temp_file" is disabled */
> + /* the hardcoded values */
> conf->upstream.cyclic_temp_file = 0;
> -
> conf->upstream.change_buffering = 1;
> + conf->upstream.pass_trailers = 0;
>
> conf->catch_stderr = NGX_CONF_UNSET_PTR;
>
> diff -r 5bab17ebe2b1 -r 488c59bd49dc
> src/http/modules/ngx_http_memcached_module.c
> --- a/src/http/modules/ngx_http_memcached_module.c
> +++ b/src/http/modules/ngx_http_memcached_module.c
> @@ -619,6 +619,7 @@ ngx_http_memcached_create_loc_conf(ngx_c
> conf->upstream.pass_request_headers = 0;
> conf->upstream.pass_request_body = 0;
> conf->upstream.force_ranges = 1;
> + conf->upstream.pass_trailers = 0;
>
> conf->index = NGX_CONF_UNSET;
> conf->gzip_flag = NGX_CONF_UNSET_UINT;
> diff -r 5bab17ebe2b1 -r 488c59bd49dc src/http/modules/ngx_http_proxy_module.c
> --- a/src/http/modules/ngx_http_proxy_module.c
> +++ b/src/http/modules/ngx_http_proxy_module.c
> @@ -2886,11 +2886,12 @@ ngx_http_proxy_create_loc_conf(ngx_conf_
> conf->ssl_passwords = NGX_CONF_UNSET_PTR;
> #endif
>
> - /* "proxy_cyclic_temp_file" is disabled */
> + /* the hardcoded values */
> conf->upstream.cyclic_temp_file = 0;
> + conf->upstream.change_buffering = 1;
> + conf->upstream.pass_trailers = 0;
>
> conf->redirect = NGX_CONF_UNSET;
> - conf->upstream.change_buffering = 1;
>
> conf->cookie_domains = NGX_CONF_UNSET_PTR;
> conf->cookie_paths = NGX_CONF_UNSET_PTR;
> diff -r 5bab17ebe2b1 -r 488c59bd49dc src/http/modules/ngx_http_scgi_module.c
> --- a/src/http/modules/ngx_http_scgi_module.c
> +++ b/src/http/modules/ngx_http_scgi_module.c
> @@ -1234,10 +1234,10 @@ ngx_http_scgi_create_loc_conf(ngx_conf_t
>
> conf->upstream.intercept_errors = NGX_CONF_UNSET;
>
> - /* "scgi_cyclic_temp_file" is disabled */
> + /* the hardcoded values */
> conf->upstream.cyclic_temp_file = 0;
> -
> conf->upstream.change_buffering = 1;
> + conf->upstream.pass_trailers = 0;
>
> ngx_str_set(&conf->upstream.module, "scgi");
>
> diff -r 5bab17ebe2b1 -r 488c59bd49dc src/http/modules/ngx_http_uwsgi_module.c
> --- a/src/http/modules/ngx_http_uwsgi_module.c
> +++ b/src/http/modules/ngx_http_uwsgi_module.c
> @@ -1448,10 +1448,10 @@ ngx_http_uwsgi_create_loc_conf(ngx_conf_
> conf->ssl_passwords = NGX_CONF_UNSET_PTR;
> #endif
>
> - /* "uwsgi_cyclic_temp_file" is disabled */
> + /* the hardcoded values */
> conf->upstream.cyclic_temp_file = 0;
> -
> conf->upstream.change_buffering = 1;
> + conf->upstream.pass_trailers = 0;
>
> ngx_str_set(&conf->upstream.module, "uwsgi");
>
> diff -r 5bab17ebe2b1 -r 488c59bd49dc src/http/ngx_http_upstream.c
> --- a/src/http/ngx_http_upstream.c
> +++ b/src/http/ngx_http_upstream.c
> @@ -55,6 +55,8 @@ static ngx_int_t ngx_http_upstream_inter
> static ngx_int_t ngx_http_upstream_test_connect(ngx_connection_t *c);
> static ngx_int_t ngx_http_upstream_process_headers(ngx_http_request_t *r,
> ngx_http_upstream_t *u);
> +static ngx_int_t ngx_http_upstream_process_trailers(ngx_http_request_t *r,
> + ngx_http_upstream_t *u);
> static void ngx_http_upstream_process_body_in_memory(ngx_http_request_t *r,
> ngx_http_upstream_t *u);
> static void ngx_http_upstream_send_response(ngx_http_request_t *r,
> @@ -149,6 +151,8 @@ static ngx_int_t ngx_http_upstream_rewri
> ngx_table_elt_t *h, ngx_uint_t offset);
> static ngx_int_t ngx_http_upstream_copy_allow_ranges(ngx_http_request_t *r,
> ngx_table_elt_t *h, ngx_uint_t offset);
> +static ngx_int_t ngx_http_upstream_copy_trailer(ngx_http_request_t *r,
> + ngx_table_elt_t *h, ngx_uint_t offset);
>
> #if (NGX_HTTP_GZIP)
> static ngx_int_t ngx_http_upstream_copy_content_encoding(ngx_http_request_t
> *r,
> @@ -166,6 +170,8 @@ static ngx_int_t ngx_http_upstream_respo
> ngx_http_request_t *r, ngx_http_variable_value_t *v, uintptr_t data);
> static ngx_int_t ngx_http_upstream_header_variable(ngx_http_request_t *r,
> ngx_http_variable_value_t *v, uintptr_t data);
> +static ngx_int_t ngx_http_upstream_trailer_variable(ngx_http_request_t *r,
> + ngx_http_variable_value_t *v, uintptr_t data);
> static ngx_int_t ngx_http_upstream_cookie_variable(ngx_http_request_t *r,
> ngx_http_variable_value_t *v, uintptr_t data);
>
> @@ -307,6 +313,10 @@ static ngx_http_upstream_header_t ngx_h
> ngx_http_upstream_process_charset, 0,
> ngx_http_upstream_copy_header_line, 0, 0 },
>
> + { ngx_string("Trailer"),
> + ngx_http_upstream_ignore_header_line, 0,
> + ngx_http_upstream_copy_trailer, 0, 0 },
> +
> { ngx_string("Transfer-Encoding"),
> ngx_http_upstream_process_transfer_encoding, 0,
> ngx_http_upstream_ignore_header_line, 0, 0 },
> @@ -422,6 +432,9 @@ static ngx_http_variable_t ngx_http_ups
> { ngx_string("upstream_http_"), NULL, ngx_http_upstream_header_variable,
> 0, NGX_HTTP_VAR_NOCACHEABLE|NGX_HTTP_VAR_PREFIX, 0 },
>
> + { ngx_string("upstream_trailer_"), NULL,
> ngx_http_upstream_trailer_variable,
> + 0, NGX_HTTP_VAR_NOCACHEABLE|NGX_HTTP_VAR_PREFIX, 0 },
> +
> { ngx_string("upstream_cookie_"), NULL,
> ngx_http_upstream_cookie_variable,
> 0, NGX_HTTP_VAR_NOCACHEABLE|NGX_HTTP_VAR_PREFIX, 0 },
>
> @@ -1041,6 +1054,13 @@ ngx_http_upstream_cache_send(ngx_http_re
> return NGX_ERROR;
> }
>
> + if (ngx_list_init(&u->headers_in.trailers, r->pool, 2,
> + sizeof(ngx_table_elt_t))
> + != NGX_OK)
> + {
> + return NGX_ERROR;
> + }
> +
> rc = u->process_header(r);
>
> if (rc == NGX_OK) {
Note: we are not able to store trailers in the cache. This will
result in surprises if/when this code will ever become working.
> @@ -1861,6 +1881,13 @@ ngx_http_upstream_reinit(ngx_http_reques
> return NGX_ERROR;
> }
>
> + if (ngx_list_init(&u->headers_in.trailers, r->pool, 2,
> + sizeof(ngx_table_elt_t))
> + != NGX_OK)
> + {
> + return NGX_ERROR;
> + }
> +
> /* reinit the request chain */
>
> file_pos = 0;
> @@ -2228,6 +2255,15 @@ ngx_http_upstream_process_header(ngx_htt
> return;
> }
>
> + if (ngx_list_init(&u->headers_in.trailers, r->pool, 2,
> + sizeof(ngx_table_elt_t))
> + != NGX_OK)
> + {
> + ngx_http_upstream_finalize_request(r, u,
> +
> NGX_HTTP_INTERNAL_SERVER_ERROR);
> + return;
> + }
> +
> #if (NGX_HTTP_CACHE)
>
> if (r->cache) {
> @@ -2735,6 +2771,44 @@ ngx_http_upstream_process_headers(ngx_ht
> }
>
>
> +static ngx_int_t
> +ngx_http_upstream_process_trailers(ngx_http_request_t *r,
> ngx_http_upstream_t *u)
> +{
> + ngx_uint_t i;
> + ngx_list_part_t *part;
> + ngx_table_elt_t *h, *ho;
> +
> + if (!u->conf->pass_trailers || !r->allow_trailers ||
> !r->expect_trailers) {
> + return NGX_OK;
> + }
As r->expect_trailers is never set by the code, trailers will be
always ignored unless there are trailers sent by other parts of
the code.
> +
> + part = &u->headers_in.trailers.part;
> + h = part->elts;
> +
> + for (i = 0; /* void */; i++) {
> +
> + if (i >= part->nelts) {
> + if (part->next == NULL) {
> + break;
> + }
> +
> + part = part->next;
> + h = part->elts;
> + i = 0;
> + }
> +
> + ho = ngx_list_push(&r->headers_out.trailers);
> + if (ho == NULL) {
> + return NGX_ERROR;
> + }
> +
> + *ho = h[i];
> + }
> +
> + return NGX_OK;
> +}
> +
> +
> static void
> ngx_http_upstream_process_body_in_memory(ngx_http_request_t *r,
> ngx_http_upstream_t *u)
> @@ -4396,6 +4470,13 @@ ngx_http_upstream_finalize_request(ngx_h
> }
>
> if (rc == 0) {
> + if (ngx_http_upstream_process_trailers(r, u) != NGX_OK) {
> + rc = NGX_ERROR;
> + flush = 1;
> + }
> + }
> +
> + if (rc == 0) {
> rc = ngx_http_send_special(r, NGX_HTTP_LAST);
>
> } else if (flush) {
> @@ -5181,6 +5262,29 @@ ngx_http_upstream_copy_allow_ranges(ngx_
> }
>
>
> +static ngx_int_t
> +ngx_http_upstream_copy_trailer(ngx_http_request_t *r,
> + ngx_table_elt_t *h, ngx_uint_t offset)
> +{
> + ngx_table_elt_t *ho;
> +
> + if (!r->upstream->conf->pass_trailers
> + || !r->allow_trailers || !r->expect_trailers)
> + {
> + return NGX_OK;
> + }
> +
> + ho = ngx_list_push(&r->headers_out.headers);
> + if (ho == NULL) {
> + return NGX_ERROR;
> + }
> +
> + *ho = *h;
> +
> + return NGX_OK;
> +}
> +
> +
> #if (NGX_HTTP_GZIP)
>
> static ngx_int_t
> @@ -5519,6 +5623,21 @@ ngx_http_upstream_header_variable(ngx_ht
>
>
> static ngx_int_t
> +ngx_http_upstream_trailer_variable(ngx_http_request_t *r,
> + ngx_http_variable_value_t *v, uintptr_t data)
> +{
> + if (r->upstream == NULL) {
> + v->not_found = 1;
> + return NGX_OK;
> + }
> +
> + return ngx_http_variable_unknown_header(v, (ngx_str_t *) data,
> +
> &r->upstream->headers_in.trailers.part,
> + sizeof("upstream_trailer_") - 1);
> +}
> +
> +
> +static ngx_int_t
> ngx_http_upstream_cookie_variable(ngx_http_request_t *r,
> ngx_http_variable_value_t *v, uintptr_t data)
> {
> diff -r 5bab17ebe2b1 -r 488c59bd49dc src/http/ngx_http_upstream.h
> --- a/src/http/ngx_http_upstream.h
> +++ b/src/http/ngx_http_upstream.h
> @@ -183,6 +183,7 @@ typedef struct {
> ngx_hash_t hide_headers_hash;
> ngx_array_t *hide_headers;
> ngx_array_t *pass_headers;
> + ngx_flag_t pass_trailers;
Flags are expected to be listed above this block.
>
> ngx_http_upstream_local_t *local;
>
> @@ -248,6 +249,7 @@ typedef struct {
>
> typedef struct {
> ngx_list_t headers;
> + ngx_list_t trailers;
>
> ngx_uint_t status_n;
> ngx_str_t status_line;
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
--
Maxim Dounin
http://nginx.org/
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
