Replace included-namespace VWH rules with MWH

Part of 13.
Fixes 37.

Add mutating webhook to set the correct `included-namespace` label on
non-excluded namespaces and ensure it's removed from any excluded
namespace.

Update the namespace tree label update logs and function name in the HC
reconciler to make it clear that the HC reconciler only updates the tree
labels and won't update included-namespace label if the MWH works fine.

Remove the `illegalIncludedNamespace` checks in the namespace validator.

Tested by `make test` with new test cases. Also tested manually by
applying namespace manifests with different labels, viewed the logs and
verified the MWH works fine.

Author: yiqigao217

cmd/manager/main.go

@@ -41,6 +41,7 @@ import (
 	v1a2 "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2"
 	"sigs.k8s.io/hierarchical-namespaces/internal/config"
 	"sigs.k8s.io/hierarchical-namespaces/internal/forest"
+	"sigs.k8s.io/hierarchical-namespaces/internal/mutator"
 	"sigs.k8s.io/hierarchical-namespaces/internal/reconcilers"
 	"sigs.k8s.io/hierarchical-namespaces/internal/stats"
 	"sigs.k8s.io/hierarchical-namespaces/internal/validators"
@@ -213,6 +214,10 @@ func startControllers(mgr ctrl.Manager, certsCreated chan struct{}) {
 		validators.Create(mgr, f)
 	}
 
+	// Create mutating admission controllers.
+	setupLog.Info("Registering mutating webhook")
+	mutator.Create(mgr)
+
 	// Create all reconciling controllers
 	setupLog.Info("Creating controllers", "maxReconciles", maxReconciles)
 	if err := reconcilers.Create(mgr, f, maxReconciles, false); err != nil {

config/webhook/manifests.yaml

@@ -1,4 +1,33 @@
 
+---
+apiVersion: admissionregistration.k8s.io/v1
+kind: MutatingWebhookConfiguration
+metadata:
+  creationTimestamp: null
+  name: mutating-webhook-configuration
+webhooks:
+- admissionReviewVersions:
+  - v1
+  - v1beta1
+  clientConfig:
+    service:
+      name: webhook-service
+      namespace: system
+      path: /mutate-namespace-included-label
+  failurePolicy: Ignore
+  name: namespacelabel.hnc.x-k8s.io
+  rules:
+  - apiGroups:
+    - ""
+    apiVersions:
+    - v1
+    operations:
+    - CREATE
+    - UPDATE
+    resources:
+    - namespaces
+  sideEffects: None
+
 ---
 apiVersion: admissionregistration.k8s.io/v1
 kind: ValidatingWebhookConfiguration

internal/mutator/namespace_included_label.go

@@ -0,0 +1,77 @@
+package mutator
+
+import (
+	"context"
+	"encoding/json"
+	"fmt"
+	"net/http"
+
+	"github.com/go-logr/logr"
+	corev1 "k8s.io/api/core/v1"
+	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
+
+	api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2"
+	"sigs.k8s.io/hierarchical-namespaces/internal/config"
+)
+
+// NamespaceIncludedLabelServingPath is where the mutator will run. Must be kept
+// in sync with the kubebuilder markers below.
+const (
+	NamespaceIncludedLabelServingPath = "/mutate-namespace-included-label"
+)
+
+// Note: the mutating webhook FAILS OPEN. This means that if the webhook goes
+// down, all further changes are allowed. (An empty line has to be kept below
+// the kubebuilder marker for the controller-gen to generate manifests.)
+//
+// +kubebuilder:webhook:admissionReviewVersions=v1;v1beta1,path=/mutate-namespace-included-label,mutating=true,failurePolicy=ignore,groups="",resources=namespaces,sideEffects=None,verbs=create;update,versions=v1,name=namespacelabel.hnc.x-k8s.io
+
+type NamespaceIncludedLabel struct {
+	Log     logr.Logger
+	decoder *admission.Decoder
+}
+
+// Handle implements the mutating webhook.
+func (n *NamespaceIncludedLabel) Handle(ctx context.Context, req admission.Request) admission.Response {
+	log := n.Log.WithValues("namespace", req.Name)
+	ns := &corev1.Namespace{}
+	err := n.decoder.Decode(req, ns)
+	if err != nil {
+		return admission.Errored(http.StatusBadRequest, err)
+	}
+	log.V(1).Info(fmt.Sprintf("%+v\n", ns))
+
+	n.handle(log, ns)
+	marshaledNS, err := json.Marshal(ns)
+	if err != nil {
+		return admission.Errored(http.StatusInternalServerError, err)
+	}
+	return admission.PatchResponseFromRaw(req.Object.Raw, marshaledNS)
+}
+
+// handle implements the non-boilerplate logic of this mutator, allowing it to
+// be more easily unit tested (ie without constructing a full admission.Request).
+func (n *NamespaceIncludedLabel) handle(log logr.Logger, ns *corev1.Namespace) {
+	isExcluded := config.ExcludedNamespaces[ns.Name]
+
+	// Ensure the labels map is not nil and remove the included-namespace label if
+	// it exists.
+	if ns.Labels == nil {
+		ns.Labels = map[string]string{}
+	}
+	delete(ns.Labels, api.LabelIncludedNamespace)
+
+	if !isExcluded {
+		log.V(1).Info("Not an excluded namespace; set included-namespace label.")
+		ns.Labels[api.LabelIncludedNamespace] = "true"
+	} else {
+		// The included-namespace label is removed earlier.
+		log.V(1).Info("Excluded namespace detected; clean included-namespace label.")
+	}
+}
+
+// InjectDecoder injects the decoder.
+func (n *NamespaceIncludedLabel) InjectDecoder(d *admission.Decoder) error {
+	n.decoder = d
+	return nil
+}

internal/mutator/namespace_included_label_test.go

@@ -0,0 +1,49 @@
+package mutator
+
+import (
+	"testing"
+
+	. "github.com/onsi/gomega"
+	corev1 "k8s.io/api/core/v1"
+	"sigs.k8s.io/controller-runtime/pkg/log/zap"
+
+	api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2"
+	"sigs.k8s.io/hierarchical-namespaces/internal/config"
+)
+
+func TestMutateNamespaceIncludedLabel(t *testing.T) {
+	m := &NamespaceIncludedLabel{}
+	l := zap.New()
+	config.ExcludedNamespaces = map[string]bool{"excluded": true}
+
+	tests := []struct {
+		name       string
+		nsn        string
+		lval       string
+		expectlval string
+	}{
+		{name: "Should set label on non-excluded namespace without label", nsn: "a", expectlval: "true"},
+		{name: "Should keep label on non-excluded namespace with label (e.g. from a yaml file)", nsn: "a", lval: "true", expectlval: "true"},
+		{name: "Should set correct label on non-excluded namespace with label with wrong value(e.g. from a yaml file)", nsn: "a", lval: "wrong", expectlval: "true"},
+		{name: "Should keep no label on excluded namespace without label", nsn: "excluded"},
+		{name: "Should remove label on excluded namespace with label (e.g. from a yaml file)", nsn: "excluded", lval: "true"},
+		{name: "Should remove label on excluded namespace with label with wrong value (e.g. from a yaml file)", nsn: "excluded", lval: "wrong"},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			nsInst := &corev1.Namespace{}
+			nsInst.Name = tc.nsn
+			if tc.lval != "" {
+				nsInst.SetLabels(map[string]string{api.LabelIncludedNamespace: tc.lval})
+			}
+
+			// Test
+			m.handle(l, nsInst)
+
+			// Report
+			g.Expect(nsInst.Labels[api.LabelIncludedNamespace]).Should(Equal(tc.expectlval))
+		})
+	}
+}

internal/mutator/setup.go

@@ -0,0 +1,14 @@
+package mutator
+
+import (
+	ctrl "sigs.k8s.io/controller-runtime"
+	"sigs.k8s.io/controller-runtime/pkg/webhook"
+)
+
+// Create creates the mutator. This function is called from main.go.
+func Create(mgr ctrl.Manager) {
+	// Create mutator for namespace `included-namespace` label.
+	mgr.GetWebhookServer().Register(NamespaceIncludedLabelServingPath, &webhook.Admission{Handler: &NamespaceIncludedLabel{
+		Log: ctrl.Log.WithName("mutators").WithName("NamespaceIncludedLabel"),
+	}})
+}

internal/reconcilers/hierarchy_config.go

@@ -298,7 +298,7 @@ func (r *HierarchyConfigReconciler) syncWithForest(log logr.Logger, nsInst *core
 	r.syncConditions(log, inst, ns, deletingCRD, hadCrit)
 
 	// Sync the tree labels. This should go last since it can depend on the conditions.
-	nsCustomerLabelUpdated := r.syncLabel(log, nsInst, ns)
+	nsCustomerLabelUpdated := r.syncTreeLabels(log, nsInst, ns)
 
 	return initial || nsCustomerLabelUpdated
 }
@@ -461,8 +461,8 @@ func (r *HierarchyConfigReconciler) syncAnchors(log logr.Logger, ns *forest.Name
 	}
 }
 
-// Sync namespace tree labels and other labels. Return true if the labels are updated.
-func (r *HierarchyConfigReconciler) syncLabel(log logr.Logger, nsInst *corev1.Namespace, ns *forest.Namespace) bool {
+// Sync namespace tree labels. Return true if the labels are updated.
+func (r *HierarchyConfigReconciler) syncTreeLabels(log logr.Logger, nsInst *corev1.Namespace, ns *forest.Namespace) bool {
 	if ns.IsExternal() {
 		metadata.SetLabel(nsInst, nsInst.Name+api.LabelTreeDepthSuffix, "0")
 		return false
@@ -504,7 +504,7 @@ func (r *HierarchyConfigReconciler) syncLabel(log logr.Logger, nsInst *corev1.Na
 	// 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 labels have been updated.")
+		log.Info("Namespace tree labels have been updated.")
 		return true
 	}
 	return false

internal/validators/namespace.go

@@ -9,8 +9,6 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
-	"sigs.k8s.io/hierarchical-namespaces/internal/config"
-
 	api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2"
 	"sigs.k8s.io/hierarchical-namespaces/internal/forest"
 )
@@ -76,18 +74,12 @@ func (v *Namespace) handle(req *nsRequest) admission.Response {
 
 	switch req.op {
 	case k8sadm.Create:
-		if rsp := v.illegalIncludedNamespaceLabel(req); !rsp.Allowed {
-			return rsp
-		}
 		// This check only applies to the Create operation since namespace name
 		// cannot be updated.
 		if rsp := v.nameExistsInExternalHierarchy(req); !rsp.Allowed {
 			return rsp
 		}
 	case k8sadm.Update:
-		if rsp := v.illegalIncludedNamespaceLabel(req); !rsp.Allowed {
-			return rsp
-		}
 		// This check only applies to the Update operation. Creating a namespace
 		// with external manager is allowed and we will prevent this conflict by not
 		// allowing setting a parent when validating the HierarchyConfiguration.
@@ -106,49 +98,6 @@ func (v *Namespace) handle(req *nsRequest) admission.Response {
 	return allow("")
 }
 
-// illegalIncludedNamespaceLabel checks if there's any illegal use of the
-// included-namespace label on namespaces.
-// TODO these validating rules will be removed and converted into a mutating
-//  webhook. See https://github.com/kubernetes-sigs/hierarchical-namespaces/issues/37
-func (v *Namespace) illegalIncludedNamespaceLabel(req *nsRequest) admission.Response {
-	labelValue, hasLabel := req.ns.Labels[api.LabelIncludedNamespace]
-	isExcluded := config.ExcludedNamespaces[req.ns.Name]
-
-	// An excluded namespace should not have the included-namespace label.
-	//
-	// Note: this only blocks the request if the excluded namespace is newly
-	// created or is updated with a newly added included-namespace label because
-	// existing illegal included-namespace label should have already been removed
-	// by our reconciler. For example, even when the VWHConfiguration is removed,
-	// adding the label to an excluded namespace would pass but the label is
-	// immediately removed; when the VWHConfiguration is there but the reconcilers
-	// are down, any request gets denied anyway.
-	if isExcluded && hasLabel {
-		// Todo add label concept and add `See https://github.com/kubernetes-sigs/hierarchical-namespaces/blob/master/docs/user-guide/concepts.md#included-namespace-label for detail`
-		//  to the message below. Github issue - https://github.com/kubernetes-sigs/hierarchical-namespaces/issues/9
-		msg := fmt.Sprintf("You cannot enforce webhook rules on this excluded namespace using the %q label.", api.LabelIncludedNamespace)
-		return deny(metav1.StatusReasonForbidden, msg)
-	}
-
-	// An included-namespace should not have the included-namespace label with a
-	// value other than "true" at any time. We will allow updating an included
-	// namespace by removing its included-namespace label, since people or system
-	// may apply namespace manifests (without the label) automatically and it
-	// doesn't make sense to block their attempts after the first time.
-	//
-	// Note: this only blocks the request if the included namespace is just
-	// created or updated with a wrong value for the included-namespace label,
-	// because our reconciler should already set it correctly before the request.
-	if !isExcluded && hasLabel && labelValue != "true" {
-		// Todo add label concept and add `See https://github.com/kubernetes-sigs/hierarchical-namespaces/blob/master/docs/user-guide/concepts.md#included-namespace-label for detail`
-		//  to the message below. Github issue - https://github.com/kubernetes-sigs/hierarchical-namespaces/issues/9
-		msg := fmt.Sprintf("You cannot unset the value of the %q label. Only HNC can edit this label on namespaces.", api.LabelIncludedNamespace)
-		return deny(metav1.StatusReasonForbidden, msg)
-	}
-
-	return allow("")
-}
-
 func (v *Namespace) nameExistsInExternalHierarchy(req *nsRequest) admission.Response {
 	for _, nm := range v.Forest.GetNamespaceNames() {
 		if _, ok := v.Forest.Get(nm).ExternalTreeLabels[req.ns.Name]; ok {

internal/validators/namespace_test.go

@@ -6,7 +6,6 @@ import (
 	. "github.com/onsi/gomega"
 	k8sadm "k8s.io/api/admission/v1"
 	corev1 "k8s.io/api/core/v1"
-	"sigs.k8s.io/hierarchical-namespaces/internal/config"
 
 	api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2"
 	"sigs.k8s.io/hierarchical-namespaces/internal/foresttest"
@@ -227,59 +226,3 @@ func setSubAnnotation(ns *corev1.Namespace, pnm string) {
 	}
 	ns.SetAnnotations(a)
 }
-
-func TestIllegalIncludedNamespaceNamespace(t *testing.T) {
-	f := foresttest.Create("-a-c") // a <- b; c <- d
-	vns := &Namespace{Forest: f}
-	config.ExcludedNamespaces["excluded"] = true
-
-	tests := []struct {
-		name       string
-		excludedNS bool
-		lval       string
-		op         k8sadm.Operation
-		fail       bool
-	}{
-		{name: "allow creating non-excluded namespace without label", op: k8sadm.Create},
-		{name: "allow creating non-excluded namespace with label (e.g. from a yaml file)", lval: "true", op: k8sadm.Create},
-		{name: "deny creating non-excluded namespace with label with wrong value(e.g. from a yaml file)", lval: "else", op: k8sadm.Create, fail: true},
-		{name: "allow creating excluded namespace without label", excludedNS: true, op: k8sadm.Create},
-		{name: "deny creating excluded namespace with label", excludedNS: true, lval: "true", op: k8sadm.Create, fail: true},
-
-		{name: "allow updating non-excluded namespace with correct label (which is actually set by HC reconciler)", lval: "true", op: k8sadm.Update},
-		{name: "allow updating non-excluded namespace without label", op: k8sadm.Update},
-		{name: "deny updating non-excluded namespace with incorrect label", lval: "else", op: k8sadm.Update, fail: true},
-		{name: "allow updating excluded namespace without label", excludedNS: true, op: k8sadm.Update},
-		{name: "deny updating excluded namespace with label", excludedNS: true, lval: "true", op: k8sadm.Update, fail: true},
-
-		{name: "allow deleting non-excluded namespace without label", op: k8sadm.Delete},
-		{name: "allow deleting non-excluded namespace with label", lval: "true", op: k8sadm.Delete},
-		{name: "allow creating excluded namespace without label", excludedNS: true, op: k8sadm.Delete},
-		{name: "allow creating excluded namespace with label", excludedNS: true, lval: "true", op: k8sadm.Delete},
-	}
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			g := NewWithT(t)
-
-			nsInst := &corev1.Namespace{}
-			nsInst.Name = "a"
-			if tc.excludedNS {
-				nsInst.Name = "excluded"
-			}
-			if tc.lval != "" {
-				nsInst.SetLabels(map[string]string{api.LabelIncludedNamespace: tc.lval})
-			}
-			req := &nsRequest{
-				ns: nsInst,
-				op: tc.op,
-			}
-
-			// Test
-			got := vns.handle(req)
-
-			// Report
-			logResult(t, got.AdmissionResponse.Result)
-			g.Expect(got.AdmissionResponse.Allowed).ShouldNot(Equal(tc.fail))
-		})
-	}
-}

internal/validators/setup.go

@@ -14,6 +14,7 @@ import (
 const (
 	serviceName     = "hnc-webhook-service"
 	vwhName         = "hnc-validating-webhook-configuration"
+	mwhName         = "hnc-mutating-webhook-configuration"
 	caName          = "hnc-ca"
 	caOrganization  = "hnc"
 	secretNamespace = "hnc-system"
@@ -45,6 +46,9 @@ func CreateCertsIfNeeded(mgr ctrl.Manager, novalidation, internalCert, restartOn
 		Webhooks: []cert.WebhookInfo{{
 			Type: cert.Validating,
 			Name: vwhName,
+		}, {
+			Type: cert.Mutating,
+			Name: mwhName,
 		}},
 		RestartOnSecretRefresh: restartOnSecretRefresh,
 	})