diff --git a/cli-plugins/examples/helloworld/main.go b/cli-plugins/examples/helloworld/main.go index 6251e85916..eca6acbef9 100644 --- a/cli-plugins/examples/helloworld/main.go +++ b/cli-plugins/examples/helloworld/main.go @@ -51,10 +51,6 @@ func main() { cmd := &cobra.Command{ Use: "helloworld", Short: "A basic Hello World plugin for tests", - // This is redundant but included to exercise - // the path where a plugin overrides this - // hook. - PersistentPreRunE: plugin.PersistentPreRunE, RunE: func(cmd *cobra.Command, args []string) error { if debug { fmt.Fprintf(dockerCli.Err(), "Plugin debug mode enabled") diff --git a/cli-plugins/plugin/plugin.go b/cli-plugins/plugin/plugin.go index 99ef36d845..bad632d1f8 100644 --- a/cli-plugins/plugin/plugin.go +++ b/cli-plugins/plugin/plugin.go @@ -4,18 +4,32 @@ import ( "encoding/json" "fmt" "os" - "sync" "github.com/docker/cli/cli" "github.com/docker/cli/cli-plugins/manager" "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/connhelper" - cliflags "github.com/docker/cli/cli/flags" "github.com/docker/docker/client" "github.com/spf13/cobra" - "github.com/spf13/pflag" ) +func runPlugin(dockerCli *command.DockerCli, plugin *cobra.Command, meta manager.Metadata) error { + tcmd := newPluginCommand(dockerCli, plugin, meta) + + // Doing this here avoids also calling it for the metadata + // command which needlessly initializes the client and tries + // to connect to the daemon. + plugin.PersistentPreRunE = func(_ *cobra.Command, _ []string) error { + return tcmd.Initialize(withPluginClientConn(plugin.Name())) + } + + cmd, _, err := tcmd.HandleGlobalFlags() + if err != nil { + return err + } + return cmd.Execute() +} + // Run is the top-level entry point to the CLI plugin framework. It should be called from your plugin's `main()` function. func Run(makeCmd func(command.Cli) *cobra.Command, meta manager.Metadata) { dockerCli, err := command.NewDockerCli() @@ -26,9 +40,7 @@ func Run(makeCmd func(command.Cli) *cobra.Command, meta manager.Metadata) { plugin := makeCmd(dockerCli) - cmd := newPluginCommand(dockerCli, plugin, meta) - - if err := cmd.Execute(); err != nil { + if err := runPlugin(dockerCli, plugin, meta); err != nil { if sterr, ok := err.(cli.StatusError); ok { if sterr.Status != "" { fmt.Fprintln(dockerCli.Err(), sterr.Status) @@ -45,40 +57,6 @@ func Run(makeCmd func(command.Cli) *cobra.Command, meta manager.Metadata) { } } -// options encapsulates the ClientOptions and FlagSet constructed by -// `newPluginCommand` such that they can be finalized by our -// `PersistentPreRunE`. This is necessary because otherwise a plugin's -// own use of that hook will shadow anything we add to the top-level -// command meaning the CLI is never Initialized. -var options struct { - name string - init, prerun sync.Once - opts *cliflags.ClientOptions - flags *pflag.FlagSet - dockerCli *command.DockerCli -} - -// PersistentPreRunE must be called by any plugin command (or -// subcommand) which uses the cobra `PersistentPreRun*` hook. Plugins -// which do not make use of `PersistentPreRun*` do not need to call -// this (although it remains safe to do so). Plugins are recommended -// to use `PersistenPreRunE` to enable the error to be -// returned. Should not be called outside of a commands -// PersistentPreRunE hook and must not be run unless Run has been -// called. -func PersistentPreRunE(cmd *cobra.Command, args []string) error { - var err error - options.prerun.Do(func() { - if options.opts == nil || options.flags == nil || options.dockerCli == nil { - panic("PersistentPreRunE called without Run successfully called first") - } - // flags must be the original top-level command flags, not cmd.Flags() - options.opts.Common.SetDefaultOptions(options.flags) - err = options.dockerCli.Initialize(options.opts, withPluginClientConn(options.name)) - }) - return err -} - func withPluginClientConn(name string) command.InitializeOpt { return command.WithInitializeClient(func(dockerCli *command.DockerCli) (client.APIClient, error) { cmd := "docker" @@ -111,7 +89,7 @@ func withPluginClientConn(name string) command.InitializeOpt { }) } -func newPluginCommand(dockerCli *command.DockerCli, plugin *cobra.Command, meta manager.Metadata) *cobra.Command { +func newPluginCommand(dockerCli *command.DockerCli, plugin *cobra.Command, meta manager.Metadata) *cli.TopLevelCommand { name := plugin.Name() fullname := manager.NamePrefix + name @@ -121,7 +99,6 @@ func newPluginCommand(dockerCli *command.DockerCli, plugin *cobra.Command, meta SilenceUsage: true, SilenceErrors: true, TraverseChildren: true, - PersistentPreRunE: PersistentPreRunE, DisableFlagsInUseLine: true, } opts, flags := cli.SetupPluginRootCommand(cmd) @@ -135,13 +112,7 @@ func newPluginCommand(dockerCli *command.DockerCli, plugin *cobra.Command, meta cli.DisableFlagsInUseLine(cmd) - options.init.Do(func() { - options.name = name - options.opts = opts - options.flags = flags - options.dockerCli = dockerCli - }) - return cmd + return cli.NewTopLevelCommand(cmd, dockerCli, opts, flags) } func newMetadataSubcommand(plugin *cobra.Command, meta manager.Metadata) *cobra.Command { @@ -151,8 +122,6 @@ func newMetadataSubcommand(plugin *cobra.Command, meta manager.Metadata) *cobra. cmd := &cobra.Command{ Use: manager.MetadataSubcommandName, Hidden: true, - // Suppress the global/parent PersistentPreRunE, which needlessly initializes the client and tries to connect to the daemon. - PersistentPreRun: func(cmd *cobra.Command, args []string) {}, RunE: func(cmd *cobra.Command, args []string) error { enc := json.NewEncoder(os.Stdout) enc.SetEscapeHTML(false) diff --git a/cli/cobra.go b/cli/cobra.go index 803872ee5a..ede2e46c98 100644 --- a/cli/cobra.go +++ b/cli/cobra.go @@ -2,9 +2,11 @@ package cli import ( "fmt" + "os" "strings" pluginmanager "github.com/docker/cli/cli-plugins/manager" + "github.com/docker/cli/cli/command" cliconfig "github.com/docker/cli/cli/config" cliflags "github.com/docker/cli/cli/flags" "github.com/docker/docker/pkg/term" @@ -84,6 +86,77 @@ func FlagErrorFunc(cmd *cobra.Command, err error) error { } } +// TopLevelCommand encapsulates a top-level cobra command (either +// docker CLI or a plugin) and global flag handling logic necessary +// for plugins. +type TopLevelCommand struct { + cmd *cobra.Command + dockerCli *command.DockerCli + opts *cliflags.ClientOptions + flags *pflag.FlagSet + args []string +} + +// NewTopLevelCommand returns a new TopLevelCommand object +func NewTopLevelCommand(cmd *cobra.Command, dockerCli *command.DockerCli, opts *cliflags.ClientOptions, flags *pflag.FlagSet) *TopLevelCommand { + return &TopLevelCommand{cmd, dockerCli, opts, flags, os.Args[1:]} +} + +// SetArgs sets the args (default os.Args[:1] used to invoke the command +func (tcmd *TopLevelCommand) SetArgs(args []string) { + tcmd.args = args + tcmd.cmd.SetArgs(args) +} + +// SetFlag sets a flag in the local flag set of the top-level command +func (tcmd *TopLevelCommand) SetFlag(name, value string) { + tcmd.cmd.Flags().Set(name, value) +} + +// HandleGlobalFlags takes care of parsing global flags defined on the +// command, it returns the underlying cobra command and the args it +// will be called with (or an error). +// +// On success the caller is responsible for calling Initialize() +// before calling `Execute` on the returned command. +func (tcmd *TopLevelCommand) HandleGlobalFlags() (*cobra.Command, []string, error) { + cmd := tcmd.cmd + + // We manually parse the global arguments and find the + // subcommand in order to properly deal with plugins. We rely + // on the root command never having any non-flag arguments. + flags := cmd.Flags() + + // We need !interspersed to ensure we stop at the first + // potential command instead of accumulating it into + // flags.Args() and then continuing on and finding other + // arguments which we try and treat as globals (when they are + // actually arguments to the subcommand). + flags.SetInterspersed(false) + defer flags.SetInterspersed(true) // Undo, any subsequent cmd.Execute() in the caller expects this. + + // We need the single parse to see both sets of flags. + flags.AddFlagSet(cmd.PersistentFlags()) + // Now parse the global flags, up to (but not including) the + // first command. The result will be that all the remaining + // arguments are in `flags.Args()`. + if err := flags.Parse(tcmd.args); err != nil { + // Our FlagErrorFunc uses the cli, make sure it is initialized + if err := tcmd.Initialize(); err != nil { + return nil, nil, err + } + return nil, nil, cmd.FlagErrorFunc()(cmd, err) + } + + return cmd, flags.Args(), nil +} + +// Initialize finalises global option parsing and initializes the docker client. +func (tcmd *TopLevelCommand) Initialize(ops ...command.InitializeOpt) error { + tcmd.opts.Common.SetDefaultOptions(tcmd.flags) + return tcmd.dockerCli.Initialize(tcmd.opts, ops...) +} + // VisitAll will traverse all commands from the root. // This is different from the VisitAll of cobra.Command where only parents // are checked. diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index dc540430d9..06f074431b 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -21,7 +21,7 @@ import ( "github.com/spf13/pflag" ) -func newDockerCommand(dockerCli *command.DockerCli) *cobra.Command { +func newDockerCommand(dockerCli *command.DockerCli) *cli.TopLevelCommand { var ( opts *cliflags.ClientOptions flags *pflag.FlagSet @@ -34,51 +34,14 @@ func newDockerCommand(dockerCli *command.DockerCli) *cobra.Command { SilenceUsage: true, SilenceErrors: true, TraverseChildren: true, - FParseErrWhitelist: cobra.FParseErrWhitelist{ - // UnknownFlags ignores any unknown - // --arguments on the top-level docker command - // only. This is necessary to allow passing - // --arguments to plugins otherwise - // e.g. `docker plugin --foo` is caught here - // in the monolithic CLI and `foo` is reported - // as an unknown argument. - UnknownFlags: true, - }, RunE: func(cmd *cobra.Command, args []string) error { if len(args) == 0 { return command.ShowHelp(dockerCli.Err())(cmd, args) } - plugincmd, err := pluginmanager.PluginRunCommand(dockerCli, args[0], cmd) - if pluginmanager.IsNotFound(err) { - return fmt.Errorf( - "docker: '%s' is not a docker command.\nSee 'docker --help'", args[0]) - } - if err != nil { - return err - } + return fmt.Errorf("docker: '%s' is not a docker command.\nSee 'docker --help'", args[0]) - err = plugincmd.Run() - if err != nil { - statusCode := 1 - exitErr, ok := err.(*exec.ExitError) - if !ok { - return err - } - if ws, ok := exitErr.Sys().(syscall.WaitStatus); ok { - statusCode = ws.ExitStatus() - } - return cli.StatusError{ - StatusCode: statusCode, - } - } - return nil }, PersistentPreRunE: func(cmd *cobra.Command, args []string) error { - // flags must be the top-level command flags, not cmd.Flags() - opts.Common.SetDefaultOptions(flags) - if err := dockerCli.Initialize(opts); err != nil { - return err - } return isSupported(cmd, dockerCli) }, Version: fmt.Sprintf("%s, build %s", version.Version, version.GitCommit), @@ -87,30 +50,28 @@ func newDockerCommand(dockerCli *command.DockerCli) *cobra.Command { opts, flags, helpCmd = cli.SetupRootCommand(cmd) flags.BoolP("version", "v", false, "Print version information and quit") - setFlagErrorFunc(dockerCli, cmd, flags, opts) + setFlagErrorFunc(dockerCli, cmd) - setupHelpCommand(dockerCli, cmd, helpCmd, flags, opts) - setHelpFunc(dockerCli, cmd, flags, opts) + setupHelpCommand(dockerCli, cmd, helpCmd) + setHelpFunc(dockerCli, cmd) cmd.SetOutput(dockerCli.Out()) commands.AddCommands(cmd, dockerCli) cli.DisableFlagsInUseLine(cmd) - setValidateArgs(dockerCli, cmd, flags, opts) + setValidateArgs(dockerCli, cmd) - return cmd + // flags must be the top-level command flags, not cmd.Flags() + return cli.NewTopLevelCommand(cmd, dockerCli, opts, flags) } -func setFlagErrorFunc(dockerCli *command.DockerCli, cmd *cobra.Command, flags *pflag.FlagSet, opts *cliflags.ClientOptions) { +func setFlagErrorFunc(dockerCli *command.DockerCli, cmd *cobra.Command) { // When invoking `docker stack --nonsense`, we need to make sure FlagErrorFunc return appropriate // output if the feature is not supported. // As above cli.SetupRootCommand(cmd) have already setup the FlagErrorFunc, we will add a pre-check before the FlagErrorFunc // is called. flagErrorFunc := cmd.FlagErrorFunc() cmd.SetFlagErrorFunc(func(cmd *cobra.Command, err error) error { - if err := initializeDockerCli(dockerCli, flags, opts); err != nil { - return err - } if err := isSupported(cmd, dockerCli); err != nil { return err } @@ -118,17 +79,12 @@ func setFlagErrorFunc(dockerCli *command.DockerCli, cmd *cobra.Command, flags *p }) } -func setupHelpCommand(dockerCli *command.DockerCli, rootCmd, helpCmd *cobra.Command, flags *pflag.FlagSet, opts *cliflags.ClientOptions) { +func setupHelpCommand(dockerCli command.Cli, rootCmd, helpCmd *cobra.Command) { origRun := helpCmd.Run origRunE := helpCmd.RunE helpCmd.Run = nil helpCmd.RunE = func(c *cobra.Command, args []string) error { - // No Persistent* hooks are called for help, so we must initialize here. - if err := initializeDockerCli(dockerCli, flags, opts); err != nil { - return err - } - if len(args) > 0 { helpcmd, err := pluginmanager.PluginRunCommand(dockerCli, args[0], rootCmd) if err == nil { @@ -163,14 +119,9 @@ func tryRunPluginHelp(dockerCli command.Cli, ccmd *cobra.Command, cargs []string return helpcmd.Run() } -func setHelpFunc(dockerCli *command.DockerCli, cmd *cobra.Command, flags *pflag.FlagSet, opts *cliflags.ClientOptions) { +func setHelpFunc(dockerCli command.Cli, cmd *cobra.Command) { defaultHelpFunc := cmd.HelpFunc() cmd.SetHelpFunc(func(ccmd *cobra.Command, args []string) { - if err := initializeDockerCli(dockerCli, flags, opts); err != nil { - ccmd.Println(err) - return - } - // Add a stub entry for every plugin so they are // included in the help output and so that // `tryRunPluginHelp` can find them or if we fall @@ -205,7 +156,7 @@ func setHelpFunc(dockerCli *command.DockerCli, cmd *cobra.Command, flags *pflag. }) } -func setValidateArgs(dockerCli *command.DockerCli, cmd *cobra.Command, flags *pflag.FlagSet, opts *cliflags.ClientOptions) { +func setValidateArgs(dockerCli *command.DockerCli, cmd *cobra.Command) { // The Args is handled by ValidateArgs in cobra, which does not allows a pre-hook. // As a result, here we replace the existing Args validation func to a wrapper, // where the wrapper will check to see if the feature is supported or not. @@ -223,9 +174,6 @@ func setValidateArgs(dockerCli *command.DockerCli, cmd *cobra.Command, flags *pf cmdArgs := ccmd.Args ccmd.Args = func(cmd *cobra.Command, args []string) error { - if err := initializeDockerCli(dockerCli, flags, opts); err != nil { - return err - } if err := isSupported(cmd, dockerCli); err != nil { return err } @@ -234,14 +182,53 @@ func setValidateArgs(dockerCli *command.DockerCli, cmd *cobra.Command, flags *pf }) } -func initializeDockerCli(dockerCli *command.DockerCli, flags *pflag.FlagSet, opts *cliflags.ClientOptions) error { - if dockerCli.Client() != nil { - return nil +func tryPluginRun(dockerCli command.Cli, cmd *cobra.Command, subcommand string) error { + plugincmd, err := pluginmanager.PluginRunCommand(dockerCli, subcommand, cmd) + if err != nil { + return err } - // when using --help, PersistentPreRun is not called, so initialization is needed. - // flags must be the top-level command flags, not cmd.Flags() - opts.Common.SetDefaultOptions(flags) - return dockerCli.Initialize(opts) + + if err := plugincmd.Run(); err != nil { + statusCode := 1 + exitErr, ok := err.(*exec.ExitError) + if !ok { + return err + } + if ws, ok := exitErr.Sys().(syscall.WaitStatus); ok { + statusCode = ws.ExitStatus() + } + return cli.StatusError{ + StatusCode: statusCode, + } + } + return nil +} + +func runDocker(dockerCli *command.DockerCli) error { + tcmd := newDockerCommand(dockerCli) + + cmd, args, err := tcmd.HandleGlobalFlags() + if err != nil { + return err + } + + if err := tcmd.Initialize(); err != nil { + return err + } + + if len(args) > 0 { + if _, _, err := cmd.Find(args); err != nil { + err := tryPluginRun(dockerCli, cmd, args[0]) + if !pluginmanager.IsNotFound(err) { + return err + } + // For plugin not found we fall through to + // cmd.Execute() which deals with reporting + // "command not found" in a consistent way. + } + } + + return cmd.Execute() } func main() { @@ -252,9 +239,7 @@ func main() { } logrus.SetOutput(dockerCli.Err()) - cmd := newDockerCommand(dockerCli) - - if err := cmd.Execute(); err != nil { + if err := runDocker(dockerCli); err != nil { if sterr, ok := err.(cli.StatusError); ok { if sterr.Status != "" { fmt.Fprintln(dockerCli.Err(), sterr.Status) diff --git a/cmd/docker/docker_test.go b/cmd/docker/docker_test.go index e0f23747c0..044daa12d3 100644 --- a/cmd/docker/docker_test.go +++ b/cmd/docker/docker_test.go @@ -2,6 +2,7 @@ package main import ( "bytes" + "io" "io/ioutil" "os" "testing" @@ -16,10 +17,12 @@ import ( func TestClientDebugEnabled(t *testing.T) { defer debug.Disable() - cmd := newDockerCommand(&command.DockerCli{}) - cmd.Flags().Set("debug", "true") - - err := cmd.PersistentPreRunE(cmd, []string{}) + tcmd := newDockerCommand(&command.DockerCli{}) + tcmd.SetFlag("debug", "true") + cmd, _, err := tcmd.HandleGlobalFlags() + assert.NilError(t, err) + assert.NilError(t, tcmd.Initialize()) + err = cmd.PersistentPreRunE(cmd, []string{}) assert.NilError(t, err) assert.Check(t, is.Equal("1", os.Getenv("DEBUG"))) assert.Check(t, is.Equal(logrus.DebugLevel, logrus.GetLevel())) @@ -27,31 +30,37 @@ func TestClientDebugEnabled(t *testing.T) { var discard = ioutil.NopCloser(bytes.NewBuffer(nil)) -func TestExitStatusForInvalidSubcommandWithHelpFlag(t *testing.T) { - cli, err := command.NewDockerCli(command.WithInputStream(discard), command.WithCombinedStreams(ioutil.Discard)) +func runCliCommand(t *testing.T, r io.ReadCloser, w io.Writer, args ...string) error { + t.Helper() + if r == nil { + r = discard + } + if w == nil { + w = ioutil.Discard + } + cli, err := command.NewDockerCli(command.WithInputStream(r), command.WithCombinedStreams(w)) assert.NilError(t, err) - cmd := newDockerCommand(cli) - cmd.SetArgs([]string{"help", "invalid"}) - err = cmd.Execute() + tcmd := newDockerCommand(cli) + tcmd.SetArgs(args) + cmd, _, err := tcmd.HandleGlobalFlags() + assert.NilError(t, err) + assert.NilError(t, tcmd.Initialize()) + return cmd.Execute() +} + +func TestExitStatusForInvalidSubcommandWithHelpFlag(t *testing.T) { + err := runCliCommand(t, nil, nil, "help", "invalid") assert.Error(t, err, "unknown help topic: invalid") } func TestExitStatusForInvalidSubcommand(t *testing.T) { - cli, err := command.NewDockerCli(command.WithInputStream(discard), command.WithCombinedStreams(ioutil.Discard)) - assert.NilError(t, err) - cmd := newDockerCommand(cli) - cmd.SetArgs([]string{"invalid"}) - err = cmd.Execute() + err := runCliCommand(t, nil, nil, "invalid") assert.Check(t, is.ErrorContains(err, "docker: 'invalid' is not a docker command.")) } func TestVersion(t *testing.T) { var b bytes.Buffer - cli, err := command.NewDockerCli(command.WithInputStream(discard), command.WithCombinedStreams(&b)) - assert.NilError(t, err) - cmd := newDockerCommand(cli) - cmd.SetArgs([]string{"--version"}) - err = cmd.Execute() + err := runCliCommand(t, nil, &b, "--version") assert.NilError(t, err) assert.Check(t, is.Contains(b.String(), "Docker version")) } diff --git a/e2e/cli-plugins/flags_test.go b/e2e/cli-plugins/flags_test.go index 4bf01b9c47..5a446f6eb8 100644 --- a/e2e/cli-plugins/flags_test.go +++ b/e2e/cli-plugins/flags_test.go @@ -3,8 +3,6 @@ package cliplugins import ( "testing" - "gotest.tools/assert" - is "gotest.tools/assert/cmp" "gotest.tools/icmd" ) @@ -73,28 +71,18 @@ func TestUnknownGlobal(t *testing.T) { run, _, cleanup := prepare(t) defer cleanup() - t.Run("no-val", func(t *testing.T) { - res := icmd.RunCmd(run("--unknown", "helloworld")) - res.Assert(t, icmd.Expected{ - ExitCode: 125, + for name, args := range map[string][]string{ + "no-val": {"--unknown", "helloworld"}, + "separate-val": {"--unknown", "foo", "helloworld"}, + "joined-val": {"--unknown=foo", "helloworld"}, + } { + t.Run(name, func(t *testing.T) { + res := icmd.RunCmd(run(args...)) + res.Assert(t, icmd.Expected{ + ExitCode: 125, + Out: icmd.None, + Err: "unknown flag: --unknown", + }) }) - assert.Assert(t, is.Equal(res.Stdout(), "")) - assert.Assert(t, is.Contains(res.Stderr(), "unknown flag: --unknown")) - }) - t.Run("separate-val", func(t *testing.T) { - res := icmd.RunCmd(run("--unknown", "foo", "helloworld")) - res.Assert(t, icmd.Expected{ - ExitCode: 125, - }) - assert.Assert(t, is.Equal(res.Stdout(), "")) - assert.Assert(t, is.Contains(res.Stderr(), "unknown flag: --unknown")) - }) - t.Run("joined-val", func(t *testing.T) { - res := icmd.RunCmd(run("--unknown=foo", "helloworld")) - res.Assert(t, icmd.Expected{ - ExitCode: 125, - }) - assert.Assert(t, is.Equal(res.Stdout(), "")) - assert.Assert(t, is.Contains(res.Stderr(), "unknown flag: --unknown")) - }) + } }