You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Yann Ylavic <yl...@gmail.com> on 2014/01/13 15:30:35 UTC
mod_proxy duplicated its headers on next balancer's worker or
100-continue ping retries
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]
Re: mod_proxy duplicated its headers on next balancer's worker or
100-continue ping retries
Posted by Yann Ylavic <yl...@gmail.com>.
Thanks for your comments.
Commited in r1588527.
On Fri, Apr 4, 2014 at 11:39 PM, Yann Ylavic <yl...@gmail.com> wrote:
> On Fri, Apr 4, 2014 at 8:38 PM, Ruediger Pluem <rp...@apache.org> wrote:
>> Why can't we fix that directly in ap_proxy_create_hdrbrgd?
>
> Actually we can, and that's indeed a much simpler patch.
>
> I was worried about modifications of "Content-Length" and/or
> "Transfer-Encoding" outside ap_proxy_create_hdrbrgd() (ie. in
> ap_proxy_http_request() itself) that would be needed by
> ap_http_filter(), but in fact all is good now regarding interactions
> between the two (recent draft-ietf-httpbis-p1-messaging-23 conformance
> commits/backport).
>
> Thus, we can simply not touch headers_in in ap_proxy_http_request(),
> simply resetting old_cl_val and old_te_val when needed.
>
>
> Thanks for your feedback, below is the corresponding patch (nearly
> back to the original patch of this thread).
>
> Regards,
> Yann.
>
> Index: modules/proxy/proxy_util.c
> ===================================================================
> --- modules/proxy/proxy_util.c (revision 1584652)
> +++ modules/proxy/proxy_util.c (working copy)
> @@ -3203,7 +3203,7 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
> char *buf;
> const apr_array_header_t *headers_in_array;
> const apr_table_entry_t *headers_in;
> - apr_table_t *headers_in_copy;
> + apr_table_t *saved_headers_in;
> apr_bucket *e;
> int do_100_continue;
> conn_rec *origin = p_conn->connection;
> @@ -3279,6 +3279,21 @@ 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);
>
> + /*
> + * Save the original headers in here and restore them when leaving, since
> + * we will apply proxy purpose only modifications (eg. clearing hop-by-hop
> + * headers, add Via or X-Forwarded-* or Expect...), whereas the originals
> + * will be needed later to prepare the correct response and logging.
> + *
> + * 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.
> + */
> + saved_headers_in = r->headers_in;
> + r->headers_in = apr_table_copy(r->pool, saved_headers_in);
> +
> /* handle Via */
> if (conf->viaopt == via_block) {
> /* Block all outgoing Via: headers */
> @@ -3363,21 +3378,10 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
> }
>
> 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.
> - */
> - headers_in_copy = apr_table_copy(r->pool, r->headers_in);
> - ap_proxy_clear_connection(r, headers_in_copy);
> + ap_proxy_clear_connection(r, r->headers_in);
> +
> /* send request headers */
> - headers_in_array = apr_table_elts(headers_in_copy);
> + headers_in_array = apr_table_elts(r->headers_in);
> headers_in = (const apr_table_entry_t *) headers_in_array->elts;
> for (counter = 0; counter < headers_in_array->nelts; counter++) {
> if (headers_in[counter].key == NULL
> @@ -3439,6 +3443,11 @@ 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);
> }
> +
> + /* Restore the original headers in (see comment above),
> + * we won't modify them anymore.
> + */
> + r->headers_in = saved_headers_in;
> return OK;
> }
>
> Index: modules/proxy/mod_proxy_http.c
> ===================================================================
> --- modules/proxy/mod_proxy_http.c (revision 1584652)
> +++ modules/proxy/mod_proxy_http.c (working copy)
> @@ -750,14 +750,8 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
> if (!r->kept_body && r->main) {
> /* XXX: Why DON'T sub-requests use keepalives? */
> p_conn->close = 1;
> - if (old_cl_val) {
> - old_cl_val = NULL;
> - apr_table_unset(r->headers_in, "Content-Length");
> - }
> - if (old_te_val) {
> - old_te_val = NULL;
> - apr_table_unset(r->headers_in, "Transfer-Encoding");
> - }
> + old_cl_val = NULL;
> + old_te_val = NULL;
> rb_method = RB_STREAM_CL;
> e = apr_bucket_eos_create(input_brigade->bucket_alloc);
> APR_BRIGADE_INSERT_TAIL(input_brigade, e);
> @@ -783,7 +777,6 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
> "client %s (%s) requested Transfer-Encoding "
> "chunked body with Content-Length (C-L ignored)",
> c->client_ip, c->remote_host ? c->remote_host: "");
> - apr_table_unset(r->headers_in, "Content-Length");
> old_cl_val = NULL;
> origin->keepalive = AP_CONN_CLOSE;
> p_conn->close = 1;
Re: mod_proxy duplicated its headers on next balancer's worker or
100-continue ping retries
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Apr 4, 2014 at 8:38 PM, Ruediger Pluem <rp...@apache.org> wrote:
> Why can't we fix that directly in ap_proxy_create_hdrbrgd?
Actually we can, and that's indeed a much simpler patch.
I was worried about modifications of "Content-Length" and/or
"Transfer-Encoding" outside ap_proxy_create_hdrbrgd() (ie. in
ap_proxy_http_request() itself) that would be needed by
ap_http_filter(), but in fact all is good now regarding interactions
between the two (recent draft-ietf-httpbis-p1-messaging-23 conformance
commits/backport).
Thus, we can simply not touch headers_in in ap_proxy_http_request(),
simply resetting old_cl_val and old_te_val when needed.
Thanks for your feedback, below is the corresponding patch (nearly
back to the original patch of this thread).
Regards,
Yann.
Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c (revision 1584652)
+++ modules/proxy/proxy_util.c (working copy)
@@ -3203,7 +3203,7 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
char *buf;
const apr_array_header_t *headers_in_array;
const apr_table_entry_t *headers_in;
- apr_table_t *headers_in_copy;
+ apr_table_t *saved_headers_in;
apr_bucket *e;
int do_100_continue;
conn_rec *origin = p_conn->connection;
@@ -3279,6 +3279,21 @@ 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);
+ /*
+ * Save the original headers in here and restore them when leaving, since
+ * we will apply proxy purpose only modifications (eg. clearing hop-by-hop
+ * headers, add Via or X-Forwarded-* or Expect...), whereas the originals
+ * will be needed later to prepare the correct response and logging.
+ *
+ * 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.
+ */
+ saved_headers_in = r->headers_in;
+ r->headers_in = apr_table_copy(r->pool, saved_headers_in);
+
/* handle Via */
if (conf->viaopt == via_block) {
/* Block all outgoing Via: headers */
@@ -3363,21 +3378,10 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
}
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.
- */
- headers_in_copy = apr_table_copy(r->pool, r->headers_in);
- ap_proxy_clear_connection(r, headers_in_copy);
+ ap_proxy_clear_connection(r, r->headers_in);
+
/* send request headers */
- headers_in_array = apr_table_elts(headers_in_copy);
+ headers_in_array = apr_table_elts(r->headers_in);
headers_in = (const apr_table_entry_t *) headers_in_array->elts;
for (counter = 0; counter < headers_in_array->nelts; counter++) {
if (headers_in[counter].key == NULL
@@ -3439,6 +3443,11 @@ 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);
}
+
+ /* Restore the original headers in (see comment above),
+ * we won't modify them anymore.
+ */
+ r->headers_in = saved_headers_in;
return OK;
}
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c (revision 1584652)
+++ modules/proxy/mod_proxy_http.c (working copy)
@@ -750,14 +750,8 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
if (!r->kept_body && r->main) {
/* XXX: Why DON'T sub-requests use keepalives? */
p_conn->close = 1;
- if (old_cl_val) {
- old_cl_val = NULL;
- apr_table_unset(r->headers_in, "Content-Length");
- }
- if (old_te_val) {
- old_te_val = NULL;
- apr_table_unset(r->headers_in, "Transfer-Encoding");
- }
+ old_cl_val = NULL;
+ old_te_val = NULL;
rb_method = RB_STREAM_CL;
e = apr_bucket_eos_create(input_brigade->bucket_alloc);
APR_BRIGADE_INSERT_TAIL(input_brigade, e);
@@ -783,7 +777,6 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
"client %s (%s) requested Transfer-Encoding "
"chunked body with Content-Length (C-L ignored)",
c->client_ip, c->remote_host ? c->remote_host: "");
- apr_table_unset(r->headers_in, "Content-Length");
old_cl_val = NULL;
origin->keepalive = AP_CONN_CLOSE;
p_conn->close = 1;
Re: mod_proxy duplicated its headers on next balancer's worker or
100-continue ping retries
Posted by Ruediger Pluem <rp...@apache.org>.
Why can't we fix that directly in ap_proxy_create_hdrbrgd?
Regards
Rüdiger
Yann Ylavic wrote:
> Here is the patch not polluted by working collisions or gotos :
>
Re: mod_proxy duplicated its headers on next balancer's worker or
100-continue ping retries
Posted by Yann Ylavic <yl...@gmail.com>.
Here is the patch not polluted by working collisions or gotos :
Index: modules/proxy/mod_proxy_wstunnel.c
===================================================================
--- modules/proxy/mod_proxy_wstunnel.c (revision 1584652)
+++ modules/proxy/mod_proxy_wstunnel.c (working copy)
@@ -320,15 +320,26 @@ static int ap_proxy_wstunnel_request(apr_pool_t *p
apr_socket_t *client_socket = ap_get_conn_socket(c);
ws_baton_t *baton = apr_pcalloc(r->pool, sizeof(ws_baton_t));
apr_socket_t *sockets[3] = {NULL, NULL, NULL};
+ apr_table_t *saved_headers_in;
int status;
header_brigade = apr_brigade_create(p, backconn->bucket_alloc);
ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, "sending request");
+ /*
+ * Save the original headers in before ap_proxy_create_hdrbrgd() and
+ * restore them after, since it will apply proxy purpose only modifications
+ * (eg. cleanup hop-by-hop headers, add Via or X-Forwarded-*...), whereas
+ * the original headers may be needed later to prepare the correct response
+ * or logging.
+ */
+ saved_headers_in = r->headers_in;
+ r->headers_in = apr_table_copy(r->pool, saved_headers_in);
rv = ap_proxy_create_hdrbrgd(p, header_brigade, r, conn,
worker, conf, uri, url, server_portstr,
&old_cl_val, &old_te_val);
+ r->headers_in = saved_headers_in;
if (rv != OK) {
return rv;
}
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c (revision 1584652)
+++ modules/proxy/mod_proxy_http.c (working copy)
@@ -717,6 +717,7 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
apr_off_t bytes;
int force10, rv;
conn_rec *origin = p_conn->connection;
+ apr_table_t *saved_headers_in;
if (apr_table_get(r->subprocess_env, "force-proxy-request-1.0")) {
if (r->expecting_100) {
@@ -727,11 +728,21 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
force10 = 0;
}
+ /*
+ * Save the original headers in here and restore them when leaving, since
+ * we will apply proxy purpose only modifications (eg. cleanup hop-by-hop
+ * headers, add Via or X-Forwarded-* or Expect...), whereas the original
+ * headers may be needed later to prepare the correct response or logging.
+ */
+ saved_headers_in = r->headers_in;
+ r->headers_in = apr_table_copy(r->pool, saved_headers_in);
+
header_brigade = apr_brigade_create(p, bucket_alloc);
rv = ap_proxy_create_hdrbrgd(p, header_brigade, r, p_conn,
worker, conf, uri, url, server_portstr,
&old_cl_val, &old_te_val);
if (rv != OK) {
+ r->headers_in = saved_headers_in;
return rv;
}
@@ -775,6 +786,7 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
if (old_te_val && strcasecmp(old_te_val, "chunked") != 0) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01093)
"%s Transfer-Encoding is not supported", old_te_val);
+ r->headers_in = saved_headers_in;
return HTTP_INTERNAL_SERVER_ERROR;
}
@@ -808,6 +820,7 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
" from %s (%s)",
p_conn->addr, p_conn->hostname ?
p_conn->hostname: "",
c->client_ip, c->remote_host ? c->remote_host: "");
+ r->headers_in = saved_headers_in;
return HTTP_BAD_REQUEST;
}
@@ -830,6 +843,7 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
" to %pI (%s) from %s (%s)",
p_conn->addr, p_conn->hostname ?
p_conn->hostname: "",
c->client_ip, c->remote_host ? c->remote_host: "");
+ r->headers_in = saved_headers_in;
return HTTP_INTERNAL_SERVER_ERROR;
}
@@ -963,6 +977,11 @@ skip_body:
break;
}
+ /* Restore the original headers in (see comment above),
+ * we won't modify them anymore.
+ */
+ r->headers_in = saved_headers_in;
+
if (rv != OK) {
/* apr_status_t value has been logged in lower level method */
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01097)
Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c (revision 1584652)
+++ modules/proxy/proxy_util.c (working copy)
@@ -3203,7 +3203,6 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
char *buf;
const apr_array_header_t *headers_in_array;
const apr_table_entry_t *headers_in;
- apr_table_t *headers_in_copy;
apr_bucket *e;
int do_100_continue;
conn_rec *origin = p_conn->connection;
@@ -3363,21 +3362,10 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
}
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.
- */
- headers_in_copy = apr_table_copy(r->pool, r->headers_in);
- ap_proxy_clear_connection(r, headers_in_copy);
+ ap_proxy_clear_connection(r, r->headers_in);
+
/* send request headers */
- headers_in_array = apr_table_elts(headers_in_copy);
+ headers_in_array = apr_table_elts(r->headers_in);
headers_in = (const apr_table_entry_t *) headers_in_array->elts;
for (counter = 0; counter < headers_in_array->nelts; counter++) {
if (headers_in[counter].key == NULL
[EOS]
Re: mod_proxy duplicated its headers on next balancer's worker or
100-continue ping retries
Posted by Yann Ylavic <yl...@gmail.com>.
On Fri, Apr 4, 2014 at 6:22 PM, Jim Jagielski <ji...@jagunet.com> wrote:
> This seems to change some logic which appear only tangentially
> associated w/ the "save headers" issue... why is that?
Do you mean, ...
>
> On Apr 4, 2014, at 11:43 AM, Yann Ylavic <yl...@gmail.com> wrote:
>> Index: modules/proxy/mod_proxy_http.c
>> ===================================================================
>> --- modules/proxy/mod_proxy_http.c (revision 1584652)
>> +++ modules/proxy/mod_proxy_http.c (working copy)
>> @@ -717,24 +717,31 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
>> apr_off_t bytes;
>> int force10, rv;
>> conn_rec *origin = p_conn->connection;
>> + apr_table_t *saved_headers_in;
>>
>> - if (apr_table_get(r->subprocess_env, "force-proxy-request-1.0")) {
>> - if (r->expecting_100) {
>> - return HTTP_EXPECTATION_FAILED;
>> - }
>> - force10 = 1;
>> - } else {
>> - force10 = 0;
>> - }
... this change?
Yes, sorry about it, it is part of another patch I'm working on
regarding 100-continue forwarding issue.
Please ignore this change, it is harmless (since
ap_proxy_create_hdrbrgd() already checks the expectation on HTTP/1.0),
but really not related to this thread.
>> +cleanup:
>> + /* Restore the original headers in (see comment above),
>> + * we won't modify them anymore.
>> + */
>> + r->headers_in = saved_headers_in;
>> + return rv;
>> }
>>
>
> Uggg... I hate gotos. We only use them sparingly and when
> breaking out of nasty, nasty code. Here, we are just doing so
> to safe some cut/paste... -0
>
I think I can avoid that, and don't like them either :p
This was to keep the patch simple for review (I can see gotos used not
far from here, and took the easy way).
Re: mod_proxy duplicated its headers on next balancer's worker or 100-continue ping retries
Posted by Jim Jagielski <ji...@jaguNET.com>.
This seems to change some logic which appear only tangentially
associated w/ the "save headers" issue... why is that?
On Apr 4, 2014, at 11:43 AM, Yann Ylavic <yl...@gmail.com> wrote:
> Index: modules/proxy/mod_proxy_http.c
> ===================================================================
> --- modules/proxy/mod_proxy_http.c (revision 1584652)
> +++ modules/proxy/mod_proxy_http.c (working copy)
> @@ -717,24 +717,31 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
> apr_off_t bytes;
> int force10, rv;
> conn_rec *origin = p_conn->connection;
> + apr_table_t *saved_headers_in;
>
> - if (apr_table_get(r->subprocess_env, "force-proxy-request-1.0")) {
> - if (r->expecting_100) {
> - return HTTP_EXPECTATION_FAILED;
> - }
> - force10 = 1;
> - } else {
> - force10 = 0;
> - }
> + /*
> + * Save the original headers in here and restore them when leaving, since
> + * we will apply proxy purpose only modifications (eg. cleanup hop-by-hop
> + * headers, add Via or X-Forwarded-* or Expect...), whereas the original
> + * headers may be needed later to prepare the correct response or logging.
> + */
> + saved_headers_in = r->headers_in;
> + r->headers_in = apr_table_copy(r->pool, saved_headers_in);
>
> header_brigade = apr_brigade_create(p, bucket_alloc);
> rv = ap_proxy_create_hdrbrgd(p, header_brigade, r, p_conn,
> worker, conf, uri, url, server_portstr,
> &old_cl_val, &old_te_val);
> if (rv != OK) {
> - return rv;
> + goto cleanup;
> +cleanup:
> + /* Restore the original headers in (see comment above),
> + * we won't modify them anymore.
> + */
> + r->headers_in = saved_headers_in;
> + return rv;
> }
>
Uggg... I hate gotos. We only use them sparingly and when
breaking out of nasty, nasty code. Here, we are just doing so
to safe some cut/paste... -0
Re: mod_proxy duplicated its headers on next balancer's worker or
100-continue ping retries
Posted by Yann Ylavic <yl...@gmail.com>.
Helo,
I revive this thread since headers in modfied by mod_proxy seems wrong
to me, I have to take that into account when, say, analysing access
logs (received X-Forwarded-* or Via headers vs the ones added by
mod_proxy, see also PR 45387), or as said in the title, which is even
worse when backend (recoverable) failures occur in load-balancing
(same headers added multiple times).
I ended up with the following patch that copies r->headers_in before
they are modified and restore them after they have been forwarded
(mod_proxy_http and mod_proxy_wstunnel only seem to be concerned, but
I may be missing something).
Feedbacks welcome, if no one objects I'll commit it to trunk.
Regards,
Yann.
Index: modules/proxy/mod_proxy_wstunnel.c
===================================================================
--- modules/proxy/mod_proxy_wstunnel.c (revision 1584652)
+++ modules/proxy/mod_proxy_wstunnel.c (working copy)
@@ -320,15 +320,26 @@ static int ap_proxy_wstunnel_request(apr_pool_t *p
apr_socket_t *client_socket = ap_get_conn_socket(c);
ws_baton_t *baton = apr_pcalloc(r->pool, sizeof(ws_baton_t));
apr_socket_t *sockets[3] = {NULL, NULL, NULL};
+ apr_table_t *saved_headers_in;
int status;
header_brigade = apr_brigade_create(p, backconn->bucket_alloc);
ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r, "sending request");
+ /*
+ * Save the original headers in before ap_proxy_create_hdrbrgd() and
+ * restore them after, since it will apply proxy purpose only modifications
+ * (eg. cleanup hop-by-hop headers, add Via or X-Forwarded-*...), whereas
+ * the original headers may be needed later to prepare the correct response
+ * or logging.
+ */
+ saved_headers_in = r->headers_in;
+ r->headers_in = apr_table_copy(r->pool, saved_headers_in);
rv = ap_proxy_create_hdrbrgd(p, header_brigade, r, conn,
worker, conf, uri, url, server_portstr,
&old_cl_val, &old_te_val);
+ r->headers_in = saved_headers_in;
if (rv != OK) {
return rv;
}
Index: modules/proxy/mod_proxy_http.c
===================================================================
--- modules/proxy/mod_proxy_http.c (revision 1584652)
+++ modules/proxy/mod_proxy_http.c (working copy)
@@ -717,24 +717,31 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
apr_off_t bytes;
int force10, rv;
conn_rec *origin = p_conn->connection;
+ apr_table_t *saved_headers_in;
- if (apr_table_get(r->subprocess_env, "force-proxy-request-1.0")) {
- if (r->expecting_100) {
- return HTTP_EXPECTATION_FAILED;
- }
- force10 = 1;
- } else {
- force10 = 0;
- }
+ /*
+ * Save the original headers in here and restore them when leaving, since
+ * we will apply proxy purpose only modifications (eg. cleanup hop-by-hop
+ * headers, add Via or X-Forwarded-* or Expect...), whereas the original
+ * headers may be needed later to prepare the correct response or logging.
+ */
+ saved_headers_in = r->headers_in;
+ r->headers_in = apr_table_copy(r->pool, saved_headers_in);
header_brigade = apr_brigade_create(p, bucket_alloc);
rv = ap_proxy_create_hdrbrgd(p, header_brigade, r, p_conn,
worker, conf, uri, url, server_portstr,
&old_cl_val, &old_te_val);
if (rv != OK) {
- return rv;
+ goto cleanup;
}
+ if (apr_table_get(r->subprocess_env, "force-proxy-request-1.0")) {
+ force10 = 1;
+ } else {
+ force10 = 0;
+ }
+
/* We have headers, let's figure out our request body... */
input_brigade = apr_brigade_create(p, bucket_alloc);
@@ -775,7 +782,8 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
if (old_te_val && strcasecmp(old_te_val, "chunked") != 0) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(01093)
"%s Transfer-Encoding is not supported", old_te_val);
- return HTTP_INTERNAL_SERVER_ERROR;
+ rv = HTTP_INTERNAL_SERVER_ERROR;
+ goto cleanup;
}
if (old_cl_val && old_te_val) {
@@ -808,7 +816,8 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
" from %s (%s)",
p_conn->addr, p_conn->hostname ?
p_conn->hostname: "",
c->client_ip, c->remote_host ? c->remote_host: "");
- return HTTP_BAD_REQUEST;
+ rv = HTTP_BAD_REQUEST;
+ goto cleanup;
}
apr_brigade_length(temp_brigade, 1, &bytes);
@@ -830,7 +839,8 @@ int ap_proxy_http_request(apr_pool_t *p, request_r
" to %pI (%s) from %s (%s)",
p_conn->addr, p_conn->hostname ?
p_conn->hostname: "",
c->client_ip, c->remote_host ? c->remote_host: "");
- return HTTP_INTERNAL_SERVER_ERROR;
+ rv = HTTP_INTERNAL_SERVER_ERROR;
+ goto cleanup;
}
/* Ensure we don't hit a wall where we have a buffer too small
@@ -969,10 +979,13 @@ skip_body:
"pass request body failed to %pI (%s) from %s (%s)",
p_conn->addr, p_conn->hostname ? p_conn->hostname: "",
c->client_ip, c->remote_host ? c->remote_host: "");
- return rv;
}
-
- return OK;
+cleanup:
+ /* Restore the original headers in (see comment above),
+ * we won't modify them anymore.
+ */
+ r->headers_in = saved_headers_in;
+ return rv;
}
/*
Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c (revision 1584652)
+++ modules/proxy/proxy_util.c (working copy)
@@ -3203,7 +3203,6 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
char *buf;
const apr_array_header_t *headers_in_array;
const apr_table_entry_t *headers_in;
- apr_table_t *headers_in_copy;
apr_bucket *e;
int do_100_continue;
conn_rec *origin = p_conn->connection;
@@ -3363,21 +3362,10 @@ PROXY_DECLARE(int) ap_proxy_create_hdrbrgd(apr_poo
}
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.
- */
- headers_in_copy = apr_table_copy(r->pool, r->headers_in);
- ap_proxy_clear_connection(r, headers_in_copy);
+ ap_proxy_clear_connection(r, r->headers_in);
+
/* send request headers */
- headers_in_array = apr_table_elts(headers_in_copy);
+ headers_in_array = apr_table_elts(r->headers_in);
headers_in = (const apr_table_entry_t *) headers_in_array->elts;
for (counter = 0; counter < headers_in_array->nelts; counter++) {
if (headers_in[counter].key == NULL
[EOS]
Re: mod_proxy duplicated its headers on next balancer's worker or
100-continue ping retries
Posted by Yann Ylavic <yl...@gmail.com>.
On Mon, Jan 13, 2014 at 5:38 PM, Jim Jagielski <ji...@jagunet.com> wrote:
> This happens, at most, what, maybe 2 times? Is that
> really an issue?
The worst case is "2 x number of balancer's workers" tries, when the
request is a POST, ping is configured and all the balancer's wokers
aren't connect()able.
Whether it is or not a big deal depends on the backend's handling of
Via/X-Forwarded-* headers duplicated values, or the log parser (since
these values are there too).
> 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?
That would certainly help for the ping failure(s), but not for
balancer's workers (recoverable) ones, where the whole process is
replayed.
Note the patch does not really add cycles since the copy of headers_in
is already there to preserve "Connection" header (needed by output
filter), so maybe it's not worth handle both cases differently (but
why not, your proposal looks simple enough to handle the common case
with no balancer).
Regards.
Re: mod_proxy duplicated its headers on next balancer's worker or 100-continue ping retries
Posted by Jim Jagielski <ji...@jaguNET.com>.
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 <yl...@gmail.com> 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>