Skip to content

Protect ProviderConfigs while managed resources terminate #1113

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
ezgidemirel wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Protect ProviderConfigs while managed resources terminate #1113

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
34d7896
Protect ProviderConfigs while managed resources terminate
ezgidemirel Aug 10, 2026
0fa133e
Move the ProviderConfigUsage finalizer into the managed reconciler
ezgidemirel Sep 2, 2026
0d4f5d6
Protect ProviderConfigUsages by default
ezgidemirel Sep 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions pkg/meta/meta.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Comment thread
ezgidemirel marked this conversation as resolved.
Outdated
func FinalizersExcludingPropagation(o metav1.Object) []string {
Comment thread
ezgidemirel marked this conversation as resolved.
Outdated
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()
Expand Down
30 changes: 30 additions & 0 deletions pkg/meta/meta_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,6 +605,36 @@ func TestFinalizerExists(t *testing.T) {
}
}

func TestFinalizersExcludingPropagation(t *testing.T) {
cases := map[string]struct {
reason string
o metav1.Object
want []string
}{
"NoFinalizers": {
reason: "An object without finalizers has no finalizers after propagation finalizers are excluded.",
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{
metav1.FinalizerDeleteDependents,
metav1.FinalizerOrphanDependents,
}}},
want: []string{},
},
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}

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

func TestAddLabels(t *testing.T) {
key, value := "key", "value"
existingKey, existingValue := "ekey", "evalue"
Expand Down
53 changes: 46 additions & 7 deletions pkg/reconciler/managed/reconciler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"

Expand Down Expand Up @@ -615,6 +616,7 @@ type mrManaged struct {
Initializer
ReferenceResolver
LocalConnectionPublisher
resource.ProviderConfigUsageCleaner
}

func defaultMRManaged(m manager.Manager) mrManaged {
Expand Down Expand Up @@ -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 {

@ulucinar ulucinar Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about making usage a ReconcilerOption? managed.NewReconciler then becomes a variadic function, providers not participating will not set the option (e.g., WithProviderConfigUsageCleaner) and then we can default to the new NewNopProviderConfigUsageCleaner in defaultMRManaged in case the provider does not specify the option.

We would then remove the nil check and the panic below.

@ezgidemirel ezgidemirel Sep 2, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. NewReconciler is back to (m, of, o ...ReconcilerOption), defaultMRManaged sets NewNopProviderConfigUsageCleaner(), and the nil check and panic are gone. This also removes the compile-time break for every provider, which was the main reason I'd made it required

if usage == nil {
panic("managed reconciler requires a ProviderConfigUsageCleaner")
}

Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
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)
Expand All @@ -907,6 +915,7 @@ func NewReconciler(m manager.Manager, of resource.ManagedKind, o ...ReconcilerOp
change: newNopChangeLogger(),
conditions: new(conditions.ObservedGenerationPropagationManager),
}
r.managed.ProviderConfigUsageCleaner = usage

@ulucinar ulucinar Aug 28, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may consider moving this to a ReconcilerOption, e.g., WithProviderConfigUsageCleaner (similar to, for example, WithFinalizer). Please see the comments above.

@ezgidemirel ezgidemirel Sep 2, 2026

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done, WithProviderConfigUsageCleaner. Its doc comment explains that the reconciler owns both ends of the finalizer and that the cleaner need not be the same tracker instance the connector uses, only one
built with the same client and usage type


for _, ro := range o {
ro(r)
Expand Down Expand Up @@ -1070,6 +1079,18 @@ 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)
Comment thread
ezgidemirel marked this conversation as resolved.
uerr := errors.Wrap(err, errUntrackProviderConfig)
status.MarkConditions(xpv2.Deleting(), xpv2.ReconcileError(uerr))
if err := updateStatus(); err != nil {
return reconcile.Result{Requeue: true}, errors.Wrap(err, errUpdateManagedStatus)
}
return reconcile.Result{Requeue: true}, uerr
}

Comment thread
coderabbitai[bot] marked this conversation as resolved.
// 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
Expand Down Expand Up @@ -1231,7 +1252,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")
Expand Down Expand Up @@ -1311,6 +1332,18 @@ 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)
uerr := errors.Wrap(err, errUntrackProviderConfig)
status.MarkConditions(xpv2.Deleting(), xpv2.ReconcileError(uerr))
if err := updateStatus(); err != nil {
return reconcile.Result{Requeue: true}, errors.Wrap(err, errUpdateManagedStatus)
}
return reconcile.Result{Requeue: true}, uerr
}

// 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
Expand Down Expand Up @@ -1576,3 +1609,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 {
Comment thread
ezgidemirel marked this conversation as resolved.
Outdated
return len(meta.FinalizersExcludingPropagation(mg))
}
4 changes: 2 additions & 2 deletions pkg/reconciler/managed/reconciler_legacy_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 != "" {
Expand Down Expand Up @@ -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 != "" {
Expand Down
Loading
Loading