Skip to content

connhelper: use ssh multiplexing #2132

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.

Sign up for GitHub

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

Merged
thaJeztah merged 2 commits into from
Jan 9, 2020
Merged

connhelper: use ssh multiplexing #2132

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c04dd6e
connhelper: add ssh multiplexing
tonistiigi Oct 10, 2019
4ecbef4
docs: document ssh multiplexing env
tonistiigi Jan 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 20 additions & 1 deletion cli/connhelper/connhelper.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,10 @@ import (
"context"
"net"
"net/url"
"os"
"strconv"

"github.com/docker/cli/cli/config"
"github.com/docker/cli/cli/connhelper/commandconn"
"github.com/docker/cli/cli/connhelper/ssh"
"github.com/pkg/errors"
Expand Down Expand Up @@ -34,7 +37,7 @@ func GetConnectionHelper(daemonURL string) (*ConnectionHelper, error) {
}
return &ConnectionHelper{
Dialer: func(ctx context.Context, network, addr string) (net.Conn, error) {
return commandconn.New(ctx, "ssh", append(sp.Args(), []string{"--", "docker", "system", "dial-stdio"}...)...)
return commandconn.New(ctx, "ssh", append(multiplexingArgs(), append(sp.Args(), []string{"--", "docker", "system", "dial-stdio"}...)...)...)
},
Host: "http://docker",
}, nil
Expand All @@ -53,3 +56,19 @@ func GetCommandConnectionHelper(cmd string, flags ...string) (*ConnectionHelper,
Host: "http://docker",
}, nil
}

func multiplexingArgs() []string {
if v := os.Getenv("DOCKER_SSH_NO_MUX"); v != "" {
Copy link
Member

@thaJeztah thaJeztah Jan 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you document the new environment variables in https://github.com/docker/cli/blob/master/docs/reference/commandline/cli.md#environment-variables ?

if b, err := strconv.ParseBool(v); err == nil && b {
return nil
}
}
if err := os.MkdirAll(config.Dir(), 0700); err != nil {
Copy link
Collaborator

@AkihiroSuda AkihiroSuda Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we should create a new directory, because when ~/.docker already exists with other permission bits, we can't guarantee the sockets are created under a directory with 0700.
On my Macintosh, ~/.docker was somehow created with 0755.

But it is more likely to hit UNIX_PATH_MAX (108)... 😢

Copy link
Collaborator

@AkihiroSuda AkihiroSuda Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably we can let commandconn change working directory for avoiding 108 limit

Copy link
Member Author

@tonistiigi tonistiigi Oct 11, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The socket itself is created with 0600. This mkdir is to avoid the case where .docker does not exist.

return nil
}
args := []string{"-o", "ControlMaster=auto", "-o", "ControlPath=" + config.Dir() + "/%r@%h:%p"}
Copy link
Collaborator

@AkihiroSuda AkihiroSuda Oct 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What happens if .ssh/config already has this?

Copy link
Member Author

@tonistiigi tonistiigi Oct 10, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It overwrites afaik. This is the main reason I left a way to opt-out. If we want to be conservative about it, one way would be to use a different helper prefix.

if v := os.Getenv("DOCKER_SSH_MUX_PERSIST"); v != "" {
args = append(args, "-o", "ControlPersist="+v)
Copy link
Contributor

@justincormack justincormack Oct 29, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason not to have a Docker config option for this as well?

Copy link
Member Author

@tonistiigi tonistiigi Dec 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you would ever set them they would probably be more dependent on the actual DOCKER_HOST value, not a global client config. I'm not even sure we even want to document these, it should be very advanced usage if you want to set them.

Copy link
Member

@thaJeztah thaJeztah Dec 12, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ping @justincormack pal ^

}
return args
}
2 changes: 2 additions & 0 deletions docs/reference/commandline/cli.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,8 @@ by the `docker` command line:
* `DOCKER_TMPDIR` Location for temporary Docker files.
* `DOCKER_CONTEXT` Specify the context to use (overrides DOCKER_HOST env var and default context set with "docker context use")
* `DOCKER_DEFAULT_PLATFORM` Specify the default platform for the commands that take the `--platform` flag.
* `DOCKER_SSH_NO_MUX` If set will turn off SSH multiplexing when connecting to daemon through SSH.
* `DOCKER_SSH_MUX_PERSIST` Set a duration for keeping SSH multiplexing socket alive between commands (e.g `60s`).

Because Docker is developed using Go, you can also use any environment
variables used by the Go runtime. In particular, you may find these useful:
Expand Down