From d6a11b71386832efb8d7315ac347b87eb258b771 Mon Sep 17 00:00:00 2001 From: Dan Winship Date: Mon, 6 Mar 2023 09:02:19 -0500 Subject: [PATCH] Update GetNodeAddressesFromNodeIP to take the unparsed annotation And simplify the callers in node_controller.go to merge the common code. --- .../controllers/node/node_controller.go | 40 +++++------------ .../cloud-provider/node/helpers/address.go | 16 ++++--- .../node/helpers/address_test.go | 44 ++++--------------- .../src/k8s.io/legacy-cloud-providers/go.mod | 1 + 4 files changed, 29 insertions(+), 72 deletions(-) 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 19b02b99c1b..c9aea6ff50c 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 @@ -20,7 +20,6 @@ import ( "context" "errors" "fmt" - "net" "time" v1 "k8s.io/api/core/v1" @@ -384,19 +383,12 @@ func (cnc *CloudNodeController) updateNodeAddress(ctx context.Context, node *v1. } } // If kubelet provided a node IP, prefer it in the node address list - nodeIP, err := getNodeProvidedIP(node) + nodeAddresses, err := updateNodeAddressesFromNodeIP(node, nodeAddresses) if err != nil { - klog.Errorf("Failed to get preferred node IP for node %q: %v", node.Name, err) + klog.Errorf("Failed to update node addresses for node %q: %v", node.Name, err) return } - if nodeIP != nil { - nodeAddresses, err = cloudnodeutil.GetNodeAddressesFromNodeIP(nodeIP, nodeAddresses) - if err != nil { - klog.Errorf("Failed to update node addresses for node %q: %v", node.Name, err) - return - } - } if !nodeAddressesChangeDetected(node.Status.Addresses, nodeAddresses) { return } @@ -533,16 +525,9 @@ func (cnc *CloudNodeController) getNodeModifiersFromCloudProvider( // If kubelet annotated the node with a node IP, ensure that it is valid // and can be applied to the discovered node addresses before removing // the taint on the node. - nodeIP, err := getNodeProvidedIP(node) + _, err := updateNodeAddressesFromNodeIP(node, instanceMeta.NodeAddresses) if err != nil { - return nil, err - } - - if nodeIP != nil { - _, 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) - } + return nil, fmt.Errorf("provided node ip for node %q is not valid: %w", node.Name, err) } if instanceMeta.InstanceType != "" { @@ -749,18 +734,15 @@ func nodeAddressesChangeDetected(addressSet1, addressSet2 []v1.NodeAddress) bool return false } -func getNodeProvidedIP(node *v1.Node) (net.IP, error) { - providedIP, ok := node.ObjectMeta.Annotations[cloudproviderapi.AnnotationAlphaProvidedIPAddr] - if !ok { - return nil, nil +func updateNodeAddressesFromNodeIP(node *v1.Node, nodeAddresses []v1.NodeAddress) ([]v1.NodeAddress, error) { + var err error + + providedNodeIP, exists := node.ObjectMeta.Annotations[cloudproviderapi.AnnotationAlphaProvidedIPAddr] + if exists { + nodeAddresses, err = cloudnodeutil.GetNodeAddressesFromNodeIP(providedNodeIP, nodeAddresses) } - nodeIP, err := nodeutil.ParseNodeIPAnnotation(providedIP) - if err != nil { - return nil, fmt.Errorf("failed to parse node IP %q for node %q: %v", providedIP, node.Name, err) - } - - return nodeIP, nil + return nodeAddresses, err } // getInstanceTypeByProviderIDOrName will attempt to get the instance type of node using its providerID 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 533de163ac8..627167799aa 100644 --- a/staging/src/k8s.io/cloud-provider/node/helpers/address.go +++ b/staging/src/k8s.io/cloud-provider/node/helpers/address.go @@ -21,6 +21,7 @@ import ( "net" "k8s.io/api/core/v1" + nodeutil "k8s.io/component-helpers/node/util" netutils "k8s.io/utils/net" ) @@ -84,11 +85,12 @@ func GetNodeAddressesFromNodeIPLegacy(nodeIP net.IP, cloudNodeAddresses []v1.Nod } // Otherwise the result is the same as for GetNodeAddressesFromNodeIP - return GetNodeAddressesFromNodeIP(nodeIP, cloudNodeAddresses) + return GetNodeAddressesFromNodeIP(nodeIP.String(), cloudNodeAddresses) } -// GetNodeAddressesFromNodeIP filters the provided list of nodeAddresses to -// match the provided nodeIP. This is used for external cloud providers. +// GetNodeAddressesFromNodeIP filters the provided list of nodeAddresses to match the +// providedNodeIP from the Node annotation (which is assumed to be non-empty). This is +// used for external cloud providers. // // It will return node addresses filtered such that: // - Any address matching nodeIP will be listed first. @@ -100,10 +102,10 @@ 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(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 +func GetNodeAddressesFromNodeIP(providedNodeIP string, cloudNodeAddresses []v1.NodeAddress) ([]v1.NodeAddress, error) { + nodeIP, err := nodeutil.ParseNodeIPAnnotation(providedNodeIP) + 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 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 f5d2c5cad02..6007bedf206 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 @@ -316,14 +316,14 @@ func TestGetNodeAddressesFromNodeIPLegacy(t *testing.T) { func TestGetNodeAddressesFromNodeIP(t *testing.T) { cases := []struct { name string - nodeIP net.IP + nodeIP string nodeAddresses []v1.NodeAddress expectedAddresses []v1.NodeAddress shouldError bool }{ { name: "A single InternalIP", - nodeIP: netutils.ParseIPSloppy("10.1.1.1"), + nodeIP: "10.1.1.1", nodeAddresses: []v1.NodeAddress{ {Type: v1.NodeInternalIP, Address: "10.1.1.1"}, {Type: v1.NodeHostName, Address: testKubeletHostname}, @@ -336,7 +336,7 @@ func TestGetNodeAddressesFromNodeIP(t *testing.T) { }, { name: "NodeIP is external", - nodeIP: netutils.ParseIPSloppy("55.55.55.55"), + nodeIP: "55.55.55.55", nodeAddresses: []v1.NodeAddress{ {Type: v1.NodeInternalIP, Address: "10.1.1.1"}, {Type: v1.NodeExternalIP, Address: "55.55.55.55"}, @@ -352,7 +352,7 @@ func TestGetNodeAddressesFromNodeIP(t *testing.T) { { // Accommodating #45201 and #49202 name: "InternalIP and ExternalIP are the same", - nodeIP: netutils.ParseIPSloppy("55.55.55.55"), + nodeIP: "55.55.55.55", nodeAddresses: []v1.NodeAddress{ {Type: v1.NodeInternalIP, Address: "44.44.44.44"}, {Type: v1.NodeExternalIP, Address: "44.44.44.44"}, @@ -369,7 +369,7 @@ func TestGetNodeAddressesFromNodeIP(t *testing.T) { }, { name: "An Internal/ExternalIP, an Internal/ExternalDNS", - nodeIP: netutils.ParseIPSloppy("10.1.1.1"), + nodeIP: "10.1.1.1", nodeAddresses: []v1.NodeAddress{ {Type: v1.NodeInternalIP, Address: "10.1.1.1"}, {Type: v1.NodeExternalIP, Address: "55.55.55.55"}, @@ -388,7 +388,7 @@ func TestGetNodeAddressesFromNodeIP(t *testing.T) { }, { name: "An Internal with multiple internal IPs", - nodeIP: netutils.ParseIPSloppy("10.1.1.1"), + nodeIP: "10.1.1.1", nodeAddresses: []v1.NodeAddress{ {Type: v1.NodeInternalIP, Address: "10.1.1.1"}, {Type: v1.NodeInternalIP, Address: "10.2.2.2"}, @@ -405,7 +405,7 @@ func TestGetNodeAddressesFromNodeIP(t *testing.T) { }, { name: "An InternalIP that isn't valid: should error", - nodeIP: netutils.ParseIPSloppy("10.2.2.2"), + nodeIP: "10.2.2.2", nodeAddresses: []v1.NodeAddress{ {Type: v1.NodeInternalIP, Address: "10.1.1.1"}, {Type: v1.NodeExternalIP, Address: "55.55.55.55"}, @@ -416,7 +416,7 @@ func TestGetNodeAddressesFromNodeIP(t *testing.T) { }, { name: "Dual-stack cloud, with nodeIP, different IPv6 formats", - nodeIP: netutils.ParseIPSloppy("2600:1f14:1d4:d101::ba3d"), + nodeIP: "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"}, @@ -428,34 +428,6 @@ func TestGetNodeAddressesFromNodeIP(t *testing.T) { }, 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 { diff --git a/staging/src/k8s.io/legacy-cloud-providers/go.mod b/staging/src/k8s.io/legacy-cloud-providers/go.mod index 023c21c920c..5db30abca9a 100644 --- a/staging/src/k8s.io/legacy-cloud-providers/go.mod +++ b/staging/src/k8s.io/legacy-cloud-providers/go.mod @@ -85,6 +85,7 @@ require ( gopkg.in/warnings.v0 v0.1.1 // indirect gopkg.in/yaml.v2 v2.4.0 // indirect gopkg.in/yaml.v3 v3.0.1 // indirect + k8s.io/component-helpers v0.0.0 // indirect k8s.io/kube-openapi v0.0.0-20230308215209-15aac26d736a // indirect sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect sigs.k8s.io/structured-merge-diff/v4 v4.2.3 // indirect