Handle bypassed webhook rules on excluded namespaces

Fixes 1443

Remove any HNC CRs (hierarchyconfiguration and anchor) from the excluded
namespaces if they are successfully created bypassing the webhook.

Set "ActivitiesHalted: IllegalParent" condition if a namespace sets an
excluded namespace as a parent, e.g.
```
$ kubectl hns describe a
Hierarchy configuration for namespace a
  Parent: kube-system
  Children:
  - d (subnamespace)
  Conditions:
  - ActivitiesHalted (IllegalParent): Parent "kube-system" is an excluded namespace
```

Subnamespace anchor using an excluded namespace name would get
`Forbidden` state. Update the `kubectl hns describe` to show:
```
$ kubectl hns describe parent
Hierarchy configuration for namespace parent
  No parent
  Children:
  - d (Conflict subnamespace)
  - e (subnamespace)
  - f
  - kube-system (Forbidden subnamespace)
  No conditions
```

Tested by `make test` since there's no wehbook in the integration tests.

Author: yiqigao217

incubator/hnc/api/v1alpha2/hierarchy_types.go

@@ -59,6 +59,7 @@ const (
 	ReasonDeletingCRD   string = "DeletingCRD"
 	ReasonInCycle       string = "InCycle"
 	ReasonParentMissing string = "ParentMissing"
+	ReasonIllegalParent string = "IllegalParent"
 	ReasonAnchorMissing string = "SubnamespaceAnchorMissing"
 )
 
@@ -71,6 +72,7 @@ var AllConditions = map[string][]string{
 		ReasonDeletingCRD,
 		ReasonInCycle,
 		ReasonParentMissing,
+		ReasonIllegalParent,
 	},
 	ConditionBadConfiguration: {
 		ReasonAnchorMissing,

incubator/hnc/internal/kubectl/describe.go

@@ -39,7 +39,7 @@ var describeCmd = &cobra.Command{
 		nnm := args[0]
 		fmt.Printf("Hierarchy configuration for namespace %s\n", nnm)
 		hier := client.getHierarchy(nnm)
-		anms := client.getAnchorNames(nnm)
+		as := client.getAnchorStatus(nnm)
 
 		// Parent
 		if hier.Spec.Parent != "" {
@@ -53,11 +53,11 @@ var describeCmd = &cobra.Command{
 		for _, cn := range hier.Status.Children {
 			childrenAndStatus[cn] = ""
 		}
-		for _, cn := range anms {
+		for cn, st := range as {
 			if _, ok := childrenAndStatus[cn]; ok {
 				childrenAndStatus[cn] = "subnamespace"
 			} else {
-				childrenAndStatus[cn] = "missing subnamespace"
+				childrenAndStatus[cn] = st + " subnamespace"
 			}
 		}
 		if len(childrenAndStatus) > 0 {

incubator/hnc/internal/kubectl/root.go

@@ -25,7 +25,6 @@ import (
 	"github.com/spf13/cobra"
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
-	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime/serializer"
 	"k8s.io/cli-runtime/pkg/genericclioptions"
 	"k8s.io/client-go/kubernetes"
@@ -43,12 +42,13 @@ var rootCmd *cobra.Command
 var client Client
 
 type realClient struct{}
+type anchorStatus map[string]string
 
 type Client interface {
 	getHierarchy(nnm string) *api.HierarchyConfiguration
 	updateHierarchy(hier *api.HierarchyConfiguration, reason string)
 	createAnchor(nnm string, hnnm string)
-	getAnchorNames(nnm string) []string
+	getAnchorStatus(nnm string) anchorStatus
 	getHNCConfig() *api.HNCConfiguration
 	updateHNCConfig(*api.HNCConfiguration)
 }
@@ -137,28 +137,25 @@ func (cl *realClient) getHierarchy(nnm string) *api.HierarchyConfiguration {
 	return hier
 }
 
-func (cl *realClient) getAnchorNames(nnm string) []string {
+func (cl *realClient) getAnchorStatus(nnm string) anchorStatus {
 	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 	defer cancel()
 
-	var anms []string
-
 	// List all the anchors in the namespace.
-	ul := &unstructured.UnstructuredList{}
-	ul.SetKind(api.AnchorKind)
-	ul.SetAPIVersion(api.AnchorAPIVersion)
-	err := hncClient.Get().Resource(api.Anchors).Namespace(nnm).Do(ctx).Into(ul)
+	al := &api.SubnamespaceAnchorList{}
+	err := hncClient.Get().Resource(api.Anchors).Namespace(nnm).Do(ctx).Into(al)
 	if err != nil && !errors.IsNotFound(err) {
 		fmt.Printf("Error listing subnamespace anchors for %s: %s\n", nnm, err)
 		os.Exit(1)
 	}
 
-	// Create a list of strings of the anchor names.
-	for _, inst := range ul.Items {
-		anms = append(anms, inst.GetName())
+	// Create the map from anchor names to their status.
+	as := make(anchorStatus)
+	for _, inst := range al.Items {
+		as[inst.Name] = string(inst.Status.State)
 	}
 
-	return anms
+	return as
 }
 
 func (cl *realClient) updateHierarchy(hier *api.HierarchyConfiguration, reason string) {

incubator/hnc/internal/reconcilers/anchor.go

@@ -18,6 +18,7 @@ package reconcilers
 import (
 	"context"
 	"errors"
+	"fmt"
 
 	"github.com/go-logr/logr"
 	corev1 "k8s.io/api/core/v1"
@@ -72,12 +73,21 @@ func (r *AnchorReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 		return ctrl.Result{}, err
 	}
 
-	// Report "Forbidden" state and early exit if the namespace is not allowed to have subnamespaces
-	// but has bypassed the webhook and successfully created the anchor. Forbidden anchors won't have
-	// finalizers.
-	// TODO refactor/split the ExcludedNamespaces map for 1) reconciler exclusion and 2) subnamespaces exclusion
-	//  purposes. See issue: https://github.com/kubernetes-sigs/multi-tenancy/issues/495
+	// Always delete anchor (and any other HNC CRs) in the excluded namespaces and
+	// early exit.
 	if config.ExcludedNamespaces[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)
+	}
+
+	// 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.ExcludedNamespaces[nm] {
 		inst.Status.State = api.Forbidden
 		return ctrl.Result{}, r.writeInstance(ctx, log, inst)
 	}
@@ -335,6 +345,15 @@ func (r *AnchorReconciler) writeInstance(ctx context.Context, log logr.Logger, i
 	return nil
 }
 
+// deleteInstance deletes the anchor instance. Note: Make sure there's no
+// finalizers on the instance before calling this function.
+func (r *AnchorReconciler) deleteInstance(ctx context.Context, log logr.Logger, inst *api.SubnamespaceAnchor) error {
+	if err := r.Delete(ctx, inst); err != nil {
+		return fmt.Errorf("while deleting on apiserver: %w", err)
+	}
+	return nil
+}
+
 // getNamespace returns the namespace if it exists, or returns an invalid, blank, unnamed one if it
 // doesn't. This allows it to be trivially identified as a namespace that doesn't exist, and also
 // allows us to easily modify it if we want to create it.

incubator/hnc/internal/reconcilers/anchor_test.go

@@ -22,6 +22,7 @@ var _ = Describe("Anchor", func() {
 	BeforeEach(func() {
 		fooName = createNS(ctx, "foo")
 		barName = createNSName("bar")
+		config.ExcludedNamespaces = nil
 	})
 
 	It("should create an subnamespace and update the hierarchy according to the anchor", func() {
@@ -50,11 +51,18 @@ var _ = Describe("Anchor", func() {
 		Eventually(getAnchorState(ctx, fooName, barName)).Should(Equal(api.Ok))
 	})
 
-	It("should set the anchor.status.state to Forbidden if the parent is not allowed to have subnamespaces", func() {
+	It("should remove the anchor in an excluded namespace", func() {
 		config.ExcludedNamespaces = map[string]bool{"kube-system": true}
 		kube_system_anchor_bar := newAnchor(barName, "kube-system")
 		updateAnchor(ctx, kube_system_anchor_bar)
-		Eventually(getAnchorState(ctx, "kube-system", barName)).Should(Equal(api.Forbidden))
+		Eventually(canGetAnchor(ctx, barName, "kube-system")).Should(Equal(false))
+	})
+
+	It("should set the anchor.status.state to Forbidden if the subnamespace is an excluded namespace", func() {
+		config.ExcludedNamespaces = map[string]bool{"kube-system": true}
+		foo_anchor_kube_system := newAnchor("kube-system", fooName)
+		updateAnchor(ctx, foo_anchor_kube_system)
+		Eventually(getAnchorState(ctx, fooName, "kube-system")).Should(Equal(api.Forbidden))
 	})
 
 	It("should set the anchor.status.state to Conflict if a namespace of the same name already exists", func() {
@@ -122,6 +130,17 @@ func getAnchorWithOffset(offset int, ctx context.Context, pnm, nm string) *api.S
 	return anchor
 }
 
+func canGetAnchor(ctx context.Context, pnm, nm string) func() bool {
+	return func() bool {
+		nsn := types.NamespacedName{Name: nm, Namespace: pnm}
+		anchor := &api.SubnamespaceAnchor{}
+		if err := k8sClient.Get(ctx, nsn, anchor); err != nil {
+			return false
+		}
+		return true
+	}
+}
+
 func updateAnchor(ctx context.Context, anchor *api.SubnamespaceAnchor) {
 	if anchor.CreationTimestamp.IsZero() {
 		ExpectWithOffset(1, k8sClient.Create(ctx, anchor)).Should(Succeed())

incubator/hnc/internal/reconcilers/hierarchy_config.go

@@ -85,17 +85,21 @@ type HierarchyConfigReconciler struct {
 
 // Reconcile sets up some basic variables and then calls the business logic.
 func (r *HierarchyConfigReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
-	if config.ExcludedNamespaces[req.Namespace] {
-		return ctrl.Result{}, nil
+	ns := req.NamespacedName.Namespace
+	log := loggerWithRID(r.Log).WithValues("ns", ns)
+
+	// Always delete hierarchyconfiguration (and any other HNC CRs) in the
+	// excluded namespaces and early exit.
+	if config.ExcludedNamespaces[ns] {
+		// 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.
+		return ctrl.Result{}, r.deleteSingletonIfExists(ctx, log, ns)
 	}
 
 	stats.StartHierConfigReconcile()
 	defer stats.StopHierConfigReconcile()
 
-	ns := req.NamespacedName.Namespace
-
-	log := loggerWithRID(r.Log).WithValues("ns", ns)
-
 	return ctrl.Result{}, r.reconcile(ctx, log, ns)
 }
 
@@ -373,8 +377,11 @@ func (r *HierarchyConfigReconciler) syncParent(log logr.Logger, inst *api.Hierar
 
 	// Sync this namespace with its current parent.
 	curParent := r.Forest.Get(inst.Spec.Parent)
-	if curParent != nil && !curParent.Exists() {
-		log.Info("The parent doesn't appear to exist (or hasn't been synced yet)", "parent", inst.Spec.Parent)
+	if config.ExcludedNamespaces[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))
+	} 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))
 	}
 
@@ -581,6 +588,37 @@ func (r *HierarchyConfigReconciler) writeHierarchy(ctx context.Context, log logr
 	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 *HierarchyConfigReconciler) 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 *HierarchyConfigReconciler) writeNamespace(ctx context.Context, log logr.Logger, orig, inst *corev1.Namespace) (bool, error) {
 	if reflect.DeepEqual(orig, inst) {
 		return false, nil

incubator/hnc/internal/reconcilers/hierarchy_config_test.go

@@ -22,6 +22,7 @@ var _ = Describe("Hierarchy", func() {
 	BeforeEach(func() {
 		fooName = createNS(ctx, "foo")
 		barName = createNS(ctx, "bar")
+		config.ExcludedNamespaces = nil
 	})
 
 	It("should set a child on the parent", func() {
@@ -31,6 +32,28 @@ 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.ExcludedNamespaces = map[string]bool{"kube-system": true}
+		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.ExcludedNamespaces = map[string]bool{"kube-system": true}
+		barHier := newHierarchy(barName)
+		barHier.Spec.Parent = "kube-system"
+		updateHierarchy(ctx, barHier)
+		// Bar singleton should have "IllegalParent" and no "ParentMissing" condition.
+		Eventually(hasCondition(ctx, barName, api.ConditionActivitiesHalted, api.ReasonIllegalParent)).Should(Equal(true))
+		Eventually(hasCondition(ctx, barName, api.ConditionActivitiesHalted, api.ReasonParentMissing)).Should(Equal(false))
+	})
+
 	It("should set ParentMissing condition if the parent is missing", func() {
 		// Set up the parent-child relationship
 		barHier := newHierarchy(barName)

incubator/hnc/internal/reconcilers/test_helpers_test.go

@@ -51,6 +51,17 @@ func getHierarchy(ctx context.Context, nm string) *api.HierarchyConfiguration {
 	return hier
 }
 
+func canGetHierarchy(ctx context.Context, nm string) func() bool {
+	return func() bool {
+		nnm := types.NamespacedName{Namespace: nm, Name: api.Singleton}
+		hier := &api.HierarchyConfiguration{}
+		if err := k8sClient.Get(ctx, nnm, hier); err != nil {
+			return false
+		}
+		return true
+	}
+}
+
 func updateHierarchy(ctx context.Context, h *api.HierarchyConfiguration) {
 	if h.CreationTimestamp.IsZero() {
 		ExpectWithOffset(1, k8sClient.Create(ctx, h)).Should(Succeed())