From 9d91d0c88533138d64b866bf96cf521ec68b7741 Mon Sep 17 00:00:00 2001 From: Trent Lloyd Date: Sat, 29 Oct 2016 00:47:06 +0800 Subject: [PATCH] Fix broken admin user on v3 API HA deployments The domain_id UUID for the admin_domain is referenced in the policy.json file to give admin rights to the cloud admin user. The domain_id was previously stored in a file on the leader node which meant it was not available to render into policy.json on the non-leader nodes and rendered the default string admin_domain_id instead. This caused the admin user to generally not work at all as only authentication requests reaching the leader unit would succeed. Store and retrieve the values using leadership settings instead so that it is available on all units. Closes-Bug: 1637453 --- hooks/keystone_context.py | 6 ++-- hooks/keystone_hooks.py | 5 +++ hooks/keystone_utils.py | 69 +++++++++++++++++++++++---------------- unit_tests/test_keystone_utils.py | 51 ++++++++++++++--------------- 4 files changed, 74 insertions(+), 57 deletions(-) diff --git a/hooks/keystone_context.py b/hooks/keystone_context.py index 0d65ff8..bcbd03d 100644 --- a/hooks/keystone_context.py +++ b/hooks/keystone_context.py @@ -207,7 +207,7 @@ class KeystoneContext(context.OSContextGenerator): from keystone_utils import ( api_port, set_admin_token, endpoint_url, resolve_address, PUBLIC, ADMIN, PKI_CERTS_DIR, ensure_pki_cert_paths, - get_admin_domain_id, get_default_domain_id + get_stored_domain_id, ADMIN_DOMAIN, DEFAULT_DOMAIN, ) ctxt = {} ctxt['token'] = set_admin_token(config('admin-token')) @@ -215,10 +215,10 @@ class KeystoneContext(context.OSContextGenerator): ctxt['admin_role'] = config('admin-role') if ctxt['api_version'] > 2: ctxt['admin_domain_id'] = ( - get_admin_domain_id() or 'admin_domain_id') + get_stored_domain_id(ADMIN_DOMAIN) or 'admin_domain_id') # default is the default for default_domain_id ctxt['default_domain_id'] = ( - get_default_domain_id() or 'default') + get_stored_domain_id(DEFAULT_DOMAIN) or 'default') ctxt['admin_port'] = determine_api_port(api_port('keystone-admin'), singlenode_mode=True) ctxt['public_port'] = determine_api_port(api_port('keystone-public'), diff --git a/hooks/keystone_hooks.py b/hooks/keystone_hooks.py index 89b4056..1f0988b 100755 --- a/hooks/keystone_hooks.py +++ b/hooks/keystone_hooks.py @@ -78,6 +78,7 @@ from keystone_utils import ( get_admin_passwd, git_install, migrate_database, + migrate_stored_domain_id, save_script_rc, synchronize_ca_if_changed, register_configs, @@ -730,6 +731,10 @@ def upgrade_charm(): ensure_ssl_dirs() + if is_elected_leader(CLUSTER_RES): + migrate_stored_domain_id(domain_name="default") + migrate_stored_domain_id(domain_name="admin_domain") + CONFIGS.write_all() # See LP bug 1519035 diff --git a/hooks/keystone_utils.py b/hooks/keystone_utils.py index 383ce56..3914fc9 100644 --- a/hooks/keystone_utils.py +++ b/hooks/keystone_utils.py @@ -92,6 +92,8 @@ from charmhelpers.core.hookenv import ( charm_dir, config, is_relation_made, + leader_get, + leader_set, log, local_unit, relation_get, @@ -883,16 +885,44 @@ def store_data(backing_file, data): fd.writelines("%s\n" % data) -def store_admin_passwd(passwd): - store_data(STORED_PASSWD, passwd) +def store_domain_id(domain_name, domain_id): + peer_key = "{}_domain_id".format(domain_name) + return leader_set({peer_key: domain_id}) + + +def migrate_stored_domain_id(domain_name): + # Previous logic stored data in the inconsistent paths + # keystone.admin_domain_id for admin_domain and keystone.default_domain_id + # for default so we override the path in the admin_domain case + backing_file = None + if domain_name == ADMIN_DOMAIN: + backing_file = "/var/lib/keystone/keystone.admin_domain_id" + else: + backing_file = "/var/lib/keystone/keystone.{}_domain_id" \ + .format(domain_name) + + if backing_file is not None: + domain_id = None + if os.path.isfile(backing_file): + with open(backing_file, 'r') as fd: + domain_id = fd.readline().strip('\n') + if domain_id is not None: + if store_domain_id(domain_name=domain_name, domain_id=domain_id): + os.unlink(backing_file) + return True + + return False -def store_admin_domain_id(domain_id): - store_data(STORED_ADMIN_DOMAIN_ID, domain_id) +def get_stored_domain_id(domain_name): + peer_key = "{}_domain_id".format(domain_name) + domain_id = leader_get(attribute=peer_key) + return domain_id -def store_default_domain_id(domain_id): - store_data(STORED_DEFAULT_DOMAIN_ID, domain_id) + +def store_admin_passwd(passwd): + store_data(STORED_PASSWD, passwd) def get_admin_passwd(): @@ -959,9 +989,9 @@ def ensure_initial_admin(config): """ if get_api_version() > 2: default_domain_id = create_or_show_domain(DEFAULT_DOMAIN) - store_default_domain_id(default_domain_id) + store_domain_id(DEFAULT_DOMAIN, default_domain_id) admin_domain_id = create_or_show_domain(ADMIN_DOMAIN) - store_admin_domain_id(admin_domain_id) + store_domain_id(ADMIN_DOMAIN, admin_domain_id) create_tenant("admin") create_tenant(config("service-tenant")) # User is managed by ldap backend when using ldap identity @@ -1683,7 +1713,8 @@ def add_service_to_keystone(relation_id=None, remote_unit=None): relation_data["service_port"] = config('service-port') relation_data["region"] = config('region') relation_data["api_version"] = get_api_version() - relation_data["admin_domain_id"] = get_admin_domain_id() + relation_data["admin_domain_id"] = \ + get_stored_domain_id(ADMIN_DOMAIN) # Get and pass CA bundle settings relation_data.update(get_ssl_ca_settings()) @@ -1809,7 +1840,7 @@ def add_service_to_keystone(relation_id=None, remote_unit=None): "auth_protocol": protocol, "service_protocol": protocol, "api_version": get_api_version(), - "admin_domain_id": get_admin_domain_id(), + "admin_domain_id": get_stored_domain_id(ADMIN_DOMAIN), } # generate or get a new cert/key for service if set to manage certs. @@ -2290,24 +2321,6 @@ def assess_status_func(configs): ports=determine_ports()) -def get_file_stored_domain_id(backing_file): - domain_id = None - if os.path.isfile(backing_file): - log("Loading stored domain id from {}".format(backing_file), - level=INFO) - with open(backing_file, 'r') as fd: - domain_id = fd.readline().strip('\n') - return domain_id - - -def get_admin_domain_id(): - return get_file_stored_domain_id(STORED_ADMIN_DOMAIN_ID) - - -def get_default_domain_id(): - return get_file_stored_domain_id(STORED_DEFAULT_DOMAIN_ID) - - def pause_unit_helper(configs): """Helper function to pause a unit, and then call assess_status(...) in effect, so that the status is correctly updated. diff --git a/unit_tests/test_keystone_utils.py b/unit_tests/test_keystone_utils.py index 8fba754..895c0cf 100644 --- a/unit_tests/test_keystone_utils.py +++ b/unit_tests/test_keystone_utils.py @@ -204,16 +204,16 @@ class TestKeystoneUtils(CharmTestCase): self.subprocess.check_output.assert_called_with(cmd) self.service_start.assert_called_with('keystone') - @patch.object(utils, 'get_admin_domain_id') + @patch.object(utils, 'get_stored_domain_id') @patch.object(utils, 'get_api_version') @patch.object(utils, 'get_manager') @patch.object(utils, 'resolve_address') @patch.object(utils, 'b64encode') def test_add_service_to_keystone_clustered_https_none_values( self, b64encode, _resolve_address, _get_manager, - _get_api_version, _get_admin_domain_id): + _get_api_version, _get_stored_domain_id): _get_api_version.return_value = 2 - _get_admin_domain_id.return_value = None + _get_stored_domain_id.return_value = None relation_id = 'identity-service:0' remote_unit = 'unit/0' _resolve_address.return_value = '10.10.10.10' @@ -252,7 +252,7 @@ class TestKeystoneUtils(CharmTestCase): **relation_data) @patch.object(utils, 'get_api_version') - @patch.object(utils, 'get_admin_domain_id') + @patch.object(utils, 'get_stored_domain_id') @patch.object(utils, 'create_user') @patch.object(utils, 'resolve_address') @patch.object(utils, 'ensure_valid_service') @@ -260,9 +260,9 @@ class TestKeystoneUtils(CharmTestCase): @patch.object(utils, 'get_manager') def test_add_service_to_keystone_no_clustered_no_https_complete_values( self, KeystoneManager, add_endpoint, ensure_valid_service, - _resolve_address, create_user, get_admin_domain_id, + _resolve_address, create_user, get_stored_domain_id, get_api_version): - get_admin_domain_id.return_value = None + get_stored_domain_id.return_value = None get_api_version.return_value = 2 relation_id = 'identity-service:0' remote_unit = 'unit/0' @@ -334,9 +334,10 @@ class TestKeystoneUtils(CharmTestCase): @patch.object(utils, 'ensure_valid_service') @patch.object(utils, 'add_endpoint') @patch.object(utils, 'get_manager') + @patch.object(utils, 'get_stored_domain_id') def test_add_service_to_keystone_nosubset( - self, KeystoneManager, add_endpoint, ensure_valid_service, - ip_config): + self, mock_get_stored_domain_id, KeystoneManager, add_endpoint, + ensure_valid_service, ip_config): relation_id = 'identity-service:0' remote_unit = 'unit/0' @@ -815,11 +816,11 @@ class TestKeystoneUtils(CharmTestCase): utils.delete_service_entry('bob', 'bill') mock_keystone.api.services.delete.assert_called_with('sid1') + @patch.object(utils, 'store_domain_id') + @patch('os.unlink') @patch('os.path.isfile') - def test_get_file_stored_domain_id(self, isfile_mock): - isfile_mock.return_value = False - x = utils.get_file_stored_domain_id('/a/file') - assert x is None + def test_migrate_stored_domain_id(self, isfile_mock, mock_unlink, + mock_store_domain_id): from sys import version_info if version_info.major == 2: import __builtin__ as builtins @@ -829,20 +830,18 @@ class TestKeystoneUtils(CharmTestCase): with patch.object(builtins, 'open', mock_open( read_data="some_data\n")): isfile_mock.return_value = True - x = utils.get_file_stored_domain_id('/a/file') - self.assertEquals(x, 'some_data') - - @patch.object(utils, 'get_file_stored_domain_id') - def test_get_admin_domain_id(self, mock_get_file_stored_domain_id): - utils.get_admin_domain_id() - mock_get_file_stored_domain_id.assert_called_with( - '/var/lib/keystone/keystone.admin_domain_id') - - @patch.object(utils, 'get_file_stored_domain_id') - def test_get_default_domain_id(self, mock_get_file_stored_domain_id): - utils.get_default_domain_id() - mock_get_file_stored_domain_id.assert_called_with( - '/var/lib/keystone/keystone.default_domain_id') + self.assertTrue(utils.migrate_stored_domain_id(utils.ADMIN_DOMAIN)) + mock_store_domain_id.assert_called_with( + domain_name=utils.ADMIN_DOMAIN, domain_id='some_data') + mock_unlink.assert_called_with( + '/var/lib/keystone/keystone.admin_domain_id') + + @patch.object(utils, 'leader_get') + def test_get_stored_domain_id(self, mock_leader_get): + mock_leader_get.return_value = 'some_data' + domain_id = utils.get_stored_domain_id(utils.ADMIN_DOMAIN) + self.assertEqual(domain_id, 'some_data') + mock_leader_get.assert_called_with(attribute='admin_domain_domain_id') def test_assess_status(self): with patch.object(utils, 'assess_status_func') as asf: -- 2.9.3