Merge lp:~abentley/workspace-runner/known-hosts into lp:workspace-runner

Proposed by Aaron Bentley on 2015-07-13
Status: Merged
Merged at revision: 23
Proposed branch: lp:~abentley/workspace-runner/known-hosts
Merge into: lp:workspace-runner
Diff against target: 128 lines (+40/-7)
2 files modified
workspace_runner/__init__.py (+24/-7)
workspace_runner/tests/__init__.py (+16/-0)
To merge this branch: bzr merge lp:~abentley/workspace-runner/known-hosts
Reviewer Review Type Date Requested Status
Martin Packman (community) 2015-07-13 Approve on 2015-07-13
Review via email: mp+264565@code.launchpad.net

Commit message

Reduce added-host warnings

Description of the change

This branch improves the output of workspace-runner.

It switches to using a per-session known hosts file, to avoid repeated messages about adding a machine to known hosts.

It demotes the near-useless "Running python -" message to a debug message, and adds high-level messages to the methods that use run_python: mktemp, mkdir and destroy.

To post a comment you must log in.
Martin Packman (gz) wrote :

Looks fine.

The next step however, if we want to add more ssh configuration, should be just having a dedicated ssh config dir for the SSHConnection object, rather than giving everything as command line options. That gives use true isolation from the user's ssh config and we can be precise about what elements we want to preserve.

review: Approve
Aaron Bentley (abentley) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2015-07-13 09:46 AM, Martin Packman wrote:
> Review: Approve
>
> Looks fine.
>
> The next step however, if we want to add more ssh configuration,
> should be just having a dedicated ssh config dir for the
> SSHConnection object, rather than giving everything as command line
> options. That gives use true isolation from the user's ssh config
> and we can be precise about what elements we want to preserve.

I'm not sure I agree. It's nice that the overrides appear in log
messages, and it would be a pain deciding what to preserve and what to
override.

>> + options = list(chain(*[['-o', o] for o in
>> config_options]))
>
> Heh, and you complained about me using map()? :)
>
> You can just do this:
>
> options = ["-o" + o for f in config_options]
>
> The ssh command line parser is fine with that.

In this specific case, where ['-ofoo'] is equivalent to ['-o', 'foo'],
sure. In general, list(chain(*my_lists)) is the least obscure way I
can find of generating a single large list containing all the elements
of a list of lists.

>> if self.private_key is not None: options.extend(['-i',
>> self.private_key]) return options @@ -190,8 +204,11 @@ except
>> Exception as e: logging.exception(e) sys.exit(1) +
>> finally: + primitives.destroy() finally: -
>> primitives.destroy() + connection.known_hosts = None
>
> Unsetting the known_hosts attribute is a little unpleasant. Would
> prefer if SSHConnection just had responsibility for the file.

Maybe in the abstract, but it's a public attribute, so there's nothing
actually wrong with setting/unsetting it. I didn't think it was worth
writing another context manager (e.g. SSHConnection.known_hosts) and
having another layer of indirection just to manage the known hosts file.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJVo8U+AAoJEK84cMOcf+9h1x0IALQpJ8dc9iuTA9fMgMs8aSux
qHxNKc5BHmFzNPrX+eXIJnES6+TCwO+vcjlenNAVEmD/ovw3rpEh6PDOdJCvksqf
JDEc273NtXA+985wOCIae71iF+Qi4ogucAoKZTzxOwND3RPzUqkZ5Fe/NAxB/t3c
uJfacxjTvM6JL8KEQUsz7UBUugo7pRQED6BVqxzE72xRwNSBWxjPMUkyljrWQzxC
3R2PgPbC4LuuNqdFj6PZaTQ1V0f+ZeUMKhlNl/qXbsZaumme13rZRhMVJVa5UlXm
5yKZq8tOf812BlG9ACA0S/8ahKniHg9u9piIteFhc4ppyr3G2Z2LzpNA7xjXRUs=
=TbsU
-----END PGP SIGNATURE-----

Martin Packman (gz) wrote :

> > The next step however, if we want to add more ssh configuration,
> > should be just having a dedicated ssh config dir for the
> > SSHConnection object, rather than giving everything as command line
> > options. That gives use true isolation from the user's ssh config
> > and we can be precise about what elements we want to preserve.
>
> I'm not sure I agree. It's nice that the overrides appear in log
> messages, and it would be a pain deciding what to preserve and what to
> override.

It's the only way we can actually control which keys are visible to ssh process, we just don't get good isolation without it.

> In this specific case, where ['-ofoo'] is equivalent to ['-o', 'foo'],
> sure. In general, list(chain(*my_lists)) is the least obscure way I
> can find of generating a single large list containing all the elements
> of a list of lists.

I'm generally fine with confusing code when it comes with tests. For-loop and extend is also a way of doing this.

> Maybe in the abstract, but it's a public attribute, so there's nothing
> actually wrong with setting/unsetting it. I didn't think it was worth
> writing another context manager (e.g. SSHConnection.known_hosts) and
> having another layer of indirection just to manage the known hosts file.

I agree. However, I do think SSHConnection should be managing a tempdir of config/known_hosts/key symlinks at some point, so will require that.

Aaron Bentley (abentley) wrote :

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 2015-07-13 10:10 AM, Martin Packman wrote:
>>> The next step however, if we want to add more ssh
>>> configuration, should be just having a dedicated ssh config dir
>>> for the SSHConnection object, rather than giving everything as
>>> command line options. That gives use true isolation from the
>>> user's ssh config and we can be precise about what elements we
>>> want to preserve.
>>
>> I'm not sure I agree. It's nice that the overrides appear in
>> log messages, and it would be a pain deciding what to preserve
>> and what to override.
>
> It's the only way we can actually control which keys are visible to
> ssh process, we just don't get good isolation without it.

I don't agree that controlling which keys are visible to the ssh
process should be a goal. For the interactive user, it would be
annoying if the workspace runner ignored their host-specific settings
(eg. ProxyCommand). For the non-interactive user, we can supply the
configuration we want to workspace-runner.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJVo8x5AAoJEK84cMOcf+9hohUH/1wSeRXHS42VBknwsjBc1ZXm
6qfLGDJNdFJtYO1zeniX3t19fFs0fohzDEd92CT3ojOLdqsrWzgmONnfgK2nQr5n
JKnZaY6JmiWVC/EpP+RvXYN3vSxI+b2pXVcaLBIzc96lbwEWhZgBrFl7t4pdxn58
7XpHI/ApapgXHXESIXXThcEWew5PVwDAP3/9sjPfBYz4Qf5+8GzHRBC4l91ro69w
VUfy7pDDyL29WW9SIe81GjAp1btvWkRU6EKmK9Ok/bj8OUOJiqfZKRnqXPqkDi3l
C5W+tdPtE/9pFhbCulFKVHelZJuXQ9rCbl4y0KNavFZjWKUXyD1Pvh9sKGZdxXs=
=IcDw
-----END PGP SIGNATURE-----

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'workspace_runner/__init__.py'
2--- workspace_runner/__init__.py 2015-06-30 16:23:45 +0000
3+++ workspace_runner/__init__.py 2015-07-13 13:29:01 +0000
4@@ -9,7 +9,10 @@
5 from shutil import rmtree
6 import subprocess
7 import sys
8-from tempfile import mkdtemp
9+from tempfile import (
10+ mkdtemp,
11+ NamedTemporaryFile,
12+ )
13 from textwrap import dedent
14
15 from yaml import (
16@@ -80,12 +83,17 @@
17 def __init__(self, host, private_key):
18 self.host = host
19 self.private_key = private_key
20- self.ssh_options = [
21- 'StrictHostKeyChecking=no', 'UserKnownHostsFile=/dev/null']
22+ self.known_hosts = None
23+ self.ssh_options = ['StrictHostKeyChecking=no']
24
25 def get_ssh_options(self):
26 """Return the standard options for this SSH connection."""
27- options = list(chain(*[['-o', o] for o in self.ssh_options]))
28+ config_options = list(self.ssh_options)
29+ known_hosts = self.known_hosts
30+ if known_hosts is None:
31+ known_hosts = '/dev/null'
32+ config_options.append('UserKnownHostsFile={}'.format(known_hosts))
33+ options = list(chain(*[['-o', o] for o in config_options]))
34 if self.private_key is not None:
35 options.extend(['-i', self.private_key])
36 return options
37@@ -100,7 +108,7 @@
38 """Run a command remotely, accepting stdin."""
39 remote_command = ' '.join(quote(x) for x in args)
40 command = self.get_ssh_cmd([remote_command])
41- logging.info('Running: {}'.format(' '.join(command)))
42+ logging.debug('Running: {}'.format(' '.join(command)))
43 proc = subprocess.Popen(command, stdin=subprocess.PIPE,
44 stdout=subprocess.PIPE)
45 output = proc.communicate(stdin)
46@@ -122,6 +130,7 @@
47 class SSHPrimitives(Primitives):
48
49 def mkdir(self, path):
50+ logging.info('Remote mkdir {}'.format(path))
51 return self.run_python(self.ssh_connection, dedent("""
52 import os
53 import sys
54@@ -132,6 +141,7 @@
55 @classmethod
56 def mktemp(cls, ssh_connection):
57 """Make a temporary directory on the specified host via ssh."""
58+ logging.info('Remote mktmp')
59 return cls.run_python(ssh_connection, dedent("""
60 import tempfile
61 print tempfile.mkdtemp()
62@@ -139,6 +149,7 @@
63
64 def destroy(self):
65 """Destroy the workspace."""
66+ logging.info('Destroying workspace'.format(self.workspace))
67 return self.run_python(self.ssh_connection, dedent("""
68 import shutil
69 import sys
70@@ -180,8 +191,11 @@
71 The primitives_factory is used to create a Primitives, and the resulting
72 primitive is used to destroy it at the end.
73 """
74- primitives = primitives_factory.create(SSHConnection(host, private_key))
75+ connection = SSHConnection(host, private_key)
76+ known_hosts = NamedTemporaryFile()
77 try:
78+ connection.known_hosts = known_hosts.name
79+ primitives = primitives_factory.create(connection)
80 try:
81 yield primitives
82 except subprocess.CalledProcessError as e:
83@@ -190,8 +204,11 @@
84 except Exception as e:
85 logging.exception(e)
86 sys.exit(1)
87+ finally:
88+ primitives.destroy()
89 finally:
90- primitives.destroy()
91+ connection.known_hosts = None
92+ known_hosts.close()
93
94
95 @contextmanager
96
97=== modified file 'workspace_runner/tests/__init__.py'
98--- workspace_runner/tests/__init__.py 2015-06-30 16:23:45 +0000
99+++ workspace_runner/tests/__init__.py 2015-07-13 13:29:01 +0000
100@@ -209,6 +209,14 @@
101 '-o', 'UserKnownHostsFile=/dev/null',
102 '-i', 'private-key'])
103
104+ def test_ssh_options_known_hosts(self):
105+ connection = SSHConnection('asdf', None)
106+ connection.known_hosts = 'foo/bar/baz'
107+ self.assertEqual(
108+ connection.get_ssh_options(), [
109+ '-o', 'StrictHostKeyChecking=no',
110+ '-o', 'UserKnownHostsFile=foo/bar/baz'])
111+
112 def test_call_with_input_error(self):
113 connection = SSHConnection('host', 'key')
114 with patch('subprocess.Popen') as po_mock:
115@@ -363,6 +371,14 @@
116 self.assertTrue(os.path.exists(primitives.workspace))
117 self.assertFalse(os.path.exists(primitives.workspace))
118
119+ def test_known_hosts(self):
120+ with workspace_context('asdf', None, FakePrimitives) as primitives:
121+ conn = primitives.ssh_connection
122+ known_hosts = conn.known_hosts
123+ self.assertTrue(os.path.isfile(known_hosts))
124+ self.assertFalse(os.path.exists(known_hosts))
125+ self.assertIs(conn.known_hosts, None)
126+
127
128 class TestRunFromConfig(TestCase):
129

Subscribers

People subscribed via source and target branches