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

nitishfy · 2024-09-24T10:48:13Z

Ref: #19624
Checklist:

bunnyshell · 2024-09-24T10:48:17Z

🔴 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 · 2024-09-24T10:48:18Z

✅ 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 · 2024-09-24T11:43:17Z

Codecov Report

Attention: Patch coverage is 7.05882% with 79 lines in your changes missing coverage. Please review.

Project coverage is 55.80%. Comparing base (e28a05f) to head (92967cd).

Files with missing lines	Patch %	Lines
cmd/argocd/commands/root.go	0.00%	47 Missing ⚠️
cmd/util/plugin.go	9.09%	30 Missing ⚠️
cmd/main.go	60.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #20074      +/-   ##
==========================================
- Coverage   55.87%   55.80%   -0.08%     
==========================================
  Files         321      322       +1     
  Lines       44490    44573      +83     
==========================================
+ Hits        24860    24872      +12     
- Misses      17067    17129      +62     
- Partials     2563     2572       +9

☔ 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.

cmd/argocd/commands/root.go

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

leoluz · 2024-10-08T13:34:22Z

cmd/argocd/commands/plugin.go

+	}
+	if filepath.Base(name) == name {
+		lp, err := exec.LookPath(name)
+		if lp != "" && !shouldSkipOnLookPathErr(err) {


This may lead to security implications. For example: if someone has their PATH configured as PATH=.:/usr/bin, the first . makes the current directory available in the PATH which enables different types of exploits. Go 1.19+ will return ErrDot to notify us that the return value from LookPath() is a relative path. We shouldn't ignore this error. Instead, we should fail and return an error stating that the executable wasn't found. Also, we must state in the docs that the plugin executable must be in the path and should not be provided as relative path.

leoluz · 2024-10-08T13:36:15Z

cmd/argocd/commands/plugin_test.go

+		{
+			name: "test that a plugin does not execute over Cobra's __completeNoDesc command",
+			args: []string{"argocd", cobra.ShellCompNoDescRequestCmd, "de"},
+		},


Please add a test case to validate that relative paths are not allowed.

leoluz · 2024-10-08T14:04:33Z

cmd/argocd/commands/root.go

+		// If it finds a command, it continues without invoking the plugin.
+		// If it doesn't find the command (err is non-nil), it means foo isn't a built-in Argo CD command,
+		// so it might be a plugin like argocd-foo.
+		if _, _, err := cmd.Find(cmdPathPieces); err != nil {


This will introduce an overhead in all argocd commands. To avoid this problem, I would suggest a modification in the proposal to only execute plugin commands in a dedicated subcommand.
For example, instead of running plugins as:
argocd some-plugin arg1 arg2
declare a dedicated subcommand to invoke plugins as:
argocd plugin some-plugin arg1 arg2

cc @christianh814

Curious to know is it like a really big deal if all the argocd commands have to go through this? IMO, It doesn't look like a big burden but I'm definitely open to opinions.

Two main reasons why I am suggesting this change are:

Will greatly simplify the code. This is a big deal as it reduces the possible edge cases and increases maintainability.

Avoid unnecessary loop that all argocd commands would have to execute. Some companies are very sensitive by the duration argocd commands take to execute. While saving a few milliseconds in a single execution looks irrelevant, it can become a significant issue when executed millions of times.

leoluz · 2024-10-08T14:08:06Z

cmd/main.go

 	switch binaryName {
 	case "argocd", "argocd-linux-amd64", "argocd-darwin-amd64", "argocd-windows-amd64.exe":
-		command = cli.NewCommand()
+		command = cli.NewDefaultArgoCDCommandWithArgs(o)


This is a bit too invasive. ~~I think plugins should be implemented as a simple cobra subcommand~~. See my other comment about defining a dedicated subcommand for running plugins.

I'm wondering why do we have to go through a separate subcommand when we can simple invoke those plugins by putting them in the path?

Sorry if I didn't explain it clearly. My suggestion is not about having every single plugin to be implemented as subcommands. It is just to create a single subcommand called plugin so that can be dedicated to finding binaries in the PATH and executing (example: argocd plugin <some-plugin>). This is mainly to avoid the loop to decide if the invoked command is a plugin or not that all argocd commands would have to execute with the current implementation.

cmd/argocd/commands/root.go

blakepettersson · 2024-10-09T16:38:31Z

cmd/main.go

@@ -53,7 +58,7 @@ func main() {
 	case "argocd-k8s-auth":
 		command = k8sauth.NewCommand()
 	default:
-		command = cli.NewCommand()
+		command = cli.NewDefaultArgoCDCommandWithArgs(o)
 	}

 	if err := command.Execute(); err != nil {


As discussed offline between @leoluz, @nitishfy and @christianh814 we can use the error path here to fallback to the plugin lookup, which would alleviate the performance concerns stated above.

@blakepettersson @nitishfy In this case we just need to make sure that the error returned by Cobra is in fact a "command not found". Unfortunately Cobra doesn't return an specific error type for that case. This PR implements it but is still open by the time of this writing: spf13/cobra#2167

We shouldn't trigger the plugin lookup if the command execution returns anything different than "unknown command..". Pattern matching on the error message isn't the direction I would prefer to go :/

@leoluz damn, I thought something so obvious would have already been implemented 😞

@leoluz need your review now. I am yet to remove the dead code here, it is only getting used for the test, but I can fix that too. Let me know what do you think about the current approach.

@nitishfy I think this is much better. Unfortunately it still doesn't address the issue that I am referring here in this thread. I explained the error scenario a bit better in the comment below.

Signed-off-by: nitishfy <justnitish06@gmail.com>

nitishfy

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

leoluz

Please check my comments

leoluz · 2024-10-11T20:48:59Z

cmd/main.go

+		PluginHandler: cli.NewDefaultPluginHandler([]string{"argocd"}),
+		Arguments:     os.Args,
+	}
+
 	binaryName := filepath.Base(os.Args[0])
 	if val := os.Getenv(binaryNameEnv); val != "" {
 		binaryName = val
 	}

 	isCLI := false


For clarity I would suggest to rename this var to isArgocdCLI as plugins are also CLIs

leoluz · 2024-10-11T20:49:35Z

cmd/main.go

@@ -63,8 +71,19 @@ func main() {
 		isCLI = true
 	}
 	util.SetAutoMaxProcs(isCLI)
+	if isCLI {


I guess in this case you want the opposite right (if !isCli)?

leoluz · 2024-10-11T20:51:31Z

cmd/main.go


 	if err := command.Execute(); err != nil {
+		if isCLI {


not sure I understand what is the real intention with this isCLI variable but what we want is to execute the plugin handler only when this is not an argocd CLI subcommand.

leoluz · 2024-10-11T21:08:00Z

cmd/main.go

+		if isCLI {
+			if pluginErr := cli.HandlePluginCommand(o.PluginHandler, o.Arguments[1:], 1); pluginErr != nil {
+				fmt.Fprintf(os.Stderr, "Error: %v\n", pluginErr)
+				os.Exit(1)


I am afraid that this is going to swallow the error returned by the command.Execute() execution above.
Consider the scenario:

run argocd app list

argocd app list returns an error

cli.HandlePluginCommand is invoked and will always return error command not found

exit(1)

the main argocd app list error is never presented to the user

leoluz · 2024-10-11T21:10:28Z

cmd/main.go

@@ -53,7 +58,7 @@ func main() {
 	case "argocd-k8s-auth":
 		command = k8sauth.NewCommand()
 	default:
-		command = cli.NewCommand()
+		command = cli.NewDefaultArgoCDCommandWithArgs(o)
 	}

 	if err := command.Execute(); err != nil {


@nitishfy I think this is much better. Unfortunately it still doesn't address the issue that I am referring here in this thread. I explained the error scenario a bit better in the comment below.

leoluz requested changes Sep 24, 2024

View reviewed changes

cmd/argocd/commands/root.go Outdated Show resolved Hide resolved

cmd/argocd/commands/root.go Outdated Show resolved Hide resolved

leoluz requested changes Sep 24, 2024

View reviewed changes

cmd/argocd/commands/root.go Outdated Show resolved Hide resolved

leoluz self-assigned this Sep 24, 2024

nitishfy force-pushed the nitish/argocd-plugin-support branch 2 times, most recently from 92967cd to 4a260dd Compare October 1, 2024 11:40

nitishfy commented Oct 1, 2024

View reviewed changes

nitishfy force-pushed the nitish/argocd-plugin-support branch 2 times, most recently from 0e8f338 to c314e43 Compare October 8, 2024 11:35

nitishfy changed the title ~~WIP: Add Plugin Support to the Argo CD CLI~~ feat: Add Plugin Support to the Argo CD CLI Oct 8, 2024

nitishfy requested a review from leoluz October 8, 2024 11:57

leoluz requested changes Oct 8, 2024

View reviewed changes

cmd/argocd/commands/root.go Outdated Show resolved Hide resolved

blakepettersson reviewed Oct 9, 2024

View reviewed changes

blakepettersson changed the title ~~feat: Add Plugin Support to the Argo CD CLI~~ feat(cli): Add Plugin Support to the Argo CD CLI Oct 10, 2024

nitishfy added 13 commits October 11, 2024 14:56

resolve merge conflicts

c8695c7

Signed-off-by: nitishfy <justnitish06@gmail.com>

create plugin.go

64a50ef

Signed-off-by: nitishfy <justnitish06@gmail.com>

add comments

b57b372

Signed-off-by: nitishfy <justnitish06@gmail.com>

move the handler function to plugin.go

5a6132a

Signed-off-by: nitishfy <justnitish06@gmail.com>

add unit test

1756fde

Signed-off-by: nitishfy <justnitish06@gmail.com>

remove comments

e888b28

Signed-off-by: nitishfy <justnitish06@gmail.com>

add a plugin

69a73c6

Signed-off-by: nitishfy <justnitish06@gmail.com>

add more unit test

8a86031

Signed-off-by: nitishfy <justnitish06@gmail.com>

change command to argocd

dbeec2c

Signed-off-by: nitishfy <justnitish06@gmail.com>

resolve dependencies

d2ff992

Signed-off-by: nitishfy <justnitish06@gmail.com>

fix lint checks again

350ea3f

Signed-off-by: nitishfy <justnitish06@gmail.com>

support plugin for all OS

a54e954

Signed-off-by: nitishfy <justnitish06@gmail.com>

remove dead code

a2581b4

Signed-off-by: nitishfy <justnitish06@gmail.com>

remove cmd.path from code

fea85cd

Signed-off-by: nitishfy <justnitish06@gmail.com>

nitishfy force-pushed the nitish/argocd-plugin-support branch from 652887c to fea85cd Compare October 11, 2024 09:46

nitishfy commented Oct 11, 2024

View reviewed changes

nitishfy requested a review from leoluz October 11, 2024 11:58

leoluz requested changes Oct 11, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

nitishfy commented Sep 24, 2024 •

edited

Loading

bunnyshell bot commented Sep 24, 2024 •

edited

Loading

bunnyshell bot commented Sep 24, 2024

codecov bot commented Sep 24, 2024 •

edited

Loading

leoluz left a comment

nitishfy left a comment •

edited

Loading

leoluz left a comment

leoluz Oct 8, 2024 •

edited

Loading

leoluz Oct 8, 2024

leoluz Oct 8, 2024

nitishfy Oct 9, 2024

leoluz Oct 9, 2024

leoluz Oct 8, 2024 •

edited

Loading

nitishfy Oct 9, 2024

leoluz Oct 9, 2024 •

edited

Loading

blakepettersson Oct 9, 2024

leoluz Oct 9, 2024

blakepettersson Oct 9, 2024

nitishfy Oct 11, 2024

leoluz Oct 11, 2024

nitishfy left a comment

leoluz left a comment

leoluz Oct 11, 2024

leoluz Oct 11, 2024

leoluz Oct 11, 2024

leoluz Oct 11, 2024

leoluz Oct 11, 2024

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

Are you sure you want to change the base?

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

Conversation

nitishfy commented Sep 24, 2024 • edited Loading

bunnyshell bot commented Sep 24, 2024 • edited Loading

🔴 Preview Environment stopped on Bunnyshell

bunnyshell bot commented Sep 24, 2024

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

codecov bot commented Sep 24, 2024 • edited Loading

Codecov Report

leoluz left a comment

Choose a reason for hiding this comment

nitishfy left a comment • edited Loading

Choose a reason for hiding this comment

leoluz left a comment

Choose a reason for hiding this comment

leoluz Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leoluz Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leoluz Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitishfy left a comment

Choose a reason for hiding this comment

leoluz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nitishfy commented Sep 24, 2024 •

edited

Loading

bunnyshell bot commented Sep 24, 2024 •

edited

Loading

codecov bot commented Sep 24, 2024 •

edited

Loading

nitishfy left a comment •

edited

Loading

leoluz Oct 8, 2024 •

edited

Loading

leoluz Oct 8, 2024 •

edited

Loading

leoluz Oct 9, 2024 •

edited

Loading