Merge pull request #529 from sophieliu15/duplicate_types

Handle multiple configurations of the same type in ConfigReconciler

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

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

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

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{