Commit Graph

112 Commits

Author SHA1 Message Date
Sebastiaan van Stijn ce4469a696
Merge pull request #5234 from thaJeztah/nicer_missing_commands
cli: improve output and consistency for unknown (sub)commands
2024-07-17 01:22:03 +02:00
Sebastiaan van Stijn b7695d6c79
cli-plugins: RunPlugin(): rename error-variable that's possibly shadowed
The logic in this function is confusing; let's start make it obvious where
the error that is returned is produced,

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2024-07-05 11:01:41 +02:00
Sebastiaan van Stijn 350a0b68a9
cli-plugins: Run(): don't discard cli.StatusError errors without message
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2024-07-05 10:59:27 +02:00
Sebastiaan van Stijn a6e96c758e
cli: improve output and consistency for unknown (sub)commands
Before this patch, output for invalid top-level and sub-commands differed.
For top-level commands, the CLI would print an error-message and a suggestion
to use `--help`. For missing *subcommands*, we would hit a different code-path,
and different output, which includes full "usage" / "help" output.

While it is a common convention to show usage output, and may have been
a nice gesture when docker was still young and only had a few commands
and options ("you did something wrong; here's an overview of what you
can use"), that's no longer the case, and many commands have a _very_
long output.

The result of this is that the error message, which is the relevant
information in this case - "You mis-typed something" - is lost in the
output, and hard to find (sometimes even requiring scrolling back).

The output is also confusing, because it _looks_ like something ran
successfully (most of the output is not about the error!).

Even further; the suggested resolution (try `--help` to see the correct
options) is rather redundant, because running teh command with `--help`
produces _exactly_ the same output as was just showh, baring the error
message. As a fun fact, due to the usage output being printed, the
output even contains not one, but _two_ "call to actions";

- `See 'docker volume --help'.` (under the erro message)
- `Run 'docker volume COMMAND --help' for more information on a command.`
  (under the usage output)

In short; the output is too verbose, confusing, and doesn't provide
a good UX. Let's reduce the output produced so that the focus is on the
important information.

This patch:

- Changes the usage to the short-usage.
- Changes the error-message to mention the _full_ command instead of only
  the command after `docker` (so `docker no-such-command` instead of
  `no-such-command`).
- Prefixes the error message with the binary / root-command name
  (usually `docker:`); this is something we can still decide on, but
  it's a pattern we already use in some places. The motivation for this
  is that `docker` commands can often produce output that's a combination
  of output from the CLI itself, output from the daemon, and even output
  from the container. The `docker:` prefix helps to distinguish where
  the message originated from (the `docker` CLI in this case).
- Adds an empty line between the error-message and the "call to action"
  (`Run 'docker volume --help'...` in the example below). This helps
  separating the error message ("unkown flag") from the call-to-action.

Before this patch:

Unknown top-level command:

    docker nosuchcommand foo
    docker: 'nosuchcommand' is not a docker command.
    See 'docker --help'

Unknown sub-command:

    docker volume nosuchcommand foo

    Usage:  docker volume COMMAND

    Manage volumes

    Commands:
      create      Create a volume
      inspect     Display detailed information on one or more volumes
      ls          List volumes
      prune       Remove unused local volumes
      rm          Remove one or more volumes
      update      Update a volume (cluster volumes only)

    Run 'docker volume COMMAND --help' for more information on a command.

After this patch:

Unknown top-level command:

    docker nosuchcommand foo
    docker: unknown command: docker nosuchcommand

    Run 'docker --help' for more information

Unknown sub-command:

    docker volume nosuchcommand foo
    docker: unknown command: 'docker volume nosuchcommand'

    Usage:  docker volume COMMAND

    Run 'docker volume --help' for more information

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2024-07-05 02:28:11 +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 55a1f6eb73
cli-plugins/manager: add GoDoc for getPluginDirs, defaultSystemPluginDirs
Add some documentation about their purpose, and document order of preference
when resolving plugins.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2024-06-19 14:02:40 +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
Sebastiaan van Stijn dfec976e84
linting: fmt.Errorf can be replaced with errors.New (perfsprint)
internal/test/cli.go:175:14: fmt.Errorf can be replaced with errors.New (perfsprint)
        return nil, fmt.Errorf("no notary client available unless defined")
                    ^
    cli/command/cli.go:318:29: fmt.Errorf can be replaced with errors.New (perfsprint)
            return docker.Endpoint{}, fmt.Errorf("no context store initialized")
                                      ^
    cli/command/container/attach.go:161:11: fmt.Errorf can be replaced with errors.New (perfsprint)
                return fmt.Errorf(result.Error.Message)
                       ^
    cli/command/container/opts.go:577:16: fmt.Errorf can be replaced with errors.New (perfsprint)
                return nil, fmt.Errorf("--health-start-period cannot be negative")
                            ^
    cli/command/container/opts.go:580:16: fmt.Errorf can be replaced with errors.New (perfsprint)
                return nil, fmt.Errorf("--health-start-interval cannot be negative")
                            ^
    cli/command/container/stats.go:221:11: fmt.Errorf can be replaced with errors.New (perfsprint)
                return fmt.Errorf("filtering is not supported when specifying a list of containers")
                       ^
    cli/command/container/attach_test.go:82:17: fmt.Errorf can be replaced with errors.New (perfsprint)
            expectedErr = fmt.Errorf("unexpected error")
                          ^
    cli/command/container/create_test.go:234:40: fmt.Errorf can be replaced with errors.New (perfsprint)
                    return container.CreateResponse{}, fmt.Errorf("shouldn't try to pull image")
                                                       ^
    cli/command/container/list_test.go:150:17: fmt.Errorf can be replaced with errors.New (perfsprint)
                    return nil, fmt.Errorf("error listing containers")
                                ^
    cli/command/container/rm_test.go:40:31: fmt.Errorf can be replaced with errors.New (perfsprint)
                            return errdefs.NotFound(fmt.Errorf("Error: no such container: " + container))
                                                    ^
    cli/command/container/run_test.go:138:40: fmt.Errorf can be replaced with errors.New (perfsprint)
                    return container.CreateResponse{}, fmt.Errorf("shouldn't try to pull image")
                                                       ^
    cli/command/image/pull_test.go:115:49: fmt.Errorf can be replaced with errors.New (perfsprint)
                    return io.NopCloser(strings.NewReader("")), fmt.Errorf("shouldn't try to pull image")
                                                                ^
    cli/command/network/connect.go:88:16: fmt.Errorf can be replaced with errors.New (perfsprint)
                return nil, fmt.Errorf("invalid key/value pair format in driver options")
                            ^
    cli/command/plugin/create_test.go:96:11: fmt.Errorf can be replaced with errors.New (perfsprint)
                return fmt.Errorf("Error creating plugin")
                       ^
    cli/command/plugin/disable_test.go:32:12: fmt.Errorf can be replaced with errors.New (perfsprint)
                    return fmt.Errorf("Error disabling plugin")
                           ^
    cli/command/plugin/enable_test.go:32:12: fmt.Errorf can be replaced with errors.New (perfsprint)
                    return fmt.Errorf("failed to enable plugin")
                           ^
    cli/command/plugin/inspect_test.go:55:22: fmt.Errorf can be replaced with errors.New (perfsprint)
                    return nil, nil, fmt.Errorf("error inspecting plugin")
                                     ^
    cli/command/plugin/install_test.go:43:17: fmt.Errorf can be replaced with errors.New (perfsprint)
                    return nil, fmt.Errorf("Error installing plugin")
                                ^
    cli/command/plugin/install_test.go:51:17: fmt.Errorf can be replaced with errors.New (perfsprint)
                    return nil, fmt.Errorf("(image) when fetching")
                                ^
    cli/command/plugin/install_test.go:95:17: fmt.Errorf can be replaced with errors.New (perfsprint)
                    return nil, fmt.Errorf("should not try to install plugin")
                                ^
    cli/command/plugin/list_test.go:35:41: fmt.Errorf can be replaced with errors.New (perfsprint)
                    return types.PluginsListResponse{}, fmt.Errorf("error listing plugins")
                                                        ^
    cli/command/plugin/remove_test.go:27:12: fmt.Errorf can be replaced with errors.New (perfsprint)
                    return fmt.Errorf("Error removing plugin")
                           ^
    cli/command/registry/login_test.go:36:46: fmt.Errorf can be replaced with errors.New (perfsprint)
            return registrytypes.AuthenticateOKBody{}, fmt.Errorf("Invalid Username or Password")
                                                       ^
    cli/command/registry/login_test.go:44:46: fmt.Errorf can be replaced with errors.New (perfsprint)
            return registrytypes.AuthenticateOKBody{}, fmt.Errorf(errUnknownUser)
                                                       ^
    cli/command/system/info.go:190:10: fmt.Errorf can be replaced with errors.New (perfsprint)
            return fmt.Errorf("errors pretty printing info")
                   ^
    cli/command/system/prune.go:77:10: fmt.Errorf can be replaced with errors.New (perfsprint)
            return fmt.Errorf(`ERROR: The "until" filter is not supported with "--volumes"`)
                   ^
    cli/command/system/version_test.go:19:28: fmt.Errorf can be replaced with errors.New (perfsprint)
                return types.Version{}, fmt.Errorf("no server")
                                        ^
    cli/command/trust/key_load.go:112:22: fmt.Errorf can be replaced with errors.New (perfsprint)
                    return []byte{}, fmt.Errorf("could not decrypt key")
                                     ^
    cli/command/trust/revoke.go:44:10: fmt.Errorf can be replaced with errors.New (perfsprint)
            return fmt.Errorf("cannot use a digest reference for IMAGE:TAG")
                   ^
    cli/command/trust/revoke.go:105:10: fmt.Errorf can be replaced with errors.New (perfsprint)
            return fmt.Errorf("no signed tags to remove")
                   ^
    cli/command/trust/signer_add.go:56:10: fmt.Errorf can be replaced with errors.New (perfsprint)
            return fmt.Errorf("releases is a reserved keyword, please use a different signer name")
                   ^
    cli/command/trust/signer_add.go:60:10: fmt.Errorf can be replaced with errors.New (perfsprint)
            return fmt.Errorf("path to a public key must be provided using the `--key` flag")
                   ^
    opts/config.go:71:10: fmt.Errorf can be replaced with errors.New (perfsprint)
            return fmt.Errorf("source is required")
                   ^
    opts/mount.go:168:10: fmt.Errorf can be replaced with errors.New (perfsprint)
            return fmt.Errorf("type is required")
                   ^
    opts/mount.go:172:10: fmt.Errorf can be replaced with errors.New (perfsprint)
            return fmt.Errorf("target is required")
                   ^
    opts/network.go:90:11: fmt.Errorf can be replaced with errors.New (perfsprint)
                return fmt.Errorf("network name/id is not specified")
                       ^
    opts/network.go:129:18: fmt.Errorf can be replaced with errors.New (perfsprint)
            return "", "", fmt.Errorf("invalid key value pair format in driver options")
                           ^
    opts/opts.go:404:13: fmt.Errorf can be replaced with errors.New (perfsprint)
            return 0, fmt.Errorf("value is too precise")
                      ^
    opts/opts.go:412:18: fmt.Errorf can be replaced with errors.New (perfsprint)
            return "", "", fmt.Errorf("empty string specified for links")
                           ^
    opts/parse.go:84:37: fmt.Errorf can be replaced with errors.New (perfsprint)
            return container.RestartPolicy{}, fmt.Errorf("invalid restart policy format: no policy provided before colon")
                                              ^
    opts/parse.go:89:38: fmt.Errorf can be replaced with errors.New (perfsprint)
                return container.RestartPolicy{}, fmt.Errorf("invalid restart policy format: maximum retry count must be an integer")
                                                  ^
    opts/port.go:105:13: fmt.Errorf can be replaced with errors.New (perfsprint)
                        return fmt.Errorf("hostip is not supported")
                               ^
    opts/secret.go:70:10: fmt.Errorf can be replaced with errors.New (perfsprint)
            return fmt.Errorf("source is required")
                   ^
    opts/env_test.go:57:11: fmt.Errorf can be replaced with errors.New (perfsprint)
                err:   fmt.Errorf("invalid environment variable: =a"),
                       ^
    opts/env_test.go:93:11: fmt.Errorf can be replaced with errors.New (perfsprint)
                err:   fmt.Errorf("invalid environment variable: ="),
                       ^
    cli-plugins/manager/error_test.go:16:11: fmt.Errorf can be replaced with errors.New (perfsprint)
        inner := fmt.Errorf("testing")
                 ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2024-06-10 21:19:31 +02:00
Paweł Gronowski 296a6f5872
plugins/hooks: Don't show empty hooks
Don't show `Next steps:` with no messages at all when plugin returns an
unitialized value of `HookMessage` (zero-initialization sets its type to
NextSteps and empty template).

Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
2024-05-20 11:20:39 +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
Alano Terblanche 67aa271410
Merge pull request #5039 from Benehiko/hooks-ctx-wiring
feat: wire ctx into plugin hooks
2024-04-29 14:57:48 +02: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 5ccb48459b
cli-plugins: PluginRunCommand: use cmd.Environ instead of os.Environ
Commit 5011759056 implemented a fix that
caused the current environment to be discarded, using `os.Environ()`.
On Windows, `os.Environ()` may produce an incorrect value for `PWD`,
for which a new function was added in go1.19;

- https://tip.golang.org/doc/go1.19#osexecpkgosexec
- https://go-review.googlesource.com/c/go/+/401340

Replace the use of `os.Environ()` with `cmd.Environ()` to address that.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2024-04-25 19:27:05 +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 5011759056
hooks: set expected environment when executing
During normal plugin execution (from the CLI), the CLI configures the
plugin command it's about to execute in order to pass all environment
variables on, as well as to set the ReExec env var that informs the
plugin about how it was executed, and which plugins rely on to check
whether they are being run standalone or not.

This commit adds the same behavior to hook invocations, which is
necessary for some plugins to know that they are not running standalone
so that they expose their root command at the correct level.

Signed-off-by: Laura Brehm <laurabrehm@hey.com>
2024-04-17 16:57:44 +01:00
Laura Brehm 867061b007
plugins/templates: break on newlines when printing hooks
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
2024-04-15 12:59:53 +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
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 509123f935
plugin: drop explicit unlink
Go's `net` package [will unlink][1] for us, as long as we used Listen &
friends to create the Unix socket.

Go will even skip the unlink when the socket appears to be abstract
(starts with a NUL, represented by an @), though we must be cautious to
only create sockets with an abstract address on platforms that actually
support it -- this caused [several][2] [bugs][3] before.

  [1]: https://pkg.go.dev/net#UnixListener.SetUnlinkOnClose
  [2]: https://github.com/docker/cli/pull/4783
  [3]: https://github.com/docker/cli/pull/4863

Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
2024-03-22 10:12:40 -06: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
Tonis Tiigi 5786f20687
plugins: fix encoding for OTEL env var passed to plugin
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
2024-02-28 12:43:07 -08: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
Sebastiaan van Stijn 3b5e814242
cli-plugins/socket: remove use of deprecated distribution uuid package
The "github.com/docker/distribution" module moved to the distribution
org ("github.com/docker/distribution/v3"), and the new module deprecated
and removed the uuid package in favor of Google's UUID package.

While we still depend on the old module through packages and as an indirect
dependency, we may want to try avoid using it.

This patch replaces the use for the socket package, and replaces it for a
local utility, taking the same approach as `stringid.GenerateRandomID()`,
which should be random enough for this purpose.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2024-02-13 16:24:53 +01:00
Kirill A. Korinsky 2c214241fa
Avoid keeping @docker_cli_[UUID] files
Seems that OpenBSD behaves like darwin and requires to unlink all
socket, after it was used.

Tested on OpenBSD 7.4

Signed-off-by: Kirill A. Korinsky <kirill@korins.ky>
2024-02-09 00:19:27 +01:00
Laura Brehm cfa9fef77d
tests: add plugin-socket-compatibility tests
Adds a new plugin to the e2e plugins that simulates an older
plugin binary and a test suite to ensure older plugin binaries
keep behaving the same with newer CLI versions.

Signed-off-by: Laura Brehm <laurabrehm@hey.com>
2024-01-29 13:39:58 +00:00
Laura Brehm 469bfc05ed
tests: add tests for `cli-plugins/socket`
Signed-off-by: Laura Brehm <laurabrehm@hey.com>
2024-01-23 14:19:33 +00:00
Tonis Tiigi 8cd3b00420
socket: return from loop after EOF
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
2024-01-19 17:06:43 -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 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 6d0b329b0d
cli-plugins: use non-abstract socket on darwin
As macOS does not support the abstract socket namespace, use a temporary
socket in $TMPDIR to connect with the plugin. Ensure this socket is
cleaned up even in the case of crash/ungraceful termination by removing
it after the first connection is accepted.

Co-authored-by: Laura Brehm <laurabrehm@hey.com>
Signed-off-by: Bjorn Neergaard <bjorn.neergaard@docker.com>
2024-01-12 12:30:27 -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 76e09dd44b
Merge pull request #4777 from thaJeztah/pluginmanager_smaller_interface
cli-plugins/manager: getPluginDirs: take ConfigFile as argument
2024-01-11 22:50:31 +01:00
Sebastiaan van Stijn c0a0b05dc8
cli-plugins: helloworld: rename var that collided with import
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2024-01-11 22:25:15 +01:00
Sebastiaan van Stijn 4dc2c895b1
cli-plugins/manager: getPluginDirs: take ConfigFile as argument
Update this function to accept a smaller interface, as it doesn't need
all of "CLI".

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2024-01-11 18:15:30 +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 7af509c7f1
cli/command: merge DockerCliOption and InitializeOpt types
The cli/command package defined two option-types with the same signature.

This patch creates a new type instead (CLIOption), and makes the existing
types an alias for this (deprecating their old names).

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-12-12 15:01:36 +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 a2c9f3c6ce
linting: address else/if/elseif statements found by gocritic
cli/command/formatter/tabwriter/tabwriter.go:579:10: elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic)
              } else {
                     ^
    cli/connhelper/connhelper.go:43:2: singleCaseSwitch: should rewrite switch statement to if statement (gocritic)
    	switch scheme := u.Scheme; scheme {
    	^
    cli/compose/loader/loader.go:666:10: elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic)
    		} else {
    		       ^
    opts/hosts_test.go:173:10: elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic)
    		} else {
    		       ^
    cli-plugins/manager/candidate_test.go:78:4: ifElseChain: rewrite if-else to switch statement (gocritic)
    			if tc.err != "" {
    			^
    cli/command/checkpoint/formatter.go:15:2: singleCaseSwitch: should rewrite switch statement to if statement (gocritic)
    	switch source {
    	^
    cli/command/image/formatter_history.go:25:2: singleCaseSwitch: should rewrite switch statement to if statement (gocritic)
    	switch source {
    	^
    cli/command/service/scale.go:107:2: ifElseChain: rewrite if-else to switch statement (gocritic)
    	if serviceMode.Replicated != nil {
    	^
    cli/command/service/update.go:804:9: elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic)
    	} else {
    	       ^
    cli/command/service/update.go:222:2: ifElseChain: rewrite if-else to switch statement (gocritic)
    	if sendAuth {
    	^
    cli/command/container/formatter_diff.go:17:2: singleCaseSwitch: should rewrite switch statement to if statement (gocritic)
    	switch source {
    	^
    cli/command/container/start.go:79:2: ifElseChain: rewrite if-else to switch statement (gocritic)
    	if opts.Attach || opts.OpenStdin {
    	^
    cli/command/container/utils.go:84:11: elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic)
    			} else {
    			       ^
    cli/command/container/exec_test.go:200:11: elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic)
    			} else {
    			       ^
    cli/command/container/logs_test.go:52:11: elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic)
    			} else {
    			       ^
    cli/command/container/opts_test.go:1014:10: elseif: can replace 'else {if cond {}}' with 'else if cond {}' (gocritic)
    		} else {
    		       ^
    cli/command/system/info.go:297:7: singleCaseSwitch: should rewrite switch statement to if statement (gocritic)
    						switch o.Key {
    						^
    cli/command/system/version.go:164:4: singleCaseSwitch: should rewrite switch statement to if statement (gocritic)
    			switch component.Name {
    			^
    cli/command/system/info_test.go:478:4: ifElseChain: rewrite if-else to switch statement (gocritic)
    			if tc.expectedOut != "" {
    			^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-11-20 16:02:16 +01:00
Sebastiaan van Stijn d0dee3cebe
linting: Consider pre-allocating sliceVar (prealloc)
While updating, also addressed some redundant fmt.Sprintf()

    opts/throttledevice.go:86:2: Consider pre-allocating `out` (prealloc)
        var out []string
        ^
    opts/ulimit.go:37:2: Consider pre-allocating `out` (prealloc)
        var out []string
        ^
    opts/ulimit.go:47:2: Consider pre-allocating `ulimits` (prealloc)
        var ulimits []*units.Ulimit
        ^
    opts/weightdevice.go:68:2: Consider pre-allocating `out` (prealloc)
        var out []string
        ^
    cli/context/store/metadatastore.go:96:2: Consider pre-allocating `res` (prealloc)
        var res []Metadata
        ^
    cli/context/store/store.go:127:2: Consider pre-allocating `names` (prealloc)
        var names []string
        ^
    cli/compose/loader/loader.go:223:2: Consider pre-allocating `keys` (prealloc)
        var keys []string
        ^
    cli/compose/loader/loader.go:397:2: Consider pre-allocating `services` (prealloc)
        var services []types.ServiceConfig
        ^
    cli/command/stack/loader/loader.go:63:2: Consider pre-allocating `msgs` (prealloc)
        var msgs []string
        ^
    cli/command/stack/loader/loader.go:118:2: Consider pre-allocating `configFiles` (prealloc)
        var configFiles []composetypes.ConfigFile
        ^
    cli/command/formatter/container.go:245:2: Consider pre-allocating `joinLabels` (prealloc)
        var joinLabels []string
        ^
    cli/command/formatter/container.go:265:2: Consider pre-allocating `mounts` (prealloc)
        var mounts []string
        ^
    cli/command/formatter/container.go:316:2: Consider pre-allocating `result` (prealloc)
        var result []string
        ^
    cli/command/formatter/displayutils.go:43:2: Consider pre-allocating `display` (prealloc)
        var (
        ^
    cli/command/formatter/volume.go:103:2: Consider pre-allocating `joinLabels` (prealloc)
        var joinLabels []string
        ^
    cli-plugins/manager/manager_test.go:49:2: Consider pre-allocating `dirs` (prealloc)
        var dirs []string
        ^
    cli/command/swarm/init.go:69:2: Consider pre-allocating `defaultAddrPool` (prealloc)
        var defaultAddrPool []string
        ^
    cli/command/manifest/push.go:195:2: Consider pre-allocating `blobReqs` (prealloc)
        var blobReqs []manifestBlob
        ^
    cli/command/secret/formatter.go:111:2: Consider pre-allocating `joinLabels` (prealloc)
        var joinLabels []string
        ^
    cli/command/network/formatter.go:104:2: Consider pre-allocating `joinLabels` (prealloc)
        var joinLabels []string
        ^
    cli/command/context/list.go:52:2: Consider pre-allocating `contexts` (prealloc)
        var contexts []*formatter.ClientContext
        ^
    cli/command/config/formatter.go:104:2: Consider pre-allocating `joinLabels` (prealloc)
        var joinLabels []string
        ^
    cli/command/trust/common_test.go:23:2: Consider pre-allocating `targetNames` (prealloc)
        var targetNames []string
        ^
    cli/command/service/generic_resource_opts.go:55:2: Consider pre-allocating `generic` (prealloc)
        var generic []swarm.GenericResource
        ^
    cli/command/service/generic_resource_opts.go:98:2: Consider pre-allocating `l` (prealloc)
        var l []swarm.GenericResource
        ^
    cli/command/service/opts.go:378:2: Consider pre-allocating `netAttach` (prealloc)
        var netAttach []swarm.NetworkAttachmentConfig
        ^
    cli/command/service/update.go:731:2: Consider pre-allocating `limits` (prealloc)
        var limits []*units.Ulimit
        ^
    cli/command/service/update.go:1315:2: Consider pre-allocating `newNetworks` (prealloc)
        var newNetworks []swarm.NetworkAttachmentConfig
        ^
    cli/command/service/update.go:1514:2: Consider pre-allocating `out` (prealloc)
        var out []string
        ^
    cli/compose/convert/service.go:713:2: Consider pre-allocating `ulimits` (prealloc)
        var ulimits []*units.Ulimit
        ^
    cli/compose/convert/volume.go:13:2: Consider pre-allocating `mounts` (prealloc)
        var mounts []mount.Mount
        ^
    cli/command/stack/swarm/list.go:39:2: Consider pre-allocating `stacks` (prealloc)
        var stacks []*formatter.Stack
        ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2023-11-20 16:02:16 +01: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 6a50c4f700
cli-plugins: remove deprecated Metadata.Experimental
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>
2023-06-12 14:53:30 +02:00