Merge pull request #424 from sophieliu15/dynamic_start

Dynamically create object reconcilers for new types.

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: Rename the method syncObjectReconcilers because we might need more than creating ObjectReconcilers in future.
+	// For example, we might need to delete an ObjectReconciler if its corresponding type is deleted in the HNCConfiguration.
+	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.
+	// TODO: Update HNCConfiguration.Status before writing the singleton 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,18 +11,25 @@ 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() {
 	ctx := context.Background()
 
+	// Setup HNCConfiguration.
+	hncConfig := newHNCConfig()
+
 	var (
 		fooName string
 		barName string
 		bazName string
 	)
 
 	BeforeEach(func() {
+		// Add secret to HNCConfiguration so that an ObjectReconciler will be created for secret.
+		addSecretToHNCConfig(ctx, hncConfig)
+
 		fooName = createNS(ctx, "foo")
 		barName = createNS(ctx, "bar")
 		bazName = createNS(ctx, "baz")
@@ -315,3 +322,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())

incubator/hnc/pkg/forest/forest.go

@@ -2,6 +2,7 @@
 package forest
 
 import (
+	"context"
 	"errors"
 	"fmt"
 	"sort"
@@ -21,6 +22,15 @@ var (
 	OutOfSync = errors.New("The forest is out of sync with itself")
 )
 
+// TypeSyncer syncs objects of a specific type. Reconcilers implement the interface so that they can be
+// called by the HierarchyReconciler if the hierarchy changes.
+type TypeSyncer interface {
+	// SyncNamespace syncs objects of a namespace for a specific type.
+	SyncNamespace(context.Context, logr.Logger, string) error
+	// Provides the GVK that is handled by the reconciler who implements the interface.
+	GetGVK() schema.GroupVersionKind
+}
+
 // Forest defines a forest of namespaces - that is, a set of trees. It includes methods to mutate
 // the forest legally (ie, prevent cycles).
 //
@@ -29,11 +39,23 @@ var (
 type Forest struct {
 	lock       sync.Mutex
 	namespaces namedNamespaces
+
+	// 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.
+	//
+	// We put the list in the forest because the access to the list is guarded by the forest lock.
+	// We can also move the lock out of the forest and pass it to all reconcilers that need the lock.
+	// In that way, we don't need to put the list in the forest.
+	types []TypeSyncer
 }
 
 func NewForest() *Forest {
 	return &Forest{
 		namespaces: namedNamespaces{},
+		types:      []TypeSyncer{},
 	}
 }
 
@@ -45,6 +67,30 @@ func (f *Forest) Unlock() {
 	f.lock.Unlock()
 }
 
+// AddTypeSyncer adds a reconciler to the types list.
+func (f *Forest) AddTypeSyncer(nss TypeSyncer) {
+	f.types = append(f.types, nss)
+}
+
+// HasTypeSyncer returns true if there is already a reconciler with the given GVK.
+func (f *Forest) HasTypeSyncer(gvk schema.GroupVersionKind) bool {
+	for _, t := range f.types {
+		if t.GetGVK() == gvk {
+			return true
+		}
+	}
+	return false
+}
+
+// GetTypeSyncers returns the types list.
+// Retuns a copy here so that the caller does not need to hold the mutex while accessing the returned value and can modify the
+// returned value without fear of corrupting the original types list.
+func (f *Forest) GetTypeSyncers() []TypeSyncer {
+	types := make([]TypeSyncer, len(f.types))
+	copy(types, f.types)
+	return types
+}
+
 // Get returns a `Namespace` object representing a namespace in K8s.
 func (f *Forest) Get(nm string) *Namespace {
 	if nm == "" {