Re: [PATCH] MINOR?: stconn/connection: Fix suspect change causing timeouts
On Mon, Jun 10, 2024, at 3:29 PM, Christopher Faulet wrote: > Le 05/06/2024 à 22:55, William Manley a écrit : > > This fixes an issue I've had where if a connection was idle for ~23s > > it would get in a bad state. I don't understand this code, so I'm > > not sure exactly why it was failing. > > > > I discovered this by bisecting to identify the commit that caused the > > regression between 2.9 and 3.0. The commit is > > d2c3f8dde7c2474616c0ea51234e6ba9433a4bc1: "MINOR: stconn/connection: > > Move shut modes at the SE descriptor level" - a part of v3.0-dev8. > > It seems to be an innocent renaming, so I looked through it and this > > stood out as suspect: > > > > -if (mode != CO_SHW_NORMAL) > > +if (mode & SE_SHW_NORMAL) > > > > It looks like the not went missing here, so this patch reverses that > > condition. It fixes my test. > > > > I don't quite understand what this is doing or is for so I can't write > > a regression test or decent commit message. Hopefully someone else > > will be able to pick this up from where I've left it. > > --- > > src/mux_h1.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/src/mux_h1.c b/src/mux_h1.c > > index 881fff77b..6714d4e98 100644 > > --- a/src/mux_h1.c > > +++ b/src/mux_h1.c > > @@ -4320,7 +4320,7 @@ static void h1_shutw(struct stconn *sc, enum > > se_shut_mode mode) > > > > do_shutw: > > h1_close(h1c); > > - if (mode & SE_SHW_NORMAL) > > + if (!(mode & SE_SHW_NORMAL)) > > h1c->flags |= H1C_F_SILENT_SHUT; > > > > if (!b_data(>obuf)) > > Thanks! > > I merged your fix. I choose to keep your message with just a comment at the > end. > However, I changed the subject to "BUG/MEDIUM: stconn/mux-h1: Fix suspect > change > causing timeouts". > > It is part of the 3.0.1. Thanks Will --- William Manley Stb-tester.com Stb-tester.com Ltd is a company registered in England and Wales. Registered number: 08800454. Registered office: 13B The Vale, London, W3 7SH, United Kingdom (This is not a remittance address.)
[PATCH] MINOR?: stconn/connection: Fix suspect change causing timeouts
This fixes an issue I've had where if a connection was idle for ~23s it would get in a bad state. I don't understand this code, so I'm not sure exactly why it was failing. I discovered this by bisecting to identify the commit that caused the regression between 2.9 and 3.0. The commit is d2c3f8dde7c2474616c0ea51234e6ba9433a4bc1: "MINOR: stconn/connection: Move shut modes at the SE descriptor level" - a part of v3.0-dev8. It seems to be an innocent renaming, so I looked through it and this stood out as suspect: -if (mode != CO_SHW_NORMAL) +if (mode & SE_SHW_NORMAL) It looks like the not went missing here, so this patch reverses that condition. It fixes my test. I don't quite understand what this is doing or is for so I can't write a regression test or decent commit message. Hopefully someone else will be able to pick this up from where I've left it. --- src/mux_h1.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/mux_h1.c b/src/mux_h1.c index 881fff77b..6714d4e98 100644 --- a/src/mux_h1.c +++ b/src/mux_h1.c @@ -4320,7 +4320,7 @@ static void h1_shutw(struct stconn *sc, enum se_shut_mode mode) do_shutw: h1_close(h1c); - if (mode & SE_SHW_NORMAL) + if (!(mode & SE_SHW_NORMAL)) h1c->flags |= H1C_F_SILENT_SHUT; if (!b_data(>obuf)) -- 2.34.1
Re: [PATCH v2] MINOR: config: rhttp: Don't require SSL when attach-srv name parsing
On Thu, May 23, 2024, at 3:52 PM, William Manley wrote: > On Thu, May 23, 2024, at 3:45 PM, Amaury Denoyelle wrote: > > On Thu, May 23, 2024 at 02:47:15PM +0100, William Manley wrote: > > > On Thu, May 23, 2024, at 2:08 PM, Amaury Denoyelle wrote: > > > > On Thu, May 23, 2024 at 11:55:13AM +0100, William Manley wrote: > > > > > On Thu, May 23, 2024, at 11:34 AM, William Manley wrote: > > > > > > On Thu, May 23, 2024, at 10:08 AM, Amaury Denoyelle wrote: > > > > > > > On Wed, May 22, 2024 at 04:58:44PM +0100, William Manley wrote: > > > > > > > > On Wed, May 22, 2024, at 1:06 PM, Amaury Denoyelle wrote: > > > > > > > > > FYI, I just merged a series of fix to improve reverse HTTP. > > > > > > > > > It is now > > > > > > > > > possible to use PROXY protocol on preconnect stage. Also, you > > > > > > > > > have the > > > > > > > > > availability to use PROXY v2 TLV to differentiate > > > > > > > > > connections. Note > > > > > > > > > however that PROXY unique-id is not supported for now due to > > > > > > > > > internal > > > > > > > > > API limitations. > > > > > > > > > > If you can do not hesitate to test this and report us if > > > > > > > > > > it's sufficient > > > > > > > > > for you. > > > > > > > > I've just tried this out and there's something about these > > > > > > > > changes that are causing my tests to fail. > > > > > > > > It seems to be triggered by "MEDIUM: rhttp: create session for > > > > > > > > active preconnect" > > > > > > > > Tested versions: > > > > > > > > eb89a7da33ae30da3ed61570aa1597987b59dff3 OK > > > > > > > > ceebb09744df367ad84586a341d9336f84f72bce OK > > > > > > > > 45b80aed70a597614e31b748328570785099dfec OK > > > > > > > > 12c40c25a9520fe3365950184fe724a1f4e91d03 BAD > > > > > > > > 60496e884e5220b9330a1d8b3a1218f7988c879a BAD > > > > > > > > 4a968d9d274a24e5d00bd3c03dd22fe2563b13af BAD > > > > > > > > I'll investigate further tomorrow. > > > > > > > > > Hum can you describe what sort of tests you are running ? > > > > > > > > > > > > [...] > > > > > > > > > > > > I've attached the configs used during the test. You'll notice that > > > > > > each block uses a different IP address from 127.0.0.*. This makes > > > > > > the TCP flow graph in wireshark meaningful. I've attached a packet > > > > > > capture of the test failing. I'd also attach a packet capture from > > > > > > it succeeding, but it's a bit big. I'll look into them and try to > > > > > > figure out where it's going wrong. > > > > > I've compared the good trace and the bad trace and I think I can see > > > > > where this is going wrong. My RHTTP block on the node looks like: > > > > > # Requests from the portal to the node arrive at this frontend over > > > > > rhttp: > > > > > listen flask_reverse_proxy > > > > > mode http > > > > > log global > > > > > bind rhttp@rhttp_backend/srv > > > > > # We dynamically route to the listening socket based on the hostname, > > > > > this > > > > > # way we can add new domains without a node deploy: > > > > > http-request capture req.hdr(X-Remote-IP) len 15 > > > > > http-request capture req.hdr(X-Remote-Port) len 5 > > > > > http-request set-dst hdr(X-Remote-IP) > > > > > http-request set-dst-port hdr(X-Remote-Port) > > > > > http-response add-header Vary X-Remote-IP,X-Remote-Port > > > > > server srv 0.0.0.0:0 source 127.0.0.8 > > > > > It seems like with the active preconnect commit (12c40c25) it ignores > > > > > set-dst, so instead of connecting to the IP requested (in this case > > > > > 127.0.0.7) it attempts to connect to 127.0.0.8. > > > > > Interesting. Could you please try the attached patch and tell me if > > > > > this > > > > solves your issue ? Many thanks, > > > It doesn't fix it. It fails in the same way. > > > > Thank you for your quick testing. I have manage to reproduce what I > > think could be a similar issue. I have updated my patch, can you please > > try the following new one ? > > Yes, that fixes it. > > Let me know whenever you want something tested, the way I've got it set up > it's pretty easy for me to do so. I can also report that I no longer need to avoid `nbthread 1` in the config on the node. Presumably thanks to ceebb09744df367ad84586a341d9336f84f72bce "rhttp: fix preconnect on single-thread". Thanks Will --- William Manley Stb-tester.com Stb-tester.com Ltd is a company registered in England and Wales. Registered number: 08800454. Registered office: 13B The Vale, London, W3 7SH, United Kingdom (This is not a remittance address.)
Re: [PATCH v2] MINOR: config: rhttp: Don't require SSL when attach-srv name parsing
On Thu, May 23, 2024, at 3:45 PM, Amaury Denoyelle wrote: > On Thu, May 23, 2024 at 02:47:15PM +0100, William Manley wrote: > > On Thu, May 23, 2024, at 2:08 PM, Amaury Denoyelle wrote: > > > On Thu, May 23, 2024 at 11:55:13AM +0100, William Manley wrote: > > > > On Thu, May 23, 2024, at 11:34 AM, William Manley wrote: > > > > > On Thu, May 23, 2024, at 10:08 AM, Amaury Denoyelle wrote: > > > > > > On Wed, May 22, 2024 at 04:58:44PM +0100, William Manley wrote: > > > > > > > On Wed, May 22, 2024, at 1:06 PM, Amaury Denoyelle wrote: > > > > > > > > FYI, I just merged a series of fix to improve reverse HTTP. It > > > > > > > > is now > > > > > > > > possible to use PROXY protocol on preconnect stage. Also, you > > > > > > > > have the > > > > > > > > availability to use PROXY v2 TLV to differentiate connections. > > > > > > > > Note > > > > > > > > however that PROXY unique-id is not supported for now due to > > > > > > > > internal > > > > > > > > API limitations. > > > > > > > > > If you can do not hesitate to test this and report us if it's > > > > > > > > > sufficient > > > > > > > > for you. > > > > > > > I've just tried this out and there's something about these > > > > > > > changes that are causing my tests to fail. > > > > > > > It seems to be triggered by "MEDIUM: rhttp: create session for > > > > > > > active preconnect" > > > > > > > Tested versions: > > > > > > > eb89a7da33ae30da3ed61570aa1597987b59dff3 OK > > > > > > > ceebb09744df367ad84586a341d9336f84f72bce OK > > > > > > > 45b80aed70a597614e31b748328570785099dfec OK > > > > > > > 12c40c25a9520fe3365950184fe724a1f4e91d03 BAD > > > > > > > 60496e884e5220b9330a1d8b3a1218f7988c879a BAD > > > > > > > 4a968d9d274a24e5d00bd3c03dd22fe2563b13af BAD > > > > > > > I'll investigate further tomorrow. > > > > > > > > Hum can you describe what sort of tests you are running ? > > > > > > > > > > [...] > > > > > > > > > > I've attached the configs used during the test. You'll notice that > > > > > each block uses a different IP address from 127.0.0.*. This makes > > > > > the TCP flow graph in wireshark meaningful. I've attached a packet > > > > > capture of the test failing. I'd also attach a packet capture from > > > > > it succeeding, but it's a bit big. I'll look into them and try to > > > > > figure out where it's going wrong. > > > > I've compared the good trace and the bad trace and I think I can see > > > > where this is going wrong. My RHTTP block on the node looks like: > > > > # Requests from the portal to the node arrive at this frontend over > > > > rhttp: > > > > listen flask_reverse_proxy > > > > mode http > > > > log global > > > > bind rhttp@rhttp_backend/srv > > > > # We dynamically route to the listening socket based on the hostname, > > > > this > > > > # way we can add new domains without a node deploy: > > > > http-request capture req.hdr(X-Remote-IP) len 15 > > > > http-request capture req.hdr(X-Remote-Port) len 5 > > > > http-request set-dst hdr(X-Remote-IP) > > > > http-request set-dst-port hdr(X-Remote-Port) > > > > http-response add-header Vary X-Remote-IP,X-Remote-Port > > > > server srv 0.0.0.0:0 source 127.0.0.8 > > > > It seems like with the active preconnect commit (12c40c25) it ignores > > > > set-dst, so instead of connecting to the IP requested (in this case > > > > 127.0.0.7) it attempts to connect to 127.0.0.8. > > > > Interesting. Could you please try the attached patch and tell me if this > > > solves your issue ? Many thanks, > > It doesn't fix it. It fails in the same way. > > Thank you for your quick testing. I have manage to reproduce what I > think could be a similar issue. I have updated my patch, can you please > try the following new one ? Yes, that fixes it. Let me know whenever you want something tested, the way I've got it set up it's pretty easy for me to do so. Note to self for next time: make -C haproxy/haproxy/ ARCH=amd64 V=1 DESTDIR=/dest PREFIX=/usr IGNOREGIT=true MANDIR=/usr/share/man DOCDIR=/usr/share/doc/haproxy USE_PCRE2=1 USE_PCRE2_JIT=1 USE_OPENSSL=1 USE_SLZ=1 USE_LUA=1 USE_PROMEX=1 LUA_LIB_NAME=lua5.3 EXTRA=admin/halog/halog TARGET=linux-glibc USE_SYSTEMD=1 ADDLIB="-latomic" -j12 && PYTHONPATH=$PWD/pythonpath PATH=$PWD/haproxy/haproxy/:$PATH pytest-3 central-server/selftests/test_haproxy.py Thanks Will --- William Manley Stb-tester.com Stb-tester.com Ltd is a company registered in England and Wales. Registered number: 08800454. Registered office: 13B The Vale, London, W3 7SH, United Kingdom (This is not a remittance address.)
Re: [PATCH v2] MINOR: config: rhttp: Don't require SSL when attach-srv name parsing
On Thu, May 23, 2024, at 2:08 PM, Amaury Denoyelle wrote: > On Thu, May 23, 2024 at 11:55:13AM +0100, William Manley wrote: > > On Thu, May 23, 2024, at 11:34 AM, William Manley wrote: > > > On Thu, May 23, 2024, at 10:08 AM, Amaury Denoyelle wrote: > > > > On Wed, May 22, 2024 at 04:58:44PM +0100, William Manley wrote: > > > > > On Wed, May 22, 2024, at 1:06 PM, Amaury Denoyelle wrote: > > > > > > FYI, I just merged a series of fix to improve reverse HTTP. It is > > > > > > now > > > > > > possible to use PROXY protocol on preconnect stage. Also, you have > > > > > > the > > > > > > availability to use PROXY v2 TLV to differentiate connections. Note > > > > > > however that PROXY unique-id is not supported for now due to > > > > > > internal > > > > > > API limitations. > > > > > > > If you can do not hesitate to test this and report us if it's > > > > > > > sufficient > > > > > > for you. > > > > > I've just tried this out and there's something about these changes > > > > > that are causing my tests to fail. > > > > > It seems to be triggered by "MEDIUM: rhttp: create session for active > > > > > preconnect" > > > > > Tested versions: > > > > > eb89a7da33ae30da3ed61570aa1597987b59dff3 OK > > > > > ceebb09744df367ad84586a341d9336f84f72bce OK > > > > > 45b80aed70a597614e31b748328570785099dfec OK > > > > > 12c40c25a9520fe3365950184fe724a1f4e91d03 BAD > > > > > 60496e884e5220b9330a1d8b3a1218f7988c879a BAD > > > > > 4a968d9d274a24e5d00bd3c03dd22fe2563b13af BAD > > > > > I'll investigate further tomorrow. > > > > > > Hum can you describe what sort of tests you are running ? > > > > > > [...] > > > > > > I've attached the configs used during the test. You'll notice that each > > > block uses a different IP address from 127.0.0.*. This makes the TCP > > > flow graph in wireshark meaningful. I've attached a packet capture of > > > the test failing. I'd also attach a packet capture from it succeeding, > > > but it's a bit big. I'll look into them and try to figure out where it's > > > going wrong. > > I've compared the good trace and the bad trace and I think I can see where > > this is going wrong. My RHTTP block on the node looks like: > > # Requests from the portal to the node arrive at this frontend over rhttp: > > listen flask_reverse_proxy > > mode http > > log global > > bind rhttp@rhttp_backend/srv > > # We dynamically route to the listening socket based on the hostname, this > > # way we can add new domains without a node deploy: > > http-request capture req.hdr(X-Remote-IP) len 15 > > http-request capture req.hdr(X-Remote-Port) len 5 > > http-request set-dst hdr(X-Remote-IP) > > http-request set-dst-port hdr(X-Remote-Port) > > http-response add-header Vary X-Remote-IP,X-Remote-Port > > server srv 0.0.0.0:0 source 127.0.0.8 > > It seems like with the active preconnect commit (12c40c25) it ignores > > set-dst, so instead of connecting to the IP requested (in this case > > 127.0.0.7) it attempts to connect to 127.0.0.8. > > Interesting. Could you please try the attached patch and tell me if this > solves your issue ? Many thanks, It doesn't fix it. It fails in the same way. Thanks Will --- William Manley Stb-tester.com Stb-tester.com Ltd is a company registered in England and Wales. Registered number: 08800454. Registered office: 13B The Vale, London, W3 7SH, United Kingdom (This is not a remittance address.)
Re: [PATCH v2] MINOR: config: rhttp: Don't require SSL when attach-srv name parsing
On Thu, May 23, 2024, at 11:34 AM, William Manley wrote:
> On Thu, May 23, 2024, at 10:08 AM, Amaury Denoyelle wrote:
> > On Wed, May 22, 2024 at 04:58:44PM +0100, William Manley wrote:
> > > On Wed, May 22, 2024, at 1:06 PM, Amaury Denoyelle wrote:
> > > > FYI, I just merged a series of fix to improve reverse HTTP. It is now
> > > > possible to use PROXY protocol on preconnect stage. Also, you have the
> > > > availability to use PROXY v2 TLV to differentiate connections. Note
> > > > however that PROXY unique-id is not supported for now due to internal
> > > > API limitations.
> > > > > If you can do not hesitate to test this and report us if it's
> > > > > sufficient
> > > > for you.
> > > I've just tried this out and there's something about these changes that
> > > are causing my tests to fail.
> > > It seems to be triggered by "MEDIUM: rhttp: create session for active
> > > preconnect"
> > > Tested versions:
> > > eb89a7da33ae30da3ed61570aa1597987b59dff3 OK
> > > ceebb09744df367ad84586a341d9336f84f72bce OK
> > > 45b80aed70a597614e31b748328570785099dfec OK
> > > 12c40c25a9520fe3365950184fe724a1f4e91d03 BAD
> > > 60496e884e5220b9330a1d8b3a1218f7988c879a BAD
> > > 4a968d9d274a24e5d00bd3c03dd22fe2563b13af BAD
> > > I'll investigate further tomorrow.
> >
> > Hum can you describe what sort of tests you are running ?
>
> [...]
>
> I've attached the configs used during the test. You'll notice that each
> block uses a different IP address from 127.0.0.*. This makes the TCP flow
> graph in wireshark meaningful. I've attached a packet capture of the test
> failing. I'd also attach a packet capture from it succeeding, but it's a bit
> big. I'll look into them and try to figure out where it's going wrong.
I've compared the good trace and the bad trace and I think I can see where this
is going wrong. My RHTTP block on the node looks like:
# Requests from the portal to the node arrive at this frontend over
rhttp:
listen flask_reverse_proxy
mode http
log global
bind rhttp@rhttp_backend/srv
# We dynamically route to the listening socket based on the
hostname, this
# way we can add new domains without a node deploy:
http-request capture req.hdr(X-Remote-IP) len 15
http-request capture req.hdr(X-Remote-Port) len 5
http-request set-dst hdr(X-Remote-IP)
http-request set-dst-port hdr(X-Remote-Port)
http-response add-header Vary X-Remote-IP,X-Remote-Port
server srv 0.0.0.0:0 source 127.0.0.8
It seems like with the active preconnect commit (12c40c25) it ignores set-dst,
so instead of connecting to the IP requested (in this case 127.0.0.7) it
attempts to connect to 127.0.0.8.
Thanks
Will
---
William Manley
Stb-tester.com
Stb-tester.com Ltd is a company registered in England and Wales.
Registered number: 08800454. Registered office: 13B The Vale,
London, W3 7SH, United Kingdom (This is not a remittance address.)
Re: [PATCH v2] MINOR: config: rhttp: Don't require SSL when attach-srv name parsing
On Thu, May 23, 2024, at 10:08 AM, Amaury Denoyelle wrote: > On Wed, May 22, 2024 at 04:58:44PM +0100, William Manley wrote: > > On Wed, May 22, 2024, at 1:06 PM, Amaury Denoyelle wrote: > > > FYI, I just merged a series of fix to improve reverse HTTP. It is now > > > possible to use PROXY protocol on preconnect stage. Also, you have the > > > availability to use PROXY v2 TLV to differentiate connections. Note > > > however that PROXY unique-id is not supported for now due to internal > > > API limitations. > > > > If you can do not hesitate to test this and report us if it's sufficient > > > for you. > > I've just tried this out and there's something about these changes that are > > causing my tests to fail. > > It seems to be triggered by "MEDIUM: rhttp: create session for active > > preconnect" > > Tested versions: > > eb89a7da33ae30da3ed61570aa1597987b59dff3 OK > > ceebb09744df367ad84586a341d9336f84f72bce OK > > 45b80aed70a597614e31b748328570785099dfec OK > > 12c40c25a9520fe3365950184fe724a1f4e91d03 BAD > > 60496e884e5220b9330a1d8b3a1218f7988c879a BAD > > 4a968d9d274a24e5d00bd3c03dd22fe2563b13af BAD > > I'll investigate further tomorrow. > > Hum can you describe what sort of tests you are running ? Ok. There's a fair amount of background to explain, apologies in advance for length. Background === My company sells a product with a hardware and a cloud component. The cloud component is called "portal" and the hardware component is called "node". The nodes run in our customer's premises on their network, while the portal runs on AWS. The portal sends commands to the node and the node sends results and status to the portal. Because we don't control the network environment of the node we want it to be as simple as possible - so all communication from the node to the portal is done via HTTP requests multiplexed over a single HTTP/2 TCP connection made from the node to the portal. This keeps the network setup simple and compatible. The downside of the setup is that it makes communication difficult - we need to use long-poll from the node which is complicated and can be slow and there's a degree of friction whenever we adding new features. We'd much prefer to just be able to use HTTP requests from the portal to the node. So I came up with this idea of using HTTP CONNECT to establish TCP connections from the node to the portal multiplexed over HTTP/2, but then reversing the flow and allowing requests to be sent from the portal to the node. In the process of testing this I noticed that HAProxy had recently added RHTTP support, with benefits beyond what I'd implemented, so I thought I'd use that instead. Roughly speaking the setup looks like this (hopefully it won't be mangled): │ ││ ││ │ │python │ HAProxy │ internet │ HAProxy │ python │ │ ││ ││ │ ┌─┐ ┌───┐ │ portal│--► node │ │ requests │HTTP/1│ flask │ └──╥──┘ └─▲─┘ ║ ┌───┐┌───┐║ ╚═► RHTTP │► flask reverse ╞╝ HTTP/1 │frontend │ HTTP/2 │ proxy │ HTTP/1 └───▲───┘└───▲───┘ || ┌──┐┌──┐ │ pool ││ pool │ └──▲───┘└───▲──┘ || ┌───┐┌───┐ │add HTTP ◄│ RHTTP │ │ pool │ RHTTP │backend│ └───▲───┘└───╥───┘ PROXY+RHTTP ║║ RHTTP ┌───╨───┐┌───▼───┐ │CONNECT◄│CONNECT│ │ terminator │ HTTP/1 CONNECT │ payloader │ └───▲───┘└───╥───┘ HTTP/1 CONNECT ║║ HTTP/1 CONNECT ┌
Re: [PATCH v2] MINOR: config: rhttp: Don't require SSL when attach-srv name parsing
On Wed, May 22, 2024, at 1:06 PM, Amaury Denoyelle wrote: > FYI, I just merged a series of fix to improve reverse HTTP. It is now > possible to use PROXY protocol on preconnect stage. Also, you have the > availability to use PROXY v2 TLV to differentiate connections. Note > however that PROXY unique-id is not supported for now due to internal > API limitations. > > If you can do not hesitate to test this and report us if it's sufficient > for you. I've just tried this out and there's something about these changes that are causing my tests to fail. It seems to be triggered by "MEDIUM: rhttp: create session for active preconnect" Tested versions: eb89a7da33ae30da3ed61570aa1597987b59dff3 OK ceebb09744df367ad84586a341d9336f84f72bce OK 45b80aed70a597614e31b748328570785099dfec OK 12c40c25a9520fe3365950184fe724a1f4e91d03 BAD 60496e884e5220b9330a1d8b3a1218f7988c879a BAD 4a968d9d274a24e5d00bd3c03dd22fe2563b13af BAD I'll investigate further tomorrow. Thanks Will --- William Manley Stb-tester.com Stb-tester.com Ltd is a company registered in England and Wales. Registered number: 08800454. Registered office: 13B The Vale, London, W3 7SH, United Kingdom (This is not a remittance address.)
Re: [PATCH v2] MINOR: config: rhttp: Don't require SSL when attach-srv name parsing
On Tue, May 14, 2024, at 3:48 PM, Amaury Denoyelle wrote: > On Wed, May 08, 2024 at 11:43:11AM +0100, William Manley wrote: > > An attach-srv config line usually looks like this: > > tcp-request session attach-srv be/srv name ssl_c_s_dn(CN) > > while a rhttp server line usually looks like this: > > server srv rhttp@ sni req.hdr(host) > > The server sni argument is used as a key for looking up connection in the > > connection pool. The attach-srv name argument is used as a key for > > inserting connections into the pool. For it to work correctly they must > > match. There was a check that either both the attach-srv and server > > provide that key or neither does. > > It also checked that SSL and SNI was activated on the server. This is too > > strict. This patch removes that requirement. Now you can pass arbitrary > > expressions as the name expression. > > With this patch we also produce a more helpful and specific error message. > > I'm doing this as I want to use `fc_pp_unique_id` as the name. > > Arguably it would be easier to understand if instead of using `name` and > > `sni` for `attach-srv` and `server` rules it used the same term in both > > places - like "conn-pool-key" or something. That would make it clear that > > the two must match. But it's too late to change that now. > > [...] > > Many thanks for your new patch. I have just merged it with only small > adaptation in the commit message. Note that however I tried to use PROXY > protocol with reverse HTTP to test it, but it appears that for now it is > impossible due to several haproxy internal limitations. Thanks for merging it. I've been using a somewhat complicated setup, with rhttp connecting (via a few other hops) to a `mode tcp` proxy which does SSL termination and adds the proxy header before connecting over localhost to the rhttp pool on the server. I'll describe our setup in a bit more detail in a future email. You'll be horrified ;) > The main issue is that send-proxy and other related options on a server > line used for reverse HTTP are simply ignored, thus you cannot connect > to a bind with accept-proxy. I managed to fix this, but this is not > enough as PROXY protocol in LOCAL mode seems to not work completely as > expected either... What is LOCAL mode? I couldn't see a mention of it in the manual. I found in my testing that: 1. The server (the one with `server srv rhttp@ sni req.hdr(host)`) requires `nbthread 1` to work 2. Conversely: the client (the one with `bind rhttp@`) wouldn't work with `nbthread 1` - the connections from the client to the server simply wouldn't be made in that configuration Thanks Will --- William Manley Stb-tester.com Stb-tester.com Ltd is a company registered in England and Wales. Registered number: 08800454. Registered office: 13B The Vale, London, W3 7SH, United Kingdom (This is not a remittance address.)
[PATCH v2] MINOR: config: rhttp: Don't require SSL when attach-srv name parsing
An attach-srv config line usually looks like this:
tcp-request session attach-srv be/srv name ssl_c_s_dn(CN)
while a rhttp server line usually looks like this:
server srv rhttp@ sni req.hdr(host)
The server sni argument is used as a key for looking up connection in the
connection pool. The attach-srv name argument is used as a key for
inserting connections into the pool. For it to work correctly they must
match. There was a check that either both the attach-srv and server
provide that key or neither does.
It also checked that SSL and SNI was activated on the server. This is too
strict. This patch removes that requirement. Now you can pass arbitrary
expressions as the name expression.
With this patch we also produce a more helpful and specific error message.
I'm doing this as I want to use `fc_pp_unique_id` as the name.
Arguably it would be easier to understand if instead of using `name` and
`sni` for `attach-srv` and `server` rules it used the same term in both
places - like "conn-pool-key" or something. That would make it clear that
the two must match. But it's too late to change that now.
---
src/tcp_act.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/tcp_act.c b/src/tcp_act.c
index a88fab4af..cef2e50d3 100644
--- a/src/tcp_act.c
+++ b/src/tcp_act.c
@@ -520,10 +520,16 @@ static int tcp_check_attach_srv(struct act_rule *rule,
struct proxy *px, char **
return 0;
}
- if ((rule->arg.attach_srv.name && (!srv->use_ssl || !srv->sni_expr)) ||
- (!rule->arg.attach_srv.name && srv->use_ssl && srv->sni_expr)) {
- memprintf(err, "attach-srv rule: connection will never be used;
either specify name argument in conjunction with defined SSL SNI on targeted
server or none of these");
- return 0;
+ if (rule->arg.attach_srv.name) {
+ if (!srv->sni_expr) {
+ memprintf(err, "attach-srv rule has a name argument
while server '%s/%s' does not have an sni argument; either add a sni argument
to the server or remove the name argument from this attach-srv rule",
ist0(be_name), ist0(sv_name));
+ return 0;
+ }
+ } else {
+ if (srv->sni_expr) {
+ memprintf(err, "attach-srv rule has no name argument
while server '%s/%s' has an sni argument; either add a name argument to the
attach-srv rule or remove the sni argument from the server", ist0(be_name),
ist0(sv_name));
+ return 0;
+ }
}
rule->arg.attach_srv.srv = srv;
--
2.34.1
Re: [PATCH] MINOR: config: rhttp: Downgrade error on attach-srv name parsing
On Thu, Apr 25, 2024, at 2:07 PM, Amaury Denoyelle wrote:
> Sorry for the delay. We have rediscussed this issue this morning and
> here is my answer on your patch.
Sorry for the even larger delay in responding :). Thanks for looking at this.
> It is definitely legitimate to want to be able to use reverse HTTP
> without SSL on the server line. However, the way that haproxy currently
> uses idle connection is that at least the SNI parameter alone must be
> set to match the name parameter of the corresponding attach-srv rule. If
> this is not the case, the connection will never be reused.
>
> But in fact, when rereading haproxy code this morning, we found that it
> is valid to have SNI parameter set even if SSL is not active, and this
> will produce the desired effect. We are definitely okay with merging an
> adjusted version of your patch for the moment, see my remarks below.
> However, using SNI on a server line without SSL is something tricky.
> Thus we plan to add a new keyword to replace it when SSL is not used to
> have the same effect. When this will be done, you should update your
> configuration to use it.
>
> On Fri, Apr 12, 2024 at 02:29:30PM +0100, William Manley wrote:
> > An attach-srv config line usually looks like this:
> > tcp-request session attach-srv be/srv name ssl_c_s_dn(CN)
> > The name is a key that is used when looking up connections in the
> > connection pool. Without this patch you'd get an error if you passed
> > anything other than "ssl_c_s_dn(CN)" as the name expression. Now you can
> > pass arbitrary expressions and it will just warn you if you aren't
> > producing a configuration that is RFC compliant.
> > I'm doing this as I want to use `fc_pp_unique_id` as the name.
>
> I'm not 100% okay with your description here. The current code condition
> does not check that "ssl_c_s_dn(CN)" is used as expression, but rather
> that if name is defined for an attach-srv rule, the targetted server
> must have both SSL and SNI activated. I think this paragraph should be
> reworded.
I'll fix this.
> > ---
> > src/tcp_act.c | 3 +--
> > 1 file changed, 1 insertion(+), 2 deletions(-)
> > diff --git a/src/tcp_act.c b/src/tcp_act.c
> > index a88fab4af..4d2a56c67 100644
> > --- a/src/tcp_act.c
> > +++ b/src/tcp_act.c
> > @@ -522,8 +522,7 @@ static int tcp_check_attach_srv(struct act_rule *rule,
> > struct proxy *px, char **
> >
> > if ((rule->arg.attach_srv.name && (!srv->use_ssl || !srv->sni_expr)) ||
> > (!rule->arg.attach_srv.name && srv->use_ssl && srv->sni_expr)) {
> > - memprintf(err, "attach-srv rule: connection will never be used; either
> > specify name argument in conjunction with defined SSL SNI on targeted
> > server or none of these");
> > - return 0;
> > + ha_warning("attach-srv rule: connection may never be used; usually name
> > argument is defined SSL SNI on targeted server or none of these");
> > }
> >
> > rule->arg.attach_srv.srv = srv;
> > --
> > 2.34.1
> >
>
> I think an alternative patch may be more desirable here. We could update
> the condition with the following statement without removing the fatal
> error :
>
> > if ((rule->arg.attach_srv.name && !srv->sni_expr) ||
> > (!rule->arg.attach_srv.name && srv->sni_expr)) {
>
> This reflects the current mandatory usage of reverse-http : if name is
> used on attach-srv, sni keyword must be specified on the server line.
I agree. Did you see my replacement patch "MINOR: config: rhttp: Don't require
SSL when attach-srv name parsing"
https://www.mail-archive.com/haproxy@formilux.org/msg44826.html . It
implements this general idea, but with a nice specific error message depending
on whether sni or name is missing.
That replacement patch still needs the commit message corrected as you remarked
above, so I'll send a v2 of that patch.
Thanks
Will
---
William Manley
Stb-tester.com
Stb-tester.com Ltd is a company registered in England and Wales.
Registered number: 08800454. Registered office: 13B The Vale,
London, W3 7SH, United Kingdom (This is not a remittance address.)
Re: [PATCH] MINOR: config: rhttp: Downgrade error on attach-srv name parsing
On Fri, Apr 12, 2024, at 4:01 PM, Amaury Denoyelle wrote: > I have a doubt though, will this kind of configuration really works ? I > though that for the moment if name parameter is specified, it is > mandatory to use a server with SSL+SNI. It may be mandatory according to the RFC, but I'm not using it that way. Usually it's RHTTP over SSL, and the incoming connection identifies itself securely using the SSL DN. The way I'm using it is RHTTP over HTTP CONNECT - and I'm validating the connection using the headers that came with the HTTP CONNECT. I have tcp server block that strips the HTTP CONNECT header and adds PROXY header instead with the connection pool name sent through using unique-id: listen connect_terminate mode tcp bind ... tcp-request inspect-delay 5s tcp-request content lua.terminate_http_connect # This allows us to send the hostname over the PROXY protocol: unique-id-format "%[var(txn.req_header.x_hostname)]" server srv 127.0.0.1:8001 send-proxy-v2 proxy-v2-options unique-id Then I use that unique id when adding the connection to the connection pool: frontend add_to_http_pool mode http bind 127.0.0.1:8001 proto h2 accept-proxy tcp-request session attach-srv rhttp_frontend/srv name fc_pp_unique_id It's a little roundabout (and this is the simplified version) but quite capable. I plan to use a similar technique to route multiple requests to different hostnames down the same RHTTP connection too. In that case I'll not be using sni req.hdr(host) either - but I haven't got that far yet. Thanks Will --- William Manley Stb-tester.com Stb-tester.com Ltd is a company registered in England and Wales. Registered number: 08800454. Registered office: 13B The Vale, London, W3 7SH, United Kingdom (This is not a remittance address.)
Re: [PATCH] MINOR: config: rhttp: Downgrade error on attach-srv name parsing
On Fri, Apr 12, 2024, at 2:37 PM, Willy Tarreau wrote: > Well, I consider that any valid (and useful) configuration must be > writable without a warning. So if you have a valid use case with a > different expression, here you still have no way to express it without > the warning. In this case I'd suggest to use ha_diag_warning() instead, > it will only report this when starting with -dD (config diagnostic mode). Thanks for taking a look. I've taken a slightly different approach now that I understand the warning better. Instead I've removed the requirement for SSL, but I keep the requirement that either name and sni are specified or neither are. This way it won't warn or error with my configuration, nor with the usual one, but it will still error if there is a mismatch between name and sni. Thanks Will --- William Manley Stb-tester.com Stb-tester.com Ltd is a company registered in England and Wales. Registered number: 08800454. Registered office: 13B The Vale, London, W3 7SH, United Kingdom (This is not a remittance address.)
[PATCH] MINOR: config: rhttp: Don't require SSL when attach-srv name parsing
An attach-srv config line usually looks like this:
tcp-request session attach-srv be/srv name ssl_c_s_dn(CN)
while a rhttp server line usually looks like this:
server srv rhttp@ sni req.hdr(host)
The server sni argument is used as a key for looking up connection in the
connection pool. The attach-srv name argument is used as a key for
inserting connections into the pool. For it to work correctly they must
match. There was a check that either both the attach-srv and server
provide that key or neither does.
It also checked that SSL was in use on the server. This is too strict.
This patch removes that requirement. Without this patch you'd get an error
if you passed anything other than "ssl_c_s_dn(CN)" as the name expression.
Now you can pass arbitrary expressions.
With this patch we also produce a more helpful and specific error message.
I'm doing this as I want to use `fc_pp_unique_id` as the name.
Arguably it would be easier to understand if instead of using `name` and
`sni` for `attach-srv` and `server` rules it used the same term in both
places - like "conn-pool-key" or something. That would make it clear that
the two must match. But it's too late to change that now.
---
src/tcp_act.c | 14 ++
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/src/tcp_act.c b/src/tcp_act.c
index a88fab4af..cef2e50d3 100644
--- a/src/tcp_act.c
+++ b/src/tcp_act.c
@@ -520,10 +520,16 @@ static int tcp_check_attach_srv(struct act_rule *rule,
struct proxy *px, char **
return 0;
}
- if ((rule->arg.attach_srv.name && (!srv->use_ssl || !srv->sni_expr)) ||
- (!rule->arg.attach_srv.name && srv->use_ssl && srv->sni_expr)) {
- memprintf(err, "attach-srv rule: connection will never be used;
either specify name argument in conjunction with defined SSL SNI on targeted
server or none of these");
- return 0;
+ if (rule->arg.attach_srv.name) {
+ if (!srv->sni_expr) {
+ memprintf(err, "attach-srv rule has a name argument
while server '%s/%s' does not have an sni argument; either add a sni argument
to the server or remove the name argument from this attach-srv rule",
ist0(be_name), ist0(sv_name));
+ return 0;
+ }
+ } else {
+ if (srv->sni_expr) {
+ memprintf(err, "attach-srv rule has no name argument
while server '%s/%s' has an sni argument; either add a name argument to the
attach-srv rule or remove the sni argument from the server", ist0(be_name),
ist0(sv_name));
+ return 0;
+ }
}
rule->arg.attach_srv.srv = srv;
--
2.34.1
[PATCH] MINOR: config: rhttp: Downgrade error on attach-srv name parsing
An attach-srv config line usually looks like this:
tcp-request session attach-srv be/srv name ssl_c_s_dn(CN)
The name is a key that is used when looking up connections in the
connection pool. Without this patch you'd get an error if you passed
anything other than "ssl_c_s_dn(CN)" as the name expression. Now you can
pass arbitrary expressions and it will just warn you if you aren't
producing a configuration that is RFC compliant.
I'm doing this as I want to use `fc_pp_unique_id` as the name.
---
src/tcp_act.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/src/tcp_act.c b/src/tcp_act.c
index a88fab4af..4d2a56c67 100644
--- a/src/tcp_act.c
+++ b/src/tcp_act.c
@@ -522,8 +522,7 @@ static int tcp_check_attach_srv(struct act_rule *rule,
struct proxy *px, char **
if ((rule->arg.attach_srv.name && (!srv->use_ssl || !srv->sni_expr)) ||
(!rule->arg.attach_srv.name && srv->use_ssl && srv->sni_expr)) {
- memprintf(err, "attach-srv rule: connection will never be used;
either specify name argument in conjunction with defined SSL SNI on targeted
server or none of these");
- return 0;
+ ha_warning("attach-srv rule: connection may never be used;
usually name argument is defined SSL SNI on targeted server or none of these");
}
rule->arg.attach_srv.srv = srv;
--
2.34.1
