From 9f6fea8e7bd3e64edab2bc5bac337b4118ea97df Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Sun, 20 Nov 2016 09:57:06 -0800 Subject: [PATCH 1/2] Improve error handling of experimental features in non-experimental mode This fix tries to address several issues raised in 28626 where run against a non-experimental daemon may not generate correct error message: 1. Incorrect flags were not checked against the supported features: ``` $ docker stack --nonsense unknown flag: --nonsense ``` 2. Subcommands were not checked against the supported features: ``` $ docker stack ls Error response from daemon: This node is not a swarm manager... ``` This fix address the above mentioned issues by: 1. Add a pre-check for FlagErrorFunc 2. Recursively check if a feature is supported for cmd and its parents. This fix fixes 28626. Signed-off-by: Yong Tang --- cmd/docker/docker.go | 24 ++++++++++++++++++++++-- 1 file changed, 22 insertions(+), 2 deletions(-) diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index 654c7f41d6..44e6529b82 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -53,6 +53,23 @@ func newDockerCommand(dockerCli *command.DockerCli) *cobra.Command { }, } cli.SetupRootCommand(cmd) + // 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 dockerCli.Client() == nil { // when using --help, PersistenPreRun is not called, so initialization is needed. + // flags must be the top-level command flags, not cmd.Flags() + opts.Common.SetDefaultOptions(flags) + dockerPreRun(opts) + dockerCli.Initialize(opts) + } + if err := isSupported(cmd, dockerCli.Client().ClientVersion(), dockerCli.HasExperimental()); err != nil { + return err + } + return flagErrorFunc(cmd, err) + }) cmd.SetHelpFunc(func(ccmd *cobra.Command, args []string) { if dockerCli.Client() == nil { // when using --help, PersistentPreRun is not called, so initialization is needed. @@ -167,9 +184,12 @@ func hideUnsupportedFeatures(cmd *cobra.Command, clientVersion string, hasExperi } func isSupported(cmd *cobra.Command, clientVersion string, hasExperimental bool) error { + // We check recursively so that, e.g., `docker stack ls` will return the same output as `docker stack` if !hasExperimental { - if _, ok := cmd.Tags["experimental"]; ok { - return errors.New("only supported on a Docker daemon with experimental features enabled") + for curr := cmd; curr != nil; curr = curr.Parent() { + if _, ok := curr.Tags["experimental"]; ok { + return errors.New("only supported on a Docker daemon with experimental features enabled") + } } } From 8421fc634907a68889b0f20c13b0cf135f0817c6 Mon Sep 17 00:00:00 2001 From: Yong Tang Date: Mon, 21 Nov 2016 14:34:55 -0800 Subject: [PATCH 2/2] Additional experimental features in non-experimental mode error handling This fix is the follow up of the last commit. In this fix: 1. If any of the parents of a command has tags, then this command's `Args` (Args validation func) will be wrapped up. The warpped up func will check to see if the feature is supported or not. If it is not supported, then a not supported message is generated instead. This fix is related to 28626. Signed-off-by: Yong Tang --- cmd/docker/docker.go | 99 ++++++++++++++++++++++++++++++++++---------- 1 file changed, 78 insertions(+), 21 deletions(-) diff --git a/cmd/docker/docker.go b/cmd/docker/docker.go index 44e6529b82..efc1cac25e 100644 --- a/cmd/docker/docker.go +++ b/cmd/docker/docker.go @@ -53,32 +53,43 @@ func newDockerCommand(dockerCli *command.DockerCli) *cobra.Command { }, } cli.SetupRootCommand(cmd) + + flags = cmd.Flags() + flags.BoolVarP(&opts.Version, "version", "v", false, "Print version information and quit") + flags.StringVar(&opts.ConfigDir, "config", cliconfig.Dir(), "Location of client config files") + opts.Common.InstallFlags(flags) + + setFlagErrorFunc(dockerCli, cmd, flags, opts) + + setHelpFunc(dockerCli, cmd, flags, opts) + + cmd.SetOutput(dockerCli.Out()) + cmd.AddCommand(newDaemonCommand()) + commands.AddCommands(cmd, dockerCli) + + setValidateArgs(dockerCli, cmd, flags, opts) + + return cmd +} + +func setFlagErrorFunc(dockerCli *command.DockerCli, cmd *cobra.Command, flags *pflag.FlagSet, opts *cliflags.ClientOptions) { // 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 dockerCli.Client() == nil { // when using --help, PersistenPreRun is not called, so initialization is needed. - // flags must be the top-level command flags, not cmd.Flags() - opts.Common.SetDefaultOptions(flags) - dockerPreRun(opts) - dockerCli.Initialize(opts) - } + initializeDockerCli(dockerCli, flags, opts) if err := isSupported(cmd, dockerCli.Client().ClientVersion(), dockerCli.HasExperimental()); err != nil { return err } return flagErrorFunc(cmd, err) }) +} +func setHelpFunc(dockerCli *command.DockerCli, cmd *cobra.Command, flags *pflag.FlagSet, opts *cliflags.ClientOptions) { cmd.SetHelpFunc(func(ccmd *cobra.Command, args []string) { - if dockerCli.Client() == nil { // 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) - dockerPreRun(opts) - dockerCli.Initialize(opts) - } - + initializeDockerCli(dockerCli, flags, opts) if err := isSupported(ccmd, dockerCli.Client().ClientVersion(), dockerCli.HasExperimental()); err != nil { ccmd.Println(err) return @@ -90,17 +101,52 @@ func newDockerCommand(dockerCli *command.DockerCli) *cobra.Command { ccmd.Println(err) } }) +} - flags = cmd.Flags() - flags.BoolVarP(&opts.Version, "version", "v", false, "Print version information and quit") - flags.StringVar(&opts.ConfigDir, "config", cliconfig.Dir(), "Location of client config files") - opts.Common.InstallFlags(flags) +func setValidateArgs(dockerCli *command.DockerCli, cmd *cobra.Command, flags *pflag.FlagSet, opts *cliflags.ClientOptions) { + // 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. + // The Args validation error will only be returned if the feature is supported. + visitAll(cmd, func(ccmd *cobra.Command) { + // if there is no tags for a command or any of its parent, + // there is no need to wrap the Args validation. + if !hasTags(ccmd) { + return + } - cmd.SetOutput(dockerCli.Out()) - cmd.AddCommand(newDaemonCommand()) - commands.AddCommands(cmd, dockerCli) + if ccmd.Args == nil { + return + } - return cmd + cmdArgs := ccmd.Args + ccmd.Args = func(cmd *cobra.Command, args []string) error { + initializeDockerCli(dockerCli, flags, opts) + if err := isSupported(cmd, dockerCli.Client().ClientVersion(), dockerCli.HasExperimental()); err != nil { + return err + } + return cmdArgs(cmd, args) + } + }) +} + +func initializeDockerCli(dockerCli *command.DockerCli, flags *pflag.FlagSet, opts *cliflags.ClientOptions) { + if dockerCli.Client() == nil { // 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) + dockerPreRun(opts) + dockerCli.Initialize(opts) + } +} + +// visitAll will traverse all commands from the root. +// This is different from the VisitAll of cobra.Command where only parents +// are checked. +func visitAll(root *cobra.Command, fn func(*cobra.Command)) { + for _, cmd := range root.Commands() { + visitAll(cmd, fn) + } + fn(root) } func noArgs(cmd *cobra.Command, args []string) error { @@ -230,3 +276,14 @@ func isFlagSupported(f *pflag.Flag, clientVersion string) bool { } return true } + +// hasTags return true if any of the command's parents has tags +func hasTags(cmd *cobra.Command) bool { + for curr := cmd; curr != nil; curr = curr.Parent() { + if len(curr.Tags) > 0 { + return true + } + } + + return false +}