From 13df52d00ab23812a39008ebf31df210fb0f1fb3 Mon Sep 17 00:00:00 2001 From: "igor.grzankowski" <@splunk.com> Date: Thu, 28 Aug 2025 14:55:06 +0200 Subject: [PATCH] Fix DeleteURLsConfigMap --- pkg/splunk/enterprise/monitoringconsole.go | 110 ++++++++++-------- .../enterprise/monitoringconsole_test.go | 64 ++++++++++ 2 files changed, 127 insertions(+), 47 deletions(-) diff --git a/pkg/splunk/enterprise/monitoringconsole.go b/pkg/splunk/enterprise/monitoringconsole.go index e56065d7d..36052125e 100644 --- a/pkg/splunk/enterprise/monitoringconsole.go +++ b/pkg/splunk/enterprise/monitoringconsole.go @@ -304,35 +304,33 @@ 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, ",") } } } @@ -340,35 +338,53 @@ func AddURLsConfigMap(revised *corev1.ConfigMap, crName string, newURLs []corev1 // 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") + } } } diff --git a/pkg/splunk/enterprise/monitoringconsole_test.go b/pkg/splunk/enterprise/monitoringconsole_test.go index 9dedae498..ec7a4d49a 100644 --- a/pkg/splunk/enterprise/monitoringconsole_test.go +++ b/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") + } +}