From 61a7cc7a368e2ac1a2e634930379d2d39629bca7 Mon Sep 17 00:00:00 2001 From: Edward Hope-Morley Date: Thu, 29 Jan 2015 17:41:20 +0000 Subject: [PATCH 01/13] synced charm-helpers --- hooks/charmhelpers/contrib/unison/__init__.py | 10 ++++++---- hooks/charmhelpers/core/sysctl.py | 16 +++++++++++----- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/hooks/charmhelpers/contrib/unison/__init__.py b/hooks/charmhelpers/contrib/unison/__init__.py index f8551d10..4adcbf61 100644 --- a/hooks/charmhelpers/contrib/unison/__init__.py +++ b/hooks/charmhelpers/contrib/unison/__init__.py @@ -73,6 +73,7 @@ from charmhelpers.core.hookenv import ( relation_set, relation_get, unit_private_ip, + INFO, ERROR, ) @@ -86,7 +87,7 @@ def get_homedir(user): 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 @@ def collect_authed_hosts(peer_interface): 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 diff --git a/hooks/charmhelpers/core/sysctl.py b/hooks/charmhelpers/core/sysctl.py index d642a371..8e1b9eeb 100644 --- a/hooks/charmhelpers/core/sysctl.py +++ b/hooks/charmhelpers/core/sysctl.py @@ -26,25 +26,31 @@ from subprocess import check_call 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]) From 17a7e408a2c68fd845c0f5163718b981bf334e28 Mon Sep 17 00:00:00 2001 From: Edward Hope-Morley Date: Thu, 19 Feb 2015 21:05:09 +0000 Subject: [PATCH 02/13] more --- hooks/keystone_utils.py | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/hooks/keystone_utils.py b/hooks/keystone_utils.py index 2a6cb8ad..8cf269a1 100644 --- a/hooks/keystone_utils.py +++ b/hooks/keystone_utils.py @@ -770,6 +770,9 @@ def check_peer_actions(): elif action == 'update-ca-certificates': log("Running %s" % (action), level=DEBUG) subprocess.check_call(['update-ca-certificates']) + elif action == 'ensure-pki-permissions': + log("Running %s" % (action), level=DEBUG) + ensure_pki_cert_permissions() else: log("Unknown action flag=%s" % (flag), level=WARNING) @@ -963,12 +966,18 @@ def synchronize_ca(fatal=False): Returns a dictionary of settings to be set on the cluster relation. """ paths_to_sync = [SYNC_FLAGS_DIR] + peer_service_actions = [] + peer_actions = [] if bool_from_string(config('https-service-endpoints')): log("Syncing all endpoint certs since https-service-endpoints=True", level=DEBUG) paths_to_sync.append(SSL_DIR) paths_to_sync.append(CA_CERT_PATH) + # We need to restart peer apache services to ensure they have picked up + # new ssl keys. + peer_service_actions.append(('restart', ('apache2'))) + peer_actions.append('update-ca-certificates') if bool_from_string(config('use-https')): log("Syncing keystone-endpoint certs since use-https=True", @@ -976,10 +985,15 @@ def synchronize_ca(fatal=False): paths_to_sync.append(SSL_DIR) paths_to_sync.append(APACHE_SSL_DIR) paths_to_sync.append(CA_CERT_PATH) + # We need to restart peer apache services to ensure they have picked up + # new ssl keys. + peer_service_actions.append(('restart', ('apache2'))) + peer_actions.append('update-ca-certificates') if is_pki_enabled(): log("Syncing token certs", level=DEBUG) paths_to_sync.append(PKI_CERTS_DIR) + peer_actions.append('ensure-pki-permissions') # Ensure unique paths_to_sync = list(set(paths_to_sync)) @@ -991,10 +1005,11 @@ def synchronize_ca(fatal=False): if not os.path.isdir(SYNC_FLAGS_DIR): mkdir(SYNC_FLAGS_DIR, SSH_USER, 'keystone', 0o775) - # We need to restart peer apache services to ensure they have picked up - # new ssl keys. - create_peer_service_actions('restart', ['apache2']) - create_peer_actions(['update-ca-certificates']) + for action, services in set(peer_service_actions): + create_peer_service_actions(action, services) + + for action in set(peer_actions): + create_peer_actions(action) cluster_rel_settings = {} From 987f3f0da6a9259d45e8a13019b903a54e1dc5e7 Mon Sep 17 00:00:00 2001 From: Edward Hope-Morley Date: Thu, 19 Feb 2015 23:42:38 +0000 Subject: [PATCH 03/13] more --- hooks/keystone_hooks.py | 8 ++++++-- hooks/keystone_utils.py | 29 +++++++++++++---------------- unit_tests/test_keystone_hooks.py | 6 ++++++ 3 files changed, 25 insertions(+), 18 deletions(-) diff --git a/hooks/keystone_hooks.py b/hooks/keystone_hooks.py index f694128e..2c8c0c0e 100755 --- a/hooks/keystone_hooks.py +++ b/hooks/keystone_hooks.py @@ -73,7 +73,8 @@ from keystone_utils import ( clear_ssl_synced_units, is_db_initialised, is_pki_enabled, - ensure_pki_cert_permissions, + ensure_ssl_dir, + ensure_pki_dir_permissions, ) from charmhelpers.contrib.hahelpers.cluster import ( @@ -133,6 +134,9 @@ def config_changed(): if openstack_upgrade_available('keystone'): do_openstack_upgrade(configs=CONFIGS) + # Ensure ssl dir exists and is unison-accessible + ensure_ssl_dir() + check_call(['chmod', '-R', 'g+wrx', '/var/lib/keystone/']) # Ensure unison can write to certs dir. @@ -180,7 +184,7 @@ def initialise_pki(): '--keystone-group', 'keystone'] check_call(cmd) - ensure_pki_cert_permissions() + ensure_pki_dir_permissions() @hooks.hook('shared-db-relation-joined') diff --git a/hooks/keystone_utils.py b/hooks/keystone_utils.py index 8cf269a1..864294cf 100644 --- a/hooks/keystone_utils.py +++ b/hooks/keystone_utils.py @@ -231,7 +231,7 @@ valid_services = { } -def ensure_pki_cert_permissions(): +def ensure_pki_dir_permissions(): perms = 0o755 # Ensure accessible by unison user and group (for sync). for path in glob.glob("%s/*" % PKI_CERTS_DIR): @@ -772,7 +772,7 @@ def check_peer_actions(): subprocess.check_call(['update-ca-certificates']) elif action == 'ensure-pki-permissions': log("Running %s" % (action), level=DEBUG) - ensure_pki_cert_permissions() + ensure_pki_dir_permissions() else: log("Unknown action flag=%s" % (flag), level=WARNING) @@ -1153,20 +1153,23 @@ def synchronize_ca_if_changed(force=False, fatal=False): return inner_synchronize_ca_if_changed1 +def ensure_ssl_dir(): + """Ensure juju ssl dir exists and is unsion read/writable.""" + perms = 0o755 + if not os.path.isdir(SSL_DIR): + mkdir(SSL_DIR, SSH_USER, 'keystone', perms) + else: + ensure_permissions(SSL_DIR, user=SSH_USER, group='keystone', + perms=perms) + + def get_ca(user='keystone', group='keystone'): """Initialize a new CA object if one hasn't already been loaded. This will create a new CA or load an existing one. """ if not ssl.CA_SINGLETON: - # Ensure unsion read/writable - perms = 0o755 - if not os.path.isdir(SSL_DIR): - mkdir(SSL_DIR, SSH_USER, 'keystone', perms) - else: - ensure_permissions(SSL_DIR, user=SSH_USER, group='keystone', - perms=perms) - + ensure_ssl_dir() d_name = '_'.join(SSL_CA_NAME.lower().split(' ')) ca = ssl.JujuCA(name=SSL_CA_NAME, user=user, group=group, ca_dir=os.path.join(SSL_DIR, @@ -1174,12 +1177,6 @@ def get_ca(user='keystone', group='keystone'): root_ca_dir=os.path.join(SSL_DIR, '%s_root_ca' % d_name)) - # SSL_DIR is synchronized via all peers over unison+ssh, need - # to ensure permissions. - subprocess.check_output(['chown', '-R', '%s.%s' % (user, group), - '%s' % SSL_DIR]) - subprocess.check_output(['chmod', '-R', 'g+rwx', '%s' % SSL_DIR]) - # 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 diff --git a/unit_tests/test_keystone_hooks.py b/unit_tests/test_keystone_hooks.py index fc0c3177..e47e2b69 100644 --- a/unit_tests/test_keystone_hooks.py +++ b/unit_tests/test_keystone_hooks.py @@ -273,6 +273,7 @@ class KeystoneRelationTests(CharmTestCase): @patch('keystone_utils.log') @patch('keystone_utils.ensure_ssl_cert_master') + @patch.object(hooks, 'ensure_ssl_dir') @patch.object(hooks, 'is_pki_enabled') @patch.object(hooks, 'is_ssl_cert_master') @patch.object(hooks, 'is_db_initialised') @@ -298,6 +299,7 @@ class KeystoneRelationTests(CharmTestCase): mock_is_db_initialised, mock_is_ssl_cert_master, mock_is_pki_enabled, + mock_ensure_ssl_dir, mock_ensure_ssl_cert_master, mock_log): mock_is_pki_enabled.return_value = True @@ -330,6 +332,7 @@ class KeystoneRelationTests(CharmTestCase): @patch('keystone_utils.log') @patch('keystone_utils.ensure_ssl_cert_master') + @patch.object(hooks, 'ensure_ssl_dir') @patch.object(hooks, 'is_pki_enabled') @patch.object(hooks, 'is_ssl_cert_master') @patch.object(hooks, 'ensure_permissions') @@ -343,6 +346,7 @@ class KeystoneRelationTests(CharmTestCase): self, configure_https, identity_changed, configs, get_homedir, ensure_user, cluster_joined, ensure_permissions, mock_is_ssl_cert_master, mock_is_pki_enabled, + mock_ensure_ssl_dir, mock_ensure_ssl_cert_master, mock_log): mock_is_pki_enabled.return_value = True mock_is_ssl_cert_master.return_value = True @@ -364,6 +368,7 @@ class KeystoneRelationTests(CharmTestCase): @patch('keystone_utils.log') @patch('keystone_utils.ensure_ssl_cert_master') + @patch.object(hooks, 'ensure_ssl_dir') @patch.object(hooks, 'is_pki_enabled') @patch.object(hooks, 'is_ssl_cert_master') @patch.object(hooks, 'is_db_initialised') @@ -388,6 +393,7 @@ class KeystoneRelationTests(CharmTestCase): mock_is_db_initialised, mock_is_ssl_cert_master, mock_is_pki_enabled, + mock_ensure_ssl_dir, mock_ensure_ssl_cert_master, mock_log): mock_is_pki_enabled.return_value = True From fac79a5bdcc0eea109cd8de0158b60a93046d70c Mon Sep 17 00:00:00 2001 From: Edward Hope-Morley Date: Fri, 20 Feb 2015 00:30:36 +0000 Subject: [PATCH 04/13] more --- hooks/keystone_utils.py | 37 +++++++++++++------------------ unit_tests/test_keystone_hooks.py | 22 +++++++++++++----- 2 files changed, 32 insertions(+), 27 deletions(-) diff --git a/hooks/keystone_utils.py b/hooks/keystone_utils.py index 864294cf..52eed64d 100644 --- a/hooks/keystone_utils.py +++ b/hooks/keystone_utils.py @@ -231,14 +231,6 @@ valid_services = { } -def ensure_pki_dir_permissions(): - perms = 0o755 - # Ensure accessible by unison user and group (for sync). - for path in glob.glob("%s/*" % PKI_CERTS_DIR): - ensure_permissions(path, user=SSH_USER, group='keystone', perms=perms, - recurse=True) - - def resource_map(): """Dynamically generate a map of resources that will be managed for a single hook execution. @@ -702,19 +694,6 @@ def ensure_permissions(path, user=None, group=None, perms=None, recurse=False, Note that -1 for uid or gid result in no change. """ - if recurse: - if not maxdepth: - log("Max recursion depth reached - skipping further recursion") - return - - paths = glob.glob("%s/*" % (path)) - if len(paths) > 1: - for path in paths: - ensure_permissions(path, user=user, group=group, perms=perms, - recurse=recurse, maxdepth=maxdepth - 1) - - return - if user: uid = pwd.getpwnam(user).pw_uid else: @@ -730,6 +709,16 @@ def ensure_permissions(path, user=None, group=None, perms=None, recurse=False, if perms: os.chmod(path, perms) + if recurse: + if not maxdepth: + log("Max recursion depth reached - skipping further recursion") + return + + paths = glob.glob("%s/*" % (path)) + for path in paths: + ensure_permissions(path, user=user, group=group, perms=perms, + recurse=recurse, maxdepth=maxdepth - 1) + def check_peer_actions(): """Honour service action requests from sync master. @@ -952,6 +941,12 @@ def is_pki_enabled(): return False +def ensure_pki_dir_permissions(): + # Ensure accessible by unison user and group (for sync). + ensure_permissions(PKI_CERTS_DIR, user=SSH_USER, group='keystone', + perms=0o755, recurse=True) + + def synchronize_ca(fatal=False): """Broadcast service credentials to peers. diff --git a/unit_tests/test_keystone_hooks.py b/unit_tests/test_keystone_hooks.py index e47e2b69..77cd4bb9 100644 --- a/unit_tests/test_keystone_hooks.py +++ b/unit_tests/test_keystone_hooks.py @@ -273,6 +273,7 @@ class KeystoneRelationTests(CharmTestCase): @patch('keystone_utils.log') @patch('keystone_utils.ensure_ssl_cert_master') + @patch.object(hooks, 'ensure_pki_dir_permissions') @patch.object(hooks, 'ensure_ssl_dir') @patch.object(hooks, 'is_pki_enabled') @patch.object(hooks, 'is_ssl_cert_master') @@ -300,6 +301,7 @@ class KeystoneRelationTests(CharmTestCase): mock_is_ssl_cert_master, mock_is_pki_enabled, mock_ensure_ssl_dir, + mock_ensure_pki_dir_permissions, mock_ensure_ssl_cert_master, mock_log): mock_is_pki_enabled.return_value = True @@ -332,6 +334,7 @@ class KeystoneRelationTests(CharmTestCase): @patch('keystone_utils.log') @patch('keystone_utils.ensure_ssl_cert_master') + @patch.object(hooks, 'ensure_pki_dir_permissions') @patch.object(hooks, 'ensure_ssl_dir') @patch.object(hooks, 'is_pki_enabled') @patch.object(hooks, 'is_ssl_cert_master') @@ -342,12 +345,17 @@ class KeystoneRelationTests(CharmTestCase): @patch.object(hooks, 'CONFIGS') @patch.object(hooks, 'identity_changed') @patch.object(hooks, 'configure_https') - def test_config_changed_no_openstack_upgrade_not_leader( - self, configure_https, identity_changed, - configs, get_homedir, ensure_user, cluster_joined, - ensure_permissions, mock_is_ssl_cert_master, mock_is_pki_enabled, - mock_ensure_ssl_dir, - mock_ensure_ssl_cert_master, mock_log): + def test_config_changed_no_upgrade_not_leader(self, configure_https, + identity_changed, + configs, get_homedir, + ensure_user, cluster_joined, + ensure_permissions, + mock_is_ssl_cert_master, + mock_is_pki_enabled, + mock_ensure_ssl_dir, + mock_ensure_pki_permissions, + mock_ensure_ssl_cert_master, + mock_log): mock_is_pki_enabled.return_value = True mock_is_ssl_cert_master.return_value = True self.openstack_upgrade_available.return_value = False @@ -368,6 +376,7 @@ class KeystoneRelationTests(CharmTestCase): @patch('keystone_utils.log') @patch('keystone_utils.ensure_ssl_cert_master') + @patch.object(hooks, 'ensure_pki_dir_permissions') @patch.object(hooks, 'ensure_ssl_dir') @patch.object(hooks, 'is_pki_enabled') @patch.object(hooks, 'is_ssl_cert_master') @@ -394,6 +403,7 @@ class KeystoneRelationTests(CharmTestCase): mock_is_ssl_cert_master, mock_is_pki_enabled, mock_ensure_ssl_dir, + mock_ensure_pki_permissions, mock_ensure_ssl_cert_master, mock_log): mock_is_pki_enabled.return_value = True From e08e63c092f9ebc30b8f1d3809ebbb15ed95b7b0 Mon Sep 17 00:00:00 2001 From: Edward Hope-Morley Date: Fri, 20 Feb 2015 12:20:16 +0000 Subject: [PATCH 05/13] fix race --- hooks/keystone_hooks.py | 6 +++++- hooks/keystone_utils.py | 10 ++++++++++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/hooks/keystone_hooks.py b/hooks/keystone_hooks.py index 2c8c0c0e..83ab2294 100755 --- a/hooks/keystone_hooks.py +++ b/hooks/keystone_hooks.py @@ -75,6 +75,7 @@ from keystone_utils import ( is_pki_enabled, ensure_ssl_dir, ensure_pki_dir_permissions, + force_ssl_sync, ) from charmhelpers.contrib.hahelpers.cluster import ( @@ -454,7 +455,7 @@ def cluster_changed(): check_peer_actions() - if is_elected_leader(CLUSTER_RES) or is_ssl_cert_master(): + if is_elected_leader(CLUSTER_RES): units = get_ssl_sync_request_units() synced_units = relation_get(attribute='ssl-synced-units', unit=local_unit()) @@ -474,6 +475,9 @@ def cluster_changed(): for rid in relation_ids('identity-admin'): admin_relation_changed(rid) + elif is_ssl_cert_master(): + # Sync and let go + force_ssl_sync() else: CONFIGS.write_all() diff --git a/hooks/keystone_utils.py b/hooks/keystone_utils.py index 52eed64d..76d28fc9 100644 --- a/hooks/keystone_utils.py +++ b/hooks/keystone_utils.py @@ -1148,6 +1148,16 @@ def synchronize_ca_if_changed(force=False, fatal=False): return inner_synchronize_ca_if_changed1 +@synchronize_ca_if_changed(force=True, fatal=True) +def force_ssl_sync(): + """Force SSL sync to all peers. + + This is useful if we need to relinquish ssl-cert-master status while + making sure that the new master has upt-o-date certs. + """ + pass + + def ensure_ssl_dir(): """Ensure juju ssl dir exists and is unsion read/writable.""" perms = 0o755 From d11af8fa625b262bf37fb4958aef0ed044d1955f Mon Sep 17 00:00:00 2001 From: Edward Hope-Morley Date: Fri, 20 Feb 2015 13:34:29 +0000 Subject: [PATCH 06/13] fix race --- hooks/keystone_hooks.py | 39 ++++++++++++++++--------------- hooks/keystone_utils.py | 2 +- unit_tests/test_keystone_hooks.py | 34 +++++++++++++++++++++++---- 3 files changed, 51 insertions(+), 24 deletions(-) diff --git a/hooks/keystone_hooks.py b/hooks/keystone_hooks.py index 83ab2294..9eb5e85f 100755 --- a/hooks/keystone_hooks.py +++ b/hooks/keystone_hooks.py @@ -159,7 +159,7 @@ def config_changed(): # Update relations since SSL may have been configured. If we have peer # units we can rely on the sync to do this in cluster relation. - if is_elected_leader(CLUSTER_RES) and not peer_units(): + if not peer_units(): update_all_identity_relation_units() for rid in relation_ids('identity-admin'): @@ -326,6 +326,7 @@ def identity_changed(relation_id=None, remote_unit=None): peerdb_settings = peer_retrieve_by_prefix(rel_id) if 'service_password' in peerdb_settings: relation_set(relation_id=rel_id, **peerdb_settings) + log('Deferring identity_changed() to service leader.') if notifications: @@ -455,27 +456,27 @@ def cluster_changed(): check_peer_actions() - if is_elected_leader(CLUSTER_RES): - units = get_ssl_sync_request_units() - synced_units = relation_get(attribute='ssl-synced-units', - unit=local_unit()) - if synced_units: - synced_units = json.loads(synced_units) - diff = set(units).symmetric_difference(set(synced_units)) + units = get_ssl_sync_request_units() + synced_units = relation_get(attribute='ssl-synced-units', + unit=local_unit()) + if synced_units: + synced_units = json.loads(synced_units) + diff = set(units).symmetric_difference(set(synced_units)) - if is_pki_enabled(): - initialise_pki() + if is_pki_enabled(): + initialise_pki() - if units and (not synced_units or diff): - log("New peers joined and need syncing - %s" % - (', '.join(units)), level=DEBUG) - update_all_identity_relation_units_force_sync() - else: - update_all_identity_relation_units() + if units and (not synced_units or diff): + log("New peers joined and need syncing - %s" % + (', '.join(units)), level=DEBUG) + update_all_identity_relation_units_force_sync() + else: + update_all_identity_relation_units() - for rid in relation_ids('identity-admin'): - admin_relation_changed(rid) - elif is_ssl_cert_master(): + for rid in relation_ids('identity-admin'): + admin_relation_changed(rid) + + if not is_elected_leader(CLUSTER_RES) and is_ssl_cert_master(): # Sync and let go force_ssl_sync() else: diff --git a/hooks/keystone_utils.py b/hooks/keystone_utils.py index 76d28fc9..121086f8 100644 --- a/hooks/keystone_utils.py +++ b/hooks/keystone_utils.py @@ -1101,7 +1101,7 @@ def synchronize_ca_if_changed(force=False, fatal=False): return f(*args, **kwargs) if not ensure_ssl_cert_master(): - log("Not leader - ignoring sync", level=DEBUG) + log("Not ssl-cert-master - ignoring sync", level=DEBUG) return f(*args, **kwargs) peer_settings = {} diff --git a/unit_tests/test_keystone_hooks.py b/unit_tests/test_keystone_hooks.py index 77cd4bb9..587c2836 100644 --- a/unit_tests/test_keystone_hooks.py +++ b/unit_tests/test_keystone_hooks.py @@ -334,9 +334,11 @@ class KeystoneRelationTests(CharmTestCase): @patch('keystone_utils.log') @patch('keystone_utils.ensure_ssl_cert_master') + @patch.object(hooks, 'update_all_identity_relation_units') @patch.object(hooks, 'ensure_pki_dir_permissions') @patch.object(hooks, 'ensure_ssl_dir') @patch.object(hooks, 'is_pki_enabled') + @patch.object(hooks, 'peer_units') @patch.object(hooks, 'is_ssl_cert_master') @patch.object(hooks, 'ensure_permissions') @patch.object(hooks, 'cluster_joined') @@ -351,13 +353,16 @@ class KeystoneRelationTests(CharmTestCase): ensure_user, cluster_joined, ensure_permissions, mock_is_ssl_cert_master, + mock_peer_units, mock_is_pki_enabled, mock_ensure_ssl_dir, mock_ensure_pki_permissions, + mock_update_all_id_rel_units, mock_ensure_ssl_cert_master, mock_log): mock_is_pki_enabled.return_value = True mock_is_ssl_cert_master.return_value = True + mock_peer_units.return_value = [] self.openstack_upgrade_available.return_value = False self.is_elected_leader.return_value = False mock_ensure_ssl_cert_master.return_value = False @@ -483,8 +488,12 @@ class KeystoneRelationTests(CharmTestCase): user=self.ssh_user, group='juju_keystone', peer_interface='cluster', ensure_local_user=True) + @patch.object(hooks, 'update_all_identity_relation_units') + @patch.object(hooks, 'apply_echo_filters') + @patch.object(hooks, 'get_ssl_sync_request_units') @patch.object(hooks, 'is_ssl_cert_master') @patch.object(hooks, 'peer_units') + @patch('keystone_utils.config') @patch('keystone_utils.log') @patch('keystone_utils.ensure_ssl_cert_master') @patch('keystone_utils.synchronize_ca') @@ -494,14 +503,31 @@ class KeystoneRelationTests(CharmTestCase): def test_cluster_changed(self, configs, ssh_authorized_peers, check_peer_actions, mock_synchronize_ca, mock_ensure_ssl_cert_master, - mock_log, mock_peer_units, - mock_is_ssl_cert_master): + mock_log, mock_config, mock_peer_units, + mock_is_ssl_cert_master, + mock_get_ssl_sync_request_units, + mock_apply_echo_filters, + mock_update_all_identity_relation_units): + + relation_settings = {'foo_passwd': '123', + 'identity-service:16_foo': 'bar'} + + mock_apply_echo_filters.return_value = (relation_settings.keys(), {}) mock_is_ssl_cert_master.return_value = False mock_peer_units.return_value = ['unit/0'] mock_ensure_ssl_cert_master.return_value = False self.is_elected_leader.return_value = False - self.relation_get.return_value = {'foo_passwd': '123', - 'identity-service:16_foo': 'bar'} + + def fake_rel_get(attribute=None, *args, **kwargs): + if not attribute: + return relation_settings + + return relation_settings.get(attribute) + + self.relation_get.side_effect = fake_rel_get + + mock_config.return_value = None + hooks.cluster_changed() self.peer_echo.assert_called_with(includes=['foo_passwd', 'identity-service:16_foo']) From 284776a166cc71cc8dc91964bc73b47886f585e6 Mon Sep 17 00:00:00 2001 From: Edward Hope-Morley Date: Fri, 20 Feb 2015 14:19:45 +0000 Subject: [PATCH 07/13] fix race --- hooks/keystone_hooks.py | 40 ++----------------------------- unit_tests/test_keystone_hooks.py | 10 ++++---- 2 files changed, 6 insertions(+), 44 deletions(-) diff --git a/hooks/keystone_hooks.py b/hooks/keystone_hooks.py index 9eb5e85f..61a5e162 100755 --- a/hooks/keystone_hooks.py +++ b/hooks/keystone_hooks.py @@ -91,7 +91,6 @@ from charmhelpers.contrib.peerstorage import ( ) from charmhelpers.contrib.openstack.ip import ( ADMIN, - PUBLIC, resolve_address, ) from charmhelpers.contrib.network.ip import ( @@ -404,37 +403,6 @@ def cluster_joined(): send_ssl_sync_request() -def apply_echo_filters(settings, echo_whitelist): - """Filter settings to be peer_echo'ed. - - We may have received some data that we don't want to re-echo so filter - out unwanted keys and provide overrides. - - Returns: - tuple(filtered list of keys to be echoed, overrides for keys omitted) - """ - filtered = [] - overrides = {} - for key in settings.iterkeys(): - for ekey in echo_whitelist: - if ekey in key: - if ekey == 'identity-service:': - auth_host = resolve_address(ADMIN) - service_host = resolve_address(PUBLIC) - if (key.endswith('auth_host') and - settings[key] != auth_host): - overrides[key] = auth_host - continue - elif (key.endswith('service_host') and - settings[key] != service_host): - overrides[key] = service_host - continue - - filtered.append(key) - - return filtered, overrides - - @hooks.hook('cluster-relation-changed', 'cluster-relation-departed') @restart_on_change(restart_map(), stopstart=True) @@ -443,13 +411,9 @@ def cluster_changed(): group='juju_keystone', peer_interface='cluster', ensure_local_user=True) - settings = relation_get() # NOTE(jamespage) re-echo passwords for peer storage - echo_whitelist, overrides = \ - apply_echo_filters(settings, ['_passwd', 'identity-service:', - 'ssl-cert-master', 'db-initialised']) - log("Peer echo overrides: %s" % (overrides), level=DEBUG) - relation_set(**overrides) + echo_whitelist = ['_passwd', 'identity-service:', 'ssl-cert-master', + 'db-initialised'] if echo_whitelist: log("Peer echo whitelist: %s" % (echo_whitelist), level=DEBUG) peer_echo(includes=echo_whitelist) diff --git a/unit_tests/test_keystone_hooks.py b/unit_tests/test_keystone_hooks.py index 587c2836..becf68ba 100644 --- a/unit_tests/test_keystone_hooks.py +++ b/unit_tests/test_keystone_hooks.py @@ -489,7 +489,6 @@ class KeystoneRelationTests(CharmTestCase): peer_interface='cluster', ensure_local_user=True) @patch.object(hooks, 'update_all_identity_relation_units') - @patch.object(hooks, 'apply_echo_filters') @patch.object(hooks, 'get_ssl_sync_request_units') @patch.object(hooks, 'is_ssl_cert_master') @patch.object(hooks, 'peer_units') @@ -506,13 +505,11 @@ class KeystoneRelationTests(CharmTestCase): mock_log, mock_config, mock_peer_units, mock_is_ssl_cert_master, mock_get_ssl_sync_request_units, - mock_apply_echo_filters, mock_update_all_identity_relation_units): relation_settings = {'foo_passwd': '123', 'identity-service:16_foo': 'bar'} - - mock_apply_echo_filters.return_value = (relation_settings.keys(), {}) + mock_is_ssl_cert_master.return_value = False mock_peer_units.return_value = ['unit/0'] mock_ensure_ssl_cert_master.return_value = False @@ -529,8 +526,9 @@ class KeystoneRelationTests(CharmTestCase): mock_config.return_value = None hooks.cluster_changed() - self.peer_echo.assert_called_with(includes=['foo_passwd', - 'identity-service:16_foo']) + whitelist = ['_passwd', 'identity-service:', 'ssl-cert-master', + 'db-initialised'] + self.peer_echo.assert_called_with(includes=whitelist) ssh_authorized_peers.assert_called_with( user=self.ssh_user, group='juju_keystone', peer_interface='cluster', ensure_local_user=True) From 78d0bd26d8a68b7f3999fc2dfdb172731a84e462 Mon Sep 17 00:00:00 2001 From: Edward Hope-Morley Date: Tue, 24 Feb 2015 12:37:32 +0000 Subject: [PATCH 08/13] synced charm-helpers; --- .../charmhelpers/contrib/openstack/context.py | 37 ++++++++++++++----- .../contrib/openstack/templates/zeromq | 14 +++++++ 2 files changed, 42 insertions(+), 9 deletions(-) create mode 100644 hooks/charmhelpers/contrib/openstack/templates/zeromq diff --git a/hooks/charmhelpers/contrib/openstack/context.py b/hooks/charmhelpers/contrib/openstack/context.py index c7c4cd4a..d268ea8f 100644 --- a/hooks/charmhelpers/contrib/openstack/context.py +++ b/hooks/charmhelpers/contrib/openstack/context.py @@ -279,9 +279,25 @@ def db_ssl(rdata, ctxt, ssl_dir): class IdentityServiceContext(OSContextGenerator): interfaces = ['identity-service'] + def __init__(self, service=None, service_user=None): + self.service = service + self.service_user = service_user + def __call__(self): log('Generating template context for identity-service', level=DEBUG) ctxt = {} + + if self.service and self.service_user: + # This is required for pki token signing if we don't want /tmp to + # be used. + cachedir = '/var/cache/%s' % (self.service) + if not os.path.isdir(cachedir): + log("Creating service cache dir %s" % (cachedir), level=DEBUG) + mkdir(path=cachedir, owner=self.service_user, + group=self.service_user, perms=0o700) + + ctxt['signing_dir'] = cachedir + for rid in relation_ids('identity-service'): for unit in related_units(rid): rdata = relation_get(rid=rid, unit=unit) @@ -291,15 +307,16 @@ class IdentityServiceContext(OSContextGenerator): auth_host = format_ipv6_addr(auth_host) or auth_host svc_protocol = rdata.get('service_protocol') or 'http' auth_protocol = rdata.get('auth_protocol') or 'http' - ctxt = {'service_port': rdata.get('service_port'), - 'service_host': serv_host, - 'auth_host': auth_host, - 'auth_port': rdata.get('auth_port'), - 'admin_tenant_name': rdata.get('service_tenant'), - 'admin_user': rdata.get('service_username'), - 'admin_password': rdata.get('service_password'), - 'service_protocol': svc_protocol, - 'auth_protocol': auth_protocol} + ctxt.update({'service_port': rdata.get('service_port'), + 'service_host': serv_host, + 'auth_host': auth_host, + 'auth_port': rdata.get('auth_port'), + 'admin_tenant_name': rdata.get('service_tenant'), + 'admin_user': rdata.get('service_username'), + 'admin_password': rdata.get('service_password'), + 'service_protocol': svc_protocol, + 'auth_protocol': auth_protocol}) + if context_complete(ctxt): # NOTE(jamespage) this is required for >= icehouse # so a missing value just indicates keystone needs @@ -1021,6 +1038,8 @@ class ZeroMQContext(OSContextGenerator): for unit in related_units(rid): ctxt['zmq_nonce'] = relation_get('nonce', unit, rid) ctxt['zmq_host'] = relation_get('host', unit, rid) + ctxt['zmq_redis_address'] = relation_get( + 'zmq_redis_address', unit, rid) return ctxt diff --git a/hooks/charmhelpers/contrib/openstack/templates/zeromq b/hooks/charmhelpers/contrib/openstack/templates/zeromq new file mode 100644 index 00000000..0695eef1 --- /dev/null +++ b/hooks/charmhelpers/contrib/openstack/templates/zeromq @@ -0,0 +1,14 @@ +{% if zmq_host -%} +# ZeroMQ configuration (restart-nonce: {{ zmq_nonce }}) +rpc_backend = zmq +rpc_zmq_host = {{ zmq_host }} +{% if zmq_redis_address -%} +rpc_zmq_matchmaker = oslo.messaging._drivers.matchmaker_redis.MatchMakerRedis +matchmaker_heartbeat_freq = 15 +matchmaker_heartbeat_ttl = 30 +[matchmaker_redis] +host = {{ zmq_redis_address }} +{% else -%} +rpc_zmq_matchmaker = oslo.messaging._drivers.matchmaker_ring.MatchMakerRing +{% endif -%} +{% endif -%} From bfeca73d148fd0d336c95a95f8d6c88134b71502 Mon Sep 17 00:00:00 2001 From: Edward Hope-Morley Date: Tue, 24 Feb 2015 13:35:09 +0000 Subject: [PATCH 09/13] don't push race-prone data on cluster relation to avoid spinning --- hooks/keystone_utils.py | 20 ++++++++++++++------ unit_tests/test_keystone_utils.py | 25 ++++++++++++++----------- 2 files changed, 28 insertions(+), 17 deletions(-) diff --git a/hooks/keystone_utils.py b/hooks/keystone_utils.py index 121086f8..0fb4cfce 100644 --- a/hooks/keystone_utils.py +++ b/hooks/keystone_utils.py @@ -1226,9 +1226,13 @@ def add_service_to_keystone(relation_id=None, remote_unit=None): # Some backend services advertise no endpoint but require a # hook execution to update auth strategy. relation_data = {} + rel_only_data = {} # Check if clustered and use vip + haproxy ports if so - relation_data["auth_host"] = resolve_address(ADMIN) - relation_data["service_host"] = resolve_address(PUBLIC) + # NOTE(hopem): don't put these on peer relation because racey + # leader election causes cluster relation to spin) + rel_only_data["auth_host"] = resolve_address(ADMIN) + rel_only_data["service_host"] = resolve_address(PUBLIC) + relation_data["auth_protocol"] = protocol relation_data["service_protocol"] = protocol relation_data["auth_port"] = config('admin-port') @@ -1251,8 +1255,8 @@ def add_service_to_keystone(relation_id=None, remote_unit=None): log("Creating requested role: %s" % role) create_role(role) - peer_store_and_set(relation_id=relation_id, - **relation_data) + relation_set(relation_id=relation_id, **rel_only_data) + peer_store_and_set(relation_id=relation_id, **relation_data) return else: ensure_valid_service(settings['service']) @@ -1355,11 +1359,14 @@ def add_service_to_keystone(relation_id=None, remote_unit=None): # we return a token, information about our API endpoints, and the generated # service credentials service_tenant = config('service-tenant') + + # NOTE(hopem): don't put these on peer relation because racey + # leader election causes cluster relation to spin) + rel_only_data = {"auth_host": resolve_address(ADMIN), + "service_host": resolve_address(PUBLIC)} relation_data = { "admin_token": token, - "service_host": resolve_address(PUBLIC), "service_port": config("service-port"), - "auth_host": resolve_address(ADMIN), "auth_port": config("admin-port"), "service_username": service_username, "service_password": service_password, @@ -1392,6 +1399,7 @@ def add_service_to_keystone(relation_id=None, remote_unit=None): relation_data['ca_cert'] = b64encode(ca_bundle) relation_data['https_keystone'] = 'True' + relation_set(relation_id=relation_id, **rel_only_data) peer_store_and_set(relation_id=relation_id, **relation_data) diff --git a/unit_tests/test_keystone_utils.py b/unit_tests/test_keystone_utils.py index ac71496c..e39af5f9 100644 --- a/unit_tests/test_keystone_utils.py +++ b/unit_tests/test_keystone_utils.py @@ -179,18 +179,19 @@ class TestKeystoneUtils(CharmTestCase): self.assertTrue(self.https.called) self.assertTrue(self.create_role.called) - relation_data = {'auth_host': '10.10.10.10', - 'service_host': '10.10.10.10', - 'auth_protocol': 'https', + rel_only_data = {'auth_host': '10.10.10.10', + 'service_host': '10.10.10.10'} + relation_data = {'auth_protocol': 'https', 'service_protocol': 'https', 'auth_port': 80, 'service_port': 81, 'https_keystone': 'True', 'ca_cert': 'certificate', 'region': 'RegionOne'} - self.peer_store_and_set.assert_called_with( - relation_id=relation_id, - **relation_data) + self.relation_set.assert_called_with(relation_id=relation_id, + **rel_only_data) + self.peer_store_and_set.assert_called_with(relation_id=relation_id, + **relation_data) @patch.object(utils, 'ensure_valid_service') @patch.object(utils, 'add_endpoint') @@ -235,19 +236,21 @@ class TestKeystoneUtils(CharmTestCase): self.grant_role.assert_called_with('keystone', 'admin', 'tenant') self.create_role.assert_called_with('role1', 'keystone', 'tenant') + rel_only_data = {'auth_host': '10.0.0.3', + 'service_host': '10.0.0.3'} relation_data = {'admin_token': 'token', 'service_port': 81, 'auth_port': 80, 'service_username': 'keystone', 'service_password': 'password', 'service_tenant': 'tenant', 'https_keystone': 'False', 'ssl_cert': '', 'ssl_key': '', - 'ca_cert': '', 'auth_host': '10.0.0.3', - 'service_host': '10.0.0.3', + 'ca_cert': '', 'auth_protocol': 'http', 'service_protocol': 'http', 'service_tenant_id': 'tenant_id'} - self.peer_store_and_set.assert_called_with( - relation_id=relation_id, - **relation_data) + self.relation_set.assert_called_with(relation_id=relation_id, + **rel_only_data) + self.peer_store_and_set.assert_called_with(relation_id=relation_id, + **relation_data) @patch.object(utils, 'ensure_valid_service') @patch.object(utils, 'add_endpoint') From fdba570747986c32675d713e565b903d6dec975e Mon Sep 17 00:00:00 2001 From: Edward Hope-Morley Date: Tue, 24 Feb 2015 14:51:28 +0000 Subject: [PATCH 10/13] remove pkiz --- hooks/keystone_context.py | 3 --- hooks/keystone_hooks.py | 4 ---- hooks/keystone_utils.py | 4 +--- 3 files changed, 1 insertion(+), 10 deletions(-) diff --git a/hooks/keystone_context.py b/hooks/keystone_context.py index a6794abe..fd3c8725 100644 --- a/hooks/keystone_context.py +++ b/hooks/keystone_context.py @@ -203,12 +203,9 @@ class KeystoneContext(context.OSContextGenerator): ctxt['ldap_config_flags'] = flags enable_pki = config('enable-pki') - enable_pkiz = config('enable-pkiz') if enable_pki and bool_from_string(enable_pki): ctxt['signing'] = True ctxt['token_provider'] = 'pki' - elif enable_pkiz and bool_from_string(enable_pkiz): - ctxt['token_provider'] = 'pkiz' if 'token_provider' in ctxt: log("Configuring PKI token cert paths", level=DEBUG) diff --git a/hooks/keystone_hooks.py b/hooks/keystone_hooks.py index 61a5e162..0e3a23b4 100755 --- a/hooks/keystone_hooks.py +++ b/hooks/keystone_hooks.py @@ -358,10 +358,6 @@ def send_ssl_sync_request(): if enable_pki and bool_from_string(enable_pki): count += 3 - enable_pkiz = config('enable-pkiz') - if enable_pkiz and bool_from_string(enable_pkiz): - count += 4 - if count: key = 'ssl-sync-required-%s' % (unit) settings = {key: count} diff --git a/hooks/keystone_utils.py b/hooks/keystone_utils.py index 0fb4cfce..60760f70 100644 --- a/hooks/keystone_utils.py +++ b/hooks/keystone_utils.py @@ -933,9 +933,7 @@ def ensure_ssl_cert_master(): def is_pki_enabled(): enable_pki = config('enable-pki') - enable_pkiz = config('enable-pkiz') - if (enable_pki and bool_from_string(enable_pki) or - enable_pkiz and bool_from_string(enable_pkiz)): + if enable_pki and bool_from_string(enable_pki): return True return False From 5695eda17eac2f2f2888c03b745dadcbbcb17073 Mon Sep 17 00:00:00 2001 From: Edward Hope-Morley Date: Wed, 11 Mar 2015 12:11:10 +0000 Subject: [PATCH 11/13] [trivial] fix Makefile publish rule --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 9b1f7ad3..1e9e06da 100644 --- a/Makefile +++ b/Makefile @@ -26,6 +26,6 @@ sync: bin/charm_helpers_sync.py @$(PYTHON) bin/charm_helpers_sync.py -c charm-helpers-hooks.yaml @$(PYTHON) bin/charm_helpers_sync.py -c charm-helpers-tests.yaml -publish: lint test +publish: lint unit_test bzr push lp:charms/keystone bzr push lp:charms/trusty/keystone From e47d3a214eb742fa41c5f425989dd86ba76f2205 Mon Sep 17 00:00:00 2001 From: Edward Hope-Morley Date: Wed, 11 Mar 2015 14:46:42 +0000 Subject: [PATCH 12/13] cleanuo --- hooks/keystone_hooks.py | 11 +++++++---- hooks/keystone_utils.py | 4 ++-- 2 files changed, 9 insertions(+), 6 deletions(-) diff --git a/hooks/keystone_hooks.py b/hooks/keystone_hooks.py index d2cd1770..4ab616c6 100755 --- a/hooks/keystone_hooks.py +++ b/hooks/keystone_hooks.py @@ -427,16 +427,18 @@ def cluster_changed(): check_peer_actions() + if is_pki_enabled(): + initialise_pki() + + # Figure out if we need to mandate a sync units = get_ssl_sync_request_units() synced_units = relation_get(attribute='ssl-synced-units', unit=local_unit()) + diff = None if synced_units: synced_units = json.loads(synced_units) diff = set(units).symmetric_difference(set(synced_units)) - if is_pki_enabled(): - initialise_pki() - if units and (not synced_units or diff): log("New peers joined and need syncing - %s" % (', '.join(units)), level=DEBUG) @@ -448,7 +450,8 @@ def cluster_changed(): admin_relation_changed(rid) if not is_elected_leader(CLUSTER_RES) and is_ssl_cert_master(): - # Sync and let go + # Force and sync and trigger a sync master re-election since we are not + # leader anymore. force_ssl_sync() else: CONFIGS.write_all() diff --git a/hooks/keystone_utils.py b/hooks/keystone_utils.py index da5590be..33dba69c 100644 --- a/hooks/keystone_utils.py +++ b/hooks/keystone_utils.py @@ -1170,9 +1170,9 @@ def force_ssl_sync(): """Force SSL sync to all peers. This is useful if we need to relinquish ssl-cert-master status while - making sure that the new master has upt-o-date certs. + making sure that the new master has up-to-date certs. """ - pass + return def ensure_ssl_dir(): From 3c3faa872898304d6a335f8592a749c2e0c99a79 Mon Sep 17 00:00:00 2001 From: Edward Hope-Morley Date: Sun, 15 Mar 2015 11:16:24 +0100 Subject: [PATCH 13/13] [trivial] fix peer actions (broken in previous commit) --- hooks/keystone_utils.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/hooks/keystone_utils.py b/hooks/keystone_utils.py index 962f23a1..ee921e15 100644 --- a/hooks/keystone_utils.py +++ b/hooks/keystone_utils.py @@ -1020,8 +1020,7 @@ def synchronize_ca(fatal=False): for action, services in set(peer_service_actions): create_peer_service_actions(action, services) - for action in set(peer_actions): - create_peer_actions(action) + create_peer_actions(peer_actions) cluster_rel_settings = {}