You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Colin Murtaugh <cm...@yahoo.com> on 2005/10/05 20:44:08 UTC
[PATCH] Bug 36816: balancer_manager doesn't work if worker name contains port
Attached is a patch for bug 36816. The balancer_manager interface
offered by mod_proxy_balancer doesn't work properly if a worker name
contains a port number.
E.g. if I have configured a cluster as:
<Proxy balancer://testcluster>
BalancerMember http://server-one.mydomain.com:1234
BalancerMember http://server-two.mydomain.com:1234
</Proxy>
then I'm not able to edit the worker settings via the
balancer_manager. This is because the worker->name is being compared
to worker->hostname; worker->name contains the port and worker-
>hostname does not.
I've created the patch below to fix this problem.
--Colin
Re: [PATCH] Bug 36816: balancer_manager doesn't work if worker name
contains port
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/06/2005 02:47 PM, Jim Jagielski wrote:
> This is actually on my ToLookAt list... The issue is
> the confusion on what defines a member *name*. Is it
> just the scheme+hostname or is it the complete
> URL. I consider it the later.
That was also my feeling after I took a look to the code.
>
> I have some ideas on how to better address this...
>
I guess you let us know your ideas :-).
[..cut..]
Regards
Rüdiger
Re: [PATCH] Bug 36816: balancer_manager doesn't work if worker name contains port
Posted by Jim Jagielski <ji...@jaguNET.com>.
This is actually on my ToLookAt list... The issue is
the confusion on what defines a member *name*. Is it
just the scheme+hostname or is it the complete
URL. I consider it the later.
I have some ideas on how to better address this...
On Oct 6, 2005, at 8:24 AM, Ruediger Pluem wrote:
> This is a follow up to the discussion I started in my comment to
> PR36816:
>
> I am currently wondering what identifies a proxy worker.
>
>> From what I read in ap_proxy_get_worker in proxy_util.c it is
>> identified by
>>
> worker->name. But I see a problem here: The url passed for
> comparison here
> gets reduced effectively to schema, hostname and port of the
> worker, but
> worker->name also contains a path if one was added during
> configuration
> as it is the result of apr_uri_unparse(p, &uri,
> APR_URI_UNP_REVEALPASSWORD);
> As an example I take the one from PR36816:
>
> <Proxy balancer://test1>
> BalancerMember http://my.server.com:1234
> BalancerMember http://otner.server.com:1234/myapp
> </Proxy>
>
> So the worker http://otner.server.com:1234/myapp can never be found by
> ap_proxy_get_worker.
>
>
>> From what I understand from other parts of the code the worker
>> should be identified
>>
> by the full URL. So I think the following patch should be applied
> to ap_proxy_get_worker
>
>
> Index: proxy_util.c
> ===================================================================
> --- proxy_util.c (Revision 295013)
> +++ proxy_util.c (Arbeitskopie)
> @@ -1218,9 +1218,6 @@
> c = strchr(uri, ':');
> if (c == NULL || c[1] != '/' || c[2] != '/' || c[3] == '\0')
> return NULL;
> - /* remove path from uri */
> - if ((c = strchr(c + 3, '/')))
> - *c = '\0';
>
> worker = (proxy_worker *)conf->workers->elts;
> for (i = 0; i < conf->workers->nelts; i++) {
>
> Last but not least all information that identifies a worker should
> be displayed
> by the manager application.
>
> Thoughts / comments?
>
> Regards
>
> Rüdiger
>
> On 10/05/2005 08:44 PM, Colin Murtaugh wrote:
>
>> Attached is a patch for bug 36816. The balancer_manager interface
>> offered by mod_proxy_balancer doesn't work properly if a worker name
>> contains a port number.
>>
>> E.g. if I have configured a cluster as:
>>
>> <Proxy balancer://testcluster>
>> BalancerMember http://server-one.mydomain.com:1234
>> BalancerMember http://server-two.mydomain.com:1234
>> </Proxy>
>>
>> then I'm not able to edit the worker settings via the
>> balancer_manager. This is because the worker->name is being compared
>> to worker->hostname; worker->name contains the port and worker-
>>
>>> hostname does not.
>>>
>>
>> I've created the patch below to fix this problem.
>>
>> --Colin
>>
>>
>>
>
>
Re: [PATCH] Bug 36816: balancer_manager doesn't work if worker name
contains port
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/11/2005 04:33 PM, Jim Jagielski wrote:
>
> On Oct 10, 2005, at 5:13 PM, Ruediger Pluem wrote:
>
[..cut..]
>
> Not sure what the (original) intent of the XML format was or is,
> but I'm assuming that the current implementation is sufficient.
>
>
As it is an undocumented feature right now and the intention is also
unclear to me I tend now to leave it as it is. Just tried to make some
noise about it to find out if somebody knows more about it, so a change
here is not forgotten unintentionally.
Regards
Rüdiger
Re: [PATCH] Bug 36816: balancer_manager doesn't work if worker name contains port
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Oct 10, 2005, at 5:13 PM, Ruediger Pluem wrote:
>
> 2. The xml output is currently broken as it only works with schema
> and hostname.
> I am not quite sure if
>
> 1. there is an xml schema already defined for this and needs to
> be adjusted
> 2. we need to parse the worker name and replace some of the
> characters with
> xml entities.
>
> I think we need to fix this or if not at least a remark in the
> documentation
> should be made that the xml output is currently broken.
>
Not sure what the (original) intent of the XML format was or is,
but I'm assuming that the current implementation is sufficient.
Re: [PATCH] Bug 36816: balancer_manager doesn't work if worker name
contains port
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/10/2005 11:13 PM, Ruediger Pluem wrote:
[..cut..]
>
> Thus ap_proxy_get_worker would be called with the URL http://backend.com/rsync/bar
> which would lead to a no match. I guess instead of a simple strcasecmp
> we need something like a "longest match" here since I think we cannot rely
> on the order the workers got stored. Do you know if a "longest match" function
Attached a new version which does a longest match and should fix the problem.
[..cut..]
BTW: Tomorrow I will have a look a the log messages, where it makes sense to exchange
worker->hostname against worker->name.
Regards
Rüdiger
Re: [PATCH] Bug 36816: balancer_manager doesn't work if worker name
contains port
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/11/2005 09:56 PM, Jim Jagielski wrote:
[..cut..]
>
> +1... I haven't looked to see if there's a "better" way to
> do the 'longest match' test, but that's nit picking :)
Ok, thanks. I think I will commit it tomorrow to trunk and 2.2.x.
Regards
Rüdiger
Re: [PATCH] Bug 36816: balancer_manager doesn't work if worker name contains port
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Oct 11, 2005, at 3:13 PM, Ruediger Pluem wrote:
>
> Sorry, too impatient again :-(. Nevertheless apart from the xml
> stuff, any
> comments about the latest version of the patch I attached yesterday?
>
+1... I haven't looked to see if there's a "better" way to
do the 'longest match' test, but that's nit picking :)
Re: [PATCH] Bug 36816: balancer_manager doesn't work if worker name
contains port
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/11/2005 02:35 PM, Jim Jagielski wrote:
>
> On Oct 10, 2005, at 5:13 PM, Ruediger Pluem wrote:
>
>>
>>
>> On 10/10/2005 05:43 PM, Jim Jagielski wrote:
>>
>>> For consideration:
[..cut..]
>
>
> Yes, I was also considering that case as well; I simply wanted to
> give people a head's up on the direction the solution was taking
> in order to get prelim feedback.
>
>
Sorry, too impatient again :-(. Nevertheless apart from the xml stuff, any
comments about the latest version of the patch I attached yesterday?
Regards
Rüdiger
Re: [PATCH] Bug 36816: balancer_manager doesn't work if worker name contains port
Posted by Jim Jagielski <ji...@jaguNET.com>.
On Oct 10, 2005, at 5:13 PM, Ruediger Pluem wrote:
>
>
> On 10/10/2005 05:43 PM, Jim Jagielski wrote:
>
>> For consideration:
>>
>
> [..cut..]
>
> Thanks for your thoughts.
> BTW: It was a little bit tricky to apply the patch as my Mozilla
> seems to have changed things in the mail spaces / empty lines.
> So I guess attached patches are easier to handle :-).
>
> I think I found one big problem with the patch:
>
> When we call ap_proxy_get_worker in line 1316 of proxy_util.c
> (ap_proxy_pre_request), then the parameter url already contains
> the complete URL we must call on the remote server. Example:
>
> ServerName example.com
> ProxyPass /mirror/foo http://backend.com/rsync
>
> This would result in a worker named
>
> http://backend.com/rsync
>
>
> Calling http://example.com/mirror/foo/bar would be translated to
>
> proxy:http://backend.com/rsync/bar in the translate_name hook of
> mod_proxy.
>
> Thus ap_proxy_get_worker would be called with the URL http://
> backend.com/rsync/bar
> which would lead to a no match. I guess instead of a simple strcasecmp
> we need something like a "longest match" here since I think we
> cannot rely
> on the order the workers got stored.
Yes, I was also considering that case as well; I simply wanted to
give people a head's up on the direction the solution was taking
in order to get prelim feedback.
Re: [PATCH] Bug 36816: balancer_manager doesn't work if worker name
contains port
Posted by Ruediger Pluem <rp...@apache.org>.
On 10/10/2005 05:43 PM, Jim Jagielski wrote:
> For consideration:
[..cut..]
Thanks for your thoughts.
BTW: It was a little bit tricky to apply the patch as my Mozilla
seems to have changed things in the mail spaces / empty lines.
So I guess attached patches are easier to handle :-).
I think I found one big problem with the patch:
When we call ap_proxy_get_worker in line 1316 of proxy_util.c
(ap_proxy_pre_request), then the parameter url already contains
the complete URL we must call on the remote server. Example:
ServerName example.com
ProxyPass /mirror/foo http://backend.com/rsync
This would result in a worker named
http://backend.com/rsync
Calling http://example.com/mirror/foo/bar would be translated to
proxy:http://backend.com/rsync/bar in the translate_name hook of mod_proxy.
Thus ap_proxy_get_worker would be called with the URL http://backend.com/rsync/bar
which would lead to a no match. I guess instead of a simple strcasecmp
we need something like a "longest match" here since I think we cannot rely
on the order the workers got stored. Do you know if a "longest match" function
is already at hand somewhere in apr / apr-util or httpd?
BTW: A similar problem should exist with the current code as in this case
ap_proxy_get_worker tries to compare http://backend.com/ with http://backend.com/rsync
which also fails.
Other smaller problems that need to be solved:
1. Some style and output issues with the manager html interface (e.g.delete column scheme)
I already did this and extended your patch with these things.
Please have a look if you like the design :-).
2. The xml output is currently broken as it only works with schema and hostname.
I am not quite sure if
1. there is an xml schema already defined for this and needs to be adjusted
2. we need to parse the worker name and replace some of the characters with
xml entities.
I think we need to fix this or if not at least a remark in the documentation
should be made that the xml output is currently broken.
Regards
Rüdiger
Re: [PATCH] Bug 36816: balancer_manager doesn't work if worker name contains port
Posted by Jim Jagielski <ji...@jaguNET.com>.
For consideration:
Index: modules/proxy/mod_proxy_balancer.c
===================================================================
--- modules/proxy/mod_proxy_balancer.c (revision 312628)
+++ modules/proxy/mod_proxy_balancer.c (working copy)
@@ -477,8 +477,12 @@
*val++ = '\0';
if ((tok = ap_strchr(val, '&')))
*tok++ = '\0';
+ /*
+ * Special case: workers are allowed path information
+ */
if ((access_status = ap_unescape_url(val)) != OK)
- return access_status;
+ if (strcmp(args, "w") || (access_status !=
HTTP_NOT_FOUND))
+ return access_status;
apr_table_setn(params, args, val);
args = tok;
}
@@ -490,13 +494,9 @@
bsel = ap_proxy_get_balancer(r->pool, conf,
apr_pstrcat(r->pool, "balancer://", name, NULL));
if ((name = apr_table_get(params, "w"))) {
- const char *sc = apr_table_get(params, "s");
- char *asname = NULL;
- proxy_worker *ws = NULL;
- if (sc) {
- asname = apr_pstrcat(r->pool, sc, "://", name, NULL);
- ws = ap_proxy_get_worker(r->pool, conf, asname);
- }
+ proxy_worker *ws;
+
+ ws = ap_proxy_get_worker(r->pool, conf, name);
if (ws) {
worker = (proxy_worker *)bsel->workers->elts;
for (n = 0; n < bsel->workers->nelts; n++) {
@@ -634,10 +634,10 @@
ap_rvputs(r, "<tr>\n<td>", worker->scheme, "</
td><td>", NULL);
ap_rvputs(r, "<a href=\"", r->uri, "?b=",
- balancer->name + sizeof("balancer://") - 1,
- "&s=", worker->scheme, "&w=", worker-
>hostname,
+ balancer->name + sizeof("balancer://") -
1, "&w=",
+ ap_escape_uri(r->pool, worker->name),
"\">", NULL);
- ap_rvputs(r, worker->hostname, "</a></td>", NULL);
+ ap_rvputs(r, worker->name, "</a></td>", NULL);
ap_rvputs(r, "<td>", worker->s->route, NULL);
ap_rvputs(r, "</td><td>", worker->s->redirect, NULL);
ap_rprintf(r, "</td><td>%d</td><td>", worker->s-
>lbfactor);
@@ -678,10 +678,8 @@
ap_rputs(" checked", r);
ap_rputs("></td><tr>\n", r);
ap_rputs("<tr><td colspan=2><input type=submit value=
\"Submit\"></td></tr>\n", r);
- ap_rvputs(r, "</table>\n<input type=hidden name=\"s\" ",
NULL);
- ap_rvputs(r, "value=\"", wsel->scheme, "\">\n", NULL);
- ap_rvputs(r, "<input type=hidden name=\"w\" ", NULL);
- ap_rvputs(r, "value=\"", wsel->hostname, "\">\n", NULL);
+ ap_rvputs(r, "</table>\n<input type=hidden name=\"w\" ",
NULL);
+ ap_rvputs(r, "value=\"", ap_escape_uri(r->pool, wsel-
>name), "\">\n", NULL);
ap_rvputs(r, "<input type=hidden name=\"b\" ", NULL);
ap_rvputs(r, "value=\"", bsel->name + sizeof
("balancer://") - 1,
"\">\n</form>\n", NULL);
Index: modules/proxy/proxy_util.c
===================================================================
--- modules/proxy/proxy_util.c (revision 312628)
+++ modules/proxy/proxy_util.c (working copy)
@@ -1218,9 +1218,6 @@
c = strchr(uri, ':');
if (c == NULL || c[1] != '/' || c[2] != '/' || c[3] == '\0')
return NULL;
- /* remove path from uri */
- if ((c = strchr(c + 3, '/')))
- *c = '\0';
worker = (proxy_worker *)conf->workers->elts;
for (i = 0; i < conf->workers->nelts; i++) {
Re: [PATCH] Bug 36816: balancer_manager doesn't work if worker name contains port
Posted by Jim Jagielski <ji...@jaguNET.com>.
I agree... For some reason in ap_proxy_get_worker() we specifically
skip over the path info, even though it is part of the stored name.
This also means that, afaik, we will miss finding valid workers
when needed.
A forthcoming patch will normalize this.
Re: [PATCH] Bug 36816: balancer_manager doesn't work if worker name
contains port
Posted by Ruediger Pluem <rp...@apache.org>.
This is a follow up to the discussion I started in my comment to PR36816:
I am currently wondering what identifies a proxy worker.
>From what I read in ap_proxy_get_worker in proxy_util.c it is identified by
worker->name. But I see a problem here: The url passed for comparison here
gets reduced effectively to schema, hostname and port of the worker, but
worker->name also contains a path if one was added during configuration
as it is the result of apr_uri_unparse(p, &uri, APR_URI_UNP_REVEALPASSWORD);
As an example I take the one from PR36816:
<Proxy balancer://test1>
BalancerMember http://my.server.com:1234
BalancerMember http://otner.server.com:1234/myapp
</Proxy>
So the worker http://otner.server.com:1234/myapp can never be found by
ap_proxy_get_worker.
>From what I understand from other parts of the code the worker should be identified
by the full URL. So I think the following patch should be applied to ap_proxy_get_worker
Index: proxy_util.c
===================================================================
--- proxy_util.c (Revision 295013)
+++ proxy_util.c (Arbeitskopie)
@@ -1218,9 +1218,6 @@
c = strchr(uri, ':');
if (c == NULL || c[1] != '/' || c[2] != '/' || c[3] == '\0')
return NULL;
- /* remove path from uri */
- if ((c = strchr(c + 3, '/')))
- *c = '\0';
worker = (proxy_worker *)conf->workers->elts;
for (i = 0; i < conf->workers->nelts; i++) {
Last but not least all information that identifies a worker should be displayed
by the manager application.
Thoughts / comments?
Regards
Rüdiger
On 10/05/2005 08:44 PM, Colin Murtaugh wrote:
> Attached is a patch for bug 36816. The balancer_manager interface
> offered by mod_proxy_balancer doesn't work properly if a worker name
> contains a port number.
>
> E.g. if I have configured a cluster as:
>
> <Proxy balancer://testcluster>
> BalancerMember http://server-one.mydomain.com:1234
> BalancerMember http://server-two.mydomain.com:1234
> </Proxy>
>
> then I'm not able to edit the worker settings via the
> balancer_manager. This is because the worker->name is being compared
> to worker->hostname; worker->name contains the port and worker-
>>hostname does not.
>
> I've created the patch below to fix this problem.
>
> --Colin
>
>