Allow ChildReconciler to work without owner references

Owner references only work within the same namespace (or cluster scope).
Finalizers give us the ability to clean up state in other scopes, where
owner references are not valid.

ChildReconciler can now opt-out of owner references with
SkipOwnerReference, using a child Finalizer implies SkipOwnerReference.
Since we can no longer link parent and child resources via the owner
reference, the child is tracked explicitly and the OurChild method must
be defined and able to uniquely distinguish the child resource that
belongs to the parent from other resources of the same kind.

Signed-off-by: Scott Andrews <andrewssc@vmware.com>

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