set config.BindAddress to IPv4 address "127.0.0.1" if not specified #83822

zouyee · 2019-10-12T02:20:23Z

SetDefaults_KubeProxyConfiguration will set config.BindAddress to IPv4 address "0.0.0.0" if not specified

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:
SetDefaults_KubeProxyConfiguration will set config.BindAddress to IPv4 address "0.0.0.0" if not specified

Does this PR introduce a user-facing change?:

set config.BindAddress to IPv4 address "127.0.0.1" if not specified

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

zouyee · 2019-10-12T02:21:50Z

/sig network
/cc @liggitt
/assign @MrHohn @freehan

zouyee · 2019-10-12T02:24:15Z

/kind bug

zouyee · 2019-10-12T04:06:25Z

/retest

liggitt · 2019-10-14T12:28:22Z

pkg/proxy/iptables/proxier.go

@@ -278,11 +278,6 @@ func NewProxier(ipt utiliptables.Interface,
 	masqueradeValue := 1 << uint(masqueradeBit)
 	masqueradeMark := fmt.Sprintf("%#08x/%#08x", masqueradeValue, masqueradeValue)

-	if nodeIP == nil {


This has been present for ~4 years (1acaa1d#diff-d51765b83fe795b469e8a86276b12dc9R218-R221). Can you explain why this is correct to remove?

@liggitt I think #77167 introduced a breaking change where kube-proxy exits if nodeIP is nil. Before v1.16, if nodeIP is nil kube-proxy continues assuming later it would default node IP to 127.0.0.1.

This has been present for ~4 years (1acaa1d#diff-d51765b83fe795b469e8a86276b12dc9R218-R221). Can you explain why this is correct to remove?

At first, I thought that iptables.NewProxier was only used here, if so, it might be redundant to validate nodeIP again. Later, I found that it was used by https://github.com/kubernetes/kubernetes/blob/master/pkg/kubemark/hollow_proxy.go#L82, so I withdrew the modification here

How is this different from simply rolling back #77167 ? I thought the point of that was to force kube-proxy not to proceed with a wrong IP address?

#77167 also adds exponential back-off which fixes #37414 where kube-proxy would try to get the node IP before the node is registered. IMO removing the nodeIP check entirely and having the proxy implementations fall back to use 127.0.0.1 is better.

liggitt · 2019-10-14T12:30:07Z

/assign @bowei @thockin
as original reviewers of #77167

zouyee · 2019-10-19T13:11:40Z

/assign @freehan

andrewsykim · 2019-10-19T14:18:44Z

cmd/kube-proxy/app/server_others.go

@@ -137,7 +137,8 @@ func newProxyServer(
 	if nodeIP.IsUnspecified() {
 		nodeIP = utilnode.GetNodeIP(client, hostname)
 		if nodeIP == nil {
-			return nil, fmt.Errorf("unable to get node IP for hostname %s", hostname)
+			klog.Warning("invalid nodeIP, initializing kube-proxy with 127.0.0.1 as nodeIP")
+			nodeIP = net.ParseIP("127.0.0.1")


The proxier implementations already sets node IP to 127.0.0.1 if it's nil, wondering if it makes more sense to remove this nil check altogether (see #84071 -- closed in favor of this PR).

This change may now impact ipvs.NewProxier() (I can't tell for sure currently as I'm on mobile only) which used to not necessarily come with a set node IP.

Is there a reason we cannot just drop line 140 without adding anything else? That's what the code originally used to do, so it seems like the cleaner approach to me with regards to addressing the bug.

IPVS proxier has the same behavior as iptables for nil nodeIP (see https://github.com/kubernetes/kubernetes/blob/master/pkg/proxy/ipvs/proxier.go#L406-L409) so agreed with @timoreimann this can just be

if nodeIP.IsUnspecified() { nodeIP = utilnode.GetNodeIP(client, hostname) }

Should we remove the code in NewProxier implementations that checks for and handles nil? This is kind of a contract change - they can assume it is non-nil.

So I guess from a compat POV this is correct. From a correctness POV, this is wrong. At least the iptables module will make a wrong decision based on this information. IPVS seems only to log it.

Having the user set the --bind-address seems like the RIGHT answer, but I acknowledge that this is a breaking change and we should fix it.

Can we enhance the log message to say something like:

klog.V(0).Infof("can't determine this node's IP, assuming 127.0.0.1; if this is incorrect, set the --bind-address flag") ``` Of course, flags are passe and config files are vogue, and it's harder to reference config file fields. This is also wrongish in the case of IPv6.

Should we remove the code in NewProxier implementations that checks for and handles nil? This is kind of a contract change - they can assume it is non-nil.

If iptables.NewProxier was only used here, it might be redundant to validate nodeIP nil again. But, I found that it was used by https://github.com/kubernetes/kubernetes/blob/master/pkg/kubemark/hollow_proxy.go#L82, so I withdrew the modification here

Can we fix that do explicitly pass 127.0.0.1 and remove it from NewProxier?

Can we fix that do explicitly pass 127.0.0.1 and remove it from NewProxier?

Done.PTAL

andrewsykim · 2019-10-21T20:37:56Z

/priority important-soon

k8s-ci-robot · 2019-10-24T05:37:45Z

@zouyee: Those labels are not set on the issue: area/dependency, kind/feature, area/code-generation

In response to this:

/remove-area apiserver
/remove-area cloudprovider
/remove-area dependency
/remove-area e2e-test-framework
/remove-area ipvs
/remove-area kubeadm
/remove-area kubectl kubelet test
/remove-kind feature
/remove-sig api-machinery apps auth cli cloud-provider cluster-lifecycle network storage testing

/remove-sig node instrumentation scheduling
/remove-kind api-change
/remove-area code-generation