Update GetNodeAddressesFromNodeIP and ParseNodeIPAnnotation for cloud dual-stack

Update callers for the new APIs, but don't change their behavior yet.

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

@@ -739,7 +739,7 @@ func updateNodeAddressesFromNodeIP(node *v1.Node, nodeAddresses []v1.NodeAddress
 
 	providedNodeIP, exists := node.ObjectMeta.Annotations[cloudproviderapi.AnnotationAlphaProvidedIPAddr]
 	if exists {
-		nodeAddresses, err = cloudnodeutil.GetNodeAddressesFromNodeIP(providedNodeIP, nodeAddresses)
+		nodeAddresses, err = cloudnodeutil.GetNodeAddressesFromNodeIP(providedNodeIP, nodeAddresses, false)
 	}
 
 	return nodeAddresses, err

staging/src/k8s.io/cloud-provider/node/helpers/address.go

@@ -85,7 +85,7 @@ func GetNodeAddressesFromNodeIPLegacy(nodeIP net.IP, cloudNodeAddresses []v1.Nod
 	}
 
 	// Otherwise the result is the same as for GetNodeAddressesFromNodeIP
-	return GetNodeAddressesFromNodeIP(nodeIP.String(), cloudNodeAddresses)
+	return GetNodeAddressesFromNodeIP(nodeIP.String(), cloudNodeAddresses, false)
 }
 
 // GetNodeAddressesFromNodeIP filters the provided list of nodeAddresses to match the
@@ -102,32 +102,40 @@ func GetNodeAddressesFromNodeIPLegacy(nodeIP net.IP, cloudNodeAddresses []v1.Nod
 // GetNodeAddressesFromNodeIPLegacy, because that case never occurs for external cloud
 // providers, because kubelet does not set the `provided-node-ip` annotation in that
 // case.)
-func GetNodeAddressesFromNodeIP(providedNodeIP string, cloudNodeAddresses []v1.NodeAddress) ([]v1.NodeAddress, error) {
+func GetNodeAddressesFromNodeIP(providedNodeIP string, cloudNodeAddresses []v1.NodeAddress, allowDualStack bool) ([]v1.NodeAddress, error) {
-	nodeIP, err := nodeutil.ParseNodeIPAnnotation(providedNodeIP)
+	nodeIPs, err := nodeutil.ParseNodeIPAnnotation(providedNodeIP, allowDualStack)
 	if err != nil {
 		return nil, fmt.Errorf("failed to parse node IP %q: %v", providedNodeIP, err)
 	}
 
-	// For every address supplied by the cloud provider that matches nodeIP, nodeIP is the enforced node address for
-	// that address Type (like InternalIP and ExternalIP), meaning other addresses of the same Type are discarded.
-	// See #61921 for more information: some cloud providers may supply secondary IPs, so nodeIP serves as a way to
-	// ensure that the correct IPs show up on a Node object.
 	enforcedNodeAddresses := []v1.NodeAddress{}
-
 	nodeIPTypes := make(map[v1.NodeAddressType]bool)
-	for _, nodeAddress := range cloudNodeAddresses {
+
-		if netutils.ParseIPSloppy(nodeAddress.Address).Equal(nodeIP) {
+	for _, nodeIP := range nodeIPs {
-			enforcedNodeAddresses = append(enforcedNodeAddresses, v1.NodeAddress{Type: nodeAddress.Type, Address: nodeAddress.Address})
+		// For every address supplied by the cloud provider that matches nodeIP,
-			nodeIPTypes[nodeAddress.Type] = true
+		// nodeIP is the enforced node address for that address Type (like
+		// InternalIP and ExternalIP), meaning other addresses of the same Type
+		// are discarded. See #61921 for more information: some cloud providers
+		// may supply secondary IPs, so nodeIP serves as a way to ensure that the
+		// correct IPs show up on a Node object.
+
+		matched := false
+		for _, nodeAddress := range cloudNodeAddresses {
+			if netutils.ParseIPSloppy(nodeAddress.Address).Equal(nodeIP) {
+				enforcedNodeAddresses = append(enforcedNodeAddresses, v1.NodeAddress{Type: nodeAddress.Type, Address: nodeAddress.Address})
+				nodeIPTypes[nodeAddress.Type] = true
+				matched = true
+			}
+		}
+
+		// nodeIP must be among the addresses supplied by the cloud provider
+		if !matched {
+			return nil, fmt.Errorf("failed to get node address from cloud provider that matches ip: %v", nodeIP)
 		}
 	}
 
-	// nodeIP must be among the addresses supplied by the cloud provider
+	// Now use all other addresses supplied by the cloud provider NOT of the same Type
-	if len(enforcedNodeAddresses) == 0 {
+	// as any nodeIP.
-		return nil, fmt.Errorf("failed to get node address from cloud provider that matches ip: %v", nodeIP)
-	}
-
-	// nodeIP was found, now use all other addresses supplied by the cloud provider NOT of the same Type as nodeIP.
 	for _, nodeAddress := range cloudNodeAddresses {
 		if !nodeIPTypes[nodeAddress.Type] {
 			enforcedNodeAddresses = append(enforcedNodeAddresses, v1.NodeAddress{Type: nodeAddress.Type, Address: nodeAddress.Address})

staging/src/k8s.io/cloud-provider/node/helpers/address_test.go

@@ -428,11 +428,131 @@ func TestGetNodeAddressesFromNodeIP(t *testing.T) {
 			},
 			shouldError: false,
 		},
+		{
+			name:   "Single-stack cloud, dual-stack request",
+			nodeIP: "10.1.1.1,fc01:1234::5678",
+			nodeAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			shouldError: true,
+		},
+		{
+			name:   "Dual-stack cloud, IPv4 first, IPv4-primary request",
+			nodeIP: "10.1.1.1,fc01:1234::5678",
+			nodeAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeInternalIP, Address: "fc01:1234::5678"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			expectedAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeInternalIP, Address: "fc01:1234::5678"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			shouldError: false,
+		},
+		{
+			name:   "Dual-stack cloud, IPv6 first, IPv4-primary request",
+			nodeIP: "10.1.1.1,fc01:1234::5678",
+			nodeAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "fc01:1234::5678"},
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			expectedAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeInternalIP, Address: "fc01:1234::5678"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			shouldError: false,
+		},
+		{
+			name:   "Dual-stack cloud, dual-stack request, multiple IPs",
+			nodeIP: "10.1.1.1,fc01:1234::5678",
+			nodeAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeInternalIP, Address: "10.1.1.2"},
+				{Type: v1.NodeInternalIP, Address: "fc01:1234::1234"},
+				{Type: v1.NodeInternalIP, Address: "fc01:1234::5678"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			// additional IPs of the same type are removed, as in the
+			// single-stack case.
+			expectedAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeInternalIP, Address: "fc01:1234::5678"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			shouldError: false,
+		},
+		{
+			name:   "Dual-stack cloud, dual-stack request, extra ExternalIP",
+			nodeIP: "10.1.1.1,fc01:1234::5678",
+			nodeAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeInternalIP, Address: "fc01:1234::1234"},
+				{Type: v1.NodeInternalIP, Address: "fc01:1234::5678"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			// The ExternalIP is preserved, since no ExternalIP was matched
+			// by --node-ip.
+			expectedAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeInternalIP, Address: "fc01:1234::5678"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			shouldError: false,
+		},
+		{
+			name:   "Dual-stack cloud, dual-stack request, multiple ExternalIPs",
+			nodeIP: "fc01:1234::5678,10.1.1.1",
+			nodeAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeInternalIP, Address: "fc01:1234::5678"},
+				{Type: v1.NodeExternalIP, Address: "2001:db1::1"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			// The ExternalIPs are preserved, since no ExternalIP was matched
+			// by --node-ip.
+			expectedAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "fc01:1234::5678"},
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeExternalIP, Address: "2001:db1::1"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			shouldError: false,
+		},
+		{
+			name:   "Dual-stack cloud, dual-stack request, mixed InternalIP/ExternalIP match",
+			nodeIP: "55.55.55.55,fc01:1234::5678",
+			nodeAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeInternalIP, Address: "fc01:1234::5678"},
+				{Type: v1.NodeExternalIP, Address: "2001:db1::1"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			// Since the IPv4 --node-ip value matched an ExternalIP, that
+			// filters out the IPv6 ExternalIP. Since the IPv6 --node-ip value
+			// matched in InternalIP, that filters out the IPv4 InternalIP
+			// value.
+			expectedAddresses: []v1.NodeAddress{
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeInternalIP, Address: "fc01:1234::5678"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			shouldError: false,
+		},
 	}
 
 	for _, tt := range cases {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := GetNodeAddressesFromNodeIP(tt.nodeIP, tt.nodeAddresses)
+			got, err := GetNodeAddressesFromNodeIP(tt.nodeIP, tt.nodeAddresses, true)
 			if (err != nil) != tt.shouldError {
 				t.Errorf("GetNodeAddressesFromNodeIP() error = %v, wantErr %v", err, tt.shouldError)
 				return

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

@@ -74,12 +74,9 @@ func ParseNodeIPArgument(nodeIP, cloudProvider string, allowCloudDualStack bool)
 }
 
 // ParseNodeIPAnnotation parses the `alpha.kubernetes.io/provided-node-ip` annotation,
-// which should be a single IP address. Unlike with ParseNodeIPArgument, invalid values
+// which can be either a single IP address or (if allowDualStack is true) a
+// comma-separated pair of IP addresses. Unlike with ParseNodeIPArgument, invalid values
 // are considered an error.
-func ParseNodeIPAnnotation(nodeIP string) (net.IP, error) {
+func ParseNodeIPAnnotation(nodeIP string, allowDualStack bool) ([]net.IP, error) {
-	ips, err := parseNodeIP(nodeIP, false, false)
+	return parseNodeIP(nodeIP, allowDualStack, 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

@@ -214,10 +214,11 @@ func TestParseNodeIPArgument(t *testing.T) {
 
 func TestParseNodeIPAnnotation(t *testing.T) {
 	testCases := []struct {
-		desc string
+		desc  string
-		in   string
+		in    string
-		out  net.IP
+		out   []net.IP
-		err  string
+		err   string
+		ssErr string
 	}{
 		{
 			desc: "empty --node-ip",
@@ -237,7 +238,9 @@ func TestParseNodeIPAnnotation(t *testing.T) {
 		{
 			desc: "single IPv4",
 			in:   "1.2.3.4",
-			out:  netutils.ParseIPSloppy("1.2.3.4"),
+			out: []net.IP{
+				netutils.ParseIPSloppy("1.2.3.4"),
+			},
 		},
 		{
 			desc: "single IPv4 with whitespace",
@@ -247,7 +250,9 @@ func TestParseNodeIPAnnotation(t *testing.T) {
 		{
 			desc: "single IPv4 non-canonical",
 			in:   "01.2.3.004",
-			out:  netutils.ParseIPSloppy("1.2.3.4"),
+			out: []net.IP{
+				netutils.ParseIPSloppy("1.2.3.4"),
+			},
 		},
 		{
 			desc: "single IPv4 invalid",
@@ -262,7 +267,9 @@ func TestParseNodeIPAnnotation(t *testing.T) {
 		{
 			desc: "single IPv4 unspecified",
 			in:   "0.0.0.0",
-			out:  net.IPv4zero,
+			out: []net.IP{
+				net.IPv4zero,
+			},
 		},
 		{
 			desc: "single IPv4 plus garbage",
@@ -272,17 +279,25 @@ func TestParseNodeIPAnnotation(t *testing.T) {
 		{
 			desc: "single IPv6",
 			in:   "abcd::ef01",
-			out:  netutils.ParseIPSloppy("abcd::ef01"),
+			out: []net.IP{
+				netutils.ParseIPSloppy("abcd::ef01"),
+			},
 		},
 		{
 			desc: "single IPv6 non-canonical",
 			in:   "abcd:0abc:00ab:0000:0000::1",
-			out:  netutils.ParseIPSloppy("abcd:abc:ab::1"),
+			out: []net.IP{
+				netutils.ParseIPSloppy("abcd:abc:ab::1"),
+			},
 		},
 		{
 			desc: "simple dual-stack",
 			in:   "1.2.3.4,abcd::ef01",
-			err:  "not supported in this configuration",
+			out: []net.IP{
+				netutils.ParseIPSloppy("1.2.3.4"),
+				netutils.ParseIPSloppy("abcd::ef01"),
+			},
+			ssErr: "not supported in this configuration",
 		},
 		{
 			desc: "dual-stack with whitespace",
@@ -300,14 +315,16 @@ func TestParseNodeIPAnnotation(t *testing.T) {
 			err:  "either a single IP or a dual-stack pair of IPs",
 		},
 		{
-			desc: "dual-stack with unspecified",
+			desc:  "dual-stack with unspecified",
-			in:   "1.2.3.4,::",
+			in:    "1.2.3.4,::",
-			err:  "not supported in this configuration",
+			err:   "cannot include '0.0.0.0' or '::'",
+			ssErr: "not supported in this configuration",
 		},
 		{
-			desc: "dual-stack with unspecified",
+			desc:  "dual-stack with unspecified",
-			in:   "0.0.0.0,abcd::1",
+			in:    "0.0.0.0,abcd::1",
-			err:  "not supported in this configuration",
+			err:   "cannot include '0.0.0.0' or '::'",
+			ssErr: "not supported in this configuration",
 		},
 		{
 			desc: "dual-stack plus garbage",
@@ -322,21 +339,36 @@ func TestParseNodeIPAnnotation(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
-		t.Run(tc.desc, func(t *testing.T) {
+		for _, allowDualStack := range []bool{false, true} {
-			parsed, err := ParseNodeIPAnnotation(tc.in)
+			desc := fmt.Sprintf("%s, allowDualStack=%v", tc.desc, allowDualStack)
+			t.Run(desc, func(t *testing.T) {
+				parsed, err := ParseNodeIPAnnotation(tc.in, allowDualStack)
 
-			if !reflect.DeepEqual(parsed, tc.out) {
+				expectedOut := tc.out
-				t.Errorf("expected %#v, got %#v", tc.out, parsed)
+				expectedErr := tc.err
-			}
+
-			if err != nil {
+				if !allowDualStack {
-				if tc.err == "" {
+					if len(tc.out) == 2 {
-					t.Errorf("unexpected error %v", err)
+						expectedOut = nil
-				} else if !strings.Contains(err.Error(), tc.err) {
+					}
-					t.Errorf("expected error with %q, got %v", tc.err, err)
+					if tc.ssErr != "" {
+						expectedErr = tc.ssErr
+					}
 				}
-			} else if tc.err != "" {
+
-				t.Errorf("expected error with %q, got no error", tc.err)
+				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)
+				}
+			})
+		}
 	}
 }