From 856c3398f7ea266732397605d686e15bb0c8343a Mon Sep 17 00:00:00 2001 From: R4mbo Date: Mon, 17 Aug 2026 13:35:21 +0530 Subject: [PATCH] fix nil pointer dereference in EnsureDeletePVC, EnsureDeletePV and EnsureDeletePod timeout paths (#10293) * fix nil pointer dereference in kube EnsureDelete timeout paths Signed-off-by: samay43 * add changelog entry Signed-off-by: samay43 --------- Signed-off-by: samay43 --- changelogs/unreleased/10293-samay43 | 1 + pkg/util/kube/pod.go | 6 ++++ pkg/util/kube/pod_test.go | 23 ++++++++++++ pkg/util/kube/pvc_pv.go | 12 +++++++ pkg/util/kube/pvc_pv_test.go | 55 +++++++++++++++++++++++++++++ 5 files changed, 97 insertions(+) create mode 100644 changelogs/unreleased/10293-samay43 diff --git a/changelogs/unreleased/10293-samay43 b/changelogs/unreleased/10293-samay43 new file mode 100644 index 000000000..eb5166687 --- /dev/null +++ b/changelogs/unreleased/10293-samay43 @@ -0,0 +1 @@ +Fix nil pointer dereference in EnsureDeletePVC, EnsureDeletePV and EnsureDeletePod when the API call times out before the object is retrieved diff --git a/pkg/util/kube/pod.go b/pkg/util/kube/pod.go index 3ced95feb..6342cbc1c 100644 --- a/pkg/util/kube/pod.go +++ b/pkg/util/kube/pod.go @@ -129,6 +129,12 @@ func EnsureDeletePod(ctx context.Context, podGetter corev1client.CoreV1Interface if err != nil { if errors.Is(err, context.DeadlineExceeded) { + // updated is only set once the pod has been retrieved successfully, so it + // is still nil when the deadline is exceeded before that happens, e.g. + // when the first Get times out. No finalizers are available to report. + if updated == nil { + return errors.Errorf("timeout to assure pod %s is deleted", pod) + } return errors.Errorf("timeout to assure pod %s is deleted, finalizers in pod %v", pod, updated.Finalizers) } else { return errors.Wrapf(err, "error to assure pod is deleted, %s", pod) diff --git a/pkg/util/kube/pod_test.go b/pkg/util/kube/pod_test.go index 6cb2c56ce..4f47ebd25 100644 --- a/pkg/util/kube/pod_test.go +++ b/pkg/util/kube/pod_test.go @@ -106,6 +106,29 @@ func TestEnsureDeletePod(t *testing.T) { }, err: "timeout to assure pod fake-pod is deleted, finalizers in pod []", }, + { + name: "wait timeout before the pod is ever retrieved", + podName: "fake-pod", + namespace: "fake-ns", + clientObj: []runtime.Object{podObjectWithFinalizer}, + reactors: []reactor{ + { + verb: "delete", + resource: "pods", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, nil + }, + }, + { + verb: "get", + resource: "pods", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, context.DeadlineExceeded + }, + }, + }, + err: "timeout to assure pod fake-pod is deleted", + }, { name: "wait fail", podName: "fake-pod", diff --git a/pkg/util/kube/pvc_pv.go b/pkg/util/kube/pvc_pv.go index 7db9df3e4..b375ce0ba 100644 --- a/pkg/util/kube/pvc_pv.go +++ b/pkg/util/kube/pvc_pv.go @@ -153,6 +153,12 @@ func EnsureDeletePVC(ctx context.Context, pvcGetter corev1client.CoreV1Interface if err != nil { if errors.Is(err, context.DeadlineExceeded) { + // updated is only set once the PVC has been retrieved successfully, so it + // is still nil when the deadline is exceeded before that happens, e.g. + // when the first Get times out. No finalizers are available to report. + if updated == nil { + return errors.Errorf("timeout to assure pvc %s is deleted", pvcName) + } return errors.Errorf("timeout to assure pvc %s is deleted, finalizers in pvc %v", pvcName, updated.Finalizers) } else { return errors.Wrapf(err, "error to ensure pvc deleted for %s", pvcName) @@ -189,6 +195,12 @@ func EnsureDeletePV(ctx context.Context, pvGetter corev1client.CoreV1Interface, if err != nil { if errors.Is(err, context.DeadlineExceeded) { + // updated is only set once the PV has been retrieved successfully, so it + // is still nil when the deadline is exceeded before that happens, e.g. + // when the first Get times out. No finalizers are available to report. + if updated == nil { + return errors.Errorf("timeout to assure pv %s is deleted", pvName) + } return errors.Errorf("timeout to assure pv %s is deleted, finalizers in pv %v", pvName, updated.Finalizers) } else { return errors.Wrapf(err, "error to ensure pv deleted for %s", pvName) diff --git a/pkg/util/kube/pvc_pv_test.go b/pkg/util/kube/pvc_pv_test.go index e3b8460bf..c805929d7 100644 --- a/pkg/util/kube/pvc_pv_test.go +++ b/pkg/util/kube/pvc_pv_test.go @@ -17,6 +17,7 @@ limitations under the License. package kube import ( + "context" "testing" "time" @@ -687,6 +688,30 @@ func TestEnsureDeletePVC(t *testing.T) { }, err: "timeout to assure pvc fake-pvc is deleted, finalizers in pvc []", }, + { + name: "wait timeout before the pvc is ever retrieved", + pvcName: "fake-pvc", + namespace: "fake-ns", + clientObj: []runtime.Object{pvcObjectWithFinalizer}, + timeout: time.Millisecond, + reactors: []reactor{ + { + verb: "delete", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, pvcObject, nil + }, + }, + { + verb: "get", + resource: "persistentvolumeclaims", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, context.DeadlineExceeded + }, + }, + }, + err: "timeout to assure pvc fake-pvc is deleted", + }, } for _, test := range tests { @@ -2059,6 +2084,13 @@ func TestEnsureDeletePV(t *testing.T) { }, } + pvObjWithFinalizer := &corev1api.PersistentVolume{ + ObjectMeta: metav1.ObjectMeta{ + Name: "fake-pv", + Finalizers: []string{"fake-finalizer-1", "fake-finalizer-2"}, + }, + } + tests := []struct { name string pvName string @@ -2134,6 +2166,29 @@ func TestEnsureDeletePV(t *testing.T) { }, expectedErr: "timeout to assure pv fake-pv is deleted, finalizers in pv []", }, + { + name: "wait timeout before the pv is ever retrieved", + pvName: "fake-pv", + timeout: time.Millisecond, + kubeClientObj: []runtime.Object{pvObjWithFinalizer}, + kubeReactors: []reactor{ + { + verb: "delete", + resource: "persistentvolumes", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, nil + }, + }, + { + verb: "get", + resource: "persistentvolumes", + reactorFunc: func(action clientTesting.Action) (handled bool, ret runtime.Object, err error) { + return true, nil, context.DeadlineExceeded + }, + }, + }, + expectedErr: "timeout to assure pv fake-pv is deleted", + }, } for _, test := range tests {