httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jan Kaluža <jkal...@redhat.com>
Subject Re: [PATCH] Fix settings options with ProxyPassMatch
Date Tue, 29 Apr 2014 11:41:43 GMT
On 04/29/2014 01:04 PM, Jim Jagielski wrote:
>
> On Apr 24, 2014, at 8:57 PM, Yann Ylavic <ylavic.dev@gmail.com> wrote:
>
>> Hi Jan,
>>
>> sorry for the late.
>>
>> On Tue, Apr 22, 2014 at 3:39 PM, Jan Kaluža <jkaluza@redhat.com> wrote:
>>> Hi again,
>>>
>>> the patch has been here for some time already. I hesitate to commit it to
>>> trunk without any review, because it changes the core code in mod_proxy and
>>> I'm afraid that there could exist more corner-cases I'm not aware of.
>>>
>>> On the other side (and to motivate someone with deeper knowledge of
>>> mod_proxy :) ), it would be great to have UDS support also for
>>> ProxyPassMatch and fix some old bugs related to setting options together
>>> with ProxyPassMatch (like PR 43513 and PR 54973).
>>>
>>> So, anyone who would have time to review this patch, please? :)
>>
>> I slightly modified your patch with the following changes :
>> 1. Use \-escaping instead of %-escaping for wildcard_match so to
>> consume less space and unescape quickly.
>
> Wouldn't this cause confusion/issue with normal escaping?
>
>> 2. Fix the recursion in ap_proxy_wildcard_match(), which was calling
>> the legacy ap_strcmp_match(), and unescape according to 1.
>
> +1
>
>> 2'. Shouldn't this function be made iterative (it is not final
>> recursive currently)?
>
> +1
>
>> 3. Always try to get the worker with its de_socketfy()ed name, and
>> only if not found use ap_proxy_define[_wildcard]_worker() with the
>> configured URL (add_pass() and proxysection() were not consistent
>> about ap_proxy_get_worker()'s given name/URL).
>
> Makes sense.
>
>> 4. Don't match both the "normal" name with strcnmp() and the wildcard
>> name with strcmp_match() in ap_proxy_get_worker(), since the worker is
>> either wildcard or not (this enforcement is also added to add_pass()
>> and proxysection() so that it is not possible to declare the same
>> worker name once with a Match, again without, or vice versa).
>> 4'. Does it makes sense to use the longest match_name or should we go
>> for the first matching wildcard worker (if any) wins?
>
> Longest match name, imo.
>
>> 5. Rename ap_proxy_wildcard_match() as ap_proxy_strcmp_ematch() to
>> suggest it could go to server/util.ch (if/when the associated
>> ap_matchexp_escape() and ap_proxy_strcasecmp_ematch() are coded).
>
> Why?
>
>> 6. Move proxy_worker_shared { char
>> wildcard_name[PROXY_WORKER_MAX_NAME_SIZE]; } to proxy_worker { char
>> *match_name; } to save shm space and avoid length issues (I don't see
>> how this could be updated at runtime, for now, balancer-manager can't
>> declare Match workers).
>
> Future effort, so -1 on the moving.
>
>> 6. Keep ap_proxy_escape_wildcard_url() static (private) since it is
>> not used outside proxy_util (now).
>
> Sounds good.
>>
>> This is not very tested (yet), I'll come back if I detect issues, but
>> first I'd like your feedbacks on this modifications...
>>
>> Patch attached.
>>
>
> Just a comment: I am really concerned that with such a major change,
> we will likely break things in ways we are unaware of... We should
> focus on fixing the actual problem w/o also combining it with
> other "fixes" that are tangentially involved. For example, maybe
> a simply flag when creating the worker that sez "worker name is a regex"
> is the way to approach it, and thus ap_proxy_get_worker() could
> add additional checks for finding the "right" worker when it
> knows it needs to allow for regex substitutions. That creates
> a additional logic path which is self-contained and isolated, instead
> of monkeying around with changing current paths...
>

That's what we do with current patch I think, don't we? In the patch, we 
create "char *match_name" which is NULL when the worker_name is not 
regex and contains the escaped name if regex is used (with "$N" replaced 
by '*').

ap_proxy_get_worker() later checks the match_name and if it's not NULL, 
it tries regex matching using ap_proxy_strcmp_ematch().

The actual problem is that ap_proxy_get_worker() cannot find out proper 
proxy_worker, because this method is not regex aware. To make it regex 
aware, we have to distinguish regex workers during creation, replace the 
regex variables ($N -> *) in some safe manner and allow 
ap_proxy_get_worker() to find the right worker using this new info. And 
that's what the patch does.

Regards,
Jan Kaluza


Mime
View raw message