This happens, at most, what, maybe 2 times? Is that really an issue? And if so, since ap_proxy_http_request() is local static, we could certainly pass the number of retries to it and bypass the extra call to ap_proxy_create_hdrbrgd() on retries, right?
Or am I missing something (which I likely am :) ) On Jan 13, 2014, at 9:30 AM, Yann Ylavic <[email protected]> wrote: > Hi, > > when mod_proxy(_http) has to forward the same request multiple times > (next balancer's worker / 100-continue ping), it duplicates > (re-merges) the same Via and X-Forwarded-* values as many times. > > This is because ap_proxy_create_hdrbrgd() works directly on > r->headers_in before constructing the headers brigade to be sent, and > this function is called for each try. > > The patch below addresses this by moving the (existing) table copy > before modifying the headers, but by doing so, any log format which > currently rely on these headers containing mod_proxy's merged values > will not work anymore. > > The question is, should "%{X-Forwarded-*|Via}i" contain mod_proxy's > values or not? > My feeling is that it should not, but working things should not be > broken either. > (there is PR 45387 about this (from 2008-07-12), but still NEW with no > answer...). > > If the response is yes (and duplication is a real issue), I could > propose another patch that still include the proxy related headers in > the client's r->headers_in, but only once. > > Please let me know, > regards, > Yann. > > Index: modules/proxy/proxy_util.c > =================================================================== > --- modules/proxy/proxy_util.c (revision 1557687) > +++ modules/proxy/proxy_util.c (working copy) > @@ -3200,10 +3200,18 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo > e = apr_bucket_pool_create(buf, strlen(buf), p, c->bucket_alloc); > APR_BRIGADE_INSERT_TAIL(header_brigade, e); > > + /* Make a copy of the to-be-modified headers_in table as we need the > + * original one later for logging, and to prepare the correct response > + * headers in the http output filter (eg. hop-by-hop headers), or else > + * to reenter with "fresh" headers should the same request be sent > + * multiple times (balancer's current worker or http ping failures). > + */ > + headers_in_copy = apr_table_copy(r->pool, r->headers_in); > + > /* handle Via */ > if (conf->viaopt == via_block) { > /* Block all outgoing Via: headers */ > - apr_table_unset(r->headers_in, "Via"); > + apr_table_unset(headers_in_copy, "Via"); > } else if (conf->viaopt != via_off) { > const char *server_name = ap_get_server_name(r); > /* If USE_CANONICAL_NAME_OFF was configured for the proxy virtual > host, > @@ -3215,7 +3223,7 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo > server_name = r->server->server_hostname; > /* Create a "Via:" request header entry and merge it */ > /* Generate outgoing Via: header with/without server comment: */ > - apr_table_mergen(r->headers_in, "Via", > + apr_table_mergen(headers_in_copy, "Via", > (conf->viaopt == via_full) > ? apr_psprintf(p, "%d.%d %s%s (%s)", > HTTP_VERSION_MAJOR(r->proto_num), > @@ -3233,7 +3241,7 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo > * to backend > */ > if (do_100_continue) { > - apr_table_mergen(r->headers_in, "Expect", "100-Continue"); > + apr_table_mergen(headers_in_copy, "Expect", "100-Continue"); > r->expecting_100 = 1; > } > > @@ -3264,38 +3272,35 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo > /* Add X-Forwarded-For: so that the upstream has a chance to > * determine, where the original request came from. > */ > - apr_table_mergen(r->headers_in, "X-Forwarded-For", > + apr_table_mergen(headers_in_copy, "X-Forwarded-For", > r->useragent_ip); > > /* Add X-Forwarded-Host: so that upstream knows what the > * original request hostname was. > */ > - if ((buf = apr_table_get(r->headers_in, "Host"))) { > - apr_table_mergen(r->headers_in, "X-Forwarded-Host", buf); > + if ((buf = apr_table_get(headers_in_copy, "Host"))) { > + apr_table_mergen(headers_in_copy, "X-Forwarded-Host", buf); > } > > /* Add X-Forwarded-Server: so that upstream knows what the > * name of this proxy server is (if there are more than one) > * XXX: This duplicates Via: - do we strictly need it? > */ > - apr_table_mergen(r->headers_in, "X-Forwarded-Server", > + apr_table_mergen(headers_in_copy, "X-Forwarded-Server", > r->server->server_hostname); > } > } > > - proxy_run_fixups(r); > - /* > - * Make a copy of the headers_in table before clearing the connection > - * headers as we need the connection headers later in the http output > - * filter to prepare the correct response headers. > - * > - * Note: We need to take r->pool for apr_table_copy as the key / value > - * pairs in r->headers_in have been created out of r->pool and > - * p might be (and actually is) a longer living pool. > - * This would trigger the bad pool ancestry abort in apr_table_copy if > - * apr is compiled with APR_POOL_DEBUG. > + /* Temporarily swap headers_in pointers for fixups hooks to work > + * with/on the new headers (while still preserving r->headers_in). > */ > - headers_in_copy = apr_table_copy(r->pool, r->headers_in); > + { > + apr_table_t *t = r->headers_in; > + r->headers_in = headers_in_copy; > + proxy_run_fixups(r); > + r->headers_in = t; > + } > + > ap_proxy_clear_connection(r, headers_in_copy); > /* send request headers */ > headers_in_array = apr_table_elts(headers_in_copy); > [EOS] > <httpd-trunk-mod_proxy_preserve_headers_in.patch>
