From cdb6139c77d7efe9e37575cb6f4e49c630a0684c Mon Sep 17 00:00:00 2001 From: Simon Lauger Date: Sun, 6 Sep 2026 10:53:56 +0200 Subject: [PATCH] fix(controller): resolve owners by controller reference and sort Jobs Three correctness problems in the path that decides which workload to act on, all of which could make the operator scale down the wrong thing. resolveOwnerWorkload read OwnerReferences[0]. At most one reference has controller: true and the ordering is not defined, so an object carrying an extra non-controller owner could resolve to the wrong parent or to nothing. Uses metav1.GetControllerOf now, at all three levels. The owner UID was never compared. A ReplicaSet or Job deleted and recreated under the same name would be acted on despite not owning the pod. A mismatch is now treated as no owner. allReplicasFailing took the newest Job of a CronJob as the last element of an unsorted List. That only worked while the unix-minute suffixes had a stable digit count. Sorted by creation timestamp instead. The test fixtures set neither controller: true nor matching UIDs, which real Kubernetes always does, so they had to become realistic before the fixes could pass. Adds cases for a non-controller reference listed first, a stale reference, and Jobs returned out of order. Closes #65 Signed-off-by: Simon Lauger --- .../crashlooppolicy_controller_test.go | 93 +++++++++++++++++-- internal/controller/helpers.go | 30 ++++-- internal/controller/testutil_test.go | 29 ++++-- 3 files changed, 133 insertions(+), 19 deletions(-) diff --git a/internal/controller/crashlooppolicy_controller_test.go b/internal/controller/crashlooppolicy_controller_test.go index 1edbfa4..37752cd 100644 --- a/internal/controller/crashlooppolicy_controller_test.go +++ b/internal/controller/crashlooppolicy_controller_test.go @@ -148,8 +148,8 @@ func TestReconcile_ScalesDownStatefulSet(t *testing.T) { func TestReconcile_SuspendsCronJob(t *testing.T) { policy := newCrashLoopPolicy("test-policy", withAllReplicasFailing(false)) - cj := newCronJob("my-cj", testNamespace) - job := newJob("my-cj-job", testNamespace, "my-cj") + cj := newCronJob() + job := newJob("my-cj-job") pod := newFailingPod("my-cj-job-pod", testNamespace, jobOwnerRef(), "CreateContainerConfigError", 5) c := setupTestClient(policy, cj, job, pod) @@ -377,6 +377,7 @@ func TestReconcile_NamespaceSelectorFilters(t *testing.T) { Kind: "ReplicaSet", Name: "dev-app-rs", UID: "rs-uid-1", + Controller: new(true), }, "CrashLoopBackOff", 15) // Deployment in prod namespace (should NOT be scaled down) @@ -389,6 +390,7 @@ func TestReconcile_NamespaceSelectorFilters(t *testing.T) { Kind: "ReplicaSet", Name: "prod-app-rs", UID: "rs-uid-1", + Controller: new(true), }, "CrashLoopBackOff", 15) c := setupTestClient(policy, devNs, prodNs, devDeploy, devRs, devPod, prodDeploy, prodRs, prodPod) @@ -486,8 +488,8 @@ func TestReconcile_CronJobAllReplicasFailing(t *testing.T) { // With allReplicasFailing=true, a CronJob should only be suspended // if the pods of its latest job are actually failing. policy := newCrashLoopPolicy("test-policy", withAllReplicasFailing(true)) - cj := newCronJob("my-cj", testNamespace) - job := newJob("my-cj-job", testNamespace, "my-cj") + cj := newCronJob() + job := newJob("my-cj-job") failingPod := newFailingPod("my-cj-pod-1", testNamespace, jobOwnerRef(), "CrashLoopBackOff", 15) c := setupTestClient(policy, cj, job, failingPod) @@ -511,8 +513,8 @@ func TestReconcile_CronJobNotAllReplicasFailing(t *testing.T) { // With allReplicasFailing=true, a CronJob should NOT be suspended // when some job pods are healthy. policy := newCrashLoopPolicy("test-policy", withAllReplicasFailing(true)) - cj := newCronJob("my-cj", testNamespace) - job := newJob("my-cj-job", testNamespace, "my-cj") + cj := newCronJob() + job := newJob("my-cj-job") failingPod := newFailingPod("my-cj-pod-1", testNamespace, jobOwnerRef(), "CrashLoopBackOff", 15) healthyPod := newHealthyPod("my-cj-pod-2", jobOwnerRef()) @@ -1146,3 +1148,82 @@ func TestReconcile_ReadyFalseOnPartialFailure(t *testing.T) { t.Errorf("expected reason ReconcilePartiallyFailed, got %s", ready.Reason) } } + +func TestResolveOwnerWorkload_IgnoresNonControllerReference(t *testing.T) { + // A pod may carry owner references besides its controller, and their order + // is not defined. Resolving from the first entry picks the wrong parent. + deploy := newDeployment("my-app", testNamespace, 3) + rs := newReplicaSet("my-app-rs", testNamespace, "my-app") + pod := newFailingPod("my-app-pod", testNamespace, rsOwnerRef(), "CrashLoopBackOff", 5) + pod.OwnerReferences = append([]metav1.OwnerReference{{ + APIVersion: "v1", + Kind: "ConfigMap", + Name: "some-annotation-owner", + UID: "cm-uid-1", + }}, pod.OwnerReferences...) + + c := setupTestClient(deploy, rs, pod) + owner, err := resolveOwnerWorkload(testCtx(), c, pod) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if owner == nil { + t.Fatal("expected the controller reference to be found past the non-controller one") + } + if owner.Kind != "Deployment" || owner.Name != "my-app" { + t.Errorf("expected Deployment/my-app, got %s/%s", owner.Kind, owner.Name) + } +} + +func TestResolveOwnerWorkload_RejectsStaleReference(t *testing.T) { + // The ReplicaSet was deleted and recreated under the same name. Acting on + // it would mean scaling down a workload that does not own this pod. + deploy := newDeployment("my-app", testNamespace, 3) + rs := newReplicaSet("my-app-rs", testNamespace, "my-app") + rs.UID = "rs-uid-recreated" + pod := newFailingPod("my-app-pod", testNamespace, rsOwnerRef(), "CrashLoopBackOff", 5) + + c := setupTestClient(deploy, rs, pod) + owner, err := resolveOwnerWorkload(testCtx(), c, pod) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if owner != nil { + t.Errorf("expected no owner for a stale reference, got %s/%s", owner.Kind, owner.Name) + } +} + +func TestAllReplicasFailing_PicksTheNewestJobRegardlessOfListOrder(t *testing.T) { + // List order is not defined. The newest Job must be chosen by creation + // time, not by position in the returned slice. + cj := newCronJob() + + oldJob := newJob("my-cj-100") + oldJob.UID = "job-uid-old" + oldJob.CreationTimestamp = metav1.NewTime(metav1.Now().Add(-2 * time.Hour)) + oldPod := newFailingPod("my-cj-100-pod", testNamespace, metav1.OwnerReference{ + APIVersion: "batch/v1", Kind: "Job", Name: "my-cj-100", + UID: "job-uid-old", Controller: new(true), + }, "CrashLoopBackOff", 5) + + newJobObj := newJob("my-cj-200") + newJobObj.UID = "job-uid-new" + newJobObj.CreationTimestamp = metav1.NewTime(metav1.Now().Add(-1 * time.Minute)) + // The newest job is healthy, so the workload must not count as all-failing. + healthyPod := newHealthyPod("my-cj-200-pod", metav1.OwnerReference{ + APIVersion: "batch/v1", Kind: "Job", Name: "my-cj-200", + UID: "job-uid-new", Controller: new(true), + }) + + // Seeded so the stale job is not last in the list. + c := setupTestClient(cj, newJobObj, oldJob, healthyPod, oldPod) + owner := &ownerWorkload{Kind: "CronJob", Name: "my-cj", Namespace: testNamespace} + + allFailing, err := allReplicasFailing(testCtx(), c, owner, DefaultWatchReasons) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if allFailing { + t.Error("expected the newest job to be evaluated, which is healthy") + } +} diff --git a/internal/controller/helpers.go b/internal/controller/helpers.go index 7a98b6a..4369ef9 100644 --- a/internal/controller/helpers.go +++ b/internal/controller/helpers.go @@ -153,7 +153,13 @@ func resolveOwnerWorkload(ctx context.Context, c client.Client, pod *corev1.Pod) return nil, nil } - ownerRef := pod.OwnerReferences[0] + // GetControllerOf returns the reference with controller: true. Index 0 is + // not guaranteed to be it: an object may carry additional non-controller + // owner references, and several tools add them. + ownerRef := metav1.GetControllerOf(pod) + if ownerRef == nil { + return nil, nil + } ns := pod.Namespace switch ownerRef.Kind { @@ -165,8 +171,13 @@ func resolveOwnerWorkload(ctx context.Context, c client.Client, pod *corev1.Pod) } return nil, err } - if len(rs.OwnerReferences) > 0 && rs.OwnerReferences[0].Kind == "Deployment" { - return &ownerWorkload{Kind: "Deployment", Name: rs.OwnerReferences[0].Name, Namespace: ns}, nil + if rs.UID != ownerRef.UID { + // The named ReplicaSet was replaced by a new one under the same + // name, so it is not the object that owns this pod. + return nil, nil + } + if rsOwner := metav1.GetControllerOf(rs); rsOwner != nil && rsOwner.Kind == "Deployment" { + return &ownerWorkload{Kind: "Deployment", Name: rsOwner.Name, Namespace: ns}, nil } return nil, nil @@ -181,8 +192,11 @@ func resolveOwnerWorkload(ctx context.Context, c client.Client, pod *corev1.Pod) } return nil, err } - if len(job.OwnerReferences) > 0 && job.OwnerReferences[0].Kind == "CronJob" { - return &ownerWorkload{Kind: "CronJob", Name: job.OwnerReferences[0].Name, Namespace: ns}, nil + if job.UID != ownerRef.UID { + return nil, nil + } + if jobOwner := metav1.GetControllerOf(job); jobOwner != nil && jobOwner.Kind == "CronJob" { + return &ownerWorkload{Kind: "CronJob", Name: jobOwner.Name, Namespace: ns}, nil } return nil, nil @@ -310,7 +324,11 @@ func allReplicasFailing(ctx context.Context, c client.Client, owner *ownerWorklo if len(ownedJobs) == 0 { return false, nil } - // Check pods of the most recent job + // Check pods of the most recent job. List order is not defined, so + // sort rather than trusting the last element. + slices.SortFunc(ownedJobs, func(a, b batchv1.Job) int { + return a.CreationTimestamp.Compare(b.CreationTimestamp.Time) + }) latestJob := ownedJobs[len(ownedJobs)-1] podList := &corev1.PodList{} if err := c.List(ctx, podList, client.InNamespace(owner.Namespace)); err != nil { diff --git a/internal/controller/testutil_test.go b/internal/controller/testutil_test.go index e7d35ad..1481371 100644 --- a/internal/controller/testutil_test.go +++ b/internal/controller/testutil_test.go @@ -21,7 +21,11 @@ import ( crashloopv1alpha1 "github.com/slauger/crashloop-operator/api/v1alpha1" ) -const testNamespace = "default" +const ( + testNamespace = "default" + // The single CronJob fixture every CronJob test builds on. + testCronJobName = "my-cj" +) func testScheme() *runtime.Scheme { s := runtime.NewScheme() @@ -234,6 +238,7 @@ func newReplicaSet(name, namespace, deploymentName string) *appsv1.ReplicaSet { Kind: "Deployment", Name: deploymentName, UID: "deploy-uid-1", + Controller: new(true), }, }, }, @@ -269,11 +274,11 @@ func newStatefulSet(name, namespace string, replicas int32) *appsv1.StatefulSet } } -func newCronJob(name, namespace string) *batchv1.CronJob { +func newCronJob() *batchv1.CronJob { return &batchv1.CronJob{ ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: namespace, + Name: testCronJobName, + Namespace: testNamespace, UID: "cj-uid-1", }, Spec: batchv1.CronJobSpec{ @@ -292,18 +297,19 @@ func newCronJob(name, namespace string) *batchv1.CronJob { } } -func newJob(name, namespace, cronJobName string) *batchv1.Job { +func newJob(name string) *batchv1.Job { return &batchv1.Job{ ObjectMeta: metav1.ObjectMeta{ Name: name, - Namespace: namespace, + Namespace: testNamespace, UID: "job-uid-1", OwnerReferences: []metav1.OwnerReference{ { APIVersion: "batch/v1", Kind: "CronJob", - Name: cronJobName, + Name: testCronJobName, UID: "cj-uid-1", + Controller: new(true), }, }, }, @@ -316,6 +322,9 @@ func rsOwnerRef() metav1.OwnerReference { Kind: "ReplicaSet", Name: "my-app-rs", UID: "rs-uid-1", + // Real Kubernetes always marks the owning reference as the + // controller, and resolution relies on that. + Controller: new(true), } } @@ -325,6 +334,9 @@ func stsOwnerRef() metav1.OwnerReference { Kind: "StatefulSet", Name: "my-sts", UID: "sts-uid-1", + // Real Kubernetes always marks the owning reference as the + // controller, and resolution relies on that. + Controller: new(true), } } @@ -334,6 +346,9 @@ func jobOwnerRef() metav1.OwnerReference { Kind: "Job", Name: "my-cj-job", UID: "job-uid-1", + // Real Kubernetes always marks the owning reference as the + // controller, and resolution relies on that. + Controller: new(true), } }