Protect ProviderConfigs while managed resources terminate

Keep ProviderConfigUsages alive until their managed resources finish teardown,
ignore Kubernetes propagation finalizers during external deletion, and reap
terminating usages whose owners are gone or have completed teardown.

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

Author: ezgidemirel

pkg/meta/meta.go

@@ -205,6 +205,23 @@ 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 {
+	f := o.GetFinalizers()
+	out := make([]string, 0, len(f))
+
+	for _, e := range f {
+		if e == metav1.FinalizerDeleteDependents || e == metav1.FinalizerOrphanDependents {
+			continue
+		}
+
+		out = append(out, e)
+	}
+
+	return out
+}
+
 // AddLabels to the supplied object.
 func AddLabels(o metav1.Object, labels map[string]string) {
 	l := o.GetLabels()

pkg/reconciler/managed/reconciler.go

@@ -67,6 +67,7 @@ const (
 	errReconcileUpdate          = "update failed"
 	errReconcileDelete          = "delete failed"
 	errRecordChangeLog          = "cannot record change log entry"
+	errUntrackProviderConfig    = "cannot release ProviderConfigUsage"
 
 	errExternalResourceNotExist = "external resource does not exist"
 
@@ -615,6 +616,7 @@ type mrManaged struct {
 	Initializer
 	ReferenceResolver
 	LocalConnectionPublisher
+	resource.ProviderConfigUsageCleaner
 }
 
 func defaultMRManaged(m manager.Manager) mrManaged {
@@ -876,12 +878,18 @@ 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. 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 {
+// 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")
+	}
+
 	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)
@@ -907,6 +915,7 @@ func NewReconciler(m manager.Manager, of resource.ManagedKind, o ...ReconcilerOp
 		change:                      newNopChangeLogger(),
 		conditions:                  new(conditions.ObservedGenerationPropagationManager),
 	}
+	r.managed.ProviderConfigUsageCleaner = usage
 
 	for _, ro := range o {
 		ro(r)
@@ -1070,6 +1079,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu
 			return reconcile.Result{Requeue: true}, errors.Wrap(updateStatus(), errUpdateManagedStatus)
 		}
 
+		// Release the usage after removing the managed resource finalizer so the
+		// ProviderConfig remains available throughout deletion.
+		if err := r.managed.Untrack(ctx, managed); err != nil {
+			log.Debug("Cannot release ProviderConfigUsage", "error", err)
+			status.MarkConditions(xpv2.Deleting(), xpv2.ReconcileError(err))
+			return reconcile.Result{Requeue: true}, errors.Wrap(updateStatus(), errUntrackProviderConfig)
+		}
+
 		// We've successfully unpublished our managed resource's connection
 		// details and removed our finalizer. If we assume we were the only
 		// controller that added a finalizer to this resource then it should no
@@ -1231,7 +1248,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu
 	if meta.WasDeleted(managed) {
 		log = log.WithValues("deletion-timestamp", managed.GetDeletionTimestamp())
 
-		if len(managed.GetFinalizers()) > 1 {
+		if numControllerFinalizers(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")
@@ -1311,6 +1328,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req reconcile.Request) (resu
 			return reconcile.Result{Requeue: true}, errors.Wrap(updateStatus(), errUpdateManagedStatus)
 		}
 
+		// Release the usage after removing the managed resource finalizer so the
+		// ProviderConfig remains available throughout deletion.
+		if err := r.managed.Untrack(ctx, managed); err != nil {
+			log.Debug("Cannot release ProviderConfigUsage", "error", err)
+			status.MarkConditions(xpv2.Deleting(), xpv2.ReconcileError(err))
+			return reconcile.Result{Requeue: true}, errors.Wrap(updateStatus(), errUntrackProviderConfig)
+		}
+
 		// We've successfully deleted our external resource (if necessary) and
 		// removed our finalizer. If we assume we were the only controller that
 		// added a finalizer to this resource then it should no longer exist and
@@ -1576,3 +1601,9 @@ 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, tc.args.o...)
+			r := NewReconciler(tc.args.m, tc.args.mg, resource.NewNopProviderConfigUsageCleaner(), 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, tc.args.o...)
+			r := NewReconciler(tc.args.m, tc.args.mg, resource.NewNopProviderConfigUsageCleaner(), tc.args.o...)
 			r.Reconcile(context.Background(), reconcile.Request{})
 
 			if diff := cmp.Diff(tc.want.callCount, len(tc.args.c.requests)); diff != "" {

pkg/reconciler/managed/reconciler_modern_test.go

@@ -43,11 +43,22 @@ import (
 
 var _ reconcile.Reconciler = &Reconciler{}
 
+func TestNewReconcilerRequiresProviderConfigUsageCleaner(t *testing.T) {
+	defer func() {
+		if recover() == nil {
+			t.Error("NewReconciler(...): expected a panic when the ProviderConfigUsageCleaner is nil")
+		}
+	}()
+
+	NewReconciler(nil, resource.ManagedKind{}, nil)
+}
+
 func TestModernReconciler(t *testing.T) {
 	type args struct {
-		m  manager.Manager
-		mg resource.ManagedKind
-		o  []ReconcilerOption
+		m     manager.Manager
+		mg    resource.ManagedKind
+		usage resource.ProviderConfigUsageCleaner
+		o     []ReconcilerOption
 	}
 
 	type want struct {
@@ -58,6 +69,7 @@ func TestModernReconciler(t *testing.T) {
 
 	errBoom := errors.New("boom")
 	now := metav1.Now()
+	finalizerRemoved := false
 
 	cases := map[string]struct {
 		reason string
@@ -180,9 +192,18 @@ func TestModernReconciler(t *testing.T) {
 					Scheme: fake.SchemeWith(&fake.ModernManaged{}),
 				},
 				mg: resource.ManagedKind(fake.GVK(&fake.ModernManaged{})),
+				usage: resource.ProviderConfigUsageCleanerFn(func(_ context.Context, _ resource.Managed) error {
+					if !finalizerRemoved {
+						t.Error("ProviderConfigUsage released before managed resource finalizer was removed")
+					}
+					return nil
+				}),
 				o: []ReconcilerOption{
 					WithManagementPolicies(),
-					WithFinalizer(resource.FinalizerFns{RemoveFinalizerFn: func(_ context.Context, _ resource.Object) error { return nil }}),
+					WithFinalizer(resource.FinalizerFns{RemoveFinalizerFn: func(_ context.Context, _ resource.Object) error {
+						finalizerRemoved = true
+						return nil
+					}}),
 				},
 			},
 			want: want{result: reconcile.Result{Requeue: false}},
@@ -2171,7 +2192,11 @@ func TestModernReconciler(t *testing.T) {
 
 	for name, tc := range cases {
 		t.Run(name, func(t *testing.T) {
-			r := NewReconciler(tc.args.m, tc.args.mg, tc.args.o...)
+			usage := tc.args.usage
+			if usage == nil {
+				usage = resource.NewNopProviderConfigUsageCleaner()
+			}
+			r := NewReconciler(tc.args.m, tc.args.mg, usage, tc.args.o...)
 
 			got, err := r.Reconcile(context.Background(), reconcile.Request{})
 			if diff := cmp.Diff(tc.want.err, err, test.EquateErrors()); diff != "" {
@@ -2837,7 +2862,7 @@ func TestReconcilerChangeLogs(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, tc.args.o...)
+			r := NewReconciler(tc.args.m, tc.args.mg, resource.NewNopProviderConfigUsageCleaner(), tc.args.o...)
 			r.Reconcile(context.Background(), reconcile.Request{})
 
 			if diff := cmp.Diff(tc.want.callCount, len(tc.args.c.requests)); diff != "" {
@@ -2981,7 +3006,7 @@ func TestReconcilePollIntervalAnnotation(t *testing.T) {
 					}),
 				},
 				Scheme: fake.SchemeWith(&fake.ModernManaged{}),
-			}, resource.ManagedKind(fake.GVK(&fake.ModernManaged{})),
+			}, resource.ManagedKind(fake.GVK(&fake.ModernManaged{})), resource.NewNopProviderConfigUsageCleaner(),
 				WithPollInterval(tc.pollInterval),
 				WithMinPollInterval(tc.minPollInterval),
 				WithInitializers(),
@@ -3070,7 +3095,7 @@ func TestReconcileRequestAnnotation(t *testing.T) {
 					}),
 				},
 				Scheme: fake.SchemeWith(&fake.ModernManaged{}),
-			}, resource.ManagedKind(fake.GVK(&fake.ModernManaged{})),
+			}, resource.ManagedKind(fake.GVK(&fake.ModernManaged{})), resource.NewNopProviderConfigUsageCleaner(),
 				WithInitializers(),
 				WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })),
 				WithExternalConnector(ExternalConnectorFn(func(_ context.Context, _ resource.Managed) (ExternalClient, error) {
@@ -3115,3 +3140,89 @@ func modernManagedMockGetFn(err error, generation int64) test.MockGetFn {
 		return nil
 	})
 }
+
+// TestReconcilePropagationFinalizersDoNotDelayDelete ensures Kubernetes
+// propagation finalizers do not prevent external deletion.
+func TestReconcilePropagationFinalizersDoNotDelayDelete(t *testing.T) {
+	errBoom := errors.New("boom")
+	now := metav1.Now()
+
+	cases := map[string]struct {
+		reason           string
+		finalizers       []string
+		wantDeleteCalled bool
+	}{
+		"ForegroundDeletion": {
+			reason:           "Kubernetes' foregroundDeletion finalizer must not delay the external delete.",
+			finalizers:       []string{FinalizerName, metav1.FinalizerDeleteDependents},
+			wantDeleteCalled: true,
+		},
+		"OrphanDependents": {
+			reason:           "Kubernetes' orphan finalizer must not delay the external delete.",
+			finalizers:       []string{FinalizerName, metav1.FinalizerOrphanDependents},
+			wantDeleteCalled: true,
+		},
+		"BothPropagationFinalizers": {
+			reason:           "Neither propagation finalizer must delay the external delete.",
+			finalizers:       []string{FinalizerName, metav1.FinalizerDeleteDependents, metav1.FinalizerOrphanDependents},
+			wantDeleteCalled: true,
+		},
+		"ForeignFinalizer": {
+			reason:           "A finalizer belonging to another controller must still delay the external delete.",
+			finalizers:       []string{FinalizerName, "example.org/another-controller"},
+			wantDeleteCalled: false,
+		},
+		"ForeignFinalizerAlongsidePropagation": {
+			reason:           "A foreign finalizer must delay the external delete even when a propagation finalizer is also present.",
+			finalizers:       []string{FinalizerName, metav1.FinalizerDeleteDependents, "example.org/another-controller"},
+			wantDeleteCalled: false,
+		},
+	}
+
+	for name, tc := range cases {
+		t.Run(name, func(t *testing.T) {
+			deleteCalled := false
+
+			m := &fake.Manager{
+				Client: &test.MockClient{
+					MockGet: test.NewMockGetFn(nil, func(obj client.Object) error {
+						mg := asModernManaged(obj, 42)
+						mg.SetDeletionTimestamp(&now)
+						mg.SetFinalizers(tc.finalizers)
+
+						return nil
+					}),
+					MockUpdate:       test.NewMockUpdateFn(nil),
+					MockStatusUpdate: test.NewMockSubResourceUpdateFn(nil),
+				},
+				Scheme: fake.SchemeWith(&fake.ModernManaged{}),
+			}
+
+			r := NewReconciler(m, resource.ManagedKind(fake.GVK(&fake.ModernManaged{})), resource.NewNopProviderConfigUsageCleaner(),
+				WithInitializers(),
+				WithReferenceResolver(ReferenceResolverFn(func(_ context.Context, _ resource.Managed) error { return nil })),
+				WithExternalConnector(ExternalConnectorFn(func(_ context.Context, _ resource.Managed) (ExternalClient, error) {
+					return &ExternalClientFns{
+						ObserveFn: func(_ context.Context, _ resource.Managed) (ExternalObservation, error) {
+							return ExternalObservation{ResourceExists: true}, nil
+						},
+						// Stop after recording whether Delete was called.
+						DeleteFn: func(_ context.Context, _ resource.Managed) (ExternalDelete, error) {
+							deleteCalled = true
+							return ExternalDelete{}, errBoom
+						},
+						DisconnectFn: func(_ context.Context) error { return nil },
+					}, nil
+				})),
+			)
+
+			if _, err := r.Reconcile(context.Background(), reconcile.Request{}); err != nil {
+				t.Errorf("%s\nr.Reconcile(...): unexpected error: %v", tc.reason, err)
+			}
+
+			if diff := cmp.Diff(tc.wantDeleteCalled, deleteCalled); diff != "" {
+				t.Errorf("%s\nr.Reconcile(...): -want external Delete called, +got:\n%s", tc.reason, diff)
+			}
+		})
+	}
+}