Plugins can't re-use the same flags as cli global flags #1699

silvin-lubecki · 2019-02-28T13:44:53Z

Description
A plugin can't use the same name as one of the cli global flags (debug, log-level, context, ...), or worse can't use the same shortcut (ex: -c).

If I add a new flag to the hello-world plugin like this following:

flags.StringVarP(&compose, "compose", "c", "", "compose file")

and try to use it:

$ docker helloworld -c my-compose-file.yml
unable to resolve docker endpoint: context "my-compose-file.yml" does not exist

$ docker --help
...
  -c, --context string     Name of the context to use to connect to the daemon (overrides DOCKER_HOST env var and default context set with "docker context use")

This context flag is a root command flag, not a PersistentFlag and so shouldn't be used by the docker cli and forwarded directly to the plugin.

The text was updated successfully, but these errors were encountered:

ijc · 2019-02-28T16:21:18Z

I think (fairly sure) this might relate to 935d47b (which was basically the same problem but for unknown arguments).

Basically when cobra parses the command line it knows when it hits a new subcommand (since it has a list), but if it hits something (without a - or a--) which it doesn't know about it accumulates as a argument to the previous command. So e.g.:

docker app -c foo.yml

It accumulates app as an argument to docker, not as a new subcommand, and since it is therefore still in the "context" of the docker command when it hits the -c it treats it as the global, instead of falling through to the "I don't know what app is, try a plugin" path.

This is going to be tricky to address I think -- right now, due to how cobra works, I can't think of a way to address this without always adding a stub command for every plugin we find at start of day (implying a scan of the plugins on each invocation). I'll investigate what can be done on the cobra end, up to and perhaps including patching it, before it comes to that though.

thaJeztah · 2019-02-28T16:24:11Z

ah, yes, I ran into this as well 😞

These won't pass right now due to docker#1699 ("Plugins can't re-use the same flags as cli global flags") and the change in 935d47b ("Ignore unknown arguments on the top-level command."), but the intention is to fix them now. Signed-off-by: Ian Campbell <[email protected]>

The issue with plugin options clashing with globals is that when cobra is parsing the command line and it comes across an argument which doesn't start with a `-` it (in the absence of plugins) distinguishes between "argument to current command" and "new subcommand" based on the list of registered sub commands. Plugins breaks that model. When presented with `docker -D plugin -c foo` cobra parses up to the `plugin`, sees it isn't a registered sub-command of the top-level docker (because it isn't, it's a plugin) so it accumulates it as an argument to the top-level `docker` command. Then it sees the `-c`, and thinks it is the global `-c` (for AKA `--context`) option and tries to treat it as that, which fails. In the specific case of the top-level `docker` subcommand we know that it has no arguments which aren't `--flags` (or `-f` short flags) and so anything which doesn't start with a `-` must either be a (known) subcommand or an attempt to execute a plugin. We could simply scan for and register all installed plugins at start of day, so that cobra can do the right thing, but we want to avoid that since it would involve executing each plugin to fetch the metadata, even if the command wasn't going to end up hitting a plugin. Instead we can parse the initial set of global arguments separately before hitting the main cobra `Execute` path, which works here exactly because we know that the top-level has no non-flag arguments. One slight wrinkle is that the top-level `PersistentPreRunE` is no longer called on the plugins path (since it no longer goes via `Execute`), so we arrange for the initialisation done there (which has to be done after global flags are parsed to handle e.g. `--config`) to happen explictly after the global flags are parsed. Rather than make `newDockerCommand` return the complicated set of results needed to make this happen, instead return a closure which achieves this. The new functionality is introduced via a common `TopLevelCommand` abstraction which lets us adjust the plugin entrypoint to use the same strategy for parsing the global arguments. This isn't strictly required (in this case the stuff in cobra's `Execute` works fine) but doing it this way avoids the possibility of subtle differences in behaviour. Fixes docker#1699, and also, as a side-effect, the first item in docker#1661. Signed-off-by: Ian Campbell <[email protected]>