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