-
Notifications
You must be signed in to change notification settings - Fork 2.1k
resurrect ssh multiplexing #4699
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,9 +9,12 @@ import ( | |
| "fmt" | ||
| "net" | ||
| "net/url" | ||
| "os" | ||
| "slices" | ||
| "strconv" | ||
| "strings" | ||
|
|
||
| "github.com/docker/cli/cli/config" | ||
| "github.com/docker/cli/cli/connhelper/commandconn" | ||
| "github.com/docker/cli/cli/connhelper/ssh" | ||
| ) | ||
|
|
@@ -50,6 +53,7 @@ func getConnectionHelper(daemonURL string, sshFlags []string) (*ConnectionHelper | |
| return nil, fmt.Errorf("ssh host connection is not valid: %w", err) | ||
| } | ||
| sshFlags = addSSHTimeout(sshFlags) | ||
| sshFlags = addMultiplexingArgs(sshFlags) | ||
| sshFlags = disablePseudoTerminalAllocation(sshFlags) | ||
|
|
||
| remoteCommand := []string{"docker", "system", "dial-stdio"} | ||
|
|
@@ -83,6 +87,24 @@ func GetCommandConnectionHelper(cmd string, flags ...string) (*ConnectionHelper, | |
| }, nil | ||
| } | ||
|
|
||
| func addMultiplexingArgs(sshFlags []string) []string { | ||
| if v := os.Getenv("DOCKER_SSH_NO_MUX"); v != "" { | ||
| if b, err := strconv.ParseBool(v); err == nil && b { | ||
| return sshFlags | ||
| } | ||
| } | ||
| if err := os.MkdirAll(config.Dir(), 0o700); err != nil { | ||
| return sshFlags | ||
| } | ||
| sshFlags = append(sshFlags, "-o", "ControlMaster=auto", "-o", "ControlPath="+config.Dir()+"/%r@%h:%p") | ||
| if v := os.Getenv("DOCKER_SSH_MUX_PERSIST"); v != "" { | ||
| sshFlags = append(sshFlags, "-o", "ControlPersist="+v) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [MEDIUM] The value of Suggestion: Validate the value against the expected format (integer seconds or SSH duration string) before appending, or at minimum document what values are accepted in the env var table. |
||
| } else { | ||
| sshFlags = append(sshFlags, "-o", "ControlPersist=60") | ||
| } | ||
| return sshFlags | ||
| } | ||
|
|
||
| func addSSHTimeout(sshFlags []string) []string { | ||
| if !strings.Contains(strings.Join(sshFlags, ""), "ConnectTimeout") { | ||
| sshFlags = append(sshFlags, "-o ConnectTimeout=30") | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[MEDIUM]
DOCKER_SSH_NO_MUX=yessilently fails to disable multiplexing — only GoParseBoolstrings acceptedstrconv.ParseBoolonly recognises"1","t","T","TRUE","true","True","0","f","F","FALSE","false","False". A user settingDOCKER_SSH_NO_MUX=yes(the common Unix/shell convention used by many tools) will haveParseBoolreturn an error, making the conditionerr == nil && bfalse, and multiplexing will not be disabled — the env var is silently ignored.The documentation in
docker.mdonly says "If set will turn off SSH multiplexing" without clarifying which values are accepted, so users will be surprised.Fix: Use the simpler
v != ""check (consistent with how Docker treatsDOCKER_TLS,NO_COLOR, etc.) or at minimum document the accepted values: