Better organize adding/removing finalizers

To be able to distinguish between updating just the status and updating the full resource.

Tested: Ran both unit-tests ('make test') and integration test ('make test-e2e') successfully.

Author: erikgb

internal/anchor/reconciler.go

@@ -26,6 +26,7 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -81,9 +82,9 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	// Anchors in unmanaged namespace should be ignored. Make sure it
 	// doesn't have any finalizers, otherwise, leave it alone.
 	if why := config.WhyUnmanaged(pnm); why != "" {
-		if len(inst.ObjectMeta.Finalizers) > 0 {
+		if controllerutil.ContainsFinalizer(inst, api.MetaGroup) {
 			log.Info("Removing finalizers from anchor in unmanaged namespace", "reason", why)
-			inst.ObjectMeta.Finalizers = nil
+			controllerutil.RemoveFinalizer(inst, api.MetaGroup)
 			return ctrl.Result{}, r.writeInstance(ctx, log, inst)
 		}
 		return ctrl.Result{}, nil
@@ -94,10 +95,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	// been bypassed and the anchor has been successfully created. Forbidden
 	// anchors won't have finalizers.
 	if why := config.WhyUnmanaged(nm); why != "" {
-		if inst.Status.State != api.Forbidden || len(inst.ObjectMeta.Finalizers) > 0 {
+		if inst.Status.State != api.Forbidden || controllerutil.ContainsFinalizer(inst, api.MetaGroup) {
 			log.Info("Setting forbidden state on anchor with unmanaged name", "reason", why)
 			inst.Status.State = api.Forbidden
-			inst.ObjectMeta.Finalizers = nil
+			controllerutil.RemoveFinalizer(inst, api.MetaGroup)
 			return ctrl.Result{}, r.writeInstance(ctx, log, inst)
 		}
 		return ctrl.Result{}, nil
@@ -130,9 +131,6 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		}
 	}
 
-	// Add finalizers on all non-forbidden anchors to ensure it's not deleted until
-	// after the subnamespace is deleted.
-	inst.ObjectMeta.Finalizers = []string{api.MetaGroup}
 	return ctrl.Result{}, r.writeInstance(ctx, log, inst)
 }
 
@@ -146,8 +144,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 // if allowCascadingDeletion is disabled but the subnamespace has descendants (see
 // shouldDeleteSubns for details). In such cases, we move straight to step 2.
 func (r *Reconciler) onDeleting(ctx context.Context, log logr.Logger, inst *api.SubnamespaceAnchor, snsInst *corev1.Namespace) (bool, error) {
-	// Early exit and continue reconciliation if the instance is not being deleted.
+	// Early exit, ensure finalizer is present and continue reconciliation if the instance is not being deleted.
 	if inst.DeletionTimestamp.IsZero() {
+		// Add finalizers on all non-forbidden anchors to ensure it's not deleted until
+		// after the subnamespace is deleted.
+		if !controllerutil.ContainsFinalizer(inst, api.MetaGroup) {
+			controllerutil.AddFinalizer(inst, api.MetaGroup)
+		}
 		return false, nil
 	}
 
@@ -168,13 +171,16 @@ func (r *Reconciler) onDeleting(ctx context.Context, log logr.Logger, inst *api.
 		log.Info("Deleting subnamespace due to anchor being deleted")
 		return true, r.deleteNamespace(ctx, log, snsInst)
 	case r.shouldFinalizeAnchor(log, inst, snsInst):
-		log.V(1).Info("Unblocking deletion") // V(1) since we'll very shortly show an "anchor deleted" message
-		inst.ObjectMeta.Finalizers = nil
-		return true, r.writeInstance(ctx, log, inst)
+		if controllerutil.ContainsFinalizer(inst, api.MetaGroup) {
+			log.V(1).Info("Unblocking deletion") // V(1) since we'll very shortly show an "anchor deleted" message
+			controllerutil.RemoveFinalizer(inst, api.MetaGroup)
+			return true, r.writeInstance(ctx, log, inst)
+		}
+		return true, nil
 	default:
 		// There's nothing to do; we're just waiting for something to happen. Print out a log message
 		// indicating what we're waiting for.
-		if len(inst.ObjectMeta.Finalizers) > 0 {
+		if controllerutil.ContainsFinalizer(inst, api.MetaGroup) {
 			log.Info("Waiting for subnamespace to be fully purged before letting the anchor be deleted")
 		} else {
 			// I doubt we'll ever get here but I suppose it's possible
@@ -213,7 +219,7 @@ func (r *Reconciler) shouldDeleteSubns(log logr.Logger, inst *api.SubnamespaceAn
 			log.V(1).Info("The subnamespace is already being deleted; no need to delete again")
 			return false
 		}
-		if len(inst.ObjectMeta.Finalizers) == 0 {
+		if !controllerutil.ContainsFinalizer(inst, api.MetaGroup) {
 			log.V(1).Info("The anchor has already been finalized; do not reconsider deleting the namespace")
 			return false
 		}
@@ -257,7 +263,7 @@ func (r *Reconciler) shouldDeleteSubns(log logr.Logger, inst *api.SubnamespaceAn
 // deleted, it's in the process of being deleted, etc).
 func (r *Reconciler) shouldFinalizeAnchor(log logr.Logger, inst *api.SubnamespaceAnchor, snsInst *corev1.Namespace) bool {
 	// If the anchor is already finalized, there's no need to do it again.
-	if len(inst.ObjectMeta.Finalizers) == 0 {
+	if !controllerutil.ContainsFinalizer(inst, api.MetaGroup) {
 		return false
 	}
 

internal/hierarchyconfig/reconciler.go

@@ -30,6 +30,7 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller"
+	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 	"sigs.k8s.io/controller-runtime/pkg/handler"
 	"sigs.k8s.io/controller-runtime/pkg/reconcile"
@@ -133,9 +134,9 @@ func (r *Reconciler) handleUnmanaged(ctx context.Context, log logr.Logger, nm st
 	// Remove the finalizers if there are any. Note that while getSingleton can return an invalid
 	// object (i.e. one without a name or namespace, if there's no singleton on the server), it can
 	// never do that if there are finalizers or children so this is safe to write back.
-	if len(inst.ObjectMeta.Finalizers) > 0 || len(inst.Status.Children) > 0 {
+	if controllerutil.ContainsFinalizer(inst, api.FinalizerHasSubnamespace) || len(inst.Status.Children) > 0 {
 		log.Info("Removing finalizers and children on unmanaged singleton")
-		inst.ObjectMeta.Finalizers = nil
+		controllerutil.RemoveFinalizer(inst, api.FinalizerHasSubnamespace)
 		inst.Status.Children = nil
 		stats.WriteHierConfig()
 		if err := r.Update(ctx, inst); err != nil {
@@ -241,27 +242,27 @@ func (r *Reconciler) addIncludedNamespaceLabel(log logr.Logger, nsInst *corev1.N
 // .spec.allowCascadingDeletion field, so if we allowed this object to be deleted before all
 // descendants have been deleted, we would lose the knowledge that cascading deletion is enabled.
 func (r *Reconciler) updateFinalizers(log logr.Logger, inst *api.HierarchyConfiguration, nsInst *corev1.Namespace, anms []string) {
-	// No-one should put a finalizer on a hierarchy config except us. See
-	// https://github.com/kubernetes-sigs/hierarchical-namespaces/issues/623 as we try to enforce that.
 	switch {
 	case len(anms) == 0:
 		// There are no subnamespaces in this namespace. The HC instance can be safely deleted anytime.
-		if len(inst.ObjectMeta.Finalizers) > 0 {
+		if controllerutil.ContainsFinalizer(inst, api.FinalizerHasSubnamespace) {
 			log.V(1).Info("Removing finalizers since there are no longer any anchors in the namespace.")
+			controllerutil.RemoveFinalizer(inst, api.FinalizerHasSubnamespace)
 		}
-		inst.ObjectMeta.Finalizers = nil
 	case !inst.DeletionTimestamp.IsZero() && nsInst.DeletionTimestamp.IsZero():
 		// If the HC instance is being deleted but not the namespace (which means
 		// it's not a cascading delete), remove the finalizers to let it go through.
 		// This is the only case the finalizers can be removed even when the
 		// namespace has subnamespaces. (A default HC will be recreated later.)
-		log.Info("Removing finalizers to allow a single deletion of the singleton (not involved in a cascading deletion).")
-		inst.ObjectMeta.Finalizers = nil
+		if controllerutil.ContainsFinalizer(inst, api.FinalizerHasSubnamespace) {
+			log.Info("Removing finalizers to allow a single deletion of the singleton (not involved in a cascading deletion).")
+			controllerutil.RemoveFinalizer(inst, api.FinalizerHasSubnamespace)
+		}
 	default:
-		if len(inst.ObjectMeta.Finalizers) == 0 {
+		if !controllerutil.ContainsFinalizer(inst, api.FinalizerHasSubnamespace) {
 			log.Info("Adding finalizers since there's at least one anchor in the namespace.")
+			controllerutil.AddFinalizer(inst, api.FinalizerHasSubnamespace)
 		}
-		inst.ObjectMeta.Finalizers = []string{api.FinalizerHasSubnamespace}
 	}
 }