Improve usability of unmanaged namespaces

This change makes it a nondestructive operation to switch a namespace
from managed to unmanaged and back, by _not_ deleting the HC or anchors
while a namespace is unmanaged. It also fixes one serious bug
(namespaces could not be changed to roots because the unnamed namespace
didn't match the regex) and adds a lot more details in the error
messages. Finally, it allows users to fix broken hierarchies by removing
a formerly-managed parent.

I also changed the terminology from "excluded" to "unmanaged"
throughout.

Testing: This is hard to test automatically since it requires constant
redeployments of HNC. So I did extensive manual testing instead:

* Try to arrange hierarchies with unmanaged namespace, both excluded
(e.g hnc-system) and ones that don't match the regex. Verify that the
error messages clearly indicate the correct problem.
* Create a legal hierarchy and then break it by changing the regex.
Verify that I can fix it without restarting HNC.
* Create a legal hierarchy, break it with the regex, and then restore it
with the regex. Verify that the original hierarchy comes back.
* Verify that anchors and hierarchyconfigs lose their finalizers when
their namespaces become unmanaged, and get them back if they become
managed again.

Author: adrianludwin

internal/anchor/reconciler.go

@@ -75,23 +75,29 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 		return ctrl.Result{}, err
 	}
 
-	// Always delete anchor (and any other HNC CRs) in the excluded namespaces and
-	// early exit.
-	if !config.IsNamespaceIncluded(pnm) {
-		// Since the anchors in the excluded namespaces are never synced by HNC,
-		// there are no finalizers on the anchors that we can delete them without
-		// removing the finalizers first.
-		log.Info("Deleting anchor in an excluded namespace")
-		return ctrl.Result{}, r.deleteInstance(ctx, log, inst)
+	// 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 {
+			log.Info("Removing finalizers from anchor in unmanaged namespace", "reason", why)
+			inst.ObjectMeta.Finalizers = nil
+			return ctrl.Result{}, r.writeInstance(ctx, log, inst)
+		}
+		return ctrl.Result{}, nil
 	}
 
 	// Report "Forbidden" state and early exit if the anchor name is an excluded
 	// namespace that should not be created as a subnamespace, but the webhook has
 	// been bypassed and the anchor has been successfully created. Forbidden
 	// anchors won't have finalizers.
-	if !config.IsNamespaceIncluded(nm) {
-		inst.Status.State = api.Forbidden
-		return ctrl.Result{}, r.writeInstance(ctx, log, inst)
+	if why := config.WhyUnmanaged(nm); why != "" {
+		if inst.Status.State != api.Forbidden || len(inst.ObjectMeta.Finalizers) > 0 {
+			log.Info("Setting forbidden state on anchor with unmanaged name", "reason", why)
+			inst.Status.State = api.Forbidden
+			inst.ObjectMeta.Finalizers = nil
+			return ctrl.Result{}, r.writeInstance(ctx, log, inst)
+		}
+		return ctrl.Result{}, nil
 	}
 
 	// Get the subnamespace. If it doesn't exist, initialize one.

internal/anchor/validator.go

@@ -79,14 +79,14 @@ func (v *Validator) handle(req *anchorRequest) admission.Response {
 
 	switch req.op {
 	case k8sadm.Create:
-		// Can't create subnamespaces in excluded namespaces
-		if !config.IsNamespaceIncluded(pnm) {
-			msg := fmt.Sprintf("Cannot create a subnamespace in the excluded namespace %q", pnm)
+		// Can't create subnamespaces in unmanaged namespaces
+		if why := config.WhyUnmanaged(pnm); why != "" {
+			msg := fmt.Sprintf("Cannot create a subnamespace in the unmanaged namespace %q (%s)", pnm, why)
 			return webhooks.Deny(metav1.StatusReasonForbidden, msg)
 		}
-		// Can't create subnamespaces using excluded namespace names
-		if !config.IsNamespaceIncluded(cnm) {
-			msg := fmt.Sprintf("Cannot create a subnamespace using the excluded namespace name %q", cnm)
+		// Can't create subnamespaces using unmanaged namespace names
+		if why := config.WhyUnmanaged(cnm); why != "" {
+			msg := fmt.Sprintf("Cannot create a subnamespace using the unmanaged namespace name %q (%s)", cnm, why)
 			return webhooks.Deny(metav1.StatusReasonForbidden, msg)
 		}
 

internal/config/default_config.go

@@ -1,7 +1,5 @@
 package config
 
-import "regexp"
-
 // UnpropgatedAnnotations is a list of annotations on objects that should _not_ be propagated by HNC.
 // Much like HNC itself, other systems (such as GKE Config Sync) use annotations to "claim" an
 // object - such as deleting objects it doesn't recognize. By removing these annotations on
@@ -10,11 +8,3 @@ import "regexp"
 // This value is controlled by the --unpropagated-annotation command line, which may be set multiple
 // times.
 var UnpropagatedAnnotations []string
-
-// ExcludedNamespaces is a list of namespaces used by reconcilers and validators
-// to exclude namespaces that shouldn't be reconciled or validated.
-//
-// This value is controlled by the --excluded-namespace command line, which may
-// be set multiple times.
-var excludedNamespaces map[string]bool
-var includedNamespacesRegex *regexp.Regexp

internal/config/namespace.go

@@ -4,27 +4,62 @@ import (
 	"regexp"
 )
 
+var (
+	// excludedNamespaces is a list of namespaces used by reconcilers and validators
+	// to exclude namespaces that shouldn't be reconciled or validated.
+	//
+	// This value is controlled by the --excluded-namespace command line, which may
+	// be set multiple times.
+	excludedNamespaces map[string]bool
+
+	// includedNamespacesRegex is the compiled regex of included namespaces
+	includedNamespacesRegex *regexp.Regexp
+
+	// includedNamespacesStr is the original pattern of the regex. It must
+	// only be used to generate error messages.
+	includedNamespacesStr string
+)
+
 func SetNamespaces(regex string, excluded ...string) {
 	if regex == "" {
 		regex = ".*"
 	}
 
-	includedNamespacesRegex = regexp.MustCompile("^" + regex + "$")
+	includedNamespacesStr = "^" + regex + "$"
+	includedNamespacesRegex = regexp.MustCompile(includedNamespacesStr)
 
 	excludedNamespaces = make(map[string]bool)
 	for _, exn := range excluded {
 		excludedNamespaces[exn] = true
 	}
 }
 
-func IsNamespaceIncluded(name string) bool {
-	if excludedNamespaces[name] {
-		return false
+// WhyUnamanged returns a human-readable message explaining why the given
+// namespace is unmanaged, or an empty string if it *is* managed.
+func WhyUnmanaged(nm string) string {
+	// We occasionally check if the _parent_ of a namespace is managed.
+	// It's an error for a managed namespace to have an unmanaged parent,
+	// so we'll treat "no parent" as managed.
+	if nm == "" {
+		return ""
+	}
+
+	if excludedNamespaces[nm] {
+		return "excluded by the HNC administrator"
 	}
 
 	if includedNamespacesRegex == nil { // unit tests
-		return true
+		return ""
+	}
+
+	if !includedNamespacesRegex.MatchString(nm) {
+		return "does not match the regex set by the HNC administrator: `" + includedNamespacesStr + "`"
 	}
 
-	return includedNamespacesRegex.MatchString(name)
+	return ""
+}
+
+// IsManagedNamespace is the same as WhyUnmanaged but converts the response to a bool for convenience.
+func IsManagedNamespace(nm string) bool {
+	return WhyUnmanaged(nm) == ""
 }

internal/config/namespace_test.go

@@ -8,28 +8,28 @@ import (
 func TestIsNamespaceIncluded(t *testing.T) {
 
 	tests := []struct {
-		name              string
-		regex             string
-		excludeNamespaces []string
-		expect            bool
+		name     string
+		regex    string
+		excluded []string
+		expect   bool
 	}{
-		{name: "foobar", regex: "foo.*", excludeNamespaces: []string{"bar"}, expect: true},
-		{name: "bar", regex: "foo-.*", excludeNamespaces: []string{"bar"}, expect: false},
-		{name: "bar", regex: ".*", excludeNamespaces: []string{"bar"}, expect: false},
-		{name: "foo", regex: ".*", excludeNamespaces: []string{"bar"}, expect: true},
-		{name: "foo", regex: ".*", excludeNamespaces: []string{"bar", "foo"}, expect: false},
+		{name: "foobar", regex: "foo.*", excluded: []string{"bar"}, expect: true},
+		{name: "bar", regex: "foo-.*", excluded: []string{"bar"}, expect: false},
+		{name: "bar", regex: ".*", excluded: []string{"bar"}, expect: false},
+		{name: "foo", regex: ".*", excluded: []string{"bar"}, expect: true},
+		{name: "foo", regex: ".*", excluded: []string{"bar", "foo"}, expect: false},
 	}
 
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
 			g := NewWithT(t)
 
 			// Test
-			SetNamespaces(tc.regex, tc.excludeNamespaces...)
-			isIncluded := IsNamespaceIncluded(tc.name)
+			SetNamespaces(tc.regex, tc.excluded...)
+			got := IsManagedNamespace(tc.name)
 
 			// Report
-			g.Expect(isIncluded).Should(Equal(tc.expect))
+			g.Expect(got).Should(Equal(tc.expect))
 		})
 	}
 

internal/hierarchyconfig/reconciler.go

@@ -99,8 +99,8 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	log := loggerWithRID(r.Log).WithValues("ns", ns)
 
 	// Early exit if it's an excluded namespace
-	if !config.IsNamespaceIncluded(ns) {
-		return ctrl.Result{}, r.handleExcludedNamespace(ctx, log, ns)
+	if !config.IsManagedNamespace(ns) {
+		return ctrl.Result{}, r.handleUnmanaged(ctx, log, ns)
 	}
 
 	stats.StartHierConfigReconcile()
@@ -109,7 +109,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Resu
 	return ctrl.Result{}, r.reconcile(ctx, log, ns)
 }
 
-func (r *Reconciler) handleExcludedNamespace(ctx context.Context, log logr.Logger, nm string) error {
+func (r *Reconciler) handleUnmanaged(ctx context.Context, log logr.Logger, nm string) error {
 	// Get the namespace. Early exist if the namespace doesn't exist or is purged.
 	// If so, there must be no namespace label or HC instance to delete.
 	nsInst, err := r.getNamespace(ctx, nm)
@@ -127,13 +127,23 @@ func (r *Reconciler) handleExcludedNamespace(ctx context.Context, log logr.Logge
 		return err
 	}
 
-	// Always delete hierarchyconfiguration (and any other HNC CRs) in the
-	// excluded namespaces. Note: since singletons in the excluded namespaces are
-	// never synced by HNC, there are no finalizers on the singletons that we can
-	// delete them without removing the finalizers first.
-	if err := r.deleteSingletonIfExists(ctx, log, nm); err != nil {
+	// Don't delete the hierarchy config, since the admin might be enabling and disabling the regex and
+	// we don't want this to be destructive. Instead, just remove the finalizers if there are any so that
+	// users can delete it if they like.
+	inst, _, err := r.getSingleton(ctx, nm)
+	if err != nil {
 		return err
 	}
+	if len(inst.ObjectMeta.Finalizers) > 0 || len(inst.Status.Children) > 0 {
+		log.Info("Removing finalizers and children on unmanaged singleton")
+		inst.ObjectMeta.Finalizers = nil
+		inst.Status.Children = nil
+		stats.WriteHierConfig()
+		if err := r.Update(ctx, inst); err != nil {
+			log.Error(err, "while removing finalizers on unmanaged namespace")
+			return err
+		}
+	}
 
 	return nil
 }
@@ -428,9 +438,9 @@ func (r *Reconciler) syncParent(log logr.Logger, inst *api.HierarchyConfiguratio
 
 	// Sync this namespace with its current parent.
 	curParent := r.Forest.Get(inst.Spec.Parent)
-	if !config.IsNamespaceIncluded(inst.Spec.Parent) {
-		log.Info("Setting ConditionActivitiesHalted: excluded namespace set as parent", "parent", inst.Spec.Parent)
-		ns.SetCondition(api.ConditionActivitiesHalted, api.ReasonIllegalParent, fmt.Sprintf("Parent %q is an excluded namespace", inst.Spec.Parent))
+	if !config.IsManagedNamespace(inst.Spec.Parent) {
+		log.Info("Setting ConditionActivitiesHalted: unmanaged namespace set as parent", "parent", inst.Spec.Parent)
+		ns.SetCondition(api.ConditionActivitiesHalted, api.ReasonIllegalParent, fmt.Sprintf("Parent %q is an unmanaged namespace", inst.Spec.Parent))
 	} else if curParent != nil && !curParent.Exists() {
 		log.Info("Setting ConditionActivitiesHalted: parent doesn't exist (or hasn't been synced yet)", "parent", inst.Spec.Parent)
 		ns.SetCondition(api.ConditionActivitiesHalted, api.ReasonParentMissing, fmt.Sprintf("Parent %q does not exist", inst.Spec.Parent))
@@ -644,37 +654,6 @@ func (r *Reconciler) writeHierarchy(ctx context.Context, log logr.Logger, orig,
 	return true, nil
 }
 
-// deleteSingletonIfExists deletes the singleton in the namespace if it exists.
-// Note: Make sure there's no finalizers on the singleton before calling this
-// function.
-func (r *Reconciler) deleteSingletonIfExists(ctx context.Context, log logr.Logger, nm string) error {
-	inst, deletingCRD, err := r.getSingleton(ctx, nm)
-	if err != nil {
-		return err
-	}
-
-	// Early exit if the singleton doesn't exist.
-	if inst.CreationTimestamp.IsZero() {
-		return nil
-	}
-
-	// If the CRD is being deleted, we don't need to delete it separately. It will
-	// be deleted with the CRD.
-	if deletingCRD {
-		log.Info("HC in excluded namespace is already being deleted")
-		return nil
-	}
-	log.Info("Deleting illegal HC in excluded namespace")
-
-	stats.WriteHierConfig()
-	if err := r.Delete(ctx, inst); err != nil {
-		log.Error(err, "while deleting on apiserver")
-		return err
-	}
-
-	return nil
-}
-
 func (r *Reconciler) writeNamespace(ctx context.Context, log logr.Logger, orig, inst *corev1.Namespace) (bool, error) {
 	if reflect.DeepEqual(orig, inst) {
 		return false, nil

internal/hierarchyconfig/reconciler_test.go

@@ -41,17 +41,6 @@ var _ = Describe("Hierarchy", func() {
 		Eventually(HasChild(ctx, barName, fooName)).Should(Equal(true))
 	})
 
-	It("should remove the hierarchyconfiguration singleton in an excluded namespacee", func() {
-		// Set the excluded-namespace "kube-system"'s parent to "bar".
-		config.SetNamespaces("", "kube-system")
-		exHier := NewHierarchy("kube-system")
-		exHier.Spec.Parent = barName
-		UpdateHierarchy(ctx, exHier)
-
-		// Verify the hierarchyconfiguration singleton is deleted.
-		Eventually(CanGetHierarchy(ctx, "kube-system")).Should(Equal(false))
-	})
-
 	It("should set IllegalParent condition if the parent is an excluded namespace", func() {
 		// Set bar's parent to the excluded-namespace "kube-system".
 		config.SetNamespaces("", "kube-system")

internal/hierarchyconfig/validator.go

@@ -120,12 +120,12 @@ func (v *Validator) handle(ctx context.Context, log logr.Logger, req *request) a
 		return allow("HNC SA")
 	}
 
-	if !config.IsNamespaceIncluded(req.hc.Namespace) {
-		reason := fmt.Sprintf("Cannot set the excluded namespace %q as a child of another namespace", req.hc.Namespace)
+	if why := config.WhyUnmanaged(req.hc.Namespace); why != "" {
+		reason := fmt.Sprintf("Namespace %q is not managed by HNC (%s) and cannot be set as a child of another namespace", req.hc.Namespace, why)
 		return deny(metav1.StatusReasonForbidden, reason)
 	}
-	if !config.IsNamespaceIncluded(req.hc.Spec.Parent) {
-		reason := fmt.Sprintf("Cannot set the parent to the excluded namespace %q", req.hc.Spec.Parent)
+	if why := config.WhyUnmanaged(req.hc.Spec.Parent); why != "" {
+		reason := fmt.Sprintf("Namespace %q is not managed by HNC (%s) and cannot be set as the parent of another namespace", req.hc.Spec.Parent, why)
 		return deny(metav1.StatusReasonForbidden, reason)
 	}
 
@@ -333,8 +333,14 @@ func (v *Validator) getServerChecks(log logr.Logger, ns, curParent, newParent *f
 	}
 
 	// If this is currently a root and we're moving into a new tree, just check the parent.
-	if curParent == nil {
-		return []serverCheck{{nnm: newParent.Name(), reason: "proposed parent", checkType: checkAuthz}}
+	//
+	// Exception: if the current parent is unmanaged (e.g. it used to be managed before, but isn't anymore),
+	// treat it as though it's currently a root and allow the change.
+	if curParent == nil || !config.IsManagedNamespace(curParent.Name()) {
+		if newParent != nil { // could be nil if old parane is unmanaged
+			return []serverCheck{{nnm: newParent.Name(), reason: "proposed parent", checkType: checkAuthz}}
+		}
+		return nil
 	}
 
 	// If the current parent is missing, ask for a missing check. Note that only the *direct* parent

internal/hierarchyconfig/validator_test.go

@@ -39,9 +39,9 @@ func TestStructure(t *testing.T) {
 		// should see denial message of excluded namespace for `kube-system`. As for
 		// `kube-public`, we will see missing parent/child instead of excluded
 		// namespaces denial message for it.
-		{name: "exclude parent kube-system", nnm: "a", pnm: "kube-system", fail: true, msgContains: "Cannot set the parent to the excluded namespace"},
+		{name: "exclude parent kube-system", nnm: "a", pnm: "kube-system", fail: true, msgContains: "excluded"},
 		{name: "missing parent kube-public", nnm: "a", pnm: "kube-public", fail: true, msgContains: "does not exist"},
-		{name: "exclude child kube-system", nnm: "kube-system", pnm: "a", fail: true, msgContains: "Cannot set the excluded namespace"},
+		{name: "exclude child kube-system", nnm: "kube-system", pnm: "a", fail: true, msgContains: "excluded"},
 		{name: "missing child kube-public", nnm: "kube-public", pnm: "a", fail: true, msgContains: "HNC has not reconciled namespace"},
 	}
 	for _, tc := range tests {

internal/namespace/mutator.go

@@ -52,7 +52,7 @@ func (m *Mutator) Handle(ctx context.Context, req admission.Request) admission.R
 // Currently, we only add `included-namespace` label to non-excluded namespaces
 // if the label is missing.
 func (m *Mutator) handle(log logr.Logger, ns *corev1.Namespace) {
-	if !config.IsNamespaceIncluded(ns.Name) {
+	if !config.IsManagedNamespace(ns.Name) {
 		return
 	}
 
@@ -61,7 +61,7 @@ func (m *Mutator) handle(log logr.Logger, ns *corev1.Namespace) {
 		if ns.Labels == nil {
 			ns.Labels = map[string]string{}
 		}
-		log.Info("Not an excluded namespace; set included-namespace label.")
+		log.Info("Managed namespace is missing included-namespace label; adding")
 		ns.Labels[api.LabelIncludedNamespace] = "true"
 	}
 }

internal/namespace/validator.go

@@ -120,11 +120,12 @@ func (v *Validator) illegalIncludedNamespaceLabel(req *nsRequest) admission.Resp
 			return webhooks.Allow("")
 		}
 	}
-	isIncluded := config.IsNamespaceIncluded(req.ns.Name)
+
+	isIncluded := config.IsManagedNamespace(req.ns.Name)
 
 	// An excluded namespaces should not have included-namespace label.
 	if !isIncluded && hasLabel {
-		msg := fmt.Sprintf("You cannot enforce webhook rules on this excluded namespace using the %q label. "+
+		msg := fmt.Sprintf("You cannot enforce webhook rules on this unmanaged namespace using the %q label. "+
 			"See https://github.com/kubernetes-sigs/hierarchical-namespaces/blob/master/docs/user-guide/concepts.md#included-namespace-label "+
 			"for detail.", api.LabelIncludedNamespace)
 		return webhooks.Deny(metav1.StatusReasonForbidden, msg)
@@ -135,7 +136,7 @@ func (v *Validator) illegalIncludedNamespaceLabel(req *nsRequest) admission.Resp
 	// Note: since we have a mutating webhook to set the correct label if it's
 	// missing before this, we only need to check if the label value is correct.
 	if isIncluded && labelValue != "true" {
-		msg := fmt.Sprintf("You cannot change the value of the %q label. It has to be set as true on a non-excluded namespace. "+
+		msg := fmt.Sprintf("You cannot change the value of the %q label. It has to be set as true on all managed namespaces. "+
 			"See https://github.com/kubernetes-sigs/hierarchical-namespaces/blob/master/docs/user-guide/concepts.md#included-namespace-label "+
 			"for detail.", api.LabelIncludedNamespace)
 		return webhooks.Deny(metav1.StatusReasonForbidden, msg)