feat(cli): Add Plugin Support to the Argo CD CLI

[nitishfy]

Ref: #19624
Checklist:

• Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
• The title of the PR states what changed and the related issues number (used for the release note).
• The title of the PR conforms to the Toolchain Guide
• I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
• I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
• Does this PR require documentation updates?
• I've updated documentation as required by this PR.
• I have signed off all my commits as required by DCO
• I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• My build is green (troubleshooting builds).
• My new feature complies with the feature status guidelines.
• I have added a brief description of why this PR is necessary and/or what this PR solves.
• Optional. My organization is added to USERS.md.
• Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

[bunnyshell]

🔴 Preview Environment stopped on Bunnyshell

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

• 🔵 /bns:start to start the environment
• 🚀 /bns:deploy to redeploy the environment
• ❌ /bns:delete to remove the environment

[bunnyshell]

✅ Preview Environment created on Bunnyshell but will not be auto-deployed

See: Environment Details

Available commands (reply to this comment):

• 🚀 /bns:deploy to deploy the environment

[codecov]

Codecov Report

Attention: Patch coverage is 73.33333% with 32 lines in your changes missing coverage. Please review.

Project coverage is 59.73%. Comparing base (545b267) to head (8e62673).
Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
cmd/main.go	27.58%	19 Missing and 2 partials ⚠️
cmd/argocd/commands/plugin.go	87.91%	9 Missing and 2 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #20074      +/-   ##
==========================================
+ Coverage   55.89%   59.73%   +3.84%     
==========================================
  Files         343      344       +1     
  Lines       57327    57442     +115     
==========================================
+ Hits        32044    34315    +2271     
+ Misses      22643    20367    -2276     
- Partials     2640     2760     +120     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[leoluz]

Please consider adding a new documentation page explaining how to write plugins and how to install and consume them.

[nitishfy]

plugin_test.go is kept in separate package because keeping it in the util will create circular dependency.

Update: Resolved

[leoluz]

@nitishfy Great progress. I think this PR needs to be reviewed by someone from the security sig as I see some potential risks in the code. I added comments to the security issues that I identified at first but a more security driven review would be important in this case.
cc @jannfis @crenshaw-dev

[nitishfy]

We are not using the cmd.Find() anymore.

[leoluz]

Please check my comments

[nitishfy]

PTAL.

[nitishfy]

Also, I think I'll have to restructure the entire tests for plugin_test.go once the change is accepted.

[leoluz]

Great progress. Tks @nitishfy!
There are a few more open items to be addressed in this PR before we merge.
Please take a look.

[agaudreault]

do not log error before returning it. Let the caller handle the error when you are returning it.

return fmt.Errorf("Error: %v\n", pluginErr)

[blakepettersson]

@nitishfy my personal opinion is still that we should still have some E2E tests for this (i.e. running tests that exercises the actual CLI).

[agaudreault]

Why setting debug logs? it should respect the cmdutil.LogLevel provided in the command arguments.

[nitishfy]

sorry the previous comment got deleted...

the log level needs to be setup manually here since the initConfig() set by the cobra.OnInitialize() was never executed because cmd.Execute() gave us a non-nil error.

[nitishfy]

@nitishfy my personal opinion is still that we should still have some E2E tests for this (i.e. running tests that exercises the actual CLI).

If you look at the unit tests that have been written, we're not testing a specific function actually. All it does is callout the similar logic that would go inside an E2E test. I just believe that's redundant testing for the similar thing again.

[agaudreault]

The command execution error should probably receive the command object in parameter. This way, the handler can use functions such as cmd.UsageString() and cmd.ErrPrefix() to handle the behavior disabled by SilenceUsage and SilenceErrors.

[nitishfy]

we don't need that because the behaviour disabled by SilenceUsage and SilenceErrors is getting handled here:

if isArgocdCLI && strings.Contains(err.Error(), "unknown command") {
		log.Debug("command does not exist, looking for a plugin...")
		pluginPath, pluginErr := h.handlePluginCommand(args[1:])
		// IMP: If a plugin doesn't exist, the returned path will be empty along with nil error
		// This means the command is neither a normal Argo CD Command nor a plugin.
		if pluginErr != nil {
			// If plugin handling fails, report the plugin error and exit
			return fmt.Errorf("Error: %w\n", pluginErr)
		} else if pluginPath == "" {
			log.Errorf("Error: %v\nRun 'argocd --help' for usage.\n", err)
			return err
		}
	} else {
		// If it's any other error (not an unknown command), report it directly and exit
		log.Errorf("Error: %v\n", err)
		return err
	}

I believe once the cobra PR to check for unknown command gets merged, we can refactor this codebase to an extent to make it better.

[blakepettersson]

@nitishfy

I just believe that's redundant testing for the similar thing again.

Well, I disagree with that. We would at the very least still need to check that the CLI parses flags correctly from the command line.

[nitishfy]

All the E2E tests have been added now.

[blakepettersson]

Minor nits, otherwise LGTM

[nitishfy]

PTAL.

[nitishfy]

sorry the previous comment got deleted...

the log level needs to be setup manually here since the initConfig() set by the cobra.OnInitialize() was never executed because cmd.Execute() gave us a non-nil error.

[nitishfy]

we don't need that because the behaviour disabled by SilenceUsage and SilenceErrors is getting handled here:

if isArgocdCLI && strings.Contains(err.Error(), "unknown command") {
		log.Debug("command does not exist, looking for a plugin...")
		pluginPath, pluginErr := h.handlePluginCommand(args[1:])
		// IMP: If a plugin doesn't exist, the returned path will be empty along with nil error
		// This means the command is neither a normal Argo CD Command nor a plugin.
		if pluginErr != nil {
			// If plugin handling fails, report the plugin error and exit
			return fmt.Errorf("Error: %w\n", pluginErr)
		} else if pluginPath == "" {
			log.Errorf("Error: %v\nRun 'argocd --help' for usage.\n", err)
			return err
		}
	} else {
		// If it's any other error (not an unknown command), report it directly and exit
		log.Errorf("Error: %v\n", err)
		return err
	}

I believe once the cobra PR to check for unknown command gets merged, we can refactor this codebase to an extent to make it better.

[nitishfy]

cc @agaudreault

[nitishfy]

Update: There's a little issue related to flag parsing that needs to be worked upon. Rn, all the global level flags won't be parsed when you are running your plugin. This is happening because cobra internally doesn't parse the flags when a command is not found. This also means that if a user has set its own flags in a binary that matches with Argo CD global flags, the default value of the Argo CD global flags will be used no matter what (why? because flag parsing never happens by cobra for an unknown command). For normal Argo CD commands, the flag parsing and everything else remains same as this is how it used to be.

What should be a better approach to handle this? Should we add this in a limitation section and tell users about not to use duplicate flags as argocd global flags? Do we even want a user to set it's own flags that overrides argocd global flag values?

> dist/argocd non-existent # default logging format is json for v3.0                   
{"level":"debug","msg":"command does not exist, looking for a plugin...","time":"2025-03-23T15:43:03+05:30"}
{"level":"warning","msg":"error looking for plugin 'argocd-non-existent': exec: \"argocd-non-existent\": executable file not found in $PATH","time":"2025-03-23T15:43:03+05:30"}
{"level":"error","msg":"Error: unknown command \"non-existent\" for \"argocd\"\nRun 'argocd --help' for usage.\n","time":"2025-03-23T15:43:03+05:30"}
              
> dist/argocd non-existent --logformat=text # logformat flag is never parsed
{"level":"debug","msg":"command does not exist, looking for a plugin...","time":"2025-03-23T15:43:06+05:30"}
{"level":"warning","msg":"error looking for plugin 'argocd-non-existent': exec: \"argocd-non-existent\": executable file not found in $PATH","time":"2025-03-23T15:43:06+05:30"}
{"level":"error","msg":"Error: unknown command \"non-existent\" for \"argocd\"\nRun 'argocd --help' for usage.\n","time":"2025-03-23T15:43:06+05:30"}

> dist/argocd login localhost:8080 --logformat=text # this works because login is an existing argocd command
FATA[0000] dial tcp 127.0.0.1:8080: connect: connection refused 

[leoluz]

What should be a better approach to handle this? Should we add this in a limitation section and tell users about not to use duplicate flags as argocd global flags? Do we even want a user to set it's own flags that overrides argocd global flag values?

@nitishfy In my view, we need to guarantee to plugin writers 2 capabilities:

1. The plugin doesn't have to authenticate with Argo CD API. Plugin authors can directly invoke the Argo CD API and the current context will be used.
2. While some global flags don't make sense to be used with plugins, some others do: --argocd-context, --auth-token, --logformat, --loglevel are some (but not the only ones) that makes sense to be available when executing plugins. However I don't think that it is the plugin responsibility to handle those flags. We would need to pre-process the flags before invoking the plugin itself.

That being said, it seems that there is more work required in order to get the plugin feature ready. However I am not opposed if other maintainers are ok with moving forward clearly documenting the current limitations and marking the feature as alpha.

[blakepettersson]

@nitishfy @leoluz

Could we not have a dummy (no-op) command for the error path? We invoke the dummy command that will parse the flags, specifically the flags we would like to still keep (we can use the command.ParseFlags function for this purpose), and then we carry on with pluginHandler.HandleCommandExecutionError as before and pass in whatever flags we want there?