Warn users with bad --nodeport-addresses

If users don't pass any --nodeport-addresses, suggest they should pass
`--nodeport-addresses primary`, to avoid accepting NodePort
connections on all interfaces.

If users pass a single-stack --nodeport-addresses in what looks like a
dual-stack cluster, warn them that they probably ought to be passing a
dual-stack --nodeport-addresses.

cmd/kube-proxy/app/server.go

@@ -661,6 +661,11 @@ func newProxyServer(logger klog.Logger, config *kubeproxyconfig.KubeProxyConfigu
 		return nil, err
 	}
 
+	err = checkBadConfig(s)
+	if err != nil {
+		logger.Error(err, "Kube-proxy configuration may be incomplete or incorrect")
+	}
+
 	ipv4Supported, ipv6Supported, dualStackSupported, err := s.platformCheckSupported()
 	if err != nil {
 		return nil, err
@@ -672,7 +677,7 @@ func newProxyServer(logger klog.Logger, config *kubeproxyconfig.KubeProxyConfigu
 		logger.Info("kube-proxy running in single-stack mode", "ipFamily", s.PrimaryIPFamily)
 	}
 
-	err, fatal := checkIPConfig(s, dualStackSupported)
+	err, fatal := checkBadIPConfig(s, dualStackSupported)
 	if err != nil {
 		if fatal {
 			return nil, fmt.Errorf("kube-proxy configuration is incorrect: %v", err)
@@ -688,8 +693,42 @@ func newProxyServer(logger klog.Logger, config *kubeproxyconfig.KubeProxyConfigu
 	return s, nil
 }
 
-// checkIPConfig confirms that s has proper configuration for its primary IP family.
-func checkIPConfig(s *ProxyServer, dualStackSupported bool) (error, bool) {
+// checkBadConfig checks for bad/deprecated configuation
+func checkBadConfig(s *ProxyServer) error {
+	var errors []error
+
+	// At this point we haven't seen any actual Services or EndpointSlices, so we
+	// don't really know if the cluster is expected to be single- or dual-stack. But
+	// we can at least take note of whether there is any explicitly-dual-stack
+	// configuration.
+	anyDualStackConfig := false
+	clusterCIDRs := strings.Split(s.Config.ClusterCIDR, ",")
+	for _, config := range [][]string{clusterCIDRs, s.Config.NodePortAddresses, s.Config.IPVS.ExcludeCIDRs, s.podCIDRs} {
+		if dual, _ := netutils.IsDualStackCIDRStrings(config); dual {
+			anyDualStackConfig = true
+			break
+		}
+	}
+
+	// Warn if NodePortAddresses does not limit connections on all IP families that
+	// seem to be in use.
+	cidrsByFamily := proxyutil.MapCIDRsByIPFamily(s.Config.NodePortAddresses)
+	if len(s.Config.NodePortAddresses) == 0 {
+		errors = append(errors, fmt.Errorf("nodePortAddresses is unset; NodePort connections will be accepted on all local IPs. Consider using `--nodeport-addresses primary`"))
+	} else if anyDualStackConfig && len(cidrsByFamily[s.PrimaryIPFamily]) == len(s.Config.NodePortAddresses) {
+		errors = append(errors, fmt.Errorf("cluster appears to be dual-stack but nodePortAddresses contains only %s addresses; NodePort connections will be accepted on all local %s IPs", s.PrimaryIPFamily, proxyutil.OtherIPFamily(s.PrimaryIPFamily)))
+	} else if len(cidrsByFamily[s.PrimaryIPFamily]) == 0 {
+		errors = append(errors, fmt.Errorf("cluster appears to be %s-primary but nodePortAddresses contains only %s addresses; NodePort connections will be accepted on all local %s IPs", s.PrimaryIPFamily, proxyutil.OtherIPFamily(s.PrimaryIPFamily), s.PrimaryIPFamily))
+	}
+
+	return utilerrors.NewAggregate(errors)
+}
+
+// checkBadIPConfig checks for bad configuration relative to s.PrimaryIPFamily.
+// Historically, we did not check most of the config options, so we cannot retroactively
+// make IP family mismatches in those options be fatal. When we add new options to check
+// here, we should make problems with those options be fatal.
+func checkBadIPConfig(s *ProxyServer, dualStackSupported bool) (err error, fatal bool) {
 	var errors []error
 	var badFamily netutils.IPFamily
 
@@ -706,11 +745,6 @@ func checkIPConfig(s *ProxyServer, dualStackSupported bool) (error, bool) {
 		clusterType = fmt.Sprintf("%s-only", s.PrimaryIPFamily)
 	}
 
-	// Historically, we did not check most of the config options, so we cannot
-	// retroactively make IP family mismatches in those options be fatal. When we add
-	// new options to check here, we should make problems with those options be fatal.
-	fatal := false
-
 	if s.Config.ClusterCIDR != "" {
 		clusterCIDRs := strings.Split(s.Config.ClusterCIDR, ",")
 		if badCIDRs(clusterCIDRs, badFamily) {
@@ -722,10 +756,6 @@ func checkIPConfig(s *ProxyServer, dualStackSupported bool) (error, bool) {
 		}
 	}
 
-	if badCIDRs(s.Config.NodePortAddresses, badFamily) {
-		errors = append(errors, fmt.Errorf("cluster is %s but nodePortAddresses contains only IPv%s addresses", clusterType, badFamily))
-	}
-
 	if badCIDRs(s.podCIDRs, badFamily) {
 		errors = append(errors, fmt.Errorf("cluster is %s but node.spec.podCIDRs contains only IPv%s addresses", clusterType, badFamily))
 		if s.Config.DetectLocalMode == kubeproxyconfig.LocalModeNodeCIDR {
@@ -753,6 +783,9 @@ func checkIPConfig(s *ProxyServer, dualStackSupported bool) (error, bool) {
 		}
 	}
 
+	// Note that s.Config.NodePortAddresses gets checked as part of checkBadConfig()
+	// so it doesn't need to be checked here.
+
 	return utilerrors.NewAggregate(errors), fatal
 }
 

cmd/kube-proxy/app/server_test.go

@@ -849,7 +849,81 @@ func Test_detectNodeIPs(t *testing.T) {
 	}
 }
 
-func Test_checkIPConfig(t *testing.T) {
+func Test_checkBadConfig(t *testing.T) {
+	cases := []struct {
+		name  string
+		proxy *ProxyServer
+		err   bool
+	}{
+		{
+			name: "single-stack NodePortAddresses with single-stack config",
+			proxy: &ProxyServer{
+				Config: &kubeproxyconfig.KubeProxyConfiguration{
+					ClusterCIDR:       "10.0.0.0/8",
+					NodePortAddresses: []string{"192.168.0.0/24"},
+				},
+				PrimaryIPFamily: v1.IPv4Protocol,
+			},
+			err: false,
+		},
+		{
+			name: "dual-stack NodePortAddresses with dual-stack config",
+			proxy: &ProxyServer{
+				Config: &kubeproxyconfig.KubeProxyConfiguration{
+					ClusterCIDR:       "10.0.0.0/8,fd09::/64",
+					NodePortAddresses: []string{"192.168.0.0/24", "fd03::/64"},
+				},
+				PrimaryIPFamily: v1.IPv4Protocol,
+			},
+			err: false,
+		},
+		{
+			name: "empty NodePortAddresses",
+			proxy: &ProxyServer{
+				Config: &kubeproxyconfig.KubeProxyConfiguration{
+					NodePortAddresses: []string{},
+				},
+				PrimaryIPFamily: v1.IPv4Protocol,
+			},
+			err: true,
+		},
+		{
+			name: "single-stack NodePortAddresses with dual-stack config",
+			proxy: &ProxyServer{
+				Config: &kubeproxyconfig.KubeProxyConfiguration{
+					ClusterCIDR:       "10.0.0.0/8,fd09::/64",
+					NodePortAddresses: []string{"192.168.0.0/24"},
+				},
+				PrimaryIPFamily: v1.IPv4Protocol,
+			},
+			err: true,
+		},
+		{
+			name: "wrong-single-stack NodePortAddresses",
+			proxy: &ProxyServer{
+				Config: &kubeproxyconfig.KubeProxyConfiguration{
+					ClusterCIDR:       "fd09::/64",
+					NodePortAddresses: []string{"192.168.0.0/24"},
+				},
+				PrimaryIPFamily: v1.IPv6Protocol,
+			},
+			err: true,
+		},
+	}
+
+	for _, c := range cases {
+		t.Run(c.name, func(t *testing.T) {
+			err := checkBadConfig(c.proxy)
+			if err != nil && !c.err {
+				t.Errorf("unexpected error: %v", err)
+			} else if err == nil && c.err {
+				t.Errorf("unexpected lack of error")
+			}
+		})
+	}
+}
+
+func Test_checkBadIPConfig(t *testing.T) {
 	cases := []struct {
 		name    string
 		proxy   *ProxyServer
@@ -929,53 +1003,6 @@ func Test_checkIPConfig(t *testing.T) {
 			dsFatal: false,
 		},
 
-		{
-			name: "ok single-stack nodePortAddresses",
-			proxy: &ProxyServer{
-				Config: &kubeproxyconfig.KubeProxyConfiguration{
-					NodePortAddresses: []string{"10.0.0.0/8", "192.168.0.0/24"},
-				},
-				PrimaryIPFamily: v1.IPv4Protocol,
-			},
-			ssErr: false,
-			dsErr: false,
-		},
-		{
-			name: "ok dual-stack nodePortAddresses",
-			proxy: &ProxyServer{
-				Config: &kubeproxyconfig.KubeProxyConfiguration{
-					NodePortAddresses: []string{"10.0.0.0/8", "fd01:2345::/64", "fd01:abcd::/64"},
-				},
-				PrimaryIPFamily: v1.IPv4Protocol,
-			},
-			ssErr: false,
-			dsErr: false,
-		},
-		{
-			name: "ok reversed dual-stack nodePortAddresses",
-			proxy: &ProxyServer{
-				Config: &kubeproxyconfig.KubeProxyConfiguration{
-					NodePortAddresses: []string{"fd01:2345::/64", "fd01:abcd::/64", "10.0.0.0/8"},
-				},
-				PrimaryIPFamily: v1.IPv4Protocol,
-			},
-			ssErr: false,
-			dsErr: false,
-		},
-		{
-			name: "wrong-family nodePortAddresses",
-			proxy: &ProxyServer{
-				Config: &kubeproxyconfig.KubeProxyConfiguration{
-					NodePortAddresses: []string{"10.0.0.0/8"},
-				},
-				PrimaryIPFamily: v1.IPv6Protocol,
-			},
-			ssErr:   true,
-			ssFatal: false,
-			dsErr:   true,
-			dsFatal: false,
-		},
-
 		{
 			name: "ok single-stack node.spec.podCIDRs",
 			proxy: &ProxyServer{
@@ -1133,7 +1160,7 @@ func Test_checkIPConfig(t *testing.T) {
 
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
-			err, fatal := checkIPConfig(c.proxy, false)
+			err, fatal := checkBadIPConfig(c.proxy, false)
 			if err != nil && !c.ssErr {
 				t.Errorf("unexpected error in single-stack case: %v", err)
 			} else if err == nil && c.ssErr {
@@ -1142,7 +1169,7 @@ func Test_checkIPConfig(t *testing.T) {
 				t.Errorf("expected fatal=%v, got %v", c.ssFatal, fatal)
 			}
 
-			err, fatal = checkIPConfig(c.proxy, true)
+			err, fatal = checkBadIPConfig(c.proxy, true)
 			if err != nil && !c.dsErr {
 				t.Errorf("unexpected error in dual-stack case: %v", err)
 			} else if err == nil && c.dsErr {