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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
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
70 changes: 56 additions & 14 deletions controllers/argocd/deployment.go
Original file line number Diff line number Diff line change
Expand Up @@ -288,11 +288,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 +313,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 All @@ -350,6 +343,55 @@ func isMergable(extraArgs []string, cmd []string) error {
return nil
}

// appendUniqueArgs appends extraArgs to cmd while ignoring any duplicate flags.
func appendUniqueArgs(cmd []string, extraArgs []string) []string {
// Parse cmd into a map to track both flags and their associated values
existingArgs := make(map[string]string)
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] = cmd[i+1] // Store flag-value pair
i++ // Skip the value
} else {
existingArgs[arg] = "" // Store flag without value
}
}
}

// Iterate over extraArgs and append only if the flag and value do not already exist in cmd
for i := 0; i < len(extraArgs); i++ {
arg := extraArgs[i]
if strings.HasPrefix(arg, "--") {
// If this flag already exists in cmd, skip it
if _, exists := existingArgs[arg]; exists {
if i+1 < len(extraArgs) && !strings.HasPrefix(extraArgs[i+1], "--") {
i++ // Skip the associated value
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if an existing flag is added with a new value, will that also be skipped?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, it is skipped

Copy link
Collaborator

@saumeya saumeya Sep 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is that the expected behavior? what if the user wants to change the current flag value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

based on the existing unit test https://github.com/argoproj-labs/argocd-operator/blob/master/controllers/argocd/deployment_test.go#L1304 which is updating existing cmd i.e --redis command, I think this is expected behaviour, I might be wrong and open for suggestions

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, maybe we can confirm with others as well. Otherwise looks good.

Copy link
Collaborator

@svghadi svghadi Oct 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think users should be allowed to override default flags. There may be cases where tuning of default params could be useful.

}
continue
}

// Append the flag to cmd
cmd = append(cmd, arg)

// Check if this flag has an associated value
if i+1 < len(extraArgs) && !strings.HasPrefix(extraArgs[i+1], "--") {
cmd = append(cmd, extraArgs[i+1])
existingArgs[arg] = extraArgs[i+1] // Track flag-value pair
i++ // Skip the value
} else {
existingArgs[arg] = "" // Track flag without a value
}
} else {
// If it's not a flag, append it directly (non-flag argument)
cmd = append(cmd, arg)
}
}

return cmd
}

// getDexServerAddress will return the Dex server address.
func getDexServerAddress(cr *argoproj.ArgoCD) string {
return fmt.Sprintf("https://%s", fqdnServiceRef("dex-server", common.ArgoCDDefaultDexHTTPPort, cr))
Expand Down
28 changes: 28 additions & 0 deletions controllers/argocd/deployment_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1317,6 +1317,34 @@ func TestArgoCDServerDeploymentCommand(t *testing.T) {

assert.Equal(t, baseCommand, 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
Loading