Fix: Add owner reference to existing resources when initially missing

[ArsalanAnwer0]

Fixes #8718

Problem

ReconcileResource does not add owner references to existing resources that were created without an owner. This occurs when NeedsUpdate() returns false (because data, labels, and annotations haven't changed), causing the update block which contains the owner reference logic to be skipped entirely.

Solution

Added a separate owner reference check that runs independently of NeedsUpdate(). This makes sure that owner references are added or updated on existing resources even when no other fields require updates.

Changes

• Added owner reference validation before the NeedsUpdate block in ReconcileResource
• Owner references are now properly set on resources that initially lacked them
• Added test case Add owner reference to existing resource without owner when NeedsUpdate returns false to verify the fix

Testing

• All existing tests pass (no regressions)
• New test case specifically validates the bug scenario
• Ran full reconciler package test suite successfully

This fix enables proper resource ownership management and garbage collection for resources created before owner references were implemented.

[prodsecmachine]

🎉 Snyk checks have passed. No issues have been found so far.

✅ security/snyk check is complete. No issues have been found. (View Details)

✅ license/snyk check is complete. No issues have been found. (View Details)

[naemono]

Fixes #8718

Problem

ReconcileResource does not add owner references to existing resources that were created without an owner. This occurs when NeedsUpdate() returns false (because data, labels, and annotations haven't changed), causing the update block which contains the owner reference logic to be skipped entirely.

@ArsalanAnwer0 Thanks for the contribution. After investigating the original problem, and looking through the implementation, I think the following approach may be simpler and includes an associated test:

diff --git a/pkg/controller/common/reconciler/secret.go b/pkg/controller/common/reconciler/secret.go
index d280023d4..831fd3408 100644
--- a/pkg/controller/common/reconciler/secret.go
+++ b/pkg/controller/common/reconciler/secret.go
@@ -52,7 +52,9 @@ func ReconcileSecret(ctx context.Context, c k8s.Client, expected corev1.Secret,
                        return !maps.IsSubset(expected.Labels, reconciled.Labels) ||
                                !maps.IsSubset(expected.Annotations, reconciled.Annotations) ||
                                // or if secret data is not strictly equal
-                               !reflect.DeepEqual(expected.Data, reconciled.Data)
+                               !reflect.DeepEqual(expected.Data, reconciled.Data) ||
+                               // or if the owner references are not equal
+                               !reflect.DeepEqual(expected.OwnerReferences, reconciled.OwnerReferences)
                },
                UpdateReconciled: func() {
                        // set expected annotations and labels, but don't remove existing ones
diff --git a/pkg/controller/common/reconciler/secret_test.go b/pkg/controller/common/reconciler/secret_test.go
index e4e616616..a1b8dba51 100644
--- a/pkg/controller/common/reconciler/secret_test.go
+++ b/pkg/controller/common/reconciler/secret_test.go
@@ -89,6 +89,12 @@ func TestReconcileSecret(t *testing.T) {
                        expected: createSecret("s", sampleData, sampleLabels, sampleAnnotations),
                        want:     withOwnerRef(t, createSecret("s", sampleData, sampleLabels, sampleAnnotations)),
                },
+               {
+                       name:     "owner reference should be added if none exist",
+                       c:        k8s.NewFakeClient(createSecret("s", sampleData, nil, nil)),
+                       expected: withOwnerRef(t, createSecret("s", sampleData, nil, nil)),
+                       want:     withOwnerRef(t, createSecret("s", sampleData, nil, nil)),
+               },
                {
                        name: "preserve existing labels and annotations",
                        c: k8s.NewFakeClient(withOwnerRef(t, createSecret("s", sampleData,

I'll ask for some verification from my colleagues and will update.

[barkbay]

@ArsalanAnwer0 Thanks for the contribution. After investigating the original problem, and looking through the implementation, I think the following approach may be simpler and includes an associated test:

+                               // or if the owner references are not equal
+                               !reflect.DeepEqual(expected.OwnerReferences, reconciled.OwnerReferences)

As stated a bit later in the code we want to support additional references. I think what we want to ensure here is that the "primary" managing controller (controller field set to true) is the expected one.

[barkbay]

Let's maybe extract the logic into a dedicated function:

Suggested change

	// Check if owner reference needs to be added/updated independently of NeedsUpdate
	if params.Owner != nil {
	reconciledMeta, err := meta.Accessor(params.Reconciled)
	if err != nil {
	return err
	}
	expectedMeta, err := meta.Accessor(params.Expected)
	if err != nil {
	return err
	}
	expectedOwners := expectedMeta.GetOwnerReferences()
	// Check if we need to update the owner reference
	needsOwnerUpdate := false
	if len(expectedOwners) > 0 {
	reconciledOwners := reconciledMeta.GetOwnerReferences()
	// Check if controller reference is missing or different
	controllerRefIndex := -1
	for i, owner := range reconciledOwners {
	if owner.Controller != nil && *owner.Controller {
	controllerRefIndex = i
	break
	}
	}
	if controllerRefIndex == -1 {
	// No controller reference exists
	needsOwnerUpdate = true
	} else if reconciledOwners[controllerRefIndex].UID != expectedOwners[0].UID {
	// Controller reference exists but points to a different owner
	needsOwnerUpdate = true
	}
	}
	if needsOwnerUpdate {
	log.Info("Updating owner reference")
	k8s.OverrideControllerReference(reconciledMeta, expectedOwners[0])
	if err := params.Client.Update(params.Context, params.Reconciled); err != nil {
	return err
	}
	log.Info("Updated owner reference successfully", resourceVersionKey, params.Reconciled.GetResourceVersion())
	}
	}
	//nolint:nestif
	// Update if needed
	if params.NeedsUpdate() {
	// Check if owner reference needs to be added/updated independently of NeedsUpdate
	needsOwnerReferenceUpdate := k8s.HasExpectedControllerOwnerReference(params.Reconciled, params.Expected)
	//nolint:nestif
	// Update if needed
	if params.NeedsUpdate() || needsOwnerReferenceUpdate {

With HasExpectedControllerOwnerReference in pkg/utils/k8s/owner_refs.go:

// HasExpectedControllerOwnerReference checks whether the controller owner reference (managing controller) on the first resource is the same as the second one.
func HasExpectedControllerOwnerReference(reconciled, expected metav1.Object) bool {
	if reconciled == nil || expected == nil {
		return false
	}

	expectedOwners := expected.GetOwnerReferences()
	expectedControllerOwnerIdx := indexOfCtrlRef(expectedOwners)
	if expectedControllerOwnerIdx == -1 {
		// No expected controller owner reference.
		return false
	}

	reconciledOwners := reconciled.GetOwnerReferences()
	reconciledControllerOwnerIdx := indexOfCtrlRef(reconciledOwners)
	if reconciledControllerOwnerIdx == -1 {
		// No reconciled controller owner reference on the reconciled object while we expect one.
		return true
	}
	return equality.Semantic.DeepEqual(reconciledOwners[reconciledControllerOwnerIdx], expectedOwners[expectedControllerOwnerIdx])
}

[barkbay]

I marked my previous comment as off topic but I still think we should not assume the first item in expectedOwners is the managing controller.

			k8s.OverrideControllerReference(reconciledMeta, expectedOwners[0])