From 891b3d953e43fa3e3ae9242365399aa7a3b071dc Mon Sep 17 00:00:00 2001 From: Ian Campbell Date: Thu, 31 Jan 2019 17:50:58 +0000 Subject: [PATCH] cli-plugins: use `docker system dial-stdio` to call the daemon This means that plugins can use whatever methods the monolithic CLI supports, which is good for consistency. This relies on `os.Args[0]` being something which can be executed again to reach the same binary, since it is propagated (via an envvar) to the plugin for this purpose. This essentially requires that the current working directory and path are not modified by the monolithic CLI before it launches the plugin nor by the plugin before it initializes the client. This should be the case. Previously the fake apiclient used by `TestExperimentalCLI` was not being used, since `cli.Initialize` was unconditionally overwriting it with a real one (talking to a real daemon during unit testing, it seems). This wasn't expected nor desirable and no longer happens with the new arrangements, exposing the fact that no `pingFunc` is provided, leading to a panic. Add a `pingFunc` to the fake client to avoid this. Signed-off-by: Ian Campbell --- cli-plugins/manager/manager.go | 9 +++++ cli-plugins/plugin/plugin.go | 40 ++++++++++++++++++++- cli/command/cli.go | 65 ++++++++++++++++++++++------------ cli/command/cli_test.go | 3 ++ cli/connhelper/connhelper.go | 10 ++++++ docs/extend/cli_plugins.md | 13 +++++++ e2e/cli-plugins/dial_test.go | 26 ++++++++++++++ 7 files changed, 143 insertions(+), 23 deletions(-) create mode 100644 e2e/cli-plugins/dial_test.go diff --git a/cli-plugins/manager/manager.go b/cli-plugins/manager/manager.go index 6fdc582c97..21d8ed0f19 100644 --- a/cli-plugins/manager/manager.go +++ b/cli-plugins/manager/manager.go @@ -12,6 +12,12 @@ import ( "github.com/spf13/cobra" ) +// ReexecEnvvar is the name of an ennvar which is set to the command +// used to originally invoke the docker CLI when executing a +// plugin. Assuming $PATH and $CWD remain unchanged this should allow +// the plugin to re-execute the original CLI. +const ReexecEnvvar = "DOCKER_CLI_PLUGIN_ORIGINAL_CLI_COMMAND" + // errPluginNotFound is the error returned when a plugin could not be found. type errPluginNotFound string @@ -155,6 +161,9 @@ func PluginRunCommand(dockerCli command.Cli, name string, rootcmd *cobra.Command cmd.Stdout = os.Stdout cmd.Stderr = os.Stderr + cmd.Env = os.Environ() + cmd.Env = append(cmd.Env, ReexecEnvvar+"="+os.Args[0]) + return cmd, nil } return nil, errPluginNotFound(name) diff --git a/cli-plugins/plugin/plugin.go b/cli-plugins/plugin/plugin.go index c11780b081..99ef36d845 100644 --- a/cli-plugins/plugin/plugin.go +++ b/cli-plugins/plugin/plugin.go @@ -9,7 +9,9 @@ import ( "github.com/docker/cli/cli" "github.com/docker/cli/cli-plugins/manager" "github.com/docker/cli/cli/command" + "github.com/docker/cli/cli/connhelper" cliflags "github.com/docker/cli/cli/flags" + "github.com/docker/docker/client" "github.com/spf13/cobra" "github.com/spf13/pflag" ) @@ -49,6 +51,7 @@ func Run(makeCmd func(command.Cli) *cobra.Command, meta manager.Metadata) { // own use of that hook will shadow anything we add to the top-level // command meaning the CLI is never Initialized. var options struct { + name string init, prerun sync.Once opts *cliflags.ClientOptions flags *pflag.FlagSet @@ -71,11 +74,43 @@ func PersistentPreRunE(cmd *cobra.Command, args []string) error { } // flags must be the original top-level command flags, not cmd.Flags() options.opts.Common.SetDefaultOptions(options.flags) - err = options.dockerCli.Initialize(options.opts) + err = options.dockerCli.Initialize(options.opts, withPluginClientConn(options.name)) }) return err } +func withPluginClientConn(name string) command.InitializeOpt { + return command.WithInitializeClient(func(dockerCli *command.DockerCli) (client.APIClient, error) { + cmd := "docker" + if x := os.Getenv(manager.ReexecEnvvar); x != "" { + cmd = x + } + var flags []string + + // Accumulate all the global arguments, that is those + // up to (but not including) the plugin's name. This + // ensures that `docker system dial-stdio` is + // evaluating the same set of `--config`, `--tls*` etc + // global options as the plugin was called with, which + // in turn is the same as what the original docker + // invocation was passed. + for _, a := range os.Args[1:] { + if a == name { + break + } + flags = append(flags, a) + } + flags = append(flags, "system", "dial-stdio") + + helper, err := connhelper.GetCommandConnectionHelper(cmd, flags...) + if err != nil { + return nil, err + } + + return client.NewClientWithOpts(client.WithDialContext(helper.Dialer)) + }) +} + func newPluginCommand(dockerCli *command.DockerCli, plugin *cobra.Command, meta manager.Metadata) *cobra.Command { name := plugin.Name() fullname := manager.NamePrefix + name @@ -101,6 +136,7 @@ func newPluginCommand(dockerCli *command.DockerCli, plugin *cobra.Command, meta cli.DisableFlagsInUseLine(cmd) options.init.Do(func() { + options.name = name options.opts = opts options.flags = flags options.dockerCli = dockerCli @@ -115,6 +151,8 @@ func newMetadataSubcommand(plugin *cobra.Command, meta manager.Metadata) *cobra. cmd := &cobra.Command{ Use: manager.MetadataSubcommandName, Hidden: true, + // Suppress the global/parent PersistentPreRunE, which needlessly initializes the client and tries to connect to the daemon. + PersistentPreRun: func(cmd *cobra.Command, args []string) {}, RunE: func(cmd *cobra.Command, args []string) error { enc := json.NewEncoder(os.Stdout) enc.SetEscapeHTML(false) diff --git a/cli/command/cli.go b/cli/command/cli.go index 28936fc3bf..47e9bdfffa 100644 --- a/cli/command/cli.go +++ b/cli/command/cli.go @@ -175,9 +175,28 @@ func (cli *DockerCli) RegistryClient(allowInsecure bool) registryclient.Registry return registryclient.NewRegistryClient(resolver, UserAgent(), allowInsecure) } +// InitializeOpt is the type of the functional options passed to DockerCli.Initialize +type InitializeOpt func(dockerCli *DockerCli) error + +// WithInitializeClient is passed to DockerCli.Initialize by callers who wish to set a particular API Client for use by the CLI. +func WithInitializeClient(makeClient func(dockerCli *DockerCli) (client.APIClient, error)) InitializeOpt { + return func(dockerCli *DockerCli) error { + var err error + dockerCli.client, err = makeClient(dockerCli) + return err + } +} + // Initialize the dockerCli runs initialization that must happen after command // line flags are parsed. -func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions) error { +func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions, ops ...InitializeOpt) error { + var err error + + for _, o := range ops { + if err := o(cli); err != nil { + return err + } + } cliflags.SetLogLevel(opts.Common.LogLevel) if opts.ConfigDir != "" { @@ -189,29 +208,31 @@ func (cli *DockerCli) Initialize(opts *cliflags.ClientOptions) error { } cli.configFile = cliconfig.LoadDefaultConfigFile(cli.err) - var err error - cli.contextStore = store.New(cliconfig.ContextStoreDir(), cli.contextStoreConfig) - cli.currentContext, err = resolveContextName(opts.Common, cli.configFile, cli.contextStore) - if err != nil { - return err - } - endpoint, err := resolveDockerEndpoint(cli.contextStore, cli.currentContext, opts.Common) - if err != nil { - return errors.Wrap(err, "unable to resolve docker endpoint") - } - cli.dockerEndpoint = endpoint - cli.client, err = newAPIClientFromEndpoint(endpoint, cli.configFile) - if tlsconfig.IsErrEncryptedKey(err) { - passRetriever := passphrase.PromptRetrieverWithInOut(cli.In(), cli.Out(), nil) - newClient := func(password string) (client.APIClient, error) { - endpoint.TLSPassword = password - return newAPIClientFromEndpoint(endpoint, cli.configFile) + if cli.client == nil { + cli.contextStore = store.New(cliconfig.ContextStoreDir(), cli.contextStoreConfig) + cli.currentContext, err = resolveContextName(opts.Common, cli.configFile, cli.contextStore) + if err != nil { + return err + } + endpoint, err := resolveDockerEndpoint(cli.contextStore, cli.currentContext, opts.Common) + if err != nil { + return errors.Wrap(err, "unable to resolve docker endpoint") + } + cli.dockerEndpoint = endpoint + + cli.client, err = newAPIClientFromEndpoint(endpoint, cli.configFile) + if tlsconfig.IsErrEncryptedKey(err) { + passRetriever := passphrase.PromptRetrieverWithInOut(cli.In(), cli.Out(), nil) + newClient := func(password string) (client.APIClient, error) { + endpoint.TLSPassword = password + return newAPIClientFromEndpoint(endpoint, cli.configFile) + } + cli.client, err = getClientWithPassword(passRetriever, newClient) + } + if err != nil { + return err } - cli.client, err = getClientWithPassword(passRetriever, newClient) - } - if err != nil { - return err } var experimentalValue string // Environment variable always overrides configuration diff --git a/cli/command/cli_test.go b/cli/command/cli_test.go index e83941ffc6..0bbe6df055 100644 --- a/cli/command/cli_test.go +++ b/cli/command/cli_test.go @@ -175,6 +175,9 @@ func TestExperimentalCLI(t *testing.T) { defer dir.Remove() apiclient := &fakeClient{ version: defaultVersion, + pingFunc: func() (types.Ping, error) { + return types.Ping{Experimental: true, OSType: "linux", APIVersion: defaultVersion}, nil + }, } cli := &DockerCli{client: apiclient, err: os.Stderr} diff --git a/cli/connhelper/connhelper.go b/cli/connhelper/connhelper.go index e852d02680..94cc1d515b 100644 --- a/cli/connhelper/connhelper.go +++ b/cli/connhelper/connhelper.go @@ -53,6 +53,16 @@ func GetConnectionHelper(daemonURL string) (*ConnectionHelper, error) { return nil, err } +// GetCommandConnectionHelper returns a ConnectionHelp constructed from an arbitrary command. +func GetCommandConnectionHelper(cmd string, flags ...string) (*ConnectionHelper, error) { + return &ConnectionHelper{ + Dialer: func(ctx context.Context, network, addr string) (net.Conn, error) { + return newCommandConn(ctx, cmd, flags...) + }, + Host: "http://docker", + }, nil +} + func newCommandConn(ctx context.Context, cmd string, args ...string) (net.Conn, error) { var ( c commandConn diff --git a/docs/extend/cli_plugins.md b/docs/extend/cli_plugins.md index a99204ca58..c47d93809e 100644 --- a/docs/extend/cli_plugins.md +++ b/docs/extend/cli_plugins.md @@ -75,6 +75,19 @@ A plugin is required to support all of the global options of the top-level CLI, i.e. those listed by `man docker 1` with the exception of `-v`. +## Connecting to the docker engine + +For consistency plugins should prefer to dial the engine by using the +`system dial-stdio` subcommand of the main Docker CLI binary. + +To facilitate this plugins will be executed with the +`$DOCKER_CLI_PLUGIN_ORIGINAL_CLI_COMMAND` environment variable +pointing back to the main Docker CLI binary. + +All global options (everything from after the binary name up to, but +not including, the primary entry point subcommand name) should be +passed back to the CLI. + ## Installation Plugins distributed in packages for system wide installation on diff --git a/e2e/cli-plugins/dial_test.go b/e2e/cli-plugins/dial_test.go new file mode 100644 index 0000000000..5c3d14992e --- /dev/null +++ b/e2e/cli-plugins/dial_test.go @@ -0,0 +1,26 @@ +package cliplugins + +import ( + "os" + "path/filepath" + "testing" + + "github.com/docker/cli/cli-plugins/manager" + "gotest.tools/assert" + is "gotest.tools/assert/cmp" + "gotest.tools/icmd" +) + +func TestDialStdio(t *testing.T) { + // Run the helloworld plugin forcing /bin/true as the `system + // dial-stdio` target. It should be passed all arguments from + // before the `helloworld` arg, but not the --who=foo which + // follows. We observe this from the debug level logging from + // the connhelper stuff. + helloworld := filepath.Join(os.Getenv("DOCKER_CLI_E2E_PLUGINS_EXTRA_DIRS"), "docker-helloworld") + cmd := icmd.Command(helloworld, "--config=blah", "--tls", "--log-level", "debug", "helloworld", "--who=foo") + res := icmd.RunCmd(cmd, icmd.WithEnv(manager.ReexecEnvvar+"=/bin/true")) + res.Assert(t, icmd.Success) + assert.Assert(t, is.Contains(res.Stderr(), `msg="connhelper: starting /bin/true with [--config=blah --tls --log-level debug system dial-stdio]"`)) + assert.Assert(t, is.Equal(res.Stdout(), "Hello foo!\n")) +}