From 7605163620c9bd2f0e5fc6d525d13ba9aec3d657 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Sun, 5 Mar 2023 15:57:51 -0500 Subject: [PATCH] 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). --- pkg/kubelet/nodestatus/setters.go | 2 +- .../controllers/node/node_controller.go | 4 +- .../cloud-provider/node/helpers/address.go | 29 ++- .../node/helpers/address_test.go | 167 +++++++++++++++++- 4 files changed, 192 insertions(+), 10 deletions(-) diff --git a/pkg/kubelet/nodestatus/setters.go b/pkg/kubelet/nodestatus/setters.go index 687e8212403..e6460f14d9f 100644 --- 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 } diff --git a/staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go b/staging/src/k8s.io/cloud-provider/controllers/node/node_controller.go index e77b45e89d6..19b02b99c1b 100644 --- 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) } diff --git a/staging/src/k8s.io/cloud-provider/node/helpers/address.go b/staging/src/k8s.io/cloud-provider/node/helpers/address.go index 6eb44a90feb..533de163ac8 100644 --- 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 diff --git a/staging/src/k8s.io/cloud-provider/node/helpers/address_test.go b/staging/src/k8s.io/cloud-provider/node/helpers/address_test.go index eccb4a75b81..f5d2c5cad02 100644 --- 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) } }) }