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
kube-proxy react on Node PodCIDR changes #111344
Conversation
@aojea: This issue is currently awaiting triage. If a SIG or subproject determines this is a relevant issue, they will accept it by applying the The Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/sig network |
Do we actually expect that to work? (Deleting and recreating a node while kubernetes components are running on that node without restarting them.) |
It's possible that the node IP would change in that circumstance as well, so this is not the only situation in which kube-proxy would need to restart itself if we want to support that. |
pkg/proxy/ipvs/proxier.go
Outdated
if proxier.localDetector.IsImplemented() && proxier.localDetector.Mode() == proxyconfigapi.LocalModeNodeCIDR { | ||
nodeLocalCIDR := proxier.localDetector.IfLocal()[1] | ||
if !utilproxy.NodeContainsPodCIDR(node, nodeLocalCIDR) { | ||
klog.ErrorS(nil, "Inconsisten statu, kube-proxy configed Local CIDR not present on Node.Spec.PodCIDRs", "node", klog.KObj(node), "local CIDR", nodeLocalCIDR, "PodCIDRs", node.Spec.PodCIDRs) |
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.
Match this log message with the one above.
Also, this code looks to be the same, we might want to factor it out to a func? (name it MaybeRestartOnInconsistentPodCIDR()?)
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.
I thought about that, the reasons I didn't do it is because these are 7 lines of code with a lot of deps on proxier
methods ... the refactor to a func is not going to save more lines and will make it harder to read IMHO ...
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.
to be honest, I like your idea, but with current state of the code on both proxies, I don't know if it is better to keep them isolated or to try to consolidate them
one bug at a time ;) ... until now we only have evidence of this one |
Left a comment about potentially being able to restrict the surface area of the change. |
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.
Sorry, forgot to hit send on the review
pkg/proxy/iptables/proxier.go
Outdated
@@ -639,6 +640,17 @@ func (proxier *Proxier) OnNodeUpdate(oldNode, node *v1.Node) { | |||
return | |||
} | |||
|
|||
// kube-proxy in LocalModeNodeCIDR mode may cache stale Node.PodCIDR. |
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.
An alternative to avoid exposing the details of the detector everywhere:
add method to localDetector
err := localDetector.IsValid(node)
if err != nil {
klkog.ErrorS(nil, "proxier.localDetector is no longer valid (%v), restarting kube-proxy", err)
...FlushAndExit()
}
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.
I've added a new method to return the Value() of the local traffic discriminator
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.
I meant instead of exposing the Value() and putting the logic outside, we expose just the notion that the detector is still valid (which is what is being checked here).
so the proxier call site just has
if proxier.localDetector.IsImplemented() {
if err := proxier.localDetector.IsValid(node); err != nil {
klog.ErrorS(...)
klog.FlushAndExit(...)
}
}
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.
yeah, but localDetectors can use CIDRs or interfaces names/prefixes, or whatever we want to implement in the detector, adding a new method IsValid(node *v1.Node)
is only valid for one of the traffic detector modes.
Do we want to add such specific method to an Interface
?
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.
I'm suggesting removing Mode() and Value() and instead have a IsValid(node *v1.Node).
For detectors that are always Valid, this can always return true
.
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.
ah, that makes more sense
Implementation LGTM. I'll leave the larger/philsophical questions about Node object changes to others... |
1d2acdf
to
a95b033
Compare
I've reduced the scope to PodCIDRs changes only when using nodeCIDR local detector, this is much safer now |
a95b033
to
6575384
Compare
nodeConfig := config.NewNodeConfig(currentNodeInformerFactory.Core().V1().Nodes(), s.ConfigSyncPeriod) | ||
// https://issues.k8s.io/111321 | ||
if s.localDetectorMode == kubeproxyconfig.LocalModeNodeCIDR { | ||
nodeConfig.RegisterEventHandler(&proxy.NodePodCIDRHandler{}) |
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.
Wouldn't that make it panic on_add since there is no local node UID set?
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.
check I changed the logic to panic on PodCIDR only and added test cases to cover the different states, if no PodCIDR exists in the handler it means that the handler wasn't initialized, os it uses the new value for that
6575384
to
5a2ad23
Compare
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.
Thanks!
/lgtm
/approve
Kube/proxy, in NodeCIDR local detector mode, uses the node.Spec.PodCIDRs field to build the Services iptables rules. The Node object depends on the kubelet, but if kube-proxy runs as a static pods or as a standalone binary, it is not possible to guarantee that the values obtained at bootsrap are valid, causing traffic outages. Kube-proxy has to react on node changes to avoid this problems, it simply restarts if detect that the node PodCIDRs have changed. In case that the Node has been deleted, kube-proxy will only log an error and keep working, since it may break graceful shutdowns of the node.
5a2ad23
to
75913e9
Compare
I had to rebase @thockin :/ |
Thanks! /lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aojea, dcbw, thockin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
/kind bug
kube-proxy handle node PodCIDR changs
Kube/proxy, in NodeCIDR local detector mode, uses the node.Spec.PodCIDRs
field to build the Services iptables rules.
The Node object depends on the kubelet, but if kube-proxy runs as a
static pods or as a standalone binary, it is not possible to guarantee
that the values obtained at bootsrap are valid, causing traffic outages.
Kube-proxy has to react on node changes to avoid this problems, it
simply restarts if detect that the node PodCIDRs have changed.
In case that the Node has been deleted, kube-proxy will only log an
error and keep working, since it may break graceful shutdowns of the
node.
Fixes #111321, #112739