cli-plugins: use system dial-stdio to contact the engine. #1654

ijc · 2019-02-01T14:11:07Z

- What I did

Arranged so that cli-plugins will use docker system dial-stdio to connect to the engine as proposed by @tonistiigi in #1534 (comment).

- How I did it

The main commit message goes into some detail:

    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 <[email protected]>

It's a bit tricky, so could use some careful thought around the possible pitfalls.

Things I've considered doing differently:

Passing the original CLI via a new global argument instead of an envvar.
Passing the entire dial-stdio command line to use explicitly to the plugin. Not sure how best to do that without getting into confusion around quoting all the way through.

- How to verify it

There is an e2e test which confirms correct argument parsing. Also the existing TestCliInitialized happens to use the mechanism.

- Description for the changelog

N/A should be covered by #1564's entry.

- A picture of a cute animal (not mandatory but encouraged)

This can be obtained with the `.Name()` method. Signed-off-by: Ian Campbell <[email protected]>

codecov-io · 2019-02-01T14:21:18Z

Codecov Report

Merging #1654 into master will decrease coverage by 0.07%.
The diff coverage is 5.71%.

@@            Coverage Diff             @@
##           master    #1654      +/-   ##
==========================================
- Coverage   56.12%   56.04%   -0.08%     
==========================================
  Files         306      306              
  Lines       20964    20925      -39     
==========================================
- Hits        11766    11728      -38     
- Misses       8345     8354       +9     
+ Partials      853      843      -10

cli/command/cli.go

thaJeztah · 2019-02-01T15:10:39Z

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) {


Wondering why we didn't call the other one NewConnectionHelper (and this one NewCommandConnectionHelper)

(just thinking out loud 🤔)

thaJeztah · 2019-02-01T15:12:57Z

cli/connhelper/connhelper.go

+		Dialer: func(ctx context.Context, network, addr string) (net.Conn, error) {
+			return newCommandConn(ctx, cmd, flags...)
+		},
+		Host: "http://docker",


Also thinking out loud while trying to grasp this PR; we should add comments with these (future readers may be confused and think "what's this magic docker host?"

This came pretty much verbatim from the existing example, I don't know where it came from nor really where it shows up...

docs/extend/cli_plugins.md

ijc · 2019-02-15T17:12:37Z

@thaJeztah ping -- I think your remaining comments were just thinking out loud rather than things that should be changed.

silvin-lubecki

LGTM, even if I think the design seems fragile: trying to call the CLI back with what we hope to be the same context (all the global flags we retrieve), but I don't see any cleaner way to do it.

thaJeztah

LGTM, but left one small comment

also would love to see @tonistiigi have a quick look

cli/command/cli.go

ijc · 2019-02-18T11:49:22Z

LGTM, even if I think the design seems fragile: trying to call the CLI back with what we hope to be the same context (all the global flags we retrieve), but I don't see any cleaner way to do it.

This is certainly true. The other choice is the status quo which is to do the dial in the plugin itself, with whatever capabilities the vendored cli libraries happen to have, which may differ from what the calling top-level CLI supports (which might be confusing for users).

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 <[email protected]>

thaJeztah · 2019-02-21T11:11:20Z

Let's get this merged; thanks!

ijc · 2019-02-21T11:14:40Z

Thanks, I ticked the box in #1661.

ijc · 2019-03-01T13:11:14Z

Hrm, I hadn't appreciated that this would require 18.09 on the daemon side;

error during connect: Get http://%2Fvar%2Frun%2Fdocker.sock/v1.40/containers/json?all=1&«...»&limit=0: command [docker system dial-stdio] has exited with exit status 1, please make sure the URL is valid, and Docker 18.09 or later is installed on the remote host: stderr=

This seems like a problem to me, @thaJeztah @tonistiigi WDYT?

Not sure how to address, since figuring out the daemon version requires calling out to the dameon, which currently required dial-stdio, rinse and repeat...

ijc · 2019-03-04T16:44:14Z

The above was due to me misreading the logs -- in fact it was telling me that my test had accidentally found /usr/bin/docker which was 18.06 and this didn't support docker system dial-stdio. Now that I fixed the test to find the right cli this aspect is ok.

However, it appears that dial-stdio does not work on Windows:

docker@WIN-NUC0 C:\Users\docker>.\docker-windows-amd64.exe version
Client:
 Version:           19.03.0-dev
 API version:       1.39 (downgraded from 1.40)
 Go version:        go1.11.5
 Git commit:        8ddde26a
 Built:             Mon Mar  4 16:34:00 2019
 OS/Arch:           windows/amd64
 Experimental:      false

Server: Docker Engine - Community
 Engine:
  Version:          18.09.0
  API version:      1.39 (minimum version 1.12)
  Go version:       go1.10.4
  Git commit:       4d60db4
  Built:            Wed Nov  7 00:55:00 2018
  OS/Arch:          linux/amd64
  Experimental:     false

docker@WIN-NUC0 C:\Users\docker>.\docker-windows-amd64.exe system dial-stdio
the raw stream connection does not implement halfCloser

Basically it seems that net.Conn doesn't have ReadClose on Windows (from my tests it does have WriteClose):

docker@WIN-NUC0 C:\Users\docker>.\docker-windows-amd64.exe -D system dial-stdio
DEBU[0000] conn is a *winio.win32MessageBytePipe
DEBU[0000] conn is a halfReadCloser: false
DEBU[0000] conn is a halfWriteCloser: true
the raw stream connection does not implement halfCloser

(this seems to be independent of plugins using this, it's just the base functionality is not there in windows)

tonistiigi · 2019-03-04T17:31:20Z

@AkihiroSuda If ReadClose is missing in win, could we add a fallback that doesn't need that.

ijc · 2019-03-04T17:38:10Z

I raised #1710 in the meantime.

Since docker#1654 so far we've had problems with it not working on Windows (npipe lacked the `ReadClose` method) and problems with using tcp with tls (the tls connection also lacks `ReadClose`). Both of these were workedaround in docker#1718 which added a nop `ReadClose` 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 891b3d9 ("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. Signed-off-by: Ian Campbell <[email protected]>

Since docker#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 docker#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 891b3d9 ("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. Signed-off-by: Ian Campbell <[email protected]>

Since docker#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 docker#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 891b3d9 ("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 <[email protected]>

cli-plugins: use only the first word of Use as the name

8fa7c57

This can be obtained with the `.Name()` method. Signed-off-by: Ian Campbell <[email protected]>

ijc requested a review from thaJeztah as a code owner February 1, 2019 14:11

GordonTheTurtle added the status/0-triage label Feb 1, 2019

ijc force-pushed the plugins-dial-stdio branch from fa57dcf to 4c1de75 Compare February 1, 2019 14:21

thaJeztah added status/2-code-review area/plugins and removed status/0-triage labels Feb 1, 2019

thaJeztah reviewed Feb 1, 2019

View reviewed changes

ijc force-pushed the plugins-dial-stdio branch from 4c1de75 to b524722 Compare February 1, 2019 16:07

ijc mentioned this pull request Feb 7, 2019

Followup to #1564 (cli plugins) #1661

Open

11 tasks

silvin-lubecki approved these changes Feb 15, 2019

View reviewed changes

thaJeztah approved these changes Feb 17, 2019

View reviewed changes

cli/command/cli.go Outdated Show resolved Hide resolved

ijc force-pushed the plugins-dial-stdio branch from b524722 to 891b3d9 Compare February 18, 2019 11:53

tonistiigi approved these changes Feb 21, 2019

View reviewed changes

thaJeztah merged commit 06b837a into docker:master Feb 21, 2019

GordonTheTurtle added this to the 19.03.0 milestone Feb 21, 2019

ijc deleted the plugins-dial-stdio branch February 21, 2019 11:14

ijc mentioned this pull request Feb 25, 2019

Add a field to the config file for plugin use. #1652

Merged

ijc mentioned this pull request Mar 1, 2019

Transform docker-app as a docker-cli plugin docker/app#469

Merged

ijc mentioned this pull request Mar 18, 2019

cli-plugins: disable use of dial-stdio #1745

Merged

thaJeztah mentioned this pull request Dec 12, 2023

plugins: fix "dial-stdio" feature or remove the feature #4708

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cli-plugins: use system dial-stdio to contact the engine. #1654

cli-plugins: use system dial-stdio to contact the engine. #1654

ijc commented Feb 1, 2019

codecov-io commented Feb 1, 2019 •

edited

Loading

thaJeztah Feb 1, 2019

thaJeztah Feb 1, 2019

ijc Feb 1, 2019 •

edited

Loading

ijc commented Feb 15, 2019

silvin-lubecki left a comment

thaJeztah left a comment

ijc commented Feb 18, 2019

thaJeztah commented Feb 21, 2019

ijc commented Feb 21, 2019

ijc commented Mar 1, 2019

ijc commented Mar 4, 2019

tonistiigi commented Mar 4, 2019

ijc commented Mar 4, 2019

cli-plugins: use system dial-stdio to contact the engine. #1654

cli-plugins: use system dial-stdio to contact the engine. #1654

Conversation

ijc commented Feb 1, 2019

codecov-io commented Feb 1, 2019 • edited Loading

Codecov Report

thaJeztah Feb 1, 2019

Choose a reason for hiding this comment

thaJeztah Feb 1, 2019

Choose a reason for hiding this comment

ijc Feb 1, 2019 • edited Loading

Choose a reason for hiding this comment

ijc commented Feb 15, 2019

silvin-lubecki left a comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

ijc commented Feb 18, 2019

thaJeztah commented Feb 21, 2019

ijc commented Feb 21, 2019

ijc commented Mar 1, 2019

ijc commented Mar 4, 2019

tonistiigi commented Mar 4, 2019

ijc commented Mar 4, 2019

codecov-io commented Feb 1, 2019 •

edited

Loading

ijc Feb 1, 2019 •

edited

Loading