This regressed in 3af168c7df ("Ensure plugins can use `PersistentPreRunE`
again.") but this wasn't noticed because the helloworld test plugin has it's
own `PersistentPreRunE` which has the effect of deferring the resolution of the
global variable. In the case where the hook isn't used the variable is resolved
during `newPluginCommand` which is before the global variable was set.
Initialize the plugin command with a stub function wrapping the call to the
(global) hook, this defers resolving the variable until after it has been set,
otherwise the initial value (`nil`) is used in the struct.
Signed-off-by: Ian Campbell <ijc@docker.com>
Since #1654 so far we've had problems with it not working on Windows (npipe
lacked the `CloseRead` method) and problems with using tcp with tls (the tls
connection also lacks `CloseRead`). Both of these were workedaround in #1718
which added a nop `CloseRead` method.
However I am now seeing hangs (on Windows) where the `system dial-stdio`
subprocess is not exiting (I'm unsure why so far).
I think the 3rd problem found with this is an indication that `dial-stdio` is
not quite ready for wider use outside of its initial usecase (support for
`ssh://` URLs to connect to remote daemons).
This change simply disables the `dial-stdio` path for all plugins. However
rather than completely reverting 891b3d953e ("cli-plugins: use `docker system
dial-stdio` to call the daemon") I've just disabled the functionality at the
point of use and left in a trap door environment variable so that those who
want to experiment with this mode (and perhaps fully debug it) have an easier
path do doing so.
The e2e test for this case is disabled unless the trap door envvar is set. I
also renamed the test to clarify that it is about cli plugins.
Signed-off-by: Ian Campbell <ijc@docker.com>
I got a bit carried away in d4ced2ef77 ("allow plugins to have argument
which match a top-level flag.") and broke the ability of a plugin to use the
`PersistentPreRun(E)` hook on its top-level command (by unconditionally
overwriting it) and also broke the plugin framework if a plugin's subcommand
used those hooks (because they would shadow the root one). This could result in
either `dockerCli.Client()` returning `nil` or whatever initialisation the
plugin hoped to do not occuring.
This change revert the relevant bits and reinstates the requirement that a
plugin calls `plugin.PersistentPreRunE` if it uses that hook itself.
It is at least a bit nicer now since we avoid the need for the global struct
since the interesting state is now encapsulated in `tcmd` (and the closure).
In principal this could be done even more simply (by calling `tcmd.Initialize`
statically between `tcmd.HandleGlobalFlags` and `cmd.Execute`) however this has
the downside of _always_ initialising the cli (and therefore dialing the
daemon) even for the `docker-cli-plugin-metadata` command but also for the
`help foo` and `foo --help` commands (Cobra short-circuits the hooks in this
case).
Signed-off-by: Ian Campbell <ijc@docker.com>
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#1699, and also, as a side-effect, the first item in #1661.
Signed-off-by: Ian Campbell <ijc@docker.com>
The `conn` here is `*winio.win32MessageBytePipe` which does not have a
`CloseRead` method (it does have `CloseWrite`) resulting in:
docker@WIN-NUC0 C:\Users\docker>.\docker-windows-amd64.exe system dial-stdio
the raw stream connection does not implement halfCloser
Also disable the path which uses this for cli-plugins on Windows.
Signed-off-by: Ian Campbell <ijc@docker.com>
This means that plugins can use whatever methods the monolithic CLI supports,
which is good for consistency.
This relies on `os.Args[0]` being something which can be executed again to
reach the same binary, since it is propagated (via an envvar) to the plugin for
this purpose. This essentially requires that the current working directory and
path are not modified by the monolithic CLI before it launches the plugin nor
by the plugin before it initializes the client. This should be the case.
Previously the fake apiclient used by `TestExperimentalCLI` was not being used,
since `cli.Initialize` was unconditionally overwriting it with a real one
(talking to a real daemon during unit testing, it seems). This wasn't expected
nor desirable and no longer happens with the new arrangements, exposing the
fact that no `pingFunc` is provided, leading to a panic. Add a `pingFunc` to
the fake client to avoid this.
Signed-off-by: Ian Campbell <ijc@docker.com>
Previously a plugin which used these hooks would overwrite the top-level plugin
command's use of this hook, resulting in the dockerCli object not being fully
initialised.
Provide a function which plugins can use to chain to the required behaviour.
This required some fairly ugly arrangements to preserve state (which was
previously in-scope in `newPluginCOmmand`) to be used by the new function.
Signed-off-by: Ian Campbell <ijc@docker.com>
That is, the helper to be used from the plugin's `main`.
Also add a `helloworld` plugin example and build integration.
Signed-off-by: Ian Campbell <ijc@docker.com>
Nicolas De Loof
Sebastiaan van Stijn
Tonis Tiigi
Ian Campbell