Changes were made in 1554ac3b5f to provide
a mechanism for the CLI to notify running plugin processes that they
should exit, in order to improve the general CLI/plugin UX. The current
implementation boils down to:
1. The CLI creates a socket
2. The CLI executes the plugin
3. The plugin connects to the socket
4. (When) the CLI receives a termination signal, it uses the socket to
notify the plugin that it should exit
5. The plugin's gets notified via the socket, and cancels it's `cmd.Context`,
which then gets handled appropriately
This change works in most cases and fixes the issue it sets out to solve
(see: https://github.com/docker/compose/pull/11292) however, in the case
where the user has a TTY attached and the plugin is not already handling
received signals, steps 4+ changes:
4. (When) the CLI receives a termination signal, before it can use the
socket to notify the plugin that it should exit, the plugin process
also receives a signal due to sharing the pgid with the CLI
Since we now have a proper "job control" mechanism, we can simplify the
scenarios by executing the plugins with their own process group id,
thereby removing the "double notification" issue and making it so that
plugins can handle the same whether attached to a TTY or not.
In order to make this change "plugin-binary" backwards-compatible, in
the case that a plugin does not connect to the socket, the CLI passes
the signal to the plugin process.
Co-authored-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
Previously, long lived CLI plugin processes weren't
properly handled
(see: https://github.com/docker/cli/issues/4402)
resulting in plugin processes being left behind
running, after the CLI process exits.
This commit changes the plugin handling code to open
an abstract unix socket before running the plugin and
passing it to the plugin process, and changes the
signal handling on the CLI side to close this socket
which tells the plugin that it should exit.
This implementation makes use of sockets instead of
simply setting PDEATHSIG on the plugin process
so that it will work on both BSDs, assorted UNIXes
and Windows.
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
The flag-set that was returned is a pointer to the command's Flags(), which
is in itself passed by reference (as it is modified / set up).
This patch removes the flags return, to prevent assuming it's different than
the command's flags.
While SetupRootCommand is exported, a search showed that it's only used internally,
so changing the signature should not be a problem.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This field was marked deprecated in 977d3ae046,
which is part of Docker 20.10 and up.
This patch removes the field.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
the "golang.org/x/sys/execabs" package was introduced to address a security
issue on Windows, and changing the default behavior of os/exec was considered
a breaking change. go1.19 applied the behavior that was previously implemented
in the execabs package;
from the release notes: https://go.dev/doc/go1.19#os-exec-path
> Command and LookPath no longer allow results from a PATH search to be found
> relative to the current directory. This removes a common source of security
> problems but may also break existing programs that depend on using, say,
> exec.Command("prog") to run a binary named prog (or, on Windows, prog.exe)
> in the current directory. See the os/exec package documentation for information
> about how best to update such programs.
>
> On Windows, Command and LookPath now respect the NoDefaultCurrentDirectoryInExePath
> environment variable, making it possible to disable the default implicit search
> of “.” in PATH lookups on Windows systems.
With those changes, we no longer need to use the execabs package, and we can
switch back to os/exec.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This field was marked deprecated in 977d3ae046,
which is part of v20.10 and up, but the comment was missing a newline before
the deprecation message, which may be picked up by IDEs, but is not matching
the correct format, so may not be picked up by linters.
This patch fixes the format, to make sure linters pick up that the field is
deprecated.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This reverts commit 62f2358b99.
Spawning a goroutine for each iteration in the loop when listing
plugins is racy unfortunately. `plugins` slice is protected with
a mutex so not sure why it fails.
I tried using a channel to collect the plugins instead of a slice
to guarantee that they will be appended to the list in the order
they are processed but no dice.
I also tried without errgroup package and simply use sync.WaitGroup
but same. I have also created an extra channel to receive errors
from the goroutines but racy too.
I think the change in this function is not related to the race
condition but newPlugin is. So revert in the meantime :(
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
We are currently loading plugin command stubs for every
invocation which still has a significant performance hit.
With this change we are doing this operation only if cobra
completion arg request is found.
- 20.10.23: `docker --version` takes ~15ms
- 23.0.1: `docker --version` takes ~93ms
With this change `docker --version` takes ~9ms
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
We are currently loading plugin commands stubs for every
command invocation to add support for Cobra v2 completion.
This cause a significant performance hit if there is a
lot of plugins in the user space (7 atm in Docker Desktop):
`docker --version` takes in current 23.0.1 ~93ms
Instead of removing completion for plugins to fix the
regression, we can slightly improve plugins discovery by
spawning a goroutine for each iteration in the loop when
listing plugins:
`docker --version` now takes ~38ms
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
Both the DockerCLI and Cobra Commands provide accessors for Input, Output,
and Error streams (usually STDIN, STDOUT, STDERR). While we were already
passing DockerCLI's Output to Cobra, we were not doing so for the other
streams (and were passing none for plugin commands), potentially resulting
in DockerCLI output/input to mean something else than a Cobra Command's
intput/output/error.
This patch sets them to the same streams when constructing the Cobra
command.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The test used `gopkg.in/yaml.v2` to verify the TextMarshaller implementation,
which was implemented to allow printing the errors in JSON formatted output;
> This exists primarily to implement encoding.TextMarshaller such that
> rendering a plugin as JSON (e.g. for `docker info -f '{{json .CLIPlugins}}'`)
> renders the Err field as a useful string and not just `{}`.
Given that both yaml.Marshal and json.Marshal use this, we may as well use
Go's stdlib.
While updating, also changed some of the assertions to checks, so that we don't
fail the test early.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Looks like the linter uses an explicit -lang, which (for go1.19)
results in some additional formatting for octal values.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Older versions of Go do not format these comments, so we can already
reformat them ahead of time to prevent gofmt linting failing once
we update to Go 1.19 or up.
Result of:
gofmt -s -w $(find . -type f -name '*.go' | grep -v "/vendor/")
With some manual adjusting.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
On Windows, the os/exec.{Command,CommandContext,LookPath} functions
resolve command names that have neither path separators nor file extension
(e.g., "git") by first looking in the current working directory before
looking in the PATH environment variable.
Go maintainers intended to match cmd.exe's historical behavior.
However, this is pretty much never the intended behavior and as an abundance of precaution
this patch prevents that when executing commands.
Example of commands that docker.exe may execute: `git`, `docker-buildx` (or other cli plugin), `docker-credential-wincred`, `docker`.
Note that this was prompted by the [Go 1.15.7 security fixes](https://blog.golang.org/path-security), but unlike in `go.exe`,
the windows path lookups in docker are not in a code path allowing remote code execution, thus there is no security impact on docker.
Signed-off-by: Tibor Vass <tibor@docker.com>
The CLI disabled experimental features by default, requiring users
to set a configuration option to enable them.
Disabling experimental features was a request from Enterprise users
that did not want experimental features to be accessible.
We are changing this policy, and now enable experimental features
by default. Experimental features may still change and/or removed,
and will be highlighted in the documentation and "usage" output.
For example, the `docker manifest inspect --help` output now shows:
EXPERIMENTAL:
docker manifest inspect is an experimental feature.
Experimental features provide early access to product functionality. These features
may change between releases without warning or can be removed entirely from a future
release. Learn more about experimental features: https://docs.docker.com/go/experimental/
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The vanity domain is down, and the project has moved
to a new location.
vendor check started failing because of this:
Collecting initial packages
Download dependencies
unrecognized import path "vbom.ml/util" (https fetch: Get https://vbom.ml/util?go-get=1: dial tcp: lookup vbom.ml on 169.254.169.254:53: no such host)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Before this change, plugins were listed in a random order:
Client:
Debug Mode: false
Plugins:
doodle: Docker Doodles all around! 🐳🎃 (thaJeztah, v0.0.1)
shell: Open a browser shell on the Docker Host. (thaJeztah, v0.0.1)
app: Docker Application (Docker Inc., v0.8.0)
buildx: Build with BuildKit (Docker Inc., v0.3.1-tp-docker)
With this change, plugins are listed alphabetically:
Client:
Debug Mode: false
Plugins:
app: Docker Application (Docker Inc., v0.8.0)
buildx: Build with BuildKit (Docker Inc., v0.3.1-tp-docker)
doodle: Docker Doodles all around! 🐳🎃 (thaJeztah, v0.0.1)
shell: Open a browser shell on the Docker Host. (thaJeztah, v0.0.1)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
To test, add $(pwd)/build/plugins-linux-amd64 to "cliPluginsExtraDirs" config and run:
make plugins
make binary
HELLO_EXPERIMENTAL=1 docker helloworld
To show it enabled:
HELLO_EXPERIMENTAL=1 DOCKER_CLI_EXPERIMENTAL=enabled docker helloworld
Signed-off-by: Tibor Vass <tibor@docker.com>
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>
With this patch it is possible to alias an existing allowed command.
At the moment only builder allows an alias.
This also properly puts the build command under builder, instead of image
where it was for historical reasons.
Signed-off-by: Tibor Vass <tibor@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>
I regressed this in d4ced2ef77 ("allow plugins to have argument which match a
top-level flag.") by unconditionally overwriting any `PersistentRunE` that the
user may have supplied.
We need to ensure two things:
1. That the user can use `PersistentRunE` (or `PersistentRun`) for their own
purposes.
2. That our initialisation always runs, even if the user has used
`PersistentRun*`, since that will shadow the root.
To do this add a `PersistentRunE` to the helloworld plugin which logs (covers 1
above) and then use it when calling the `apiversion` subcommand (which covers 2
since that uses the client)
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>
These won't pass right now due to https://github.com/docker/cli/issues/1699
("Plugins can't re-use the same flags as cli global flags") and the change in
935d47bbe9 ("Ignore unknown arguments on the top-level command."), but the
intention is to fix them now.
Signed-off-by: Ian Campbell <ijc@docker.com>
Laura Brehm
Sebastiaan van Stijn
Sebastiaan van Stijn
Kevin Alvarez
Nicolas De Loof
Tibor Vass
Silvin Lubecki
Tonis Tiigi
Ian Campbell
Stefan Scherer
Silvin Lubecki