Commit Graph

67 Commits

Author SHA1 Message Date
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
Sebastiaan van Stijn 4cf04988ae
remove uses of golang.org/x/sys/execabs
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>
2023-05-26 02:03:45 +02:00
Sebastiaan van Stijn 10bade23e1
Merge pull request #4261 from thaJeztah/remove_old_buildtags
remove pre-go1.17 build-tags
2023-05-16 18:12:50 +01:00
Sebastiaan van Stijn 72e3813ab9
cli-plugins/manager: fix deprecation comment of Metadata.Experimental
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>
2023-05-09 22:17:27 +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
Kevin Alvarez 89583b92b7
improve plugins listing performance
We can slightly improve plugins listing by spawning a
goroutine for each iteration.

Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
2023-04-01 16:02:34 +02:00
CrazyMax e14f5fc1a7
revert "improve plugins discovery performance"
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>
2023-03-31 16:20:42 +02:00
Kevin Alvarez c39c711a18
load plugin command stubs when required
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>
2023-03-28 06:16:55 +02:00
CrazyMax 62f2358b99
improve plugins discovery performance
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>
2023-03-28 06:16:55 +02:00
Sebastiaan van Stijn fc6be6ad30
cli: pass dockerCLI's in/out/err to cobra cmds
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>
2023-01-15 13:44:33 +01:00
Sebastiaan van Stijn 5aba4860de
cli-plugins/manager: TestPluginError: don't use yaml.Marshal
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>
2022-11-17 13:17:51 +01:00
Sebastiaan van Stijn 616124525e
format go with gofumpt (with -lang=1.19)
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>
2022-09-30 19:14:36 +02:00
Sebastiaan van Stijn 1da95ff6aa
format code with gofumpt
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2022-09-30 11:59:11 +02:00
Sebastiaan van Stijn 90f1238fb2
cli-plugins/manager: add IsPluginCommand(() utility
This makes it more convenient to check if a command is a plugin-stub

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2022-09-30 02:24:23 +02:00
Sebastiaan van Stijn 491407b541
linting: remove unused nolint comments (nolintlint)
cli/context/store/tlsstore.go:46:88: directive `//nolint:unused` is unused for linter "unused" (nolintlint)
    func (s *tlsStore) remove(contextID contextdir, endpointName, filename string) error { //nolint:unused
                                                                                           ^
    cli-plugins/manager/plugin.go:37:1: directive `//nolint:gocyclo` is unused for linter "gocyclo" (nolintlint)
    //nolint:gocyclo
    ^
    cli/command/image/formatter_history_test.go:189:2: directive `//nolint:lll` is unused for linter "lll" (nolintlint)
        //nolint:lll
        ^
    cli/command/service/list.go:113:1: directive `//nolint:gocyclo` is unused for linter "gocyclo" (nolintlint)
    //nolint:gocyclo
    ^
    cli/command/stack/swarm/deploy_composefile.go:178:1: directive `//nolint:gocyclo` is unused for linter "gocyclo" (nolintlint)
    //nolint:gocyclo
    ^

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2022-09-03 21:25:34 +02:00
Sebastiaan van Stijn 82427d1a07
format (GoDoc) comments with Go 1.19 to prepare for go updates
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>
2022-07-19 19:10:16 +02:00
Nicolas De Loof cbec75e2f3
Adopt Cobra completion v2 to support completion by CLI plugins
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
2022-05-12 12:59:10 +02:00
Sebastiaan van Stijn a0f0578299
gofmt with go1.17
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2022-03-26 20:21:00 +01:00
Sebastiaan van Stijn 6c06950cdf
cli-plugins/manager: remove uses of deprecated io/ioutil
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2022-02-25 16:01:20 +01:00
Nicolas De Loof d9a2a555d5
introduce plugin.RunningStandalone as utility func for a plugin to know it is run standalone
Signed-off-by: Nicolas De Loof <nicolas.deloof@gmail.com>
2022-02-23 10:20:23 +01:00
CrazyMax 6fef143dbc
Set buildx as default builder
Signed-off-by: CrazyMax <crazy-max@users.noreply.github.com>
2022-02-03 10:38:05 +01:00
Tibor Vass 8d199d5bba Use golang.org/x/sys/execabs
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>
2021-01-26 17:18:04 +00:00
Sebastiaan van Stijn 977d3ae046
Always enable experimental features
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>
2020-10-02 15:59:42 +02:00
Sebastiaan van Stijn 6703919c71
Replace vbom.ml/util with fvbommel/sortorder
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>
2020-08-28 15:35:42 +02:00
Silvin Lubecki 459c6082f8 Removing CLI plugins API documentation from official documentation as it is now deprecated.
Adding depreciation notice.

Signed-off-by: Silvin Lubecki <silvin.lubecki@docker.com>
2020-05-19 10:37:35 +02:00
Sebastiaan van Stijn bb7ef2cb3a
Update some uses of errors.Cause() to errors.Is()
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2020-05-10 19:56:35 +02:00
Sebastiaan van Stijn 719169db63
Replace deprecated Cobra command.SetOutput() with command.SetOut()
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2020-05-07 14:25:59 +02:00
Tonis Tiigi d2632cea78 plugin: make runplugin public
Signed-off-by: Tonis Tiigi <tonistiigi@gmail.com>
2020-04-28 15:48:13 -07:00
Sebastiaan van Stijn 2c0e93063b
bump gotest.tools v3.0.1 for compatibility with Go 1.14
full diff: https://github.com/gotestyourself/gotest.tools/compare/v2.3.0...v3.0.1

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
2020-02-23 00:28:55 +01:00
Sebastiaan van Stijn 54ba28f402
docker info: list CLI plugins alphabetically
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>
2019-12-31 14:33:07 +01:00
Tibor Vass bb8e89bb2e cli-plugins: add test names for easier debugging
Signed-off-by: Tibor Vass <tibor@docker.com>
2019-05-22 15:35:34 +00:00
Tibor Vass 6ca8783730 cli-plugins: add concept of experimental plugin, only enabled in experimental mode
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>
2019-05-22 15:35:34 +00:00
Ian Campbell af200f14ed cli-plugins: fix when plugin does not use PersistentPreRun* hooks
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>
2019-04-30 10:45:29 +01:00
Tibor Vass 1ed02c40fe cli-plugins: alias an existing allowed command (only builder for now)
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>
2019-04-19 01:26:45 +00:00
Ian Campbell 1425aeba4a cli-plugins: only parse global arguments once.
This fixes `TestGlobalArgsOnlyParsedOnce/plugin` in the cli-plugins e2e tests.

Signed-off-by: Ian Campbell <ijc@docker.com>
2019-04-03 15:07:38 +01:00
Stefan Scherer 4d3a76d71e
Search Windows CLI plugins also in ProgramFiles
Signed-off-by: Stefan Scherer <stefan.scherer@docker.com>
2019-03-20 11:45:03 -07:00
Ian Campbell ff2ed6efa8 cli-plugins: disable use of dial-stdio
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>
2019-03-18 10:58:51 +00:00
Ian Campbell 3af168c7df Ensure plugins can use PersistentPreRunE again.
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>
2019-03-14 14:29:09 +00:00
Ian Campbell 0b794e0620 e2e/cli-plugins: explicitly check that PersistentPreRunE works
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>
2019-03-14 14:20:19 +00:00
Ian Campbell d4ced2ef77 allow plugins to have argument which match a top-level flag.
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>
2019-03-13 11:28:17 +00:00
Ian Campbell 8289ae03f8 Add e2e tests for plugin vs global argument issues
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>
2019-03-13 10:54:31 +00:00
Ian Campbell 0449ad8d06 Revert "Disable `docker system dial-stdio` on Windows"
This reverts commit c41c23813c.

This case is now handled due to the previous commit.

Signed-off-by: Ian Campbell <ijc@docker.com>
2019-03-12 14:57:52 +00:00
Silvin Lubecki b86bff84b6
Merge pull request #1710 from ijc/no-dial-stdio-on-windows
Disable `docker system dial-stdio` on Windows
2019-03-11 15:14:23 +01:00
Nick Adcock ff51b0d77d harden config.Path() to disallow directory traversal
Signed-off-by: Nick Adcock <nick.adcock@docker.com>
2019-03-07 14:40:53 +00:00
Ian Campbell c41c23813c Disable `docker system dial-stdio` on Windows
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>
2019-03-04 17:36:21 +00:00
Corey Quon d871451049
Fix issue where plugin command error exit code is printed out
Signed-off-by: Corey Quon <corey.quon@docker.com>
2019-02-26 09:39:07 -08:00