From ab69e4ff39930d1417affaebfb90daf2ee9cd9c3 Mon Sep 17 00:00:00 2001 From: Lukas Krejci Date: Wed, 11 Jun 2025 14:29:36 +0200 Subject: [PATCH] introduce a simple abstraction for easier handling of finalizers --- pkg/finalizers/finalizers.go | 83 +++++++++++++++++++ pkg/finalizers/finalizers_test.go | 130 ++++++++++++++++++++++++++++++ 2 files changed, 213 insertions(+) create mode 100644 pkg/finalizers/finalizers.go create mode 100644 pkg/finalizers/finalizers_test.go diff --git a/pkg/finalizers/finalizers.go b/pkg/finalizers/finalizers.go new file mode 100644 index 00000000..35b90314 --- /dev/null +++ b/pkg/finalizers/finalizers.go @@ -0,0 +1,83 @@ +package finalizers + +import ( + "context" + "errors" + + toolchainv1alpha1 "github.com/codeready-toolchain/api/api/v1alpha1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/finalizer" +) + +// Finalizers is a simple wrapper around finalizer.Finalizers that can also perform the actual update +// of the resource based on the finalization result. +type Finalizers struct { + // finalizers is the implementation of the finalizer control logic implemented in the controller runtime. + finalizers finalizer.Finalizers +} + +// FinalizerFunc is a functional implementation of the finalizer.Finalizer interface. +type FinalizerFunc func(context.Context, client.Object) (finalizer.Result, error) + +// RegisterWithStandardName registers the provided finalizer with the standard finalizer name defined in +// toolchainv1alpha1. +func (fs *Finalizers) RegisterWithStandardName(f finalizer.Finalizer) error { + return fs.Register(toolchainv1alpha1.FinalizerName, f) +} + +// Register registers the finalizer so that FinalizeAndUpdate will call it. Note that the finalizer MUST +// return an error if the condition for removing it from the object is not satisfied. +func (fs *Finalizers) Register(key string, f finalizer.Finalizer) error { + fs.ensureInitialized() + return fs.finalizers.Register(key, f) +} + +// FinalizeAndUpdate runs the registered finalizers on the object and reports true if the object or its status +// has been updated in the cluster using the provided client. +// +// The result of calling this method on an object that is not being deleted is that all the registered finalizers +// are added to the set of the finalizers on the object and the object is updated in the cluster (and therefore true +// is returned if the finalizers were added). +// +// The result of calling this method on an object that is being deleted is that the all the registered finalizers are +// called and the finalizers are removed if they succeed (i.e. they don't return an error). +// +// Note also, that, given the logic described above, there is no need to check for the object's deletion timestamp during +// the reconciliation. Returning early from the reconciler when this method returns true (or an error) is the correct +// thing to do in all cases. +func (f *Finalizers) FinalizeAndUpdate(ctx context.Context, cl client.Client, obj client.Object) (bool, error) { + f.ensureInitialized() + + res, err := f.finalizers.Finalize(ctx, obj) + + var errs []error + + if err != nil { + errs = append(errs, err) + } + + if res.Updated { + if err := cl.Update(ctx, obj); err != nil { + errs = append(errs, err) + } + } + if res.StatusUpdated { + if err := cl.Status().Update(ctx, obj); err != nil { + errs = append(errs, err) + } + } + + return res.Updated || res.StatusUpdated, errors.Join(errs...) +} + +func (f *Finalizers) ensureInitialized() { + if f.finalizers == nil { + f.finalizers = finalizer.NewFinalizers() + } +} + +func (f FinalizerFunc) Finalize(ctx context.Context, obj client.Object) (finalizer.Result, error) { + return f(ctx, obj) +} + +var _ finalizer.Finalizer = (FinalizerFunc)(nil) diff --git a/pkg/finalizers/finalizers_test.go b/pkg/finalizers/finalizers_test.go new file mode 100644 index 00000000..f25bf48a --- /dev/null +++ b/pkg/finalizers/finalizers_test.go @@ -0,0 +1,130 @@ +package finalizers + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/codeready-toolchain/toolchain-common/pkg/test" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/finalizer" +) + +func TestFinalizers(t *testing.T) { + t.Run("adds a finalizer on non-deleted", func(t *testing.T) { + // given + var fs Finalizers + require.NoError(t, fs.Register("dummy", FinalizerFunc(func(ctx context.Context, o client.Object) (finalizer.Result, error) { + return finalizer.Result{}, nil + }))) + obj := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cm", + Namespace: test.HostOperatorNs, + }, + } + cl := fake.NewClientBuilder().WithObjects(obj).Build() + + // when + updated, err := fs.FinalizeAndUpdate(context.TODO(), cl, obj) + inCluster := &corev1.ConfigMap{} + require.NoError(t, cl.Get(context.TODO(), client.ObjectKeyFromObject(obj), inCluster)) + + // then + require.NoError(t, err) + assert.True(t, updated) + assert.Contains(t, inCluster.Finalizers, "dummy") + }) + + t.Run("does not modify when finalizer already present", func(t *testing.T) { + // given + var fs Finalizers + require.NoError(t, fs.Register("dummy", FinalizerFunc(func(ctx context.Context, o client.Object) (finalizer.Result, error) { + return finalizer.Result{}, nil + }))) + obj := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cm", + Namespace: test.HostOperatorNs, + Finalizers: []string{"dummy"}, + }, + } + cl := fake.NewClientBuilder().WithObjects(obj).Build() + + // when + updated, err := fs.FinalizeAndUpdate(context.TODO(), cl, obj) + inCluster := &corev1.ConfigMap{} + require.NoError(t, cl.Get(context.TODO(), client.ObjectKeyFromObject(obj), inCluster)) + + // then + require.NoError(t, err) + assert.False(t, updated) + assert.Contains(t, inCluster.Finalizers, "dummy") + }) + + t.Run("removes the finalizer when it runs successfully on deleted object", func(t *testing.T) { + // given + var fs Finalizers + + require.NoError(t, fs.Register("dummy", FinalizerFunc(func(ctx context.Context, o client.Object) (finalizer.Result, error) { + return finalizer.Result{}, nil + }))) + obj := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cm", + Namespace: test.HostOperatorNs, + DeletionTimestamp: &metav1.Time{Time: time.Now()}, + Finalizers: []string{"dummy", "other"}, + }, + } + cl := fake.NewClientBuilder().WithObjects(obj).Build() + + // when + updated, err := fs.FinalizeAndUpdate(context.TODO(), cl, obj) + inCluster := &corev1.ConfigMap{} + require.NoError(t, cl.Get(context.TODO(), client.ObjectKeyFromObject(obj), inCluster)) + + // then + require.NoError(t, err) + assert.True(t, updated) + assert.Len(t, inCluster.Finalizers, 1) + assert.Contains(t, inCluster.Finalizers, "other") + }) + + t.Run("updates even on error", func(t *testing.T) { + // given + var fs Finalizers + + require.NoError(t, fs.Register("dummy", FinalizerFunc(func(ctx context.Context, o client.Object) (finalizer.Result, error) { + cm := o.(*corev1.ConfigMap) + cm.Data = map[string]string{"key": "value"} + + return finalizer.Result{Updated: true}, fmt.Errorf("intentional error") + }))) + obj := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "cm", + Namespace: test.HostOperatorNs, + DeletionTimestamp: &metav1.Time{Time: time.Now()}, + Finalizers: []string{"dummy"}, + }, + } + cl := fake.NewClientBuilder().WithObjects(obj).Build() + + // when + updated, err := fs.FinalizeAndUpdate(context.TODO(), cl, obj) + inCluster := &corev1.ConfigMap{} + require.NoError(t, cl.Get(context.TODO(), client.ObjectKeyFromObject(obj), inCluster)) + + // then + require.Error(t, err) + assert.True(t, updated) + assert.Equal(t, "value", inCluster.Data["key"]) + }) +}