Merge lp:~hopem/charms/trusty/keystone/stable-backport-lp1415579 into lp:~openstack-charmers-archive/charms/trusty/keystone/trunk

[Edward Hope-Morley]

[uosci-testing-bot (uosci-testing-bot)]

charm_lint_check #1781 keystone for hopem mp248894
    LINT OK: passed

Build: http://10.245.162.77:8080/job/charm_lint_check/1781/

[uosci-testing-bot (uosci-testing-bot)]

charm_unit_test #1609 keystone for hopem mp248894
    UNIT OK: passed

Build: http://10.245.162.77:8080/job/charm_unit_test/1609/

[uosci-testing-bot (uosci-testing-bot)]

charm_amulet_test #1801 keystone for hopem mp248894
    AMULET OK: passed

Build: http://10.245.162.77:8080/job/charm_amulet_test/1801/

[James Page (james-page)]

review: Approve

=== modified file 'hooks/charmhelpers/contrib/unison/__init__.py'
--- hooks/charmhelpers/contrib/unison/__init__.py	2015-01-26 09:44:47 +0000
+++ hooks/charmhelpers/contrib/unison/__init__.py	2015-02-06 12:31:31 +0000
@@ -73,6 +73,7 @@
     relation_set,
     relation_get,
     unit_private_ip,
+    INFO,
     ERROR,
 )
 
@@ -86,7 +87,7 @@
         user = pwd.getpwnam(user)
         return user.pw_dir
     except KeyError:
-        log('Could not get homedir for user %s: user exists?', ERROR)
+        log('Could not get homedir for user %s: user exists?' % (user), ERROR)
         raise Exception
 
 
@@ -233,14 +234,15 @@
                                         rid=r_id, unit=unit)
 
             if not authed_hosts:
-                log('Peer %s has not authorized *any* hosts yet, skipping.')
+                log('Peer %s has not authorized *any* hosts yet, skipping.' %
+                    (unit), level=INFO)
                 continue
 
             if unit_private_ip() in authed_hosts.split(':'):
                 hosts.append(private_addr)
             else:
-                log('Peer %s has not authorized *this* host yet, skipping.')
-
+                log('Peer %s has not authorized *this* host yet, skipping.' %
+                    (unit), level=INFO)
     return hosts
 
 
 
=== modified file 'hooks/charmhelpers/core/sysctl.py'
--- hooks/charmhelpers/core/sysctl.py	2015-01-26 09:44:47 +0000
+++ hooks/charmhelpers/core/sysctl.py	2015-02-06 12:31:31 +0000
@@ -26,25 +26,31 @@
 from charmhelpers.core.hookenv import (
     log,
     DEBUG,
+    ERROR,
 )
 
 
 def create(sysctl_dict, sysctl_file):
     """Creates a sysctl.conf file from a YAML associative array
 
-    :param sysctl_dict: a dict of sysctl options eg { 'kernel.max_pid': 1337 }
-    :type sysctl_dict: dict
+    :param sysctl_dict: a YAML-formatted string of sysctl options eg "{ 'kernel.max_pid': 1337 }"
+    :type sysctl_dict: str
     :param sysctl_file: path to the sysctl file to be saved
     :type sysctl_file: str or unicode
     :returns: None
     """
-    sysctl_dict = yaml.load(sysctl_dict)
+    try:
+        sysctl_dict_parsed = yaml.safe_load(sysctl_dict)
+    except yaml.YAMLError:
+        log("Error parsing YAML sysctl_dict: {}".format(sysctl_dict),
+            level=ERROR)
+        return
 
     with open(sysctl_file, "w") as fd:
-        for key, value in sysctl_dict.items():
+        for key, value in sysctl_dict_parsed.items():
             fd.write("{}={}\n".format(key, value))
 
-    log("Updating sysctl_file: %s values: %s" % (sysctl_file, sysctl_dict),
+    log("Updating sysctl_file: %s values: %s" % (sysctl_file, sysctl_dict_parsed),
         level=DEBUG)
 
     check_call(["sysctl", "-p", sysctl_file])
 
=== modified file 'hooks/keystone_hooks.py'
--- hooks/keystone_hooks.py	2015-01-27 22:21:37 +0000
+++ hooks/keystone_hooks.py	2015-02-06 12:31:31 +0000
@@ -67,6 +67,7 @@
     is_str_true,
     is_ssl_cert_master,
     is_db_ready,
+    clear_ssl_synced_units,
 )
 
 from charmhelpers.contrib.hahelpers.cluster import (
@@ -150,11 +151,8 @@
         admin_relation_changed(rid)
 
     # Ensure sync request is sent out (needed for upgrade to ssl from non-ssl)
-    settings = {}
-    append_ssl_sync_request(settings)
-    if settings:
-        for rid in relation_ids('cluster'):
-            relation_set(relation_id=rid, relation_settings=settings)
+    send_ssl_sync_request()
+
     for r_id in relation_ids('ha'):
         ha_joined(relation_id=r_id)
 
@@ -283,15 +281,39 @@
         send_notifications(notifications)
 
 
-def append_ssl_sync_request(settings):
-    """Add request to be synced to relation settings.
-
-    This will be consumed by cluster-relation-changed ssl master.
+def send_ssl_sync_request():
+    """Set sync request on cluster relation.
+
+    Value set equals number of ssl configs currently enabled so that if they
+    change, we ensure that certs are synced. This setting is consumed by
+    cluster-relation-changed ssl master. We also clear the 'synced' set to
+    guarantee that a sync will occur.
+
+    Note the we do nothing if the setting is already applied.
     """
-    if (is_str_true(config('use-https')) or
-            is_str_true(config('https-service-endpoints'))):
-        unit = local_unit().replace('/', '-')
-        settings['ssl-sync-required-%s' % (unit)] = '1'
+    unit = local_unit().replace('/', '-')
+    count = 0
+    if is_str_true(config('use-https')):
+        count += 1
+
+    if is_str_true(config('https-service-endpoints')):
+        count += 2
+
+    if count:
+        key = 'ssl-sync-required-%s' % (unit)
+        settings = {key: count}
+        prev = 0
+        rid = None
+        for rid in relation_ids('cluster'):
+            for unit in related_units(rid):
+                _prev = relation_get(rid=rid, unit=unit, attribute=key) or 0
+                if _prev and _prev > prev:
+                    prev = _prev
+
+        if rid and prev < count:
+            clear_ssl_synced_units()
+            log("Setting %s=%s" % (key, count), level=DEBUG)
+            relation_set(relation_id=rid, relation_settings=settings)
 
 
 @hooks.hook('cluster-relation-joined')
@@ -314,9 +336,8 @@
         private_addr = get_ipv6_addr(exc_list=[config('vip')])[0]
         settings['private-address'] = private_addr
 
-    append_ssl_sync_request(settings)
-
     relation_set(relation_settings=settings)
+    send_ssl_sync_request()
 
 
 def apply_echo_filters(settings, echo_whitelist):
 
=== modified file 'hooks/keystone_utils.py'
--- hooks/keystone_utils.py	2015-02-02 13:52:05 +0000
+++ hooks/keystone_utils.py	2015-02-06 12:31:31 +0000
@@ -21,7 +21,6 @@
     determine_api_port,
     https,
     peer_units,
-    oldest_peer,
 )
 
 from charmhelpers.contrib.openstack import context, templating
@@ -764,14 +763,27 @@
 def unison_sync(paths_to_sync):
     """Do unison sync and retry a few times if it fails since peers may not be
     ready for sync.
+
+    Returns list of synced units or None if one or more peers was not synced.
     """
     log('Synchronizing CA (%s) to all peers.' % (', '.join(paths_to_sync)),
         level=INFO)
     keystone_gid = grp.getgrnam('keystone').gr_gid
+
+    # NOTE(dosaboy): This will sync to all peers who have already provided
+    # their ssh keys. If any existing peers have not provided their keys yet,
+    # they will be silently ignored.
     unison.sync_to_peers(peer_interface='cluster', paths=paths_to_sync,
                          user=SSH_USER, verbose=True, gid=keystone_gid,
                          fatal=True)
 
+    synced_units = peer_units()
+    if len(unison.collect_authed_hosts('cluster')) != len(synced_units):
+        log("Not all peer units synced due to missing public keys", level=INFO)
+        return None
+    else:
+        return synced_units
+
 
 def get_ssl_sync_request_units():
     """Get list of units that have requested to be synced.
@@ -791,14 +803,22 @@
     return units
 
 
-def is_ssl_cert_master():
+def is_ssl_cert_master(votes=None):
     """Return True if this unit is ssl cert master."""
     master = None
     for rid in relation_ids('cluster'):
         master = relation_get(attribute='ssl-cert-master', rid=rid,
                               unit=local_unit())
 
-    return master == local_unit()
+    if master == local_unit():
+        votes = votes or get_ssl_cert_master_votes()
+        if not peer_units() or (len(votes) == 1 and master in votes):
+            return True
+
+        log("Did not get consensus from peers on who is ssl-cert-master "
+            "(%s)" % (votes), level=INFO)
+
+    return False
 
 
 def is_ssl_enabled():
@@ -812,7 +832,21 @@
     return True
 
 
-def ensure_ssl_cert_master(use_oldest_peer=False):
+def get_ssl_cert_master_votes():
+    """Returns a list of unique votes."""
+    votes = []
+    # Gather election results from peers. These will need to be consistent.
+    for rid in relation_ids('cluster'):
+        for unit in related_units(rid):
+            m = relation_get(rid=rid, unit=unit,
+                             attribute='ssl-cert-master')
+            if m is not None:
+                votes.append(m)
+
+    return list(set(votes))
+
+
+def ensure_ssl_cert_master():
     """Ensure that an ssl cert master has been elected.
 
     Normally the cluster leader will take control but we allow for this to be
@@ -822,31 +856,19 @@
     if not is_ssl_enabled():
         return False
 
-    elect = False
-    peers = peer_units()
     master_override = False
-    if use_oldest_peer:
-        elect = oldest_peer(peers)
-    else:
-        elect = is_elected_leader(CLUSTER_RES)
+    elect = is_elected_leader(CLUSTER_RES)
 
     # If no peers we allow this unit to elect itsef as master and do
     # sync immediately.
-    if not peers and not is_ssl_cert_master():
+    if not peer_units():
         elect = True
         master_override = True
 
     if elect:
-        masters = []
-        for rid in relation_ids('cluster'):
-            for unit in related_units(rid):
-                m = relation_get(rid=rid, unit=unit,
-                                 attribute='ssl-cert-master')
-                if m is not None:
-                    masters.append(m)
-
+        votes = get_ssl_cert_master_votes()
         # We expect all peers to echo this setting
-        if not masters or 'unknown' in masters:
+        if not votes or 'unknown' in votes:
             log("Notifying peers this unit is ssl-cert-master", level=INFO)
             for rid in relation_ids('cluster'):
                 settings = {'ssl-cert-master': local_unit()}
@@ -855,10 +877,11 @@
             # Return now and wait for cluster-relation-changed (peer_echo) for
             # sync.
             return master_override
-        elif len(set(masters)) != 1 and local_unit() not in masters:
-            log("Did not get consensus from peers on who is ssl-cert-master "
-                "(%s) - waiting for current master to release before "
-                "self-electing" % (masters), level=INFO)
+        elif not is_ssl_cert_master(votes):
+            if not master_override:
+                log("Conscensus not reached - current master will need to "
+                    "release", level=INFO)
+
             return master_override
 
     if not is_ssl_cert_master():
@@ -887,15 +910,18 @@
         log("Syncing all endpoint certs since https-service-endpoints=True",
             level=DEBUG)
         paths_to_sync.append(SSL_DIR)
-        paths_to_sync.append(APACHE_SSL_DIR)
         paths_to_sync.append(CA_CERT_PATH)
-    elif is_str_true(config('use-https')):
+
+    if is_str_true(config('use-https')):
         log("Syncing keystone-endpoint certs since use-https=True",
             level=DEBUG)
         paths_to_sync.append(SSL_DIR)
         paths_to_sync.append(APACHE_SSL_DIR)
         paths_to_sync.append(CA_CERT_PATH)
 
+    # Ensure unique
+    paths_to_sync = list(set(paths_to_sync))
+
     if not paths_to_sync:
         log("Nothing to sync - skipping", level=DEBUG)
         return {}
@@ -908,8 +934,7 @@
     create_peer_service_actions('restart', ['apache2'])
     create_peer_actions(['update-ca-certificates'])
 
-    # Format here needs to match that used when peers request sync
-    synced_units = [unit.replace('/', '-') for unit in peer_units()]
+    cluster_rel_settings = {}
 
     retries = 3
     while True:
@@ -918,7 +943,12 @@
             update_hash_from_path(hash1, path)
 
         try:
-            unison_sync(paths_to_sync)
+            synced_units = unison_sync(paths_to_sync)
+            if synced_units:
+                # Format here needs to match that used when peers request sync
+                synced_units = [u.replace('/', '-') for u in synced_units]
+                cluster_rel_settings['ssl-synced-units'] = \
+                    json.dumps(synced_units)
         except:
             if fatal:
                 raise
@@ -947,10 +977,22 @@
     hash = hash1.hexdigest()
     log("Sending restart-services-trigger=%s to all peers" % (hash),
         level=DEBUG)
+    cluster_rel_settings['restart-services-trigger'] = hash
 
     log("Sync complete", level=DEBUG)
-    return {'restart-services-trigger': hash,
-            'ssl-synced-units': json.dumps(synced_units)}
+    return cluster_rel_settings
+
+
+def clear_ssl_synced_units():
+    """Clear the 'synced' units record on the cluster relation.
+
+    If new unit sync reauests are set this will ensure that a sync occurs when
+    the sync master receives the requests.
+    """
+    log("Clearing ssl sync units", level=DEBUG)
+    for rid in relation_ids('cluster'):
+        relation_set(relation_id=rid,
+                     relation_settings={'ssl-synced-units': None})
 
 
 def update_hash_from_path(hash, path, recurse_depth=10):
@@ -1058,11 +1100,11 @@
                                  '%s' % SSL_DIR])
         subprocess.check_output(['chmod', '-R', 'g+rwx', '%s' % SSL_DIR])
 
-        # Ensure a master has been elected and prefer this unit. Note that we
-        # prefer oldest peer as predicate since this action i normally only
-        # performed once at deploy time when the oldest peer should be the
-        # first to be ready.
-        ensure_ssl_cert_master(use_oldest_peer=True)
+        # Ensure a master is elected. This should cover the following cases:
+        # * single unit == 'oldest' unit is elected as master
+        # * multi unit + not clustered == 'oldest' unit is elcted as master
+        # * multi unit + clustered == cluster leader is elected as master
+        ensure_ssl_cert_master()
 
         ssl.CA_SINGLETON.append(ca)
 
 
=== modified file 'unit_tests/test_keystone_utils.py'
--- unit_tests/test_keystone_utils.py	2015-02-02 13:52:05 +0000
+++ unit_tests/test_keystone_utils.py	2015-02-06 12:31:31 +0000
@@ -28,6 +28,7 @@
     'grant_role',
     'configure_installation_source',
     'is_elected_leader',
+    'is_ssl_cert_master',
     'https',
     'peer_store_and_set',
     'service_stop',
@@ -380,45 +381,141 @@
         self.assertTrue(utils.is_db_ready())
 
     @patch.object(utils, 'peer_units')
-    @patch.object(utils, 'is_elected_leader')
-    @patch.object(utils, 'oldest_peer')
-    @patch.object(utils, 'is_ssl_enabled')
-    def test_ensure_ssl_cert_master(self, mock_is_str_true, mock_oldest_peer,
-                                    mock_is_elected_leader, mock_peer_units):
-        self.relation_ids.return_value = ['cluster:0']
-        self.local_unit.return_value = 'unit/0'
-
-        mock_is_str_true.return_value = False
-        self.assertFalse(utils.ensure_ssl_cert_master())
-        self.assertFalse(self.relation_set.called)
-
-        mock_is_elected_leader.return_value = False
-        self.assertFalse(utils.ensure_ssl_cert_master())
-        self.assertFalse(self.relation_set.called)
-
-        mock_is_str_true.return_value = True
-        mock_is_elected_leader.return_value = False
-        mock_peer_units.return_value = ['unit/0']
-        self.assertFalse(utils.ensure_ssl_cert_master())
-        self.assertFalse(self.relation_set.called)
-
-        mock_peer_units.return_value = []
-        self.assertTrue(utils.ensure_ssl_cert_master())
-        settings = {'ssl-cert-master': 'unit/0'}
-        self.relation_set.assert_called_with(relation_id='cluster:0',
-                                             relation_settings=settings)
-        self.relation_set.reset_mock()
-
-        self.assertTrue(utils.ensure_ssl_cert_master(use_oldest_peer=True))
-        settings = {'ssl-cert-master': 'unit/0'}
-        self.relation_set.assert_called_with(relation_id='cluster:0',
-                                             relation_settings=settings)
-        self.relation_set.reset_mock()
-
-        mock_peer_units.return_value = ['unit/0']
-        self.assertFalse(utils.ensure_ssl_cert_master())
-        self.assertFalse(utils.ensure_ssl_cert_master(use_oldest_peer=True))
-        settings = {'ssl-cert-master': 'unit/0'}
-        self.relation_set.assert_called_with(relation_id='cluster:0',
-                                             relation_settings=settings)
-        self.relation_set.reset_mock()
+    @patch.object(utils, 'is_ssl_enabled')
+    def test_ensure_ssl_cert_master_no_ssl(self, mock_is_ssl_enabled,
+                                           mock_peer_units):
+        mock_is_ssl_enabled.return_value = False
+        self.assertFalse(utils.ensure_ssl_cert_master())
+        self.assertFalse(self.relation_set.called)
+
+    @patch.object(utils, 'peer_units')
+    @patch.object(utils, 'is_ssl_enabled')
+    def test_ensure_ssl_cert_master_ssl_no_peers(self, mock_is_ssl_enabled,
+                                                 mock_peer_units):
+        def mock_rel_get(unit=None, **kwargs):
+            return None
+
+        self.relation_get.side_effect = mock_rel_get
+        mock_is_ssl_enabled.return_value = True
+        self.relation_ids.return_value = ['cluster:0']
+        self.local_unit.return_value = 'unit/0'
+        self.related_units.return_value = []
+        mock_peer_units.return_value = []
+        # This should get ignored since we are overriding
+        self.is_ssl_cert_master.return_value = False
+        self.is_elected_leader.return_value = False
+        self.assertTrue(utils.ensure_ssl_cert_master())
+        settings = {'ssl-cert-master': 'unit/0'}
+        self.relation_set.assert_called_with(relation_id='cluster:0',
+                                             relation_settings=settings)
+
+    @patch.object(utils, 'peer_units')
+    @patch.object(utils, 'is_ssl_enabled')
+    def test_ensure_ssl_cert_master_ssl_master_no_peers(self,
+                                                        mock_is_ssl_enabled,
+                                                        mock_peer_units):
+        def mock_rel_get(unit=None, **kwargs):
+            if unit == 'unit/0':
+                return 'unit/0'
+
+            return None
+
+        self.relation_get.side_effect = mock_rel_get
+        mock_is_ssl_enabled.return_value = True
+        self.relation_ids.return_value = ['cluster:0']
+        self.local_unit.return_value = 'unit/0'
+        self.related_units.return_value = []
+        mock_peer_units.return_value = []
+        # This should get ignored since we are overriding
+        self.is_ssl_cert_master.return_value = False
+        self.is_elected_leader.return_value = False
+        self.assertTrue(utils.ensure_ssl_cert_master())
+        settings = {'ssl-cert-master': 'unit/0'}
+        self.relation_set.assert_called_with(relation_id='cluster:0',
+                                             relation_settings=settings)
+
+    @patch.object(utils, 'peer_units')
+    @patch.object(utils, 'is_ssl_enabled')
+    def test_ensure_ssl_cert_master_ssl_not_leader(self, mock_is_ssl_enabled,
+                                                   mock_peer_units):
+        mock_is_ssl_enabled.return_value = True
+        self.relation_ids.return_value = ['cluster:0']
+        self.local_unit.return_value = 'unit/0'
+        mock_peer_units.return_value = ['unit/1']
+        self.is_ssl_cert_master.return_value = False
+        self.is_elected_leader.return_value = False
+        self.assertFalse(utils.ensure_ssl_cert_master())
+        self.assertFalse(self.relation_set.called)
+
+    @patch.object(utils, 'peer_units')
+    @patch.object(utils, 'is_ssl_enabled')
+    def test_ensure_ssl_cert_master_is_leader_new_peer(self,
+                                                       mock_is_ssl_enabled,
+                                                       mock_peer_units):
+        def mock_rel_get(unit=None, **kwargs):
+            if unit == 'unit/0':
+                return 'unit/0'
+
+            return 'unknown'
+
+        self.relation_get.side_effect = mock_rel_get
+        mock_is_ssl_enabled.return_value = True
+        self.relation_ids.return_value = ['cluster:0']
+        self.local_unit.return_value = 'unit/0'
+        mock_peer_units.return_value = ['unit/1']
+        self.related_units.return_value = ['unit/1']
+        self.is_ssl_cert_master.return_value = False
+        self.is_elected_leader.return_value = True
+        self.assertFalse(utils.ensure_ssl_cert_master())
+        settings = {'ssl-cert-master': 'unit/0'}
+        self.relation_set.assert_called_with(relation_id='cluster:0',
+                                             relation_settings=settings)
+
+    @patch.object(utils, 'peer_units')
+    @patch.object(utils, 'is_ssl_enabled')
+    def test_ensure_ssl_cert_master_is_leader_no_new_peer(self,
+                                                          mock_is_ssl_enabled,
+                                                          mock_peer_units):
+        def mock_rel_get(unit=None, **kwargs):
+            if unit == 'unit/0':
+                return 'unit/0'
+
+            return 'unit/0'
+
+        self.relation_get.side_effect = mock_rel_get
+        mock_is_ssl_enabled.return_value = True
+        self.relation_ids.return_value = ['cluster:0']
+        self.local_unit.return_value = 'unit/0'
+        mock_peer_units.return_value = ['unit/1']
+        self.related_units.return_value = ['unit/1']
+        self.is_ssl_cert_master.return_value = False
+        self.is_elected_leader.return_value = True
+        self.assertFalse(utils.ensure_ssl_cert_master())
+        self.assertFalse(self.relation_set.called)
+
+    @patch.object(utils, 'peer_units')
+    @patch.object(utils, 'is_ssl_enabled')
+    def test_ensure_ssl_cert_master_is_leader_bad_votes(self,
+                                                        mock_is_ssl_enabled,
+                                                        mock_peer_units):
+        counter = {0: 0}
+
+        def mock_rel_get(unit=None, **kwargs):
+            """Returns a mix of votes."""
+            if unit == 'unit/0':
+                return 'unit/0'
+
+            ret = 'unit/%d' % (counter[0])
+            counter[0] += 1
+            return ret
+
+        self.relation_get.side_effect = mock_rel_get
+        mock_is_ssl_enabled.return_value = True
+        self.relation_ids.return_value = ['cluster:0']
+        self.local_unit.return_value = 'unit/0'
+        mock_peer_units.return_value = ['unit/1']
+        self.related_units.return_value = ['unit/1']
+        self.is_ssl_cert_master.return_value = False
+        self.is_elected_leader.return_value = True
+        self.assertFalse(utils.ensure_ssl_cert_master())
+        self.assertFalse(self.relation_set.called)