Merge pull request #769 from yiqigao217/ext-webhook

Update admission controllers for external hierarchy

incubator/hnc/config/webhook/manifests.yaml

@@ -76,6 +76,8 @@ webhooks:
     - v1
     operations:
     - DELETE
+    - CREATE
+    - UPDATE
     resources:
     - namespaces
 - clientConfig:

incubator/hnc/internal/forest/namespace.go

@@ -42,6 +42,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 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.

incubator/hnc/internal/reconcilers/hierarchy_config.go

@@ -227,13 +227,14 @@ func (r *HierarchyConfigReconciler) syncWithForest(log logr.Logger, nsInst *core
 // syncExternalNamespace sets external tree labels to the namespace in the forest
 // if the namespace is an external namespace not managed by HNC.
 func (r *HierarchyConfigReconciler) syncExternalNamespace(log logr.Logger, nsInst *corev1.Namespace, ns *forest.Namespace) {
-	manager := nsInst.Annotations[api.AnnotationManagedBy]
-	if manager == "" || manager == api.MetaGroup {
+	ns.Manager = nsInst.Annotations[api.AnnotationManagedBy]
+	if ns.Manager == "" || ns.Manager == 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
 	}

incubator/hnc/internal/validators/hierarchy.go

@@ -176,6 +176,11 @@ func (v *Hierarchy) checkNS(ns *forest.Namespace) admission.Response {
 
 // checkParent validates if the parent is legal based on the current in-memory state of the forest.
 func (v *Hierarchy) checkParent(ns, curParent, newParent *forest.Namespace) admission.Response {
+	if ns.IsExternal() && newParent != nil {
+		msg := fmt.Sprintf("Namespace %q is managed by %q, not HNC, so it cannot have a parent in HNC.", ns.Name(), ns.Manager)
+		return deny(metav1.StatusReasonForbidden, msg)
+	}
+
 	if curParent == newParent {
 		return allow("parent unchanged")
 	}

incubator/hnc/internal/validators/hierarchy_test.go

@@ -52,6 +52,50 @@ func TestStructure(t *testing.T) {
 	}
 }
 
+func TestChangeParentOnManagedBy(t *testing.T) {
+	f := foresttest.Create("-a-c") // a <- b; c <- d
+	h := &Hierarchy{Forest: f}
+	l := zap.Logger(false)
+
+	// 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}
+
+	// These cases test changing parent for internal or external namespaces, described
+	// in the table at https://bit.ly/hnc-external-hierarchy#heading=h.z9mkbslfq41g
+	// with other cases covered in the namespace_test.go.
+	tests := []struct {
+		name string
+		nnm  string
+		pnm  string
+		fail bool
+	}{
+		{name: "ok: change internal namespace parent from none to existing", nnm: "a", pnm: "c"},
+		{name: "ok: change internal namespace existing parent", nnm: "b", pnm: "c"},
+		{name: "ok: change internal namespace parent from existing to none", nnm: "b", pnm: ""},
+		{name: "not ok: change external namespace parent from none to existing", nnm: "c", pnm: "a", fail: true},
+		{name: "not ok: change external namespace existing parent", nnm: "d", pnm: "a", fail: true},
+		{name: "ok: change external namespace parent from existing to none", nnm: "d", pnm: ""},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			// Setup
+			g := NewGomegaWithT(t)
+			hc := &api.HierarchyConfiguration{Spec: api.HierarchyConfigurationSpec{Parent: tc.pnm}}
+			hc.ObjectMeta.Name = api.Singleton
+			hc.ObjectMeta.Namespace = tc.nnm
+			req := &request{hc: hc}
+
+			// Test
+			got := h.handle(context.Background(), l, req)
+
+			// Report
+			logResult(t, got.AdmissionResponse.Result)
+			g.Expect(got.AdmissionResponse.Allowed).ShouldNot(Equal(tc.fail))
+		})
+	}
+}
+
 func TestAuthz(t *testing.T) {
 	tests := []struct {
 		name   string

incubator/hnc/internal/validators/namespace.go

@@ -23,7 +23,7 @@ const (
 // Note: the validating webhook FAILS CLOSE. This means that if the webhook goes down, all further
 // changes are forbidden.
 //
-// +kubebuilder:webhook:path=/validate-v1-namespace,mutating=false,failurePolicy=fail,groups="",resources=namespaces,verbs=delete,versions=v1,name=namespaces.hnc.x-k8s.io
+// +kubebuilder:webhook:path=/validate-v1-namespace,mutating=false,failurePolicy=fail,groups="",resources=namespaces,verbs=delete;create;update,versions=v1,name=namespaces.hnc.x-k8s.io
 
 type Namespace struct {
 	Log     logr.Logger
@@ -79,7 +79,28 @@ func (v *Namespace) checkForest(req *nsRequest) admission.Response {
 
 	ns := v.Forest.Get(req.ns.Name)
 
-	// Early exit to allow non-delete requests or if the namespace allows cascading deletion.
+	// Allow all create namespace requests except if the namespace name already exists in
+	// the external hierarchy (the apiserver already denies existing namespace names).
+	if req.op == v1beta1.Create {
+		for _, nm := range v.Forest.GetNamespaceNames() {
+			if _, ok := v.Forest.Get(nm).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 deny(metav1.StatusReasonAlreadyExists, msg)
+			}
+		}
+	}
+
+	// Allow all update namespace requests except if the namespace has a parent and wants
+	// to add "hnc.x-k8s.io/managedBy" annotation other than "hnc.x-k8s.io".
+	if req.op == v1beta1.Update && req.ns.Annotations[api.AnnotationManagedBy] != "" {
+		if req.ns.Annotations[api.AnnotationManagedBy] != api.MetaGroup && ns.Parent() != nil {
+			msg := fmt.Sprintf("Namespace %q is a child of %q. Namespaces with parents defined by HNC cannot also be managed externally. "+
+				"To manage this namespace with %q, first make it a root in HNC.", ns.Name(), ns.Parent().Name(), req.ns.Annotations[api.AnnotationManagedBy])
+			return deny(metav1.StatusReasonForbidden, msg)
+		}
+	}
+
+	// Early exit to allow other non-delete requests or if the namespace allows cascading deletion.
 	if req.op != v1beta1.Delete || ns.AllowsCascadingDelete() {
 		return allow("")
 	}

incubator/hnc/internal/validators/namespace_test.go

@@ -81,9 +81,103 @@ func TestDeleteOwnerNamespace(t *testing.T) {
 		// Report - Should allow deleting the parent namespace with allowCascadingDelete set on it.
 		logResult(t, got.AdmissionResponse.Result)
 		g.Expect(got.AdmissionResponse.Allowed).Should(BeTrue())
+	})
+}
+
+func TestCreateNamespace(t *testing.T) {
+	// nm is the name of the namespace to be created, which already exists in external hierarchy.
+	nm := "exhier"
+
+	// Create a single external namespace "a" with "exhier" in the external hierarchy.
+	f := foresttest.Create("-")
+	vns := &Namespace{Forest: f}
+	a := f.Get("a")
+	a.ExternalTreeLabels = map[string]int{
+		nm:       1,
+		a.Name(): 0,
+	}
+
+	// Requested namespace uses "exhier" as name.
+	ns := &corev1.Namespace{}
+	ns.Name = nm
+
+	t.Run("Create namespace with an already existing name in external hierarchy", func(t *testing.T) {
+		g := NewGomegaWithT(t)
+		req := &nsRequest{
+			ns: ns,
+			op: v1beta1.Create,
+		}
 
+		// Test
+		got := vns.handle(req)
+
+		// Report
+		logResult(t, got.AdmissionResponse.Result)
+		g.Expect(got.AdmissionResponse.Allowed).Should(BeFalse())
 	})
+}
+
+func TestUpdateNamespaceManagedBy(t *testing.T) {
+	f := foresttest.Create("-a-c") // a <- b; c <- d
+	vns := &Namespace{Forest: f}
 
+	aInst := &corev1.Namespace{}
+	aInst.Name = "a"
+	bInst := &corev1.Namespace{}
+	bInst.Name = "b"
+
+	// Add 'hnc.x-k8s.io/managedBy: other' annotation on c.
+	cInst := &corev1.Namespace{}
+	cInst.Name = "c"
+	cInst.SetAnnotations(map[string]string{api.AnnotationManagedBy: "other"})
+
+	// ** Please note this will make d in an *illegal* state. **
+	// Add 'hnc.x-k8s.io/managedBy: other' annotation on d.
+	dInst := &corev1.Namespace{}
+	dInst.Name = "d"
+	dInst.SetAnnotations(map[string]string{api.AnnotationManagedBy: "other"})
+
+	// These cases test converting namespaces between internal and external, described
+	// in the table at https://bit.ly/hnc-external-hierarchy#heading=h.z9mkbslfq41g
+	// with other cases covered in the hierarchy_test.go.
+	tests := []struct {
+		name      string
+		nsInst    *corev1.Namespace
+		managedBy string
+		fail      bool
+	}{
+		{name: "ok: default (no annotation)", nsInst: aInst, managedBy: ""},
+		{name: "ok: explicitly managed by HNC", nsInst: aInst, managedBy: "hnc.x-k8s.io"},
+		{name: "ok: convert a root internal namespace to external", nsInst: aInst, managedBy: "other"},
+		{name: "not ok: convert a non-root internal namespace to external", nsInst: bInst, managedBy: "other", fail: true},
+		{name: "ok: convert an external namespace to internal by changing annotation value", nsInst: cInst, managedBy: "hnc.x-k8s.io"},
+		{name: "ok: convert an external namespace to internal by removing annotation", nsInst: cInst, managedBy: ""},
+		{name: "ok: resolve illegal state by changing annotation value", nsInst: dInst, managedBy: "hnc.x-k8s.io"},
+		{name: "ok: resolve illegal state by removing annotation", nsInst: dInst, managedBy: ""},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewGomegaWithT(t)
+			tnsInst := tc.nsInst
+			if tc.managedBy == "" {
+				tnsInst.SetAnnotations(map[string]string{})
+			} else {
+				tnsInst.SetAnnotations(map[string]string{api.AnnotationManagedBy: tc.managedBy})
+			}
+
+			req := &nsRequest{
+				ns: tc.nsInst,
+				op: v1beta1.Update,
+			}
+
+			// Test
+			got := vns.handle(req)
+
+			// Report
+			logResult(t, got.AdmissionResponse.Result)
+			g.Expect(got.AdmissionResponse.Allowed).ShouldNot(Equal(tc.fail))
+		})
+	}
 }
 
 func setSubAnnotation(ns *corev1.Namespace) {