Split up PreferNodeIP into legacy and non-legacy versions

Though not obvious as currently written, PreferNodeIP() has different semantics with legacy and external cloud providers, since one kind of node IP value never gets passed in the external cloud provider case. Split it into two functions to make this clearer (and to prepare for adding new external-cloud-only semantics, and to make it clearer that some of the code can be deleted when legacy cloud providers go away).
2025-07-19 09:52:49 +00:00 · 2023-03-05 15:57:51 -05:00 · 2023-03-05 15:57:51 -05:00 · 7605163620
commit 7605163620
parent 77e0fbe774
4 changed files with 192 additions and 10 deletions
--- a/pkg/kubelet/nodestatus/setters.go
+++ b/pkg/kubelet/nodestatus/setters.go
@ -131,7 +131,7 @@ func NodeAddress(nodeIPs []net.IP, // typically Kubelet.nodeIPs
 				return err
 			}

-			nodeAddresses, err := cloudprovidernodeutil.PreferNodeIP(nodeIP, cloudNodeAddresses)
+			nodeAddresses, err := cloudprovidernodeutil.GetNodeAddressesFromNodeIPLegacy(nodeIP, cloudNodeAddresses)
 			if err != nil {
 				return err
 			}
--- a/staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go
+++ b/staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go
@ -391,7 +391,7 @@ func (cnc *CloudNodeController) updateNodeAddress(ctx context.Context, node *v1.
 	}

 	if nodeIP != nil {
-		nodeAddresses, err = cloudnodeutil.PreferNodeIP(nodeIP, nodeAddresses)
+		nodeAddresses, err = cloudnodeutil.GetNodeAddressesFromNodeIP(nodeIP, nodeAddresses)
 		if err != nil {
 			klog.Errorf("Failed to update node addresses for node %q: %v", node.Name, err)
 			return
@ -539,7 +539,7 @@ func (cnc *CloudNodeController) getNodeModifiersFromCloudProvider(
 	}

 	if nodeIP != nil {
-		_, err := cloudnodeutil.PreferNodeIP(nodeIP, instanceMeta.NodeAddresses)
+		_, err := cloudnodeutil.GetNodeAddressesFromNodeIP(nodeIP, instanceMeta.NodeAddresses)
 		if err != nil {
 			return nil, fmt.Errorf("provided node ip for node %q is not valid: %w", node.Name, err)
 		}
--- a/staging/src/k8s.io/cloud-provider/node/helpers/address.go
+++ b/staging/src/k8s.io/cloud-provider/node/helpers/address.go
@ -41,8 +41,8 @@ func AddToNodeAddresses(addresses *[]v1.NodeAddress, addAddresses ...v1.NodeAddr
 	}
 }

-// PreferNodeIP filters node addresses to prefer a specific node IP or address
-// family.
+// GetNodeAddressesFromNodeIPLegacy filters node addresses to prefer a specific node IP or
+// address family. This function is used only with legacy cloud providers.
 //
 // If nodeIP is either '0.0.0.0' or '::' it is taken to represent any address of
 // that address family: IPv4 or IPv6. i.e. if nodeIP is '0.0.0.0' we will return
@ -55,7 +55,7 @@ func AddToNodeAddresses(addresses *[]v1.NodeAddress, addAddresses ...v1.NodeAddr
 //   - If nodeIP matches an address of a particular type (internal or external),
 //     that will be the *only* address of that type returned.
 //   - All remaining addresses are listed after.
-func PreferNodeIP(nodeIP net.IP, cloudNodeAddresses []v1.NodeAddress) ([]v1.NodeAddress, error) {
+func GetNodeAddressesFromNodeIPLegacy(nodeIP net.IP, cloudNodeAddresses []v1.NodeAddress) ([]v1.NodeAddress, error) {
 	// If nodeIP is unset, just use the addresses provided by the cloud provider as-is
 	if nodeIP == nil {
 		return cloudNodeAddresses, nil
@ -83,6 +83,29 @@ func PreferNodeIP(nodeIP net.IP, cloudNodeAddresses []v1.NodeAddress) ([]v1.Node
 		return sortedAddresses, nil
 	}

+	// Otherwise the result is the same as for GetNodeAddressesFromNodeIP
+	return GetNodeAddressesFromNodeIP(nodeIP, cloudNodeAddresses)
+}
+
+// GetNodeAddressesFromNodeIP filters the provided list of nodeAddresses to
+// match the provided nodeIP. This is used for external cloud providers.
+//
+// It will return node addresses filtered such that:
+//   - Any address matching nodeIP will be listed first.
+//   - If nodeIP matches an address of a particular type (internal or external),
+//     that will be the *only* address of that type returned.
+//   - All remaining addresses are listed after.
+//
+// (This does not have the same behavior with `0.0.0.0` and `::` as
+// 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(nodeIP net.IP, cloudNodeAddresses []v1.NodeAddress) ([]v1.NodeAddress, error) {
+	// If nodeIP is unset, just use the addresses provided by the cloud provider as-is
+	if nodeIP == nil {
+		return cloudNodeAddresses, nil
+	}
+
 	// 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
--- a/staging/src/k8s.io/cloud-provider/node/helpers/address_test.go
+++ b/staging/src/k8s.io/cloud-provider/node/helpers/address_test.go
@ -94,7 +94,7 @@ func TestAddToNodeAddresses(t *testing.T) {
 	}
 }

-func TestPreferNodeIP(t *testing.T) {
+func TestGetNodeAddressesFromNodeIPLegacy(t *testing.T) {
 	cases := []struct {
 		name              string
 		nodeIP            net.IP
@ -301,13 +301,172 @@ func TestPreferNodeIP(t *testing.T) {

 	for _, tt := range cases {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := PreferNodeIP(tt.nodeIP, tt.nodeAddresses)
+			got, err := GetNodeAddressesFromNodeIPLegacy(tt.nodeIP, tt.nodeAddresses)
 			if (err != nil) != tt.shouldError {
-				t.Errorf("PreferNodeIP() error = %v, wantErr %v", err, tt.shouldError)
+				t.Errorf("GetNodeAddressesFromNodeIPLegacy() error = %v, wantErr %v", err, tt.shouldError)
 				return
 			}
 			if !reflect.DeepEqual(got, tt.expectedAddresses) {
-				t.Errorf("PreferNodeIP() = %v, want %v", got, tt.expectedAddresses)
+				t.Errorf("GetNodeAddressesFromNodeIPLegacy() = %v, want %v", got, tt.expectedAddresses)
+			}
+		})
+	}
+}
+
+func TestGetNodeAddressesFromNodeIP(t *testing.T) {
+	cases := []struct {
+		name              string
+		nodeIP            net.IP
+		nodeAddresses     []v1.NodeAddress
+		expectedAddresses []v1.NodeAddress
+		shouldError       bool
+	}{
+		{
+			name:   "A single InternalIP",
+			nodeIP: netutils.ParseIPSloppy("10.1.1.1"),
+			nodeAddresses: []v1.NodeAddress{
+				{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.NodeHostName, Address: testKubeletHostname},
+			},
+			shouldError: false,
+		},
+		{
+			name:   "NodeIP is external",
+			nodeIP: netutils.ParseIPSloppy("55.55.55.55"),
+			nodeAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			expectedAddresses: []v1.NodeAddress{
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			shouldError: false,
+		},
+		{
+			// Accommodating #45201 and #49202
+			name:   "InternalIP and ExternalIP are the same",
+			nodeIP: netutils.ParseIPSloppy("55.55.55.55"),
+			nodeAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "44.44.44.44"},
+				{Type: v1.NodeExternalIP, Address: "44.44.44.44"},
+				{Type: v1.NodeInternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			expectedAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			shouldError: false,
+		},
+		{
+			name:   "An Internal/ExternalIP, an Internal/ExternalDNS",
+			nodeIP: netutils.ParseIPSloppy("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.NodeInternalDNS, Address: "ip-10-1-1-1.us-west-2.compute.internal"},
+				{Type: v1.NodeExternalDNS, Address: "ec2-55-55-55-55.us-west-2.compute.amazonaws.com"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			expectedAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeInternalDNS, Address: "ip-10-1-1-1.us-west-2.compute.internal"},
+				{Type: v1.NodeExternalDNS, Address: "ec2-55-55-55-55.us-west-2.compute.amazonaws.com"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			shouldError: false,
+		},
+		{
+			name:   "An Internal with multiple internal IPs",
+			nodeIP: netutils.ParseIPSloppy("10.1.1.1"),
+			nodeAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeInternalIP, Address: "10.2.2.2"},
+				{Type: v1.NodeInternalIP, Address: "10.3.3.3"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			expectedAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			shouldError: false,
+		},
+		{
+			name:   "An InternalIP that isn't valid: should error",
+			nodeIP: netutils.ParseIPSloppy("10.2.2.2"),
+			nodeAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeExternalIP, Address: "55.55.55.55"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			expectedAddresses: nil,
+			shouldError:       true,
+		},
+		{
+			name:   "Dual-stack cloud, with nodeIP, different IPv6 formats",
+			nodeIP: netutils.ParseIPSloppy("2600:1f14:1d4:d101::ba3d"),
+			nodeAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{Type: v1.NodeInternalIP, Address: "2600:1f14:1d4:d101:0:0:0:ba3d"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			expectedAddresses: []v1.NodeAddress{
+				{Type: v1.NodeInternalIP, Address: "2600:1f14:1d4:d101:0:0:0:ba3d"},
+				{Type: v1.NodeHostName, Address: testKubeletHostname},
+			},
+			shouldError: false,
+		},
+		{
+			name: "Dual-stack cloud, IPv4 first, no nodeIP",
+			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, no nodeIP",
+			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: "fc01:1234::5678"},
+				{Type: v1.NodeInternalIP, Address: "10.1.1.1"},
+				{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)
+			if (err != nil) != tt.shouldError {
+				t.Errorf("GetNodeAddressesFromNodeIP() error = %v, wantErr %v", err, tt.shouldError)
+				return
+			}
+			if !reflect.DeepEqual(got, tt.expectedAddresses) {
+				t.Errorf("GetNodeAddressesFromNodeIP() = %v, want %v", got, tt.expectedAddresses)
 			}
 		})
 	}