Commit Graph

146 Commits

Author SHA1 Message Date
Sebastiaan van Stijn 787caf2fe4
cmd/docker: fix completion for --context
registerCompletionFuncForGlobalFlags was called from newDockerCommand,
at which time no context-store is initialized yet, so it would return
a nil value, probably resulting in `store.Names` to panic, but these
errors are not shown when running the completion. As a result, the flag
completion would fall back to completing from filenames.

This patch changes the function to dynamically get the context-store;
this fixes the problem mentioned above, because at the time the completion
function is _invoked_, the CLI is fully initialized, and does have a
context-store available.

A (non-exported) interface is defined to allow the function to accept
alternative implementations (not requiring a full command.DockerCLI).

Before this patch:

    docker context create one
    docker context create two

    docker --context <TAB>
    .DS_Store                   .idea/                      Makefile
    .dockerignore               .mailmap                    build/
    ...

With this patch:

    docker context create one
    docker context create two

    docker --context <TAB>
    default  one      two

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 42b68a3ed7)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2024-07-19 01:56:24 +02:00
Sebastiaan van Stijn 7a4062a4a9
cli/command/completion: add FromList utility
It's an alias for cobra.FixedCompletions but takes a variadic list
of strings, so that it's not needed to construct an array for this.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 5e7bcbeac6)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2024-07-19 01:56:24 +02:00
Sebastiaan van Stijn 617eb5271a
cli: make initializing the global meter- and tracing providers optional
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2024-06-24 15:44:20 +02:00
Sebastiaan van Stijn b83cf582cd
Merge pull request #5178 from thaJeztah/buildkit_windows
build: allow BuildKit to be used on Windows daemons that advertise it
2024-06-20 18:46:41 +02:00
Alano Terblanche faf7647dcf
refactor: remove test
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
2024-06-20 17:03:33 +02:00
Alano Terblanche 1322f585fe
test: cli force exit signal handler
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
2024-06-20 14:32:38 +02:00
Sebastiaan van Stijn e5d26a8d40
build: allow BuildKit to be used on Windows daemons that advertise it
Commit 6fef143dbc switched the CLI to use
BuildKit by default, but as part of that removed the use of the
BuildkitVersion field as returned by Ping.

Some follow-up changes in commits e38e6c51ff and
e7a8748b93 updated the logic for detecting whether
BuildKit should be used or the legacy builder, but hard-coded using the
legacy builder for Windows daemons.

While Windows / WCOW does not yet support BuildKit by default, there is
work in progress to implement it, so we should not hard-code the assumption
that a Windows daemon cannot support BuildKit.

On the daemon-side, [moby@7b153b9] (Docker v23.0) changed the default as
advertised by the daemon to be BuildKit for Linux daemons. That change
still hardcoded BuildKit to be unsupported for Windows daemons (and does
not yet allow overriding the config), but this may change for future
versions of the daemon, or test-builds.

This patch:

- Re-introduces checks for the BuildkitVersion field in the "Ping" response.
- If the Ping response from the daemon advertises that it supports BuildKit,
  the CLI will now use BuildKit as builder.
- If we didn't get a Ping response, or the Ping response did NOT advertise
  that the daemon supported BuildKit, we continue to use the current
  defaults (BuildKit for Linux daemons, and the legacy builder for Windows)
- Handling of the DOCKER_BUILDKIT environment variable is unchanged; for
  CLI.BuildKitEnabled, DOCKER_BUILDKIT always takes precedence, and for
  processBuilder the value is taken into account, but will print a warning
  when BuildKit is disabled and a Linux daemon is used. For Windows daemons,
  no warning is printed.

[moby@7b153b9]: 7b153b9e28

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2024-06-20 02:49:22 +02:00
Alano Terblanche a4bfd8c744
feat: add a global sigint/sigterm handler as a fallback to ctx cancellation
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
2024-06-18 15:42:34 +02:00
Sebastiaan van Stijn c07cee05e2
Update go:build comments to go1.21
Match the minimum version that's specified on our vendor.mod.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2024-06-18 12:17:13 +02:00
Laura Brehm 3dcc653533
plugins: cleanup sockets when done
Since 509123f935, we've been leaking sockets
in the filesystem on platforms where abstract sockets aren't supported.

That change relied on Go to cleanup our sockets for us, which Go will happily
do as long as we make sure to close the listener, which we weren't previously
doing unless to signal the plugin to terminate.

This change adds a deferred call to `PluginServer.Close()`, which makes sure we
close the plugin server at the end of the plugin execution, so that we never exit
without cleaning up.

Signed-off-by: Laura Brehm <laurabrehm@hey.com>
2024-06-12 11:16:10 +01:00
Alano Terblanche 3f0d90a2a9
feat: global signal handling with context cancellation
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
2024-06-07 16:56:34 +02:00
Laura Brehm f07834d185
OTel: add `command.time` metric to plugin commands
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
2024-05-15 00:05:30 +01:00
Christopher Petito 02537eac59 Use funcs on DockerCli to return Meter/TracerProviders, not initialize them. Initialize them during DockerCli struct init
Signed-off-by: Christopher Petito <chrisjpetito@gmail.com>
2024-05-14 15:23:49 +00:00
Alano Terblanche 1d666b4105
feat: wire ctx into plugin hooks
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
2024-04-26 13:03:56 +02:00
Sebastiaan van Stijn 86162f7816
feat: use main func ctx for cobra and use ctx in tests
Explicitly create the context and set it on the CLI, instead of depending on
NewDockerCli() to instance a default context.

Co-authored-by: Sebastiaan van Stijn <github@gone.nl>
Co-authored-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>

Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
2024-04-25 12:00:31 +02:00
Laura Brehm 43cb06e1ae
hooks: pass command execution error to plugins
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
2024-04-22 17:12:53 +01:00
Laura Brehm 9d8320de9d
hooks: include full configured command
Before, for plugin commands, only the plugin name (such as `buildx`)
would be both included as `RootCmd` when passed to the hook plugin,
which isn't enough information for a plugin to decide whether to execute
a hook or not since plugins implement multiple varied commands (`buildx
build`, `buildx prune`, etc.).

This commit changes the hook logic to account for this situation, so
that the the entire configured hook is passed, i.e., if a user has a
hook configured for `buildx imagetools inspect` and the command
`docker buildx imagetools inspect alpine` is called, then the plugin
hooks will be passed `buildx imagetools inspect`.

This logic works for aliased commands too, so whether `docker build ...`
or `docker buildx build` is executed (unless Buildx is disabled) the
hook will be invoked with `buildx build`.

Signed-off-by: Laura Brehm <laurabrehm@hey.com>

hooks: include full match when invoking plugins

Signed-off-by: Laura Brehm <laurabrehm@hey.com>
2024-04-22 13:16:26 +01:00
Laura Brehm c449c1a49d
plugins/hooks: run hooks when exit code != 0
Particularly for cases such as `docker exec -it`, it's relevant that the CLI
still executes hooks even if the exec exited with a non-zero exit code,
since this is can be part of a normal `docker exec` invocation depending on
how the user exits.

In the future, this might also be interesting to allow plugins to run
hooks after an error so they can offer error-state recovery suggestions,
although this would require additional work to give the plugin more
information about the failed execution.

Signed-off-by: Laura Brehm <laurabrehm@hey.com>
2024-04-17 15:21:08 +01:00
Bjorn Neergaard 10b9810989
Merge pull request #4978 from laurazard/otel-add-tty
otel: capture whether process was invoked from a terminal
2024-04-04 06:09:48 -06:00
Laura Brehm 204b324291
Merge pull request #4975 from jsternberg/otel-error-handler
command: include default otel error handler for the cli
2024-04-04 03:56:41 +01:00
Laura Brehm ee1b2836af
otel: capture whether process was invoked from a terminal
This commit adds a "terminal" attribute to `BaseMetricAttributes`
that allows us to discern whether an invocation was from an interactive
terminal or not.

Signed-off-by: Laura Brehm <laurabrehm@hey.com>
2024-04-04 03:28:17 +01:00
Jonathan A. Sternberg 8f45f1495c
command: include default otel error handler for the cli
This adds a default otel error handler for the cli in the debug package.
It uses logrus to log the error on the debug level and should work out
of the box with the `--debug` flag and `DEBUG` environment variable.

Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
2024-04-03 12:01:28 -05:00
Sebastiaan van Stijn 9ca30bd2ac
Merge pull request #4939 from Benehiko/prompt-termination
feat: standardize error for prompt
2024-04-02 19:09:12 +02:00
Christopher Petito efd82e1e31 Initial otel impl using our utils
Signed-off-by: Christopher Petito <chrisjpetito@gmail.com>
2024-03-28 16:23:01 +00:00
Alano Terblanche 7c722c08d0
feat: standardize error for prompt
Signed-off-by: Alano Terblanche <18033717+Benehiko@users.noreply.github.com>
2024-03-26 14:11:55 +01:00
Bjorn Neergaard 799bf52680
Merge pull request #4376 from laurazard/plugin-hooks
Introduce support for CLI plugin hooks
2024-03-22 14:34:14 -06:00
Laura Brehm c5016c6d5b
cli-plugins: Introduce support for hooks
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
2024-03-22 17:30:18 +00:00
Bjorn Neergaard 542e82caeb
plugin: update/improve process lifecycle documentation
Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
2024-03-22 01:07:05 -06:00
Brian Goff d68cc0e8d0
plugin: closer-based plugin notification socket
This changes things to rely on a plugin server that manages all
connections made to the server.

An optional handler can be passed into the server when the caller wants
to do extra things with the connection.

It is the caller's responsibility to close the server.
When the server is closed, first all existing connections are closed
(and new connections are prevented).

Now the signal loop only needs to close the server and not deal with
`net.Conn`'s directly (or double-indirects as the case was before this
change).

The socket, when present in the filesystem, is no longer unlinked
eagerly, as reconnections require it to be present for the lifecycle of
the plugin server.

Co-authored-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
Signed-off-by: Brian Goff <cpuguy83@gmail.com>
Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
2024-03-21 15:08:19 -06:00
Jonathan A. Sternberg 9392831817
builder: correct the command path for docker build
The command path sent for `docker build` should be `docker` rather than
`docker build` to be consistent with the other command paths.

* `docker buildx build` has a command path of `docker buildx`
* `docker builder build` has a command path of `docker builder`
* `docker image build` has a command path of `docker image`

The reason this gets set to `docker buildx` rather than `docker buildx
build` is because the `build` portion of the command path is processed
by the plugin. So the command path only contains the portions of the
command path that were processed by this tool.

Since the `build` of `docker build` gets forwarded to `buildx`, it is
not included in the command path.

Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
2024-03-15 11:36:38 -05:00
Jonathan A. Sternberg 85dcacd78f
plugins: set OTEL_RESOURCE_ATTRIBUTES when invoking a plugin
When a plugin is invoked, the docker cli will now set
`OTEL_RESOURCE_ATTRIBUTES` to pass OTEL resource attribute names to the
plugin as additional resource attributes. At the moment, the only
resource attribute passed is `cobra.command_path`.

All resource attributes passed by the CLI are prepended with the
namespace `docker.cli` to avoid clashing with existing ones the plugin
uses or ones defined by the user.

For aliased commands like the various builder commands, the command path
is overwritten to match with the original name (such as `docker
builder`) instead of the forwarded name (such as `docker buildx build`).

Signed-off-by: Jonathan A. Sternberg <jonathan.sternberg@docker.com>
2024-02-28 12:43:05 -08:00
Laura Brehm 508346ef61
plugins: fix plugin socket being closed before use
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
2024-01-15 15:48:57 +00:00
Laura Brehm 5f6c55a724
plugins: don't handle signal/notify if TTY
In order to solve the "double notification" issue (see:
ef5e5fa03f)
without running the plugin process under a new pgid (see:
https://github.com/moby/moby/issues/47073) we instead check if we're
attached to a TTY, and if so skip signalling the plugin process since it
will already be signalled.

Signed-off-by: Laura Brehm <laurabrehm@hey.com>
2024-01-15 13:30:17 +00:00
Laura Brehm 26560ff93c
Revert "plugins: run plugin with new process group ID"
This reverts commit ef5e5fa03f.

Running new plugins under a new pgid isn't a viable solution due to
it causing issues with plugin processes attempting to read from the
TTY (see: https://github.com/moby/moby/issues/47073).

Signed-off-by: Laura Brehm <laurabrehm@hey.com>
2024-01-15 13:30:01 +00:00
Sebastiaan van Stijn 688de6db16
Merge pull request #4769 from laurazard/signal-handling-fix-tty
plugins: run plugin with new process group ID
2024-01-12 22:06:23 +01:00
Laura Brehm ef5e5fa03f
plugins: run plugin with new process group ID
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>
2024-01-12 13:53:28 -07:00
Bjorn Neergaard dbf992f91f
cli-plugins: move socket code into common package
Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
2024-01-12 11:49:25 -07:00
Sebastiaan van Stijn 859154b94c
Merge pull request #4778 from thaJeztah/cmd_docker_smaller_interface
cmd/docker: registerCompletionFuncForGlobalFlags: take store.Store as argument
2024-01-11 22:50:47 +01:00
Sebastiaan van Stijn 0e37dd49f0
cmd/docker: registerCompletionFuncForGlobalFlags: take store.Store as argument
Update this function to accept a smaller interface, as it doesn't need
all of "CLI". Also return errors encountered during its operation (although
the caller currently has no error return on its own).

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2024-01-11 22:31:17 +01:00
Sebastiaan van Stijn 11b2e871bc
cmd/docker: move main() to the top
It was hidden half-way the file; let's move it to the top, where I'd expect
to find it.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2024-01-11 22:19:17 +01:00
Sebastiaan van Stijn 70216b662d
add //go:build directives to prevent downgrading to go1.16 language
This is a follow-up to 0e73168b7e

This repository is not yet a module (i.e., does not have a `go.mod`). This
is not problematic when building the code in GOPATH or "vendor" mode, but
when using the code as a module-dependency (in module-mode), different semantics
are applied since Go1.21, which switches Go _language versions_ on a per-module,
per-package, or even per-file base.

A condensed summary of that logic [is as follows][1]:

- For modules that have a go.mod containing a go version directive; that
  version is considered a minimum _required_ version (starting with the
  go1.19.13 and go1.20.8 patch releases: before those, it was only a
  recommendation).
- For dependencies that don't have a go.mod (not a module), go language
  version go1.16 is assumed.
- Likewise, for modules that have a go.mod, but the file does not have a
  go version directive, go language version go1.16 is assumed.
- If a go.work file is present, but does not have a go version directive,
  language version go1.17 is assumed.

When switching language versions, Go _downgrades_ the language version,
which means that language features (such as generics, and `any`) are not
available, and compilation fails. For example:

    # github.com/docker/cli/cli/context/store
    /go/pkg/mod/github.com/docker/cli@v25.0.0-beta.2+incompatible/cli/context/store/storeconfig.go:6:24: predeclared any requires go1.18 or later (-lang was set to go1.16; check go.mod)
    /go/pkg/mod/github.com/docker/cli@v25.0.0-beta.2+incompatible/cli/context/store/store.go:74:12: predeclared any requires go1.18 or later (-lang was set to go1.16; check go.mod)

Note that these fallbacks are per-module, per-package, and can even be
per-file, so _(indirect) dependencies_ can still use modern language
features, as long as their respective go.mod has a version specified.

Unfortunately, these failures do not occur when building locally (using
vendor / GOPATH mode), but will affect consumers of the module.

Obviously, this situation is not ideal, and the ultimate solution is to
move to go modules (add a go.mod), but this comes with a non-insignificant
risk in other areas (due to our complex dependency tree).

We can revert to using go1.16 language features only, but this may be
limiting, and may still be problematic when (e.g.) matching signatures
of dependencies.

There is an escape hatch: adding a `//go:build` directive to files that
make use of go language features. From the [go toolchain docs][2]:

> The go line for each module sets the language version the compiler enforces
> when compiling packages in that module. The language version can be changed
> on a per-file basis by using a build constraint.
>
> For example, a module containing code that uses the Go 1.21 language version
> should have a `go.mod` file with a go line such as `go 1.21` or `go 1.21.3`.
> If a specific source file should be compiled only when using a newer Go
> toolchain, adding `//go:build go1.22` to that source file both ensures that
> only Go 1.22 and newer toolchains will compile the file and also changes
> the language version in that file to Go 1.22.

This patch adds `//go:build` directives to those files using recent additions
to the language. It's currently using go1.19 as version to match the version
in our "vendor.mod", but we can consider being more permissive ("any" requires
go1.18 or up), or more "optimistic" (force go1.21, which is the version we
currently use to build).

For completeness sake, note that any file _without_ a `//go:build` directive
will continue to use go1.16 language version when used as a module.

[1]: 58c28ba286/src/cmd/go/internal/gover/version.go (L9-L56)
[2]; https://go.dev/doc/toolchain#:~:text=The%20go%20line%20for,file%20to%20Go%201.22

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-12-14 15:03:46 +01:00
Sebastiaan van Stijn 7d92573852
Merge pull request #4599 from laurazard/plugin-signal-handling
cli-plugins: terminate plugin when CLI exits
2023-12-12 14:58:04 +01:00
Laura Brehm 1554ac3b5f
cli-plugins: terminate plugin when CLI exits
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>
2023-12-12 13:54:30 +00:00
Sebastiaan van Stijn 0e73168b7e
golangci-lint: revive: enable use-any
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-11-20 19:52:46 +01:00
Sebastiaan van Stijn 8e9aec6904
golangci-lint: revive: enable import-shadowing
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-11-20 19:52:41 +01:00
Sebastiaan van Stijn 112d79a413
appcontext: remove unused parts
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-09-28 00:05:00 +02:00
Sebastiaan van Stijn febb37a38e
remove buildkit as dependency
This copies the github.com/moby/buildkit/util/appcontext
package as an internal package. The appcontext package from
BuildKit was the only remaining dependency on BuildKit, and
while we may need some of its functionality, the implementation
is not correct for how it's used in docker/cli (so would need
a rewrite).

Moving a copy of the code into the docker/cli (but as internal
package to prevent others from depending on it) is a first step
in that process, and removes the circular dependency between
BuildKit and the CLi.

We are only using these:

    tree vendor/github.com/moby/buildkit
    vendor/github.com/moby/buildkit
    ├── AUTHORS
    ├── LICENSE
    └── util
        └── appcontext
            ├── appcontext.go
            ├── appcontext_unix.go
            ├── appcontext_windows.go
            └── register.go

    3 directories, 6 files

Before this:

    go mod graph | grep ' github.com/docker/cli'
    github.com/moby/buildkit@v0.11.6 github.com/docker/cli@v23.0.0-rc.1+incompatible

After this:

    go mod graph | grep ' github.com/docker/cli'
    # (nothing)

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-09-28 00:04:51 +02:00
Sebastiaan van Stijn bb57783ab8
cmd/docker: areFlagsSupported: don't Ping if not needed
This is a similar fix as 006c946389, which
fixed this for detection of commands that were executed. Make sure we don't
call the "/_ping" endpoint if we don't need to.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-08-22 09:34:09 +02:00
Sebastiaan van Stijn 88f44ec159
cli: SetupRootCommand: remove redundant flags return
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>
2023-06-28 16:26:50 +02:00
Sebastiaan van Stijn 2ae223038c
remove pre-go1.17 build-tags
Removed pre-go1.17 build-tags with go fix;

    go mod init
    go fix -mod=readonly ./...
    rm go.mod

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-05-05 18:23:03 +02:00