feat: improve ConfigMap handling with better tests and error handling

- Add comprehensive test coverage for ConfigMap changes
- Fix timing issues with hash annotations
- Add test for multiple ConfigMaps error condition
- Improve controller to reject multiple ConfigMaps per provider
- Add proper ReadyCondition setting in tests

Author: PavanNeerudu

internal/controller/configmap_changes_test.go

@@ -24,7 +24,9 @@ import (
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	operatorv1 "sigs.k8s.io/cluster-api-operator/api/v1alpha2"
+	clusterv1 "sigs.k8s.io/cluster-api/api/v1beta1"
 	"sigs.k8s.io/cluster-api/util/conditions"
+	"sigs.k8s.io/cluster-api/util/patch"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
 
@@ -45,8 +47,8 @@ func TestConfigMapChangesAreAppliedToTheProvider(t *testing.T) {
 	// Create ConfigMap with initial content
 	configMap := &corev1.ConfigMap{
 		ObjectMeta: metav1.ObjectMeta{
-			GenerateName: "edge-provider-",
-			Namespace:    ns.Name,
+			Name:      testCurrentVersion,
+			Namespace: ns.Name,
 			Labels: map[string]string{
 				"provider-components": "edge",
 			},
@@ -86,6 +88,12 @@ func TestConfigMapChangesAreAppliedToTheProvider(t *testing.T) {
 
 	t.Log("CoreProvider is installed")
 
+	// Manually set ReadyCondition as it's not set automatically in test env
+	patchHelper, err := patch.NewHelper(coreProvider, env)
+	g.Expect(err).ToNot(HaveOccurred())
+	conditions.MarkTrue(coreProvider, clusterv1.ReadyCondition)
+	g.Expect(patchHelper.Patch(ctx, coreProvider)).To(Succeed())
+
 	// Create InfrastructureProvider that uses the ConfigMap
 	provider := &operatorv1.InfrastructureProvider{
 		ObjectMeta: metav1.ObjectMeta{
@@ -121,6 +129,15 @@ func TestConfigMapChangesAreAppliedToTheProvider(t *testing.T) {
 
 	t.Log("Provider is installed")
 
+	// Wait for the provider to have a hash annotation (this happens after full reconciliation)
+	g.Eventually(func() bool {
+		if err := env.Get(ctx, client.ObjectKeyFromObject(provider), provider); err != nil {
+			return false
+		}
+		hash := provider.GetAnnotations()[appliedSpecHashAnnotation]
+		return hash != ""
+	}, timeout).Should(BeTrue(), "Provider should have a hash annotation after reconciliation")
+
 	// Get the initial hash annotation
 	g.Expect(env.Get(ctx, client.ObjectKeyFromObject(provider), provider)).To(Succeed())
 	initialHash := provider.GetAnnotations()[appliedSpecHashAnnotation]
@@ -148,40 +165,20 @@ func TestConfigMapChangesAreAppliedToTheProvider(t *testing.T) {
 	t.Log("Provider reconciled with new hash")
 }
 
-func TestConfigMapChangesWithMultipleProviders(t *testing.T) {
+func TestConfigMapChangesWithNonMatchingSelector(t *testing.T) {
 	g := NewWithT(t)
 	objs := []client.Object{}
 
 	defer func() {
 		g.Expect(env.CleanupAndWait(ctx, objs...)).To(Succeed())
 	}()
 
-	ns, err := env.CreateNamespace(ctx, "multiple-providers-namespace")
+	ns, err := env.CreateNamespace(ctx, "non-matching-selector-namespace")
 	g.Expect(err).ToNot(HaveOccurred())
 	g.Expect(ns.Name).NotTo(BeEmpty())
 
 	t.Log("Ensure namespace exists", ns.Name)
 
-	// Create ConfigMap with initial content
-	configMap := &corev1.ConfigMap{
-		ObjectMeta: metav1.ObjectMeta{
-			GenerateName: "shared-configmap-",
-			Namespace:    ns.Name,
-			Labels: map[string]string{
-				"provider-components": "shared",
-				"environment":         "test",
-			},
-		},
-		Data: map[string]string{
-			"metadata":   testMetadata,
-			"components": testComponents,
-		},
-	}
-	g.Expect(env.CreateAndWait(ctx, configMap)).To(Succeed())
-	objs = append(objs, configMap)
-
-	t.Log("Created shared ConfigMap")
-
 	// Create CoreProvider first (required for InfrastructureProvider)
 	coreProvider := &operatorv1.CoreProvider{
 		ObjectMeta: metav1.ObjectMeta{
@@ -207,93 +204,11 @@ func TestConfigMapChangesWithMultipleProviders(t *testing.T) {
 
 	t.Log("CoreProvider is installed")
 
-	// Create multiple providers that use the same ConfigMap
-	providers := []*operatorv1.InfrastructureProvider{}
-	for i := 0; i < 2; i++ {
-		provider := &operatorv1.InfrastructureProvider{
-			ObjectMeta: metav1.ObjectMeta{
-				GenerateName: "provider-",
-				Namespace:    ns.Name,
-			},
-			Spec: operatorv1.InfrastructureProviderSpec{
-				ProviderSpec: operatorv1.ProviderSpec{
-					Version: testCurrentVersion,
-					FetchConfig: &operatorv1.FetchConfiguration{
-						Selector: &metav1.LabelSelector{
-							MatchLabels: map[string]string{
-								"provider-components": "shared",
-							},
-						},
-					},
-				},
-			},
-		}
-		g.Expect(env.CreateAndWait(ctx, provider)).To(Succeed())
-		objs = append(objs, provider)
-		providers = append(providers, provider)
-	}
-
-	t.Log("Created multiple providers")
-
-	// Wait for all providers to be ready
-	for _, provider := range providers {
-		g.Eventually(func() bool {
-			if err := env.Get(ctx, client.ObjectKeyFromObject(provider), provider); err != nil {
-				return false
-			}
-
-			return conditions.IsTrue(provider, operatorv1.ProviderInstalledCondition)
-		}, timeout).Should(BeTrue())
-	}
-
-	t.Log("All providers are installed")
-
-	// Get initial hash annotations
-	initialHashes := make(map[string]string)
-	for i, provider := range providers {
-		g.Expect(env.Get(ctx, client.ObjectKeyFromObject(provider), provider)).To(Succeed())
-		hash := provider.GetAnnotations()[appliedSpecHashAnnotation]
-		g.Expect(hash).ToNot(BeEmpty())
-		initialHashes[provider.GetName()] = hash
-		t.Logf("Provider %d initial hash: %s", i, hash)
-	}
-
-	// Update the ConfigMap content
-	g.Expect(env.Get(ctx, client.ObjectKeyFromObject(configMap), configMap)).To(Succeed())
-	configMap.Data["components"] = testComponents + "\n# Updated shared content"
-	g.Expect(env.Update(ctx, configMap)).To(Succeed())
-
-	t.Log("Shared ConfigMap updated")
-
-	// Wait for all providers to be reconciled with new hashes
-	for i, provider := range providers {
-		g.Eventually(func() bool {
-			if err := env.Get(ctx, client.ObjectKeyFromObject(provider), provider); err != nil {
-				return false
-			}
-
-			currentHash := provider.GetAnnotations()[appliedSpecHashAnnotation]
-			initialHash := initialHashes[provider.GetName()]
-			return currentHash != "" && currentHash != initialHash
-		}, 30*time.Second).Should(BeTrue())
-
-		t.Logf("Provider %d reconciled with new hash", i)
-	}
-}
-
-func TestConfigMapChangesWithNonMatchingSelector(t *testing.T) {
-	g := NewWithT(t)
-	objs := []client.Object{}
-
-	defer func() {
-		g.Expect(env.CleanupAndWait(ctx, objs...)).To(Succeed())
-	}()
-
-	ns, err := env.CreateNamespace(ctx, "non-matching-selector-namespace")
+	// Manually set ReadyCondition as it's not set automatically in test env
+	patchHelper, err := patch.NewHelper(coreProvider, env)
 	g.Expect(err).ToNot(HaveOccurred())
-	g.Expect(ns.Name).NotTo(BeEmpty())
-
-	t.Log("Ensure namespace exists", ns.Name)
+	conditions.MarkTrue(coreProvider, clusterv1.ReadyCondition)
+	g.Expect(patchHelper.Patch(ctx, coreProvider)).To(Succeed())
 
 	// Create ConfigMap that won't match any provider selector
 	configMap := &corev1.ConfigMap{
@@ -362,6 +277,15 @@ func TestConfigMapChangesWithNonMatchingSelector(t *testing.T) {
 		return conditions.IsTrue(provider, operatorv1.ProviderInstalledCondition)
 	}, timeout).Should(BeTrue())
 
+	// Wait for the provider to have a hash annotation (this happens after full reconciliation)
+	g.Eventually(func() bool {
+		if err := env.Get(ctx, client.ObjectKeyFromObject(provider), provider); err != nil {
+			return false
+		}
+		hash := provider.GetAnnotations()[appliedSpecHashAnnotation]
+		return hash != ""
+	}, timeout).Should(BeTrue(), "Provider should have a hash annotation after reconciliation")
+
 	// Get initial hash
 	g.Expect(env.Get(ctx, client.ObjectKeyFromObject(provider), provider)).To(Succeed())
 	initialHash := provider.GetAnnotations()[appliedSpecHashAnnotation]
@@ -405,3 +329,119 @@ func TestConfigMapChangesWithNonMatchingSelector(t *testing.T) {
 
 	t.Log("Provider reconciled with new hash after matching ConfigMap update")
 }
+
+func TestMultipleConfigMapsError(t *testing.T) {
+	g := NewWithT(t)
+	objs := []client.Object{}
+
+	defer func() {
+		g.Expect(env.CleanupAndWait(ctx, objs...)).To(Succeed())
+	}()
+
+	ns, err := env.CreateNamespace(ctx, "multiple-configmaps-error-namespace")
+	g.Expect(err).ToNot(HaveOccurred())
+	g.Expect(ns.Name).NotTo(BeEmpty())
+
+	t.Log("Ensure namespace exists", ns.Name)
+
+	// Create CoreProvider first (required for InfrastructureProvider)
+	coreProvider := &operatorv1.CoreProvider{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "cluster-api",
+			Namespace: ns.Name,
+		},
+		Spec: operatorv1.CoreProviderSpec{
+			ProviderSpec: operatorv1.ProviderSpec{
+				Version: testCurrentVersion,
+			},
+		},
+	}
+	g.Expect(env.CreateAndWait(ctx, coreProvider)).To(Succeed())
+	objs = append(objs, coreProvider)
+
+	// Wait for CoreProvider to be installed
+	g.Eventually(func() bool {
+		if err := env.Get(ctx, client.ObjectKeyFromObject(coreProvider), coreProvider); err != nil {
+			return false
+		}
+		return conditions.IsTrue(coreProvider, operatorv1.ProviderInstalledCondition)
+	}, timeout).Should(BeTrue())
+
+	t.Log("CoreProvider is installed")
+
+	// Manually set ReadyCondition as it's not set automatically in test env
+	patchHelper, err := patch.NewHelper(coreProvider, env)
+	g.Expect(err).ToNot(HaveOccurred())
+	conditions.MarkTrue(coreProvider, clusterv1.ReadyCondition)
+	g.Expect(patchHelper.Patch(ctx, coreProvider)).To(Succeed())
+
+	// Create multiple ConfigMaps with the same labels (this should cause an error)
+	configMap1 := &corev1.ConfigMap{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      testCurrentVersion,
+			Namespace: ns.Name,
+			Labels: map[string]string{
+				"provider-components": "edge",
+			},
+		},
+		Data: map[string]string{
+			"metadata":   testMetadata,
+			"components": testComponents,
+		},
+	}
+	g.Expect(env.CreateAndWait(ctx, configMap1)).To(Succeed())
+	objs = append(objs, configMap1)
+
+	configMap2 := &corev1.ConfigMap{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "v1.1.0",
+			Namespace: ns.Name,
+			Labels: map[string]string{
+				"provider-components": "edge",
+			},
+		},
+		Data: map[string]string{
+			"metadata":   testMetadata,
+			"components": testComponents + "\n# Different content",
+		},
+	}
+	g.Expect(env.CreateAndWait(ctx, configMap2)).To(Succeed())
+	objs = append(objs, configMap2)
+
+	t.Log("Created multiple ConfigMaps with same labels")
+
+	// Create InfrastructureProvider that uses the ConfigMaps (should fail due to multiple matches)
+	provider := &operatorv1.InfrastructureProvider{
+		ObjectMeta: metav1.ObjectMeta{
+			GenerateName: "edge-provider-",
+			Namespace:    ns.Name,
+		},
+		Spec: operatorv1.InfrastructureProviderSpec{
+			ProviderSpec: operatorv1.ProviderSpec{
+				Version: testCurrentVersion,
+				FetchConfig: &operatorv1.FetchConfiguration{
+					Selector: &metav1.LabelSelector{
+						MatchLabels: map[string]string{
+							"provider-components": "edge",
+						},
+					},
+				},
+			},
+		},
+	}
+	g.Expect(env.CreateAndWait(ctx, provider)).To(Succeed())
+	objs = append(objs, provider)
+
+	t.Log("Created provider with selector matching multiple ConfigMaps")
+
+	// Provider should have error condition due to multiple ConfigMaps
+	g.Eventually(func() bool {
+		if err := env.Get(ctx, client.ObjectKeyFromObject(provider), provider); err != nil {
+			return false
+		}
+		return conditions.IsFalse(provider, operatorv1.ProviderInstalledCondition) &&
+			conditions.GetReason(provider, operatorv1.ProviderInstalledCondition) != ""
+	}, timeout).Should(BeTrue())
+
+	t.Log("Provider correctly failed with multiple ConfigMaps error")
+}

internal/controller/genericprovider_controller.go

@@ -309,18 +309,14 @@ func addConfigMapToHash(ctx context.Context, k8sClient client.Client, hash hash.
 			return err
 		}
 
-		// Sort the ConfigMaps by name for consistent hashing
-		configMaps := configMapList.Items
-		for i := 0; i < len(configMaps)-1; i++ {
-			for j := i + 1; j < len(configMaps); j++ {
-				if configMaps[i].Name > configMaps[j].Name {
-					configMaps[i], configMaps[j] = configMaps[j], configMaps[i]
-				}
-			}
+		// Ensure only one ConfigMap matches the selector
+		if len(configMapList.Items) > 1 {
+			return fmt.Errorf("multiple ConfigMaps match the provider selector, only one ConfigMap per provider is allowed")
 		}
 
-		// Add each ConfigMap's data to the hash
-		for _, cm := range configMaps {
+		// Add the ConfigMap's data to the hash (if any ConfigMap exists)
+		if len(configMapList.Items) == 1 {
+			cm := configMapList.Items[0]
 			if err := addObjectToHash(hash, cm.Data); err != nil {
 				return err
 			}