Restructure by feature

HNC has always been divided into three main packages (all inside
internal/): reconcilers, validators and mutators. This meant that any
one feature (like subnamespace anchors) were split across multiple
packages, with their internals exposed to other packages. It also
ensured that all tests ran serially.

This change refactors those three packages into five major ones
(anchor, namespace, hierarchyconfig, hncconfig, and objects), as well as
a few minor ones to cover code that's now shared across them (such as
integtest).

Tested: all integ and smoke tests pass.

Makefile

@@ -89,7 +89,7 @@ test: build
 	@echo "If tests fail due to no matches for kind \"CustomResourceDefinition\" in version \"apiextensions.k8s.io/v1\","
 	@echo "please remove the old kubebuilder and reinstall it - https://book.kubebuilder.io/quick-start.html#installation"
 	@echo
-	go test ${DIRS} -coverprofile cover.out
+	go test ${DIRS} -coverprofile cover.out -v
 
 # Builds all binaries (manager and kubectl) and manifests
 build: generate fmt vet manifests

cmd/manager/main.go

@@ -41,10 +41,8 @@ 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/mutators"
-	"sigs.k8s.io/hierarchical-namespaces/internal/reconcilers"
+	"sigs.k8s.io/hierarchical-namespaces/internal/setup"
 	"sigs.k8s.io/hierarchical-namespaces/internal/stats"
-	"sigs.k8s.io/hierarchical-namespaces/internal/validators"
 )
 
 var (
@@ -183,7 +181,7 @@ func main() {
 
 	// Make sure certs are generated and valid if webhooks are enabled and internal certs are used.
 	setupLog.Info("Starting certificate generation")
-	certsCreated, err := validators.CreateCertsIfNeeded(mgr, novalidation, internalCert, restartOnSecretRefresh)
+	certsCreated, err := setup.CreateCertsIfNeeded(mgr, novalidation, internalCert, restartOnSecretRefresh)
 	if err != nil {
 		setupLog.Error(err, "unable to set up cert rotation")
 		os.Exit(1)
@@ -212,19 +210,15 @@ func startControllers(mgr ctrl.Manager, certsCreated chan struct{}) {
 	// other components.
 	f := forest.NewForest()
 
-	// Create all validating admission controllers.
+	// Create all validating and mutating admission controllers.
 	if !novalidation {
 		setupLog.Info("Registering validating webhook (won't work when running locally; use --novalidation)")
-		validators.Create(mgr, f)
+		setup.CreateWebhooks(mgr, f)
 	}
 
-	// Create mutating admission controllers.
-	setupLog.Info("Registering mutating webhook")
-	mutators.Create(mgr)
-
 	// Create all reconciling controllers
 	setupLog.Info("Creating controllers", "maxReconciles", maxReconciles)
-	if err := reconcilers.Create(mgr, f, maxReconciles, false); err != nil {
+	if err := setup.CreateReconcilers(mgr, f, maxReconciles, false); err != nil {
 		setupLog.Error(err, "cannot create controllers")
 		os.Exit(1)
 	}

config/webhook/manifests.yaml

@@ -80,60 +80,60 @@ webhooks:
     service:
       name: webhook-service
       namespace: system
-      path: /validate-hnc-x-k8s-io-v1alpha2-hncconfigurations
+      path: /validate-objects
   failurePolicy: Fail
-  name: hncconfigurations.hnc.x-k8s.io
+  name: objects.hnc.x-k8s.io
   rules:
   - apiGroups:
-    - hnc.x-k8s.io
+    - '*'
     apiVersions:
-    - v1alpha2
+    - '*'
     operations:
     - CREATE
     - UPDATE
     - DELETE
     resources:
-    - hncconfigurations
+    - '*'
   sideEffects: None
 - admissionReviewVersions:
   - v1
   clientConfig:
     service:
       name: webhook-service
       namespace: system
-      path: /validate-v1-namespace
+      path: /validate-hnc-x-k8s-io-v1alpha2-hncconfigurations
   failurePolicy: Fail
-  name: namespaces.hnc.x-k8s.io
+  name: hncconfigurations.hnc.x-k8s.io
   rules:
   - apiGroups:
-    - ""
+    - hnc.x-k8s.io
     apiVersions:
-    - v1
+    - v1alpha2
     operations:
-    - DELETE
     - CREATE
     - UPDATE
+    - DELETE
     resources:
-    - namespaces
+    - hncconfigurations
   sideEffects: None
 - admissionReviewVersions:
   - v1
   clientConfig:
     service:
       name: webhook-service
       namespace: system
-      path: /validate-objects
+      path: /validate-v1-namespace
   failurePolicy: Fail
-  name: objects.hnc.x-k8s.io
+  name: namespaces.hnc.x-k8s.io
   rules:
   - apiGroups:
-    - '*'
+    - ""
     apiVersions:
-    - '*'
+    - v1
     operations:
+    - DELETE
     - CREATE
     - UPDATE
-    - DELETE
     resources:
-    - '*'
+    - namespaces
   sideEffects: None

internal/reconcilers/anchor.go → internal/anchor/reconciler.go

@@ -13,7 +13,7 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-package reconcilers
+package anchor
 
 import (
 	"context"
@@ -33,17 +33,19 @@ import (
 
 	api "sigs.k8s.io/hierarchical-namespaces/api/v1alpha2"
 	"sigs.k8s.io/hierarchical-namespaces/internal/config"
+	"sigs.k8s.io/hierarchical-namespaces/internal/crd"
 	"sigs.k8s.io/hierarchical-namespaces/internal/forest"
+	"sigs.k8s.io/hierarchical-namespaces/internal/logutils"
 	"sigs.k8s.io/hierarchical-namespaces/internal/metadata"
 )
 
-// AnchorReconciler reconciles SubnamespaceAnchor CRs to make sure all the subnamespaces are
+// Reconciler reconciles SubnamespaceAnchor CRs to make sure all the subnamespaces are
 // properly maintained.
-type AnchorReconciler struct {
+type Reconciler struct {
 	client.Client
 	Log logr.Logger
 
-	forest *forest.Forest
+	Forest *forest.Forest
 
 	// Affected is a channel of event.GenericEvent (see "Watching Channels" in
 	// https://book-v1.book.kubebuilder.io/beyond_basics/controller_watches.html) that is used to
@@ -53,8 +55,8 @@ type AnchorReconciler struct {
 
 // Reconcile sets up some basic variables and then calls the business logic. It currently
 // only handles the creation of the namespaces but no deletion or state reporting yet.
-func (r *AnchorReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
-	log := loggerWithRID(r.Log).WithValues("trigger", req.NamespacedName)
+func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
+	log := logutils.WithRID(r.Log).WithValues("trigger", req.NamespacedName)
 	log.V(1).Info("Reconciling anchor")
 
 	// Get names of the hierarchical namespace and the current namespace.
@@ -134,7 +136,7 @@ func (r *AnchorReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 // There are several conditions where we skip step 1 - for example, if we're uninstalling HNC, or
 // if allowCascadingDeletion is disabled but the subnamespace has descendants (see
 // shouldDeleteSubns for details). In such cases, we move straight to step 2.
-func (r *AnchorReconciler) onDeleting(ctx context.Context, log logr.Logger, inst *api.SubnamespaceAnchor, snsInst *corev1.Namespace) (bool, error) {
+func (r *Reconciler) onDeleting(ctx context.Context, log logr.Logger, inst *api.SubnamespaceAnchor, snsInst *corev1.Namespace) (bool, error) {
 	// Early exit and continue reconciliation if the instance is not being deleted.
 	if inst.DeletionTimestamp.IsZero() {
 		return false, nil
@@ -143,7 +145,7 @@ func (r *AnchorReconciler) onDeleting(ctx context.Context, log logr.Logger, inst
 	// We handle deletions differently depending on whether _one_ anchor is being deleted (i.e., the
 	// user wants to delete the namespace) or whether the Anchor CRD is being deleted, which usually
 	// means HNC is being uninstalled and we shouldn't delete _any_ namespaces.
-	deletingCRD, err := isDeletingCRD(ctx, api.Anchors)
+	deletingCRD, err := crd.IsDeletingCRD(ctx, api.Anchors)
 	if err != nil {
 		log.Error(err, "Couldn't determine if CRD is being deleted")
 		return false, err
@@ -180,9 +182,9 @@ func (r *AnchorReconciler) onDeleting(ctx context.Context, log logr.Logger, inst
 //
 // This is the only part of the deletion process that needs to access the forest, in order to check
 // the recursive AllowCascadingDeletion setting.
-func (r *AnchorReconciler) shouldDeleteSubns(log logr.Logger, inst *api.SubnamespaceAnchor, nsInst *corev1.Namespace, deletingCRD bool) bool {
-	r.forest.Lock()
-	defer r.forest.Unlock()
+func (r *Reconciler) shouldDeleteSubns(log logr.Logger, inst *api.SubnamespaceAnchor, nsInst *corev1.Namespace, deletingCRD bool) bool {
+	r.Forest.Lock()
+	defer r.Forest.Unlock()
 
 	// If the CRD is being deleted, we want to leave the subnamespace alone.
 	if deletingCRD {
@@ -211,7 +213,7 @@ func (r *AnchorReconciler) shouldDeleteSubns(log logr.Logger, inst *api.Subnames
 		// if ACD=true on one of their ancestors; the webhooks *should* ensure that this is always true
 		// before we get here, but if something slips by, we'll just leave the old subnamespace alone
 		// with a missing-anchor condition.
-		cns := r.forest.Get(inst.Name)
+		cns := r.Forest.Get(inst.Name)
 		if cns.ChildNames() != nil && !cns.AllowsCascadingDeletion() {
 			log.Info("This subnamespace has descendants and allowCascadingDeletion is disabled; will not delete")
 			return false
@@ -244,7 +246,7 @@ func (r *AnchorReconciler) shouldDeleteSubns(log logr.Logger, inst *api.Subnames
 // shouldFinalizeAnchor determines whether the anchor is safe to delete. It should only be called once
 // we know that we don't need to delete the subnamespace itself (e.g. it's already gone, it can't be
 // deleted, it's in the process of being deleted, etc).
-func (r *AnchorReconciler) shouldFinalizeAnchor(log logr.Logger, inst *api.SubnamespaceAnchor, snsInst *corev1.Namespace) bool {
+func (r *Reconciler) shouldFinalizeAnchor(log logr.Logger, inst *api.SubnamespaceAnchor, snsInst *corev1.Namespace) bool {
 	// If the anchor is already finalized, there's no need to do it again.
 	if len(inst.ObjectMeta.Finalizers) == 0 {
 		return false
@@ -289,7 +291,7 @@ func (r *AnchorReconciler) shouldFinalizeAnchor(log logr.Logger, inst *api.Subna
 	}
 }
 
-func (r *AnchorReconciler) updateState(log logr.Logger, inst *api.SubnamespaceAnchor, snsInst *corev1.Namespace) {
+func (r *Reconciler) updateState(log logr.Logger, inst *api.SubnamespaceAnchor, snsInst *corev1.Namespace) {
 	pnm := inst.Namespace
 	sOf := snsInst.Annotations[api.SubnamespaceOf]
 	switch {
@@ -308,9 +310,9 @@ func (r *AnchorReconciler) updateState(log logr.Logger, inst *api.SubnamespaceAn
 	}
 }
 
-// It enqueues a subnamespace anchor for later reconciliation. This occurs in a goroutine
+// Enqueue enqueues a subnamespace anchor for later reconciliation. This occurs in a goroutine
 // so the caller doesn't block; since the reconciler is never garbage-collected, this is safe.
-func (r *AnchorReconciler) enqueue(log logr.Logger, nm, pnm, reason string) {
+func (r *Reconciler) Enqueue(log logr.Logger, nm, pnm, reason string) {
 	go func() {
 		// The watch handler doesn't care about anything except the metadata.
 		inst := &api.SubnamespaceAnchor{}
@@ -321,7 +323,7 @@ func (r *AnchorReconciler) enqueue(log logr.Logger, nm, pnm, reason string) {
 	}()
 }
 
-func (r *AnchorReconciler) getInstance(ctx context.Context, pnm, nm string) (*api.SubnamespaceAnchor, error) {
+func (r *Reconciler) getInstance(ctx context.Context, pnm, nm string) (*api.SubnamespaceAnchor, error) {
 	nsn := types.NamespacedName{Namespace: pnm, Name: nm}
 	inst := &api.SubnamespaceAnchor{}
 	if err := r.Get(ctx, nsn, inst); err != nil {
@@ -330,7 +332,7 @@ func (r *AnchorReconciler) getInstance(ctx context.Context, pnm, nm string) (*ap
 	return inst, nil
 }
 
-func (r *AnchorReconciler) writeInstance(ctx context.Context, log logr.Logger, inst *api.SubnamespaceAnchor) error {
+func (r *Reconciler) writeInstance(ctx context.Context, log logr.Logger, inst *api.SubnamespaceAnchor) error {
 	if inst.CreationTimestamp.IsZero() {
 		if err := r.Create(ctx, inst); err != nil {
 			log.Error(err, "while creating on apiserver")
@@ -347,7 +349,7 @@ func (r *AnchorReconciler) writeInstance(ctx context.Context, log logr.Logger, i
 
 // 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 {
+func (r *Reconciler) 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)
 	}
@@ -357,7 +359,7 @@ func (r *AnchorReconciler) deleteInstance(ctx context.Context, log logr.Logger,
 // 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.
-func (r *AnchorReconciler) getNamespace(ctx context.Context, nm string) (*corev1.Namespace, error) {
+func (r *Reconciler) getNamespace(ctx context.Context, nm string) (*corev1.Namespace, error) {
 	ns := &corev1.Namespace{}
 	nnm := types.NamespacedName{Name: nm}
 	if err := r.Get(ctx, nnm, ns); err != nil {
@@ -369,7 +371,7 @@ func (r *AnchorReconciler) getNamespace(ctx context.Context, nm string) (*corev1
 	return ns, nil
 }
 
-func (r *AnchorReconciler) writeNamespace(ctx context.Context, log logr.Logger, nm, pnm string) error {
+func (r *Reconciler) writeNamespace(ctx context.Context, log logr.Logger, nm, pnm string) error {
 	inst := &corev1.Namespace{}
 	inst.ObjectMeta.Name = nm
 	metadata.SetAnnotation(inst, api.SubnamespaceOf, pnm)
@@ -386,15 +388,15 @@ func (r *AnchorReconciler) writeNamespace(ctx context.Context, log logr.Logger,
 	return nil
 }
 
-func (r *AnchorReconciler) deleteNamespace(ctx context.Context, log logr.Logger, inst *corev1.Namespace) error {
+func (r *Reconciler) deleteNamespace(ctx context.Context, log logr.Logger, inst *corev1.Namespace) error {
 	if err := r.Delete(ctx, inst); err != nil {
 		log.Error(err, "While deleting subnamespace")
 		return err
 	}
 	return nil
 }
 
-func (r *AnchorReconciler) SetupWithManager(mgr ctrl.Manager) error {
+func (r *Reconciler) SetupWithManager(mgr ctrl.Manager) error {
 	// Maps an subnamespace to its anchor in the parent namespace.
 	nsMapFn := func(obj client.Object) []reconcile.Request {
 		if obj.GetAnnotations()[api.SubnamespaceOf] == "" {