fix(controller): resolve owners by controller reference and sort Jobs

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")
+	}
+}

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 {

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),
 	}
 }