From bc9d98991a1a42c34912487c29a962d7a3f81da0 Mon Sep 17 00:00:00 2001 From: sophieliu15 Date: Thu, 27 Feb 2020 13:50:34 -0500 Subject: [PATCH] Add Conditions in HNCConfiguration.Status to indicate error conditions. This PR adds Conditions in HNCConfiguration.Status to indicate error scenarios. Condition specifies error code and message for a specific error. Currently we support following error code: - critSingletonNameInvalid: the specified singleton name is invalid. - objectReconcilerCreationFailed: an error exists when creating the object reconciler for a specific type. This PR includes changes introduced in #453 Tested: GKE cluster; unit tests. Design doc: http://bit.ly/hnc-type-configuration Issue: #411 --- incubator/hnc/api/v1alpha1/hnc_config.go | 39 ++++ .../hnc/api/v1alpha1/zz_generated.deepcopy.go | 20 ++ .../bases/hnc.x-k8s.io_hncconfigurations.yaml | 29 +++ incubator/hnc/pkg/reconcilers/hnc_config.go | 96 ++++++--- .../hnc/pkg/reconcilers/hnc_config_test.go | 192 ++++++++++++++++-- incubator/hnc/pkg/reconcilers/object_test.go | 60 +----- incubator/hnc/pkg/reconcilers/setup.go | 11 + incubator/hnc/pkg/reconcilers/suite_test.go | 17 +- .../hnc/pkg/reconcilers/test_helpers_test.go | 65 +++++- 9 files changed, 414 insertions(+), 115 deletions(-) diff --git a/incubator/hnc/api/v1alpha1/hnc_config.go b/incubator/hnc/api/v1alpha1/hnc_config.go index be488ff4d..7a8551f19 100644 --- a/incubator/hnc/api/v1alpha1/hnc_config.go +++ b/incubator/hnc/api/v1alpha1/hnc_config.go @@ -42,6 +42,13 @@ const ( Remove SynchronizationMode = "remove" ) +// HNCConfigurationCondition codes. *All* codes must also be documented in the +// comment to HNCConfigurationCondition.Code. +const ( + CritSingletonNameInvalid HNCConfigurationCode = "critSingletonNameInvalid" + ObjectReconcilerCreationFailed HNCConfigurationCode = "objectReconcilerCreationFailed" +) + // TypeSynchronizationSpec defines the desired synchronization state of a specific kind. type TypeSynchronizationSpec struct { // API version of the kind defined below. This is used to unambiguously identifies the kind. @@ -88,6 +95,9 @@ type HNCConfigurationSpec struct { type HNCConfigurationStatus struct { // Types indicates the observed synchronization states of kinds, if any. Types []TypeSynchronizationStatus `json:"types,omitempty"` + + // Conditions describes the errors, if any. + Conditions []HNCConfigurationCondition `json:"conditions,omitempty"` } // +kubebuilder:object:root=true @@ -99,6 +109,35 @@ type HNCConfigurationList struct { Items []HNCConfiguration `json:"items"` } +// HNCConfigurationCode is the machine-readable, enum-like type of `HNCConfigurationCondition.Code`. +// See that field for more information. +type HNCConfigurationCode string + +// HNCConfigurationCondition specifies the code and the description of an error condition. +type HNCConfigurationCondition struct { + // Describes the condition in a machine-readable string value. The currently valid values are + // shown below, but new values may be added over time. This field is always present in a + // condition. + // + // All codes that begin with the prefix `crit` indicate that reconciliation has + // been paused for this configuration. Future changes of the configuration will be + // ignored by HNC until the condition has been resolved. Non-critical conditions + // typically indicate some kinds of error that HNC itself can ignore. However, + // the behaviors of some types might be out-of-sync with the users' expectations. + // + // Currently, the supported values are: + // + // - "critSingletonNameInvalid": the specified singleton name is invalid. + // + // - "objectReconcilerCreationFailed": an error exists when creating the object + // reconciler for a specific type. + Code HNCConfigurationCode `json:"code"` + + // A human-readable description of the condition, if the `code` field is not + // sufficiently clear on their own. + Msg string `json:"msg,omitempty"` +} + func init() { SchemeBuilder.Register(&HNCConfiguration{}, &HNCConfigurationList{}) } diff --git a/incubator/hnc/api/v1alpha1/zz_generated.deepcopy.go b/incubator/hnc/api/v1alpha1/zz_generated.deepcopy.go index 80273d8af..d4265431a 100644 --- a/incubator/hnc/api/v1alpha1/zz_generated.deepcopy.go +++ b/incubator/hnc/api/v1alpha1/zz_generated.deepcopy.go @@ -85,6 +85,21 @@ func (in *HNCConfiguration) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *HNCConfigurationCondition) DeepCopyInto(out *HNCConfigurationCondition) { + *out = *in +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new HNCConfigurationCondition. +func (in *HNCConfigurationCondition) DeepCopy() *HNCConfigurationCondition { + if in == nil { + return nil + } + out := new(HNCConfigurationCondition) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *HNCConfigurationList) DeepCopyInto(out *HNCConfigurationList) { *out = *in @@ -147,6 +162,11 @@ func (in *HNCConfigurationStatus) DeepCopyInto(out *HNCConfigurationStatus) { (*in)[i].DeepCopyInto(&(*out)[i]) } } + if in.Conditions != nil { + in, out := &in.Conditions, &out.Conditions + *out = make([]HNCConfigurationCondition, len(*in)) + copy(*out, *in) + } } // DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new HNCConfigurationStatus. diff --git a/incubator/hnc/config/crd/bases/hnc.x-k8s.io_hncconfigurations.yaml b/incubator/hnc/config/crd/bases/hnc.x-k8s.io_hncconfigurations.yaml index 1d404f74c..c8e63039c 100644 --- a/incubator/hnc/config/crd/bases/hnc.x-k8s.io_hncconfigurations.yaml +++ b/incubator/hnc/config/crd/bases/hnc.x-k8s.io_hncconfigurations.yaml @@ -58,6 +58,35 @@ spec: status: description: HNCConfigurationStatus defines the observed state of HNC configuration. properties: + conditions: + description: Conditions describes the errors, if any. + items: + description: HNCConfigurationCondition specifies the code and the + description of an error condition. + properties: + code: + description: "Describes the condition in a machine-readable string + value. The currently valid values are shown below, but new values + may be added over time. This field is always present in a condition. + \n All codes that begin with the prefix `crit` indicate that + reconciliation has been paused for this configuration. Future + changes of the configuration will be ignored by HNC until the + condition has been resolved. Non-critical conditions typically + indicate some kinds of error that HNC itself can ignore. However, + the behaviors of some types might be out-of-sync with the users' + expectations. \n Currently, the supported values are: \n - \"critSingletonNameInvalid\": + the specified singleton name is invalid. \n - \"objectReconcilerCreationFailed\": + an error exists when creating the object reconciler for a specific + type." + type: string + msg: + description: A human-readable description of the condition, if + the `code` field is not sufficiently clear on their own. + type: string + required: + - code + type: object + type: array types: description: Types indicates the observed synchronization states of kinds, if any. diff --git a/incubator/hnc/pkg/reconcilers/hnc_config.go b/incubator/hnc/pkg/reconcilers/hnc_config.go index 157320ef2..60ef4f447 100644 --- a/incubator/hnc/pkg/reconcilers/hnc_config.go +++ b/incubator/hnc/pkg/reconcilers/hnc_config.go @@ -5,6 +5,8 @@ import ( "fmt" "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/source" "github.com/go-logr/logr" "k8s.io/apimachinery/pkg/api/errors" @@ -29,6 +31,11 @@ type ConfigReconciler struct { // Forest is the in-memory data structure that is shared with all other reconcilers. Forest *forest.Forest + // Igniter 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 the singleton for initial reconciliation. + Igniter chan event.GenericEvent + // 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. @@ -38,34 +45,32 @@ type ConfigReconciler struct { // Reconcile sets up some basic variable and logs the Spec. // TODO: Updates the comment above when adding more logic to the Reconcile method. func (r *ConfigReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { - // TODO: Surface the error more prominently and add a validating admission controller to prevent - // the problem in the first place. - if req.NamespacedName.Name != api.HNCConfigSingleton { - r.Log.Error(nil, "Singleton name is wrong. It should be 'config'.") + ctx := context.Background() + + // Validate the singleton name. + // TODO: Add a validating admission controller to prevent the problem in the first place. + if err := r.validateSingletonName(ctx, req.NamespacedName.Name); err != nil { + r.Log.Error(err, "Singleton name validation failed.") return ctrl.Result{}, nil } - ctx := context.Background() inst, err := r.getSingleton(ctx) if err != nil { r.Log.Error(err, "Couldn't read singleton.") - return ctrl.Result{}, nil + return ctrl.Result{}, err } // TODO: Modify this and other reconcilers (e.g., hierarchy and object reconcilers) to // 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. + // Clear the existing conditions because we will reconstruct the latest conditions. + inst.Status.Conditions = nil + + // Create corresponding ObjectReconcilers for newly added types, if needed. // 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 - } + r.createObjectReconcilers(inst) // Write back to the apiserver. // TODO: Update HNCConfiguration.Status before writing the singleton back to the apiserver. @@ -117,8 +122,8 @@ func (r *ConfigReconciler) writeSingleton(ctx context.Context, inst *api.HNCConf // 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 { +// ObjectReconcilers. If a type is misconfigured, the corresponding object reconciler will not be created. +func (r *ConfigReconciler) createObjectReconcilers(inst *api.HNCConfiguration) { // 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). // @@ -133,22 +138,19 @@ func (r *ConfigReconciler) createObjectReconcilers(inst *api.HNCConfiguration) e // using the latest forest structure. r.Forest.Lock() defer r.Forest.Unlock() + for _, t := range inst.Spec.Types { 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) - } + r.createObjectReconciler(gvk, inst) } - - 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 { +func (r *ConfigReconciler) createObjectReconciler(gvk schema.GroupVersionKind, inst *api.HNCConfiguration) { r.Log.Info("Creating an object reconciler.", "GVK", gvk) or := &ObjectReconciler{ @@ -162,18 +164,64 @@ func (r *ConfigReconciler) createObjectReconciler(gvk schema.GroupVersionKind) e // 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()) + r.Log.Error(err, "Error while trying to create ObjectReconciler", "gvk", gvk) + condition := api.HNCConfigurationCondition{ + Code: api.ObjectReconcilerCreationFailed, + Msg: fmt.Sprintf("Couldn't create ObjectReconciler for type %s: %s", gvk, err), + } + inst.Status.Conditions = append(inst.Status.Conditions, condition) + return } // Informs the in-memory forest about the new reconciler by adding it to the types list. r.Forest.AddTypeSyncer(or) +} - return nil +func (r *ConfigReconciler) validateSingletonName(ctx context.Context, nm string) error { + if nm == api.HNCConfigSingleton { + return nil + } + + nnm := types.NamespacedName{Name: nm} + inst := &api.HNCConfiguration{} + if err := r.Get(ctx, nnm, inst); err != nil { + return err + } + + msg := "Singleton name is wrong. It should be 'config'." + condition := api.HNCConfigurationCondition{ + Code: api.CritSingletonNameInvalid, + Msg: msg, + } + inst.Status.Conditions = nil + inst.Status.Conditions = append(inst.Status.Conditions, condition) + + if err := r.writeSingleton(ctx, inst); err != nil { + return err + } + + return fmt.Errorf("Error while validating singleton name: %s", msg) } // SetupWithManager builds a controller with the reconciler. func (r *ConfigReconciler) SetupWithManager(mgr ctrl.Manager) error { return ctrl.NewControllerManagedBy(mgr). For(&api.HNCConfiguration{}). + Watches(&source.Channel{Source: r.Igniter}, &handler.EnqueueRequestForObject{}). Complete(r) } + +// forceInitialReconcile forces reconciliation to start after setting up the +// controller with the manager. This is used to create a default singleton if +// there is no singleton in the cluster. This occurs in a goroutine so the +// caller doesn't block; since the reconciler is never garbage-collected, +// this is safe. +func (r *ConfigReconciler) forceInitialReconcile(log logr.Logger, reason string) { + go func() { + log.Info("Enqueuing for reconciliation", "reason", reason) + // The watch handler doesn't care about anything except the metadata. + inst := &api.HNCConfiguration{} + inst.ObjectMeta.Name = api.HNCConfigSingleton + r.Igniter <- event.GenericEvent{Meta: inst} + }() +} diff --git a/incubator/hnc/pkg/reconcilers/hnc_config_test.go b/incubator/hnc/pkg/reconcilers/hnc_config_test.go index d0978bc5e..608539988 100644 --- a/incubator/hnc/pkg/reconcilers/hnc_config_test.go +++ b/incubator/hnc/pkg/reconcilers/hnc_config_test.go @@ -2,12 +2,13 @@ package reconcilers_test import ( "context" - "time" api "github.com/kubernetes-sigs/multi-tenancy/incubator/hnc/api/v1alpha1" . "github.com/onsi/ginkgo" . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + v1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ) @@ -22,6 +23,11 @@ var _ = Describe("HNCConfiguration", func() { BeforeEach(func() { fooName = createNS(ctx, "foo") barName = createNS(ctx, "bar") + + // Give foo a role. + makeRole(ctx, fooName, "foo-role") + // Give foo a role binding. + makeRoleBinding(ctx, fooName, "foo-role", "foo-admin", "foo-role-binding") }) AfterEach(func() { @@ -31,32 +37,85 @@ var _ = Describe("HNCConfiguration", func() { }).Should(Succeed()) }) - It("should propagate objects whose types have been added to HNCConfiguration", func() { + It("should set mode of Roles and RoleBindings as propagate by default", func() { + config := getHNCConfig(ctx) + + Eventually(hasTypeWithMode("rbac.authorization.k8s.io/v1", "Role", api.Propagate, config)).Should(BeTrue()) + Eventually(hasTypeWithMode("rbac.authorization.k8s.io/v1", "RoleBinding", api.Propagate, config)).Should(BeTrue()) + }) + + It("should propagate Roles by default", func() { setParent(ctx, barName, fooName) - makeSecret(ctx, fooName, "foo-sec") - // Wait 1 second to give "foo-sec" a chance to be propagated to bar, if it can be propagated. - time.Sleep(1 * time.Second) - // Foo should have "foo-sec" since we created it there. - Eventually(hasSecret(ctx, fooName, "foo-sec")).Should(BeTrue()) - // "foo-sec" is not propagated to bar because Secret hasn't been configured in HNCConfiguration. - Eventually(hasSecret(ctx, barName, "foo-sec")).Should(BeFalse()) + Eventually(hasRole(ctx, barName, "foo-role")).Should(BeTrue()) + Expect(roleInheritedFrom(ctx, barName, "foo-role")).Should(Equal(fooName)) + }) + + It("should propagate RoleBindings by default", func() { + setParent(ctx, barName, fooName) + Eventually(hasRoleBinding(ctx, barName, "foo-role-binding")).Should(BeTrue()) + Expect(roleBindingInheritedFrom(ctx, barName, "foo-role-binding")).Should(Equal(fooName)) + }) + + It("should set CritSingletonNameInvalid condition if singleton name is wrong", func() { + nm := "wrong-config-1" Eventually(func() error { - c := getHNCConfig(ctx) - return addSecretToHNCConfig(ctx, c) + config := &api.HNCConfiguration{} + config.ObjectMeta.Name = nm + return updateHNCConfig(ctx, config) }).Should(Succeed()) + Eventually(hasHNCConfigurationConditionWithName(ctx, api.CritSingletonNameInvalid, nm)).Should(BeTrue()) + }) + + It("should set ObjectReconcilerCreationFailed condition if an object reconciler creation fails", func() { + // API version of Secret should be "v1" + addToHNCConfig(ctx, "v2", "ConfigMap", api.Propagate) + + Eventually(hasHNCConfigurationCondition(ctx, api.ObjectReconcilerCreationFailed)).Should(BeTrue()) + }) + + It("should unset ObjectReconcilerCreationFailed condition if an object reconciler creation later succeeds", func() { + // API version of LimitRange should be "v1" + addToHNCConfig(ctx, "v2", "LimitRange", api.Propagate) + + Eventually(hasHNCConfigurationCondition(ctx, api.ObjectReconcilerCreationFailed)).Should(BeTrue()) + + updateHNCConfigSpec(ctx, "v2", "v1", "LimitRange", "LimitRange", api.Propagate, api.Propagate) + + Eventually(hasHNCConfigurationCondition(ctx, api.ObjectReconcilerCreationFailed)).Should(BeFalse()) + }) + + It("should only propagate objects whose types are in HNCConfiguration", func() { + setParent(ctx, barName, fooName) + makeSecret(ctx, fooName, "foo-sec") + makeResourceQuota(ctx, fooName, "foo-resource-quota") + + addToHNCConfig(ctx, "v1", "Secret", api.Propagate) + + // Foo should have both "foo-sec" and "foo-resource-quota" since we created there. + Eventually(hasSecret(ctx, fooName, "foo-sec")).Should(BeTrue()) + Eventually(hasResourceQuota(ctx, fooName, "foo-resource-quota")).Should(BeTrue()) // "foo-sec" should now be propagated from foo to bar. Eventually(hasSecret(ctx, barName, "foo-sec")).Should(BeTrue()) Expect(secretInheritedFrom(ctx, barName, "foo-sec")).Should(Equal(fooName)) + // "foo-resource-quota" should not be propagated from foo to bar because ResourceQuota + // is not added to HNCConfiguration. + Expect(hasResourceQuota(ctx, barName, "foo-resource-quota")).Should(BeFalse()) }) }) -func addSecretToHNCConfig(ctx context.Context, c *api.HNCConfiguration) error { - secSpec := api.TypeSynchronizationSpec{APIVersion: "v1", Kind: "Secret", Mode: api.Propagate} - c.Spec.Types = append(c.Spec.Types, secSpec) - return updateHNCConfig(ctx, c) +func hasTypeWithMode(apiVersion, kind string, mode api.SynchronizationMode, config *api.HNCConfiguration) func() bool { + // `Eventually` only works with a fn that doesn't take any args + return func() bool { + for _, t := range config.Spec.Types { + if t.APIVersion == apiVersion && t.Kind == kind && t.Mode == mode { + return true + } + } + return false + } } func makeSecret(ctx context.Context, nsName, secretName string) { @@ -89,3 +148,106 @@ func secretInheritedFrom(ctx context.Context, nsName, secretName string) string lif, _ := sec.ObjectMeta.Labels["hnc.x-k8s.io/inheritedFrom"] return lif } + +func makeRoleBinding(ctx context.Context, nsName, roleName, userName, roleBindingName string) { + roleBinding := &v1.RoleBinding{ + ObjectMeta: metav1.ObjectMeta{ + Name: roleBindingName, + Namespace: nsName, + }, + Subjects: []v1.Subject{ + { + Kind: "User", + Name: userName, + }, + }, + RoleRef: v1.RoleRef{ + APIGroup: "rbac.authorization.k8s.io", + Kind: "Role", + Name: roleName, + }, + } + ExpectWithOffset(1, k8sClient.Create(ctx, roleBinding)).Should(Succeed()) +} + +func hasRoleBinding(ctx context.Context, nsName, roleBindingName string) func() bool { + // `Eventually` only works with a fn that doesn't take any args + return func() bool { + nnm := types.NamespacedName{Namespace: nsName, Name: roleBindingName} + roleBinding := &v1.RoleBinding{} + err := k8sClient.Get(ctx, nnm, roleBinding) + return err == nil + } +} + +func roleBindingInheritedFrom(ctx context.Context, nsName, roleBindingName string) string { + nnm := types.NamespacedName{Namespace: nsName, Name: roleBindingName} + roleBinding := &v1.RoleBinding{} + if err := k8sClient.Get(ctx, nnm, roleBinding); err != nil { + // should have been caught above + return err.Error() + } + if roleBinding.ObjectMeta.Labels == nil { + return "" + } + lif, _ := roleBinding.ObjectMeta.Labels["hnc.x-k8s.io/inheritedFrom"] + return lif +} + +func makeResourceQuota(ctx context.Context, nsName, resourceQuotaName string) { + resourceQuota := &corev1.ResourceQuota{} + resourceQuota.Name = resourceQuotaName + resourceQuota.Namespace = nsName + ExpectWithOffset(1, k8sClient.Create(ctx, resourceQuota)).Should(Succeed()) +} + +func hasResourceQuota(ctx context.Context, nsName, resourceQuotaName string) bool { + // `Eventually` only works with a fn that doesn't take any args + nnm := types.NamespacedName{Namespace: nsName, Name: resourceQuotaName} + resourceQuota := &corev1.ResourceQuota{} + err := k8sClient.Get(ctx, nnm, resourceQuota) + return err == nil +} + +func hasHNCConfigurationCondition(ctx context.Context, code api.HNCConfigurationCode) func() bool { + return func() bool { + c := getHNCConfig(ctx) + return hasHNCConfigurationConditionWithSingleton(code, c) + } +} + +func hasHNCConfigurationConditionWithName(ctx context.Context, code api.HNCConfigurationCode, nm string) func() bool { + return func() bool { + c := getHNCConfigWithOffsetAndName(1, ctx, nm) + return hasHNCConfigurationConditionWithSingleton(code, c) + } +} + +func hasHNCConfigurationConditionWithSingleton(code api.HNCConfigurationCode, c *api.HNCConfiguration) bool { + conds := c.Status.Conditions + if code == "" { + return len(conds) > 0 + } + for _, cond := range conds { + if cond.Code == code { + return true + } + } + return false +} + +func updateHNCConfigSpec(ctx context.Context, prevApiVersion, newApiVersion, prevKind, newKind string, + preMode, newMode api.SynchronizationMode) { + Eventually(func() error { + c := getHNCConfig(ctx) + for i := 0; i < len(c.Spec.Types); i++ { + if c.Spec.Types[i].APIVersion == prevApiVersion && c.Spec.Types[i].Kind == prevKind && c.Spec.Types[i].Mode == preMode { + c.Spec.Types[i].APIVersion = newApiVersion + c.Spec.Types[i].Kind = newKind + c.Spec.Types[i].Mode = newMode + break + } + } + return updateHNCConfig(ctx, c) + }).Should(Succeed()) +} diff --git a/incubator/hnc/pkg/reconcilers/object_test.go b/incubator/hnc/pkg/reconcilers/object_test.go index 2ca1fbe89..194c9ab32 100644 --- a/incubator/hnc/pkg/reconcilers/object_test.go +++ b/incubator/hnc/pkg/reconcilers/object_test.go @@ -10,7 +10,6 @@ import ( corev1 "k8s.io/api/core/v1" v1 "k8s.io/api/rbac/v1" "k8s.io/apimachinery/pkg/api/errors" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" ) @@ -28,7 +27,7 @@ var _ = Describe("Secret", func() { barName = createNS(ctx, "bar") bazName = createNS(ctx, "baz") - // Give them each a secret + // Give them each a role. makeRole(ctx, fooName, "foo-role") makeRole(ctx, barName, "bar-role") makeRole(ctx, bazName, "baz-role") @@ -60,7 +59,7 @@ var _ = Describe("Secret", func() { // Creates an empty ConfigMap. We use ConfigMap for this test because the apiserver will not // add additional fields to an empty ConfigMap object to make it non-empty. makeConfigMap(ctx, fooName, "foo-config") - addConfigMapToHNCConfig(ctx) + addToHNCConfig(ctx, "v1", "ConfigMap", api.Propagate) // "foo-config" should now be propagated from foo to bar. Eventually(hasConfigMap(ctx, barName, "foo-config")).Should(BeTrue()) @@ -243,52 +242,6 @@ func newOrGetHierarchy(ctx context.Context, nm string) *api.HierarchyConfigurati return hier } -func makeRole(ctx context.Context, nsName, roleName string) { - role := &v1.Role{ - ObjectMeta: metav1.ObjectMeta{ - Name: roleName, - Namespace: nsName, - }, - TypeMeta: metav1.TypeMeta{ - Kind: "Role", - APIVersion: "rbac.authorization.k8s.io/v1", - }, - Rules: []v1.PolicyRule{ - // Allow the users to read all secrets, namespaces and configmaps. - { - APIGroups: []string{""}, - Resources: []string{"secrets", "namespaces", "configmaps"}, - Verbs: []string{"get", "watch", "list"}, - }, - }, - } - ExpectWithOffset(1, k8sClient.Create(ctx, role)).Should(Succeed()) -} - -func hasRole(ctx context.Context, nsName, roleName string) func() bool { - // `Eventually` only works with a fn that doesn't take any args - return func() bool { - nnm := types.NamespacedName{Namespace: nsName, Name: roleName} - role := &v1.Role{} - err := k8sClient.Get(ctx, nnm, role) - return err == nil - } -} - -func roleInheritedFrom(ctx context.Context, nsName, roleName string) string { - nnm := types.NamespacedName{Namespace: nsName, Name: roleName} - role := &v1.Role{} - if err := k8sClient.Get(ctx, nnm, role); err != nil { - // should have been caught above - return err.Error() - } - if role.ObjectMeta.Labels == nil { - return "" - } - lif, _ := role.ObjectMeta.Labels["hnc.x-k8s.io/inheritedFrom"] - return lif -} - func modifyRole(ctx context.Context, nsName, roleName string) { nnm := types.NamespacedName{Namespace: nsName, Name: roleName} role := &v1.Role{} @@ -332,15 +285,6 @@ func removeRole(ctx context.Context, nsName, roleName string) { ExpectWithOffset(1, k8sClient.Delete(ctx, role)).Should(Succeed()) } -func addConfigMapToHNCConfig(ctx context.Context) { - Eventually(func() error { - c := getHNCConfig(ctx) - configMap := api.TypeSynchronizationSpec{APIVersion: "v1", Kind: "ConfigMap", Mode: api.Propagate} - c.Spec.Types = append(c.Spec.Types, configMap) - return updateHNCConfig(ctx, c) - }).Should(Succeed()) -} - // Makes an empty ConfigMap object. func makeConfigMap(ctx context.Context, nsName, configMapName string) { configMap := &corev1.ConfigMap{} diff --git a/incubator/hnc/pkg/reconcilers/setup.go b/incubator/hnc/pkg/reconcilers/setup.go index 03d448021..3b182b6f4 100644 --- a/incubator/hnc/pkg/reconcilers/setup.go +++ b/incubator/hnc/pkg/reconcilers/setup.go @@ -41,11 +41,22 @@ func Create(mgr ctrl.Manager, f *forest.Forest, maxReconciles int) error { Log: ctrl.Log.WithName("reconcilers").WithName("HNCConfiguration"), Manager: mgr, Forest: f, + Igniter: make(chan event.GenericEvent), HierarchyConfigUpdates: hcChan, } if err := cr.SetupWithManager(mgr); err != nil { return fmt.Errorf("cannot create Config reconciler: %s", err.Error()) } + // Create a default singleton if there is no singleton in the cluster. + // + // The cache used by the client to retrieve objects might not be populated + // at this point. As a result, we cannot use r.Get() to determine the existence + // of the singleton and then use r.Create() to create the singleton if + // it does not exist. As a workaround, we decide to enforce reconciliation. The + // cache is populated at the reconciliation stage. A default singleton will be + // created during the reconciliation if there is no singleton in the cluster. + cr.forceInitialReconcile(cr.Log, "Enforce reconciliation to create a default"+ + "HNCConfiguration singleton if it does not exist.") return nil } diff --git a/incubator/hnc/pkg/reconcilers/suite_test.go b/incubator/hnc/pkg/reconcilers/suite_test.go index 835013f8a..b41be3e7f 100644 --- a/incubator/hnc/pkg/reconcilers/suite_test.go +++ b/incubator/hnc/pkg/reconcilers/suite_test.go @@ -16,7 +16,6 @@ limitations under the License. package reconcilers_test import ( - "context" "path/filepath" "testing" "time" @@ -36,7 +35,6 @@ import ( // +kubebuilder:scaffold:imports 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" ) @@ -59,6 +57,8 @@ func TestAPIs(t *testing.T) { []Reporter{envtest.NewlineReporter{}}) } +// All tests in the reconcilers_test package are in one suite. As a result, they +// share the same test environment (e.g., same api server). var _ = BeforeSuite(func(done Done) { logf.SetLogger(zap.LoggerTo(GinkgoWriter, true)) @@ -92,12 +92,6 @@ var _ = BeforeSuite(func(done Done) { k8sClient = k8sManager.GetClient() Expect(k8sClient).ToNot(BeNil()) - // Setup HNCConfiguration object here because it is a cluster-wide singleton shared by all reconcilers. - hncConfig := newHNCConfig() - Expect(hncConfig).ToNot(BeNil()) - ctx := context.Background() - updateHNCConfig(ctx, hncConfig) - go func() { err = k8sManager.Start(ctrl.SetupSignalHandler()) Expect(err).ToNot(HaveOccurred()) @@ -111,10 +105,3 @@ var _ = AfterSuite(func() { err := testEnv.Stop() Expect(err).ToNot(HaveOccurred()) }) - -func newHNCConfig() *api.HNCConfiguration { - hncConfig := &api.HNCConfiguration{} - hncConfig.ObjectMeta.Name = api.HNCConfigSingleton - hncConfig.Spec = config.GetDefaultConfigSpec() - return hncConfig -} diff --git a/incubator/hnc/pkg/reconcilers/test_helpers_test.go b/incubator/hnc/pkg/reconcilers/test_helpers_test.go index e0e0ecf44..c7d3c4ba3 100644 --- a/incubator/hnc/pkg/reconcilers/test_helpers_test.go +++ b/incubator/hnc/pkg/reconcilers/test_helpers_test.go @@ -7,6 +7,8 @@ import ( . "github.com/onsi/gomega" corev1 "k8s.io/api/core/v1" + v1 "k8s.io/api/rbac/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" api "github.com/kubernetes-sigs/multi-tenancy/incubator/hnc/api/v1alpha1" @@ -78,18 +80,75 @@ func updateHNCConfig(ctx context.Context, c *api.HNCConfiguration) error { func resetHNCConfigToDefault(ctx context.Context) error { c := getHNCConfig(ctx) c.Spec = config.GetDefaultConfigSpec() + c.Status.Types = nil + c.Status.Conditions = nil return k8sClient.Update(ctx, c) } func getHNCConfig(ctx context.Context) *api.HNCConfiguration { - return getHNCConfigWithOffset(1, ctx) + return getHNCConfigWithOffsetAndName(1, ctx, api.HNCConfigSingleton) } -func getHNCConfigWithOffset(offset int, ctx context.Context) *api.HNCConfiguration { - snm := types.NamespacedName{Name: api.HNCConfigSingleton} +func getHNCConfigWithOffsetAndName(offset int, ctx context.Context, nm string) *api.HNCConfiguration { + snm := types.NamespacedName{Name: nm} config := &api.HNCConfiguration{} EventuallyWithOffset(offset+1, func() error { return k8sClient.Get(ctx, snm, config) }).Should(Succeed()) return config } + +func addToHNCConfig(ctx context.Context, apiVersion, kind string, mode api.SynchronizationMode) { + Eventually(func() error { + c := getHNCConfig(ctx) + spec := api.TypeSynchronizationSpec{APIVersion: apiVersion, Kind: kind, Mode: mode} + c.Spec.Types = append(c.Spec.Types, spec) + return updateHNCConfig(ctx, c) + }).Should(Succeed()) +} + +func makeRole(ctx context.Context, nsName, roleName string) { + role := &v1.Role{ + ObjectMeta: metav1.ObjectMeta{ + Name: roleName, + Namespace: nsName, + }, + TypeMeta: metav1.TypeMeta{ + Kind: "Role", + APIVersion: "rbac.authorization.k8s.io/v1", + }, + Rules: []v1.PolicyRule{ + // Allow the users to read all secrets, namespaces and configmaps. + { + APIGroups: []string{""}, + Resources: []string{"secrets", "namespaces", "configmaps"}, + Verbs: []string{"get", "watch", "list"}, + }, + }, + } + ExpectWithOffset(1, k8sClient.Create(ctx, role)).Should(Succeed()) +} + +func hasRole(ctx context.Context, nsName, roleName string) func() bool { + // `Eventually` only works with a fn that doesn't take any args + return func() bool { + nnm := types.NamespacedName{Namespace: nsName, Name: roleName} + role := &v1.Role{} + err := k8sClient.Get(ctx, nnm, role) + return err == nil + } +} + +func roleInheritedFrom(ctx context.Context, nsName, roleName string) string { + nnm := types.NamespacedName{Namespace: nsName, Name: roleName} + role := &v1.Role{} + if err := k8sClient.Get(ctx, nnm, role); err != nil { + // should have been caught above + return err.Error() + } + if role.ObjectMeta.Labels == nil { + return "" + } + lif, _ := role.ObjectMeta.Labels["hnc.x-k8s.io/inheritedFrom"] + return lif +}