From 2f1cbfb5c08bd78e4c9602acd4123e28936378c8 Mon Sep 17 00:00:00 2001 From: Scott Andrews Date: Fri, 13 May 2022 12:47:52 -0400 Subject: [PATCH] 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 --- README.md | 4 +- reconcilers/reconcilers.go | 50 ++++++++++++++++-- reconcilers/reconcilers_test.go | 66 +++++++++++++++++++++--- reconcilers/reconcilers_validate_test.go | 30 +++++++++++ testing/tracker.go | 17 ++++++ tracker/tracker.go | 22 ++++++++ 6 files changed, 177 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 182dece..e2794b1 100644 --- a/README.md +++ b/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:** diff --git a/reconcilers/reconcilers.go b/reconcilers/reconcilers.go index 3e38dd8..0a9c8c2 100644 --- a/reconcilers/reconcilers.go +++ b/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. // @@ -752,6 +762,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 // @@ -791,7 +803,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 @@ -802,6 +818,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) @@ -892,6 +913,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 @@ -982,8 +1007,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)) @@ -1023,6 +1050,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 @@ -1051,6 +1087,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", @@ -1181,7 +1223,7 @@ func (r *ChildReconciler) filterChildren(parent client.Object, children client.O } 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? diff --git a/reconcilers/reconcilers_test.go b/reconcilers/reconcilers_test.go index 64c8a63..214930c 100644 --- a/reconcilers/reconcilers_test.go +++ b/reconcilers/reconcilers_test.go @@ -919,9 +919,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), @@ -940,7 +945,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{ { @@ -1001,15 +1009,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), @@ -1028,6 +1044,9 @@ func TestChildReconciler(t *testing.T) { }), ExpectUpdates: []client.Object{ configMapGiven. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.OwnerReferences() + }). AddData("new", "field"), }, }, { @@ -1041,15 +1060,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), @@ -1071,6 +1098,9 @@ func TestChildReconciler(t *testing.T) { }), ExpectUpdates: []client.Object{ configMapGiven. + MetadataDie(func(d *diemetav1.ObjectMetaDie) { + d.OwnerReferences() + }). AddData("new", "field"), }, ExpectPatches: []rtesting.PatchRef{ @@ -1108,12 +1138,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 }, }, @@ -1148,12 +1183,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 }, }, @@ -1230,12 +1270,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 }, }, @@ -1469,6 +1514,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 }, }, @@ -1497,12 +1544,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 }, }, diff --git a/reconcilers/reconcilers_validate_test.go b/reconcilers/reconcilers_validate_test.go index cc3c9d5..ec62fc5 100644 --- a/reconcilers/reconcilers_validate_test.go +++ b/reconcilers/reconcilers_validate_test.go @@ -755,6 +755,36 @@ func TestChildReconciler_validate(t *testing.T) { }, shouldErr: `ChildReconciler "HarmonizeImmutableFields num out" must implement HarmonizeImmutableFields: nil | func(*v1.Pod, *v1.Pod), found: func(*v1.Pod, *v1.Pod) error`, }, + { + 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{}, diff --git a/testing/tracker.go b/testing/tracker.go index ba34dc6..8a285bc 100644 --- a/testing/tracker.go +++ b/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...) } diff --git a/tracker/tracker.go b/tracker/tracker.go index f3c30c7..45cfca4 100644 --- a/tracker/tracker.go +++ b/tracker/tracker.go @@ -29,8 +29,10 @@ import ( "time" "github.com/go-logr/logr" + "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" ) // Tracker defines the interface through which an object can register @@ -40,6 +42,9 @@ type Tracker interface { // referenced object. Track(ctx context.Context, ref Key, obj types.NamespacedName) + // TrackChild tracks the child by the parent. + TrackChild(ctx context.Context, parent, child client.Object, s *runtime.Scheme) error + // Lookup returns actively tracked objects for the reference. Lookup(ctx context.Context, ref Key) []types.NamespacedName } @@ -89,6 +94,23 @@ var _ Tracker = (*impl)(nil) // set is a map from keys to expirations type set map[types.NamespacedName]time.Time +// TrackChild implements Tracker. +func (i *impl) 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) + } + i.Track( + ctx, + NewKey(gvks[0], types.NamespacedName{Namespace: child.GetNamespace(), Name: child.GetName()}), + types.NamespacedName{Namespace: parent.GetNamespace(), Name: parent.GetName()}, + ) + return nil +} + // Track implements Tracker. func (i *impl) Track(ctx context.Context, ref Key, obj types.NamespacedName) { log := logr.FromContextOrDiscard(ctx).WithName("tracker")