Add a finalizer to the NSTemplateTier to prevent its deletion (#1182)

Author: metlos

controllers/nstemplatetier/nstemplatetier_controller.go

@@ -10,9 +10,10 @@ import (
 	"github.com/codeready-toolchain/host-operator/controllers/toolchainconfig"
 	"github.com/codeready-toolchain/host-operator/pkg/templates/nstemplatetiers"
 	"github.com/codeready-toolchain/toolchain-common/pkg/condition"
-	"github.com/redhat-cop/operator-utils/pkg/util"
+	"github.com/codeready-toolchain/toolchain-common/pkg/hash"
 	corev1 "k8s.io/api/core/v1"
 
+	"github.com/redhat-cop/operator-utils/pkg/util"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -62,9 +63,11 @@ func (r *Reconciler) Reconcile(ctx context.Context, request ctrl.Request) (ctrl.
 		logger.Error(err, "unable to get the current NSTemplateTier")
 		return reconcile.Result{}, fmt.Errorf("unable to get the current NSTemplateTier: %w", err)
 	}
-	// if the NSTemplateTier is being deleted, then do nothing
 	if util.IsBeingDeleted(tier) {
-		return reconcile.Result{}, nil
+		return reconcile.Result{}, r.handleDeletion(ctx, tier)
+	}
+	if err := r.addFinalizer(ctx, tier); err != nil {
+		return reconcile.Result{}, err
 	}
 
 	_, err := toolchainconfig.GetToolchainConfig(r.Client)
@@ -183,7 +186,6 @@ func (r *Reconciler) ensureTTRforTemplate(ctx context.Context, nsTmplTier *toolc
 		return false, "", err
 	}
 	return true, ttrName, nil
-
 }
 
 func (r *Reconciler) createNewTierTemplateRevision(ctx context.Context, nsTmplTier *toolchainv1alpha1.NSTemplateTier, tierTemplate *toolchainv1alpha1.TierTemplate) (string, error) {
@@ -255,6 +257,56 @@ func (r *Reconciler) updateStatus(ctx context.Context, tmplTier *toolchainv1alph
 	return r.Client.Status().Update(ctx, tmplTier)
 }
 
+func (r *Reconciler) handleDeletion(ctx context.Context, tier *toolchainv1alpha1.NSTemplateTier) error {
+	list := &toolchainv1alpha1.SpaceList{}
+	if err := r.Client.List(ctx, list,
+		runtimeclient.InNamespace(tier.GetNamespace()),
+		runtimeclient.HasLabels{hash.TemplateTierHashLabelKey(tier.GetName())}); err != nil {
+		return err
+	}
+
+	hasSpaces := false
+
+	for _, s := range list.Items {
+		if util.IsBeingDeleted(&s) {
+			// As an optimization, we skip the spaces that are being deleted.
+			// There is no functional relationship between the spaces and the tier, we just don't want
+			// there to be spaces that refer to a non-existing tier. So if the space is being deleted,
+			// we can pretend like it doesn't already exist.
+			//
+			// This speeds up the deletion of the tier because it doesn't linger around for the spaces
+			// that are being deleted (which can take a good amount of time).
+			continue
+		}
+		// we found a space that is not being deleted and that references our tier.
+		hasSpaces = true
+		break
+	}
+
+	if hasSpaces {
+		// leave the finalizer in place - it's still being used
+		return fmt.Errorf("NSTemplateTier is still used by some spaces")
+	}
+
+	// Remove finalizer from the NSTemplateTier, it's no longer used anywhere
+	// so we can let the cluster proceed with the deletion.
+	util.RemoveFinalizer(tier, toolchainv1alpha1.FinalizerName)
+	return r.Client.Update(ctx, tier)
+}
+
+func (r *Reconciler) addFinalizer(ctx context.Context, tier *toolchainv1alpha1.NSTemplateTier) error {
+	// Add the finalizer if it is not present
+	if !util.HasFinalizer(tier, toolchainv1alpha1.FinalizerName) {
+		logger := log.FromContext(ctx)
+		logger.Info("adding finalizer on NSTemplateTier")
+		util.AddFinalizer(tier, toolchainv1alpha1.FinalizerName)
+		if err := r.Client.Update(ctx, tier); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
 func FailedCondition(reason string, cause string) toolchainv1alpha1.Condition {
 	return toolchainv1alpha1.Condition{
 		Type:    toolchainv1alpha1.ConditionReady,

controllers/nstemplatetier/nstemplatetier_controller_test.go

@@ -13,6 +13,7 @@ import (
 	"github.com/codeready-toolchain/host-operator/pkg/apis"
 	tiertest "github.com/codeready-toolchain/host-operator/test/nstemplatetier"
 	"github.com/codeready-toolchain/host-operator/test/tiertemplaterevision"
+	"github.com/codeready-toolchain/toolchain-common/pkg/hash"
 	"github.com/codeready-toolchain/toolchain-common/pkg/test"
 	templatev1 "github.com/openshift/api/template/v1"
 	"github.com/stretchr/testify/assert"
@@ -27,6 +28,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation"
 	"k8s.io/client-go/kubernetes/scheme"
 	runtimeclient "sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
 )
 
@@ -37,9 +39,7 @@ const (
 func TestReconcile(t *testing.T) {
 	// given
 	t.Run("failures", func(t *testing.T) {
-
 		t.Run("unable to get NSTemplateTier", func(t *testing.T) {
-
 			t.Run("tier not found", func(t *testing.T) {
 				// given
 				base1nsTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates)
@@ -75,12 +75,11 @@ func TestReconcile(t *testing.T) {
 				assert.Equal(t, reconcile.Result{}, res) // no explicit requeue
 			})
 		})
-
 	})
 
 	t.Run("revisions management", func(t *testing.T) {
 		// given
-		base1nsTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates,
+		base1nsTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithFinalizer(),
 			// the tiertemplate revision CR should have a copy of those parameters
 			tiertest.WithParameter("DEPLOYMENT_QUOTA", "60"),
 		)
@@ -194,15 +193,14 @@ func TestReconcile(t *testing.T) {
 						require.Len(t, ttrs.Items, len(tierTemplatesRefs)) // it's one TTR per each tiertemplate in the NSTemplateTier
 					})
 				})
-
 			})
 
 			t.Run("revision field is set but TierTemplateRevision CRs are missing, they should be created", func(t *testing.T) {
 				// given
 				// the NSTemplateTier has already the status.revisions field populated
 				// but the TierTemplateRevision CRs are missing
 				tierTemplates := initTierTemplates(t, withTemplateObjects(crq), base1nsTier.Name)
-				base1nsTierWithRevisions := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates,
+				base1nsTierWithRevisions := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithFinalizer(),
 					// the tiertemplate revision CR should have a copy of those parameters
 					tiertest.WithParameter("DEPLOYMENT_QUOTA", "60"),
 				)
@@ -240,7 +238,7 @@ func TestReconcile(t *testing.T) {
 				// the TierTemplateRevision CRs are missing, and their name are based on the tier name.
 				// Making the TierName already 63chars long we test that the TTR name stays within 63 chars
 				veryLongTierName := "somerandomstringtomakethenamelongerthan63chars12345678912345678"
-				tierWithVeryLongName := tiertest.Tier(t, veryLongTierName, tiertest.NSTemplateTierSpecWithTierName(veryLongTierName),
+				tierWithVeryLongName := tiertest.Tier(t, veryLongTierName, tiertest.NSTemplateTierSpecWithTierName(veryLongTierName), tiertest.WithFinalizer(),
 					tiertest.WithParameter("DEPLOYMENT_QUOTA", "60"),
 				)
 				tierTemplatesWithLongNames := initTierTemplates(t, withTemplateObjects(crq), tierWithVeryLongName.Name)
@@ -264,7 +262,6 @@ func TestReconcile(t *testing.T) {
 			})
 
 			t.Run("errors", func(t *testing.T) {
-
 				t.Run("error when TierTemplate is missing ", func(t *testing.T) {
 					// given
 					// make sure revisions field is nill before starting the test
@@ -285,46 +282,69 @@ func TestReconcile(t *testing.T) {
 					// and the TierTemplateRevision CRs are not created
 					tiertemplaterevision.AssertThatTTRs(t, cl, base1nsTier.GetNamespace()).DoNotExist()
 				})
-
 			})
-
 		})
 
-		t.Run("if being deleted, then do nothing", func(t *testing.T) {
+		t.Run("block deletion if used", func(t *testing.T) {
 			// given
 			tierBeingDeleted := base1nsTier.DeepCopy()
 			tierBeingDeleted.DeletionTimestamp = &metav1.Time{Time: time.Now()}
-			tierBeingDeleted.Finalizers = []string{"dummy"}
-			r, req, cl := prepareReconcile(t, tierBeingDeleted.Name, tierBeingDeleted)
-			called := false
-			cl.MockGet = func(ctx context.Context, key runtimeclient.ObjectKey, obj runtimeclient.Object, opts ...runtimeclient.GetOption) error {
-				if called {
-					return fmt.Errorf("should not call Get more than once")
-				}
-				called = true
-				return cl.Client.Get(ctx, key, obj, opts...)
-			}
-			cl.MockCreate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.CreateOption) error {
-				return fmt.Errorf("should not call Create")
+			controllerutil.AddFinalizer(tierBeingDeleted, toolchainv1alpha1.FinalizerName)
+			space := &toolchainv1alpha1.Space{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "test-space",
+					Namespace: tierBeingDeleted.Namespace,
+					Labels:    map[string]string{hash.TemplateTierHashLabelKey(tierBeingDeleted.Name): "1234"},
+				},
 			}
-			cl.MockStatusUpdate = func(ctx context.Context, obj runtimeclient.Object, opts ...runtimeclient.SubResourceUpdateOption) error {
-				return fmt.Errorf("should not call StatusUpdate")
+
+			r, req, cl := prepareReconcile(t, tierBeingDeleted.Name, tierBeingDeleted, space)
+
+			// when
+			res, err := r.Reconcile(context.TODO(), req)
+			inCluster := &toolchainv1alpha1.NSTemplateTier{}
+			gerr := cl.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(tierBeingDeleted), inCluster)
+
+			// then
+			require.Error(t, err)
+			require.Empty(t, res)
+			require.NoError(t, gerr)
+			require.Len(t, inCluster.Finalizers, 1)
+			require.Contains(t, inCluster.Finalizers, toolchainv1alpha1.FinalizerName)
+		})
+		t.Run("space being deleted doesn't block tier deletion", func(t *testing.T) {
+			// given
+			tierBeingDeleted := base1nsTier.DeepCopy()
+			tierBeingDeleted.DeletionTimestamp = &metav1.Time{Time: time.Now()}
+			controllerutil.AddFinalizer(tierBeingDeleted, toolchainv1alpha1.FinalizerName)
+			space := &toolchainv1alpha1.Space{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:              "test-space",
+					Namespace:         tierBeingDeleted.Namespace,
+					Labels:            map[string]string{hash.TemplateTierHashLabelKey(tierBeingDeleted.Name): "1234"},
+					DeletionTimestamp: &metav1.Time{Time: time.Now()},
+					Finalizers:        []string{"dummy"},
+				},
 			}
 
+			r, req, cl := prepareReconcile(t, tierBeingDeleted.Name, tierBeingDeleted, space)
+
 			// when
 			res, err := r.Reconcile(context.TODO(), req)
+			inCluster := &toolchainv1alpha1.NSTemplateTier{}
+			gerr := cl.Get(context.TODO(), runtimeclient.ObjectKeyFromObject(tierBeingDeleted), inCluster)
 
 			// then
 			require.NoError(t, err)
-			assert.Empty(t, res.Requeue)
+			require.Empty(t, res)
+			require.True(t, errors.IsNotFound(gerr))
 		})
 	})
-
 }
 
 func TestUpdateNSTemplateTier(t *testing.T) {
 	// given
-	base1nsTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates,
+	base1nsTier := tiertest.Base1nsTier(t, tiertest.CurrentBase1nsTemplates, tiertest.WithFinalizer(),
 		// the tiertemplate revision CR should have a copy of those parameters
 		tiertest.WithParameter("DEPLOYMENT_QUOTA", "60"),
 	)
@@ -454,13 +474,11 @@ func TestUpdateNSTemplateTier(t *testing.T) {
 				HasStatusTierTemplateRevisions(tierTemplatesRefs).Tier()
 			require.NotEqual(t, tierBeingUpdated.Status.Revisions, newNSTmplTier.Status.Revisions)
 		})
-
 	})
-
 }
 
 func newTestCRQ(podsCount string) unstructured.Unstructured {
-	var crq = unstructured.Unstructured{Object: map[string]interface{}{
+	crq := unstructured.Unstructured{Object: map[string]interface{}{
 		"kind": "ClusterResourceQuota",
 		"metadata": map[string]interface{}{
 			"name": "for-{{.SPACE_NAME}}-deployments",
@@ -512,6 +530,7 @@ func prepareReconcile(t *testing.T, name string, initObjs ...runtimeclient.Objec
 		Client: cl,
 		Scheme: s,
 	}
+
 	return r, reconcile.Request{
 		NamespacedName: types.NamespacedName{
 			Name:      name,

go.mod

@@ -2,7 +2,7 @@ module github.com/codeready-toolchain/host-operator
 
 require (
 	cloud.google.com/go/recaptchaenterprise/v2 v2.13.0
-	github.com/codeready-toolchain/api v0.0.0-20250506092100-39b4862e1271
+	github.com/codeready-toolchain/api v0.0.0-20250605152105-383ffe6cac27
 	github.com/codeready-toolchain/toolchain-common v0.0.0-20250506093954-2b65ad3a2e12
 	github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect
 	github.com/go-logr/logr v1.4.2

go.sum

@@ -38,8 +38,8 @@ github.com/cloudflare/circl v1.1.0/go.mod h1:prBCrKB9DV4poKZY1l9zBXg2QJY7mvgRvtM
 github.com/cloudflare/circl v1.6.1 h1:zqIqSPIndyBh1bjLVVDHMPpVKqp8Su/V+6MeDzzQBQ0=
 github.com/cloudflare/circl v1.6.1/go.mod h1:uddAzsPgqdMAYatqJ0lsjX1oECcQLIlRpzZh3pJrofs=
 github.com/cncf/udpa/go v0.0.0-20191209042840-269d4d468f6f/go.mod h1:M8M6+tZqaGXZJjfX53e64911xZQV5JYwmTeXPW+k8Sc=
-github.com/codeready-toolchain/api v0.0.0-20250506092100-39b4862e1271 h1:AHrFr/aPuJ4+0zHw4sFXcfMA92kChy12JAPS5bLODlw=
-github.com/codeready-toolchain/api v0.0.0-20250506092100-39b4862e1271/go.mod h1:20258od6i5+jP406Z76YaI2ow/vc7URwsDU2bokpkRE=
+github.com/codeready-toolchain/api v0.0.0-20250605152105-383ffe6cac27 h1:g1ivSPPHTC96RHp8S/gRmqODWgoHyivq+/d5kSI0pEs=
+github.com/codeready-toolchain/api v0.0.0-20250605152105-383ffe6cac27/go.mod h1:20258od6i5+jP406Z76YaI2ow/vc7URwsDU2bokpkRE=
 github.com/codeready-toolchain/toolchain-common v0.0.0-20250506093954-2b65ad3a2e12 h1:w54sojJJ8PsHZzK1mC+/EUBrQ9F2sC/k7JUVc8LSqK4=
 github.com/codeready-toolchain/toolchain-common v0.0.0-20250506093954-2b65ad3a2e12/go.mod h1:TrMvD0sP69wI6Rouzfs7OsOUSj4CGn/ZiIdiDBAFQjk=
 github.com/creack/pty v1.1.9/go.mod h1:oKZEueFk5CKHvIhNR5MUki03XCEU+Q6VDXinZuGJ33E=

test/nstemplatetier/nstemplatetier.go

@@ -8,6 +8,7 @@ import (
 	"github.com/codeready-toolchain/toolchain-common/pkg/hash"
 	"github.com/codeready-toolchain/toolchain-common/pkg/test"
 	"github.com/codeready-toolchain/toolchain-common/pkg/test/nstemplateset"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
 	"github.com/stretchr/testify/require"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -245,6 +246,12 @@ func WithParameter(name, value string) TierOption {
 	}
 }
 
+func WithFinalizer() TierOption {
+	return func(nt *toolchainv1alpha1.NSTemplateTier) {
+		controllerutil.AddFinalizer(nt, toolchainv1alpha1.FinalizerName)
+	}
+}
+
 // OtherTier returns an "other" NSTemplateTier
 func OtherTier(t *testing.T, options ...TierOption) *toolchainv1alpha1.NSTemplateTier {
 	return Tier(t, "other", toolchainv1alpha1.NSTemplateTierSpec{