Allow to use cagent binary as a docker cli plugin docker-agent. No functional change for cagent command.#1748
Conversation
There was a problem hiding this comment.
Review Summary
🔍 Found 1 high-severity and 1 medium-severity issue in the added code.
Critical Issue
- Line 41: The condition
len(os.Args) >= 0is always true and can cause a panic ifos.Argsis empty. Should belen(os.Args) > 0.
Additional Issue
- Line 164: The context passed to
Execute()is not propagated toplugin.Run(), so context cancellation won't work in plugin mode.
Please review the inline comments for details.
cmd/root/root.go
Outdated
| if runtime.GOOS == "windows" { | ||
| cliPluginBinary += ".exe" | ||
| } | ||
| return len(os.Args) >= 0 && strings.HasSuffix(os.Args[0], cliPluginBinary) |
There was a problem hiding this comment.
🔴 HIGH SEVERITY: Logic error - always true condition can cause panic
The condition len(os.Args) >= 0 is always true since slice length cannot be negative. More critically, if os.Args is empty (length 0), accessing os.Args[0] will cause a panic (index out of range).
Fix: Change the condition to len(os.Args) > 0 to ensure os.Args[0] is accessible:
return len(os.Args) > 0 && strings.HasSuffix(os.Args[0], cliPluginBinary)While os.Args is typically guaranteed to have at least one element in normal execution, it can be empty in test scenarios or if manipulated, making this a real risk.
| if strings.HasPrefix(err.Error(), "unknown command ") || strings.HasPrefix(err.Error(), "accepts ") { | ||
| _ = rootCmd.Usage() | ||
| if isCliPLugin() { | ||
| plugin.Run(func(dockerCli command.Cli) *cobra.Command { |
There was a problem hiding this comment.
The ctx parameter passed to Execute() is not used in the plugin path. In the non-plugin path (line 186), rootCmd.ExecuteContext(ctx) properly propagates the context. However, in the plugin path, the context is never passed to or used by plugin.Run().
Impact: Context cancellation, timeouts, and other context-based operations won't work when running as a CLI plugin. This creates inconsistent behavior between plugin and non-plugin execution modes.
Suggested fix: Consider calling rootCmd.SetContext(ctx) before passing it to the plugin callback, or investigate if the Docker CLI plugin framework provides a way to pass context.
… functional change if using `cagent` command. Signed-off-by: Guillaume Tardif <guillaume.tardif@gmail.com>
Signed-off-by: Guillaume Tardif <guillaume.tardif@gmail.com>
086fa66 to
0fb4d74
Compare
Signed-off-by: Guillaume Tardif <guillaume.tardif@gmail.com>
0fb4d74 to
c313c26
Compare
task deploy-local./bin/cagent(or .exe on windows) works exactly as beforedocker agentas a replacement for thecagentcommand, with no other changes:docker agent run ...