Don't store external tree labels explicitly

While preparing to add managed labels, I found it was weird to have an
explicit set of external tree labels when these can easily be derived
from the existing list of labels. This change removes the explicit
external tree labels and instead recomputes them on-demand, which should
be rare (external namespaces are neither used nor synced frequently).

Tested: existing unit/integ tests (no change in functionality)

Author: adrianludwin

internal/forest/namespace.go

@@ -2,6 +2,8 @@ package forest
 
 import (
 	"reflect"
+	"strconv"
+	"strings"
 
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/labels"
@@ -26,7 +28,8 @@ type Namespace struct {
 	exists                 bool
 	allowCascadingDeletion bool
 
-	// labels store the original namespaces' labels
+	// labels store the original namespaces' labels, and are used for object propagation exceptions
+	// and to store the tree labels of external namespaces.
 	labels map[string]string
 
 	// sourceObjects store the objects created by users, identified by GVK and name.
@@ -44,15 +47,10 @@ type Namespace struct {
 	// Anchors store a list of anchors in the namespace.
 	Anchors []string
 
-	// Manager stores the manager of the namespace. The default value
-	// "hnc.x-k8s.io" means it's managed by HNC.
+	// Manager stores the manager of the namespace. The default value of "hnc.x-k8s.io" means it's
+	// managed by HNC. Any other value means that the namespace is an "external" namespace, whose
+	// metadata (e.g. labels) are set outside of HNC.
 	Manager string
-
-	// ExternalTreeLabels stores external tree labels if this namespace is an external namespace.
-	// It will be empty if the namespace is not external. External namespace will at least have one
-	// tree label of itself. The key is the tree label without ".tree.hnc.x-k8s.io/depth" suffix.
-	// The value is the depth.
-	ExternalTreeLabels map[string]int
 }
 
 // Name returns the name of the namespace, of "<none>" if the namespace is nil.
@@ -90,6 +88,19 @@ func (ns *Namespace) UnsetExists() bool {
 	return changed
 }
 
+// GetTreeLabels returns all the tree labels with the values converted into integers for easier
+// manipulation.
+func (ns *Namespace) GetTreeLabels() map[string]int {
+	r := map[string]int{}
+	for k, v := range ns.labels {
+		if !strings.Contains(k, api.LabelTreeDepthSuffix) {
+			continue
+		}
+		r[k], _ = strconv.Atoi(v)
+	}
+	return r
+}
+
 func (ns *Namespace) GetLabels() labels.Set {
 	return labels.Set(ns.labels)
 }
@@ -176,5 +187,5 @@ func (ns *Namespace) HasAnchor(n string) bool {
 
 // IsExternal returns true if the namespace is not managed by HNC.
 func (ns *Namespace) IsExternal() bool {
-	return len(ns.ExternalTreeLabels) > 0
+	return ns.Manager != "" && ns.Manager != api.MetaGroup
 }

internal/hierarchyconfig/reconciler.go

@@ -331,15 +331,14 @@ func (r *Reconciler) syncWithForest(log logr.Logger, nsInst *corev1.Namespace, i
 // syncExternalNamespace sets external tree labels to the namespace in the forest
 // if the namespace is an external namespace not managed by HNC.
 func (r *Reconciler) syncExternalNamespace(log logr.Logger, nsInst *corev1.Namespace, ns *forest.Namespace) {
-	ns.Manager = nsInst.Annotations[api.AnnotationManagedBy]
-	if ns.Manager == "" || ns.Manager == api.MetaGroup {
+	mgr := nsInst.Annotations[api.AnnotationManagedBy]
+	if mgr == "" || mgr == api.MetaGroup {
 		// If the internal namespace is just converted from an external namespace,
 		// enqueue the descendants to remove the propagated external tree labels.
 		if ns.IsExternal() {
 			r.enqueueAffected(log, "subtree root converts from external to internal", ns.DescendantNames()...)
 		}
 		ns.Manager = api.MetaGroup
-		ns.ExternalTreeLabels = nil
 		return
 	}
 
@@ -348,18 +347,7 @@ func (r *Reconciler) syncExternalNamespace(log logr.Logger, nsInst *corev1.Names
 	if !ns.IsExternal() {
 		r.enqueueAffected(log, "subtree root converts from internal to external", ns.DescendantNames()...)
 	}
-
-	// Get tree labels and set them in the forest. If there's no tree labels on the
-	// namespace, set only one tree label of itself.
-	etls := make(map[string]int)
-	for tl, d := range nsInst.Labels {
-		enm := strings.TrimSuffix(tl, api.LabelTreeDepthSuffix)
-		if enm != tl {
-			etls[enm], _ = strconv.Atoi(d)
-		}
-	}
-	etls[ns.Name()] = 0
-	ns.ExternalTreeLabels = etls
+	ns.Manager = mgr
 }
 
 // syncSubnamespaceParent sets the parent to the owner and updates the SubnamespaceAnchorMissing
@@ -504,6 +492,9 @@ func (r *Reconciler) syncAnchors(log logr.Logger, ns *forest.Namespace, anms []s
 func (r *Reconciler) syncTreeLabels(log logr.Logger, nsInst *corev1.Namespace, ns *forest.Namespace) bool {
 	if ns.IsExternal() {
 		metadata.SetLabel(nsInst, nsInst.Name+api.LabelTreeDepthSuffix, "0")
+
+		// Set the labels so we can retrieve the external tree labels in the future if needed
+		ns.SetLabels(nsInst.Labels)
 		return false
 	}
 
@@ -530,9 +521,8 @@ func (r *Reconciler) syncTreeLabels(log logr.Logger, nsInst *corev1.Namespace, n
 		// 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 curNS.IsExternal() {
-			for k, v := range curNS.ExternalTreeLabels {
-				l = k + api.LabelTreeDepthSuffix
-				metadata.SetLabel(nsInst, l, strconv.Itoa(depth+v))
+			for k, v := range curNS.GetTreeLabels() {
+				metadata.SetLabel(nsInst, k, strconv.Itoa(depth+v))
 			}
 			break
 		}
@@ -543,7 +533,7 @@ func (r *Reconciler) syncTreeLabels(log logr.Logger, nsInst *corev1.Namespace, n
 	// Update the labels in the forest so that we can quickly access the labels and
 	// compare if they match the given selector
 	if ns.SetLabels(nsInst.Labels) {
-		log.Info("Namespace tree labels have been updated.")
+		log.Info("Namespace managed and tree labels have been updated")
 		return true
 	}
 	return false

internal/hierarchyconfig/validator_test.go

@@ -70,8 +70,8 @@ func TestChangeParentOnManagedBy(t *testing.T) {
 	l := zap.New()
 
 	// Make c and d external namespaces
-	f.Get("c").ExternalTreeLabels = map[string]int{"c" + api.LabelTreeDepthSuffix: 0}
-	f.Get("d").ExternalTreeLabels = map[string]int{"d" + api.LabelTreeDepthSuffix: 0}
+	f.Get("c").Manager = "external-tool"
+	f.Get("d").Manager = "external-tool"
 
 	// These cases test changing parent for internal or external namespaces, described
 	// in the table at https://bit.ly/hnc-external-hierarchy#heading=h.z9mkbslfq41g

internal/namespace/validator.go

@@ -149,7 +149,12 @@ func (v *Validator) illegalIncludedNamespaceLabel(req *nsRequest) admission.Resp
 // namespace name cannot be updated.
 func (v *Validator) nameExistsInExternalHierarchy(req *nsRequest) admission.Response {
 	for _, nm := range v.Forest.GetNamespaceNames() {
-		if _, ok := v.Forest.Get(nm).ExternalTreeLabels[req.ns.Name]; ok {
+		ns := v.Forest.Get(nm)
+		if !ns.IsExternal() {
+			continue
+		}
+		externalTreeLabels := ns.GetTreeLabels()
+		if _, ok := externalTreeLabels[req.ns.Name]; ok {
 			msg := fmt.Sprintf("The namespace name %q is reserved by the external hierarchy manager %q.", req.ns.Name, v.Forest.Get(nm).Manager)
 			return webhooks.Deny(metav1.StatusReasonAlreadyExists, msg)
 		}

internal/namespace/validator_test.go

@@ -124,10 +124,10 @@ func TestCreateNamespace(t *testing.T) {
 	f := foresttest.Create("-")
 	vns := &Validator{Forest: f}
 	a := f.Get("a")
-	a.ExternalTreeLabels = map[string]int{
-		nm:       1,
-		a.Name(): 0,
-	}
+	a.SetLabels(map[string]string{
+		nm + api.LabelTreeDepthSuffix:       "1",
+		a.Name() + api.LabelTreeDepthSuffix: "0",
+	})
 
 	// Requested namespace uses "exhier" as name.
 	ns := &corev1.Namespace{}