Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix issue in extraCommandArgs field #1541

Merged
merged 5 commits into from
Jan 13, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 1 addition & 6 deletions controllers/argocd/applicationset.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down
37 changes: 36 additions & 1 deletion controllers/argocd/applicationset_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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))

Expand Down Expand Up @@ -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{}
Expand All @@ -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) {
Expand Down
27 changes: 8 additions & 19 deletions controllers/argocd/deployment.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand All @@ -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))
Expand All @@ -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
}

Expand Down
59 changes: 57 additions & 2 deletions controllers/argocd/deployment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand All @@ -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{}
Expand Down Expand Up @@ -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(),
Expand All @@ -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{}
Expand Down
59 changes: 54 additions & 5 deletions controllers/argocd/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
62 changes: 61 additions & 1 deletion controllers/argocd/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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)
}
})
}
}
Loading