On Tue, Feb 15, 2011 at 12:26:54AM +0100, Craig wrote: > Hi, > > > Thanks to all your tests and observations, I managed to spot the bug and to > > fix it. The headers are linked in a list whose tail is known as > > hdr_idx->tail. > > This pointer is used when adding new headers. Unfortunately the header > > removal > > function did not update it when it removed the last header. The effect is > > that > > when adding the x-forwarded-for header, it's added just after the previously > > removed Connection header so it does not appear in the list when walked from > > the beginning. It's really when you said that it broke when Connection was > > the last header that I understood what to look for. Thanks ! > I've confirmed it's working fine now, and successfully deployed the new > version yesterday in our live environment. Many thanks for fixing it! :)
And thanks for confirming :-) > Sidenote to list-readers: Do not use stunnel with the out-of-tree > x-forwarded-for patch for SSL termination if you want to balance based > on that header with two haproxy frontends and one backend. Stunnel will > add an additional X-Forwarded-For header even if your clients send one. Yes and that's expected, otherwise you'd get random addresses, sometimes the one added by stunnel, sometimes the one the client's sets. The real issue in fact is that "balance hdr()" was never meant to be used to hash a header that is allowed to appear multiple times. Right now it takes the first occurrence. Ideally we should improve it so that it can work as "usesrc hdr_ip()" and that we can pass it the occurrence number. That way we'd be able to ask for the last occurrence only. But some changes are still required to be able to do that with any header. I'd say that the real issue with the stunnel x-forwarded-for patch is that it is not compatible with keep-alive. It will only modify the first request in the connection, so that can become a problem when hashing as you do if keep-alive is enabled. Regards, Willy
