Dynamically create object reconcilers for new types.

This PR creates object reconciler(s) when new type(s) are added to the
HNCConfiguration and informs the hierarchy reconciler about the new
object reconciler(s).

Specifically, it does the following tasks:
  - Dynamically creates an object reconciler when a new type is added
    in HNCConfiguration.
  - Informs the hierarchy reconciler about the newly added object
    reconciler so the corresponding objects can be propagated correctly
    when the hierarchy structure changes.

Test: Verified on a GKE cluster that
   - When adding a new type in HNCConfiguration, corresponding object
     reconciler is created successfully.
   - When adding a new object of the same type to a node in the namespace
     tree, the object is propagated correctly to the descendants of the
     node.
   - When the tree structure changes (e.g., add another descendant to the
     node above), the new object of the same type is propagated
     correctly.

incubator/hnc/config/samples/hnc_v1_config.yaml

@@ -10,3 +10,6 @@ spec:
     - apiVersion: rbac.authorization.k8s.io/v1
       kind: Role
       mode: propagate
+    - apiVersion: rbac.authorization.k8s.io/v1
+      kind: RoleBinding
+      mode: propagate

incubator/hnc/pkg/controllers/config_controller.go

@@ -2,23 +2,37 @@ package controllers
 
 import (
 	"context"
+	"fmt"
+
+	"k8s.io/apimachinery/pkg/runtime/schema"
 
 	"github.com/go-logr/logr"
 	"k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/types"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
+	"sigs.k8s.io/controller-runtime/pkg/event"
 
 	api "github.com/kubernetes-sigs/multi-tenancy/incubator/hnc/api/v1alpha1"
 	"github.com/kubernetes-sigs/multi-tenancy/incubator/hnc/pkg/config"
+	"github.com/kubernetes-sigs/multi-tenancy/incubator/hnc/pkg/forest"
 )
 
 // ConfigReconciler is responsible for determining the HNC configuration from the HNCConfiguration CR,
 // as well as ensuring all objects are propagated correctly when the HNC configuration changes.
 // It can also set the status of the HNCConfiguration CR.
 type ConfigReconciler struct {
 	client.Client
-	Log logr.Logger
+	Log     logr.Logger
+	Manager ctrl.Manager
+
+	// Forest is the in-memory data structure that is shared with all other reconcilers.
+	Forest *forest.Forest
+
+	// HierarchyConfigUpdates is a channel of events used to update hierarchy configuration changes performed by
+	// ObjectReconcilers. It is passed on to ObjectReconcilers for the updates. The ConfigReconciler itself does
+	// not use it.
+	HierarchyConfigUpdates chan event.GenericEvent
 }
 
 // Reconcile sets up some basic variable and logs the Spec.
@@ -42,13 +56,23 @@ func (r *ConfigReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 	// achieve the reconciliation.
 	r.Log.Info("Reconciling cluster-wide HNC configuration.")
 
+	// Create cooresponding ObjectReconcilers for newly added types, if needed.
+	// TODO: Returning an error here will make controller-runtime to requeue the object to be reconciled again.
+	// A better way to handle this is to display the error as part of the HNCConfig.status field to suggest the error is a
+	// permanent problem and to stop controller-runtime from retrying.
+	// TODO: We might need more than createObjectReconcilers in future. For example, we might need to delete
+	// an ObjectReconciler if its corresponding type is deleted in the HNCConfiguration. In that case, we will
+	// rename the method syncObjectReconcilers.
+	if err := r.createObjectReconcilers(inst); err != nil {
+		r.Log.Error(err, "Couldn't create the new object reconciler.")
+		return ctrl.Result{}, nil
+	}
+
 	// Write back to the apiserver.
 	if err := r.writeSingleton(ctx, inst); err != nil {
 		r.Log.Error(err, "Couldn't write singleton.")
 		return ctrl.Result{}, err
 	}
-
-	r.logSpec(inst)
 	return ctrl.Result{}, nil
 }
 
@@ -91,16 +115,60 @@ func (r *ConfigReconciler) writeSingleton(ctx context.Context, inst *api.HNCConf
 	return nil
 }
 
-// logSpec logs current Spec of the CRD.
-// TODO: This method is mainly for debuging and testing in the early development stage. Remove
-// this method when the implementation is compeleted.
-func (r *ConfigReconciler) logSpec(inst *api.HNCConfiguration) {
-	r.Log.Info("Record length of Types", "length", len(inst.Spec.Types))
+// createObjectReconcilers creates corresponding ObjectReconcilers for the newly added types in the
+// HNC configuration, if there is any. It also informs the in-memory forest about the newly created
+// ObjectReconcilers.
+func (r *ConfigReconciler) createObjectReconcilers(inst *api.HNCConfiguration) error {
+	// This method is guarded by the forest mutex. The mutex is guarding two actions: creating ObjectReconcilers for
+	// the newly added types and adding the created ObjectReconcilers to the types list in the Forest (r.Forest.types).
+	//
+	// We use mutex to guard write access to the types list because the list is a shared resource between various
+	// reconcilers. It is sufficient because both write and read access to the list are guarded by the same mutex.
+	//
+	// We use the forest mutex to guard ObjectReconciler creation together with types list modification so that the
+	// forest cannot be changed by the HierarchyReconciler until both actions are finished. If we do not use the
+	// forest mutex to guard both actions, HierarchyReconciler might change the forest structure and read the types list
+	// from the forest for the object reconciliation, after we create ObjectReconcilers but before we write the
+	// ObjectReconcilers to the types list. As a result, objects of the newly added types might not be propagated correctly
+	// using the latest forest structure.
+	r.Forest.Lock()
+	defer r.Forest.Unlock()
 	for _, t := range inst.Spec.Types {
-		r.Log.Info("spec:", "apiVersion: ", t.APIVersion)
-		r.Log.Info("spec:", "kind: ", t.Kind)
-		r.Log.Info("spec:", "mode: ", t.Mode)
+		gvk := schema.FromAPIVersionAndKind(t.APIVersion, t.Kind)
+		if r.Forest.HasTypeSyncer(gvk) {
+			continue
+		}
+		if err := r.createObjectReconciler(gvk); err != nil {
+			return fmt.Errorf("Error while trying to create ObjectReconciler: %s", err)
+		}
 	}
+
+	return nil
+}
+
+// createObjectReconciler creates an ObjectReconciler for the given GVK and informs forest about the reconciler.
+// TODO: May need to pass in spec instead to provide information about Mode to the ObjectReconciler in future.
+func (r *ConfigReconciler) createObjectReconciler(gvk schema.GroupVersionKind) error {
+	r.Log.Info("Creating an object reconciler.", "GVK", gvk)
+
+	or := &ObjectReconciler{
+		Client:            r.Client,
+		Log:               ctrl.Log.WithName("controllers").WithName(gvk.Kind),
+		Forest:            r.Forest,
+		GVK:               gvk,
+		Affected:          make(chan event.GenericEvent),
+		AffectedNamespace: r.HierarchyConfigUpdates,
+	}
+
+	// TODO: figure out MaxConcurrentReconciles option - https://github.com/kubernetes-sigs/multi-tenancy/issues/291
+	if err := or.SetupWithManager(r.Manager, 10); err != nil {
+		return fmt.Errorf("Cannot create %v reconciler: %s", gvk, err.Error())
+	}
+
+	// Informs the in-memory forest about the new reconciler by adding it to the types list.
+	r.Forest.AddTypeSyncer(or)
+
+	return nil
 }
 
 // SetupWithManager builds a controller with the reconciler.

incubator/hnc/pkg/controllers/hierarchy_controller.go

@@ -54,13 +54,6 @@ type HierarchyReconciler struct {
 	// use it to determine how to propagate objects.
 	Forest *forest.Forest
 
-	// Types is a list of other reconcillers that HierarchyReconciler can call if the hierarchy
-	// changes. This will force all objects to be re-propagated.
-	//
-	// This is probably wildly inefficient, and we can probably make better use of things like
-	// owner references to make this better. But for a PoC, it works just fine.
-	Types []NamespaceSyncer
-
 	// 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
 	// enqueue additional namespaces that need updating.
@@ -72,13 +65,6 @@ type HierarchyReconciler struct {
 	reconcileID int32
 }
 
-// NamespaceSyncer syncs various aspects of a namespace. The HierarchyReconciler both implements
-// it (so it can be called by NamespaceSyncer) and uses it (to sync the objects in the
-// namespace).
-type NamespaceSyncer interface {
-	SyncNamespace(context.Context, logr.Logger, string) error
-}
-
 // +kubebuilder:rbac:groups=hnc.x-k8s.io,resources=hierarchies,verbs=get;list;watch;create;update;patch;delete
 // +kubebuilder:rbac:groups=hnc.x-k8s.io,resources=hierarchies/status,verbs=get;update;patch
 // +kubebuilder:rbac:groups=core,resources=namespaces,verbs=get;list;watch;update;patch
@@ -522,7 +508,12 @@ func (r *HierarchyReconciler) writeNamespace(ctx context.Context, log logr.Logge
 
 // updateObjects calls all type reconcillers in this namespace.
 func (r *HierarchyReconciler) updateObjects(ctx context.Context, log logr.Logger, ns string) error {
-	for _, tr := range r.Types {
+	// Use mutex to guard the read from the types list of the forest to prevent the ConfigReconciler
+	// from modifying the list at the same time.
+	r.Forest.Lock()
+	trs := r.Forest.GetTypeSyncers()
+	r.Forest.Unlock()
+	for _, tr := range trs {
 		if err := tr.SyncNamespace(ctx, log, ns); err != nil {
 			return err
 		}

incubator/hnc/pkg/controllers/object_controller.go

@@ -91,6 +91,11 @@ func (r *ObjectReconciler) SyncNamespace(ctx context.Context, log logr.Logger, n
 	return nil
 }
 
+// GetGVK provides GVK that is handled by this reconciler.
+func (r *ObjectReconciler) GetGVK() schema.GroupVersionKind {
+	return r.GVK
+}
+
 func (r *ObjectReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
 	if ex[req.Namespace] {
 		return ctrl.Result{}, nil

incubator/hnc/pkg/controllers/object_controller_test.go

@@ -11,11 +11,15 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 
 	api "github.com/kubernetes-sigs/multi-tenancy/incubator/hnc/api/v1alpha1"
+	"github.com/kubernetes-sigs/multi-tenancy/incubator/hnc/pkg/config"
 )
 
-var _ = Describe("Secret", func() {
+var _ = Describe("Role", func() {
 	ctx := context.Background()
 
+	// Setup HNCConfiguration.
+	hncConfig := newHNCConfig()
+
 	var (
 		fooName string
 		barName string
@@ -34,6 +38,9 @@ var _ = Describe("Secret", func() {
 	})
 
 	It("should be copied to descendents", func() {
+		// Add secret to HNCConfiguration so that an ObjectReconciler will be created for secret.
+		addSecretToHNCConfig(ctx, hncConfig)
+
 		setParent(ctx, barName, fooName)
 		setParent(ctx, bazName, barName)
 
@@ -48,6 +55,8 @@ var _ = Describe("Secret", func() {
 	})
 
 	It("should be removed if the hierarchy changes", func() {
+		// Add secret to HNCConfiguration so that an ObjectReconciler will be created for secret.
+		addSecretToHNCConfig(ctx, hncConfig)
 		setParent(ctx, barName, fooName)
 		setParent(ctx, bazName, barName)
 		Eventually(hasSecret(ctx, bazName, "foo-sec")).Should(BeTrue())
@@ -61,6 +70,9 @@ var _ = Describe("Secret", func() {
 	})
 
 	It("should not be propagated if modified", func() {
+		// Add secret to HNCConfiguration so that an ObjectReconciler will be created for secret.
+		addSecretToHNCConfig(ctx, hncConfig)
+
 		// Set tree as bar -> foo and make sure the first-time propagation of foo-sec
 		// is finished before modifying the foo-sec in bar namespace
 		setParent(ctx, barName, fooName)
@@ -83,6 +95,9 @@ var _ = Describe("Secret", func() {
 	})
 
 	It("should be removed if the source no longer exists", func() {
+		// Add secret to HNCConfiguration so that an ObjectReconciler will be created for secret.
+		addSecretToHNCConfig(ctx, hncConfig)
+
 		setParent(ctx, barName, fooName)
 		setParent(ctx, bazName, barName)
 		Eventually(hasSecret(ctx, barName, "foo-sec")).Should(BeTrue())
@@ -97,6 +112,9 @@ var _ = Describe("Secret", func() {
 	})
 
 	It("should overwrite the propagated ones if the source is updated", func() {
+		// Add secret to HNCConfiguration so that an ObjectReconciler will be created for secret.
+		addSecretToHNCConfig(ctx, hncConfig)
+
 		setParent(ctx, barName, fooName)
 		setParent(ctx, bazName, barName)
 		Eventually(isModified(ctx, fooName, "foo-sec")).Should(BeFalse())
@@ -114,6 +132,9 @@ var _ = Describe("Secret", func() {
 	})
 
 	It("shouldn't propagate/delete if the namespace has Crit condition", func() {
+		// Add secret to HNCConfiguration so that an ObjectReconciler will be created for secret.
+		addSecretToHNCConfig(ctx, hncConfig)
+
 		// Set tree as baz -> bar -> foo(root).
 		setParent(ctx, barName, fooName)
 		setParent(ctx, bazName, barName)
@@ -182,6 +203,9 @@ var _ = Describe("Secret", func() {
 	})
 
 	It("should set conditions if it's excluded from being propagated, and clear them if it's fixed", func() {
+		// Add secret to HNCConfiguration so that an ObjectReconciler will be created for secret.
+		addSecretToHNCConfig(ctx, hncConfig)
+
 		// Set tree as baz -> bar -> foo(root) and make sure the secret gets propagated.
 		setParent(ctx, barName, fooName)
 		setParent(ctx, bazName, barName)
@@ -315,3 +339,24 @@ func removeSecret(ctx context.Context, nsName, secretName string) {
 	sec.Namespace = nsName
 	ExpectWithOffset(1, k8sClient.Delete(ctx, sec)).Should(Succeed())
 }
+
+func newHNCConfig() *api.HNCConfiguration {
+	hncConfig := &api.HNCConfiguration{}
+	hncConfig.ObjectMeta.Name = api.HNCConfigSingleton
+	hncConfig.Spec = config.GetDefaultConfigSpec()
+	return hncConfig
+}
+
+func updateHNCConfig(ctx context.Context, c *api.HNCConfiguration) {
+	if c.CreationTimestamp.IsZero() {
+		ExpectWithOffset(1, k8sClient.Create(ctx, c)).Should(Succeed())
+	} else {
+		ExpectWithOffset(1, k8sClient.Update(ctx, c)).Should(Succeed())
+	}
+}
+
+func addSecretToHNCConfig(ctx context.Context, c *api.HNCConfiguration) {
+	secSpec := api.TypeSynchronizationSpec{APIVersion: "v1", Kind: "Secret", Mode: api.Propagate}
+	c.Spec.Types = append(c.Spec.Types, secSpec)
+	updateHNCConfig(ctx, c)
+}

incubator/hnc/pkg/controllers/setup.go

@@ -6,7 +6,6 @@ import (
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/event"
 
-	"github.com/kubernetes-sigs/multi-tenancy/incubator/hnc/pkg/config"
 	"github.com/kubernetes-sigs/multi-tenancy/incubator/hnc/pkg/forest"
 )
 
@@ -25,30 +24,11 @@ var ex = map[string]bool{
 func Create(mgr ctrl.Manager, f *forest.Forest, maxReconciles int) error {
 	hcChan := make(chan event.GenericEvent)
 
-	// Create all object reconcillers
-	objReconcilers := []NamespaceSyncer{}
-	for _, gvk := range config.GVKs {
-		or := &ObjectReconciler{
-			Client:            mgr.GetClient(),
-			Log:               ctrl.Log.WithName("controllers").WithName(gvk.Kind),
-			Forest:            f,
-			GVK:               gvk,
-			Affected:          make(chan event.GenericEvent),
-			AffectedNamespace: hcChan,
-		}
-		// TODO figure out MaxConcurrentReconciles option - https://github.com/kubernetes-sigs/multi-tenancy/issues/291
-		if err := or.SetupWithManager(mgr, 10); err != nil {
-			return fmt.Errorf("cannot create %v controller: %s", gvk, err.Error())
-		}
-		objReconcilers = append(objReconcilers, or)
-	}
-
-	// Create the HierarchyReconciler, passing it the object reconcillers so it can call them.
+	// Create the HierarchyReconciler.
 	hr := &HierarchyReconciler{
 		Client:   mgr.GetClient(),
 		Log:      ctrl.Log.WithName("controllers").WithName("Hierarchy"),
 		Forest:   f,
-		Types:    objReconcilers,
 		Affected: hcChan,
 	}
 	if err := hr.SetupWithManager(mgr, maxReconciles); err != nil {
@@ -57,8 +37,11 @@ func Create(mgr ctrl.Manager, f *forest.Forest, maxReconciles int) error {
 
 	// Create the ConfigReconciler.
 	cr := &ConfigReconciler{
-		Client: mgr.GetClient(),
-		Log:    ctrl.Log.WithName("controllers").WithName("HNCConfiguration"),
+		Client:                 mgr.GetClient(),
+		Log:                    ctrl.Log.WithName("controllers").WithName("HNCConfiguration"),
+		Manager:                mgr,
+		Forest:                 f,
+		HierarchyConfigUpdates: hcChan,
 	}
 	if err := cr.SetupWithManager(mgr); err != nil {
 		return fmt.Errorf("cannot create Config controller: %s", err.Error())