From 09efb948091985712e60b28f23695730eba4990f Mon Sep 17 00:00:00 2001
From: Baptiste Girard-Carrabin <baptiste.girardcarrabin@datadoghq.com>
Date: Thu, 8 Aug 2024 19:13:57 +0200
Subject: [PATCH] [node-cleanup] Keep the original get when checking for node

Only fallback ont the list call with the label if the node is not found in the get. This should be safer.
---
 pkg/common/common.go                          | 26 ++++++++++++-------
 pkg/common/common_test.go                     | 23 +++++++++++-----
 .../controller/controller_test.go             |  6 ++---
 pkg/node-cleanup/deleter/deleter_test.go      |  4 +--
 4 files changed, 38 insertions(+), 21 deletions(-)

diff --git a/pkg/common/common.go b/pkg/common/common.go
index 2efd3e734..56daf93b2 100644
--- a/pkg/common/common.go
+++ b/pkg/common/common.go
@@ -33,6 +33,7 @@ import (
 	"sigs.k8s.io/yaml"
 
 	v1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/resource"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
@@ -489,17 +490,22 @@ func GetVolumeMode(volUtil util.VolumeUtil, fullPath string) (v1.PersistentVolum
 }
 
 // NodeExists checks to see if a Node exists in the Indexer of a NodeLister.
-// It uses the well known label `kubernetes.io/hostname` to find the Node.
-func NodeExists(nodeLister corelisters.NodeLister, nodeLabelValue string) (bool, error) {
-	req, err := labels.NewRequirement(NodeLabelKey, selection.Equals, []string{nodeLabelValue})
-	if err != nil {
-		return false, err
-	}
-	nodes, err := nodeLister.List(labels.NewSelector().Add(*req))
-	if err != nil {
-		return false, err
+// It tries to get the node and if it fails, it uses the well known label
+// `kubernetes.io/hostname` to find the Node.
+func NodeExists(nodeLister corelisters.NodeLister, nodeName string) (bool, error) {
+	_, err := nodeLister.Get(nodeName)
+	if errors.IsNotFound(err) {
+		req, err := labels.NewRequirement(NodeLabelKey, selection.Equals, []string{nodeName})
+		if err != nil {
+			return false, err
+		}
+		nodes, err := nodeLister.List(labels.NewSelector().Add(*req))
+		if err != nil {
+			return false, err
+		}
+		return len(nodes) > 0, nil
 	}
-	return len(nodes) > 0, nil
+	return err == nil, err
 }
 
 // NodeAttachedToLocalPV gets the name of the Node that a local PV has a NodeAffinity to.
diff --git a/pkg/common/common_test.go b/pkg/common/common_test.go
index 4bf3295f2..14896b867 100644
--- a/pkg/common/common_test.go
+++ b/pkg/common/common_test.go
@@ -478,10 +478,16 @@ func TestGetVolumeMode(t *testing.T) {
 }
 
 func TestNodeExists(t *testing.T) {
-	node := &v1.Node{
+	nodeName := "test-node"
+	nodeWithName := &v1.Node{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: nodeName,
+		},
+	}
+	nodeWithLabel := &v1.Node{
 		ObjectMeta: metav1.ObjectMeta{
 			Labels: map[string]string{
-				NodeLabelKey: "test-node",
+				NodeLabelKey: nodeName,
 			},
 		},
 	}
@@ -493,12 +499,17 @@ func TestNodeExists(t *testing.T) {
 		expectedResult bool
 	}{
 		{
-			nodeAdded:      node,
-			nodeQueried:    node,
+			nodeAdded:      nodeWithName,
+			nodeQueried:    nodeWithName,
+			expectedResult: true,
+		},
+		{
+			nodeAdded:      nodeWithLabel,
+			nodeQueried:    nodeWithName,
 			expectedResult: true,
 		},
 		{
-			nodeQueried:    node,
+			nodeQueried:    nodeWithName,
 			expectedResult: false,
 		},
 	}
@@ -512,7 +523,7 @@ func TestNodeExists(t *testing.T) {
 			nodeInformer.Informer().GetStore().Add(test.nodeAdded)
 		}
 
-		exists, err := NodeExists(nodeInformer.Lister(), test.nodeQueried.Labels[NodeLabelKey])
+		exists, err := NodeExists(nodeInformer.Lister(), test.nodeQueried.Name)
 		if err != nil {
 			t.Errorf("Got unexpected error: %s", err.Error())
 		}
diff --git a/pkg/node-cleanup/controller/controller_test.go b/pkg/node-cleanup/controller/controller_test.go
index 257394959..4a2604e58 100644
--- a/pkg/node-cleanup/controller/controller_test.go
+++ b/pkg/node-cleanup/controller/controller_test.go
@@ -330,7 +330,7 @@ func pvWithPVCAndNode(pvc *v1.PersistentVolumeClaim, node *v1.Node) *v1.Persiste
 						{
 							Key:      common.NodeLabelKey,
 							Operator: v1.NodeSelectorOpIn,
-							Values:   []string{node.Labels[common.NodeLabelKey]},
+							Values:   []string{node.Name},
 						},
 					},
 				},
@@ -351,7 +351,7 @@ func pvWithNode(node *v1.Node) *v1.PersistentVolume {
 						{
 							Key:      common.NodeLabelKey,
 							Operator: v1.NodeSelectorOpIn,
-							Values:   []string{node.Labels[common.NodeLabelKey]},
+							Values:   []string{node.Name},
 						},
 					},
 				},
@@ -386,7 +386,7 @@ func pvcWithUID(uid string) *v1.PersistentVolumeClaim {
 func node() *v1.Node {
 	return &v1.Node{
 		ObjectMeta: metav1.ObjectMeta{
-			Labels: map[string]string{common.NodeLabelKey: defaultNodeName},
+			Name: defaultNodeName,
 		},
 	}
 }
diff --git a/pkg/node-cleanup/deleter/deleter_test.go b/pkg/node-cleanup/deleter/deleter_test.go
index e3733b47c..754cfec02 100644
--- a/pkg/node-cleanup/deleter/deleter_test.go
+++ b/pkg/node-cleanup/deleter/deleter_test.go
@@ -211,7 +211,7 @@ func localPV(node *v1.Node, phase v1.PersistentVolumePhase, reclaimPolicy v1.Per
 								{
 									Key:      common.NodeLabelKey,
 									Operator: v1.NodeSelectorOpIn,
-									Values:   []string{node.Labels[common.NodeLabelKey]},
+									Values:   []string{node.Name},
 								},
 							},
 						},
@@ -230,7 +230,7 @@ func localPV(node *v1.Node, phase v1.PersistentVolumePhase, reclaimPolicy v1.Per
 func node() *v1.Node {
 	return &v1.Node{
 		ObjectMeta: metav1.ObjectMeta{
-			Labels: map[string]string{common.NodeLabelKey: testNodeName},
+			Name: testNodeName,
 		},
 	}
 }