From 21e3890d310c0c68b1a95533de972ebcc7d81078 Mon Sep 17 00:00:00 2001 From: Liam Young Date: Fri, 24 Oct 2014 08:51:39 +0000 Subject: [PATCH 1/7] Set admin password in identity-admin relation regardless of how it was generated --- hooks/keystone_hooks.py | 13 +++++++------ hooks/keystone_utils.py | 30 +++++++++++++++++------------- 2 files changed, 24 insertions(+), 19 deletions(-) diff --git a/hooks/keystone_hooks.py b/hooks/keystone_hooks.py index 788fc531..2e1d37b3 100755 --- a/hooks/keystone_hooks.py +++ b/hooks/keystone_hooks.py @@ -44,6 +44,7 @@ from keystone_utils import ( determine_packages, do_openstack_upgrade, ensure_initial_admin, + get_admin_passwd, migrate_database, save_script_rc, synchronize_ca, @@ -53,7 +54,6 @@ from keystone_utils import ( CLUSTER_RES, KEYSTONE_CONF, SSH_USER, - STORED_PASSWD, setup_ipv6 ) @@ -120,6 +120,9 @@ def config_changed(): for unit in relation_list(r_id): identity_changed(relation_id=r_id, remote_unit=unit) + for r_id in relation_ids('identity-admin'): + for unit in relation_list(r_id): + admin_relation_changed(relation_id=r_id) [cluster_joined(rid) for rid in relation_ids('cluster')] @@ -316,7 +319,7 @@ def ha_changed(): @hooks.hook('identity-admin-relation-changed') -def admin_relation_changed(): +def admin_relation_changed(relation_id=None): # TODO: fixup relation_data = { 'service_hostname': unit_get('private-address'), @@ -325,10 +328,8 @@ def admin_relation_changed(): 'service_tenant_name': config('admin-role'), 'service_region': config('region'), } - if os.path.isfile(STORED_PASSWD): - with open(STORED_PASSWD) as f: - relation_data['service_password'] = f.readline().strip('\n') - relation_set(**relation_data) + relation_data['service_password'] = get_admin_passwd() + relation_set(relation_id=relation_id, **relation_data) def configure_https(): diff --git a/hooks/keystone_utils.py b/hooks/keystone_utils.py index 936cd2f8..08db851a 100644 --- a/hooks/keystone_utils.py +++ b/hooks/keystone_utils.py @@ -467,19 +467,7 @@ def grant_role(user, role, tenant): (user, role, tenant)) -def ensure_initial_admin(config): - """ Ensures the minimum admin stuff exists in whatever database we're - using. - This and the helper functions it calls are meant to be idempotent and - run during install as well as during db-changed. This will maintain - the admin tenant, user, role, service entry and endpoint across every - datastore we might use. - TODO: Possibly migrate data from one backend to another after it - changes? - """ - create_tenant("admin") - create_tenant(config("service-tenant")) - +def get_admin_passwd(): passwd = "" if config("admin-password") != "None": passwd = config("admin-password") @@ -492,6 +480,22 @@ def ensure_initial_admin(config): cmd = ['pwgen', '-c', '16', '1'] passwd = str(subprocess.check_output(cmd)).strip() open(STORED_PASSWD, 'w+').writelines("%s\n" % passwd) + return passwd + + +def ensure_initial_admin(config): + """ Ensures the minimum admin stuff exists in whatever database we're + using. + This and the helper functions it calls are meant to be idempotent and + run during install as well as during db-changed. This will maintain + the admin tenant, user, role, service entry and endpoint across every + datastore we might use. + TODO: Possibly migrate data from one backend to another after it + changes? + """ + create_tenant("admin") + create_tenant(config("service-tenant")) + passwd = get_admin_passwd() # User is managed by ldap backend when using ldap identity if not (config('identity-backend') == 'ldap' and config('ldap-readonly')): create_user(config('admin-user'), passwd, tenant='admin') From fbc15500aace1a6fda5c98b7c05e428f0fefa1ef Mon Sep 17 00:00:00 2001 From: Liam Young Date: Fri, 24 Oct 2014 10:47:14 +0000 Subject: [PATCH 2/7] All node should update their admin relation on config-caged incase the password has changed --- hooks/keystone_hooks.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/hooks/keystone_hooks.py b/hooks/keystone_hooks.py index 2e1d37b3..91e83eec 100755 --- a/hooks/keystone_hooks.py +++ b/hooks/keystone_hooks.py @@ -120,10 +120,8 @@ def config_changed(): for unit in relation_list(r_id): identity_changed(relation_id=r_id, remote_unit=unit) - for r_id in relation_ids('identity-admin'): - for unit in relation_list(r_id): - admin_relation_changed(relation_id=r_id) + [admin_relation_changed(rid) for rid in relation_ids('identity-admin')] [cluster_joined(rid) for rid in relation_ids('cluster')] From a22a33e25419e424acf3bf088c2bc2f61da2b9b3 Mon Sep 17 00:00:00 2001 From: Liam Young Date: Fri, 24 Oct 2014 12:52:11 +0000 Subject: [PATCH 3/7] Send correct admin address on identity-admin relation --- hooks/keystone_hooks.py | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/hooks/keystone_hooks.py b/hooks/keystone_hooks.py index 91e83eec..111d2cf5 100755 --- a/hooks/keystone_hooks.py +++ b/hooks/keystone_hooks.py @@ -68,12 +68,16 @@ from charmhelpers.contrib.peerstorage import ( peer_retrieve_by_prefix, peer_echo, ) +from charmhelpers.contrib.openstack.ip import ( + ADMIN, + resolve_address, +) from charmhelpers.contrib.network.ip import ( get_iface_for_address, get_netmask_for_address, get_address_in_network, get_ipv6_addr, - is_ipv6 + is_ipv6, ) from charmhelpers.contrib.openstack.context import ADDRESS_TYPES @@ -167,7 +171,6 @@ def db_changed(): # units acl entry has been added. So, if the db supports passing # a list of permitted units then check if we're in the list. allowed_units = relation_get('allowed_units') - print "allowed_units:" + str(allowed_units) if allowed_units and local_unit() not in allowed_units.split(): log('Allowed_units list provided and this unit not present') return @@ -320,7 +323,7 @@ def ha_changed(): def admin_relation_changed(relation_id=None): # TODO: fixup relation_data = { - 'service_hostname': unit_get('private-address'), + 'service_hostname': resolve_address(ADMIN), 'service_port': config('service-port'), 'service_username': config('admin-user'), 'service_tenant_name': config('admin-role'), From 7bbe871bde26a93b31516977e67d920a3cac1ab4 Mon Sep 17 00:00:00 2001 From: Liam Young Date: Fri, 24 Oct 2014 14:31:02 +0000 Subject: [PATCH 4/7] Store and retrieve admin password from peerdb to enstore its consistent --- hooks/keystone_hooks.py | 1 + hooks/keystone_utils.py | 47 +++++++++++++++++++++++++++-------------- 2 files changed, 32 insertions(+), 16 deletions(-) diff --git a/hooks/keystone_hooks.py b/hooks/keystone_hooks.py index 111d2cf5..18c8c25a 100755 --- a/hooks/keystone_hooks.py +++ b/hooks/keystone_hooks.py @@ -253,6 +253,7 @@ def cluster_changed(): for unit in relation_list(r_id): identity_changed(relation_id=r_id, remote_unit=unit) + [admin_relation_changed(rid) for rid in relation_ids('identity-admin')] @hooks.hook('ha-relation-joined') diff --git a/hooks/keystone_utils.py b/hooks/keystone_utils.py index 08db851a..b7cf77b4 100644 --- a/hooks/keystone_utils.py +++ b/hooks/keystone_utils.py @@ -39,6 +39,7 @@ import charmhelpers.contrib.unison as unison from charmhelpers.core.hookenv import ( config, + is_relation_made, log, relation_get, relation_set, @@ -467,20 +468,33 @@ def grant_role(user, role, tenant): (user, role, tenant)) +def store_admin_passwd(passwd): + open(STORED_PASSWD, 'w+').writelines("%s\n" % passwd) + + def get_admin_passwd(): + if config("admin-password") not in ["None", ""]: + return config("admin-password") passwd = "" - if config("admin-password") != "None": - passwd = config("admin-password") - elif os.path.isfile(STORED_PASSWD): - log("Loading stored passwd from %s" % STORED_PASSWD) - passwd = open(STORED_PASSWD, 'r').readline().strip('\n') - if passwd == "": - log("Generating new passwd for user: %s" % - config("admin-user")) - cmd = ['pwgen', '-c', '16', '1'] - passwd = str(subprocess.check_output(cmd)).strip() - open(STORED_PASSWD, 'w+').writelines("%s\n" % passwd) - return passwd + if eligible_leader(CLUSTER_RES): + if os.path.isfile(STORED_PASSWD): + log("Loading stored passwd from %s" % STORED_PASSWD) + passwd = open(STORED_PASSWD, 'r').readline().strip('\n') + if passwd == "": + log("Generating new passwd for user: %s" % + config("admin-user")) + cmd = ['pwgen', '-c', '16', '1'] + passwd = str(subprocess.check_output(cmd)).strip() + store_admin_passwd(passwd) + if is_relation_made("cluster"): + peer_store("admin_passwd", passwd) + return passwd + else: + if is_relation_made("cluster"): + passwd = peer_retrieve('admin_passwd') + if passwd: + store_admin_passwd(passwd) + return passwd def ensure_initial_admin(config): @@ -495,12 +509,13 @@ def ensure_initial_admin(config): """ create_tenant("admin") create_tenant(config("service-tenant")) - passwd = get_admin_passwd() # User is managed by ldap backend when using ldap identity if not (config('identity-backend') == 'ldap' and config('ldap-readonly')): - create_user(config('admin-user'), passwd, tenant='admin') - update_user_password(config('admin-user'), passwd) - create_role(config('admin-role'), config('admin-user'), 'admin') + passwd = get_admin_passwd() + if passwd: + create_user(config('admin-user'), passwd, tenant='admin') + update_user_password(config('admin-user'), passwd) + create_role(config('admin-role'), config('admin-user'), 'admin') create_service_entry("keystone", "identity", "Keystone Identity Service") for region in config('region').split(): From 5bbf15c3a5f1f7949ac4f3a1a154a3fa41b47104 Mon Sep 17 00:00:00 2001 From: Liam Young Date: Wed, 14 Jan 2015 10:38:56 +0000 Subject: [PATCH 5/7] Fix unit tests --- hooks/keystone_hooks.py | 1 - unit_tests/test_keystone_hooks.py | 18 ++++++++++++------ 2 files changed, 12 insertions(+), 7 deletions(-) diff --git a/hooks/keystone_hooks.py b/hooks/keystone_hooks.py index d858fe06..1c5cd8bd 100755 --- a/hooks/keystone_hooks.py +++ b/hooks/keystone_hooks.py @@ -56,7 +56,6 @@ from keystone_utils import ( CLUSTER_RES, KEYSTONE_CONF, SSH_USER, - STORED_PASSWD, setup_ipv6, send_notifications, ) diff --git a/unit_tests/test_keystone_hooks.py b/unit_tests/test_keystone_hooks.py index 30f72880..4217789e 100644 --- a/unit_tests/test_keystone_hooks.py +++ b/unit_tests/test_keystone_hooks.py @@ -235,6 +235,7 @@ class KeystoneRelationTests(CharmTestCase): relation_id='identity-service:0', remote_unit='unit/0') + @patch.object(hooks, 'admin_relation_changed') @patch.object(hooks, 'cluster_joined') @patch.object(unison, 'ensure_user') @patch.object(unison, 'get_homedir') @@ -243,10 +244,11 @@ class KeystoneRelationTests(CharmTestCase): @patch.object(hooks, 'configure_https') def test_config_changed_no_openstack_upgrade_leader( self, configure_https, identity_changed, - configs, get_homedir, ensure_user, cluster_joined): + configs, get_homedir, ensure_user, cluster_joined, + admin_relation_changed): self.openstack_upgrade_available.return_value = False self.eligible_leader.return_value = True - self.relation_ids.return_value = ['identity-service:0'] + self.relation_ids.return_value = ['dummyid:0'] self.relation_list.return_value = ['unit/0'] hooks.config_changed() @@ -262,8 +264,9 @@ class KeystoneRelationTests(CharmTestCase): self.log.assert_called_with( 'Firing identity_changed hook for all related services.') identity_changed.assert_called_with( - relation_id='identity-service:0', + relation_id='dummyid:0', remote_unit='unit/0') + admin_relation_changed.assert_called_with('dummyid:0') @patch.object(hooks, 'cluster_joined') @patch.object(unison, 'ensure_user') @@ -289,6 +292,7 @@ class KeystoneRelationTests(CharmTestCase): self.assertFalse(self.ensure_initial_admin.called) self.assertFalse(identity_changed.called) + @patch.object(hooks, 'admin_relation_changed') @patch.object(hooks, 'cluster_joined') @patch.object(unison, 'ensure_user') @patch.object(unison, 'get_homedir') @@ -297,10 +301,11 @@ class KeystoneRelationTests(CharmTestCase): @patch.object(hooks, 'configure_https') def test_config_changed_with_openstack_upgrade( self, configure_https, identity_changed, - configs, get_homedir, ensure_user, cluster_joined): + configs, get_homedir, ensure_user, cluster_joined, + admin_relation_changed): self.openstack_upgrade_available.return_value = True self.eligible_leader.return_value = True - self.relation_ids.return_value = ['identity-service:0'] + self.relation_ids.return_value = ['dummyid:0'] self.relation_list.return_value = ['unit/0'] hooks.config_changed() @@ -318,8 +323,9 @@ class KeystoneRelationTests(CharmTestCase): self.log.assert_called_with( 'Firing identity_changed hook for all related services.') identity_changed.assert_called_with( - relation_id='identity-service:0', + relation_id='dummyid:0', remote_unit='unit/0') + admin_relation_changed.assert_called_with('dummyid:0') @patch.object(hooks, 'hashlib') @patch.object(hooks, 'send_notifications') From fb12d7185f40d572d23caa83c4ce3d92ba512735 Mon Sep 17 00:00:00 2001 From: Liam Young Date: Wed, 14 Jan 2015 11:04:13 +0000 Subject: [PATCH 6/7] Tideyup up after feedback from hopem --- hooks/keystone_hooks.py | 9 ++++++--- hooks/keystone_utils.py | 33 ++++++++++++++++++++------------- 2 files changed, 26 insertions(+), 16 deletions(-) diff --git a/hooks/keystone_hooks.py b/hooks/keystone_hooks.py index 1c5cd8bd..31ab4ba5 100755 --- a/hooks/keystone_hooks.py +++ b/hooks/keystone_hooks.py @@ -131,8 +131,10 @@ def config_changed(): identity_changed(relation_id=r_id, remote_unit=unit) - [admin_relation_changed(rid) for rid in relation_ids('identity-admin')] - [cluster_joined(rid) for rid in relation_ids('cluster')] + for rid in relation_ids('identity-admin'): + admin_relation_changed(rid) + for rid in relation_ids('cluster'): + cluster_joined(rid) @hooks.hook('shared-db-relation-joined') @@ -277,7 +279,8 @@ def cluster_changed(): for unit in relation_list(r_id): identity_changed(relation_id=r_id, remote_unit=unit) - [admin_relation_changed(rid) for rid in relation_ids('identity-admin')] + for rid in relation_ids('identity-admin'): + admin_relation_changed(rid) @hooks.hook('ha-relation-joined') diff --git a/hooks/keystone_utils.py b/hooks/keystone_utils.py index 6d4c839b..61309557 100644 --- a/hooks/keystone_utils.py +++ b/hooks/keystone_utils.py @@ -486,32 +486,39 @@ def grant_role(user, role, tenant): def store_admin_passwd(passwd): - open(STORED_PASSWD, 'w+').writelines("%s\n" % passwd) + with open(STORED_PASSWD, 'w+') as fd: + fd.writelines("%s\n" % passwd) def get_admin_passwd(): - if config("admin-password") not in ["None", ""]: - return config("admin-password") - passwd = "" + passwd = config("admin-password") + if passwd not in ["None", ""]: + return passwd + if eligible_leader(CLUSTER_RES): if os.path.isfile(STORED_PASSWD): - log("Loading stored passwd from %s" % STORED_PASSWD) - passwd = open(STORED_PASSWD, 'r').readline().strip('\n') - if passwd == "": + log("Loading stored passwd from %s" % STORED_PASSWD, level=INFO) + with open(STORED_PASSWD, 'r') as fd: + passwd = fd.readline().strip('\n') + + if not passwd: log("Generating new passwd for user: %s" % config("admin-user")) cmd = ['pwgen', '-c', '16', '1'] passwd = str(subprocess.check_output(cmd)).strip() store_admin_passwd(passwd) + if is_relation_made("cluster"): peer_store("admin_passwd", passwd) + return passwd - else: - if is_relation_made("cluster"): - passwd = peer_retrieve('admin_passwd') - if passwd: - store_admin_passwd(passwd) - return passwd + + if is_relation_made("cluster"): + passwd = peer_retrieve('admin_passwd') + if passwd: + store_admin_passwd(passwd) + + return passwd def ensure_initial_admin(config): From cb532ba0687cb39321b0cd51a0ed4ec274f15d52 Mon Sep 17 00:00:00 2001 From: Liam Young Date: Wed, 14 Jan 2015 13:17:50 +0000 Subject: [PATCH 7/7] Added unit tests --- unit_tests/test_keystone_utils.py | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/unit_tests/test_keystone_utils.py b/unit_tests/test_keystone_utils.py index f504b21e..5d564dea 100644 --- a/unit_tests/test_keystone_utils.py +++ b/unit_tests/test_keystone_utils.py @@ -1,4 +1,4 @@ -from mock import patch, call, MagicMock +from mock import patch, call, MagicMock, Mock from test_utils import CharmTestCase import os import manager @@ -35,6 +35,8 @@ TO_PATCH = [ 'relation_get', 'relation_set', 'https', + 'is_relation_made', + 'peer_store', # generic 'apt_update', 'apt_upgrade', @@ -323,3 +325,25 @@ class TestKeystoneUtils(CharmTestCase): settings['trigger'] = '1234' mock_relation_set.assert_called_once_with(relation_id=relation_id, relation_settings=settings) + + def test_get_admin_passwd_pwd_set(self): + self.test_config.set('admin-password', 'supersecret') + self.assertEqual(utils.get_admin_passwd(), 'supersecret') + + @patch('os.path.isfile') + def test_get_admin_passwd_pwd_file_load(self, isfile): + self.test_config.set('admin-password', '') + isfile.return_value = True + with patch('__builtin__.open') as mock_open: + mock_open.return_value.__enter__ = lambda s: s + mock_open.return_value.__exit__ = Mock() + mock_open.return_value.readline.return_value = 'supersecretfilepwd' + self.assertEqual(utils.get_admin_passwd(), 'supersecretfilepwd') + + @patch.object(utils, 'store_admin_passwd') + @patch('os.path.isfile') + def test_get_admin_passwd_genpass(self, isfile, store_admin_passwd): + self.test_config.set('admin-password', '') + isfile.return_value = False + self.subprocess.check_output.return_value = 'supersecretgen' + self.assertEqual(utils.get_admin_passwd(), 'supersecretgen')