On Wed, Feb 14, 2018 at 1:28 PM, Graham Leggett <minf...@sharp.fm> wrote:
> On 14 Feb 2018, at 1:03 PM, Yann Ylavic <ylavic....@gmail.com> wrote:
>>
>> The docs talk about connection based config, while ap_server_conf is
>> really the main server config.
>> The code should be improved to be based on c->baser_server config
>> (with merging of RemoteIPProxyProtocol*), unless I'm missing something
>> it seems (as of now) that the directives overwrite each other when
>> used in vhost context (not only for name-based vhosts).
>> So now (or post-backport) I think we should at least document the
>> scope as being "server config" only, and follow up with
>> "c->baser_server config" when possible (not a blocker for the first
>> version).
>
> The docs explain the above here, which makes sense to me:
>
>     <p>While this directive may be specified in any virtual host, it is
>     important to understand that because the proxy protocol is connection
>     based and protocol agnostic, the enabling and disabling is actually
> based
>     on ip-address and port. This means that if you have multiple name-based
>     virtual hosts for the same host and port, and you enable it any one of
>     them, then it is enabled for all them (with that host and port). It also
>     means that if you attempt to enable the proxy protocol in one and
> disable
>     in the other, that won't work; in such a case the last one wins and a
>     notice will be logged indicating which setting was being overridden.</p>

It makes sense, and actually I missed some logic w.r.t.
enabled/disabled being a list of sockaddrs (based on servers'
server_addr_rec, and not a global boolean as I first thought...). This
is later compared to incoming connections local addr.
Let me grok that... but at first glance it looks quite cumbersome, why
not the usual server merging and a simple "enabled" boolean?
If we only care about the config of the base server,
"ap_get_module_config(c->base_server->module_config,
&remoteip_module)" could do it no?
For now it seems that all the vhost selection logic is duplicated, but
indeed it's not global (nor really per vhost, but yes this is thee
scope which comes closest).


Regards,
Yann.

Reply via email to