Merge lp:~louis/charms/precise/nova-cloud-controller/lp1313602-multiline-known-hosts into lp:~openstack-charmers-archive/charms/precise/nova-cloud-controller/trunk

[Louis Bouchard]

Send known_hosts and authorized_keys line by line

Only empty lines are not carried over

[James Page (james-page)]

Hi Louis

The general approach to fixing this looks fine, however the unit tests all need updating:

make test
Starting tests...
test_compute_changed_ssh_migration (unit_tests.test_nova_cc_hooks.NovaCCHooksTests) ... ERROR
test_compute_joined_neutron (unit_tests.test_nova_cc_hooks.NovaCCHooksTests) ... ERROR
test_config_changed_no_upgrade (unit_tests.test_nova_cc_hooks.NovaCCHooksTests) ... ERROR
test_config_changed_with_upgrade (unit_tests.test_nova_cc_hooks.NovaCCHooksTests) ... ERROR
test_db_changed (unit_tests.test_nova_cc_hooks.NovaCCHooksTests) ... ERROR
test_db_changed_missing_relation_data (unit_tests.test_nova_cc_hooks.NovaCCHooksTests) ... ERROR
test_db_joined (unit_tests.test_nova_cc_hooks.NovaCCHooksTests) ... ERROR
test_db_joined_with_postgresql (unit_tests.test_nova_cc_hooks.NovaCCHooksTests) ... ERROR
test_install_hook (unit_tests.test_nova_cc_hooks.NovaCCHooksTests) ... ERROR
test_nova_vmware_joined (unit_tests.test_nova_cc_hooks.NovaCCHooksTests) ... ERROR
test_postgresql_db_changed (unit_tests.test_nova_cc_hooks.NovaCCHooksTests) ... ERROR
test_postgresql_neutron_db_joined (unit_tests.test_nova_cc_hooks.NovaCCHooksTests) ... ERROR
test_postgresql_neutron_joined_with_db (unit_tests.test_nova_cc_hooks.NovaCCHooksTests) ... ERROR
test_postgresql_nova_db_changed_missing_relation_data (unit_tests.test_nova_cc_hooks.NovaCCHooksTests) ... ERROR
test_postgresql_nova_db_joined (unit_tests.test_nova_cc_hooks.NovaCCHooksTests) ... ERROR
test_postgresql_nova_joined_with_db (unit_tests.test_nova_cc_hooks.NovaCCHooksTests) ... ERROR
test_add_known_host_exists (unit_tests.test_nova_cc_utils.NovaCCUtilsTests) ... ok
test_add_known_host_exists_added (unit_tests.test_nova_cc_utils.NovaCCUtilsTests) ... ok
test_add_known_host_exists_outdated (unit_tests.test_nova_cc_utils.NovaCCUtilsTests) ... ok
test_authorized_keys (unit_tests.test_nova_cc_utils.NovaCCUtilsTests) ... ok
test_determine_endpoints_base (unit_tests.test_nova_cc_utils.NovaCCUtilsTests) ... ok
test_determine_endpoints_nova_volume (unit_tests.test_nova_cc_utils.NovaCCUtilsTests) ... ok
test_determine_endpoints_quantum_neutron (unit_tests.test_nova_cc_utils.NovaCCUtilsTests) ... ok
test_determine_packages_base (unit_tests.test_nova_cc_utils.NovaCCUtilsTests) ... ok
test_determine_packages_base_grizzly_beyond (unit_tests.test_nova_cc_utils.NovaCCUtilsTests) ... ok
test_determine_packages_neutron (unit_tests.test_nova_cc_utils.NovaCCUtilsTests) ... ok
test_determine_packages_nova_volume (unit_tests.test_nova_cc_utils.NovaCCUtilsTests) ... ok
test_determine_packages_quantum (unit_tests.test_nova_cc_utils.NovaCCUtilsTests) ... ok
test_determine_ports (unit_tests.test_nova_cc_utils.NovaCCUtilsTests) ... ok
test_determine_volume_service_essex (unit_tests.test_nova_cc_utils.NovaCCUtilsTests) ... ok
test_determine_volume_service_folsom_cinder (unit_tests.test_nova_cc_utils.NovaCCUtilsTests) ... ok
test_determine_volume_service_folsom_nova_vol (unit_tests.test_nova_cc_utils.NovaCCUtilsTests) ... ok
test_determine_volume_service_grizzly_and_beyond (unit_tests.test_nova_cc_utils.NovaCCUtilsTests) ... ok
test_known_hosts (unit_tests.test_nova_cc_utils.NovaCCUtilsTests) ... ok
Migrate database w...

review: Needs Fixing

[lp:~louis/charms/precise/nova-cloud-controller/lp1313602-multiline-known-hosts]

77. By Louis Bouchard on 2014-05-22

Adapt unit tests to changes made by previous commit

Tests now cover multi-line known_hosts & authorized_keys

78. By Louis Bouchard on 2014-05-22

Add new test_compute_changed_nova_public_key unit test

Test the compute_changed hook when 'enable-resize' is
used by nova-compute

79. By Louis Bouchard on 2014-05-23

Modify hook funcions & util to work in non-relation hooks

Some hook functions did not work when invoked from non-relation
hooks (compute_changed in particular). Modified all utils
hook functions to honour relation-id & unit-id when required

80. By Louis Bouchard on 2014-05-27

Modified relation_get mechanism in compute_changed

Get all relations in one call in use dictionary

81. By Louis Bouchard on 2014-05-27

Add missing rel_settings test

82. By Louis Bouchard on 2014-05-27

Fix unit_test to honour calls with unit & relation ids

83. By Louis Bouchard on 2014-05-27

More PEP8 cleanup

[Louis Bouchard (louis)]

Added call in upgrade-charm hook to action the new code that use multi-line mechanism. Tested that it can be used to recover from a previous failure caused by authorised_keys/known_hosts that were too long.

Fixed unit test for previous modifications and adapted them to changes made to support non-relation hook calls (required for upgrade-charm).

Added one unit-test to cover part of the modified code not previously tested.

More test would be needed to cover the non-relation context but out of scope for this bug

review: Needs Resubmitting

[lp:~louis/charms/precise/nova-cloud-controller/lp1313602-multiline-known-hosts]

84. By Louis Bouchard on 2014-06-24

Implement changes following Merge Proposal

 - Replaced uid by unit
 - Rolled back change to compute_departed
 - Use rel_settings.get()

85. By Louis Bouchard on 2014-07-16

Fix unit test following uid->unit change in hook

Unmerged revisions

=== modified file 'hooks/nova_cc_hooks.py'
--- hooks/nova_cc_hooks.py	2014-04-11 16:41:42 +0000
+++ hooks/nova_cc_hooks.py	2014-05-27 14:01:55 +0000
@@ -19,6 +19,7 @@
     relation_get,
     relation_ids,
     relation_set,
+    related_units,
     open_port,
     unit_get,
 )
@@ -54,8 +55,8 @@
     save_script_rc,
     ssh_compute_add,
     ssh_compute_remove,
-    ssh_known_hosts_b64,
-    ssh_authorized_keys_b64,
+    ssh_known_hosts_lines,
+    ssh_authorized_keys_lines,
     register_configs,
     restart_map,
     volume_service,
@@ -339,26 +340,52 @@
 
 
 @hooks.hook('cloud-compute-relation-changed')
-def compute_changed():
-    migration_auth = relation_get('migration_auth_type')
-    if migration_auth == 'ssh':
-        key = relation_get('ssh_public_key')
+def compute_changed(rid=None, uid=None):
+    rel_settings = relation_get(rid=rid, unit=uid)
+    if 'migration_auth_type' not in rel_settings:
+        return
+    if rel_settings['migration_auth_type'] == 'ssh':
+        key = rel_settings['ssh_public_key']
         if not key:
             log('SSH migration set but peer did not publish key.')
             return
-        ssh_compute_add(key)
-        relation_set(known_hosts=ssh_known_hosts_b64(),
-                     authorized_keys=ssh_authorized_keys_b64())
-    if relation_get('nova_ssh_public_key'):
-        key = relation_get('nova_ssh_public_key')
-        ssh_compute_add(key, user='nova')
-        relation_set(nova_known_hosts=ssh_known_hosts_b64(user='nova'),
-                     nova_authorized_keys=ssh_authorized_keys_b64(user='nova'))
+        ssh_compute_add(key, rid=rid, uid=uid)
+        index = 0
+        for line in ssh_known_hosts_lines(uid=uid):
+            relation_set(relation_id=rid, relation_settings=
+                         {'known_hosts_{}'.format(index): line})
+            index += 1
+        relation_set(relation_id=rid, known_hosts_max_index=index)
+        index = 0
+        for line in ssh_authorized_keys_lines(uid=uid):
+            relation_set(relation_id=rid, relation_settings=
+                         {'authorized_keys_{}'.format(index): line})
+            index += 1
+        relation_set(relation_id=rid, authorized_keys_max_index=index)
+    if 'nova_ssh_public_key' not in rel_settings:
+        return
+    if rel_settings['nova_ssh_public_key']:
+        ssh_compute_add(rel_settings['nova_ssh_public_key'],
+                        rid=rid, uid=uid, user='nova')
+        index = 0
+        for line in ssh_known_hosts_lines(uid=uid, user='nova'):
+            relation_set(relation_id=rid, relation_settings=
+                         {'{}_known_hosts_{}'.format('nova', index): line})
+            index += 1
+        relation_set(relation_id=rid, relation_settings=
+                     {'{}_known_hosts_max_index'.format('nova'): index})
+        index = 0
+        for line in ssh_authorized_keys_lines(uid=uid, user='nova'):
+            relation_set(relation_id=rid, relation_settings=
+                         {'{}_authorized_keys_{}'.format('nova', index): line})
+            index += 1
+        relation_set(relation_id=rid, relation_settings=
+                     {'{}_authorized_keys_max_index'.format('nova'): index})
 
 
 @hooks.hook('cloud-compute-relation-departed')
-def compute_departed():
-    ssh_compute_remove(public_key=relation_get('ssh_public_key'))
+def compute_departed(uid=None):
+    ssh_compute_remove(uid=uid, public_key=relation_get('ssh_public_key'))
 
 
 @hooks.hook('neutron-network-service-relation-joined',
@@ -498,6 +525,9 @@
         amqp_joined(relation_id=r_id)
     for r_id in relation_ids('identity-service'):
         identity_joined(rid=r_id)
+    for r_id in relation_ids('cloud-compute'):
+        for unit in related_units(r_id):
+            compute_changed(r_id, unit)
 
 
 def main():

=== modified file 'hooks/nova_cc_utils.py'
--- hooks/nova_cc_utils.py	2014-05-21 10:03:01 +0000
+++ hooks/nova_cc_utils.py	2014-05-27 14:01:55 +0000
@@ -509,8 +509,11 @@
         return b64encode(_in.read())
 
 
-def ssh_directory_for_unit(user=None):
-    remote_service = remote_unit().split('/')[0]
+def ssh_directory_for_unit(remunit=None, user=None):
+    if remunit:
+        remote_service = remunit.split('/')[0]
+    else:
+        remote_service = remote_unit().split('/')[0]
     if user:
         remote_service = "{}_{}".format(remote_service, user)
     _dir = os.path.join(NOVA_SSH_DIR, remote_service)
@@ -524,29 +527,29 @@
     return _dir
 
 
-def known_hosts(user=None):
-    return os.path.join(ssh_directory_for_unit(user), 'known_hosts')
-
-
-def authorized_keys(user=None):
-    return os.path.join(ssh_directory_for_unit(user), 'authorized_keys')
-
-
-def ssh_known_host_key(host, user=None):
-    cmd = ['ssh-keygen', '-f', known_hosts(user), '-H', '-F', host]
+def known_hosts(unit=None, user=None):
+    return os.path.join(ssh_directory_for_unit(unit, user), 'known_hosts')
+
+
+def authorized_keys(unit=None, user=None):
+    return os.path.join(ssh_directory_for_unit(unit, user), 'authorized_keys')
+
+
+def ssh_known_host_key(host, uid=None, user=None):
+    cmd = ['ssh-keygen', '-f', known_hosts(uid, user), '-H', '-F', host]
     try:
         return subprocess.check_output(cmd).strip()
     except subprocess.CalledProcessError:
         return None
 
 
-def remove_known_host(host, user=None):
+def remove_known_host(host, uid=None, user=None):
     log('Removing SSH known host entry for compute host at %s' % host)
-    cmd = ['ssh-keygen', '-f', known_hosts(user), '-R', host]
+    cmd = ['ssh-keygen', '-f', known_hosts(uid, user), '-R', host]
     subprocess.check_call(cmd)
 
 
-def add_known_host(host, user=None):
+def add_known_host(host, uid=None, user=None):
     '''Add variations of host to a known hosts file.'''
     cmd = ['ssh-keyscan', '-H', '-t', 'rsa', host]
     try:
@@ -555,33 +558,34 @@
         log('Could not obtain SSH host key from %s' % host, level=ERROR)
         raise e
 
-    current_key = ssh_known_host_key(host, user)
+    current_key = ssh_known_host_key(host, uid, user)
     if current_key:
         if remote_key == current_key:
             log('Known host key for compute host %s up to date.' % host)
             return
         else:
-            remove_known_host(host, user)
+            remove_known_host(host, uid, user)
 
     log('Adding SSH host key to known hosts for compute node at %s.' % host)
-    with open(known_hosts(user), 'a') as out:
+    with open(known_hosts(uid, user), 'a') as out:
         out.write(remote_key + '\n')
 
 
-def ssh_authorized_key_exists(public_key, user=None):
-    with open(authorized_keys(user)) as keys:
+def ssh_authorized_key_exists(public_key, uid=None, user=None):
+    with open(authorized_keys(uid, user)) as keys:
         return (' %s ' % public_key) in keys.read()
 
 
-def add_authorized_key(public_key, user=None):
-    with open(authorized_keys(user), 'a') as keys:
+def add_authorized_key(public_key, uid=None, user=None):
+    with open(authorized_keys(uid, user), 'a') as keys:
         keys.write(public_key + '\n')
 
 
-def ssh_compute_add(public_key, user=None):
+def ssh_compute_add(public_key, rid=None, uid=None, user=None):
     # If remote compute node hands us a hostname, ensure we have a
     # known hosts entry for its IP, hostname and FQDN.
-    private_address = relation_get('private-address')
+    private_address = relation_get(rid=rid, unit=uid,
+                                   attribute='private-address')
     hosts = [private_address]
 
     if not is_ip(private_address):
@@ -593,31 +597,41 @@
         hosts.append(hn.split('.')[0])
 
     for host in list(set(hosts)):
-        if not ssh_known_host_key(host, user):
-            add_known_host(host, user)
+        if not ssh_known_host_key(host, uid, user):
+            add_known_host(host, uid, user)
 
-    if not ssh_authorized_key_exists(public_key, user):
+    if not ssh_authorized_key_exists(public_key, uid, user):
         log('Saving SSH authorized key for compute host at %s.' %
             private_address)
-        add_authorized_key(public_key, user)
-
-
-def ssh_known_hosts_b64(user=None):
-    with open(known_hosts(user)) as hosts:
-        return b64encode(hosts.read())
-
-
-def ssh_authorized_keys_b64(user=None):
-    with open(authorized_keys(user)) as keys:
-        return b64encode(keys.read())
-
-
-def ssh_compute_remove(public_key, user=None):
-    if not (os.path.isfile(authorized_keys(user)) or
-            os.path.isfile(known_hosts(user))):
+        add_authorized_key(public_key, uid, user)
+
+
+def ssh_known_hosts_lines(uid=None, user=None):
+    known_hosts_list = []
+
+    with open(known_hosts(uid, user)) as hosts:
+        for hosts_line in hosts:
+            if hosts_line.rstrip():
+                known_hosts_list.append(hosts_line.rstrip())
+    return(known_hosts_list)
+
+
+def ssh_authorized_keys_lines(uid=None, user=None):
+    authorized_keys_list = []
+
+    with open(authorized_keys(uid, user)) as keys:
+        for authkey_line in keys:
+            if authkey_line.rstrip():
+                authorized_keys_list.append(authkey_line.rstrip())
+    return(authorized_keys_list)
+
+
+def ssh_compute_remove(public_key, uid=None, user=None):
+    if not (os.path.isfile(authorized_keys(uid, user)) or
+            os.path.isfile(known_hosts(uid, user))):
         return
 
-    with open(authorized_keys(user)) as _keys:
+    with open(authorized_keys(uid, user)) as _keys:
         keys = [k.strip() for k in _keys.readlines()]
 
     if public_key not in keys:
@@ -625,7 +639,7 @@
 
     [keys.remove(key) for key in keys if key == public_key]
 
-    with open(authorized_keys(user), 'w') as _keys:
+    with open(authorized_keys(uid, user), 'w') as _keys:
         keys = '\n'.join(keys)
         if not keys.endswith('\n'):
             keys += '\n'

=== modified file 'unit_tests/test_nova_cc_hooks.py'
--- unit_tests/test_nova_cc_hooks.py	2014-05-21 10:03:01 +0000
+++ unit_tests/test_nova_cc_hooks.py	2014-05-27 14:01:55 +0000
@@ -35,8 +35,8 @@
     'relation_set',
     'relation_ids',
     'ssh_compute_add',
-    'ssh_known_hosts_b64',
-    'ssh_authorized_keys_b64',
+    'ssh_known_hosts_lines',
+    'ssh_authorized_keys_lines',
     'save_script_rc',
     'execd_preinstall',
     'network_manager',
@@ -98,12 +98,60 @@
         self.test_relation.set({
             'migration_auth_type': 'ssh', 'ssh_public_key': 'fookey',
             'private-address': '10.0.0.1'})
-        self.ssh_known_hosts_b64.return_value = 'hosts'
-        self.ssh_authorized_keys_b64.return_value = 'keys'
-        hooks.compute_changed()
-        self.ssh_compute_add.assert_called_with('fookey')
-        self.relation_set.assert_called_with(known_hosts='hosts',
-                                             authorized_keys='keys')
+        self.ssh_known_hosts_lines.return_value = [
+            'k_h_0', 'k_h_1', 'k_h_2']
+        self.ssh_authorized_keys_lines.return_value = [
+            'auth_0', 'auth_1', 'auth_2']
+        hooks.compute_changed()
+        self.ssh_compute_add.assert_called_with('fookey', rid=None, uid=None)
+        expected_relations = [
+            call(relation_settings={'authorized_keys_0': 'auth_0'},
+                 relation_id=None),
+            call(relation_settings={'authorized_keys_1': 'auth_1'},
+                 relation_id=None),
+            call(relation_settings={'authorized_keys_2': 'auth_2'},
+                 relation_id=None),
+            call(relation_settings={'known_hosts_0': 'k_h_0'},
+                 relation_id=None),
+            call(relation_settings={'known_hosts_1': 'k_h_1'},
+                 relation_id=None),
+            call(relation_settings={'known_hosts_2': 'k_h_2'},
+                 relation_id=None),
+            call(authorized_keys_max_index=3, relation_id=None),
+            call(known_hosts_max_index=3, relation_id=None)]
+        self.assertEquals(sorted(self.relation_set.call_args_list),
+                          sorted(expected_relations))
+
+    def test_compute_changed_nova_public_key(self):
+        self.test_relation.set({
+            'migration_auth_type': 'sasl', 'nova_ssh_public_key': 'fookey',
+            'private-address': '10.0.0.1'})
+        self.ssh_known_hosts_lines.return_value = [
+            'k_h_0', 'k_h_1', 'k_h_2']
+        self.ssh_authorized_keys_lines.return_value = [
+            'auth_0', 'auth_1', 'auth_2']
+        hooks.compute_changed()
+        self.ssh_compute_add.assert_called_with('fookey', user='nova',
+                                                rid=None, uid=None)
+        expected_relations = [
+            call(relation_settings={'nova_authorized_keys_0': 'auth_0'},
+                 relation_id=None),
+            call(relation_settings={'nova_authorized_keys_1': 'auth_1'},
+                 relation_id=None),
+            call(relation_settings={'nova_authorized_keys_2': 'auth_2'},
+                 relation_id=None),
+            call(relation_settings={'nova_known_hosts_0': 'k_h_0'},
+                 relation_id=None),
+            call(relation_settings={'nova_known_hosts_1': 'k_h_1'},
+                 relation_id=None),
+            call(relation_settings={'nova_known_hosts_2': 'k_h_2'},
+                 relation_id=None),
+            call(relation_settings={'nova_known_hosts_max_index': 3},
+                 relation_id=None),
+            call(relation_settings={'nova_authorized_keys_max_index': 3},
+                 relation_id=None)]
+        self.assertEquals(sorted(self.relation_set.call_args_list),
+                          sorted(expected_relations))
 
     @patch.object(hooks, '_auth_config')
     def test_compute_joined_neutron(self, auth_config):

=== modified file 'unit_tests/test_nova_cc_utils.py'
--- unit_tests/test_nova_cc_utils.py	2014-05-02 10:06:23 +0000
+++ unit_tests/test_nova_cc_utils.py	2014-05-27 14:01:55 +0000
@@ -321,8 +321,8 @@
         check_output.return_value = 'fookey'
         host_key.return_value = 'fookey_old'
         with patch_open() as (_open, _file):
-            utils.add_known_host('foohost')
-            rm.assert_called_with('foohost', None)
+            utils.add_known_host('foohost', None, None)
+            rm.assert_called_with('foohost', None, None)
 
     @patch.object(utils, 'known_hosts')
     @patch.object(utils, 'remove_known_host')
@@ -355,19 +355,19 @@
     def test_known_hosts(self, ssh_dir):
         ssh_dir.return_value = '/tmp/foo'
         self.assertEquals(utils.known_hosts(), '/tmp/foo/known_hosts')
-        ssh_dir.assert_called_with(None)
+        ssh_dir.assert_called_with(None, None)
         self.assertEquals(utils.known_hosts('bar'), '/tmp/foo/known_hosts')
-        ssh_dir.assert_called_with('bar')
+        ssh_dir.assert_called_with('bar', None)
 
     @patch.object(utils, 'ssh_directory_for_unit')
     def test_authorized_keys(self, ssh_dir):
         ssh_dir.return_value = '/tmp/foo'
         self.assertEquals(utils.authorized_keys(), '/tmp/foo/authorized_keys')
-        ssh_dir.assert_called_with(None)
+        ssh_dir.assert_called_with(None, None)
         self.assertEquals(
             utils.authorized_keys('bar'),
             '/tmp/foo/authorized_keys')
-        ssh_dir.assert_called_with('bar')
+        ssh_dir.assert_called_with('bar', None)
 
     @patch.object(utils, 'known_hosts')
     @patch('subprocess.check_call')
@@ -461,9 +461,9 @@
         _check_output.assert_called_with(
             ['ssh-keygen', '-f', '/foo/known_hosts',
              '-H', '-F', 'test'])
-        _known_hosts.assert_called_with(None)
+        _known_hosts.assert_called_with(None, None)
         utils.ssh_known_host_key('test', 'bar')
-        _known_hosts.assert_called_with('bar')
+        _known_hosts.assert_called_with('bar', None)
 
     @patch.object(utils, 'known_hosts')
     @patch('subprocess.check_call')
@@ -473,9 +473,9 @@
         _check_call.assert_called_with(
             ['ssh-keygen', '-f', '/foo/known_hosts',
              '-R', 'test'])
-        _known_hosts.assert_called_with(None)
+        _known_hosts.assert_called_with(None, None)
         utils.remove_known_host('test', 'bar')
-        _known_hosts.assert_called_with('bar')
+        _known_hosts.assert_called_with('bar', None)
 
     @patch('subprocess.check_output')
     def test_migrate_database(self, check_output):