Shell out to local SSH client as alternative to a paramiko connection #2680

aiordache · 2020-10-05T14:43:46Z

We have several issues raised for failures of the paramiko SSH connection, support for SSH config option etc. Shelling out to the local ssh client may provide a better experience and it mirrors the behaviour of docker cli. We keep the paramiko connection as the default one.

To use the local ssh client instead of paramiko, we must set the parameter use_ssh_client when creating the docker client as below:

def test_ssh_client():
    client = APIClient(base_url='ssh://remote', use_ssh_client=True)

    containers = client.containers()
    print("Running container IDs:")
    for c in containers:
        print("\t{}".format(c['Id']))

Make sure we have configured key-based ssh login to the target docker host:

Host remote
	Hostname 192.168.0.10
	User root
	IdentityFile ~/test/id_rsa
	Port 22

Output

Running container IDs:
        bd07c66651afc84f4223df96f59c4ffb3ba4ebb425feb6a6e0365e1a8987a024
        8625e8663fcaaa512d54444c08d99057ca893c806e05dd702cbd1dc86764065e

Closes #2659

chris-crone · 2020-10-06T11:54:47Z

Dockerfile

@@ -13,3 +13,11 @@ RUN pip install -r test-requirements.txt

 COPY . /src
 RUN pip install .
+
+# install SSHD
+RUN apt-get install -y openssh-client


This might be included in the python:3.x image

better to move this to the top of this stage, so that it doesn't re-install with every code change.

Also needs a apt-get update, --no-install-recommends, and cleaning up after

chris-crone · 2020-10-06T11:54:58Z

Dockerfile-py3

+# Add the keys and set permissions
+COPY tests/ssh-keys /root/.ssh
+RUN chmod 600 /root/.ssh/id_rsa && \
+    chmod 600 /root/.ssh/id_rsa.pub


Makefile

docker/api/client.py

chris-crone · 2020-10-06T11:58:51Z

tests/Dockerfile

+# Add the keys and set permissions
+COPY tests/ssh-keys /root/.ssh
+RUN chmod 600 /root/.ssh/id_rsa && \
+    chmod 600 /root/.ssh/id_rsa.pub


chris-crone

Thanks @aiordache! I think we're nearly there

chris-crone · 2020-10-08T08:15:25Z

Makefile

@@ -1,3 +1,6 @@
+TEST_API_VERSION ?= 1.39
+TEST_ENGINE_VERSION ?= 19.03.12


nit: 19.03.13 is out now

docker/transport/sshconn.py

chris-crone · 2020-10-08T08:20:31Z

docker/transport/sshconn.py

        self.timeout = timeout
+        self.host = host
+
+        # self.base_url = six.moves.urllib_parse.urlparse(host)


Can we remove this? Do we know why it was added originally?

Re-ping on this as I see it's still here

Undid it by mistake.

chris-crone · 2020-10-08T08:24:07Z

tests/Dockerfile

    gnupg2 \
    pass \
-    curl
+    curl \


I think that curl and ssh are already in the Python 3.7 Debian image:

docker run --rm python:3.7 sh -c "which curl && which ssh" /usr/bin/curl /usr/bin/ssh

If there's another reason you need them, I prefer having lists like this sorted so that it's easy to check if something's being installed.

thaJeztah · 2020-10-08T12:44:30Z

Dockerfile

+RUN chmod 600 /root/.ssh/id_rsa && \
+    chmod 600 /root/.ssh/id_rsa.pub


I think with engine 20.x, we'll be able to use COPY --chmod moby/buildkit#1492

would a chmod -R work for this?

Suggested change

RUN chmod 600 /root/.ssh/id_rsa && \

chmod 600 /root/.ssh/id_rsa.pub

RUN chmod -R 600 /root/.ssh/

thaJeztah · 2020-10-08T12:50:35Z

tests/Dockerfile

+RUN chmod 600 /root/.ssh/id_rsa && \
+    chmod 600 /root/.ssh/id_rsa.pub


Suggested change

RUN chmod 600 /root/.ssh/id_rsa && \

chmod 600 /root/.ssh/id_rsa.pub

RUN chmod -R 600 /root/.ssh

chris-crone

Once my comments are addressed, this LGTM! Thanks @aiordache!

chris-crone · 2020-10-12T11:49:40Z

docker/transport/sshconn.py

+        if not self.proc or self.proc.stdin.closed:
+            raise Exception('SSH subprocess not initiated.'
+                            'connect() must be called first.')
+        self.proc.stdin.write(msg)
+        self.proc.stdin.flush()
+
+    def send(self, msg):
+        self.sendall(msg)
+        return len(msg)


nit: To avoid having to compute the message length separately, I would call send from sendall and use the byte count from the write() function's return.

Suggested change

if not self.proc or self.proc.stdin.closed:

raise Exception('SSH subprocess not initiated.'

'connect() must be called first.')

self.proc.stdin.write(msg)

self.proc.stdin.flush()

def send(self, msg):

self.sendall(msg)

return len(msg)

self.send(msg)

def send(self, msg):

if not self.proc or self.proc.stdin.closed:

raise Exception('SSH subprocess not initiated.'

'connect() must be called first.')

written = self.proc.stdin.write(msg)

self.proc.stdin.flush()

return written

Py2.7 request library seems to have a problem if we call send from sendall. Moved the logic intro a private method to be called in these two.

chris-crone · 2020-10-12T11:50:42Z

docker/transport/sshconn.py

        self.timeout = timeout
+        self.host = host
+
+        # self.base_url = six.moves.urllib_parse.urlparse(host)


Re-ping on this as I see it's still here

chris-crone · 2020-10-12T11:52:25Z

tests/integration/api_build_test.py

@@ -339,7 +339,6 @@ def test_build_with_extra_hosts(self):

        assert self.client.inspect_image(img_name)
        ctnr = self.run_container(img_name, 'cat /hosts-file')
-        self.tmp_containers.append(ctnr)


I haven't dived into the code but why this change?

The self.run_container already appends it to the list so there is no need to do it after.

Signed-off-by: aiordache <anca.iordache@docker.com>

aiordache requested a review from ulyssessouza as a code owner October 5, 2020 14:43

aiordache requested review from thaJeztah and chris-crone October 5, 2020 14:43

aiordache marked this pull request as draft October 5, 2020 17:38

chris-crone reviewed Oct 6, 2020

View reviewed changes

aiordache force-pushed the replace_paramiko branch 7 times, most recently from f21a36a to 19e8141 Compare October 7, 2020 18:49

aiordache marked this pull request as ready for review October 7, 2020 18:56

aiordache requested a review from chris-crone October 7, 2020 20:16

chris-crone reviewed Oct 8, 2020

View reviewed changes

aiordache force-pushed the replace_paramiko branch 2 times, most recently from 041e654 to 072215b Compare October 8, 2020 12:40

thaJeztah reviewed Oct 8, 2020

View reviewed changes

aiordache force-pushed the replace_paramiko branch 2 times, most recently from f931f46 to 515b23c Compare October 8, 2020 12:55

aiordache requested review from chris-crone and thaJeztah October 8, 2020 13:28

chris-crone approved these changes Oct 12, 2020

View reviewed changes

aiordache force-pushed the replace_paramiko branch 3 times, most recently from 66fa566 to a54e94f Compare October 13, 2020 08:34

Shell out to SSH client for an ssh connection

180414d

Signed-off-by: aiordache <anca.iordache@docker.com>

aiordache force-pushed the replace_paramiko branch from a54e94f to 180414d Compare October 13, 2020 08:42

aiordache merged commit e09b070 into docker:master Oct 16, 2020

aiordache added this to the 4.4.0 milestone Nov 23, 2020

ulyssessouza mentioned this pull request Dec 15, 2020

DOCKER_HOST=ssh://example.com works with docker 19.03.8 but not docker-compose 1.26 docker/compose#7514

Closed

MisterWS mentioned this pull request Mar 25, 2021

Feature Request: support "use_ssh_client" parameter for Docker SSH connections ansible-collections/community.docker#108

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Shell out to local SSH client as alternative to a paramiko connection #2680

Shell out to local SSH client as alternative to a paramiko connection #2680

aiordache commented Oct 5, 2020

chris-crone Oct 6, 2020

thaJeztah Oct 6, 2020

chris-crone Oct 6, 2020

chris-crone Oct 6, 2020

chris-crone left a comment

chris-crone Oct 8, 2020

chris-crone Oct 8, 2020

chris-crone Oct 12, 2020

aiordache Oct 13, 2020

chris-crone Oct 8, 2020

thaJeztah Oct 8, 2020

thaJeztah Oct 8, 2020

chris-crone left a comment

chris-crone Oct 12, 2020

aiordache Oct 13, 2020

chris-crone Oct 12, 2020

chris-crone Oct 12, 2020

aiordache Oct 13, 2020

		@@ -1,3 +1,6 @@
		TEST_API_VERSION ?= 1.39
		TEST_ENGINE_VERSION ?= 19.03.12

		RUN chmod 600 /root/.ssh/id_rsa && \
		chmod 600 /root/.ssh/id_rsa.pub

	RUN chmod 600 /root/.ssh/id_rsa && \
	chmod 600 /root/.ssh/id_rsa.pub
	RUN chmod -R 600 /root/.ssh/

	RUN chmod 600 /root/.ssh/id_rsa && \
	chmod 600 /root/.ssh/id_rsa.pub
	RUN chmod -R 600 /root/.ssh

Shell out to local SSH client as alternative to a paramiko connection #2680

Shell out to local SSH client as alternative to a paramiko connection #2680

Conversation

aiordache commented Oct 5, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-crone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris-crone left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment