CPLB userspace reverse proxy load balancer #5279

juanluisvaladas · 2024-11-20T19:16:02Z

Description

Implement userspace CPLB reverse proxy load balancer.

Due to the keepalived virtualservers using IPVS for the load balancing, we simply couldn't make it work on some environments and needed to implement a userspace reverse proxy.

Fixes #4700

Type of change

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to not work as expected)
Documentation update

How Has This Been Tested?

Manual test
Auto test added

Checklist:

juanluisvaladas · 2024-11-22T11:54:06Z

Got some feedback on the docs from Daniel, so I'm making it a draft temporarily

github-actions · 2024-11-26T14:06:30Z

This pull request has merge conflicts that need to be resolved.

github-actions · 2024-12-05T12:57:02Z

This pull request has merge conflicts that need to be resolved.

tcpproxy is a subset of github.com/inetaf/tcpproxy with some modifications: 1- Implement the method SetRoutes to allow to set routes in bulk. Also allows deletion of routes which otherwise would be impossible. 2- Implement round robin load balancing (there was no load balancing at all). 3- Remove unused code. 4- Append Mirantis copyright for the modifications. Additionaly this PR had to do some modifications in the copyright linter script and `.golangci.yaml` because this is a file copied and modified. This is required to Apache 2.0 retribution right. Signed-off-by: Juan-Luis de Sousa-Valadas Castaño <jvaladas@mirantis.com>

pkg/component/controller/cplb/tcpproxy/tcpproxy_test.go

pkg/component/controller/cplb/tcpproxy/tcpproxy.go

twz123 · 2024-12-17T10:52:53Z

pkg/component/controller/cplb/tcpproxy/tcpproxy.go

+// The order that routes are added in matters; each is matched in the order
+// registered.
+type Proxy struct {
+	configs map[string]*config // ip:port => config


NB: There's the special type netip.AddrPort for IP plus port number in the standard library, so this could be typed accordingly:

Suggested change

configs map[string]*config // ip:port => config

configs map[netip.AddrPort]*config

I guess this would make the public Proxy API clearer as well.

Since it's a private field, how about we leave the map as it is (a string is more convinient than a struct for the map) and we use netip.AddrPort in the public functions?

twz123 · 2024-12-17T10:57:11Z

pkg/component/controller/cplb/tcpproxy/tcpproxy.go

+
+	// ListenFunc optionally specifies an alternate listen
+	// function. If nil, net.Dial is used.
+	// The provided net is always "tcp".


If net is always "tcp", wouldn't it make sense to remove that parameter?

This is the interface for net.Listen, we could remove the field but we need for the tests. I'd rather leave this as it is.

twz123 · 2024-12-17T11:08:23Z

pkg/component/controller/cplb/tcpproxy/tcpproxy.go

+	for _, c := range p.lns {
+		c.Close()
+	}
+	return nil


The errors are completely ignored here. Let's either remove the error return completely, or collect them instead.

Suggested change

for _, c := range p.lns {

c.Close()

}

return nil

var errs []error

for _, c := range p.lns {

if err := c.Close(); err != nil {

errs = append(errs, err)

}

}

return errors.Join(errs...)

twz123 · 2024-12-17T15:48:25Z

inttest/cplb-userspace-extaddr/cplbuserspaceextaddr_test.go

Is there a way to merge this with the other inttest, and call this multiple times, as we're doing it e.g. for the dualstack tests.

twz123 · 2024-12-17T15:53:26Z

cmd/controller/controller.go

 	if cplbCfg := nodeConfig.Spec.Network.ControlPlaneLoadBalancing; cplbCfg != nil && cplbCfg.Enabled {
 		if c.SingleNode {
 			return errors.New("control plane load balancing cannot be used in a single-node cluster")
 		}

+		if enableK0sEndpointReconciler {
+			return errors.New("control plane load balancing requires the component 'endpoint-reconciler' to be disabled")


I wonder if there's some way of making this error message a bit more comprehensive 🤔

The message is technically correct, but might not be too helpful for folks to understand the connection between externalAddress and k0s's understanding of endpoint reconciliation...

inttest/cplb-userspace-extaddr/cplbuserspaceextaddr_test.go

inttest/cplb-userspace/cplbuserspace_test.go

docs/cplb.md

IPVS is problematic for many reasons, implement a userspace load balancer which should get the job done and should be far less problematic. Signed-off-by: Juan-Luis de Sousa-Valadas Castaño <jvaladas@mirantis.com>

The only reason why we block externalAddres is that we need a disabled endpoint-reconciler. It doesn't make sense to move all this logic and these parameters to the config validation, instead do it all in cmd. Signed-off-by: Juan-Luis de Sousa-Valadas Castaño <jvaladas@mirantis.com>

1- Included userspace reverse proxy and made it the default option. 2- Explained better the difference between virtual IP and a load balancer, it was confusing for many users. 3- Added a whole troubleshooting section. Signed-off-by: Juan-Luis de Sousa-Valadas Castaño <jvaladas@mirantis.com>

1- Simplify tcpproxy by reomivng useless interfaces: The original tcpproxy allowed different types of routes and needed to do a bunch of interfacing for it to work. Since we only implement one kind of route and one kind of target, we remove all the interfaces and merge both structs into a unique struct. 2- Remove proxy.AddRoute: We only used it once and setRoutes can cover that use case 3- Lock tcpproxy.Proxy when modifying routes to make it thread safe. Prior to this we relied on the proxy being called only from one goroutine. Now it can be called concurrently, not that we expect to do that but a lock gives us extra safety. 4- Panic if tcpproxy.SetRoutes gets and empty route list. We now check this in cplb_unix.go. 5- Remove the route feeding goroutine for round robin, since we added a lock to make proxy.SetRoutes threadsafe we don't need that anymore and it can be made much simpler by adding a lock. Signed-off-by: Juan-Luis de Sousa-Valadas Castaño <jvaladas@mirantis.com>

juanluisvaladas · 2024-12-19T16:36:13Z

I addressed most of the comments. As for the comments:

1-Add tcpproxy remains unchanged for easier reviews
2- Implement CPLB userspace reverse proxy LB addresses all its comments. That includes adding proxy.Stop() to keepalived.Stop() reverting the changes of errors.As to errors.Is, clearer error messages, fixing the iptables path to use k0sVars.BinDir, and all the comments regarding the test
3- Move the check for CPLB and externalAddress to cmd unifies the tests in one and only changes one error message to make it clearer, everything else remains unchanged (there were no more concerns.
4- Overhaul CPLB docs only changes the code blocks from shell to console
5- Multiple refactors on cplb.tcpproxy addresses most of the concerns regarding the tcpproxy

There are a few things unchanged:
1- Copyright: I know it looks weird but it's literally copied and pasted from an email thread from the legal team. They said this is the right way of doing it 🤷
2- Allowing to disable the userspace proxy: Because this is already far too large we need at least two subsequent PRs, one for more feature (unicast VRRP) and another one to fix a bug (etcd getting the VIP which has a conflict with the current code) how about we get that in one of the next PRs?
3- Having the tcpproxy binary as its own process: I'm not opposed, but I don't think it's a priority and doing it this way is simpler in terms of permissions (we need to query the API). If we think it's worth blocking then I'll take it out.
4-I didn't address this because I don't think it's required, but I'm not too sure: https://github.com/k0sproject/k0s/pull/5279/files#r1888710381

juanluisvaladas force-pushed the cplb-usrp branch from c7886ee to 3682cf7 Compare November 20, 2024 19:37

juanluisvaladas changed the title ~~Cplb usrp~~ Cplb userspace reverse proxy load balancer Nov 20, 2024

juanluisvaladas changed the title ~~Cplb userspace reverse proxy load balancer~~ CPLB userspace reverse proxy load balancer Nov 20, 2024

juanluisvaladas force-pushed the cplb-usrp branch 3 times, most recently from ac4e148 to 0910d28 Compare November 20, 2024 21:03

juanluisvaladas marked this pull request as ready for review November 20, 2024 21:05

juanluisvaladas requested review from a team as code owners November 20, 2024 21:05

juanluisvaladas requested review from ncopa and makhov November 20, 2024 21:05

juanluisvaladas marked this pull request as draft November 21, 2024 11:01

juanluisvaladas force-pushed the cplb-usrp branch from 0910d28 to a651dbd Compare November 21, 2024 11:07

juanluisvaladas marked this pull request as ready for review November 21, 2024 11:07

juanluisvaladas marked this pull request as draft November 22, 2024 11:53

juanluisvaladas mentioned this pull request Nov 26, 2024

Move CPLB into its own package #5295

Merged

16 tasks

github-actions bot added the merge-conflict label Nov 26, 2024

juanluisvaladas force-pushed the cplb-usrp branch from a651dbd to dfaf7d8 Compare December 2, 2024 12:57

github-actions bot removed the merge-conflict label Dec 2, 2024

github-actions bot added the merge-conflict label Dec 5, 2024

juanluisvaladas force-pushed the cplb-usrp branch from dfaf7d8 to cf158fb Compare December 11, 2024 14:55

juanluisvaladas force-pushed the cplb-usrp branch from cf158fb to 8220614 Compare December 11, 2024 15:36

github-actions bot removed the merge-conflict label Dec 11, 2024

juanluisvaladas force-pushed the cplb-usrp branch 3 times, most recently from b0a4984 to 5c2c026 Compare December 11, 2024 17:19

juanluisvaladas marked this pull request as ready for review December 11, 2024 17:21

twz123 reviewed Dec 17, 2024

View reviewed changes

juanluisvaladas force-pushed the cplb-usrp branch 2 times, most recently from aed0b18 to 5155c18 Compare December 19, 2024 16:09

juanluisvaladas added 4 commits December 19, 2024 17:18

Implement CPLB userspace reverse proxy LB

3b3a175

IPVS is problematic for many reasons, implement a userspace load balancer which should get the job done and should be far less problematic. Signed-off-by: Juan-Luis de Sousa-Valadas Castaño <jvaladas@mirantis.com>

juanluisvaladas force-pushed the cplb-usrp branch from 5155c18 to ba2de2c Compare December 19, 2024 16:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CPLB userspace reverse proxy load balancer #5279

CPLB userspace reverse proxy load balancer #5279

juanluisvaladas commented Nov 20, 2024 •

edited

Loading

juanluisvaladas commented Nov 22, 2024

github-actions bot commented Nov 26, 2024

github-actions bot commented Dec 5, 2024

twz123 Dec 17, 2024

juanluisvaladas Dec 19, 2024

twz123 Dec 17, 2024

juanluisvaladas Dec 19, 2024

twz123 Dec 17, 2024

twz123 Dec 17, 2024

twz123 Dec 17, 2024

juanluisvaladas commented Dec 19, 2024

	configs map[string]*config // ip:port => config
	configs map[netip.AddrPort]*config

CPLB userspace reverse proxy load balancer #5279

Are you sure you want to change the base?

CPLB userspace reverse proxy load balancer #5279

Conversation

juanluisvaladas commented Nov 20, 2024 • edited Loading

Description

Type of change

How Has This Been Tested?

Checklist:

juanluisvaladas commented Nov 22, 2024

github-actions bot commented Nov 26, 2024

github-actions bot commented Dec 5, 2024

twz123 Dec 17, 2024

Choose a reason for hiding this comment

juanluisvaladas Dec 19, 2024

Choose a reason for hiding this comment

twz123 Dec 17, 2024

Choose a reason for hiding this comment

juanluisvaladas Dec 19, 2024

Choose a reason for hiding this comment

twz123 Dec 17, 2024

Choose a reason for hiding this comment

twz123 Dec 17, 2024

Choose a reason for hiding this comment

twz123 Dec 17, 2024

Choose a reason for hiding this comment

juanluisvaladas commented Dec 19, 2024

juanluisvaladas commented Nov 20, 2024 •

edited

Loading