Fix issue in extraCommandArgs field (#1541)

* Fix issue in extraArgsCommand field Signed-off-by: Rizwana777 <[email protected]> * Override the default values Signed-off-by: Rizwana777 <[email protected]> * Add appendUnique args logic to all the cases Signed-off-by: Rizwana777 <[email protected]> * Resolve merge conflicts Signed-off-by: Rizwana777 <[email protected]> * Add test for appendUniqueArgs and added logic for ExtraRepoCommandArgs Signed-off-by: Rizwana777 <[email protected]> --------- Signed-off-by: Rizwana777 <[email protected]>
argoproj-labs · Jan 13, 2025 · 8caffc7 · 8caffc7
1 parent 361478b
commit 8caffc7
Show file tree

Hide file tree

Showing 6 changed files with 217 additions and 34 deletions.
diff --git a/controllers/argocd/applicationset.go b/controllers/argocd/applicationset.go
@@ -94,12 +94,7 @@ func (r *ReconcileArgoCD) getArgoApplicationSetCommand(cr *argoproj.ArgoCD) []st
 
 	// ApplicationSet command arguments provided by the user
 	extraArgs := cr.Spec.ApplicationSet.ExtraCommandArgs
-	err = isMergable(extraArgs, cmd)
-	if err != nil {
-		return cmd
-	}
-
-	cmd = append(cmd, extraArgs...)
+	cmd = appendUniqueArgs(cmd, extraArgs)
 
 	return cmd
 }

diff --git a/controllers/argocd/applicationset_test.go b/controllers/argocd/applicationset_test.go
@@ -1032,6 +1032,15 @@ func TestArgoCDApplicationSetCommand(t *testing.T) {
 		"bar",
 	}
 
+	wantCmd := []string{
+		"entrypoint.sh",
+		"argocd-applicationset-controller",
+		"--argocd-repo-server",
+		"foo.scv.cluster.local:6379",
+		"--loglevel",
+		"info",
+	}
+
 	deployment := &appsv1.Deployment{}
 	assert.NoError(t, r.reconcileApplicationSetController(a))
 
@@ -1082,7 +1091,7 @@ func TestArgoCDApplicationSetCommand(t *testing.T) {
 		},
 		deployment))
 
-	assert.Equal(t, baseCommand, deployment.Spec.Template.Spec.Containers[0].Command)
+	assert.Equal(t, wantCmd, deployment.Spec.Template.Spec.Containers[0].Command)
 
 	// Remove all the command arguments that were added.
 	a.Spec.ApplicationSet.ExtraCommandArgs = []string{}
@@ -1097,6 +1106,32 @@ func TestArgoCDApplicationSetCommand(t *testing.T) {
 		deployment))
 
 	assert.Equal(t, baseCommand, deployment.Spec.Template.Spec.Containers[0].Command)
+
+	// When ExtraCommandArgs contains a non-duplicate argument along with a duplicate
+	a.Spec.ApplicationSet.ExtraCommandArgs = []string{
+		"--foo",
+		"bar",
+		"--ping",
+		"pong",
+		"test",
+		"--newarg", // Non-duplicate argument
+		"newvalue",
+		"--newarg", // Duplicate argument passing at once
+		"newvalue",
+	}
+
+	assert.NoError(t, r.reconcileApplicationSetController(a))
+	assert.NoError(t, r.Client.Get(
+		context.TODO(),
+		types.NamespacedName{
+			Name:      "argocd-applicationset-controller",
+			Namespace: a.Namespace,
+		},
+		deployment))
+
+	// Non-duplicate argument "--newarg" should be added, duplicate "--newarg" which is added twice is ignored
+	cmd = append(cmd, "--newarg", "newvalue")
+	assert.Equal(t, cmd, deployment.Spec.Template.Spec.Containers[0].Command)
 }
 
 func TestArgoCDApplicationSetEnv(t *testing.T) {

diff --git a/controllers/argocd/deployment.go b/controllers/argocd/deployment.go
@@ -256,12 +256,8 @@ func getArgoRepoCommand(cr *argoproj.ArgoCD, useTLSForRedis bool) []string {
 	// *** NOTE ***
 	// Do Not add any new default command line arguments below this.
 	extraArgs := cr.Spec.Repo.ExtraRepoCommandArgs
-	err := isMergable(extraArgs, cmd)
-	if err != nil {
-		return cmd
-	}
+	cmd = appendUniqueArgs(cmd, extraArgs)
 
-	cmd = append(cmd, extraArgs...)
 	return cmd
 }
 
@@ -288,11 +284,9 @@ func getArgoServerCommand(cr *argoproj.ArgoCD, useTLSForRedis bool) []string {
 		cmd = append(cmd, "--repo-server-strict-tls")
 	}
 
-	cmd = append(cmd, "--staticassets")
-	cmd = append(cmd, "/shared/app")
+	cmd = append(cmd, "--staticassets", "/shared/app")
 
-	cmd = append(cmd, "--dex-server")
-	cmd = append(cmd, getDexServerAddress(cr))
+	cmd = append(cmd, "--dex-server", getDexServerAddress(cr))
 
 	if cr.Spec.Repo.IsEnabled() {
 		cmd = append(cmd, "--repo-server", getRepoServerAddress(cr))
@@ -315,22 +309,17 @@ func getArgoServerCommand(cr *argoproj.ArgoCD, useTLSForRedis bool) []string {
 		}
 	}
 
-	cmd = append(cmd, "--loglevel")
-	cmd = append(cmd, getLogLevel(cr.Spec.Server.LogLevel))
-
-	cmd = append(cmd, "--logformat")
-	cmd = append(cmd, getLogFormat(cr.Spec.Server.LogFormat))
+	cmd = append(cmd, "--loglevel", getLogLevel(cr.Spec.Server.LogLevel))
+	cmd = append(cmd, "--logformat", getLogFormat(cr.Spec.Server.LogFormat))
 
+	// Merge extraArgs while ignoring duplicates
 	extraArgs := cr.Spec.Server.ExtraCommandArgs
-	err := isMergable(extraArgs, cmd)
-	if err != nil {
-		return cmd
-	}
+	cmd = appendUniqueArgs(cmd, extraArgs)
+
 	if len(cr.Spec.SourceNamespaces) > 0 {
 		cmd = append(cmd, "--application-namespaces", fmt.Sprint(strings.Join(cr.Spec.SourceNamespaces, ",")))
 	}
 
-	cmd = append(cmd, extraArgs...)
 	return cmd
 }
 

diff --git a/controllers/argocd/deployment_test.go b/controllers/argocd/deployment_test.go
@@ -1307,6 +1307,22 @@ func TestArgoCDServerDeploymentCommand(t *testing.T) {
 		"foo.scv.cluster.local:6379",
 	}
 
+	wantCmd := []string{
+		"argocd-server",
+		"--staticassets",
+		"/shared/app",
+		"--dex-server",
+		"https://argocd-dex-server.argocd.svc.cluster.local:5556",
+		"--repo-server",
+		"argocd-repo-server.argocd.svc.cluster.local:8081",
+		"--redis",
+		"foo.scv.cluster.local:6379",
+		"--loglevel",
+		"info",
+		"--logformat",
+		"text",
+	}
+
 	assert.NoError(t, r.reconcileServerDeployment(a, false))
 	assert.NoError(t, r.Client.Get(
 		context.TODO(),
@@ -1316,7 +1332,35 @@ func TestArgoCDServerDeploymentCommand(t *testing.T) {
 		},
 		deployment))
 
-	assert.Equal(t, baseCommand, deployment.Spec.Template.Spec.Containers[0].Command)
+	assert.Equal(t, wantCmd, deployment.Spec.Template.Spec.Containers[0].Command)
+
+	// When ExtraCommandArgs contains a non-duplicate argument along with a duplicate
+	a.Spec.Server.ExtraCommandArgs = []string{
+		"--rootpath",
+		"/argocd",
+		"--foo",
+		"bar",
+		"test",
+		"--logformat", // Duplicate flag and value
+		"text",
+		"--newarg", // Non-duplicate argument
+		"newvalue",
+		"--newarg", // Duplicate argument passing at once
+		"newvalue",
+	}
+
+	assert.NoError(t, r.reconcileServerDeployment(a, false))
+	assert.NoError(t, r.Client.Get(
+		context.TODO(),
+		types.NamespacedName{
+			Name:      "argocd-server",
+			Namespace: a.Namespace,
+		},
+		deployment))
+
+	// Non-duplicate argument "--newarg" should be added, duplicate "--newarg" which is added twice is ignored
+	cmd = append(cmd, "--newarg", "newvalue")
+	assert.Equal(t, cmd, deployment.Spec.Template.Spec.Containers[0].Command)
 
 	// Remove all the command arguments that were added.
 	a.Spec.Server.ExtraCommandArgs = []string{}
@@ -2313,6 +2357,17 @@ func TestArgoCDRepoServerDeploymentCommand(t *testing.T) {
 		"foo.scv.cluster.local:6379",
 	}
 
+	wantCmd := []string{
+		"uid_entrypoint.sh",
+		"argocd-repo-server",
+		"--redis",
+		"foo.scv.cluster.local:6379",
+		"--loglevel",
+		"info",
+		"--logformat",
+		"text",
+	}
+
 	assert.NoError(t, r.reconcileRepoDeployment(a, false))
 	assert.NoError(t, r.Client.Get(
 		context.TODO(),
@@ -2322,7 +2377,7 @@ func TestArgoCDRepoServerDeploymentCommand(t *testing.T) {
 		},
 		deployment))
 
-	assert.Equal(t, baseCommand, deployment.Spec.Template.Spec.Containers[0].Command)
+	assert.Equal(t, wantCmd, deployment.Spec.Template.Spec.Containers[0].Command)
 
 	// Remove all the command arguments that were added.
 	a.Spec.Repo.ExtraRepoCommandArgs = []string{}

diff --git a/controllers/argocd/util.go b/controllers/argocd/util.go
@@ -177,11 +177,7 @@ func getArgoApplicationControllerCommand(cr *argoproj.ArgoCD, useTLSForRedis boo
 
 	// check if extra args are present
 	extraArgs := cr.Spec.Controller.ExtraCommandArgs
-	err := isMergable(extraArgs, cmd)
-	if err != nil {
-		return cmd
-	}
-	cmd = append(cmd, extraArgs...)
+	cmd = appendUniqueArgs(cmd, extraArgs)
 
 	return cmd
 }
@@ -1803,3 +1799,56 @@ func createCondition(message string) metav1.Condition {
 		Status:  metav1.ConditionFalse,
 	}
 }
+
+// appendUniqueArgs appends extraArgs to cmd while ignoring any duplicate flags.
+func appendUniqueArgs(cmd []string, extraArgs []string) []string {
+	// Parse cmd into a map to track flags and their indices
+	existingArgs := make(map[string]int) // Map to track index of each flag in cmd
+	for i := 0; i < len(cmd); i++ {
+		arg := cmd[i]
+		if strings.HasPrefix(arg, "--") {
+			// Check if the next item is a value (not another flag)
+			if i+1 < len(cmd) && !strings.HasPrefix(cmd[i+1], "--") {
+				existingArgs[arg] = i
+				i++ // Skip the value
+			} else {
+				existingArgs[arg] = i
+			}
+		}
+	}
+
+	// Iterate over extraArgs to append or override existing flags
+	for i := 0; i < len(extraArgs); i++ {
+		arg := extraArgs[i]
+		if strings.HasPrefix(arg, "--") {
+			if index, exists := existingArgs[arg]; exists {
+				// If the flag exists, check if we need to override its value
+				if i+1 < len(extraArgs) && !strings.HasPrefix(extraArgs[i+1], "--") {
+					// If the flag has a value in extraArgs,update it in cmd
+					if index+1 < len(cmd) && !strings.HasPrefix(cmd[index+1], "--") {
+						cmd[index+1] = extraArgs[i+1] // Override the existing value
+					} else {
+						// Insert the new value if it didn't previously have one
+						cmd = append(cmd[:index+1], append([]string{extraArgs[i+1]}, cmd[index+1:]...)...)
+					}
+					i++ // Skip the value in extraArgs
+				}
+			} else {
+				// Append the new flag and its value if not present
+				cmd = append(cmd, arg)
+				if i+1 < len(extraArgs) && !strings.HasPrefix(extraArgs[i+1], "--") {
+					cmd = append(cmd, extraArgs[i+1])
+					existingArgs[arg] = len(cmd) - 2 // Update index tracking
+					i++                              // Skip the value
+				} else {
+					existingArgs[arg] = len(cmd) - 1 // Update index tracking
+				}
+			}
+		} else {
+			// Append non-flag arguments directly
+			cmd = append(cmd, arg)
+		}
+	}
+
+	return cmd
+}
diff --git a/controllers/argocd/util_test.go b/controllers/argocd/util_test.go
@@ -582,13 +582,18 @@ func TestGetArgoApplicationControllerCommand(t *testing.T) {
 		{
 			"overriding default argument using extraCommandArgs",
 			[]argoCDOpt{extraCommandArgs([]string{"--operation-processors", "15"})},
-			defaultResult,
+			operationProcesorsChangedResult("15"),
 		},
 		{
 			"configured empty extraCommandArgs",
 			[]argoCDOpt{extraCommandArgs([]string{})},
 			defaultResult,
 		},
+		{
+			"configured extraCommandArgs with duplicate values",
+			[]argoCDOpt{extraCommandArgs([]string{"--status-processors", "20"})},
+			defaultResult,
+		},
 	}
 
 	for _, tt := range cmdTests {
@@ -1326,3 +1331,58 @@ func TestInsertOrUpdateConditionsInSlice_add_another_condition(t *testing.T) {
 	assert.Equal(t, conditions[1].Reason, newCondition.Reason)
 	assert.Equal(t, conditions[1].Message, newCondition.Message)
 }
+
+func TestAppendUniqueArgs(t *testing.T) {
+	tests := []struct {
+		name      string
+		cmd       []string
+		extraArgs []string
+		want      []string
+	}{
+		{
+			name:      "append new flags and values",
+			cmd:       []string{"--foo", "bar"},
+			extraArgs: []string{"--baz", "qux"},
+			want:      []string{"--foo", "bar", "--baz", "qux"},
+		},
+		{
+			name:      "override existing flag value",
+			cmd:       []string{"--foo", "bar"},
+			extraArgs: []string{"--foo", "baz"},
+			want:      []string{"--foo", "baz"},
+		},
+		{
+			name:      "add flag without value",
+			cmd:       []string{"--foo", "bar"},
+			extraArgs: []string{"--baz"},
+			want:      []string{"--foo", "bar", "--baz"},
+		},
+		{
+			name:      "override flag with no value to have a value",
+			cmd:       []string{"--foo"},
+			extraArgs: []string{"--foo", "baz"},
+			want:      []string{"--foo", "baz"},
+		},
+		{
+			name:      "append non-flag arguments",
+			cmd:       []string{"--foo", "bar"},
+			extraArgs: []string{"extra", "--baz", "qux"},
+			want:      []string{"--foo", "bar", "extra", "--baz", "qux"},
+		},
+		{
+			name:      "ignore duplicate non-flag arguments",
+			cmd:       []string{"arg1", "arg2"},
+			extraArgs: []string{"arg2", "arg3"},
+			want:      []string{"arg1", "arg2", "arg2", "arg3"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := appendUniqueArgs(tt.cmd, tt.extraArgs)
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("appendUniqueArgs() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}