From 123981b1f350894c2cc40e8f48f32515d95f39ca Mon Sep 17 00:00:00 2001 From: sophieliu15 Date: Tue, 18 Feb 2020 10:46:54 -0500 Subject: [PATCH] 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. --- .../hnc/config/samples/hnc_v1_config.yaml | 3 + .../hnc/pkg/controllers/config_controller.go | 90 ++++++++++++++++--- .../pkg/controllers/hierarchy_controller.go | 21 ++--- .../hnc/pkg/controllers/object_controller.go | 5 ++ .../pkg/controllers/object_controller_test.go | 28 ++++++ incubator/hnc/pkg/controllers/setup.go | 29 ++---- incubator/hnc/pkg/forest/forest.go | 46 ++++++++++ 7 files changed, 173 insertions(+), 49 deletions(-) diff --git a/incubator/hnc/config/samples/hnc_v1_config.yaml b/incubator/hnc/config/samples/hnc_v1_config.yaml index 935609887..ff2212a5f 100644 --- a/incubator/hnc/config/samples/hnc_v1_config.yaml +++ b/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 diff --git a/incubator/hnc/pkg/controllers/config_controller.go b/incubator/hnc/pkg/controllers/config_controller.go index 820f018ca..c0486ea3e 100644 --- a/incubator/hnc/pkg/controllers/config_controller.go +++ b/incubator/hnc/pkg/controllers/config_controller.go @@ -2,15 +2,20 @@ 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, @@ -18,7 +23,16 @@ import ( // 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. diff --git a/incubator/hnc/pkg/controllers/hierarchy_controller.go b/incubator/hnc/pkg/controllers/hierarchy_controller.go index 0ae9b0cf5..08b186144 100644 --- a/incubator/hnc/pkg/controllers/hierarchy_controller.go +++ b/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 } diff --git a/incubator/hnc/pkg/controllers/object_controller.go b/incubator/hnc/pkg/controllers/object_controller.go index 11f5610ff..12640fe2e 100644 --- a/incubator/hnc/pkg/controllers/object_controller.go +++ b/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 diff --git a/incubator/hnc/pkg/controllers/object_controller_test.go b/incubator/hnc/pkg/controllers/object_controller_test.go index e2005b02f..8dd4939ca 100644 --- a/incubator/hnc/pkg/controllers/object_controller_test.go +++ b/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() { ctx := context.Background() + // Setup HNCConfiguration. + hncConfig := newHNCConfig() + var ( fooName string barName string @@ -23,6 +27,9 @@ var _ = Describe("Secret", func() { ) 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) +} diff --git a/incubator/hnc/pkg/controllers/setup.go b/incubator/hnc/pkg/controllers/setup.go index 082bf287a..598a97772 100644 --- a/incubator/hnc/pkg/controllers/setup.go +++ b/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()) diff --git a/incubator/hnc/pkg/forest/forest.go b/incubator/hnc/pkg/forest/forest.go index bb1941d5e..53cd64ce5 100644 --- a/incubator/hnc/pkg/forest/forest.go +++ b/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 == "" {