<div dir="ltr"><div><div>Hi Maxim,<br></div><div><br>OK, I've implemented your advice about the cache, and some initial testing shows that it works. I have removed all the code that manages the "kcf cahce", and now there is only one default "ngx_http_upstream_keepalive_srv_conf_t".<br>
I have not yet implemented the decoupling from the upstream module, but I'll get to it soon.<br><br></div><div>Here is the improved patch:<br></div><a href="https://gist.github.com/gregvish/6525382/raw/8e0d71a69319d3a9628c903e0112a275b3aff9c7/v2_default_keepalive_patch">https://gist.github.com/gregvish/6525382/raw/8e0d71a69319d3a9628c903e0112a275b3aff9c7/v2_default_keepalive_patch</a><br>
</div><div><br></div><br><div><div><div>You've said the following:<br>> Yes. The sockaddr contains information needed to identify a peer,<br>> and it's already used in multi-server upstream blocks for this.<div class="">
<div id=":1rk" class="" tabindex="0"><img class="" src="https://mail.google.com/mail/u/0/images/cleardot.gif"><br></div><div id=":1rk" class="" tabindex="0">However, in case of SSL connections, it is insufficient to identify a peer according to the sockaddr. The hostname is important. For examlple:<br>
</div><div id=":1rk" class="" tabindex="0"><a href="https://a.host.com">https://a.host.com</a> resolves to <a href="http://1.1.1.1:443">1.1.1.1:443</a><br></div><div id=":1rk" class="" tabindex="0"><a href="https://b.host.com">https://b.host.com</a> also resoves to <a href="http://1.1.1.1:443">1.1.1.1:443</a><br>
</div><div id=":1rk" class="" tabindex="0">If the server at 1.1.1.1 holds an SSL cert _only_ for <a href="http://a.host.com">a.host.com</a>, it would be wrong to use keepalive connections that were opened to this sockaddr for requests for <a href="http://b.host.com">b.host.com</a>. If a connection will not be reused, during SSL handshake the host cert can be properly verified for each new host.<br>
</div><div id=":1rk" class="" tabindex="0">The solution that I implemented for this is to add a "host" field to "ngx_http_upstream_keepalive_cache_t" and "ngx_http_upstream_keepalive_peer_data_t". The function "ngx_http_upstream_get_keepalive_peer" now also checks that the host matches, as well as the sockaddr to reuse a keepalive connection.<br>
<br></div><div id=":1rk" class="" tabindex="0">Please tell me what you think so far.<br><br></div><div id=":1rk" class="" tabindex="0">Thanks,<br></div><div id=":1rk" class="" tabindex="0">Greg<br></div></div></div></div>
</div></div><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Sep 11, 2013 at 4:30 PM, Maxim Dounin <span dir="ltr"><<a href="mailto:mdounin@mdounin.ru" target="_blank">mdounin@mdounin.ru</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello!<br>
<div class="im"><br>
On Wed, Sep 11, 2013 at 03:46:51PM +0300, Greg Vishnepolsky wrote:<br>
<br>
> Hi Maxim, thanks for the prompt reply!<br>
><br>
> > While the patch may work, it looks bad from architectural point of<br>
> > view. It essentially makes upstream keepalive module an integral<br>
> > part of the upstream module, which isn't a good thing (and also<br>
> > will break --without-http_upstream_<br>
> > keepalive_module). The<br>
> > upstream module should provide an interface to do things instead.<br>
><br>
> You're definitely right about this, I haven't thought about that configure<br>
> option. How do you suggest to decouple the code? Perhaps add some kind of<br>
> callback to the proxy configuration and expose a setter interface?<br>
<br>
</div>I think right aproach would be to expose some kind of "default"<br>
upstream which can be used by modules / configured by users. Not<br>
sure how exactly this should be done from user point of view<br>
though.<br>
<div class="im"><br>
> > Also, it looks like the patch adds lots of code duplication.<br>
> > The code to check peer address and lookup a connection in the<br>
> > cache is already present in the upstream keepalive module, and it<br>
> > should be used instead of adding another structures/code to do the<br>
> > same task.<br>
><br>
> When you're saying "is already present", are you referring to the code in<br>
> "ngx_http_upstream_get_keepalive_peer", where "item->sockaddr" is being<br>
> compared, as the key to the connection cache?<br>
> If so, I'll try to see if it works in the described case. Perhaps a<br>
> hostname should be added as another "uniqueness" identifier to this cache<br>
> in addition to "sockaddr"? Then a single<br>
> "ngx_http_upstream_keepalive_srv_conf_t" can be used for many hosts?<br>
> If you believe that this should work, I agree that this is a better way to<br>
> do the patch.<br>
<br>
</div>Yes. The sockaddr contains information needed to identify a peer,<br>
and it's already used in multi-server upstream blocks for this.<br>
<div class="HOEnZb"><div class="h5"><br>
--<br>
Maxim Dounin<br>
<a href="http://nginx.org/en/donation.html" target="_blank">http://nginx.org/en/donation.html</a><br>
<br>
_______________________________________________<br>
nginx-devel mailing list<br>
<a href="mailto:nginx-devel@nginx.org">nginx-devel@nginx.org</a><br>
<a href="http://mailman.nginx.org/mailman/listinfo/nginx-devel" target="_blank">http://mailman.nginx.org/mailman/listinfo/nginx-devel</a><br>
</div></div></blockquote></div><br></div>