Hi Dorian, I'm now done with the release and having more time to read your work. First, thanks for this update. I understand that you're almost running out of time on this topic which must be completed before June so I'm not going to make you waste your time. Some comments below.
On Thu, May 16, 2024 at 03:53:40PM +0200, Dorian Craps wrote: > From: Dorian Craps <[email protected]> > > Multipath TCP (MPTCP), standardized in RFC8684 [1], is a TCP extension > that enables a TCP connection to use different paths. > > Multipath TCP has been used for several use cases. On smartphones, MPTCP > enables seamless handovers between cellular and Wi-Fi networks while > preserving established connections. This use-case is what pushed Apple > to use MPTCP since 2013 in multiple applications [2]. On dual-stack > hosts, Multipath TCP enables the TCP connection to automatically use the > best performing path, either IPv4 or IPv6. If one path fails, MPTCP > automatically uses the other path. > > To benefit from MPTCP, both the client and the server have to support > it. Multipath TCP is a backward-compatible TCP extension that is enabled > by default on recent Linux distributions (Debian, Ubuntu, Redhat, ...). > Multipath TCP is included in the Linux kernel since version 5.6 [3]. To > use it on Linux, an application must explicitly enable it when creating > the socket. No need to change anything else in the application. > > This attached patch adds the "mptcp" global option in the config, which > allows the creation of an MPTCP socket instead of TCP on Linux. If > Multipath TCP is not supported on the system, an error will be reported, > and the application will stop. > > A test has been added, it is a copy of "default_rules.vtc" in tcp-rules > with the addition of "mptcp" in the config. I'm not sure what else needs > to be tested for the moment, with this global MPTCP option. > > Note: another patch is coming to support enabling MPTCP per address, but > I prefer to already send this patch, just in case, as I will soon have > less time to dedicate to this. Thanks for this. As I previously mentioned, I'm not going to merge a patch with a global option that does all or nothing, however having a working patch like this can definitely serve as a proof-of-concept and a reference when testing a more granular approach, and in this regard your patch is much welcome! > Due to the limited impact within a data center environment, > we have decided not to implement MPTCP between the proxy and the servers. > The high-speed, low-latency nature of data center networks reduces > the benefits of MPTCP, making the complexity of its implementation > unnecessary in this context. I definitely understand. However let's keep in mind that whenever we envision a use case, someone will come with a different one and suggest that we should support it. Here I can imagine that some CDN operators might be interested in using MPTCP to the origin server as well by connecting via multiple paths. This opens a big can of worms with questions about source address binding, interface binding etc, which cannot be addressed easily as they'll all have impacts on the way servers are configured. So I'm totally fine with having only a frontend support in a first time. I just mentioned this to give you more context and so that you know it's not only the DC that's on the backend, even if it's by far the most common, and what some of the complex aspects can be. I'll be in vacation for two weeks at the end of this week, will you need some review of a possible next patch, or did you give up due to some difficulties you faced while exploring the per-listener configuration ? I'll give you a few comments on the patch below: > diff --git a/src/cfgparse-global.c b/src/cfgparse-global.c > index b173511c9..0feccd4b2 100644 > --- a/src/cfgparse-global.c > +++ b/src/cfgparse-global.c > @@ -23,6 +23,8 @@ > #include <haproxy/log.h> > #include <haproxy/peers.h> > #include <haproxy/protocol.h> > +#include <haproxy/proto_rhttp.h> > +#include <haproxy/proto_tcp.h> > #include <haproxy/tools.h> > > int cluster_secret_isset; > @@ -52,7 +54,7 @@ static const char *common_kw_list[] = { > "presetenv", "unsetenv", "resetenv", "strict-limits", "localpeer", > "numa-cpu-mapping", "defaults", "listen", "frontend", "backend", > "peers", "resolvers", "cluster-secret", "no-quic", "limited-quic", > - NULL /* must be last */ > + "mptcp", NULL /* must be last */ > }; Style only: better leave the NULL alone on its line so that it remains distinct from the rest. > /* > @@ -1334,6 +1336,23 @@ int cfg_parse_global(const char *file, int linenum, > char **args, int kwm) > HA_ATOMIC_STORE(&global.anon_key, tmp); > } > } > + else if (strcmp(args[0], "mptcp") == 0) { > + if (alertif_too_many_args(0, file, linenum, args, &err_code)) > + goto out; > +#ifdef __linux__ > +#ifndef IPPROTO_MPTCP > +#define IPPROTO_MPTCP 262 > +#endif This would be better placed in include/haproxy/compat.h (there are already other ones there). > + proto_tcpv4.sock_prot = IPPROTO_MPTCP; > + proto_tcpv6.sock_prot = IPPROTO_MPTCP; > + proto_rhttp.sock_prot = IPPROTO_MPTCP; In my opinion it should not be needed to assign anything to the rhttp stuff. I know it's complicated (and I get my mind mixed about it very often due to the reversed nature), but the protocol here is in fact used for outgoing connections that later get reversed and assigned to the listener in an established state. > +#else > + ha_alert("parsing [%s:%d]: '%s' is only supported on Linux.\n", > + file, linenum, args[0]); > + err_code |= ERR_ALERT | ERR_FATAL; > + goto out; > +#endif > + } > else { > struct cfg_kw_list *kwl; > const char *best; Thanks! Willy
