Allow ChildReconciler to work without owner references

README.md

@@ -141,7 +141,7 @@ The [`ChildReconciler`](https://pkg.go.dev/github.com/vmware-labs/reconciler-run
 The ChildReconciler is responsible for:
 - looking up an existing child
 - creating/updating/deleting the child resource based on the desired state
-- setting the owner reference on the child resource
+- setting the owner reference on the child resource (when not using a finalizer)
 - logging the reconcilers activities
 - recording child mutations and errors for the parent resource
 - adapting to child resource changes applied by mutating webhooks
@@ -156,6 +156,8 @@ The implementor is responsible for:
 
 When a finalizer is defined, the parent resource is patched to add the finalizer before creating the child; it is removed after the child is deleted. If the parent resource is pending deletion, the desired child method is not called, and existing children are deleted.
 
+Using a finalizer means that the child resource will not use an owner reference. The OurChild method must be implemented in a way that can uniquely and unambiguously identify the child that this parent resource is responsible for from any other resources of the same kind. The child resource is tracked explicitly.
+
 > Warning: It is crucial that each ChildReconciler using a finalizer have a unique and stable finalizer name. Two reconcilers that use the same finalizer, or a reconciler that changed the name of its finalizer, may leak the child resource when the parent is deleted, or the parent resource may never terminate.
 
 **Example:**

reconcilers/reconcilers.go

@@ -31,6 +31,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/cluster"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
+	"sigs.k8s.io/controller-runtime/pkg/source"
 	"sigs.k8s.io/controller-runtime/pkg/webhook"
 
 	"github.com/vmware-labs/reconciler-runtime/tracker"
@@ -693,9 +694,18 @@ type ChildReconciler struct {
 	// the child resource automatically, like when the parent and child are in different
 	// namespaces, scopes or clusters.
 	//
+	// Use of a finalizer implies that SkipOwnerReference is true, and OurChild must be defined.
+	//
 	// +optional
 	Finalizer string
 
+	// SkipOwnerReference when true will not create and find child resources via an owner
+	// reference. OurChild must be defined for the reconciler to distinguish the child being
+	// reconciled from other resources of the same type.
+	//
+	// Any child resource created is tracked for changes.
+	SkipOwnerReference bool
+
 	// Setup performs initialization on the manager and builder this reconciler
 	// will run with. It's common to setup field indexes and watch resources.
 	//
@@ -766,6 +776,8 @@ type ChildReconciler struct {
 	// should match this function, otherwise they may be orphaned. If not specified, all
 	// children match.
 	//
+	// OurChild is required when a Finalizer is defined or SkipOwnerReference is true.
+	//
 	// Expected function signature:
 	//     func(parent, child client.Object) bool
 	//
@@ -805,7 +817,11 @@ func (r *ChildReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager
 		return err
 	}
 
-	bldr.Owns(r.ChildType)
+	if r.SkipOwnerReference {
+		bldr.Watches(&source.Kind{Type: r.ChildType}, EnqueueTracked(ctx, r.ChildType))
+	} else {
+		bldr.Owns(r.ChildType)
+	}
 
 	if r.Setup == nil {
 		return nil
@@ -816,6 +832,11 @@ func (r *ChildReconciler) SetupWithManager(ctx context.Context, mgr ctrl.Manager
 func (r *ChildReconciler) validate(ctx context.Context) error {
 	castParentType := RetrieveCastParentType(ctx)
 
+	// default implicit values
+	if r.Finalizer != "" {
+		r.SkipOwnerReference = true
+	}
+
 	// validate ChildType value
 	if r.ChildType == nil {
 		return fmt.Errorf("ChildReconciler %q must define ChildType", r.Name)
@@ -919,6 +940,10 @@ func (r *ChildReconciler) validate(ctx context.Context) error {
 			return fmt.Errorf("ChildReconciler %q must implement OurChild: nil | func(%s, %s) bool, found: %s", r.Name, reflect.TypeOf(castParentType), reflect.TypeOf(r.ChildType), fn)
 		}
 	}
+	if r.OurChild == nil && r.SkipOwnerReference {
+		// OurChild is required when SkipOwnerReference is true
+		return fmt.Errorf("ChildReconciler %q must implement OurChild", r.Name)
+	}
 
 	// validate Sanitize function signature:
 	//     nil
@@ -1009,8 +1034,10 @@ func (r *ChildReconciler) reconcile(ctx context.Context, parent client.Object) (
 		return nil, err
 	}
 	if desired != nil {
-		if err := ctrl.SetControllerReference(parent, desired, c.Scheme()); err != nil {
-			return nil, err
+		if !r.SkipOwnerReference {
+			if err := ctrl.SetControllerReference(parent, desired, c.Scheme()); err != nil {
+				return nil, err
+			}
 		}
 		if !r.ourChild(parent, desired) {
 			log.Info("object returned from DesiredChild does not match OurChild, this can result in orphaned children", "child", namespaceName(desired))
@@ -1050,6 +1077,15 @@ func (r *ChildReconciler) reconcile(ctx context.Context, parent client.Object) (
 				"Failed to create %s %q: %v", typeName(r.ChildType), desired.GetName(), err)
 			return nil, err
 		}
+		if r.SkipOwnerReference {
+			// track since we can't lean on owner refs
+
+			// normally tracks should occur before API operations, but when creating a resource with a
+			// generated name, we need to know the actual resource name.
+			if err := c.Tracker.TrackChild(ctx, parent, desired, c.Scheme()); err != nil {
+				return nil, err
+			}
+		}
 		pc.Recorder.Eventf(parent, corev1.EventTypeNormal, "Created",
 			"Created %s %q", typeName(r.ChildType), desired.GetName())
 		return desired, nil
@@ -1078,6 +1114,12 @@ func (r *ChildReconciler) reconcile(ctx context.Context, parent client.Object) (
 	current := actual.DeepCopyObject().(client.Object)
 	r.mergeBeforeUpdate(current, desiredPatched)
 	log.Info("updating child", "diff", cmp.Diff(r.sanitize(actual), r.sanitize(current)))
+	if r.SkipOwnerReference {
+		// track since we can't lean on owner refs
+		if err := c.Tracker.TrackChild(ctx, parent, current, c.Scheme()); err != nil {
+			return nil, err
+		}
+	}
 	if err := c.Update(ctx, current); err != nil {
 		log.Error(err, "unable to update child", "child", namespaceName(current))
 		pc.Recorder.Eventf(parent, corev1.EventTypeWarning, "UpdateFailed",
@@ -1222,7 +1264,7 @@ func (r *ChildReconciler) listOptions(ctx context.Context, parent client.Object)
 }
 
 func (r *ChildReconciler) ourChild(parent, obj client.Object) bool {
-	if !metav1.IsControlledBy(obj, parent) {
+	if !r.SkipOwnerReference && !metav1.IsControlledBy(obj, parent) {
 		return false
 	}
 	// TODO do we need to remove resources pending deletion?

reconcilers/reconcilers_test.go

@@ -945,9 +945,14 @@ func TestChildReconciler(t *testing.T) {
 			"SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler {
 				r := defaultChildReconciler(c)
 				r.Finalizer = testFinalizer
+				r.SkipOwnerReference = true
+				r.OurChild = func(parent, child client.Object) bool { return true }
 				return r
 			},
 		},
+		ExpectTracks: []rtesting.TrackRequest{
+			rtesting.NewTrackRequest(configMapCreate, resource, scheme),
+		},
 		ExpectEvents: []rtesting.Event{
 			rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "FinalizerPatched",
 				`Patched finalizer %q`, testFinalizer),
@@ -966,7 +971,10 @@ func TestChildReconciler(t *testing.T) {
 				d.AddField("foo", "bar")
 			}),
 		ExpectCreates: []client.Object{
-			configMapCreate,
+			configMapCreate.
+				MetadataDie(func(d *diemetav1.ObjectMetaDie) {
+					d.OwnerReferences()
+				}),
 		},
 		ExpectPatches: []rtesting.PatchRef{
 			{
@@ -1027,15 +1035,23 @@ func TestChildReconciler(t *testing.T) {
 				d.AddField("foo", "bar")
 			}),
 		GivenObjects: []client.Object{
-			configMapGiven,
+			configMapGiven.
+				MetadataDie(func(d *diemetav1.ObjectMetaDie) {
+					d.OwnerReferences()
+				}),
 		},
 		Metadata: map[string]interface{}{
 			"SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler {
 				r := defaultChildReconciler(c)
 				r.Finalizer = testFinalizer
+				r.SkipOwnerReference = true
+				r.OurChild = func(parent, child client.Object) bool { return true }
 				return r
 			},
 		},
+		ExpectTracks: []rtesting.TrackRequest{
+			rtesting.NewTrackRequest(configMapGiven, resource, scheme),
+		},
 		ExpectEvents: []rtesting.Event{
 			rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "Updated",
 				`Updated ConfigMap %q`, testName),
@@ -1054,6 +1070,9 @@ func TestChildReconciler(t *testing.T) {
 			}),
 		ExpectUpdates: []client.Object{
 			configMapGiven.
+				MetadataDie(func(d *diemetav1.ObjectMetaDie) {
+					d.OwnerReferences()
+				}).
 				AddData("new", "field"),
 		},
 	}, {
@@ -1067,15 +1086,23 @@ func TestChildReconciler(t *testing.T) {
 				d.AddField("foo", "bar")
 			}),
 		GivenObjects: []client.Object{
-			configMapGiven,
+			configMapGiven.
+				MetadataDie(func(d *diemetav1.ObjectMetaDie) {
+					d.OwnerReferences()
+				}),
 		},
 		Metadata: map[string]interface{}{
 			"SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler {
 				r := defaultChildReconciler(c)
 				r.Finalizer = testFinalizer
+				r.SkipOwnerReference = true
+				r.OurChild = func(parent, child client.Object) bool { return true }
 				return r
 			},
 		},
+		ExpectTracks: []rtesting.TrackRequest{
+			rtesting.NewTrackRequest(configMapGiven, resource, scheme),
+		},
 		ExpectEvents: []rtesting.Event{
 			rtesting.NewEvent(resource, scheme, corev1.EventTypeNormal, "FinalizerPatched",
 				`Patched finalizer %q`, testFinalizer),
@@ -1097,6 +1124,9 @@ func TestChildReconciler(t *testing.T) {
 			}),
 		ExpectUpdates: []client.Object{
 			configMapGiven.
+				MetadataDie(func(d *diemetav1.ObjectMetaDie) {
+					d.OwnerReferences()
+				}).
 				AddData("new", "field"),
 		},
 		ExpectPatches: []rtesting.PatchRef{
@@ -1134,12 +1164,17 @@ func TestChildReconciler(t *testing.T) {
 				d.Finalizers(testFinalizer)
 			}),
 		GivenObjects: []client.Object{
-			configMapGiven,
+			configMapGiven.
+				MetadataDie(func(d *diemetav1.ObjectMetaDie) {
+					d.OwnerReferences()
+				}),
 		},
 		Metadata: map[string]interface{}{
 			"SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler {
 				r := defaultChildReconciler(c)
 				r.Finalizer = testFinalizer
+				r.SkipOwnerReference = true
+				r.OurChild = func(parent, child client.Object) bool { return true }
 				return r
 			},
 		},
@@ -1174,12 +1209,17 @@ func TestChildReconciler(t *testing.T) {
 				d.Finalizers("some.other.finalizer")
 			}),
 		GivenObjects: []client.Object{
-			configMapGiven,
+			configMapGiven.
+				MetadataDie(func(d *diemetav1.ObjectMetaDie) {
+					d.OwnerReferences()
+				}),
 		},
 		Metadata: map[string]interface{}{
 			"SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler {
 				r := defaultChildReconciler(c)
 				r.Finalizer = testFinalizer
+				r.SkipOwnerReference = true
+				r.OurChild = func(parent, child client.Object) bool { return true }
 				return r
 			},
 		},
@@ -1256,12 +1296,17 @@ func TestChildReconciler(t *testing.T) {
 				d.Finalizers(testFinalizer)
 			}),
 		GivenObjects: []client.Object{
-			configMapGiven,
+			configMapGiven.
+				MetadataDie(func(d *diemetav1.ObjectMetaDie) {
+					d.OwnerReferences()
+				}),
 		},
 		Metadata: map[string]interface{}{
 			"SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler {
 				r := defaultChildReconciler(c)
 				r.Finalizer = testFinalizer
+				r.SkipOwnerReference = true
+				r.OurChild = func(parent, child client.Object) bool { return true }
 				return r
 			},
 		},
@@ -1495,6 +1540,8 @@ func TestChildReconciler(t *testing.T) {
 			"SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler {
 				r := defaultChildReconciler(c)
 				r.Finalizer = testFinalizer
+				r.SkipOwnerReference = true
+				r.OurChild = func(parent, child client.Object) bool { return true }
 				return r
 			},
 		},
@@ -1523,12 +1570,17 @@ func TestChildReconciler(t *testing.T) {
 				d.Finalizers(testFinalizer)
 			}),
 		GivenObjects: []client.Object{
-			configMapGiven,
+			configMapGiven.
+				MetadataDie(func(d *diemetav1.ObjectMetaDie) {
+					d.OwnerReferences()
+				}),
 		},
 		Metadata: map[string]interface{}{
 			"SubReconciler": func(t *testing.T, c reconcilers.Config) reconcilers.SubReconciler {
 				r := defaultChildReconciler(c)
 				r.Finalizer = testFinalizer
+				r.SkipOwnerReference = true
+				r.OurChild = func(parent, child client.Object) bool { return true }
 				return r
 			},
 		},

reconcilers/reconcilers_validate_test.go

@@ -843,6 +843,36 @@ func TestChildReconciler_validate(t *testing.T) {
 			},
 			shouldErr: `ChildReconciler "ListOptions out 1" must implement ListOptions: nil | func(context.Context, *v1.ConfigMap) []client.ListOption, found: func(context.Context, *v1.ConfigMap) client.ListOptions`,
 		},
+		{
+			name:   "Finalizer without OurChild",
+			parent: &corev1.ConfigMap{},
+			reconciler: &ChildReconciler{
+				Name:                       "Finalizer without OurChild",
+				ChildType:                  &corev1.Pod{},
+				ChildListType:              &corev1.PodList{},
+				DesiredChild:               func(ctx context.Context, parent *corev1.ConfigMap) (*corev1.Pod, error) { return nil, nil },
+				ReflectChildStatusOnParent: func(parent *corev1.ConfigMap, child *corev1.Pod, err error) {},
+				MergeBeforeUpdate:          func(current, desired *corev1.Pod) {},
+				SemanticEquals:             func(a1, a2 *corev1.Pod) bool { return false },
+				Finalizer:                  "my-finalizer",
+			},
+			shouldErr: `ChildReconciler "Finalizer without OurChild" must implement OurChild`,
+		},
+		{
+			name:   "SkipOwnerReference without OurChild",
+			parent: &corev1.ConfigMap{},
+			reconciler: &ChildReconciler{
+				Name:                       "SkipOwnerReference without OurChild",
+				ChildType:                  &corev1.Pod{},
+				ChildListType:              &corev1.PodList{},
+				DesiredChild:               func(ctx context.Context, parent *corev1.ConfigMap) (*corev1.Pod, error) { return nil, nil },
+				ReflectChildStatusOnParent: func(parent *corev1.ConfigMap, child *corev1.Pod, err error) {},
+				MergeBeforeUpdate:          func(current, desired *corev1.Pod) {},
+				SemanticEquals:             func(a1, a2 *corev1.Pod) bool { return false },
+				SkipOwnerReference:         true,
+			},
+			shouldErr: `ChildReconciler "SkipOwnerReference without OurChild" must implement OurChild`,
+		},
 		{
 			name:   "OurChild",
 			parent: &corev1.ConfigMap{},

testing/tracker.go

@@ -7,6 +7,7 @@ package testing
 
 import (
 	"context"
+	"fmt"
 	"time"
 
 	"github.com/vmware-labs/reconciler-runtime/tracker"
@@ -69,6 +70,22 @@ func (t *mockTracker) Track(ctx context.Context, ref tracker.Key, obj types.Name
 	t.reqs = append(t.reqs, TrackRequest{Tracked: ref, Tracker: obj})
 }
 
+func (t *mockTracker) TrackChild(ctx context.Context, parent, child client.Object, s *runtime.Scheme) error {
+	gvks, _, err := s.ObjectKinds(child)
+	if err != nil {
+		return err
+	}
+	if len(gvks) != 1 {
+		return fmt.Errorf("expected exactly one GVK, found: %s", gvks)
+	}
+	t.Track(
+		ctx,
+		tracker.NewKey(gvks[0], types.NamespacedName{Namespace: child.GetNamespace(), Name: child.GetName()}),
+		types.NamespacedName{Namespace: parent.GetNamespace(), Name: parent.GetName()},
+	)
+	return nil
+}
+
 func (t *mockTracker) getTrackRequests() []TrackRequest {
 	return append([]TrackRequest{}, t.reqs...)
 }