From 3f58b2861b70473e49d8b53d09d13b34e77a133f Mon Sep 17 00:00:00 2001 From: Frode Nordahl Date: Fri, 27 Jul 2018 14:11:22 +0200 Subject: [PATCH] Use leader storage for passwords There are no relation-level concerns for these values. Any pre-existing deployments with recent charms (as in released 2015 or later) will have already migrated the peer storage to leader storage, so this change can build on that work directly. Ref: https://github.com/juju/charm-helpers/blame/master/charmhelpers/contrib/peerstorage/__init__.py Change-Id: I85d1746bdf3e9d3e1ff514e6dd2b4c565dee4dfc Closes-Bug: #1783943 --- hooks/keystone_utils.py | 40 ++++++++++++------------------- unit_tests/test_keystone_hooks.py | 8 ++----- unit_tests/test_keystone_utils.py | 26 +++++++++----------- 3 files changed, 28 insertions(+), 46 deletions(-) diff --git a/hooks/keystone_utils.py b/hooks/keystone_utils.py index 9ffe9d25..34672425 100644 --- a/hooks/keystone_utils.py +++ b/hooks/keystone_utils.py @@ -72,6 +72,7 @@ from charmhelpers.core.decorators import ( from charmhelpers.core.hookenv import ( config, + is_leader, leader_get, leader_set, log, @@ -105,8 +106,6 @@ from charmhelpers.core.host import ( from charmhelpers.contrib.peerstorage import ( peer_store_and_set, - peer_store, - peer_retrieve, ) import keystone_context @@ -1035,18 +1034,12 @@ def store_data(backing_file, data): def get_admin_passwd(user=None): passwd = config("admin-password") if passwd and passwd.lower() != "none": - # Previous charm versions did not always store on leader setting so do - # this now to avoid an initial update on install/upgrade - if (is_elected_leader(CLUSTER_RES) and - peer_retrieve('{}_passwd'.format(user)) is None): - set_admin_passwd(passwd, user=user) - return passwd _migrate_admin_password() - passwd = peer_retrieve('{}_passwd'.format(user)) + passwd = leader_get('{}_passwd'.format(user)) - if not passwd and is_elected_leader(CLUSTER_RES): + if not passwd and is_leader(): log("Generating new passwd for user: %s" % config("admin-user")) cmd = ['pwgen', '-c', '16', '1'] @@ -1059,7 +1052,7 @@ def set_admin_passwd(passwd, user=None): if user is None: user = 'admin' - peer_store('{}_passwd'.format(user), passwd) + leader_set({'{}_passwd'.format(user): passwd}) def get_api_version(): @@ -1228,29 +1221,28 @@ def load_stored_passwords(path=SERVICE_PASSWD_PATH): def _migrate_admin_password(): - """Migrate on-disk admin passwords to peer storage""" - if os.path.exists(STORED_PASSWD): - log('Migrating on-disk stored passwords to peer storage') + """Migrate on-disk admin passwords to leader storage""" + if is_leader() and os.path.exists(STORED_PASSWD): + log('Migrating on-disk stored passwords to leader storage') with open(STORED_PASSWD) as fd: - peer_store("admin_passwd", fd.readline().strip('\n')) + leader_set({"admin_passwd": fd.readline().strip('\n')}) os.unlink(STORED_PASSWD) def _migrate_service_passwords(): - """Migrate on-disk service passwords to peer storage""" - if os.path.exists(SERVICE_PASSWD_PATH): - log('Migrating on-disk stored passwords to peer storage') + """Migrate on-disk service passwords to leader storage""" + if is_leader() and os.path.exists(SERVICE_PASSWD_PATH): + log('Migrating on-disk stored passwords to leader storage') creds = load_stored_passwords() for k, v in creds.iteritems(): - peer_store(key="{}_passwd".format(k), value=v) + leader_set({"{}_passwd".format(k): v}) os.unlink(SERVICE_PASSWD_PATH) def get_service_password(service_username): _migrate_service_passwords() - peer_key = "{}_passwd".format(service_username) - passwd = peer_retrieve(peer_key) + passwd = leader_get("{}_passwd".format(service_username)) if passwd is None: passwd = pwgen(length=64) @@ -1258,13 +1250,11 @@ def get_service_password(service_username): def set_service_password(passwd, user): - peer_key = "{}_passwd".format(user) - peer_store(key=peer_key, value=passwd) + leader_set({"{}_passwd".format(user): passwd}) def is_password_changed(username, passwd): - peer_key = "{}_passwd".format(username) - _passwd = peer_retrieve(peer_key) + _passwd = leader_get("{}_passwd".format(username)) return (_passwd is None or passwd != _passwd) diff --git a/unit_tests/test_keystone_hooks.py b/unit_tests/test_keystone_hooks.py index 4c518675..720c37d4 100644 --- a/unit_tests/test_keystone_hooks.py +++ b/unit_tests/test_keystone_hooks.py @@ -631,12 +631,10 @@ class KeystoneRelationTests(CharmTestCase): self.assertTrue(self.update_dns_ha_resource_params.called) self.relation_set.assert_called_with(**args) - @patch.object(utils, 'peer_retrieve') @patch('keystone_utils.log') @patch.object(hooks, 'CONFIGS') def test_ha_relation_changed_not_clustered_not_leader(self, configs, - mock_log, - mock_peer_retrieve): + mock_log): self.relation_get.return_value = False hooks.ha_changed() @@ -842,7 +840,6 @@ class KeystoneRelationTests(CharmTestCase): # Still updates relations self.assertTrue(self.relation_ids.called) - @patch.object(utils, 'peer_retrieve') @patch.object(hooks, 'update_all_identity_relation_units') @patch.object(utils, 'os_release') @patch('keystone_utils.log') @@ -850,11 +847,10 @@ class KeystoneRelationTests(CharmTestCase): def test_upgrade_charm_not_leader(self, mock_relation_ids, mock_log, - os_release, update, mock_peer_retrieve): + os_release, update): os_release.return_value = 'havana' self.filter_installed_packages.return_value = [] - mock_peer_retrieve.return_value = 'true' self.is_elected_leader.return_value = False hooks.upgrade_charm() self.assertTrue(self.apt_install.called) diff --git a/unit_tests/test_keystone_utils.py b/unit_tests/test_keystone_utils.py index bd210c59..86b0e346 100644 --- a/unit_tests/test_keystone_utils.py +++ b/unit_tests/test_keystone_utils.py @@ -60,7 +60,6 @@ TO_PATCH = [ 'local_unit', 'related_units', 'https', - 'peer_store', 'mkdir', 'write_file', # generic @@ -291,6 +290,7 @@ class TestKeystoneUtils(CharmTestCase): self.peer_store_and_set.assert_called_with(relation_id=relation_id, **relation_data) + @patch.object(utils, 'leader_set') @patch.object(utils, 'leader_get') @patch.object(utils, 'get_api_version') @patch.object(utils, 'create_user') @@ -301,7 +301,7 @@ class TestKeystoneUtils(CharmTestCase): def test_add_service_to_keystone_no_clustered_no_https_complete_values( self, KeystoneManager, add_endpoint, ensure_valid_service, _resolve_address, create_user, get_api_version, leader_get, - test_api_version=2): + leader_set, test_api_version=2): get_api_version.return_value = test_api_version leader_get.return_value = None relation_id = 'identity-service:0' @@ -395,6 +395,8 @@ class TestKeystoneUtils(CharmTestCase): test_add_service_to_keystone_no_clustered_no_https_complete_values( test_api_version=3) + @patch.object(utils, 'leader_set') + @patch.object(utils, 'is_leader') @patch.object(utils, 'leader_get') @patch('charmhelpers.contrib.openstack.ip.config') @patch.object(utils, 'ensure_valid_service') @@ -402,7 +404,7 @@ class TestKeystoneUtils(CharmTestCase): @patch.object(utils, 'get_manager') def test_add_service_to_keystone_nosubset( self, KeystoneManager, add_endpoint, ensure_valid_service, - ip_config, leader_get): + ip_config, leader_get, is_leader, leader_set): relation_id = 'identity-service:0' remote_unit = 'unit/0' @@ -666,22 +668,16 @@ class TestKeystoneUtils(CharmTestCase): mock_relation_set.assert_called_once_with(relation_id=relation_id, relation_settings=settings) - @patch.object(utils, 'is_elected_leader') - @patch.object(utils, 'peer_retrieve') - @patch.object(utils, 'peer_store') - def test_get_admin_passwd_pwd_set(self, mock_peer_store, - mock_peer_retrieve, - mock_is_elected_leader): - mock_peer_retrieve.return_value = None + def test_get_admin_passwd_pwd_set(self): self.test_config.set('admin-password', 'supersecret') - mock_is_elected_leader.return_value = True self.assertEqual(utils.get_admin_passwd(), 'supersecret') - mock_peer_store.assert_called_once_with('admin_passwd', 'supersecret') - @patch.object(utils, 'peer_retrieve') + @patch.object(utils, 'is_leader') + @patch.object(utils, 'leader_get') @patch('os.path.isfile') - def test_get_admin_passwd_genpass(self, isfile, peer_retrieve): - peer_retrieve.return_value = 'supersecretgen' + def test_get_admin_passwd_genpass(self, isfile, leader_get, is_leader): + is_leader.return_value = True + leader_get.return_value = 'supersecretgen' self.test_config.set('admin-password', '') isfile.return_value = False self.subprocess.check_output.return_value = 'supersecretgen'