Move the ProviderConfigUsage finalizer into the managed reconciler

Address review feedback on the deletion-protection change:

- Make the usage cleaner a ReconcilerOption, WithProviderConfigUsageCleaner,
  defaulting to a no-op, instead of a required NewReconciler parameter.
- Stop adding ProviderConfigUsageFinalizer in Track. The reconciler now owns
  both ends of the finalizer: Protect adds it after Connect and Untrack
  removes it after teardown, so a provider that does not wire the cleaner
  never gets a finalizer nothing will remove. Track preserves whatever
  finalizers the usage already carries when it updates the reference.
- Rename FinalizersExcludingPropagation to NonGCFinalizers and name the two
  garbage collector finalizers it excludes.
- Emit warning events when protecting or releasing a usage fails.
- Release the usage finalizer before deleting a usage that has no owner
  reference, so it cannot be left terminating forever.
- Drop the numControllerFinalizers helper.

Signed-off-by: ezgidemirel <ezgidemirel91@gmail.com>

Author: ezgidemirel

pkg/meta/meta.go

@@ -205,9 +205,11 @@ func FinalizerExists(o metav1.Object, finalizer string) bool {
 	return slices.Contains(f, finalizer)
 }
 
-// FinalizersExcludingPropagation returns the supplied object's finalizers
-// without Kubernetes deletion propagation finalizers.
-func FinalizersExcludingPropagation(o metav1.Object) []string {
+// NonGCFinalizers returns the supplied object's finalizers, excluding the two
+// the Kubernetes garbage collector uses to implement deletion propagation -
+// foregroundDeletion and orphan. Neither belongs to a controller, and neither
+// makes a claim on an external system.
+func NonGCFinalizers(o metav1.Object) []string {
 	f := o.GetFinalizers()
 	out := make([]string, 0, len(f))
 

pkg/meta/meta_test.go

@@ -605,31 +605,45 @@ func TestFinalizerExists(t *testing.T) {
 	}
 }
 
-func TestFinalizersExcludingPropagation(t *testing.T) {
+func TestNonGCFinalizers(t *testing.T) {
+	type args struct {
+		o metav1.Object
+	}
+
 	cases := map[string]struct {
 		reason string
-		o      metav1.Object
+		args   args
 		want   []string
 	}{
 		"NoFinalizers": {
-			reason: "An object without finalizers has no finalizers after propagation finalizers are excluded.",
-			o:      &corev1.Pod{},
+			reason: "An object without finalizers has no finalizers after garbage collector finalizers are excluded.",
+			args:   args{o: &corev1.Pod{}},
 			want:   []string{},
 		},
-		"OnlyPropagationFinalizers": {
-			reason: "Kubernetes foreground and orphan propagation finalizers must both be excluded.",
-			o: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Finalizers: []string{
+		"OnlyGCFinalizers": {
+			reason: "The foregroundDeletion and orphan finalizers must both be excluded.",
+			args: args{o: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Finalizers: []string{
 				metav1.FinalizerDeleteDependents,
 				metav1.FinalizerOrphanDependents,
-			}}},
+			}}}},
 			want: []string{},
 		},
+		"KeepsControllerFinalizers": {
+			reason: "Finalizers that don't belong to the garbage collector must be kept, in order.",
+			args: args{o: &corev1.Pod{ObjectMeta: metav1.ObjectMeta{Finalizers: []string{
+				metav1.FinalizerDeleteDependents,
+				"example.org/first",
+				metav1.FinalizerOrphanDependents,
+				"example.org/second",
+			}}}},
+			want: []string{"example.org/first", "example.org/second"},
+		},
 	}
 
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
-			if diff := cmp.Diff(tc.want, FinalizersExcludingPropagation(tc.o)); diff != "" {
-				t.Errorf("%s\nFinalizersExcludingPropagation(...): -want, +got:\n%s", tc.reason, diff)
+			if diff := cmp.Diff(tc.want, NonGCFinalizers(tc.args.o)); diff != "" {
+				t.Errorf("%s\nNonGCFinalizers(...): -want, +got:\n%s", tc.reason, diff)
 			}
 		})
 	}

pkg/reconciler/managed/reconciler.go

@@ -67,6 +67,7 @@ const (
 	errReconcileUpdate          = "update failed"
 	errReconcileDelete          = "delete failed"
 	errRecordChangeLog          = "cannot record change log entry"
+	errProtectProviderConfig    = "cannot protect ProviderConfigUsage"
 	errUntrackProviderConfig    = "cannot release ProviderConfigUsage"
 
 	errExternalResourceNotExist = "external resource does not exist"
@@ -83,6 +84,8 @@ const (
 	reasonCannotObserve           event.Reason = "CannotObserveExternalResource"
 	reasonCannotCreate            event.Reason = "CannotCreateExternalResource"
 	reasonCannotDelete            event.Reason = "CannotDeleteExternalResource"
+	reasonCannotProtect           event.Reason = "CannotProtectProviderConfigUsage"
+	reasonCannotUntrack           event.Reason = "CannotReleaseProviderConfigUsage"
 	reasonCannotPublish           event.Reason = "CannotPublishConnectionDetails"
 	reasonCannotUnpublish         event.Reason = "CannotUnpublishConnectionDetails"
 	reasonCannotUpdate            event.Reason = "CannotUpdateExternalResource"
@@ -627,6 +630,10 @@ func defaultMRManaged(m manager.Manager) mrManaged {
 		ReferenceResolver:         NewAPISimpleReferenceResolver(m.GetClient()),
 		ConnectionPublisher:       NewAPISecretPublisher(m.GetClient(), m.GetScheme()),
 		LocalConnectionPublisher:  NewAPILocalSecretPublisher(m.GetClient(), m.GetScheme()),
+
+		// Providers opt in to ProviderConfigUsage protection by supplying their
+		// tracker via WithProviderConfigUsageCleaner.
+		ProviderConfigUsageCleaner: resource.NewNopProviderConfigUsageCleaner(),
 	}
 }
 
@@ -805,6 +812,25 @@ func WithFinalizer(f resource.Finalizer) ReconcilerOption {
 	}
 }
 
+// WithProviderConfigUsageCleaner specifies how the Reconciler should protect
+// and release the managed resource's ProviderConfigUsage. Managed resources
+// that don't use a ProviderConfigUsage don't need this option; the Reconciler
+// neither protects nor releases a usage by default.
+//
+// Supplying a cleaner is what enables ProviderConfig deletion protection. The
+// Reconciler adds the usage's finalizer once it has connected, and removes it
+// once the managed resource no longer needs its ProviderConfig, so the
+// finalizer is only ever added where something is wired up to remove it.
+//
+// The cleaner need not be the tracker the provider records usages with. It's
+// only used to find the usage belonging to a managed resource, so a separately
+// constructed one with the same client and usage type behaves identically.
+func WithProviderConfigUsageCleaner(c resource.ProviderConfigUsageCleaner) ReconcilerOption {
+	return func(r *Reconciler) {
+		r.managed.ProviderConfigUsageCleaner = c
+	}
+}
+
 // WithReferenceResolver specifies how the Reconciler should resolve any
 // inter-resource references it encounters while reconciling managed resources.
 func WithReferenceResolver(rr ReferenceResolver) ReconcilerOption {
@@ -878,18 +904,12 @@ func (r *Reconciler) effectivePollInterval(o metav1.Object) time.Duration {
 
 // NewReconciler returns a Reconciler that reconciles managed resources of the
 // supplied ManagedKind with resources in an external system such as a cloud
-// provider API. The usage cleaner should be the tracker used to track the
-// managed resource's ProviderConfigUsage. Managed resources that do not use a
-// ProviderConfigUsage should pass resource.NewNopProviderConfigUsageCleaner().
-// NewReconciler panics if the usage cleaner is nil or the managed resource kind
-// is not registered with the supplied manager's runtime.Scheme. The returned
-// Reconciler uses a no-op external system by default; callers should supply an
-// ExternalConnector that can manage resources in a real system.
-func NewReconciler(m manager.Manager, of resource.ManagedKind, usage resource.ProviderConfigUsageCleaner, o ...ReconcilerOption) *Reconciler {
-	if usage == nil {
-		panic("managed reconciler requires a ProviderConfigUsageCleaner")
-	}
-
+// provider API. It panics if asked to reconcile a managed resource kind that is
+// not registered with the supplied manager's runtime.Scheme. The returned
+// Reconciler reconciles with a dummy, no-op 'external system' by default;
+// callers should supply an ExternalConnector that returns an ExternalClient
+// capable of managing resources in a real system.
+func NewReconciler(m manager.Manager, of resource.ManagedKind, o ...ReconcilerOption) *Reconciler {
 	nm := func() resource.Managed {
 		//nolint:forcetypeassert // If this isn't an MR it's a programming error and we want to panic.
 		return resource.MustCreateObject(schema.GroupVersionKind(of), m.GetScheme()).(resource.Managed)
@@ -915,7 +935,6 @@ func NewReconciler(m manager.Manager, of resource.ManagedKind, usage resource.Pr
 		change:                      newNopChangeLogger(),
 		conditions:                  new(conditions.ObservedGenerationPropagationManager),
 	}
-	r.managed.ProviderConfigUsageCleaner = usage
 
 	for _, ro := range o {
 		ro(r)
@@ -1084,6 +1103,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu
 		if err := r.managed.Untrack(ctx, managed); err != nil {
 			log.Debug("Cannot release ProviderConfigUsage", "error", err)
 			uerr := errors.Wrap(err, errUntrackProviderConfig)
+			record.Event(managed, event.Warning(reasonCannotUntrack, uerr))
 			status.MarkConditions(xpv2.Deleting(), xpv2.ReconcileError(uerr))
 			if err := updateStatus(); err != nil {
 				return reconcile.Result{Requeue: true}, errors.Wrap(err, errUpdateManagedStatus)
@@ -1196,6 +1216,24 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu
 		}
 	}()
 
+	// We've connected, so the managed resource is using its ProviderConfig and
+	// its usage must outlive that need. The Reconciler owns both ends of the
+	// usage finalizer - added here, removed by Untrack below - so a provider's
+	// tracker never has to, and can't get out of step with us.
+	if err := r.managed.Protect(ctx, managed); err != nil {
+		log.Debug("Cannot protect ProviderConfigUsage", "error", err)
+
+		if kerrors.IsConflict(err) {
+			return reconcile.Result{Requeue: true}, nil
+		}
+
+		perr := errors.Wrap(err, errProtectProviderConfig)
+		record.Event(managed, event.Warning(reasonCannotProtect, perr))
+		status.MarkConditions(xpv2.ReconcileError(perr))
+
+		return reconcile.Result{Requeue: true}, errors.Wrap(updateStatus(), errUpdateManagedStatus)
+	}
+
 	observation, err := external.Observe(externalCtx, managed)
 	if err != nil {
 		// We'll usually hit this case if our Provider credentials are invalid
@@ -1252,7 +1290,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu
 	if meta.WasDeleted(managed) {
 		log = log.WithValues("deletion-timestamp", managed.GetDeletionTimestamp())
 
-		if numControllerFinalizers(managed) > 1 {
+		if len(meta.NonGCFinalizers(managed)) > 1 {
 			// There are other controllers monitoring this resource so preserve the external instance
 			// until all other finalizers have been removed
 			log.Debug("Delay external deletion until all finalizers have been removed")
@@ -1337,6 +1375,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu
 		if err := r.managed.Untrack(ctx, managed); err != nil {
 			log.Debug("Cannot release ProviderConfigUsage", "error", err)
 			uerr := errors.Wrap(err, errUntrackProviderConfig)
+			record.Event(managed, event.Warning(reasonCannotUntrack, uerr))
 			status.MarkConditions(xpv2.Deleting(), xpv2.ReconcileError(uerr))
 			if err := updateStatus(); err != nil {
 				return reconcile.Result{Requeue: true}, errors.Wrap(err, errUpdateManagedStatus)
@@ -1609,9 +1648,3 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu
 
 	return reconcile.Result{RequeueAfter: reconcileAfter}, errors.Wrap(updateStatus(), errUpdateManagedStatus)
 }
-
-// numControllerFinalizers returns the number of finalizers not managed by the
-// Kubernetes garbage collector.
-func numControllerFinalizers(mg resource.Managed) int {
-	return len(meta.FinalizersExcludingPropagation(mg))
-}

pkg/reconciler/managed/reconciler_legacy_test.go

@@ -2165,7 +2165,7 @@ func TestReconciler(t *testing.T) {
 
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
-			r := NewReconciler(tc.args.m, tc.args.mg, resource.NewNopProviderConfigUsageCleaner(), tc.args.o...)
+			r := NewReconciler(tc.args.m, tc.args.mg, tc.args.o...)
 
 			got, err := r.Reconcile(context.Background(), reconcile.Request{})
 			if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
@@ -2886,7 +2886,7 @@ func TestLegacyReconcilerChangeLogs(t *testing.T) {
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
 			tc.args.o = append(tc.args.o, WithChangeLogger(NewGRPCChangeLogger(tc.args.c, WithProviderVersion("provider-cool:v9.99.999"))))
-			r := NewReconciler(tc.args.m, tc.args.mg, resource.NewNopProviderConfigUsageCleaner(), tc.args.o...)
+			r := NewReconciler(tc.args.m, tc.args.mg, tc.args.o...)
 			r.Reconcile(context.Background(), reconcile.Request{})
 
 			if diff := cmp.Diff(tc.want.callCount, len(tc.args.c.requests)); diff != "" {