Merge pull request #121 from adrianludwin/no-ext-tree

Don't store external tree labels explicitly

Author: k8s-ci-robot

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

@@ -194,7 +194,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{}