From 4aecb151b8301a6fc69f63c08f640c7b2b4289fc Mon Sep 17 00:00:00 2001
From: Brian Pursley <bpursley@cinlogic.com>
Date: Thu, 4 Jan 2024 08:53:57 -0500
Subject: [PATCH] Fix kubectl drain error handling bug.

Fixed a bug where kubectl drain would consider a pod as having been deleted if an error occurs while calling the API.
---
 staging/src/k8s.io/kubectl/pkg/drain/drain.go |  4 ++-
 .../k8s.io/kubectl/pkg/drain/drain_test.go    | 31 ++++++++++++++-----
 2 files changed, 27 insertions(+), 8 deletions(-)

diff --git a/staging/src/k8s.io/kubectl/pkg/drain/drain.go b/staging/src/k8s.io/kubectl/pkg/drain/drain.go
index 1f6502eda6d..c180754183e 100644
--- a/staging/src/k8s.io/kubectl/pkg/drain/drain.go
+++ b/staging/src/k8s.io/kubectl/pkg/drain/drain.go
@@ -417,7 +417,9 @@ func waitForDelete(params waitForDeleteParams) ([]corev1.Pod, error) {
 		pendingPods := []corev1.Pod{}
 		for i, pod := range pods {
 			p, err := params.getPodFn(pod.Namespace, pod.Name)
-			if apierrors.IsNotFound(err) || (p != nil && p.ObjectMeta.UID != pod.ObjectMeta.UID) {
+			// The implementation of getPodFn that uses client-go returns an empty Pod struct when there is an error,
+			// so we need to check that err == nil and p != nil to know that a pod was found successfully.
+			if apierrors.IsNotFound(err) || (err == nil && p != nil && p.ObjectMeta.UID != pod.ObjectMeta.UID) {
 				if params.onFinishFn != nil {
 					params.onFinishFn(&pod, params.usingEviction, nil)
 				} else if params.onDoneFn != nil {
diff --git a/staging/src/k8s.io/kubectl/pkg/drain/drain_test.go b/staging/src/k8s.io/kubectl/pkg/drain/drain_test.go
index c3cc84869bc..6ca4fe6dfac 100644
--- a/staging/src/k8s.io/kubectl/pkg/drain/drain_test.go
+++ b/staging/src/k8s.io/kubectl/pkg/drain/drain_test.go
@@ -72,10 +72,27 @@ func TestDeletePods(t *testing.T) {
 						newPod := newPodMap[name]
 						return &newPod, nil
 					}
-					return nil, apierrors.NewNotFound(schema.GroupResource{Resource: "pods"}, name)
-
+					return &corev1.Pod{}, apierrors.NewNotFound(schema.GroupResource{Resource: "pods"}, name)
 				}
-				return nil, apierrors.NewNotFound(schema.GroupResource{Resource: "pods"}, name)
+				return &corev1.Pod{}, apierrors.NewNotFound(schema.GroupResource{Resource: "pods"}, name)
+			},
+		},
+		{
+			description:       "Pod found with same name but different UID",
+			interval:          100 * time.Millisecond,
+			timeout:           10 * time.Second,
+			expectPendingPods: false,
+			expectError:       false,
+			expectedError:     nil,
+			getPodFn: func(namespace, name string) (*corev1.Pod, error) {
+				// Return a pod with the same name, but different UID
+				return &corev1.Pod{
+					ObjectMeta: metav1.ObjectMeta{
+						Namespace: namespace,
+						Name:      name,
+						UID:       "SOME_OTHER_UID",
+					},
+				}, nil
 			},
 		},
 		{
@@ -90,7 +107,7 @@ func TestDeletePods(t *testing.T) {
 				if oldPod, found := oldPodMap[name]; found {
 					return &oldPod, nil
 				}
-				return nil, fmt.Errorf("%q: not found", name)
+				return &corev1.Pod{}, fmt.Errorf("%q: not found", name)
 			},
 		},
 		{
@@ -106,7 +123,7 @@ func TestDeletePods(t *testing.T) {
 				if oldPod, found := oldPodMap[name]; found {
 					return &oldPod, nil
 				}
-				return nil, fmt.Errorf("%q: not found", name)
+				return &corev1.Pod{}, fmt.Errorf("%q: not found", name)
 			},
 		},
 		{
@@ -123,7 +140,7 @@ func TestDeletePods(t *testing.T) {
 					oldPod.ObjectMeta.SetDeletionTimestamp(dTime)
 					return &oldPod, nil
 				}
-				return nil, fmt.Errorf("%q: not found", name)
+				return &corev1.Pod{}, fmt.Errorf("%q: not found", name)
 			},
 		},
 		{
@@ -134,7 +151,7 @@ func TestDeletePods(t *testing.T) {
 			expectError:       true,
 			expectedError:     nil,
 			getPodFn: func(namespace, name string) (*corev1.Pod, error) {
-				return nil, errors.New("This is a random error for testing")
+				return &corev1.Pod{}, errors.New("This is a random error for testing")
 			},
 		},
 	}