Move kubelet --node-ip parsing to component-helpers

The same code should be used to parse the command-line argument and
the annotation. Unfortunately, for compatiblity reasons, they have to
handle invalid inputs differently...

(It doesn't make sense to put this code in cloud-provider, since
ParseNodeIPArgument is used for the non-cloud-provider case too.)

cmd/kubelet/app/server.go

@@ -1120,24 +1120,9 @@ func RunKubelet(kubeServer *options.KubeletServer, kubeDeps *kubelet.Dependencie
 	// Setup event recorder if required.
 	makeEventRecorder(kubeDeps, nodeName)
 
-	var nodeIPs []net.IP
+	nodeIPs, err := nodeutil.ParseNodeIPArgument(kubeServer.NodeIP, kubeServer.CloudProvider)
-	if kubeServer.NodeIP != "" {
+	if err != nil {
-		for _, ip := range strings.Split(kubeServer.NodeIP, ",") {
+		return fmt.Errorf("bad --node-ip %q: %v", kubeServer.NodeIP, err)
-			parsedNodeIP := netutils.ParseIPSloppy(strings.TrimSpace(ip))
-			if parsedNodeIP == nil {
-				klog.InfoS("Could not parse --node-ip ignoring", "IP", ip)
-			} else {
-				nodeIPs = append(nodeIPs, parsedNodeIP)
-			}
-		}
-	}
-
-	if len(nodeIPs) > 2 || (len(nodeIPs) == 2 && netutils.IsIPv6(nodeIPs[0]) == netutils.IsIPv6(nodeIPs[1])) {
-		return fmt.Errorf("bad --node-ip %q; must contain either a single IP or a dual-stack pair of IPs", kubeServer.NodeIP)
-	} else if len(nodeIPs) == 2 && kubeServer.CloudProvider != "" {
-		return fmt.Errorf("dual-stack --node-ip %q not supported when using a cloud provider", kubeServer.NodeIP)
-	} else if len(nodeIPs) == 2 && (nodeIPs[0].IsUnspecified() || nodeIPs[1].IsUnspecified()) {
-		return fmt.Errorf("dual-stack --node-ip %q cannot include '0.0.0.0' or '::'", kubeServer.NodeIP)
 	}
 
 	capabilities.Initialize(capabilities.Capabilities{

staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go

@@ -45,7 +45,6 @@ import (
 	controllersmetrics "k8s.io/component-base/metrics/prometheus/controllers"
 	nodeutil "k8s.io/component-helpers/node/util"
 	"k8s.io/klog/v2"
-	netutils "k8s.io/utils/net"
 )
 
 // labelReconcileInfo lists Node labels to reconcile, and how to reconcile them.
@@ -756,9 +755,9 @@ func getNodeProvidedIP(node *v1.Node) (net.IP, error) {
 		return nil, nil
 	}
 
-	nodeIP := netutils.ParseIPSloppy(providedIP)
+	nodeIP, err := nodeutil.ParseNodeIPAnnotation(providedIP)
-	if nodeIP == nil {
+	if err != nil {
-		return nil, fmt.Errorf("failed to parse node IP %q for node %q", providedIP, node.Name)
+		return nil, fmt.Errorf("failed to parse node IP %q for node %q: %v", providedIP, node.Name, err)
 	}
 
 	return nodeIP, nil

staging/src/k8s.io/component-helpers/node/util/ips.go

@@ -0,0 +1,81 @@
+/*
+Copyright 2023 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package util
+
+import (
+	"fmt"
+	"net"
+	"strings"
+
+	"k8s.io/klog/v2"
+	netutils "k8s.io/utils/net"
+)
+
+const (
+	cloudProviderNone     = ""
+	cloudProviderExternal = "external"
+)
+
+// parseNodeIP implements ParseNodeIPArgument and ParseNodeIPAnnotation
+func parseNodeIP(nodeIP string, allowDual, sloppy bool) ([]net.IP, error) {
+	var nodeIPs []net.IP
+	if nodeIP != "" || !sloppy {
+		for _, ip := range strings.Split(nodeIP, ",") {
+			if sloppy {
+				ip = strings.TrimSpace(ip)
+			}
+			parsedNodeIP := netutils.ParseIPSloppy(ip)
+			if parsedNodeIP == nil {
+				if sloppy {
+					klog.InfoS("Could not parse node IP. Ignoring", "IP", ip)
+				} else {
+					return nil, fmt.Errorf("could not parse %q", ip)
+				}
+			} else {
+				nodeIPs = append(nodeIPs, parsedNodeIP)
+			}
+		}
+	}
+
+	if len(nodeIPs) > 2 || (len(nodeIPs) == 2 && netutils.IsIPv6(nodeIPs[0]) == netutils.IsIPv6(nodeIPs[1])) {
+		return nil, fmt.Errorf("must contain either a single IP or a dual-stack pair of IPs")
+	} else if len(nodeIPs) == 2 && !allowDual {
+		return nil, fmt.Errorf("dual-stack not supported in this configuration")
+	} else if len(nodeIPs) == 2 && (nodeIPs[0].IsUnspecified() || nodeIPs[1].IsUnspecified()) {
+		return nil, fmt.Errorf("dual-stack node IP cannot include '0.0.0.0' or '::'")
+	}
+
+	return nodeIPs, nil
+}
+
+// ParseNodeIPArgument parses kubelet's --node-ip argument. If nodeIP contains invalid
+// values, they will be logged and ignored. Dual-stack node IPs are only supported if
+// cloudProvider is unset.
+func ParseNodeIPArgument(nodeIP, cloudProvider string) ([]net.IP, error) {
+	return parseNodeIP(nodeIP, cloudProvider == cloudProviderNone, true)
+}
+
+// ParseNodeIPAnnotation parses the `alpha.kubernetes.io/provided-node-ip` annotation,
+// which should be a single IP address. Unlike with ParseNodeIPArgument, invalid values
+// are considered an error.
+func ParseNodeIPAnnotation(nodeIP string) (net.IP, error) {
+	ips, err := parseNodeIP(nodeIP, false, false)
+	if err != nil || len(ips) == 0 {
+		return nil, err
+	}
+	return ips[0], nil
+}

staging/src/k8s.io/component-helpers/node/util/ips_test.go

@@ -0,0 +1,330 @@
+/*
+Copyright 2023 The Kubernetes Authors.
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package util
+
+import (
+	"fmt"
+	"net"
+	"reflect"
+	"strings"
+	"testing"
+
+	netutils "k8s.io/utils/net"
+)
+
+func TestParseNodeIPArgument(t *testing.T) {
+	testCases := []struct {
+		desc  string
+		in    string
+		out   []net.IP
+		err   string
+		ssErr string
+	}{
+		{
+			desc: "empty --node-ip",
+			in:   "",
+			out:  nil,
+		},
+		{
+			desc: "just whitespace (ignored)",
+			in:   " ",
+			out:  nil,
+		},
+		{
+			desc: "garbage (ignored)",
+			in:   "blah",
+			out:  nil,
+		},
+		{
+			desc: "single IPv4",
+			in:   "1.2.3.4",
+			out: []net.IP{
+				netutils.ParseIPSloppy("1.2.3.4"),
+			},
+		},
+		{
+			desc: "single IPv4 with whitespace",
+			in:   " 1.2.3.4   ",
+			out: []net.IP{
+				netutils.ParseIPSloppy("1.2.3.4"),
+			},
+		},
+		{
+			desc: "single IPv4 non-canonical",
+			in:   "01.2.3.004",
+			out: []net.IP{
+				netutils.ParseIPSloppy("1.2.3.4"),
+			},
+		},
+		{
+			desc: "single IPv4 invalid (ignored)",
+			in:   "1.2.3",
+			out:  nil,
+		},
+		{
+			desc: "single IPv4 CIDR (ignored)",
+			in:   "1.2.3.0/24",
+			out:  nil,
+		},
+		{
+			desc: "single IPv4 unspecified",
+			in:   "0.0.0.0",
+			out: []net.IP{
+				net.IPv4zero,
+			},
+		},
+		{
+			desc: "single IPv4 plus ignored garbage",
+			in:   "1.2.3.4,not-an-IPv6-address",
+			out: []net.IP{
+				netutils.ParseIPSloppy("1.2.3.4"),
+			},
+		},
+		{
+			desc: "single IPv6",
+			in:   "abcd::ef01",
+			out: []net.IP{
+				netutils.ParseIPSloppy("abcd::ef01"),
+			},
+		},
+		{
+			desc: "single IPv6 non-canonical",
+			in:   "abcd:0abc:00ab:0000:0000::1",
+			out: []net.IP{
+				netutils.ParseIPSloppy("abcd:abc:ab::1"),
+			},
+		},
+		{
+			desc: "simple dual-stack",
+			in:   "1.2.3.4,abcd::ef01",
+			out: []net.IP{
+				netutils.ParseIPSloppy("1.2.3.4"),
+				netutils.ParseIPSloppy("abcd::ef01"),
+			},
+			ssErr: "not supported in this configuration",
+		},
+		{
+			desc: "dual-stack with whitespace",
+			in:   "abcd::ef01 , 1.2.3.4",
+			out: []net.IP{
+				netutils.ParseIPSloppy("abcd::ef01"),
+				netutils.ParseIPSloppy("1.2.3.4"),
+			},
+			ssErr: "not supported in this configuration",
+		},
+		{
+			desc: "double IPv4",
+			in:   "1.2.3.4,5.6.7.8",
+			err:  "either a single IP or a dual-stack pair of IPs",
+		},
+		{
+			desc: "double IPv6",
+			in:   "abcd::1,abcd::2",
+			err:  "either a single IP or a dual-stack pair of IPs",
+		},
+		{
+			desc:  "dual-stack with unspecified",
+			in:    "1.2.3.4,::",
+			err:   "cannot include '0.0.0.0' or '::'",
+			ssErr: "not supported in this configuration",
+		},
+		{
+			desc:  "dual-stack with unspecified",
+			in:    "0.0.0.0,abcd::1",
+			err:   "cannot include '0.0.0.0' or '::'",
+			ssErr: "not supported in this configuration",
+		},
+		{
+			desc: "dual-stack plus ignored garbage",
+			in:   "abcd::ef01 , 1.2.3.4, something else",
+			out: []net.IP{
+				netutils.ParseIPSloppy("abcd::ef01"),
+				netutils.ParseIPSloppy("1.2.3.4"),
+			},
+			ssErr: "not supported in this configuration",
+		},
+		{
+			desc: "triple stack!",
+			in:   "1.2.3.4,abcd::1,5.6.7.8",
+			err:  "either a single IP or a dual-stack pair of IPs",
+		},
+	}
+
+	for _, tc := range testCases {
+		for _, cloudProvider := range []string{cloudProviderNone, cloudProviderExternal, "gce"} {
+			desc := fmt.Sprintf("%s, cloudProvider=%q", tc.desc, cloudProvider)
+			t.Run(desc, func(t *testing.T) {
+				parsed, err := ParseNodeIPArgument(tc.in, cloudProvider)
+
+				expectedOut := tc.out
+				expectedErr := tc.err
+
+				// Dual-stack is only supported with no cloudProvider
+				if cloudProvider != "" {
+					if len(tc.out) == 2 {
+						expectedOut = nil
+					}
+					if tc.ssErr != "" {
+						expectedErr = tc.ssErr
+					}
+				}
+
+				if !reflect.DeepEqual(parsed, expectedOut) {
+					t.Errorf("expected %#v, got %#v", expectedOut, parsed)
+				}
+				if err != nil {
+					if expectedErr == "" {
+						t.Errorf("unexpected error %v", err)
+					} else if !strings.Contains(err.Error(), expectedErr) {
+						t.Errorf("expected error with %q, got %v", expectedErr, err)
+					}
+				} else if expectedErr != "" {
+					t.Errorf("expected error with %q, got no error", expectedErr)
+				}
+			})
+		}
+	}
+}
+
+func TestParseNodeIPAnnotation(t *testing.T) {
+	testCases := []struct {
+		desc string
+		in   string
+		out  net.IP
+		err  string
+	}{
+		{
+			desc: "empty --node-ip",
+			in:   "",
+			err:  "could not parse",
+		},
+		{
+			desc: "just whitespace",
+			in:   " ",
+			err:  "could not parse",
+		},
+		{
+			desc: "garbage",
+			in:   "blah",
+			err:  "could not parse",
+		},
+		{
+			desc: "single IPv4",
+			in:   "1.2.3.4",
+			out:  netutils.ParseIPSloppy("1.2.3.4"),
+		},
+		{
+			desc: "single IPv4 with whitespace",
+			in:   " 1.2.3.4   ",
+			err:  "could not parse",
+		},
+		{
+			desc: "single IPv4 non-canonical",
+			in:   "01.2.3.004",
+			out:  netutils.ParseIPSloppy("1.2.3.4"),
+		},
+		{
+			desc: "single IPv4 invalid",
+			in:   "1.2.3",
+			err:  "could not parse",
+		},
+		{
+			desc: "single IPv4 CIDR",
+			in:   "1.2.3.0/24",
+			err:  "could not parse",
+		},
+		{
+			desc: "single IPv4 unspecified",
+			in:   "0.0.0.0",
+			out:  net.IPv4zero,
+		},
+		{
+			desc: "single IPv4 plus garbage",
+			in:   "1.2.3.4,not-an-IPv6-address",
+			err:  "could not parse",
+		},
+		{
+			desc: "single IPv6",
+			in:   "abcd::ef01",
+			out:  netutils.ParseIPSloppy("abcd::ef01"),
+		},
+		{
+			desc: "single IPv6 non-canonical",
+			in:   "abcd:0abc:00ab:0000:0000::1",
+			out:  netutils.ParseIPSloppy("abcd:abc:ab::1"),
+		},
+		{
+			desc: "simple dual-stack",
+			in:   "1.2.3.4,abcd::ef01",
+			err:  "not supported in this configuration",
+		},
+		{
+			desc: "dual-stack with whitespace",
+			in:   "abcd::ef01 , 1.2.3.4",
+			err:  "could not parse",
+		},
+		{
+			desc: "double IPv4",
+			in:   "1.2.3.4,5.6.7.8",
+			err:  "either a single IP or a dual-stack pair of IPs",
+		},
+		{
+			desc: "double IPv6",
+			in:   "abcd::1,abcd::2",
+			err:  "either a single IP or a dual-stack pair of IPs",
+		},
+		{
+			desc: "dual-stack with unspecified",
+			in:   "1.2.3.4,::",
+			err:  "not supported in this configuration",
+		},
+		{
+			desc: "dual-stack with unspecified",
+			in:   "0.0.0.0,abcd::1",
+			err:  "not supported in this configuration",
+		},
+		{
+			desc: "dual-stack plus garbage",
+			in:   "abcd::ef01 , 1.2.3.4, something else",
+			err:  "could not parse",
+		},
+		{
+			desc: "triple stack!",
+			in:   "1.2.3.4,abcd::1,5.6.7.8",
+			err:  "either a single IP or a dual-stack pair of IPs",
+		},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			parsed, err := ParseNodeIPAnnotation(tc.in)
+
+			if !reflect.DeepEqual(parsed, tc.out) {
+				t.Errorf("expected %#v, got %#v", tc.out, parsed)
+			}
+			if err != nil {
+				if tc.err == "" {
+					t.Errorf("unexpected error %v", err)
+				} else if !strings.Contains(err.Error(), tc.err) {
+					t.Errorf("expected error with %q, got %v", tc.err, err)
+				}
+			} else if tc.err != "" {
+				t.Errorf("expected error with %q, got no error", tc.err)
+			}
+		})
+	}
+}