launchpad-reviewers team mailing list archive

[Graham Binns <graham@xxxxxxxxxxxxx>]

Graham Binns has proposed merging lp:~gmb/lpsetup/make-lp-lxc-ip-discoverable-bug-1019181 into lp:lpsetup.

Requested reviews:
  Launchpad code reviewers (launchpad-reviewers)
Related bugs:
  Bug #1019181 in lpsetup: "lpsetup expects lp-lxc-ip to be in /usr/bin; setup.py install it in /usr/local/bin"
  https://bugs.launchpad.net/lpsetup/+bug/1019181

For more details, see:
https://code.launchpad.net/~gmb/lpsetup/make-lp-lxc-ip-discoverable-bug-1019181/+merge/113955

This branch fixes bug 1019181 by changing the LXC_IP_COMMAND constant in settings.py to point to `/usr/bin/env lp-lxc-ip`, thus sidestepping the problem of the .deb and setup.py putting it in different places.

I've also made a driveby change in this branch to further fix init-host's initialize() function, which, by adding a header when it generated, read in and re-wrote the user's SSH keys, would raise an exception every time it ran. It will now only write keys out if it's been passed them to write. I've also added tests for this behaviour.
-- 
https://code.launchpad.net/~gmb/lpsetup/make-lp-lxc-ip-discoverable-bug-1019181/+merge/113955
Your team Launchpad code reviewers is requested to review the proposed merge of lp:~gmb/lpsetup/make-lp-lxc-ip-discoverable-bug-1019181 into lp:lpsetup.

=== modified file 'lpsetup/settings.py'
--- lpsetup/settings.py	2012-07-03 18:13:48 +0000
+++ lpsetup/settings.py	2012-07-09 11:44:19 +0000
@@ -68,7 +68,7 @@
 LXC_GUEST_ARCH = 'i386'
 LXC_GUEST_CHOICES = ('lucid', 'oneiric', 'precise')
 LXC_GUEST_OS = LXC_GUEST_CHOICES[0]
-LXC_IP_COMMAND = ('/usr/bin/lp-lxc-ip', '-i', 'eth0', '-n')
+LXC_IP_COMMAND = ('/usr/bin/env', 'lp-lxc-ip', '-i', 'eth0', '-n')
 LXC_LP_DIR_PATTERN = '/tmp/lxc-lp-*'
 LXC_LP_TEST_DIR_PATTERN = '/var/lib/lxc/{lxc_name}-tmp-*'
 LXC_NAME = 'lptests'

=== modified file 'lpsetup/subcommands/inithost.py'
--- lpsetup/subcommands/inithost.py	2012-07-06 14:59:07 +0000
+++ lpsetup/subcommands/inithost.py	2012-07-09 11:44:19 +0000
@@ -97,6 +97,88 @@
     os.chmod(filename, 0644)
 
 
+def ensure_ssh_keys(ssh_dir, private_key, public_key, valid_ssh_keys,
+                    ssh_key_path):
+    """Ensure that SSH is configured correctly for the user.
+
+    If valid SSH keys are passed to the function, they will be written
+    out to `ssh_key_path`
+
+        >>> public = "Public key"
+        >>> private = "Private key"
+        >>> ssh_dir = "/tmp"
+        >>> ensure_ssh_keys(ssh_dir, private, public, True, '/tmp/test_key')
+        >>> public_keyfile = open('/tmp/test_key.pub', 'r')
+        >>> print public_keyfile.read() # doctest: +ELLIPSIS
+        # This file created at ...
+        Public key
+        <BLANKLINE>
+        >>> private_keyfile = open('/tmp/test_key', 'r')
+        >>> print private_keyfile.read() # doctest: +ELLIPSIS
+        # This file created at ...
+        Private key
+        <BLANKLINE>
+
+    Two other files will have been generated: authorized_keys and known_hosts
+
+        >>> print open(
+        ...     '/tmp/authorized_keys', 'r').read() # doctest: +ELLIPSIS
+        # This file created at ...
+        Public key
+        <BLANKLINE>
+        >>> print open('/tmp/known_hosts', 'r').read() # doctest: +ELLIPSIS
+        # This file created at ...
+        bazaar.launchpad.net ...
+        >>> os.remove('/tmp/test_key')
+        >>> os.remove('/tmp/test_key.pub')
+        >>> os.remove('/tmp/authorized_keys')
+        >>> os.remove('/tmp/known_hosts')
+
+    If valid keys are not passed, new keys will be generated.
+
+        >>> ensure_ssh_keys(ssh_dir, None, None, False, '/tmp/test_key')
+        >>> os.path.exists('/tmp/test_key')
+        True
+        >>> os.path.exists('/tmp/test_key.pub')
+        True
+        >>> os.path.exists('/tmp/authorized_keys')
+        True
+        >>> os.path.exists('/tmp/known_hosts')
+        True
+        >>> os.remove('/tmp/test_key')
+        >>> os.remove('/tmp/test_key.pub')
+        >>> os.remove('/tmp/authorized_keys')
+        >>> os.remove('/tmp/known_hosts')
+
+    """
+    # Set up the user's ssh directory.  The ssh key must be associated
+    # with the lpuser's Launchpad account.
+    mkdirs(ssh_dir)
+    # Generate user ssh keys if none are supplied.
+    pub_key_path = ssh_key_path + '.pub'
+    if not valid_ssh_keys:
+        generate_ssh_keys(ssh_key_path)
+        private_key = open(ssh_key_path).read()
+        public_key = open(pub_key_path).read()
+    auth_file = os.path.join(ssh_dir, 'authorized_keys')
+    known_hosts = os.path.join(ssh_dir, 'known_hosts')
+    known_host_content = subprocess.check_output([
+        'ssh-keyscan', '-t', 'rsa', 'bazaar.launchpad.net'])
+    files_to_write = [
+        (auth_file, public_key, 'a'),
+        (known_hosts, known_host_content, 'a'),
+        ]
+    if valid_ssh_keys:
+        # We only write out the SSH keys if we haven't had to
+        # generate them further up this function.
+        files_to_write.append((ssh_key_path, private_key, 'w'))
+        files_to_write.append((pub_key_path, public_key, 'w'))
+    for filename, contents, mode in files_to_write:
+        contents = "{0}\n{1}".format(get_file_header(), contents)
+        write_file_contents(filename, contents, mode)
+    os.chmod(ssh_key_path, 0600)
+
+
 def initialize(
     user, full_name, email, lpuser, private_key, public_key, valid_ssh_keys,
     ssh_key_path, feed_random):
@@ -108,29 +190,10 @@
     if not user_exists(user):
         call('useradd', '-m', '-s', '/bin/bash', '-U', user)
     with su(user) as env:
-        # Set up the user's ssh directory.  The ssh key must be associated
-        # with the lpuser's Launchpad account.
         ssh_dir = os.path.join(env.home, '.ssh')
-        mkdirs(ssh_dir)
-        # Generate user ssh keys if none are supplied.
-        pub_key_path = ssh_key_path + '.pub'
-        if not valid_ssh_keys:
-            generate_ssh_keys(ssh_key_path)
-            private_key = open(ssh_key_path).read()
-            public_key = open(pub_key_path).read()
-        auth_file = os.path.join(ssh_dir, 'authorized_keys')
-        known_hosts = os.path.join(ssh_dir, 'known_hosts')
-        known_host_content = subprocess.check_output([
-            'ssh-keyscan', '-t', 'rsa', 'bazaar.launchpad.net'])
-        for filename, contents, mode in [
-            (ssh_key_path, private_key, 'w'),
-            (pub_key_path, public_key, 'w'),
-            (auth_file, public_key, 'a'),
-            (known_hosts, known_host_content, 'a'),
-            ]:
-            contents = "{0}\n{1}".format(get_file_header(), contents)
-            write_file_contents(filename, contents, mode)
-        os.chmod(ssh_key_path, 0600)
+        ensure_ssh_keys(
+            ssh_dir, private_key, public_key, valid_ssh_keys, ssh_key_path)
+
         # Set up bzr and Launchpad authentication.
         call('bzr', 'whoami', formataddr([full_name, email]))
         if valid_ssh_keys: