diff --git a/internal/anchor/reconciler.go b/internal/anchor/reconciler.go index 1c7b625c1..2336f8dc5 100644 --- a/internal/anchor/reconciler.go +++ b/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 @@ -113,9 +114,13 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu r.updateState(log, inst, snsInst) // Handle the case where the anchor is being deleted. - if deleting, err := r.onDeleting(ctx, log, inst, snsInst); deleting { - return ctrl.Result{}, err + if !inst.DeletionTimestamp.IsZero() { + // Stop reconciliation as anchor is being deleted + return ctrl.Result{}, r.onDeleting(ctx, log, inst, snsInst) } + // Add finalizers on all non-forbidden anchors to ensure it's not deleted until + // after the subnamespace is deleted. + controllerutil.AddFinalizer(inst, api.MetaGroup) // If the subnamespace doesn't exist, create it. if inst.Status.State == api.Missing { @@ -130,9 +135,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) } @@ -145,19 +147,14 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu // There are several conditions where we skip step 1 - for example, if we're uninstalling HNC, or // 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. - if inst.DeletionTimestamp.IsZero() { - return false, nil - } - +func (r *Reconciler) onDeleting(ctx context.Context, log logr.Logger, inst *api.SubnamespaceAnchor, snsInst *corev1.Namespace) error { // We handle deletions differently depending on whether _one_ anchor is being deleted (i.e., the // user wants to delete the namespace) or whether the Anchor CRD is being deleted, which usually // means HNC is being uninstalled and we shouldn't delete _any_ namespaces. deletingCRD, err := crd.IsDeletingCRD(ctx, api.Anchors) if err != nil { log.Error(err, "Couldn't determine if CRD is being deleted") - return false, err + return err } log.V(1).Info("Anchor is being deleted", "deletingCRD", deletingCRD) @@ -166,21 +163,21 @@ func (r *Reconciler) onDeleting(ctx context.Context, log logr.Logger, inst *api. switch { case r.shouldDeleteSubns(log, inst, snsInst, deletingCRD): log.Info("Deleting subnamespace due to anchor being deleted") - return true, r.deleteNamespace(ctx, log, snsInst) + return 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) + controllerutil.RemoveFinalizer(inst, api.MetaGroup) + return r.writeInstance(ctx, log, inst) 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 log.Info("Waiting for K8s to delete this anchor (all finalizers are removed)") } - return true, nil + return nil } } @@ -213,7 +210,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 +254,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 } diff --git a/internal/hierarchyconfig/reconciler.go b/internal/hierarchyconfig/reconciler.go index 0fa219f63..1886e319d 100644 --- a/internal/hierarchyconfig/reconciler.go +++ b/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} } }