Hi Maxim, 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". I have not yet implemented the decoupling from the upstream module, but I'll get to it soon.
Here is the improved patch: https://gist.github.com/gregvish/6525382/raw/8e0d71a69319d3a9628c903e0112a275b3aff9c7/v2_default_keepalive_patch You've said the following: > Yes. The sockaddr contains information needed to identify a peer, > and it's already used in multi-server upstream blocks for this. However, in case of SSL connections, it is insufficient to identify a peer according to the sockaddr. The hostname is important. For examlple: https://a.host.com resolves to 1.1.1.1:443 https://b.host.com also resoves to 1.1.1.1:443 If the server at 1.1.1.1 holds an SSL cert _only_ for a.host.com, it would be wrong to use keepalive connections that were opened to this sockaddr for requests for b.host.com. If a connection will not be reused, during SSL handshake the host cert can be properly verified for each new host. 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. Please tell me what you think so far. Thanks, Greg On Wed, Sep 11, 2013 at 4:30 PM, Maxim Dounin <[email protected]> wrote: > Hello! > > On Wed, Sep 11, 2013 at 03:46:51PM +0300, Greg Vishnepolsky wrote: > > > Hi Maxim, thanks for the prompt reply! > > > > > While the patch may work, it looks bad from architectural point of > > > view. It essentially makes upstream keepalive module an integral > > > part of the upstream module, which isn't a good thing (and also > > > will break --without-http_upstream_ > > > keepalive_module). The > > > upstream module should provide an interface to do things instead. > > > > You're definitely right about this, I haven't thought about that > configure > > option. How do you suggest to decouple the code? Perhaps add some kind of > > callback to the proxy configuration and expose a setter interface? > > I think right aproach would be to expose some kind of "default" > upstream which can be used by modules / configured by users. Not > sure how exactly this should be done from user point of view > though. > > > > Also, it looks like the patch adds lots of code duplication. > > > The code to check peer address and lookup a connection in the > > > cache is already present in the upstream keepalive module, and it > > > should be used instead of adding another structures/code to do the > > > same task. > > > > When you're saying "is already present", are you referring to the code in > > "ngx_http_upstream_get_keepalive_peer", where "item->sockaddr" is being > > compared, as the key to the connection cache? > > If so, I'll try to see if it works in the described case. Perhaps a > > hostname should be added as another "uniqueness" identifier to this cache > > in addition to "sockaddr"? Then a single > > "ngx_http_upstream_keepalive_srv_conf_t" can be used for many hosts? > > If you believe that this should work, I agree that this is a better way > to > > do the patch. > > Yes. The sockaddr contains information needed to identify a peer, > and it's already used in multi-server upstream blocks for this. > > -- > Maxim Dounin > http://nginx.org/en/donation.html > > _______________________________________________ > nginx-devel mailing list > [email protected] > http://mailman.nginx.org/mailman/listinfo/nginx-devel >
_______________________________________________ nginx-devel mailing list [email protected] http://mailman.nginx.org/mailman/listinfo/nginx-devel
