From 8caffc711b54f42a60d3e630a4d6fbb045775870 Mon Sep 17 00:00:00 2001 From: Rizwana Naaz Date: Mon, 13 Jan 2025 22:04:42 +0530 Subject: [PATCH] Fix issue in extraCommandArgs field (#1541) * Fix issue in extraArgsCommand field Signed-off-by: Rizwana777 * Override the default values Signed-off-by: Rizwana777 * Add appendUnique args logic to all the cases Signed-off-by: Rizwana777 * Resolve merge conflicts Signed-off-by: Rizwana777 * Add test for appendUniqueArgs and added logic for ExtraRepoCommandArgs Signed-off-by: Rizwana777 --------- Signed-off-by: Rizwana777 --- controllers/argocd/applicationset.go | 7 +-- controllers/argocd/applicationset_test.go | 37 +++++++++++++- controllers/argocd/deployment.go | 27 +++------- controllers/argocd/deployment_test.go | 59 ++++++++++++++++++++- controllers/argocd/util.go | 59 +++++++++++++++++++-- controllers/argocd/util_test.go | 62 ++++++++++++++++++++++- 6 files changed, 217 insertions(+), 34 deletions(-) diff --git a/controllers/argocd/applicationset.go b/controllers/argocd/applicationset.go index 50a8ab8ce..674d2a618 100644 --- 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 index c16555098..734670188 100644 --- 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 index ebc84ec8a..71f031f3f 100644 --- 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 index 9e326f8ec..aa117cd99 100644 --- 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 index f7f0184dd..1a1c5d1b6 100644 --- 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 index c676764b9..4502fb2e6 100644 --- 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) + } + }) + } +}