Fix bug that caused agents to bypass local loadbalancer #10280

brandond · 2024-06-03T22:17:04Z

Proposed Changes

Fix bug that caused agents to bypass local loadbalancer

If proxy.SetAPIServerPort was called multiple times, all calls after the first one would cause the apiserver address to be set to the default server address, bypassing the local load-balancer. This was most likely to occur on RKE2, where the supervisor may be up for a longer period of time before it is ready to manage node password secrets, causing the agent to retry. Also, K3s does not ever take the affected code path due to the apiserver and supervisor always using the same port.

I also added some comments to the code as I was stepping through it trying to figure out what's going on.

Types of Changes

bugfix

Verification

Testing

Linked Issues

User-Facing Change

Further Comments

VestigeJ

LGTM

codecov · 2024-06-03T22:53:27Z

Codecov Report

Attention: Patch coverage is 26.08696% with 17 lines in your changes missing coverage. Please review.

Project coverage is 41.69%. Comparing base (79ba10f) to head (967cc14).

Files	Patch %	Lines
pkg/agent/proxy/apiproxy.go	10.00%	9 Missing ⚠️
pkg/agent/config/config.go	11.11%	5 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #10280      +/-   ##
==========================================
- Coverage   47.89%   41.69%   -6.20%     
==========================================
  Files         177      177              
  Lines       14792    14801       +9     
==========================================
- Hits         7085     6172     -913     
- Misses       6362     7450    +1088     
+ Partials     1345     1179     -166

Flag	Coverage Δ
e2etests	`36.43% <13.04%> (-10.03%)`	⬇️
inttests	`37.00% <8.69%> (+0.04%)`	⬆️
unittests	`11.33% <13.04%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

If proxy.SetAPIServerPort was called multiple times, all calls after the first one would cause the apiserver address to be set to the default server address, bypassing the local load-balancer. This was most likely to occur on RKE2, where the supervisor may be up for a period of time before it is ready to manage node password secrets, causing the agent to retry. Signed-off-by: Brad Davidson <brad.davidson@rancher.com>

manuelbuil

LGTM and that's a great catch! Just a few comments. BTW, is it possible to "decollocate" kube-api and supervisor in K3s via config?

manuelbuil · 2024-06-04T13:55:39Z

pkg/agent/proxy/apiproxy.go

@@ -132,29 +133,35 @@ func (p *proxy) setSupervisorPort(addresses []string) []string {
 // load-balancer, and the address of this load-balancer is returned instead of the actual apiserver
 // addresses.
 func (p *proxy) SetAPIServerPort(port int, isIPv6 bool) error {
+	if p.apiServerEnabled {
+		logrus.Debugf("Supervisor proxy apiserver port already set")


IIUC, this is the agent proxy, not the supervisor proxy. By supervisor proxy, I understand the one we use for egressSelector in the supervisor

We call this the "supervisor proxy" because it is a proxy for retrieving supervisor (and now apiserver as well) addresses. This is covered in the doc comment:

k3s/pkg/agent/proxy/apiproxy.go

Lines 28 to 35 in 79ba10f

// NewSupervisorProxy sets up a new proxy for retrieving supervisor and apiserver addresses. If

// lbEnabled is true, a load-balancer is started on the requested port to connect to the supervisor

// address, and the address of this local load-balancer is returned instead of the actual supervisor

// and apiserver addresses.

// NOTE: This is a proxy in the API sense - it returns either actual server URLs, or the URL of the

// local load-balancer. It is not actually responsible for proxying requests at the network level;

// this is handled by the load-balancers that the proxy optionally steers connections towards.

func NewSupervisorProxy(ctx context.Context, lbEnabled bool, dataDir, supervisorURL string, lbServerPort int, isIPv6 bool) (Proxy, error) {

aaaah ok, ufff it can easily be confusing. The supervisorproxy carries the supervisor and the kube-api addresses and load-balancers

manuelbuil · 2024-06-04T13:56:16Z

pkg/agent/proxy/apiproxy.go

 	}

+	logrus.Debugf("Supervisor proxy apiserver port changed; apiserver=%s lb=%v", p.apiServerURL, p.lbEnabled)


Same comment. I think supervisor proxy is confusing here

Yeah it is a little confusing that we have so many proxies. This one is a proxy for retrieving supervisor and apiserver addresses without having to know if the loadbalancer is enabled, in that it provides a layer of abstraction that allows us to avoid hardcoding addresses.

We have a similar "proxy" for etcd here as well:

k3s/pkg/etcd/etcdproxy.go

Lines 34 to 36 in 79ba10f

// NewETCDProxy initializes a new proxy structure that contain a load balancer

// which listens on port 2379 and proxy between etcd cluster members

func NewETCDProxy(ctx context.Context, supervisorPort int, dataDir, etcdURL string, isIPv6 bool) (Proxy, error) {

ok, thanks for the clarification

manuelbuil · 2024-06-04T14:04:39Z

pkg/agent/config/config.go

 	kubeconfigKubelet := filepath.Join(envInfo.DataDir, "agent", "kubelet.kubeconfig")
-	if err := deps.KubeConfig(kubeconfigKubelet, proxy.APIServerURL(), serverCAFile, clientKubeletCert, clientKubeletKey); err != nil {
+	if err := deps.KubeConfig(kubeconfigKubelet, apiServerURL, serverCAFile, clientKubeletCert, clientKubeletKey); err != nil {


Out of curiosity: given that the bug in how we set proxy.APIServerURL() is fixed, both proxy.APIServerURL() or apiServerURL will point to the same server, right? The former via the loadBalancer but at this "booting" step I expect it to only have one server backend

Yeah, but I figured it made a little more sense to just retrieve it once and then use a local variable? It shouldn't really make a difference, no since the APIServerURL function itself just exposes a field on the proxy struct.

Agree. I just wanted to doublecheck if my understanding was correct :)

brandond requested a review from a team as a code owner June 3, 2024 22:17

VestigeJ previously approved these changes Jun 3, 2024

View reviewed changes

brandond dismissed VestigeJ’s stale review via 967cc14 June 4, 2024 00:26

brandond force-pushed the fix-agent-server-address branch from 4496622 to 967cc14 Compare June 4, 2024 00:26

brandond requested review from VestigeJ and a team June 4, 2024 00:52

manuelbuil reviewed Jun 4, 2024

View reviewed changes

brandond requested review from manuelbuil and a team June 4, 2024 16:52

manuelbuil approved these changes Jun 4, 2024

View reviewed changes

dereknola approved these changes Jun 4, 2024

View reviewed changes

brandond merged commit 1661f10 into k3s-io:master Jun 4, 2024
27 checks passed

This was referenced Jun 4, 2024

[release-1.29] More backports for 2024-06 release cycle #10288

Merged

[release-1.28] More backports for 2024-06 release cycle #10289

Merged

[release-1.27] More backports for 2024-06 release cycle #10290

Merged

brandond deleted the fix-agent-server-address branch June 6, 2024 21:15

cguertin14 mentioned this pull request Jun 28, 2024

new release: k3s update from v1.30.1+k3s1 to v1.30.2+k3s1 cguertin14/k3s-ansible-ha#39

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix bug that caused agents to bypass local loadbalancer #10280

Fix bug that caused agents to bypass local loadbalancer #10280

brandond commented Jun 3, 2024 •

edited

Loading

VestigeJ left a comment

codecov bot commented Jun 3, 2024 •

edited

Loading

manuelbuil left a comment

manuelbuil Jun 4, 2024

brandond Jun 4, 2024

manuelbuil Jun 4, 2024

manuelbuil Jun 4, 2024

brandond Jun 4, 2024 •

edited

Loading

manuelbuil Jun 4, 2024

manuelbuil Jun 4, 2024

brandond Jun 4, 2024 •

edited

Loading

manuelbuil Jun 4, 2024

	// NewSupervisorProxy sets up a new proxy for retrieving supervisor and apiserver addresses. If
	// lbEnabled is true, a load-balancer is started on the requested port to connect to the supervisor
	// address, and the address of this local load-balancer is returned instead of the actual supervisor
	// and apiserver addresses.
	// NOTE: This is a proxy in the API sense - it returns either actual server URLs, or the URL of the
	// local load-balancer. It is not actually responsible for proxying requests at the network level;
	// this is handled by the load-balancers that the proxy optionally steers connections towards.
	func NewSupervisorProxy(ctx context.Context, lbEnabled bool, dataDir, supervisorURL string, lbServerPort int, isIPv6 bool) (Proxy, error) {

		}

		logrus.Debugf("Supervisor proxy apiserver port changed; apiserver=%s lb=%v", p.apiServerURL, p.lbEnabled)

	// NewETCDProxy initializes a new proxy structure that contain a load balancer
	// which listens on port 2379 and proxy between etcd cluster members
	func NewETCDProxy(ctx context.Context, supervisorPort int, dataDir, etcdURL string, isIPv6 bool) (Proxy, error) {

Fix bug that caused agents to bypass local loadbalancer #10280

Fix bug that caused agents to bypass local loadbalancer #10280

Conversation

brandond commented Jun 3, 2024 • edited Loading

Proposed Changes

Types of Changes

Verification

Testing

Linked Issues

User-Facing Change

Further Comments

VestigeJ left a comment

Choose a reason for hiding this comment

codecov bot commented Jun 3, 2024 • edited Loading

Codecov Report

manuelbuil left a comment

Choose a reason for hiding this comment

manuelbuil Jun 4, 2024

Choose a reason for hiding this comment

brandond Jun 4, 2024

Choose a reason for hiding this comment

manuelbuil Jun 4, 2024

Choose a reason for hiding this comment

manuelbuil Jun 4, 2024

Choose a reason for hiding this comment

brandond Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

manuelbuil Jun 4, 2024

Choose a reason for hiding this comment

manuelbuil Jun 4, 2024

Choose a reason for hiding this comment

brandond Jun 4, 2024 • edited Loading

Choose a reason for hiding this comment

manuelbuil Jun 4, 2024

Choose a reason for hiding this comment

brandond commented Jun 3, 2024 •

edited

Loading

codecov bot commented Jun 3, 2024 •

edited

Loading

brandond Jun 4, 2024 •

edited

Loading

brandond Jun 4, 2024 •

edited

Loading