From bde8491e4fbaf1f2150ff6fcf1249271ab7b39c2 Mon Sep 17 00:00:00 2001 From: sophieliu15 Date: Mon, 23 Mar 2020 12:32:03 -0400 Subject: [PATCH] Handle multiple configurations of the same type This PR handles multiple configurations for the same type in ConfigReconciler. If there are multiple configurations for the same type, only the first configuration will be followed, the rest will be ignored. Tested:unit tests, GKE cluster Issue: #411 --- incubator/hnc/api/v1alpha1/hnc_config.go | 8 +++- .../bases/hnc.x-k8s.io_hncconfigurations.yaml | 4 +- incubator/hnc/pkg/reconcilers/hnc_config.go | 31 ++++++++++++- .../hnc/pkg/reconcilers/hnc_config_test.go | 44 +++++++++++++++++++ 4 files changed, 83 insertions(+), 4 deletions(-) diff --git a/incubator/hnc/api/v1alpha1/hnc_config.go b/incubator/hnc/api/v1alpha1/hnc_config.go index fa1288885..3b1f1d677 100644 --- a/incubator/hnc/api/v1alpha1/hnc_config.go +++ b/incubator/hnc/api/v1alpha1/hnc_config.go @@ -45,8 +45,9 @@ const ( // HNCConfigurationCondition codes. *All* codes must also be documented in the // comment to HNCConfigurationCondition.Code. const ( - CritSingletonNameInvalid HNCConfigurationCode = "critSingletonNameInvalid" - ObjectReconcilerCreationFailed HNCConfigurationCode = "objectReconcilerCreationFailed" + CritSingletonNameInvalid HNCConfigurationCode = "critSingletonNameInvalid" + ObjectReconcilerCreationFailed HNCConfigurationCode = "objectReconcilerCreationFailed" + MultipleConfigurationsForOneType HNCConfigurationCode = "multipleConfigurationsForOneType" ) // TypeSynchronizationSpec defines the desired synchronization state of a specific kind. @@ -133,6 +134,9 @@ type HNCConfigurationCondition struct { // // - "objectReconcilerCreationFailed": an error exists when creating the object // reconciler for the type specified in Msg. + // + // - "multipleConfigurationsForOneType": Multiple configurations exist for the type specified + // in the Msg. One type should only have one configuration. Code HNCConfigurationCode `json:"code"` // A human-readable description of the condition, if the `code` field is not 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 2a607ead5..2ac140dfe 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 @@ -80,7 +80,9 @@ spec: the specified singleton name is invalid. The name should be the same as HNCConfigSingleton. \n - \"objectReconcilerCreationFailed\": an error exists when creating the object reconciler for the - type specified in Msg." + type specified in Msg. \n - \"multipleConfigurationsForOneType\": + Multiple configurations exist for the type specified in the + Msg. One type should only have one configuration." type: string msg: description: A human-readable description of the condition, if diff --git a/incubator/hnc/pkg/reconcilers/hnc_config.go b/incubator/hnc/pkg/reconcilers/hnc_config.go index d4d3a0f54..17a0761d6 100644 --- a/incubator/hnc/pkg/reconcilers/hnc_config.go +++ b/incubator/hnc/pkg/reconcilers/hnc_config.go @@ -44,13 +44,14 @@ type ConfigReconciler struct { HierarchyConfigUpdates chan event.GenericEvent } +type gvkSet map[schema.GroupVersionKind]bool + // 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) { 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 @@ -222,7 +223,14 @@ func (r *ConfigReconciler) syncObjectReconcilers(ctx context.Context, inst *api. // syncActiveReconcilers syncs object reconcilers for types that are in the Spec. If an object reconciler exists, it sets // its mode according to the Spec; otherwise, it creates the object reconciler. func (r *ConfigReconciler) syncActiveReconcilers(ctx context.Context, inst *api.HNCConfiguration) error { + // exist keeps track of existing types in the `config` singleton. + exist := gvkSet{} for _, t := range inst.Spec.Types { + // If there are multiple configurations of the same type, we will follow the first + // configuration and ignore the rest. + if !r.ensureNoDuplicateTypeConfigurations(inst, t, exist) { + continue + } gvk := schema.FromAPIVersionAndKind(t.APIVersion, t.Kind) if ts := r.Forest.GetTypeSyncer(gvk); ts != nil { if err := ts.SetMode(ctx, t.Mode, r.Log); err != nil { @@ -235,6 +243,27 @@ func (r *ConfigReconciler) syncActiveReconcilers(ctx context.Context, inst *api. return nil } +// ensureNoDuplicateTypeConfigurations checks whether the configuration of a type already exists and sets +// a condition in the status if the type exists. The method returns true if the type configuration does not exist; +// otherwise, returns false. +func (r *ConfigReconciler) ensureNoDuplicateTypeConfigurations(inst *api.HNCConfiguration, t api.TypeSynchronizationSpec, + mp gvkSet) bool { + gvk := schema.FromAPIVersionAndKind(t.APIVersion, t.Kind) + if !mp[gvk] { + mp[gvk] = true + return true + } + specMsg := fmt.Sprintf("APIVersion: %s, Kind: %s, Mode: %s", t.APIVersion, t.Kind, t.Mode) + r.Log.Info(fmt.Sprintf("Ignoring the configuration: %s", specMsg)) + condition := api.HNCConfigurationCondition{ + Code: api.MultipleConfigurationsForOneType, + Msg: fmt.Sprintf("Ignore the configuration: %s because the configuration of the type already exists; "+ + "only the first configuration will be applied", specMsg), + } + inst.Status.Conditions = append(inst.Status.Conditions, condition) + return false +} + // syncRemovedReconcilers sets object reconcilers to "ignore" mode for types that are removed from the Spec. func (r *ConfigReconciler) syncRemovedReconcilers(ctx context.Context, inst *api.HNCConfiguration) error { // If a type exists in the forest but not exists in the Spec, we will diff --git a/incubator/hnc/pkg/reconcilers/hnc_config_test.go b/incubator/hnc/pkg/reconcilers/hnc_config_test.go index f1eee37a8..02e57384c 100644 --- a/incubator/hnc/pkg/reconcilers/hnc_config_test.go +++ b/incubator/hnc/pkg/reconcilers/hnc_config_test.go @@ -2,6 +2,7 @@ package reconcilers_test import ( "context" + "fmt" "strings" "time" @@ -116,6 +117,30 @@ var _ = Describe("HNCConfiguration", func() { Eventually(hasHNCConfigurationConditionWithMsg(ctx, api.ObjectReconcilerCreationFailed, "/v2, Kind=LimitRange")).Should(BeFalse()) }) + It("should set MultipleConfigurationsForOneType if there are multiple configurations for one type", func() { + // Add multiple configurations for a type. + addToHNCConfig(ctx, "v1", "Secret", api.Propagate) + addToHNCConfig(ctx, "v1", "Secret", api.Remove) + + // The second configuration should be ignored. + Eventually(hasHNCConfigurationConditionWithMsg(ctx, api.MultipleConfigurationsForOneType, + fmt.Sprintf("APIVersion: v1, Kind: Secret, Mode: %s", api.Remove))).Should(BeTrue()) + }) + + It("should unset MultipleConfigurationsForOneType if extra configurations are later removed", func() { + // Add multiple configurations for a type. + addToHNCConfig(ctx, "v1", "Secret", api.Propagate) + addToHNCConfig(ctx, "v1", "Secret", api.Remove) + + Eventually(hasHNCConfigurationConditionWithMsg(ctx, api.MultipleConfigurationsForOneType, + fmt.Sprintf("APIVersion: v1, Kind: Secret, Mode: %s", api.Remove))).Should(BeTrue()) + + removeHNCConfigTypeWithMode(ctx, "v1", "Secret", api.Remove) + + Eventually(hasHNCConfigurationConditionWithMsg(ctx, api.MultipleConfigurationsForOneType, + fmt.Sprintf("APIVersion: v1, Kind: Secret, Mode: %s", api.Remove))).Should(BeFalse()) + }) + It("should not propagate objects if the type is not in HNCConfiguration", func() { setParent(ctx, barName, fooName) makeObject(ctx, "ResourceQuota", fooName, "foo-resource-quota") @@ -361,6 +386,25 @@ func removeHNCConfigType(ctx context.Context, apiVersion, kind string) { }).Should(Succeed()) } +func removeHNCConfigTypeWithMode(ctx context.Context, apiVersion, kind string, mode api.SynchronizationMode) { + Eventually(func() error { + c := getHNCConfig(ctx) + i := 0 + for ; i < len(c.Spec.Types); i++ { + if c.Spec.Types[i].APIVersion == apiVersion && c.Spec.Types[i].Kind == kind && c.Spec.Types[i].Mode == mode { + break + } + } + // The type does not exist. Nothing to remove. + if i == len(c.Spec.Types) { + return nil + } + c.Spec.Types[i] = c.Spec.Types[len(c.Spec.Types)-1] + c.Spec.Types = c.Spec.Types[:len(c.Spec.Types)-1] + return updateHNCConfig(ctx, c) + }).Should(Succeed()) +} + func createCronTabCRD(ctx context.Context) { crontab := v1beta1.CustomResourceDefinition{ TypeMeta: metav1.TypeMeta{