Merge lp:~jtv/maas-test/ssh-key into lp:maas-test

[Jeroen T. Vermeulen]

Set up temporary, passphrase-less ssh key pair for communicating with the virtual machine.

The keypair gets created during KVMFixture.setUp(). After I was done with the tests, I thought implementation would be easy. But I couldn't get my tests to pass! Turns out the tests' setUp() was patching out run_command, and so ssh-keygen was never actually being run. I had to make that patch explicit, in only the tests for which it was appropriate.

Some other tests were a bit brittle, expecting too much detail about command lines being run, so that small changes required updates to seemingly tests. I made the tests look for more specific changes, leaving out the irrelevant. It's not as easy, and sometimes I think test output will suffer, but hopefully that's still better than a future where we mindlessly update tests to match whatever actually happens.

Along the way, I also remove some TODOs that no longer match the evolution of the design. And a few tests were unnecessarily nested in "with:" blocks. I don't like unnecessary nesting. Instead, I keep just the necessary data-gathering inside the nested block, and do the testing at the base indentation level.

Jeroen

[lp:~jtv/maas-test/ssh-key]

18. By Jeroen T. Vermeulen on 2013-11-08

Merge trunk, resolve conflicts.

[Raphaël Badin (rvb)]

Not a proper review but a couple of remarks:
- The main functionality you're adding here seems fine to me.
- You probably need to merge trunk as I've made changes in the same area recently (test_kvmfixture.py).
- I'm really not fond of find_matching_call() and the refactoring that goes with. I think you're relaxing the assertions done in the tests too much with this. In my opinion, it's better to keep checking the complete commands that are run. It's a sort of double-entry bookkeeping system that is there to guarantee that any change to the code is voluntary and I don't think the tests are brittle at all.

[Raphaël Badin (rvb)]

One more thing: I think it would be better to put the keys in a fixed place like ~/.maas-test/key[,.pub] to help debugging. I suspect it will be very useful to be able to manually log into the VM while maas-test is running (and actually, I've used that a lot already when working on the networking stuff) and that will be more easy to do if the private ssh key is in a fixed place rather than somewhere in /tmp/.

[Jeroen T. Vermeulen (jtv)]

> - I'm really not fond of find_matching_call() and the refactoring that goes
> with. I think you're relaxing the assertions done in the tests too much with
> this. In my opinion, it's better to keep checking the complete commands that
> are run. It's a sort of double-entry bookkeeping system that is there to
> guarantee that any change to the code is voluntary and I don't think the tests
> are brittle at all.

I see that there's some missing test coverage on setUp as a whole, and I'll fix that. But there is no refactoring here that went with find_matching_call — it's entirely the other way around. I first ran into the problem of test brittleness. Several tests really were going to break because of unrelated changes to command lines. The choice was one of increasing it or decreasing it, and I first specified find_matching_call to avoid increasing it. I only wrote the actual function once I saw that it allowed me to solve that problem. Adding another option to the ssh command line will now break one test, but no more.

Tests are like double-entry bookkeeping, but double-entry bookkeeping is not writing down the same transaction in multiple places like these tests were doing. Rather, it's a system of recording complementary aspects of a transaction. Failure of the two records to match up is a crucial signal that something went wrong. False positives, and the habit of updating the tests to match the code, degrade the value of that signal. That's how double-entry bookkeeping devolves towards forms-in-triplicate: a bureaucratic exercise that slows things down and confuses, and doesn't improve quality much. It's also self-reinforcing, which is why it's so important to fight the tendency.

[Jeroen T. Vermeulen (jtv)]

> One more thing: I think it would be better to put the keys in a fixed place
> like ~/.maas-test/key[,.pub] to help debugging. I suspect it will be very
> useful to be able to manually log into the VM while maas-test is running (and
> actually, I've used that a lot already when working on the networking stuff)
> and that will be more easy to do if the private ssh key is in a fixed place
> rather than somewhere in /tmp/.

These are fleeting, single-use keys, so keeping them in a permanent location does not make sense to me from a lifetime point of view. What you need for debugging is *access* to the keys, not a fixed location. There are other ways to skin that cat, such as logging the key we use.

[Julian Edwards (julian-edwards)]

First comment - I don't think this key should be fleeting, I agree with Raphaël and his reasoning here. Generate it on demand if it doesn't exist and stick it in ~/.maas-test.

[Julian Edwards (julian-edwards)]

43 + def get_ssh_public_key(self):

This is returning a path to a key, not a key per se. It's worth renaming to avoid confusion.

128 + self.patch(kvmfixture, 'sleep', mock.MagicMock())

You don't need the third param here, it defaults to that.

233 + self.assertIsNotNone(
234 + self.find_matching_call(['sudo', 'uvt-kvm', 'create']))

Get rid of this and find_matching_call() and use Mock's assert_has_calls() or assert_any_call().

236 + self.assertEqual(
237 + [
238 + mock.call(interface),
239 + ],
240 + mock_make_kvm_template.mock_calls)

And here! And a few other places in the code.

[lp:~jtv/maas-test/ssh-key]

19. By Jeroen T. Vermeulen on 2013-11-11

Merge trunk, resolve conflicts. Not trivial.

[Jeroen T. Vermeulen (jtv)]

> 43 + def get_ssh_public_key(self):
>
> This is returning a path to a key, not a key per se. It's worth renaming to
> avoid confusion.

OK.

> 128 + self.patch(kvmfixture, 'sleep', mock.MagicMock())
>
> You don't need the third param here, it defaults to that.

In MAAS it does. Here, it doesn't and so I do.

> 233 + self.assertIsNotNone(
> 234 + self.find_matching_call(['sudo', 'uvt-kvm', 'create']))
>
> Get rid of this and find_matching_call() and use Mock's assert_has_calls() or
> assert_any_call().

Doesn't work — mock.ANY can be useful for pattern-matching on the call itself, but not within the list that is being passed here. :(

[lp:~jtv/maas-test/ssh-key]

20. By Jeroen T. Vermeulen on 2013-11-11

Review change: rename ssh_{public,private}_key to say they're filenames.

21. By Jeroen T. Vermeulen on 2013-11-11

Tests for review change: generate ssh keys in home directory, and keep them persistent. Tests: 2 failures.

22. By Jeroen T. Vermeulen on 2013-11-11

Move ssh keys into home directory. Satisfy tests.

23. By Jeroen T. Vermeulen on 2013-11-11

Test read_file.

[Jeroen T. Vermeulen (jtv)]

The branch has grown a bit. I had to remove SSH_COMMAND along the way. It still broke with the changes to the ssh command. Instead, I added a method that generates the ssh command line.

The ssh keys are in a fixed location now, so it would be possible to go back to using SSH_COMMAND with some trickery, but even then we'd have the ugly situation that it couldn't contain the full command line — so tests still had to extend that list for themselves. What we have now neatly eliminates that intimacy between the tests and the main code.

[Graham Binns (gmb)]

After all the conversation heretofore, I rather feel like my job is an easy one :). This is a good branch; nice work Jeroen.

review: Approve

=== modified file 'maastest/kvmfixture.py'
--- maastest/kvmfixture.py	2013-11-08 15:33:37 +0000
+++ maastest/kvmfixture.py	2013-11-11 10:35:06 +0000
@@ -17,6 +17,7 @@
 
 import itertools
 import logging
+import os.path
 from pipes import quote
 import textwrap
 from time import sleep
@@ -28,12 +29,6 @@
 from testtools.content import text_content
 
 
-SSH_COMMAND = [
-    "ssh", "-o", "LogLevel=quiet", "-o",
-    "UserKnownHostsFile=/dev/null", "-o",
-    "StrictHostKeyChecking=no"]
-
-
 class KVMFixture(Fixture):
     """A fixture for a Kernel-based Virtual Machine.
 
@@ -90,16 +85,16 @@
             assert direct_ip is not None, (
                 "'direct_ip' cannot be empty if 'direct_interface' is not "
                 "provided")
-        # TODO: add an option to prepare (and later reuse) an existing
-        # KVM instance.
         if name is None:
             name = unicode(uuid1())
         self.name = name
+        self.ssh_private_key_file = os.path.join(
+            os.path.expanduser('~'), '.maas-test', 'vm_ssh_id_rsa')
 
     def setUp(self):
         super(KVMFixture, self).setUp()
-        # TODO: check that there is a ssh public/private key pair set up.
         self.import_image()
+        self.generate_ssh_key()
         self.start()
         self.configure()
         self.addCleanup(self.destroy)
@@ -115,6 +110,10 @@
             ["uvt-kvm", "ip", self.name], check_call=True)
         return ip_address.strip()
 
+    def get_ssh_public_key_file(self):
+        """Return path to public SSH key file, for VM sshd configuration."""
+        return self.ssh_private_key_file + '.pub'
+
     def import_image(self):
         """Download the image required to create the virtual machine."""
         logging.info(
@@ -128,18 +127,36 @@
             "Done downloading KVM image for series=%s, arch=%s.",
             self.series, self.architecture)
 
+    def generate_ssh_key(self):
+        """Set up an ssh key pair for accessing the virtual machine.
+
+        The key pair will be in a directory `~/.maas-test/`.  If this did not
+        exist yet, it will be created.
+        """
+        key_dir = os.path.dirname(self.ssh_private_key_file)
+        if not os.path.exists(key_dir):
+            os.makedirs(key_dir, mode=0700)
+        if not os.path.exists(self.ssh_private_key_file):
+            run_command(
+                [
+                    'ssh-keygen',
+                    '-t', 'rsa',
+                    '-f', self.ssh_private_key_file,
+                    '-N', '',
+                ],
+                check_call=True)
+
     def start(self):
         """Create and start the virtual machine.
 
         When this method returns, the VM is ready to be used."""
-        # TODO: check if already running (in case this is reusing an
-        # existing instance).
         logging.info(
             "Creating virtual machine %s, arch=%s.", self.name,
             self.architecture)
         template = make_kvm_template(self.direct_interface)
         run_command([
             "sudo", "uvt-kvm", "create", self.name,
+            "--ssh-public-key-file=%s" % self.get_ssh_public_key_file(),
             "arch=%s" % self.architecture,
             "release=%s" % self.series, "--template", "-"],
             input=template, check_call=True)
@@ -178,12 +195,31 @@
             ["sudo", "uvt-kvm", "destroy", self.name], check_call=True)
         logging.info("Done destroying virtual machine %s.", self.name)
 
+    def _make_ssh_command(self, command_line):
+        """Compose an `ssh` command line to execute `command_line` in the VM.
+
+        :param command_line: Shell command to be executed on the virtual
+            machine, in the form of a sequence of arguments (starting with the
+            executable).
+        :return: An `ssh` command line to execute `command_line` on the virtual
+            machine, in the form of a list.
+        """
+        remote_command_line = ' '.join(map(quote, command_line))
+        return [
+            "ssh",
+            "-i", self.ssh_private_key_file,
+            "-o", "LogLevel=quiet",
+            "-o", "UserKnownHostsFile=/dev/null",
+            "-o", "StrictHostKeyChecking=no",
+            self.identity(),
+            remote_command_line,
+            ]
+
     def run_command(self, args, input=None, check_call=False):
         """Run the given command in the VM."""
-        cmd = " ".join(map(quote, args))
-        args = SSH_COMMAND + [self.identity(), cmd]
+        args = self._make_ssh_command(args)
         retcode, stdout, stderr = run_command(
-            args, input=input, check_call=check_call)
+            args, check_call=check_call, input=input)
         count = next(self.command_count)
         self.addDetail('cmd #%d' % count, text_content(' '.join(args)))
         self.addDetail('cmd #%d retcode' % count, text_content(str(retcode)))

=== modified file 'maastest/tests/test_kvmfixture.py'
--- maastest/tests/test_kvmfixture.py	2013-11-08 14:26:53 +0000
+++ maastest/tests/test_kvmfixture.py	2013-11-11 10:35:06 +0000
@@ -12,25 +12,50 @@
 __metaclass__ = type
 __all__ = []
 
+import os.path
+from random import randint
 import textwrap
 
 import fixtures
 from lxml import etree
 from maastest import kvmfixture
+from maastest.utils import read_file
 import mock
 import netaddr
 import testtools
-from testtools.content import text_content
+from testtools.matchers import (
+    FileContains,
+    FileExists,
+    HasPermissions,
+    MatchesRegex,
+    Not,
+    StartsWith,
+    )
 
 
 class TestKVMFixture(testtools.TestCase):
 
     def setUp(self):
         super(TestKVMFixture, self).setUp()
+        self.logger = self.useFixture(fixtures.FakeLogger())
+        self.patch(kvmfixture, 'sleep', mock.MagicMock())
+        # KVMFixture.generate_ssh_key() generates an ssh key in ~/.maas-test,
+        # creating that directory if necessary.  Set up a fake home directory
+        # so it can do what it needs to.
+        home_dir = self.useFixture(fixtures.TempDir()).path
+        self.patch(
+            os.path, 'expanduser', mock.MagicMock(return_value=home_dir))
+
+    def patch_run_command(self):
+        """Patch out `kvmfixture` calls to `run_command`.
+
+        Call this so your test doesn't accidentally make `KVMFixture` download
+        machine images, ssh into it, etc.
+
+        You can access the fake version as `self.mock_run_command`.
+        """
         self.mock_run_command = mock.MagicMock()
-        self.logger = self.useFixture(fixtures.FakeLogger())
         self.patch(kvmfixture, 'run_command', self.mock_run_command)
-        self.patch(kvmfixture, 'sleep', mock.MagicMock())
 
     def make_KVMFixture(self, series=None, architecture=None, name=None,
                         direct_interface=None, direct_network=None,
@@ -50,15 +75,39 @@
         fixture._details = {}
         return fixture
 
+    def find_matching_call(self, command_prefix):
+        """Find the first `run_command` call with a matching command line.
+
+        This assumes that you have called `patch_run_command` first.
+
+        :param command_prefix: A sequence of items on the command line.  A
+            call to `run_commanad` will match this when this forms a prefix of
+            the executed command line.  For instance, a `command_prefix` of
+            `['ls']` will match a command line `['ls', '/tmp']` but not
+            a command line `['sudo', 'ls', '/tmp']`.  On the other hand, a
+            prefix `['sudo', 'ls']` will match `['sudo', 'ls', '/tmp']` but
+            not `['ls', '/tmp']`.
+        :rtype: `mock.call`
+        """
+        command_prefix = list(command_prefix)
+        prefix_length = len(command_prefix)
+        for invocation in self.mock_run_command.mock_calls:
+            _, args, _ = invocation
+            command_line = args[0]
+            if command_line[:prefix_length] == command_prefix:
+                return invocation
+        return None
+
     def test_init_sets_properties(self):
         series = "test-series"
         architecture = "test-arch"
-        with self.make_KVMFixture(series, architecture) as fixture:
-            self.assertEqual(
-                (series, architecture, 36),
-                (fixture.series, fixture.architecture, len(fixture.name)))
+        fixture = kvmfixture.KVMFixture(series, architecture)
+        self.assertEqual(
+            (series, architecture, 36),
+            (fixture.series, fixture.architecture, len(fixture.name)))
 
     def test_ip_address(self):
+        self.patch_run_command()
         series = "test-series"
         architecture = "test-arch"
         ip_address = '192.168.2.1'
@@ -67,6 +116,7 @@
             self.assertEqual(ip_address, fixture.ip_address())
 
     def test_identity(self):
+        self.patch_run_command()
         series = "test-series"
         architecture = "test-arch"
         ip_address = '192.168.2.1'
@@ -75,6 +125,7 @@
             self.assertEqual("ubuntu@%s" % ip_address, fixture.identity())
 
     def test_setUp_calls_machine_creation_methods(self):
+        self.patch_run_command()
         mock_import_image = mock.MagicMock()
         self.patch(kvmfixture.KVMFixture, 'import_image', mock_import_image)
         mock_start = mock.MagicMock()
@@ -99,32 +150,39 @@
     def test_start_starts_vm(self):
         series = "test-series"
         architecture = "test-arch"
+        self.patch_run_command()
+        self.patch(kvmfixture.KVMFixture, 'generate_ssh_key', mock.MagicMock())
         mock_make_kvm_template = mock.MagicMock()
         kvm_template = self.getUniqueString()
         mock_make_kvm_template.return_value = kvm_template
         self.patch(kvmfixture, 'make_kvm_template', mock_make_kvm_template)
+
         with self.make_KVMFixture(series, architecture) as fixture:
-            self.assertEqual(
-                [
-                    mock.call([
-                        'sudo', 'uvt-simplestreams-libvirt', 'sync',
-                        'arch=%s' % architecture, 'release=%s' % series],
-                        check_call=True),
-                    mock.call([
-                        'sudo', 'uvt-kvm', 'create', fixture.name,
-                        'arch=%s' % architecture, 'release=%s' % series,
-                        '--template', '-'],
-                        input=kvm_template,
-                        check_call=True),
-                ],
-                self.mock_run_command.mock_calls)
-            self.assertEqual(
-                [
-                    mock.call(None),
-                ],
-                mock_make_kvm_template.mock_calls)
+            name = fixture.name
+            public_key = fixture.get_ssh_public_key_file()
+
+        self.mock_run_command.assert_has_calls(
+            [
+                mock.call([
+                    'sudo', 'uvt-simplestreams-libvirt', 'sync',
+                    'arch=%s' % architecture, 'release=%s' % series],
+                    check_call=True),
+                mock.call([
+                    'sudo', 'uvt-kvm', 'create', name,
+                    '--ssh-public-key-file=%s' % public_key,
+                    'arch=%s' % architecture, 'release=%s' % series,
+                    '--template', '-'],
+                    input=kvm_template,
+                    check_call=True),
+            ])
+        self.assertEqual(
+            [
+                mock.call(None),
+            ],
+            mock_make_kvm_template.mock_calls)
 
     def test_import_image_logs_to_root_logger(self):
+        self.patch_run_command()
         fixture = self.make_KVMFixture()
         fixture.import_image()
         self.assertEqual(
@@ -136,8 +194,87 @@
             ],
             self.logger.output.strip().split("\n"))
 
+    def test_generate_ssh_key_sets_up_temporary_key_pair(self):
+        home_dir = os.path.expanduser('~')
+        fixture = self.make_KVMFixture()
+        fixtures.Fixture.setUp(fixture)
+
+        fixture.generate_ssh_key()
+
+        self.assertThat(fixture.ssh_private_key_file, FileExists())
+        self.assertThat(fixture.get_ssh_public_key_file(), FileExists())
+        self.assertThat(
+            fixture.ssh_private_key_file, StartsWith(home_dir + '/'))
+
+        self.assertThat(
+            fixture.ssh_private_key_file,
+            FileContains(
+                matcher=StartsWith('-----BEGIN RSA PRIVATE KEY-----')))
+        self.assertThat(
+            fixture.get_ssh_public_key_file(),
+            FileContains(matcher=StartsWith('ssh-rsa')))
+
+        # Other users are not allowed to read the private key.
+        self.assertThat(fixture.ssh_private_key_file, HasPermissions('0600'))
+
+        # The key pair is not protected by a passphrase, as indicated by
+        # the absence of this line.
+        self.assertThat(
+            fixture.ssh_private_key_file,
+            Not(FileContains(matcher=MatchesRegex("^Proc-Type: .*ENCRYPTED"))))
+
+    def test_generate_ssh_key_reuses_existing_key_pair(self):
+        earlier_fixture = self.make_KVMFixture()
+        fixtures.Fixture.setUp(earlier_fixture)
+        earlier_fixture.generate_ssh_key()
+        earlier_private_key_contents = read_file(
+            earlier_fixture.ssh_private_key_file)
+        earlier_public_key_contents = read_file(
+            earlier_fixture.get_ssh_public_key_file())
+
+        later_fixture = self.make_KVMFixture()
+        fixtures.Fixture.setUp(later_fixture)
+        later_fixture.generate_ssh_key()
+
+        self.assertEqual(
+            earlier_fixture.ssh_private_key_file,
+            later_fixture.ssh_private_key_file)
+        self.assertEqual(
+            earlier_fixture.get_ssh_public_key_file(),
+            later_fixture.get_ssh_public_key_file())
+        self.assertThat(
+            later_fixture.ssh_private_key_file,
+            FileContains(earlier_private_key_contents))
+        self.assertThat(
+            later_fixture.get_ssh_public_key_file(),
+            FileContains(earlier_public_key_contents))
+
+    def test_start_passes_public_ssh_key(self):
+        self.patch_run_command()
+
+        with self.make_KVMFixture() as fixture:
+            fixture.start()
+
+        uvt_call = self.find_matching_call(['sudo', 'uvt-kvm'])
+        self.assertIsNotNone(uvt_call)
+        _, args, _ = uvt_call
+        command_line = args[0]
+        self.assertIn(
+            '--ssh-public-key-file=%s' % fixture.get_ssh_public_key_file(),
+            command_line)
+
+    def test_get_ssh_public_key_file_returns_public_key(self):
+        self.patch_run_command()
+
+        with self.make_KVMFixture() as fixture:
+            public_key = fixture.get_ssh_public_key_file()
+
+        self.assertEqual(fixture.ssh_private_key_file + '.pub', public_key)
+
     def test_start_logs_to_root_logger(self):
+        self.patch_run_command()
         fixture = self.make_KVMFixture()
+
         fixture.start()
         self.assertEqual(
             [
@@ -149,6 +286,7 @@
             self.logger.output.strip().split("\n"))
 
     def test_destroy_logs_to_root_logger(self):
+        self.patch_run_command()
         fixture = self.make_KVMFixture()
         fixture.destroy()
         self.assertEqual(
@@ -159,37 +297,36 @@
             self.logger.output.strip().split("\n"))
 
     def test_configure_configures_network(self):
+        self.patch_run_command()
         series = "test-series"
         architecture = "test-arch"
         mock_identity = mock.MagicMock()
         mock_identity.return_value = "ubuntu@test"
         self.patch(kvmfixture.KVMFixture, 'identity', mock_identity)
         network = netaddr.IPNetwork('192.168.12.3/24')
-        self.mock_run_command.return_value = (0, '', '')
         ip = "192.168.12.22"
         interface = self.getUniqueString()
         fixture = self.make_KVMFixture(
             series, architecture, direct_interface=interface,
             direct_network=network, direct_ip=ip)
+        kvm_run_command = mock.MagicMock(return_value=(0, '', ''))
+        self.patch(kvmfixture.KVMFixture, 'run_command', kvm_run_command)
 
         fixture.configure()
 
-        netconfig_command = (
-            kvmfixture.SSH_COMMAND +
-            [fixture.identity(), "sudo tee -a /etc/network/interfaces"])
+        netconfig_command = ["sudo", "tee", "-a", "/etc/network/interfaces"]
         input = kvmfixture.make_network_interface_config(
             'eth1', ip, network)
-        netup_command = (
-            kvmfixture.SSH_COMMAND +
-            [fixture.identity(), "sudo ifup eth1"])
+        netup_command = ["sudo", "ifup", "eth1"]
         self.assertEqual(
             [
                 mock.call(netconfig_command, input=input, check_call=True),
-                mock.call(netup_command, input=None, check_call=True),
+                mock.call(netup_command, check_call=True),
             ],
-            self.mock_run_command.mock_calls)
+            kvm_run_command.mock_calls)
 
     def test_machine_is_destroyed(self):
+        self.patch_run_command()
         series = "test-series"
         architecture = "test-arch"
         with kvmfixture.KVMFixture(series, architecture) as fixture:
@@ -201,41 +338,56 @@
             self.mock_run_command.mock_calls[-1])
 
     def test_run_command_runs_command_remotely(self):
+        self.patch_run_command()
         series = "test-series"
         architecture = "test-arch"
         input = self.getUniqueString()
         self.mock_run_command.return_value = (0, '', '')
+        command = ["echo", "Hello"]
         with self.make_KVMFixture(series, architecture) as fixture:
-            fixture.run_command(
-                ["echo", "Hello"], input=input, check_call=False)
-            command = (
-                kvmfixture.SSH_COMMAND + [fixture.identity(), "echo Hello"])
-            self.assertEqual(
-                mock.call(command, input=input, check_call=False),
-                self.mock_run_command.mock_calls[-2])
+            expected_command = fixture._make_ssh_command(command)
+            fixture.run_command(command, input=input, check_call=False)
+            ssh_command = self.mock_run_command.mock_calls[-1]
+
+        self.assertEqual(
+            mock.call(expected_command, input=input, check_call=False),
+            ssh_command)
 
     def test_run_command_adds_details(self):
+        self.patch_run_command()
         series = "test-series"
         architecture = "test-arch"
+        return_value = randint(0, 3)
         input = self.getUniqueString()
         stdout_content = self.getUniqueString()
         stderr_content = self.getUniqueString()
         self.mock_run_command.return_value = (
-            0, stdout_content, stderr_content)
+            return_value,
+            stdout_content,
+            stderr_content,
+            )
+
         with self.make_KVMFixture(series, architecture) as fixture:
-            expected_args = (
-                kvmfixture.SSH_COMMAND + [fixture.identity(), "echo Hello"])
             fixture.run_command(
                 ["echo", "Hello"], input=input, check_call=False)
-            self.assertEqual(
-                {
-                    'cmd #1': text_content(' '.join(expected_args)),
-                    'cmd #1 retcode': text_content('0'),
-                    'cmd #1 input': text_content(input),
-                    'cmd #1 stdout': text_content(stdout_content),
-                    'cmd #1 stderr': text_content(stderr_content),
-                },
-                fixture.getDetails())
+            details = fixture.getDetails()
+
+        self.assertThat(
+            details['cmd #1'].as_text(),
+            MatchesRegex('ssh .* echo Hello$'))
+        self.assertEqual(
+            (
+                unicode(return_value),
+                input,
+                stdout_content,
+                stderr_content,
+            ),
+            (
+                details['cmd #1 retcode'].as_text(),
+                details['cmd #1 input'].as_text(),
+                details['cmd #1 stdout'].as_text(),
+                details['cmd #1 stderr'].as_text(),
+            ))
 
 
 def xml_normalize(snippet):

=== modified file 'maastest/tests/test_utils.py'
--- maastest/tests/test_utils.py	2013-11-07 14:38:49 +0000
+++ maastest/tests/test_utils.py	2013-11-11 10:35:06 +0000
@@ -12,8 +12,10 @@
 __metaclass__ = type
 __all__ = []
 
+import os.path
 from subprocess import PIPE
 
+from fixtures import TempDir
 from maastest import utils
 import mock
 import testtools
@@ -21,6 +23,18 @@
 
 class TestRunCommand(testtools.TestCase):
 
+    def test_read_file_returns_file_contents_as_bytes(self):
+        temp_dir = self.useFixture(TempDir())
+        sample_file = os.path.join(temp_dir.path, self.getUniqueString())
+        contents = self.getUniqueString().encode('ascii')
+        with open(sample_file, 'wb') as f:
+            f.write(contents)
+
+        read_contents = utils.read_file(sample_file)
+
+        self.assertEqual(contents, read_contents)
+        self.assertIsInstance(contents, bytes)
+
     def test_run_command_calls_Popen(self):
         mock_Popen = mock.MagicMock()
         expected_retcode = self.getUniqueInteger()

=== modified file 'maastest/utils.py'
--- maastest/utils.py	2013-11-07 16:05:06 +0000
+++ maastest/utils.py	2013-11-11 10:35:06 +0000
@@ -11,6 +11,7 @@
 
 __metaclass__ = type
 __all__ = [
+    "read_file",
     "run_command",
     ]
 
@@ -21,6 +22,12 @@
     )
 
 
+def read_file(path):
+    """Return a given file's contents, as `bytes`."""
+    with open(path) as f:
+        return f.read()
+
+
 def run_command(args, input=None, check_call=False):
     """A wrapper to Popen to run commands in the command-line."""
     process = Popen(args, stdout=PIPE, stderr=PIPE, stdin=PIPE, shell=False)