diff --git a/pkg/addon/controllers/cmainstallprogression/controller.go b/pkg/addon/controllers/cmainstallprogression/controller.go index 0b73f2587..b408a0d8d 100644 --- a/pkg/addon/controllers/cmainstallprogression/controller.go +++ b/pkg/addon/controllers/cmainstallprogression/controller.go @@ -5,7 +5,6 @@ import ( "sort" "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/util/sets" "k8s.io/klog/v2" addonv1beta1 "open-cluster-management.io/api/addon/v1beta1" @@ -130,17 +129,15 @@ func setInstallProgression(supportedConfigs []addonv1beta1.AddOnConfig, placemen } // set config references as default configuration - installConfigReferencesMap := map[addonv1beta1.ConfigGroupResource]sets.Set[addonv1beta1.ConfigReferent]{} + installConfigReferencesMap := map[addonv1beta1.ConfigGroupResource][]addonv1beta1.ConfigReferent{} for _, config := range supportedConfigs { if config.ConfigReferent.Name == "" || config.ConfigReferent.Name == addonv1beta1.ReservedNoDefaultConfigName { continue } - refs, ok := installConfigReferencesMap[config.ConfigGroupResource] - if !ok { - refs = sets.New[addonv1beta1.ConfigReferent]() - installConfigReferencesMap[config.ConfigGroupResource] = refs + if !containsConfigReferent(installConfigReferencesMap[config.ConfigGroupResource], config.ConfigReferent) { + installConfigReferencesMap[config.ConfigGroupResource] = append( + installConfigReferencesMap[config.ConfigGroupResource], config.ConfigReferent) } - refs.Insert(config.ConfigReferent) } // override the default configuration for each placement @@ -162,7 +159,7 @@ func setInstallProgression(supportedConfigs []addonv1beta1.AddOnConfig, placemen installConfigReferences := []addonv1beta1.InstallConfigReference{} for _, gvk := range gvks { if configRefs, ok := installConfigReferencesMap[gvk]; ok { - for configRef := range configRefs { + for _, configRef := range configRefs { installConfigReferences = append(installConfigReferences, addonv1beta1.InstallConfigReference{ ConfigGroupResource: gvk, @@ -223,23 +220,37 @@ func mergeInstallProgression(newobj, oldobj *addonv1beta1.InstallProgression) { } // Override the desired configs by a slice of AddOnConfig (from cma install strategy), +// preserving the order in which configs appear in addOnConfigs and deduplicating by +// ConfigReferent (namespace + name) within each GVK. func overrideConfigMapByAddOnConfigs( - desiredConfigs map[addonv1beta1.ConfigGroupResource]sets.Set[addonv1beta1.ConfigReferent], + desiredConfigs map[addonv1beta1.ConfigGroupResource][]addonv1beta1.ConfigReferent, addOnConfigs []addonv1beta1.AddOnConfig, ) { - gvkOverwritten := sets.New[addonv1beta1.ConfigGroupResource]() + gvkOverwritten := map[addonv1beta1.ConfigGroupResource]bool{} // Go through the cma install strategy configs, // for a group of configs with same gvk, install strategy configs override the desiredConfigs. for _, config := range addOnConfigs { gr := config.ConfigGroupResource - if !gvkOverwritten.Has(gr) { - desiredConfigs[gr] = sets.New[addonv1beta1.ConfigReferent]() - gvkOverwritten.Insert(gr) + if !gvkOverwritten[gr] { + desiredConfigs[gr] = []addonv1beta1.ConfigReferent{} + gvkOverwritten[gr] = true } - // If a config not exist in the desiredConfigs, append it. + // If a config does not exist in the desiredConfigs, append it. // This is to avoid adding duplicate configs (name + namespace). - if !desiredConfigs[gr].Has(config.ConfigReferent) { - desiredConfigs[gr].Insert(config.ConfigReferent) + if !containsConfigReferent(desiredConfigs[gr], config.ConfigReferent) { + desiredConfigs[gr] = append(desiredConfigs[gr], config.ConfigReferent) } } } + +// containsConfigReferent returns true if the given ConfigReferent exists in the slice. +// ConfigReferent is a struct of two strings (Namespace, Name), so == performs a +// field-by-field value comparison equivalent to r.Namespace == ref.Namespace && r.Name == ref.Name. +func containsConfigReferent(refs []addonv1beta1.ConfigReferent, ref addonv1beta1.ConfigReferent) bool { + for _, r := range refs { + if r == ref { + return true + } + } + return false +} diff --git a/pkg/addon/controllers/cmainstallprogression/controller_test.go b/pkg/addon/controllers/cmainstallprogression/controller_test.go index 1558128f9..8e0a55721 100644 --- a/pkg/addon/controllers/cmainstallprogression/controller_test.go +++ b/pkg/addon/controllers/cmainstallprogression/controller_test.go @@ -243,7 +243,16 @@ func TestReconcile(t *testing.T) { t.Errorf("InstallProgressions ConfigReferences object is not correct: %v", cma.Status.InstallProgressions[0].ConfigReferences) } if len(cma.Status.InstallProgressions[2].ConfigReferences) != 2 { - t.Errorf("InstallProgressions ConfigReferences object is not correct: %v", cma.Status.InstallProgressions[0].ConfigReferences) + t.Fatalf("InstallProgressions ConfigReferences object is not correct: %v", cma.Status.InstallProgressions[0].ConfigReferences) + } + // Verify ordering matches the spec order: test/test first, then test1/test + if cma.Status.InstallProgressions[2].ConfigReferences[0].DesiredConfig.Namespace != "test" || + cma.Status.InstallProgressions[2].ConfigReferences[0].DesiredConfig.Name != "test" { + t.Errorf("InstallProgressions ConfigReferences[0] ordering is not correct: %v", cma.Status.InstallProgressions[2].ConfigReferences[0].DesiredConfig) + } + if cma.Status.InstallProgressions[2].ConfigReferences[1].DesiredConfig.Namespace != "test1" || + cma.Status.InstallProgressions[2].ConfigReferences[1].DesiredConfig.Name != "test" { + t.Errorf("InstallProgressions ConfigReferences[1] ordering is not correct: %v", cma.Status.InstallProgressions[2].ConfigReferences[1].DesiredConfig) } if len(cma.Status.InstallProgressions[3].ConfigReferences) != 1 { t.Errorf("InstallProgressions ConfigReferences object is not correct: %v", cma.Status.InstallProgressions[0].ConfigReferences) @@ -362,7 +371,14 @@ func TestReconcile(t *testing.T) { t.Errorf("InstallProgressions ConfigReferences object is not correct: %v", cma.Status.InstallProgressions[0].ConfigReferences[0].DesiredConfig.Name) } if len(cma.Status.InstallProgressions[1].ConfigReferences) != 2 { - t.Errorf("InstallProgressions ConfigReferences object is not correct: %v", cma.Status.InstallProgressions[0].ConfigReferences) + t.Fatalf("InstallProgressions ConfigReferences object is not correct: %v", cma.Status.InstallProgressions[0].ConfigReferences) + } + // Verify ordering matches the spec order: test1 first, then test2 + if cma.Status.InstallProgressions[1].ConfigReferences[0].DesiredConfig.Name != "test1" { + t.Errorf("InstallProgressions ConfigReferences[0] ordering is not correct: %v", cma.Status.InstallProgressions[1].ConfigReferences[0].DesiredConfig) + } + if cma.Status.InstallProgressions[1].ConfigReferences[1].DesiredConfig.Name != "test2" { + t.Errorf("InstallProgressions ConfigReferences[1] ordering is not correct: %v", cma.Status.InstallProgressions[1].ConfigReferences[1].DesiredConfig) } if len(cma.Status.InstallProgressions[2].ConfigReferences) != 3 { t.Errorf("InstallProgressions ConfigReferences object is not correct: %v", cma.Status.InstallProgressions[0].ConfigReferences) diff --git a/test/integration/addon/addon_configs_test.go b/test/integration/addon/addon_configs_test.go index 5fb9ddccc..3bccbcf14 100644 --- a/test/integration/addon/addon_configs_test.go +++ b/test/integration/addon/addon_configs_test.go @@ -440,4 +440,169 @@ var _ = ginkgo.Describe("AddConfigs Beta", func() { }, }) }) + + ginkgo.It("Should preserve config reference ordering for multiple same-GVK configs in a placement", func() { + // Create two AddonDeploymentConfigs of the same GVK in the placement namespace. + // The ordering in the CMA placement spec should be preserved in both the CMA status + // InstallProgressions and the MCA status ConfigReferences. + config1 := &addonapiv1beta1.AddOnDeploymentConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "config-first", + Namespace: configDefaultNamespace, + }, + Spec: addOnTest1ConfigSpecBeta, + } + _, err = hubAddonClient.AddonV1beta1().AddOnDeploymentConfigs(configDefaultNamespace).Create( + context.Background(), config1, metav1.CreateOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + config2 := &addonapiv1beta1.AddOnDeploymentConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "config-second", + Namespace: configDefaultNamespace, + }, + Spec: addOnTest2ConfigSpecBeta, + } + _, err = hubAddonClient.AddonV1beta1().AddOnDeploymentConfigs(configDefaultNamespace).Create( + context.Background(), config2, metav1.CreateOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + // Patch CMA to use a placement strategy with 2 configs of the same GVK. + // The ordering config-first → config-second must be preserved. + cma, err := hubAddonClient.AddonV1beta1().ClusterManagementAddOns().Get( + context.Background(), testAddOnConfigsImpl.name, metav1.GetOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + cma.Spec.InstallStrategy = addonapiv1beta1.InstallStrategy{ + Type: addonapiv1beta1.AddonInstallStrategyPlacements, + Placements: []addonapiv1beta1.PlacementStrategy{ + { + PlacementRef: addonapiv1beta1.PlacementRef{ + Name: "test-placement", + Namespace: configDefaultNamespace, + }, + Configs: []addonapiv1beta1.AddOnConfig{ + { + ConfigGroupResource: addonapiv1beta1.ConfigGroupResource{ + Group: addOnDeploymentConfigGVR.Group, + Resource: addOnDeploymentConfigGVR.Resource, + }, + ConfigReferent: addonapiv1beta1.ConfigReferent{ + Namespace: configDefaultNamespace, + Name: "config-first", + }, + }, + { + ConfigGroupResource: addonapiv1beta1.ConfigGroupResource{ + Group: addOnDeploymentConfigGVR.Group, + Resource: addOnDeploymentConfigGVR.Resource, + }, + ConfigReferent: addonapiv1beta1.ConfigReferent{ + Namespace: configDefaultNamespace, + Name: "config-second", + }, + }, + }, + RolloutStrategy: clusterv1alpha1.RolloutStrategy{ + Type: clusterv1alpha1.All, + }, + }, + }, + } + patchClusterManagementAddOnBeta(context.Background(), cma) + + // Create placement and placement decision so the cluster is selected. + placement := &clusterv1beta1.Placement{ + ObjectMeta: metav1.ObjectMeta{Name: "test-placement", Namespace: configDefaultNamespace}, + } + _, err = hubClusterClient.ClusterV1beta1().Placements(configDefaultNamespace).Create( + context.Background(), placement, metav1.CreateOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + decision := &clusterv1beta1.PlacementDecision{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test-placement", + Namespace: configDefaultNamespace, + Labels: map[string]string{ + clusterv1beta1.PlacementLabel: "test-placement", + clusterv1beta1.DecisionGroupIndexLabel: "0", + }, + }, + } + decision, err = hubClusterClient.ClusterV1beta1().PlacementDecisions(configDefaultNamespace).Create( + context.Background(), decision, metav1.CreateOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + decision.Status.Decisions = []clusterv1beta1.ClusterDecision{ + {ClusterName: managedClusterName}, + } + _, err = hubClusterClient.ClusterV1beta1().PlacementDecisions(configDefaultNamespace).UpdateStatus( + context.Background(), decision, metav1.UpdateOptions{}) + gomega.Expect(err).ToNot(gomega.HaveOccurred()) + + // CMA status: InstallProgressions should have both configs in spec order (config-first → config-second). + assertClusterManagementAddOnInstallProgressionBeta(testAddOnConfigsImpl.name, addonapiv1beta1.InstallProgression{ + PlacementRef: addonapiv1beta1.PlacementRef{Name: "test-placement", Namespace: configDefaultNamespace}, + ConfigReferences: []addonapiv1beta1.InstallConfigReference{ + { + ConfigGroupResource: addonapiv1beta1.ConfigGroupResource{ + Group: addOnDeploymentConfigGVR.Group, + Resource: addOnDeploymentConfigGVR.Resource, + }, + DesiredConfig: &addonapiv1beta1.ConfigSpecHash{ + ConfigReferent: addonapiv1beta1.ConfigReferent{ + Namespace: configDefaultNamespace, + Name: "config-first", + }, + SpecHash: addOnTest1ConfigSpecHash, + }, + }, + { + ConfigGroupResource: addonapiv1beta1.ConfigGroupResource{ + Group: addOnDeploymentConfigGVR.Group, + Resource: addOnDeploymentConfigGVR.Resource, + }, + DesiredConfig: &addonapiv1beta1.ConfigSpecHash{ + ConfigReferent: addonapiv1beta1.ConfigReferent{ + Namespace: configDefaultNamespace, + Name: "config-second", + }, + SpecHash: addOnTest2ConfigSpecHash, + }, + }, + }, + }) + + // MCA status: ConfigReferences should reflect the same order (config-first → config-second). + assertManagedClusterAddOnConfigReferencesBeta(testAddOnConfigsImpl.name, managedClusterName, + addonapiv1beta1.ConfigReference{ + ConfigGroupResource: addonapiv1beta1.ConfigGroupResource{ + Group: addOnDeploymentConfigGVR.Group, + Resource: addOnDeploymentConfigGVR.Resource, + }, + LastObservedGeneration: 1, + DesiredConfig: &addonapiv1beta1.ConfigSpecHash{ + ConfigReferent: addonapiv1beta1.ConfigReferent{ + Namespace: configDefaultNamespace, + Name: "config-first", + }, + SpecHash: addOnTest1ConfigSpecHash, + }, + }, + addonapiv1beta1.ConfigReference{ + ConfigGroupResource: addonapiv1beta1.ConfigGroupResource{ + Group: addOnDeploymentConfigGVR.Group, + Resource: addOnDeploymentConfigGVR.Resource, + }, + LastObservedGeneration: 1, + DesiredConfig: &addonapiv1beta1.ConfigSpecHash{ + ConfigReferent: addonapiv1beta1.ConfigReferent{ + Namespace: configDefaultNamespace, + Name: "config-second", + }, + SpecHash: addOnTest2ConfigSpecHash, + }, + }, + ) + }) })