Draft: Fix DeleteURLsConfigMap

pkg/splunk/enterprise/monitoringconsole.go

@@ -304,71 +304,87 @@ func AddURLsConfigMap(revised *corev1.ConfigMap, crName string, newURLs []corev1
 		if !ok {
 			revised.Data[url.Name] = url.Value
 		} else {
+			// Split both existing and new URLs into slices
+			existingURLs := strings.Split(revised.Data[url.Name], ",")
 			newInsURLs := strings.Split(url.Value, ",")
-			//1. Find number of URLs, that crname,  present in the current configmap
-			var crURLs string
-			for _, newURL := range newInsURLs {
-				if strings.Contains(revised.Data[url.Name], newURL) {
-					if crURLs == "" {
-						crURLs = newURL
-					} else {
-						str := []string{crURLs, newURL}
-						crURLs = strings.Join(str, ",")
-					}
+
+			// Create a map to track existing URLs
+			urlMap := make(map[string]bool)
+			for _, existingURL := range existingURLs {
+				if strings.TrimSpace(existingURL) != "" {
+					urlMap[strings.TrimSpace(existingURL)] = true
 				}
 			}
-			//2. if length of both same then just reconcile
-			if len(crURLs) == len(url.Value) {
-				//reconcile
-				break
-			} else if len(crURLs) < len(url.Value) { //3. incoming URLs are more than current scaling up
-				//scaling UP
-				for _, newEntry := range newInsURLs {
-					if !strings.Contains(revised.Data[url.Name], newEntry) {
-						str := []string{revised.Data[url.Name], newEntry}
-						revised.Data[url.Name] = strings.Join(str, ",")
-					}
+
+			// Add new URLs that don't already exist
+			for _, newURL := range newInsURLs {
+				trimmedURL := strings.TrimSpace(newURL)
+				if trimmedURL != "" && !urlMap[trimmedURL] {
+					urlMap[trimmedURL] = true
 				}
-			} else { //4. incoming URLs are less than current then scaling down
-				//scaling DOWN pods
-				DeleteURLsConfigMap(revised, crName, newURLs, false)
 			}
+
+			// Rebuild the comma-separated string
+			var finalURLs []string
+			for urlKey := range urlMap {
+				finalURLs = append(finalURLs, urlKey)
+			}
+			sort.Strings(finalURLs) // Ensure consistent ordering
+			revised.Data[url.Name] = strings.Join(finalURLs, ",")
 		}
 	}
 }
 
 // DeleteURLsConfigMap for deleting server peers to the monitoring console or scaling down
 func DeleteURLsConfigMap(revised *corev1.ConfigMap, crName string, newURLs []corev1.EnvVar, deleteCR bool) {
 	for _, url := range newURLs {
+		if revised.Data[url.Name] == "" {
+			continue
+		}
+
 		currentURLs := strings.Split(revised.Data[url.Name], ",")
-		sort.Strings(currentURLs)
-		for _, curr := range currentURLs {
-			//scale DOWN
-			if strings.Contains(curr, crName) && !strings.Contains(url.Value, curr) && !deleteCR {
-				revised.Data[url.Name] = strings.ReplaceAll(revised.Data[url.Name], curr, "")
-			} else if strings.Contains(curr, crName) && deleteCR {
-				revised.Data[url.Name] = strings.ReplaceAll(revised.Data[url.Name], url.Value, "")
-			}
-			//if deleting "SPLUNK_MULTISITE_MASTER" delete "SPLUNK_SITE"
-			if url.Name == "SPLUNK_SITE" && deleteCR {
-				delete(revised.Data, "SPLUNK_SITE")
-			}
-			if strings.HasPrefix(revised.Data[url.Name], ",") {
-				str := revised.Data[url.Name]
-				revised.Data[url.Name] = strings.TrimPrefix(str, ",")
+		urlsToRemove := strings.Split(url.Value, ",")
+
+		// Create map of URLs to remove
+		removeMap := make(map[string]bool)
+		for _, removeURL := range urlsToRemove {
+			if strings.TrimSpace(removeURL) != "" {
+				removeMap[strings.TrimSpace(removeURL)] = true
 			}
-			if strings.HasSuffix(revised.Data[url.Name], ",") {
-				str := revised.Data[url.Name]
-				revised.Data[url.Name] = strings.TrimSuffix(str, ",")
+		}
+
+		// Filter out URLs that should be removed
+		var remainingURLs []string
+		for _, currentURL := range currentURLs {
+			trimmedURL := strings.TrimSpace(currentURL)
+			if trimmedURL == "" {
+				continue
 			}
-			if strings.Contains(revised.Data[url.Name], ",,") {
-				str := revised.Data[url.Name]
-				revised.Data[url.Name] = strings.ReplaceAll(str, ",,", ",")
+
+			shouldRemove := false
+			if deleteCR && strings.Contains(trimmedURL, crName) {
+				shouldRemove = true
+			} else if !deleteCR && removeMap[trimmedURL] && strings.Contains(trimmedURL, crName) {
+				shouldRemove = true
 			}
-			if revised.Data[url.Name] == "" {
-				delete(revised.Data, url.Name)
+
+			if !shouldRemove {
+				remainingURLs = append(remainingURLs, trimmedURL)
 			}
 		}
+
+		// Update or delete the key
+		if len(remainingURLs) == 0 {
+			delete(revised.Data, url.Name)
+		} else {
+			sort.Strings(remainingURLs) // Ensure consistent ordering
+			revised.Data[url.Name] = strings.Join(remainingURLs, ",")
+		}
+
+		// Handle SPLUNK_SITE cleanup
+		if url.Name == "SPLUNK_SITE" && deleteCR {
+			delete(revised.Data, "SPLUNK_SITE")
+		}
 	}
 }
 

pkg/splunk/enterprise/monitoringconsole_test.go

@@ -20,6 +20,7 @@ import (
 	"os"
 	"path/filepath"
 	"runtime/debug"
+	"strings"
 	"testing"
 	"time"
 
@@ -1272,3 +1273,66 @@ func TestChangeMonitoringConsoleAnnotations(t *testing.T) {
 		t.Errorf("changeMonitoringConsoleAnnotations should have set the checkUpdateImage annotation field to the current image")
 	}
 }
+
+func TestAddURLsConfigMap_StringLengthComparisonBug(t *testing.T) {
+	// Test the flawed length-based comparison logic
+	revised := &corev1.ConfigMap{
+		Data: map[string]string{
+			"SPLUNK_STANDALONE_URL": "http://pod-1:8089,http://pod-2:8089",
+		},
+	}
+
+	newURLs := []corev1.EnvVar{
+		{Name: "SPLUNK_STANDALONE_URL", Value: "http://pod-3:8089"}, // Different length but should be added
+	}
+
+	AddURLsConfigMap(revised, "test", newURLs)
+
+	// Current implementation incorrectly compares lengths and may not add the URL
+	result := revised.Data["SPLUNK_STANDALONE_URL"]
+	if !strings.Contains(result, "http://pod-3:8089") {
+		t.Errorf("URL should have been added but wasn't due to length comparison bug")
+	}
+}
+
+func TestDeleteURLsConfigMap_MalformedCommaCleanup(t *testing.T) {
+	// Test the multiple string replacement issue that creates malformed lists
+	revised := &corev1.ConfigMap{
+		Data: map[string]string{
+			"SPLUNK_STANDALONE_URL": "http://test-pod-1:8089,http://test-pod-2:8089,http://other-pod:8089",
+		},
+	}
+
+	newURLs := []corev1.EnvVar{
+		{Name: "SPLUNK_STANDALONE_URL", Value: "http://test-pod-1:8089"},
+	}
+
+	DeleteURLsConfigMap(revised, "test", newURLs, false)
+
+	result := revised.Data["SPLUNK_STANDALONE_URL"]
+	// Check for malformed comma patterns that the current implementation creates
+	if strings.Contains(result, ",,") || strings.HasPrefix(result, ",") || strings.HasSuffix(result, ",") {
+		t.Errorf("Current implementation creates malformed comma-separated list: %s", result)
+	}
+}
+
+func TestDeleteURLsConfigMap_PartialStringMatching(t *testing.T) {
+	// Test the strings.Contains() bug that matches partial URLs incorrectly
+	revised := &corev1.ConfigMap{
+		Data: map[string]string{
+			"SPLUNK_STANDALONE_URL": "http://test-pod:8089,http://test-pod-extended:8089",
+		},
+	}
+
+	newURLs := []corev1.EnvVar{
+		{Name: "SPLUNK_STANDALONE_URL", Value: "http://test-pod:8089"},
+	}
+
+	DeleteURLsConfigMap(revised, "test", newURLs, false)
+
+	result := revised.Data["SPLUNK_STANDALONE_URL"]
+	// Current implementation might incorrectly remove "test-pod-extended" due to partial matching
+	if !strings.Contains(result, "http://test-pod-extended:8089") {
+		t.Errorf("Partial string matching incorrectly removed extended pod URL")
+	}
+}