Skip to content

Conversation

nitishfy
Copy link
Member

@nitishfy nitishfy commented Sep 24, 2024

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

Copy link

bunnyshell bot commented Sep 24, 2024

❗ Preview Environment undeploy from Bunnyshell failed

See: Environment Details | Pipeline Logs

Available commands (reply to this comment):

  • 🚀 /bns:deploy to redeploy the environment
  • /bns:delete to try again to remove the environment

Copy link

bunnyshell bot commented Sep 24, 2024

❌ Preview Environment deleted from Bunnyshell

Available commands (reply to this comment):

  • 🚀 /bns:deploy to deploy the environment

Copy link

codecov bot commented Sep 24, 2024

Codecov Report

❌ Patch coverage is 73.33333% with 32 lines in your changes missing coverage. Please review.
✅ Project coverage is 59.94%. Comparing base (9a738b2) to head (8fafefb).
⚠️ Report is 487 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   56.08%   59.94%   +3.85%     
==========================================
  Files         343      344       +1     
  Lines       57546    57656     +110     
==========================================
+ Hits        32275    34561    +2286     
+ Misses      22627    20335    -2292     
- Partials     2644     2760     +116     

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

Copy link
Collaborator

@leoluz leoluz left a comment

Choose a reason for hiding this comment

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

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

@leoluz leoluz self-assigned this Sep 24, 2024
@nitishfy nitishfy force-pushed the nitish/argocd-plugin-support branch 2 times, most recently from 92967cd to 4a260dd Compare October 1, 2024 11:40
Copy link
Member Author

@nitishfy nitishfy left a comment

Choose a reason for hiding this comment

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

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

Update: Resolved

@nitishfy nitishfy force-pushed the nitish/argocd-plugin-support branch 2 times, most recently from 0e8f338 to c314e43 Compare October 8, 2024 11:35
@nitishfy 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 nitishfy requested a review from leoluz October 8, 2024 11:57
Copy link
Collaborator

@leoluz leoluz left a comment

Choose a reason for hiding this comment

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

@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

@blakepettersson 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 nitishfy force-pushed the nitish/argocd-plugin-support branch from 652887c to fea85cd Compare October 11, 2024 09:46
Copy link
Member Author

@nitishfy nitishfy left a comment

Choose a reason for hiding this comment

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

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

@nitishfy nitishfy requested a review from leoluz October 11, 2024 11:58
Copy link
Collaborator

@leoluz leoluz left a comment

Choose a reason for hiding this comment

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

Please check my comments

Copy link
Member Author

@nitishfy nitishfy left a comment

Choose a reason for hiding this comment

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

PTAL.

@nitishfy
Copy link
Member Author

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

Copy link
Collaborator

@leoluz leoluz left a comment

Choose a reason for hiding this comment

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

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

Copy link
Member

@blakepettersson blakepettersson left a comment

Choose a reason for hiding this comment

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

Minor nits, otherwise LGTM

Copy link
Member Author

@nitishfy nitishfy left a comment

Choose a reason for hiding this comment

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

PTAL.

// set by the cobra.OnInitialize() was never executed because cmd.Execute()
// gave us a non-nil error.
initConfig()
log.SetLevel(log.DebugLevel)
Copy link
Member Author

Choose a reason for hiding this comment

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

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
Copy link
Member Author

cc @agaudreault

@nitishfy
Copy link
Member Author

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 

Copy link
Collaborator

@leoluz leoluz left a comment

Choose a reason for hiding this comment

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

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
Copy link
Member

blakepettersson commented Mar 31, 2025

@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?

Copy link
Member Author

@nitishfy nitishfy left a comment

Choose a reason for hiding this comment

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

After some discusssions at KubeCon, I'm proposing few things here:

  1. A plugin should always respect it's own flags and it's a default behaviour if it doesn't respect the global flags.
  2. Although we may not require all flags for parsing, this actually can't be done because cobra internally doesn't parse flags if a command doesn't exist. Kubectl does the similar way. For example, If I have a kubectl plugin, a global flag wouldn't be parsed such as:
kubectl demo -h # unknown flag '-h'

TLDR: A Plugin should respect it's own flag and we should properly document this in the iimitations section that when you're creating your custom plugins, the global flag won't be parsed similar to how you can't create your own subcommand that override the existing argocd commnds.

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

modify mkdocs.yaml

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

add unit test

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

add more tests

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

modify docs a bit

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

update docs

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

fix merge conflicts

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

fix go.mod

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

add line

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

fix go.mod

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

fix lint checks

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

fix failing tests

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

fix lint checks

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

add e2e tests

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

add one more e2e test

Signed-off-by: nitishfy <justnitish06@gmail.com>
Signed-off-by: nitishfy <justnitish06@gmail.com>
Signed-off-by: nitishfy <justnitish06@gmail.com>
Signed-off-by: nitishfy <justnitish06@gmail.com>
@nitishfy
Copy link
Member Author

nitishfy commented Apr 18, 2025

  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.

@leoluz I think the plugin is working as expected, I just tested it for this scenario:

 > dist/argocd cluster-list    
Fetching the list of clusters connected to Argo CD...
FATA[0000] rpc error: code = Unauthenticated desc = invalid session: Token is expired 
Error: Failed to fetch cluster list. Ensure you are logged in to the Argo CD CLI.
               
> dist/argocd login localhost:8080 
WARNING: server certificate had error: tls: failed to verify certificate: x509: certificate signed by unknown authority. Proceed insecurely (y/n)? y
Username: admin
Password: 
'admin:login' logged in successfully
Context 'localhost:8080' updated
               
>  dist/argocd cluster-list        
Fetching the list of clusters connected to Argo CD...
SERVER                          NAME        VERSION  STATUS      MESSAGE  PROJECT
https://kubernetes.default.svc  in-cluster  1.31     Successful           
Cluster list retrieved successfully.

argocd-cluster-list content:

 cat /usr/local/bin/argocd-cluster-list 
#!/bin/bash

# Check if the argocd CLI is installed
if ! command -v argocd &> /dev/null; then
    echo "Error: Argo CD CLI (argocd) is not installed. Please install it first."
    exit 1
fi

# Run the `argocd cluster list` command
echo "Fetching the list of clusters connected to Argo CD..."
argocd cluster list

# Check if the command was successful
if [ $? -eq 0 ]; then
    echo "Cluster list retrieved successfully."
else
    echo "Error: Failed to fetch cluster list. Ensure you are logged in to the Argo CD CLI."
fi

This means once you've logged in to the server using the CLI, the current context will be used by the Plugin unless you change the context explicitly.

@agaudreault FYI

>  dist/argocd non-existent                 
WARN[0000] error looking for plugin 'argocd-non-existent': exec: "argocd-non-existent": executable file not found in $PATH 
Error: unknown command "non-existent" for "argocd"
Run 'argocd --help' for usage.

I've explicitly set the log format to text for plugin than json because the text looks actually better than json due to which you see the log level warning appearing as text and not json.

Signed-off-by: nitishfy <justnitish06@gmail.com>
@nitishfy nitishfy force-pushed the nitish/argocd-plugin-support branch from 8e62673 to ae2177b Compare April 18, 2025 06:07
Signed-off-by: nitishfy <justnitish06@gmail.com>
Signed-off-by: nitishfy <justnitish06@gmail.com>
@nitishfy
Copy link
Member Author

Ping @agaudreault

Copy link
Member

@agaudreault agaudreault left a comment

Choose a reason for hiding this comment

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

LGTM

@agaudreault agaudreault merged commit 6cf2961 into argoproj:master May 5, 2025
27 checks passed
@nitishfy nitishfy deleted the nitish/argocd-plugin-support branch May 6, 2025 07:34
LyhengTep pushed a commit to LyhengTep/argo-cd that referenced this pull request May 10, 2025
Signed-off-by: nitishfy <justnitish06@gmail.com>
Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
ranakan19 pushed a commit to ranakan19/argo-cd that referenced this pull request May 20, 2025
Signed-off-by: nitishfy <justnitish06@gmail.com>
Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
Signed-off-by: Kanika Rana <krana@redhat.com>
olivergondza pushed a commit to olivergondza/argo-cd that referenced this pull request May 20, 2025
Signed-off-by: nitishfy <justnitish06@gmail.com>
Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
Signed-off-by: Oliver Gondža <ogondza@gmail.com>
chansuke pushed a commit to chansuke/argo-cd that referenced this pull request Jun 4, 2025
Signed-off-by: nitishfy <justnitish06@gmail.com>
Co-authored-by: Alexandre Gaudreault <alexandre_gaudreault@intuit.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants