-
Notifications
You must be signed in to change notification settings - Fork 2.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix bug that caused agents to bypass local loadbalancer #10280
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -63,6 +63,7 @@ func NewSupervisorProxy(ctx context.Context, lbEnabled bool, dataDir, supervisor | |||||||||||||||||
p.fallbackSupervisorAddress = u.Host | ||||||||||||||||||
p.supervisorPort = u.Port() | ||||||||||||||||||
|
||||||||||||||||||
logrus.Debugf("Supervisor proxy using supervisor=%s apiserver=%s lb=%v", p.supervisorURL, p.apiServerURL, p.lbEnabled) | ||||||||||||||||||
return &p, nil | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
|
@@ -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") | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IIUC, this is the agent proxy, not the supervisor proxy. By supervisor proxy, I understand the one we use for egressSelector in the supervisor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. aaaah ok, ufff it can easily be confusing. The supervisorproxy carries the supervisor and the kube-api addresses and load-balancers |
||||||||||||||||||
return nil | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
u, err := url.Parse(p.initialSupervisorURL) | ||||||||||||||||||
if err != nil { | ||||||||||||||||||
return errors.Wrapf(err, "failed to parse server URL %s", p.initialSupervisorURL) | ||||||||||||||||||
} | ||||||||||||||||||
p.apiServerPort = strconv.Itoa(port) | ||||||||||||||||||
u.Host = sysnet.JoinHostPort(u.Hostname(), p.apiServerPort) | ||||||||||||||||||
|
||||||||||||||||||
p.apiServerURL = u.String() | ||||||||||||||||||
p.apiServerEnabled = true | ||||||||||||||||||
|
||||||||||||||||||
if p.lbEnabled && p.apiServerLB == nil { | ||||||||||||||||||
lbServerPort := p.lbServerPort | ||||||||||||||||||
if lbServerPort != 0 { | ||||||||||||||||||
lbServerPort = lbServerPort - 1 | ||||||||||||||||||
} | ||||||||||||||||||
lb, err := loadbalancer.New(p.context, p.dataDir, loadbalancer.APIServerServiceName, p.apiServerURL, lbServerPort, isIPv6) | ||||||||||||||||||
lb, err := loadbalancer.New(p.context, p.dataDir, loadbalancer.APIServerServiceName, u.String(), lbServerPort, isIPv6) | ||||||||||||||||||
if err != nil { | ||||||||||||||||||
return err | ||||||||||||||||||
} | ||||||||||||||||||
p.apiServerURL = lb.LoadBalancerServerURL() | ||||||||||||||||||
p.apiServerLB = lb | ||||||||||||||||||
p.apiServerURL = lb.LoadBalancerServerURL() | ||||||||||||||||||
} else { | ||||||||||||||||||
p.apiServerURL = u.String() | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
logrus.Debugf("Supervisor proxy apiserver port changed; apiserver=%s lb=%v", p.apiServerURL, p.lbEnabled) | ||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same comment. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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: Lines 34 to 36 in 79ba10f
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ok, thanks for the clarification |
||||||||||||||||||
p.apiServerEnabled = true | ||||||||||||||||||
return nil | ||||||||||||||||||
} | ||||||||||||||||||
|
||||||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity: given that the bug in how we set
proxy.APIServerURL()
is fixed, bothproxy.APIServerURL()
orapiServerURL
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 backendThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree. I just wanted to doublecheck if my understanding was correct :)