Skip to content

Conversation

@panekj
Copy link
Contributor

@panekj panekj commented Mar 4, 2023

- What I did

Modified connhelper to allow socket path in SSH URI

- How I did it

Removed error if path is specified when using ssh:// scheme
Added capture of URI Path
Append --host flag with formatted socket path as unix://%s between passed args

- How to verify it

Run below

Linux/macOS:

DOCKER_HOST='ssh://host/var/run/docker.sock' docker system info

Windows

$env:DOCKER_HOST='ssh://host/var/run/docker.sock'
docker system info

- Description for the changelog

Allow specifying socket path when connecting via SSH

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

@panekj
Copy link
Contributor Author

panekj commented Mar 4, 2023

alternative impl, since Args() can receive arguments other than docker but I don't know how likely that is, if it happens at all

	case "ssh":
		sp, err := ssh.ParseURL(daemonURL)
		if err != nil {
			return nil, errors.Wrap(err, "ssh host connection is not valid")
		}
		return &ConnectionHelper{
			Dialer: func(ctx context.Context, network, addr string) (net.Conn, error) {
				args := []string{"docker"}
				if sp.Path != "" {
					args = append(args, "--host", fmt.Sprintf("unix://%s", sp.Path))
				}
				args = append(args, "system", "dial-stdio")
				return commandconn.New(ctx, "ssh", append(sshFlags, sp.Args(args...)...)...)
			},
			Host: "http://docker.example.com",
		}, nil
	}

@thaJeztah
Copy link
Member

Do you have a specific use-case for this? IIUC, the current design was for the remote CLI to already be configured to connect to a specific socket location (/var/run/docker.sock by default, but could be different if set in ~/.docker/config.json). This PR would mean that the remote CLI would control the remote CLI "on-the-fly" to connect to a different path?

@panekj
Copy link
Contributor Author

panekj commented Mar 6, 2023

I didn't knew it could be set in config but that would not help if I have multiple daemons running on the remote host, since I would have to change the config each time before connecting.
With this change I can keep it as a simple context or use on-demand via DOCKER_HOST

@panekj
Copy link
Contributor Author

panekj commented Mar 6, 2023

This PR would mean that the remote CLI would control the remote CLI "on-the-fly" to connect to a different path?

Basically, yes.

@neersighted
Copy link
Member

I'm also skeptical of this method and the format. Giving a special meaning to the colon character is not great (and do we already support :port?), and trying to configure the remote CLI seems a bit fraught. It feels to me like a context or at least a URL anchor/query might be better ways to encode and represent this, if it's even desirable.

Copy link
Member

@neersighted neersighted left a comment

Choose a reason for hiding this comment

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

I took a closer look at this diff, and this is a lot more palatable now that I see this is not in fact a microformat, but instead simply using standard URL parsing logic ala Git. user@host:port/path/to/docker.sock will work, as will user@host:/path/to/docker.sock.

However, I think that Args definitely needs refactoring. It now only works in terms of the docker CLI, and hardcodes knowledge that the path to the CLI will be the first field of add. That should become more generic (or maybe we should set DOCKER_HOST instead as SSH always invokes a shell) to make this maintainable.

@panekj
Copy link
Contributor Author

panekj commented Mar 6, 2023

I took a closer look at this diff, and this is a lot more palatable now that I see this is not in fact a microformat, but instead simply using standard URL parsing logic ala Git. user@host:port/path/to/docker.sock will work, as will user@host:/path/to/docker.sock.

This has been a well established format for all tools that are based on SSH (ssh, scp, git, etc.)

@panekj
Copy link
Contributor Author

panekj commented Mar 6, 2023

However, I think that Args definitely needs refactoring.

I would be glad to leave that work for whoever has better understanding of whole codebase and vision for the feature (and more time to do it).

@neersighted
Copy link
Member

Yes, your examples have a null port (user@host:/path) which is misleading, a simple user@host/path example could have been used. Now that that is clear, my objections are addressed. Your alternate proposed implementation that keeps the --host concern out of the Args function looks much better; could you please push that version up for further discussion?

@panekj
Copy link
Contributor Author

panekj commented Mar 6, 2023

Yes, your examples have a null port (user@host:/path) which is misleading

Sorry, it's my unfortunate habit if typing the URI :)

@panekj panekj force-pushed the feat/ssh-socket-path branch from 467dd2f to 8540142 Compare March 6, 2023 22:30
@panekj panekj requested a review from thaJeztah as a code owner March 6, 2023 22:30
Signed-off-by: Jakub Panek <me@panekj.dev>
@panekj panekj force-pushed the feat/ssh-socket-path branch from 8540142 to 25ebf0e Compare March 6, 2023 23:48
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

@cpuguy83 cpuguy83 merged commit f4201b9 into docker:master May 5, 2023
@thaJeztah thaJeztah added this to the 24.0.0 milestone May 5, 2023
tianon added a commit to tianon/debian-moby that referenced this pull request May 20, 2025
- "connhelper: Allow socket path when using SSH"
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants