diff --git a/internal/forest/namespaceconditions.go b/internal/forest/namespaceconditions.go index c1063bff2..95846529a 100644 --- a/internal/forest/namespaceconditions.go +++ b/internal/forest/namespaceconditions.go @@ -1,36 +1,47 @@ package forest import ( + "fmt" + api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2" ) -// HasLocalCritCondition returns if the namespace itself has any local critical conditions, ignoring -// its ancestors. Any code with the "Crit" prefix is a critical condition. -func (ns *Namespace) HasLocalCritCondition() bool { +// IsHalted returns true if this namespace has an ActivitiesHalted condition for any reason *other* +// than one of its ancestors also being halted. +func (ns *Namespace) IsHalted() bool { + if ns == nil { + return false + } for _, cond := range ns.conditions { - if cond.Type == api.ConditionActivitiesHalted && cond.Reason != api.ReasonAncestor { + if cond.Type == api.ConditionActivitiesHalted { return true } } return false } -// GetCritAncestor returns the name of the first ancestor with a critical condition, or the empty -// string if there are no such ancestors. It *can* return the name of the current namespace. -func (ns *Namespace) GetCritAncestor() string { - if ns.HasLocalCritCondition() { - return ns.name - } - if ns.Parent() == nil { +// GetHaltedRoot returns the name of the lowest subtree that's halted for any reason *other* than a +// higher ancestor being halted. This can be the name of the current namespace, or the empty string +// if there are no halted ancestors. +func (ns *Namespace) GetHaltedRoot() string { + if ns == nil { return "" } - return ns.Parent().GetCritAncestor() + if ns.IsHalted() { + return ns.name + } + return ns.Parent().GetHaltedRoot() } // SetCondition adds a new condition to the current condition list. +// +// Any condition with ReasonAncestor is ignored; this is added dynamically when calling +// Conditions(). func (ns *Namespace) SetCondition(tp, reason, msg string) { - oldCond := ns.conditions - ns.conditions = append(oldCond, api.NewCondition(tp, reason, msg)) + if reason == api.ReasonAncestor { + return + } + ns.conditions = append(ns.conditions, api.NewCondition(tp, reason, msg)) } // ClearConditions set conditions to nil. @@ -40,5 +51,11 @@ func (ns *Namespace) ClearConditions() { // Conditions returns a full list of the conditions in the namespace. func (ns *Namespace) Conditions() []api.Condition { + if root := ns.GetHaltedRoot(); root != "" && root != ns.name { + msg := fmt.Sprintf("Propagation paused in %q and its descendants due to ActivitiesHalted condition on ancestor %q", ns.name, root) + ret := []api.Condition{api.NewCondition(api.ConditionActivitiesHalted, api.ReasonAncestor, msg)} + return append(ret, ns.conditions...) + } + return ns.conditions } diff --git a/internal/hierarchyconfig/reconciler.go b/internal/hierarchyconfig/reconciler.go index e4c0a79aa..ad9578500 100644 --- a/internal/hierarchyconfig/reconciler.go +++ b/internal/hierarchyconfig/reconciler.go @@ -290,11 +290,15 @@ func (r *Reconciler) syncWithForest(log logr.Logger, nsInst *corev1.Namespace, i defer r.Forest.Unlock() ns := r.Forest.Get(inst.ObjectMeta.Namespace) - // Clear locally-set conditions in the forest; we'll re-add them if they're still relevant. But - // first, record whether there were any critical ones since if this changes, we'll need to notify + // Clear all conditions; we'll re-add them if they're still relevant. But first, record whether + // this namespace (excluding ancestors) was halted, since if that changes, we'll need to notify // other namespaces. - hadCrit := ns.HasLocalCritCondition() + wasHalted := ns.IsHalted() ns.ClearConditions() + // We can figure out this condition pretty easily... + if deletingCRD { + ns.SetCondition(api.ConditionActivitiesHalted, api.ReasonDeletingCRD, "The HierarchyConfiguration CRD is being deleted; all propagation is disabled.") + } // Set external tree labels in the forest if this is an external namespace. r.syncExternalNamespace(log, nsInst, ns) @@ -306,6 +310,9 @@ func (r *Reconciler) syncWithForest(log logr.Logger, nsInst *corev1.Namespace, i r.syncParent(log, inst, ns) initial := r.markExisting(log, ns) + // Sync labels and annotations, now that the structure's been updated. + nsCustomerLabelUpdated := r.syncTreeLabels(log, nsInst, ns) + // Sync other spec and spec-like info r.syncAnchors(log, ns, anms) if ns.UpdateAllowCascadingDeletion(inst.Spec.AllowCascadingDeletion) { @@ -315,11 +322,9 @@ func (r *Reconciler) syncWithForest(log logr.Logger, nsInst *corev1.Namespace, i // Sync the status inst.Status.Children = ns.ChildNames() - r.syncConditions(log, inst, ns, deletingCRD, hadCrit) - - // Sync the tree labels. This should go last since it can depend on the conditions. - nsCustomerLabelUpdated := r.syncTreeLabels(log, nsInst, ns) + r.syncConditions(log, inst, ns, wasHalted) + r.HNCConfigReconciler.Enqueue("namespace reconciled") return initial || nsCustomerLabelUpdated } @@ -431,6 +436,9 @@ func (r *Reconciler) markExisting(log logr.Logger, ns *forest.Namespace) bool { } func (r *Reconciler) syncParent(log logr.Logger, inst *api.HierarchyConfiguration, ns *forest.Namespace) { + // As soon as the structure has been updated, do a cycle check. + defer r.setCycleCondition(log, ns) + if ns.IsExternal() { ns.SetParent(nil) return @@ -459,10 +467,10 @@ func (r *Reconciler) syncParent(log logr.Logger, inst *api.HierarchyConfiguratio // Change the parent. ns.SetParent(curParent) - // Finally, enqueue all other namespaces that could be directly affected. The old and new parents - // have just gained/lost a child, while the descendants need to have their tree labels updated and - // their objects resynced. Note that it's fine if oldParent or curParent is nil - see - // enqueueAffected for details. + // Enqueue all other namespaces that could be directly affected. The old and new parents have just + // gained/lost a child, while the descendants need to have their tree labels updated and their + // objects resynced. Note that it's fine if oldParent or curParent is nil - see enqueueAffected + // for details. // // If we've just created a cycle, all the members of that cycle will be listed as the descendants, // so enqueuing them will ensure that the conditions show up in all members of the cycle. @@ -471,6 +479,17 @@ func (r *Reconciler) syncParent(log logr.Logger, inst *api.HierarchyConfiguratio r.enqueueAffected(log, "subtree root has changed", ns.DescendantNames()...) } +func (r *Reconciler) setCycleCondition(log logr.Logger, ns *forest.Namespace) { + cycle := ns.CycleNames() + if cycle == nil { + return + } + + msg := fmt.Sprintf("Namespace is a member of the cycle: %s", strings.Join(cycle, " <- ")) + log.Info(msg) + ns.SetCondition(api.ConditionActivitiesHalted, api.ReasonInCycle, msg) +} + // syncAnchors updates the anchor list. If any anchor is created/deleted, it will enqueue // the child to update its SubnamespaceAnchorMissing condition. A modified anchor will appear // twice in the change list (one in deleted, one in created), both subnamespaces @@ -498,27 +517,27 @@ func (r *Reconciler) syncTreeLabels(log logr.Logger, nsInst *corev1.Namespace, n // Look for all ancestors. Stop as soon as we find a namespaces that has a critical condition in // the forest (note that AncestorHaltActivities is never included in the forest). This should handle orphans // and cycles. - anc := ns + curNS := ns depth := 0 - for anc != nil { - l := anc.Name() + api.LabelTreeDepthSuffix + for curNS != nil { + l := curNS.Name() + api.LabelTreeDepthSuffix metadata.SetLabel(nsInst, l, strconv.Itoa(depth)) - if anc.HasLocalCritCondition() { + if curNS.IsHalted() { break } // If the root is an external namespace, add all its external tree labels too. // Note it's impossible to have an external namespace as a non-root, which is // enforced by both admission controllers and the reconciler here. - if anc.IsExternal() { - for k, v := range anc.ExternalTreeLabels { + if curNS.IsExternal() { + for k, v := range curNS.ExternalTreeLabels { l = k + api.LabelTreeDepthSuffix metadata.SetLabel(nsInst, l, strconv.Itoa(depth+v)) } break } - anc = anc.Parent() + curNS = curNS.Parent() depth++ } // Update the labels in the forest so that we can quickly access the labels and @@ -530,61 +549,22 @@ func (r *Reconciler) syncTreeLabels(log logr.Logger, nsInst *corev1.Namespace, n return false } -func (r *Reconciler) syncConditions(log logr.Logger, inst *api.HierarchyConfiguration, ns *forest.Namespace, deletingCRD, hadCrit bool) { - // Sync critical conditions after all locally-set conditions are updated. - r.syncCritConditions(log, ns, deletingCRD, hadCrit) +func (r *Reconciler) syncConditions(log logr.Logger, inst *api.HierarchyConfiguration, ns *forest.Namespace, wasHalted bool) { + // If the halted status has changed, notify + if ns.IsHalted() != wasHalted { + msg := "" + if wasHalted { + log.Info("ActivitiesHalted condition removed") + msg = "removed" + } else { + log.Info("Setting ActivitiesHalted on namespace", "conditions", ns.Conditions()) + msg = "added" + } + r.enqueueAffected(log, "descendant of a namespace with ActivitiesHalted "+msg, ns.DescendantNames()...) + } // Convert and pass in-memory conditions to HierarchyConfiguration object. inst.Status.Conditions = ns.Conditions() - setCritAncestorCondition(log, inst, ns) - r.HNCConfigReconciler.Enqueue("namespace reconciled") -} - -// syncCritConditions enqueues the children of a namespace if the existing critical conditions in the -// namespace are gone or critical conditions are newly found. -func (r *Reconciler) syncCritConditions(log logr.Logger, ns *forest.Namespace, deletingCRD, hadCrit bool) { - // If we're in a cycle, determine that now - if cycle := ns.CycleNames(); cycle != nil { - msg := fmt.Sprintf("Namespace is a member of the cycle: %s", strings.Join(cycle, " <- ")) - ns.SetCondition(api.ConditionActivitiesHalted, api.ReasonInCycle, msg) - } - - if deletingCRD { - ns.SetCondition(api.ConditionActivitiesHalted, api.ReasonDeletingCRD, "The HierarchyConfiguration CRD is being deleted; all syncing is disabled.") - } - - // Early exit if nothing's changed and there's no need to enqueue relatives. - if hadCrit == ns.HasLocalCritCondition() { - return - } - - msg := "" - if hadCrit { - log.Info("ActivitiesHalted condition removed") - msg = "removed" - } else { - log.Info("Setting ActivitiesHalted on namespace", "conditions", ns.Conditions()) - msg = "added" - } - r.enqueueAffected(log, "descendant of a namespace with ActivitiesHalted "+msg, ns.DescendantNames()...) -} - -func setCritAncestorCondition(log logr.Logger, inst *api.HierarchyConfiguration, ns *forest.Namespace) { - if ns.HasLocalCritCondition() { - return - } - ans := ns.Parent() - for ans != nil { - if !ans.HasLocalCritCondition() { - ans = ans.Parent() - continue - } - log.Info("Ancestor has a ActivitiesHalted condition", "ancestor", ans.Name()) - msg := fmt.Sprintf("Propagation paused in the subtree of %q due to ActivitiesHalted condition", ans.Name()) - condition := api.NewCondition(api.ConditionActivitiesHalted, api.ReasonAncestor, msg) - inst.Status.Conditions = append(inst.Status.Conditions, condition) - return - } } // enqueueAffected enqueues all affected namespaces for later reconciliation. This occurs in a diff --git a/internal/hierarchyconfig/validator.go b/internal/hierarchyconfig/validator.go index fc320a573..7e8b3c10e 100644 --- a/internal/hierarchyconfig/validator.go +++ b/internal/hierarchyconfig/validator.go @@ -175,11 +175,11 @@ func (v *Validator) checkNS(ns *forest.Namespace) admission.Response { return deny(metav1.StatusReasonServiceUnavailable, msg) } - // Deny the request if the namespace has a critical ancestor - but not if it's critical itself, - // since we may be trying to resolve the critical condition. - critAnc := ns.GetCritAncestor() - if critAnc != "" && critAnc != ns.Name() { - msg := fmt.Sprintf("The ancestor %q of namespace %q has a critical condition, which must be resolved before any changes can be made to the hierarchy configuration.", critAnc, ns.Name()) + // Deny the request if the namespace has a halted root - but not if it's halted itself, since we + // may be trying to resolve the halted condition. + haltedRoot := ns.GetHaltedRoot() + if haltedRoot != "" && haltedRoot != ns.Name() { + msg := fmt.Sprintf("The ancestor %q of namespace %q has a critical condition, which must be resolved before any changes can be made to the hierarchy configuration.", haltedRoot, ns.Name()) return deny(metav1.StatusReasonForbidden, msg) } diff --git a/internal/objects/reconciler.go b/internal/objects/reconciler.go index ef8a38421..04a37474d 100644 --- a/internal/objects/reconciler.go +++ b/internal/objects/reconciler.go @@ -256,8 +256,8 @@ func (r *Reconciler) syncWithForest(ctx context.Context, log logr.Logger, inst * // what we'd _like_ to do. We still needed to sync the forest since we want to know when objects // are added and removed, so we can sync them properly if the critical condition is resolved, but // don't do anything else for now. - if ca := r.Forest.Get(inst.GetNamespace()).GetCritAncestor(); ca != "" { - log.Info("Namespace has 'ActivitiesHalted' condition; will not touch propagated object", "affectedNamespace", ca, "suppressedAction", action) + if halted := r.Forest.Get(inst.GetNamespace()).GetHaltedRoot(); halted != "" { + log.Info("Namespace has 'ActivitiesHalted' condition; will not touch propagated object", "haltedRoot", halted, "suppressedAction", action) return actionNop, nil } @@ -478,10 +478,10 @@ func cleanSource(src *unstructured.Unstructured) *unstructured.Unstructured { func (r *Reconciler) enqueueDescendants(ctx context.Context, log logr.Logger, src *unstructured.Unstructured, reason string) { sns := r.Forest.Get(src.GetNamespace()) - if ca := sns.GetCritAncestor(); ca != "" { - // There's no point enqueuing anything if the source namespace has a crit condition since we'll - // just skip any action anyway. - log.V(1).Info("Will not enqueue descendants due to crit ancestor", "critAncestor", ca, "oldReason", reason) + if halted := sns.GetHaltedRoot(); halted != "" { + // There's no point enqueuing anything if the source namespace is halted since we'll just skip + // any action anyway. + log.V(1).Info("Will not enqueue descendants since activities are halted", "haltedRoot", halted, "oldReason", reason) return } log.V(1).Info("Enqueuing descendant objects", "reason", reason)