Fwd: Automatic pooling of upstream keepalive connections (patch proposal)

[Greg Vishnepolsky]

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 <mdounin at mdounin.ru> 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
> nginx-devel at nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20130911/47db44de/attachment.html>