New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support SSH connection #1014

Merged
merged 1 commit into from Aug 14, 2018

Conversation

@AkihiroSuda
Member

AkihiroSuda commented Apr 20, 2018

Signed-off-by: Akihiro Suda suda.akihiro@lab.ntt.co.jp

- What I did

Added support for SSH connection. e.g. docker -H ssh://me@server

- How I did it

Followed @tonistiigi 's suggestion: #889 (comment)

The cli should accept ssh://me@server for DOCKER_HOST and -H. Using that would execute ssh with the passed config.
The ssh command would call a hidden command on the docker CLI binary on the remote side. For example, docker dial-stdio.
This command will make a connection to the local DOCKER_HOST variable (almost always the default local socket) and forward that connection on the commands stdio.
Even though this command is supposed to run locally to the dockerd binary, we think that it is an invalid configuration for this feature to remove the local docker binary so we can rely on it always being present.

- How to verify it

docker -H ssh://me@server run -it --rm busybox

- Description for the changelog

support SSH connection

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

penguin

https://commons.wikimedia.org/wiki/File:Manchot_Ad%C3%A9lie_juv%C3%A9nile.jpg

Vendor: moby/moby#36630 (merged)


Tests (currently tested manually):

  • echo hello | ./docker -H ssh://10.2.54.48 run -i --rm busybox cat; echo world
  • ./docker -H ssh://10.2.54.48 run --rm busybox echo hello; echo world
  • ./docker -H ssh://10.2.54.48 run -i --rm busybox echo hello; echo world
@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Apr 23, 2018

Collaborator

Design LGTM! cc @tonistiigi for design as well.
Tested this out and it works pretty well.

I did notice that this:

echo hello | ./docker-dev -H ssh://docker@$ip run -i --rm busybox cat

Doesn't echo anything to my term when using the ssh connector.
But a normal interactive container works fine. Image pull also works well.

Collaborator

cpuguy83 commented Apr 23, 2018

Design LGTM! cc @tonistiigi for design as well.
Tested this out and it works pretty well.

I did notice that this:

echo hello | ./docker-dev -H ssh://docker@$ip run -i --rm busybox cat

Doesn't echo anything to my term when using the ssh connector.
But a normal interactive container works fine. Image pull also works well.

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Apr 23, 2018

Member

@cpuguy83
Thank you for testing.

The issue can be mitigated by increasing this timeout, which is the equivalent of socat -t and nc -q: https://github.com/docker/cli/pull/1014/files#diff-065a55b1c9d14a6817c97ca733652920R17

However, increasing this value introduces extra sleep.

I'll try to find a more robust way

Member

AkihiroSuda commented Apr 23, 2018

@cpuguy83
Thank you for testing.

The issue can be mitigated by increasing this timeout, which is the equivalent of socat -t and nc -q: https://github.com/docker/cli/pull/1014/files#diff-065a55b1c9d14a6817c97ca733652920R17

However, increasing this value introduces extra sleep.

I'll try to find a more robust way

@AkihiroSuda AkihiroSuda reopened this Apr 23, 2018

@vdemeester

This comment has been minimized.

Show comment
Hide comment
@vdemeester

vdemeester Apr 24, 2018

Member

Design LGTM too, this is cool 😻

Member

vdemeester commented Apr 24, 2018

Design LGTM too, this is cool 😻

@tonistiigi

Overall design LGTM. I don't understand the timeout logic on handling EOF though.

Show outdated Hide outdated cli/command/system/dial_stdio.go
Show outdated Hide outdated cli/command/system/dial_stdio.go
Show outdated Hide outdated cli/connhelper/connhelper.go
clientOpts = append(clientOpts, client.WithHost(host))
} else {
clientOpts = append(clientOpts, func(c *client.Client) error {
httpClient := &http.Client{

This comment has been minimized.

@tonistiigi

tonistiigi Apr 25, 2018

Member

I think we should just add WithDialer instead of overriding client and remove WithHijackDialer. Using it would create a new client with a transport pointing to the current dialer and set it to hijack as well.

@tonistiigi

tonistiigi Apr 25, 2018

Member

I think we should just add WithDialer instead of overriding client and remove WithHijackDialer. Using it would create a new client with a transport pointing to the current dialer and set it to hijack as well.

Show outdated Hide outdated cli/command/system/dial_stdio.go
Show outdated Hide outdated cli/command/system/dial_stdio.go
Show outdated Hide outdated cli/command/system/dial_stdio.go
@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jul 2, 2018

Collaborator

Any update here?

Collaborator

cpuguy83 commented Jul 2, 2018

Any update here?

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Jul 3, 2018

Member

Sorry still stuck at the stream connection issue #1014 (comment)

Will try to look into this again ASAP

Member

AkihiroSuda commented Jul 3, 2018

Sorry still stuck at the stream connection issue #1014 (comment)

Will try to look into this again ASAP

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jul 3, 2018

Collaborator

No worries I just thought this was already merged and realized it wasn't!

Collaborator

cpuguy83 commented Jul 3, 2018

No worries I just thought this was already merged and realized it wasn't!

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jul 27, 2018

Codecov Report

Merging #1014 into master will decrease coverage by 0.28%.
The diff coverage is 34.46%.

@@            Coverage Diff            @@
##           master   #1014      +/-   ##
=========================================
- Coverage   54.29%     54%   -0.29%     
=========================================
  Files         268     272       +4     
  Lines       17843   18068     +225     
=========================================
+ Hits         9687    9758      +71     
- Misses       7546    7694     +148     
- Partials      610     616       +6

codecov-io commented Jul 27, 2018

Codecov Report

Merging #1014 into master will decrease coverage by 0.28%.
The diff coverage is 34.46%.

@@            Coverage Diff            @@
##           master   #1014      +/-   ##
=========================================
- Coverage   54.29%     54%   -0.29%     
=========================================
  Files         268     272       +4     
  Lines       17843   18068     +225     
=========================================
+ Hits         9687    9758      +71     
- Misses       7546    7694     +148     
- Partials      610     616       +6

@AkihiroSuda AkihiroSuda changed the title from support SSH connection to [WIP] support SSH connection Jul 27, 2018

@AkihiroSuda AkihiroSuda changed the title from [WIP] support SSH connection to support SSH connection Jul 27, 2018

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Jul 27, 2018

Member

updated, PTAL

Member

AkihiroSuda commented Jul 27, 2018

updated, PTAL

@tonistiigi

This comment has been minimized.

Show comment
Hide comment
@tonistiigi

tonistiigi Jul 27, 2018

Member

@AkihiroSuda Thanks! This is starting look great and worked well when testing.

Can you offer a quick explanation of why these halfclosers, CloseWrite/CloseRead are needed. It wasn't obvious for me what would break without them. Maybe a case I could try out.

Probably nothing but when running buildkit integration through this and canceling I get WARN[0031] commandConn.Close: CloseRead: wait: no child processe in the output.

Everything else sgtm on first pass.

Member

tonistiigi commented Jul 27, 2018

@AkihiroSuda Thanks! This is starting look great and worked well when testing.

Can you offer a quick explanation of why these halfclosers, CloseWrite/CloseRead are needed. It wasn't obvious for me what would break without them. Maybe a case I could try out.

Probably nothing but when running buildkit integration through this and canceling I get WARN[0031] commandConn.Close: CloseRead: wait: no child processe in the output.

Everything else sgtm on first pass.

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Jul 28, 2018

Member

Can you offer a quick explanation of why these halfclosers, CloseWrite/CloseRead are needed. It wasn't obvious for me what would break without them. Maybe a case I could try out.

When we got EOF from stdin we still want to half-open the connection so that we can wait for stdout.

Probably nothing but when running buildkit integration through this and canceling I get WARN[0031] commandConn.Close: CloseRead: wait: no child processe in the output.

Should be fixed in the latest commit.

Member

AkihiroSuda commented Jul 28, 2018

Can you offer a quick explanation of why these halfclosers, CloseWrite/CloseRead are needed. It wasn't obvious for me what would break without them. Maybe a case I could try out.

When we got EOF from stdin we still want to half-open the connection so that we can wait for stdout.

Probably nothing but when running buildkit integration through this and canceling I get WARN[0031] commandConn.Close: CloseRead: wait: no child processe in the output.

Should be fixed in the latest commit.

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jul 31, 2018

Collaborator

Looks good. There's a bunch of vendor changes are these needed?

Collaborator

cpuguy83 commented Jul 31, 2018

Looks good. There's a bunch of vendor changes are these needed?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jul 31, 2018

Collaborator

Oh one thing I noticed, when the host is not valid we get a pretty bad EOF message without any details.
Wonder if we can fix that.

Collaborator

cpuguy83 commented Jul 31, 2018

Oh one thing I noticed, when the host is not valid we get a pretty bad EOF message without any details.
Wonder if we can fix that.

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Aug 1, 2018

Member

Updated PR with vendor for moby/moby#36630 and UTs for the EOF case.

Member

AkihiroSuda commented Aug 1, 2018

Updated PR with vendor for moby/moby#36630 and UTs for the EOF case.

@vdemeester

SGTM 👼
Small lint error though 😝

cli/connhelper/ssh/ssh.go:12:6:warning: type name will be used as ssh.SSHCmd by other packages, and that stutters; consider calling this Cmd (golint)
@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Aug 1, 2018

Member

fixed lint issue

Member

AkihiroSuda commented Aug 1, 2018

fixed lint issue

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Aug 1, 2018

Collaborator

Can we detect a case where the CLI version on the server side is not supported?

./docker: error during connect: Post http://docker/v1.38/containers/create: command /usr/bin/ssh has exited with exit status 1, stderr="\nUsage:\tdocker system COMMAND\n\nManage Docker\n\nCommands:\n  df          Show docker disk usage\n  events      Get real time events from the server\n  info        Display system-wide information\n  prune       Remove unused data\n\nRun 'docker system COMMAND --help' for more information on a command.\n".
See './docker run --help'.
Collaborator

cpuguy83 commented Aug 1, 2018

Can we detect a case where the CLI version on the server side is not supported?

./docker: error during connect: Post http://docker/v1.38/containers/create: command /usr/bin/ssh has exited with exit status 1, stderr="\nUsage:\tdocker system COMMAND\n\nManage Docker\n\nCommands:\n  df          Show docker disk usage\n  events      Get real time events from the server\n  info        Display system-wide information\n  prune       Remove unused data\n\nRun 'docker system COMMAND --help' for more information on a command.\n".
See './docker run --help'.
Show outdated Hide outdated cli/command/system/dial_stdio.go
Show outdated Hide outdated cli/command/system/dial_stdio.go
func (c *commandConn) CloseWrite() error {
// NOTE: maybe already closed here
if err := c.stdin.Close(); err != nil && !ignorableCloseError(err) {
logrus.Warnf("commandConn.CloseWrite: %v", err)

This comment has been minimized.

@cpuguy83

cpuguy83 Aug 1, 2018

Collaborator

No logrus?

@cpuguy83

cpuguy83 Aug 1, 2018

Collaborator

No logrus?

This comment has been minimized.

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Aug 2, 2018

Member

Or do you mean we should just ignore close errors?

@AkihiroSuda

AkihiroSuda Aug 2, 2018

Member

Or do you mean we should just ignore close errors?

func (c *commandConn) Close() error {
var err error
if err = c.CloseRead(); err != nil {
logrus.Warnf("commandConn.Close: CloseRead: %v", err)

This comment has been minimized.

@cpuguy83

cpuguy83 Aug 1, 2018

Collaborator

No logrus?

@cpuguy83

cpuguy83 Aug 1, 2018

Collaborator

No logrus?

logrus.Warnf("commandConn.Close: CloseRead: %v", err)
}
if err = c.CloseWrite(); err != nil {
logrus.Warnf("commandConn.Close: CloseWrite: %v", err)

This comment has been minimized.

@cpuguy83

cpuguy83 Aug 1, 2018

Collaborator

No logrus?

@cpuguy83

cpuguy83 Aug 1, 2018

Collaborator

No logrus?

Show outdated Hide outdated cli/connhelper/connhelper.go
Show outdated Hide outdated cli/connhelper/connhelper.go
Show outdated Hide outdated cli/command/system/dial_stdio.go
}
connHalfCloser, ok := conn.(halfCloser)
if !ok {
return errors.New("the raw stream connection does not implement halfCloser")

This comment has been minimized.

@cpuguy83

cpuguy83 Aug 1, 2018

Collaborator

Seems like this will be passed on to the user. Maybe we can explain what this error is better?

@cpuguy83

cpuguy83 Aug 1, 2018

Collaborator

Seems like this will be passed on to the user. Maybe we can explain what this error is better?

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Aug 2, 2018

Member

Actually this type assertion should never fail and the error won't be printed

@AkihiroSuda

AkihiroSuda Aug 2, 2018

Member

Actually this type assertion should never fail and the error won't be printed

This comment has been minimized.

@cpuguy83

cpuguy83 Aug 7, 2018

Collaborator

Maybe we should make the error a little more clear in this case?
"If you see this, this is a bad bug, please report to github.com/docker/cli"

@cpuguy83

cpuguy83 Aug 7, 2018

Collaborator

Maybe we should make the error a little more clear in this case?
"If you see this, this is a bad bug, please report to github.com/docker/cli"

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Aug 8, 2018

Member

How about just panic here and defer func(){panic(errors.Wrap(recover(), "If you see this, this is a bad bug, please report to github.com/docker/cli"))} (in another PR)

@AkihiroSuda

AkihiroSuda Aug 8, 2018

Member

How about just panic here and defer func(){panic(errors.Wrap(recover(), "If you see this, this is a bad bug, please report to github.com/docker/cli"))} (in another PR)

func ignorableCloseError(err error) bool {
errS := err.Error()
ss := []string{
os.ErrClosed.Error(),

This comment has been minimized.

@cpuguy83

cpuguy83 Aug 1, 2018

Collaborator

Not sure why we are comparing error strings here.

@cpuguy83

cpuguy83 Aug 1, 2018

Collaborator

Not sure why we are comparing error strings here.

This comment has been minimized.

@AkihiroSuda

AkihiroSuda Aug 2, 2018

Member

errors.Cause does not work for error that was not crafted with github.com/pkg/errors

@AkihiroSuda

AkihiroSuda Aug 2, 2018

Member

errors.Cause does not work for error that was not crafted with github.com/pkg/errors

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Aug 2, 2018

Member

updated PR

Member

AkihiroSuda commented Aug 2, 2018

updated PR

support SSH connection
e.g. docker -H ssh://me@server

The `docker` CLI also needs to be installed on the remote host to
provide `docker system dial-stdio`, which proxies the daemon socket to stdio.

Please refer to docs/reference/commandline/dockerd.md .

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>
@tonistiigi

I didn't have time to play around with the halfclosers but don't want to block this. In an odd chance if I find that something can be simplified a bit in the copy loops it can easily be a separate change.

LGTM

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda
Member

AkihiroSuda commented Aug 14, 2018

@cpuguy83 PTAL?

@vdemeester

LGTM 🔥

@cpuguy83

LGTM

Was this ever addressed? If not let's open an issue to track it as I'm sure it will be confusing.

@cpuguy83 cpuguy83 merged commit e92614a into docker:master Aug 14, 2018

8 of 9 checks passed

codecov/patch 34.46% of diff hit (target 50%)
Details
ci/circleci: cross Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: shellcheck Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: validate Your tests passed on CircleCI!
Details
codecov/project 54% (-0.29%) compared to 261ff66
Details
continuous-integration/jenkins/pr-head This commit looks good
Details
dco-signed All commits are signed

@GordonTheTurtle GordonTheTurtle added this to the 18.09.0 milestone Aug 14, 2018

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Aug 15, 2018

Member

Thanks for merging 🎉

Was this ever addressed?

Could you be more specific?

Member

AkihiroSuda commented Aug 15, 2018

Thanks for merging 🎉

Was this ever addressed?

Could you be more specific?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Aug 15, 2018

Collaborator

Sorry, meant to link the comment ;)

#1014 (comment)

Collaborator

cpuguy83 commented Aug 15, 2018

Sorry, meant to link the comment ;)

#1014 (comment)

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Aug 15, 2018

Member

I think the problem is substantially covered by this change, although it does not negotiate versions actually for avoiding potential performance overhead: #1014 (comment)

updated to print full args, so that user can notice whether the error is from SSH or Docker

$ ./docker -H ssh://dummy@github.com info
error during connect: Get http://docker/v1.38/info: command [ssh -l dummy github.com -- docker
system dial-stdio] has exited with exit status 255, please make sure the URL is valid, and Docker 
18.09 or later is installed on the remote host: stderr="dummy@github.com: Permission denied 
(publickey).\r\n"
Member

AkihiroSuda commented Aug 15, 2018

I think the problem is substantially covered by this change, although it does not negotiate versions actually for avoiding potential performance overhead: #1014 (comment)

updated to print full args, so that user can notice whether the error is from SSH or Docker

$ ./docker -H ssh://dummy@github.com info
error during connect: Get http://docker/v1.38/info: command [ssh -l dummy github.com -- docker
system dial-stdio] has exited with exit status 255, please make sure the URL is valid, and Docker 
18.09 or later is installed on the remote host: stderr="dummy@github.com: Permission denied 
(publickey).\r\n"
@sandys

This comment has been minimized.

Show comment
Hide comment
@sandys

sandys Aug 20, 2018

How does one add options to SSH here ?
We enforce hardware tokens for all our SSH (using opensc). We use epass2003 tokens. This is how the command looks.

#Ubuntu -
ssh  -o PKCS11Provider=/usr/lib/x86_64-linux-gnu/opensc-pkcs11.so  user@gcp.red.com

#OSX -
ssh -o PKCS11Provider=/Library/OpenSC/lib/opensc-pkcs11.so user@gcp.red.com

sandys commented Aug 20, 2018

How does one add options to SSH here ?
We enforce hardware tokens for all our SSH (using opensc). We use epass2003 tokens. This is how the command looks.

#Ubuntu -
ssh  -o PKCS11Provider=/usr/lib/x86_64-linux-gnu/opensc-pkcs11.so  user@gcp.red.com

#OSX -
ssh -o PKCS11Provider=/Library/OpenSC/lib/opensc-pkcs11.so user@gcp.red.com

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Aug 20, 2018

Member

@sandys ~/.ssh/config should work

Host foo
        Hostname          gcp.red.com
        PKCS11Provider    /usr/lib/x86_64-linux-gnu/opensc-pkcs11.so
Member

AkihiroSuda commented Aug 20, 2018

@sandys ~/.ssh/config should work

Host foo
        Hostname          gcp.red.com
        PKCS11Provider    /usr/lib/x86_64-linux-gnu/opensc-pkcs11.so
@sandys

This comment has been minimized.

Show comment
Hide comment
@sandys

sandys Aug 20, 2018

sandys commented Aug 20, 2018

@AkihiroSuda

This comment has been minimized.

Show comment
Hide comment
@AkihiroSuda

AkihiroSuda Aug 20, 2018

Member

Currently no, could you open a github issue?

Secondly we have more -o options that we pass.

Any option that cannot be configured via ~/.ssh/config?

Member

AkihiroSuda commented Aug 20, 2018

Currently no, could you open a github issue?

Secondly we have more -o options that we pass.

Any option that cannot be configured via ~/.ssh/config?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment