From c7f78109a55fb3aa3b7844d0ebd26e3ac3fdc02b Mon Sep 17 00:00:00 2001 From: Qingchuan Hao Date: Wed, 20 Mar 2024 03:27:17 +0000 Subject: [PATCH] fix: handle comma-separated provided IPs --- pkg/nodemanager/nodemanager.go | 39 ++++--- pkg/nodemanager/nodemanager_test.go | 170 ++++++++++++++++++++++++++++ 2 files changed, 193 insertions(+), 16 deletions(-) diff --git a/pkg/nodemanager/nodemanager.go b/pkg/nodemanager/nodemanager.go index 55ef8c9b84..824a3ddf1d 100644 --- a/pkg/nodemanager/nodemanager.go +++ b/pkg/nodemanager/nodemanager.go @@ -302,10 +302,8 @@ func (cnc *CloudNodeController) updateNodeAddress(ctx context.Context, node *v1. } // If nodeIP was suggested by user, ensure that // it can be found in the cloud as well (consistent with the behaviour in kubelet) - if nodeIP, ok := ensureNodeProvidedIPExists(node, nodeAddresses); ok { - if nodeIP == nil { - return fmt.Errorf("specified Node IP %s not found in cloudprovider for node %q", nodeAddresses, node.Name) - } + if existNodeIPs, ok := ensureNodeProvidedIPsExists(node, nodeAddresses); !ok { + return fmt.Errorf("not all specified Node IP %s found in cloudprovider for node %q, existing Node IPs are %s ", nodeAddresses, node.Name, existNodeIPs) } if !nodeAddressesChangeDetected(node.Status.Addresses, nodeAddresses) { return nil @@ -468,10 +466,8 @@ func (cnc *CloudNodeController) getNodeModifiersFromCloudProvider(ctx context.Co // If user provided an IP address, ensure that IP address is found // in the cloud provider before removing the taint on the node - if nodeIP, ok := ensureNodeProvidedIPExists(node, nodeAddresses); ok { - if nodeIP == nil { - return nil, errors.New("failed to find kubelet node IP from cloud provider") - } + if _, ok := ensureNodeProvidedIPsExists(node, nodeAddresses); !ok { + return nil, errors.New("failed to find kubelet node IP from cloud provider") } if instanceType, err := cnc.getInstanceTypeByName(ctx, node); err != nil { @@ -602,19 +598,30 @@ func nodeAddressesChangeDetected(addressSet1, addressSet2 []v1.NodeAddress) bool return false } -func ensureNodeProvidedIPExists(node *v1.Node, nodeAddresses []v1.NodeAddress) (*v1.NodeAddress, bool) { - var nodeIP *v1.NodeAddress - nodeIPExists := false - if providedIP, ok := node.ObjectMeta.Annotations[cloudproviderapi.AnnotationAlphaProvidedIPAddr]; ok { - nodeIPExists = true - for i := range nodeAddresses { +// Ensure all provided node ip addresses are found in the cloud provider, otherwise return false +// When there's no provided node ip addresses, it will return true. +func ensureNodeProvidedIPsExists(node *v1.Node, nodeAddresses []v1.NodeAddress) ([]v1.NodeAddress, bool) { + providedIPStr, ok := node.ObjectMeta.Annotations[cloudproviderapi.AnnotationAlphaProvidedIPAddr] + if !ok || len(providedIPStr) == 0 { + return []v1.NodeAddress{}, true + } + var existNodeIPs []v1.NodeAddress + nodeIPsExists := false + providedIPs := strings.Split(providedIPStr, ",") + for i := range nodeAddresses { + for _, providedIP := range providedIPs { + providedIP = strings.TrimSpace(providedIP) if nodeAddresses[i].Address == providedIP { - nodeIP = &nodeAddresses[i] + existNodeIPs = append(existNodeIPs, nodeAddresses[i]) break } } } - return nodeIP, nodeIPExists + if len(existNodeIPs) == len(providedIPs) { + nodeIPsExists = true + } + + return existNodeIPs, nodeIPsExists } func (cnc *CloudNodeController) getInstanceTypeByName(ctx context.Context, node *v1.Node) (string, error) { diff --git a/pkg/nodemanager/nodemanager_test.go b/pkg/nodemanager/nodemanager_test.go index 84e3da0493..63caf8abca 100644 --- a/pkg/nodemanager/nodemanager_test.go +++ b/pkg/nodemanager/nodemanager_test.go @@ -1338,3 +1338,173 @@ func TestNodeProviderIDNotSet(t *testing.T) { // Node update should fail assert.Equal(t, 0, len(fnh.UpdatedNodes), "Node was updated (unexpected)") } + +func Test_ensureNodeProvidedIPsExists(t *testing.T) { + testcases := []struct { + name string + node *v1.Node + nodeAddresses []v1.NodeAddress + expectedExistingNodeIPs []v1.NodeAddress + nodeIPsExists bool + }{ + { + name: "return true when there's provide node ip address", + node: &v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node0", + Labels: map[string]string{}, + Annotations: map[string]string{}, + }, + }, + nodeAddresses: []v1.NodeAddress{ + { + Address: "10.0.0.1", + }, + }, + expectedExistingNodeIPs: []v1.NodeAddress{}, + nodeIPsExists: true, + }, + { + name: "return true when all provided IPv4 IP address are found", + node: &v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node0", + Labels: map[string]string{}, + Annotations: map[string]string{ + cloudproviderapi.AnnotationAlphaProvidedIPAddr: "10.0.0.1", + }, + }, + }, + nodeAddresses: []v1.NodeAddress{ + { + Address: "10.0.0.1", + }, + }, + expectedExistingNodeIPs: []v1.NodeAddress{{Address: "10.0.0.1"}}, + nodeIPsExists: true, + }, + { + name: "return true when all provided dual stack IP addresses are found", + node: &v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node0", + Labels: map[string]string{}, + Annotations: map[string]string{ + cloudproviderapi.AnnotationAlphaProvidedIPAddr: "10.0.0.1,fd47:c915:f8a8:e63d::5", + }, + }, + }, + nodeAddresses: []v1.NodeAddress{ + { + Address: "10.0.0.1", + }, + { + Address: "fd47:c915:f8a8:e63d::5", + }, + }, + expectedExistingNodeIPs: []v1.NodeAddress{ + { + Address: "10.0.0.1", + }, + { + Address: "fd47:c915:f8a8:e63d::5", + }, + }, + nodeIPsExists: true, + }, + { + name: "return true when all provided dual stack IP addresses are found but joined with extra space", + node: &v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node0", + Labels: map[string]string{}, + Annotations: map[string]string{ + cloudproviderapi.AnnotationAlphaProvidedIPAddr: "10.0.0.1, fd47:c915:f8a8:e63d::5", + }, + }, + }, + nodeAddresses: []v1.NodeAddress{ + { + Address: "10.0.0.1", + }, + { + Address: "fd47:c915:f8a8:e63d::5", + }, + }, + expectedExistingNodeIPs: []v1.NodeAddress{ + { + Address: "10.0.0.1", + }, + { + Address: "fd47:c915:f8a8:e63d::5", + }, + }, + nodeIPsExists: true, + }, + { + name: "return false when not all ip addresses are found for provided dual stack IP addresses", + node: &v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node0", + Labels: map[string]string{}, + Annotations: map[string]string{ + cloudproviderapi.AnnotationAlphaProvidedIPAddr: "10.0.0.1,fd47:c915:f8a8:e63d::5", + }, + }, + }, + nodeAddresses: []v1.NodeAddress{ + { + Address: "10.0.0.1", + }, + }, + expectedExistingNodeIPs: []v1.NodeAddress{ + { + Address: "10.0.0.1", + }, + }, + nodeIPsExists: false, + }, + { + name: "return false when wrong ip addresses are provided for provided dual stack IP addresses", + node: &v1.Node{ + ObjectMeta: metav1.ObjectMeta{ + Name: "node0", + Labels: map[string]string{}, + Annotations: map[string]string{ + cloudproviderapi.AnnotationAlphaProvidedIPAddr: "10.0.0.1,fd47:c915:f8a8:e63d::10", + }, + }, + }, + nodeAddresses: []v1.NodeAddress{ + { + Address: "10.0.0.1", + }, + { + Address: "fd47:c915:f8a8:e63d::5", + }, + }, + expectedExistingNodeIPs: []v1.NodeAddress{ + { + Address: "10.0.0.1", + }, + }, + nodeIPsExists: false, + }, + } + + for _, test := range testcases { + t.Run(test.name, func(t *testing.T) { + + actualNodeIP, actualNodeIPsExists := ensureNodeProvidedIPsExists(test.node, test.nodeAddresses) + + if !reflect.DeepEqual(actualNodeIP, test.expectedExistingNodeIPs) { + t.Logf("Actual existing node IPs: %v", actualNodeIP) + t.Logf("Expected existing node IPs: %v", test.expectedExistingNodeIPs) + t.Errorf("Actual existing node IP does not match expected existing node IP") + } + if actualNodeIPsExists != test.nodeIPsExists { + t.Errorf("all node ip addresses exist result mismatch, got: %t, wanted: %t", actualNodeIPsExists, test.nodeIPsExists) + } + }) + } +}