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 == "" {