Replace local storage of domain UUIDs with leader storage

Currently the Keystone leader charm creates new domains and stores
the UUIDs locally on disk. This approach predates charm relation-/
leader- storage, is error prone, and causes problems in HA setups.

Move to leader storage and remove old interfaces. There is no need
to migrate the on-disk stored data as it is read from the deployment
and stored as a part of the upgrade process.

Do not set default values for service_tenant_id, admin_domain_id and
default_domain_id. This will cause context to be incomplete on peer
units until the values are actually available.

Change functional tests to run on Keystone cluster to verify contents of
configuration and operation of services in clustered environment.

Closes-Bug: 1637453
Change-Id: Id0eaf7bfceead627cc691e9b52dd889d60c05fa9

hooks/keystone_context.py

@@ -207,23 +207,20 @@ class KeystoneContext(context.OSContextGenerator):
     def __call__(self):
         from keystone_utils import (
             api_port, set_admin_token, endpoint_url, resolve_address,
-            PUBLIC, ADMIN, PKI_CERTS_DIR, ensure_pki_cert_paths,
+            PUBLIC, ADMIN, PKI_CERTS_DIR, ensure_pki_cert_paths, ADMIN_DOMAIN,
-            get_admin_domain_id, get_default_domain_id, ADMIN_DOMAIN,
         )
         ctxt = {}
         ctxt['token'] = set_admin_token(config('admin-token'))
         ctxt['api_version'] = int(config('preferred-api-version'))
         ctxt['admin_role'] = config('admin-role')
         if ctxt['api_version'] > 2:
-            ctxt['service_tenant_id'] = (
+            ctxt['service_tenant_id'] = \
-                leader_get(attribute='service_tenant_id') or
+                leader_get(attribute='service_tenant_id')
-                'service_tenant_id')
             ctxt['admin_domain_name'] = ADMIN_DOMAIN
-            ctxt['admin_domain_id'] = (
+            ctxt['admin_domain_id'] = \
-                get_admin_domain_id() or 'admin_domain_id')
+                leader_get(attribute='admin_domain_id')
-            # default is the default for default_domain_id
+            ctxt['default_domain_id'] = \
-            ctxt['default_domain_id'] = (
+                leader_get(attribute='default_domain_id')
-                get_default_domain_id() 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'),

hooks/keystone_utils.py

@@ -92,6 +92,7 @@ from charmhelpers.core.hookenv import (
     charm_dir,
     config,
     is_relation_made,
+    leader_get,
     leader_set,
     log,
     local_unit,
@@ -917,14 +918,6 @@ def store_admin_passwd(passwd):
     store_data(STORED_PASSWD, passwd)
 
 
-def store_admin_domain_id(domain_id):
-    store_data(STORED_ADMIN_DOMAIN_ID, domain_id)
-
-
-def store_default_domain_id(domain_id):
-    store_data(STORED_DEFAULT_DOMAIN_ID, domain_id)
-
-
 def get_admin_passwd():
     passwd = config("admin-password")
     if passwd and passwd.lower() != "none":
@@ -990,9 +983,9 @@ def ensure_initial_admin(config):
         if get_api_version() > 2:
             manager = get_manager()
             default_domain_id = create_or_show_domain(DEFAULT_DOMAIN)
-            store_default_domain_id(default_domain_id)
+            leader_set({'default_domain_id': default_domain_id})
             admin_domain_id = create_or_show_domain(ADMIN_DOMAIN)
-            store_admin_domain_id(admin_domain_id)
+            leader_set({'admin_domain_id': admin_domain_id})
             create_or_show_domain(SERVICE_DOMAIN)
             create_tenant("admin", ADMIN_DOMAIN)
             create_tenant(config("service-tenant"), SERVICE_DOMAIN)
@@ -1748,7 +1741,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"] = leader_get(
+                attribute='admin_domain_id')
             # Get and pass CA bundle settings
             relation_data.update(get_ssl_ca_settings())
 
@@ -1871,7 +1865,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": leader_get(attribute='admin_domain_id'),
     }
 
     # generate or get a new cert/key for service if set to manage certs.
@@ -2363,14 +2357,6 @@ def get_file_stored_domain_id(backing_file):
     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.

tests/basic_deployment.py

@@ -19,6 +19,7 @@ Basic keystone amulet functional tests.
 """
 
 import amulet
+import json
 import os
 import yaml
 
@@ -46,6 +47,12 @@ class KeystoneBasicDeployment(OpenStackAmuletDeployment):
         """Deploy the entire test environment."""
         super(KeystoneBasicDeployment, self).__init__(series, openstack,
                                                       source, stable)
+        if self.is_liberty_or_newer():
+            self.keystone_num_units = 3
+        else:
+            # issues with starting haproxy when clustered on trusty with
+            # icehouse and kilo. See LP #1648396
+            self.keystone_num_units = 1
         self.keystone_api_version = 2
         self.git = git
         self._add_services()
@@ -65,8 +72,9 @@ class KeystoneBasicDeployment(OpenStackAmuletDeployment):
             services = ("apache2", "haproxy")
         else:
             services = ("keystone-all", "apache2", "haproxy")
-        u.get_unit_process_ids(
+        for unit in self.keystone_sentries:
-            {self.keystone_sentry: services}, expect_success=should_run)
+            u.get_unit_process_ids(
+                {unit: services}, expect_success=should_run)
 
     def _add_services(self):
         """Add services
@@ -75,7 +83,7 @@ class KeystoneBasicDeployment(OpenStackAmuletDeployment):
            and the rest of the service are from lp branches that are
            compatible with the local charm (e.g. stable or next).
            """
-        this_service = {'name': 'keystone'}
+        this_service = {'name': 'keystone', 'units': self.keystone_num_units}
         other_services = [
             {'name': 'percona-cluster', 'constraints': {'mem': '3072M'}},
             {'name': 'rabbitmq-server'},  # satisfy wrkload stat
@@ -151,32 +159,47 @@ class KeystoneBasicDeployment(OpenStackAmuletDeployment):
         u.log.debug('Setting preferred-api-version={}'.format(api_version))
         self.d.configure('keystone', set_alternate)
         self.keystone_api_version = api_version
-        client = self.get_keystone_client(api_version=api_version)
+        for i in range(0, self.keystone_num_units):
-        # List an artefact that needs authorisation to check admin user
+            rel = self.keystone_sentries[i].relation('identity-service',
-        # has been setup. If that is still in progess
+                                                     'cinder:identity-service')
-        # keystoneclient.exceptions.Unauthorized will be thrown and caught by
+            u.log.debug('keystone unit {} relation data: {}'.format(i, rel))
-        # @retry_on_exception
+            if rel['api_version'] != str(api_version):
+                raise Exception("api_version not propagated through relation"
+                                " data yet ('{}' != '{}')."
+                                "".format(rel['api_version'], api_version))
+            client = self.get_keystone_client(api_version=api_version,
+                                              keystone_ip=rel[
+                                                  'private-address'])
+            # List an artefact that needs authorisation to check admin user
+            # has been setup on each Keystone unit. If that is still in progess
+            # keystoneclient.exceptions.Unauthorized will be thrown and caught
+            # by @retry_on_exception
+            if api_version == 2:
+                client.tenants.list()
+            else:
+                client.projects.list()
+        # Success if we get here, get and store client.
         if api_version == 2:
-            client.tenants.list()
             self.keystone_v2 = self.get_keystone_client(api_version=2)
         else:
-            client.projects.list()
             self.keystone_v3 = self.get_keystone_client(api_version=3)
 
-    def get_keystone_client(self, api_version=None):
+    def get_keystone_client(self, api_version=None, keystone_ip=None):
+        if keystone_ip is None:
+            keystone_ip = self.keystone_ip
         if api_version == 2:
-            return u.authenticate_keystone_admin(self.keystone_sentry,
+            return u.authenticate_keystone_admin(self.keystone_sentries[0],
                                                  user='admin',
                                                  password='openstack',
                                                  tenant='admin',
                                                  api_version=api_version,
-                                                 keystone_ip=self.keystone_ip)
+                                                 keystone_ip=keystone_ip)
         else:
-            return u.authenticate_keystone_admin(self.keystone_sentry,
+            return u.authenticate_keystone_admin(self.keystone_sentries[0],
                                                  user='admin',
                                                  password='openstack',
                                                  api_version=api_version,
-                                                 keystone_ip=self.keystone_ip)
+                                                 keystone_ip=keystone_ip)
 
     def create_users_v2(self):
         # Create a demo tenant/role/user
@@ -244,13 +267,15 @@ class KeystoneBasicDeployment(OpenStackAmuletDeployment):
         """Perform final initialization before tests get run."""
         # Access the sentries for inspecting service units
         self.pxc_sentry = self.d.sentry['percona-cluster'][0]
-        self.keystone_sentry = self.d.sentry['keystone'][0]
+        self.keystone_sentries = []
+        for i in range(0, self.keystone_num_units):
+            self.keystone_sentries.append(self.d.sentry['keystone'][i])
         self.cinder_sentry = self.d.sentry['cinder'][0]
         u.log.debug('openstack release val: {}'.format(
             self._get_openstack_release()))
         u.log.debug('openstack release str: {}'.format(
             self._get_openstack_release_string()))
-        self.keystone_ip = self.keystone_sentry.relation(
+        self.keystone_ip = self.keystone_sentries[0].relation(
             'shared-db',
             'percona-cluster:shared-db')['private-address']
         self.set_api_version(2)
@@ -263,15 +288,15 @@ class KeystoneBasicDeployment(OpenStackAmuletDeployment):
         """Verify the expected services are running on the corresponding
            service units."""
         services = {
-            self.keystone_sentry: ['keystone'],
             self.cinder_sentry: ['cinder-api',
                                  'cinder-scheduler',
                                  'cinder-volume']
         }
         if self.is_liberty_or_newer():
-            services[self.keystone_sentry] = ['apache2']
+            for i in range(0, self.keystone_num_units):
+                services.update({self.keystone_sentries[i]: ['apache2']})
         else:
-            services[self.keystone_sentry] = ['keystone']
+            services.update({self.keystone_sentries[0]: ['keystone']})
         ret = u.validate_services_by_name(services)
         if ret:
             amulet.raise_status(amulet.FAIL, msg=ret)
@@ -482,7 +507,6 @@ class KeystoneBasicDeployment(OpenStackAmuletDeployment):
     def test_200_keystone_mysql_shared_db_relation(self):
         """Verify the keystone shared-db relation data"""
         u.log.debug('Checking keystone to mysql db relation data...')
-        unit = self.keystone_sentry
         relation = ['shared-db', 'percona-cluster:shared-db']
         expected = {
             'username': 'keystone',
@@ -490,10 +514,11 @@ class KeystoneBasicDeployment(OpenStackAmuletDeployment):
             'hostname': u.valid_ip,
             'database': 'keystone'
         }
-        ret = u.validate_relation_data(unit, relation, expected)
+        for unit in self.keystone_sentries:
-        if ret:
+            ret = u.validate_relation_data(unit, relation, expected)
-            message = u.relation_error('keystone shared-db', ret)
+            if ret:
-            amulet.raise_status(amulet.FAIL, msg=message)
+                message = u.relation_error('keystone shared-db', ret)
+                amulet.raise_status(amulet.FAIL, msg=message)
 
     def test_201_mysql_keystone_shared_db_relation(self):
         """Verify the mysql shared-db relation data"""
@@ -513,7 +538,6 @@ class KeystoneBasicDeployment(OpenStackAmuletDeployment):
     def test_202_keystone_cinder_identity_service_relation(self):
         """Verify the keystone identity-service relation data"""
         u.log.debug('Checking keystone to cinder id relation data...')
-        unit = self.keystone_sentry
         relation = ['identity-service', 'cinder:identity-service']
         expected = {
             'service_protocol': 'http',
@@ -529,10 +553,11 @@ class KeystoneBasicDeployment(OpenStackAmuletDeployment):
             'service_tenant_id': u.not_null,
             'service_host': u.valid_ip
         }
-        ret = u.validate_relation_data(unit, relation, expected)
+        for unit in self.keystone_sentries:
-        if ret:
+            ret = u.validate_relation_data(unit, relation, expected)
-            message = u.relation_error('keystone identity-service', ret)
+            if ret:
-            amulet.raise_status(amulet.FAIL, msg=message)
+                message = u.relation_error('keystone identity-service', ret)
+                amulet.raise_status(amulet.FAIL, msg=message)
 
     def test_203_cinder_keystone_identity_service_relation(self):
         """Verify the cinder identity-service relation data"""
@@ -561,10 +586,10 @@ class KeystoneBasicDeployment(OpenStackAmuletDeployment):
         """Verify the data in the keystone config file,
            comparing some of the variables vs relation data."""
         u.log.debug('Checking keystone config file...')
-        unit = self.keystone_sentry
         conf = '/etc/keystone/keystone.conf'
-        ks_ci_rel = unit.relation('identity-service',
+        ks_ci_rel = self.keystone_sentries[0].relation(
-                                  'cinder:identity-service')
+            'identity-service',
+            'cinder:identity-service')
         my_ks_rel = self.pxc_sentry.relation('shared-db',
                                              'keystone:shared-db')
         db_uri = "mysql://{}:{}@{}/{}".format('keystone',
@@ -612,16 +637,62 @@ class KeystoneBasicDeployment(OpenStackAmuletDeployment):
                 'bind_host': '0.0.0.0',
             })
 
-        for section, pairs in expected.iteritems():
+        for unit in self.keystone_sentries:
-            ret = u.validate_config_data(unit, conf, section, pairs)
+            for section, pairs in expected.iteritems():
+                ret = u.validate_config_data(unit, conf, section, pairs)
+                if ret:
+                    message = "keystone config error: {}".format(ret)
+                    amulet.raise_status(amulet.FAIL, msg=message)
+
+    def test_301_keystone_default_policy(self):
+        """Verify the data in the keystone policy.json file,
+           comparing some of the variables vs relation data."""
+        if not self.is_liberty_or_newer():
+            return
+        u.log.debug('Checking keystone v3 policy.json file')
+        self.set_api_version(3)
+        conf = '/etc/keystone/policy.json'
+        ks_ci_rel = self.keystone_sentries[0].relation(
+            'identity-service',
+            'cinder:identity-service')
+        if self._get_openstack_release() >= self.trusty_mitaka:
+            expected = {
+                'admin_required': 'role:Admin',
+                'cloud_admin':
+                    'rule:admin_required and '
+                    '(token.is_admin_project:True or '
+                    'domain_id:{admin_domain_id})'.format(
+                        admin_domain_id=ks_ci_rel['admin_domain_id']),
+                'service_role':
+                    'role:service '
+                    'and project_id:{service_tenant_id}'.format(
+                        service_tenant_id=ks_ci_rel['service_tenant_id']),
+                'identity:list_projects':
+                    'rule:cloud_admin or '
+                    'rule:admin_and_matching_domain_id or '
+                    'rule:service_role',
+            }
+        else:
+            expected = {
+                'admin_required': 'role:Admin',
+                'cloud_admin':
+                    'rule:admin_required and '
+                    'domain_id:{admin_domain_id}'.format(
+                        admin_domain_id=ks_ci_rel['admin_domain_id']),
+            }
+
+        for unit in self.keystone_sentries:
+            data = json.loads(unit.file_contents(conf))
+            ret = u._validate_dict_data(expected, data)
             if ret:
-                message = "keystone config error: {}".format(ret)
+                message = "keystone policy.json error: {}".format(ret)
                 amulet.raise_status(amulet.FAIL, msg=message)
 
+        u.log.debug('OK')
+
     def test_302_keystone_logging_config(self):
         """Verify the data in the keystone logging config file"""
         u.log.debug('Checking keystone config file...')
-        unit = self.keystone_sentry
         conf = '/etc/keystone/logging.conf'
         expected = {
             'logger_root': {
@@ -637,16 +708,17 @@ class KeystoneBasicDeployment(OpenStackAmuletDeployment):
             }
         }
 
-        for section, pairs in expected.iteritems():
+        for unit in self.keystone_sentries:
-            ret = u.validate_config_data(unit, conf, section, pairs)
+            for section, pairs in expected.iteritems():
-            if ret:
+                ret = u.validate_config_data(unit, conf, section, pairs)
-                message = "keystone logging config error: {}".format(ret)
+                if ret:
-                amulet.raise_status(amulet.FAIL, msg=message)
+                    message = "keystone logging config error: {}".format(ret)
+                    amulet.raise_status(amulet.FAIL, msg=message)
 
     def test_900_keystone_restart_on_config_change(self):
         """Verify that the specified services are restarted when the config
            is changed."""
-        sentry = self.keystone_sentry
+        sentry = self.keystone_sentries[0]
         juju_service = 'keystone'
 
         # Expected default and alternate values
@@ -689,13 +761,15 @@ class KeystoneBasicDeployment(OpenStackAmuletDeployment):
         set_default = {'use-syslog': 'False'}
         set_alternate = {'use-syslog': 'True'}
         self._assert_services(should_run=True)
-        action_id = u.run_action(self.keystone_sentry, "pause")
+        for unit in self.keystone_sentries:
-        assert u.wait_on_action(action_id), "Pause action failed."
+            action_id = u.run_action(unit, "pause")
+            assert u.wait_on_action(action_id), "Pause action failed."
 
         self._assert_services(should_run=False)
         self.d.configure('keystone', set_alternate)
-        action_id = u.run_action(self.keystone_sentry, "resume")
+        for unit in self.keystone_sentries:
-        assert u.wait_on_action(action_id), "Resume action failed"
+            action_id = u.run_action(unit, "resume")
+            assert u.wait_on_action(action_id), "Resume action failed"
         self._assert_services(should_run=True)
         self.d.configure('keystone', set_default)
         self._auto_wait_for_status(message="Unit is ready",

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, 'leader_get')
     @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, _leader_get):
         _get_api_version.return_value = 2
-        _get_admin_domain_id.return_value = None
+        _leader_get.return_value = None
         relation_id = 'identity-service:0'
         remote_unit = 'unit/0'
         _resolve_address.return_value = '10.10.10.10'
@@ -251,8 +251,8 @@ class TestKeystoneUtils(CharmTestCase):
         self.peer_store_and_set.assert_called_with(relation_id=relation_id,
                                                    **relation_data)
 
+    @patch.object(utils, 'leader_get')
     @patch.object(utils, 'get_api_version')
-    @patch.object(utils, 'get_admin_domain_id')
     @patch.object(utils, 'create_user')
     @patch.object(utils, 'resolve_address')
     @patch.object(utils, 'ensure_valid_service')
@@ -260,10 +260,10 @@ 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_api_version, leader_get,
-            get_api_version, test_api_version=2):
+            test_api_version=2):
-        get_admin_domain_id.return_value = None
         get_api_version.return_value = test_api_version
+        leader_get.return_value = None
         relation_id = 'identity-service:0'
         remote_unit = 'unit/0'
         self.get_admin_token.return_value = 'token'
@@ -350,13 +350,14 @@ class TestKeystoneUtils(CharmTestCase):
             test_add_service_to_keystone_no_clustered_no_https_complete_values(
                 test_api_version=3)
 
+    @patch.object(utils, 'leader_get')
     @patch('charmhelpers.contrib.openstack.ip.config')
     @patch.object(utils, 'ensure_valid_service')
     @patch.object(utils, 'add_endpoint')
     @patch.object(utils, 'get_manager')
     def test_add_service_to_keystone_nosubset(
             self, KeystoneManager, add_endpoint, ensure_valid_service,
-            ip_config):
+            ip_config, leader_get):
         relation_id = 'identity-service:0'
         remote_unit = 'unit/0'
 
@@ -367,6 +368,7 @@ class TestKeystoneUtils(CharmTestCase):
                                           'ec2_internal_url': '192.168.1.2'}
         self.get_local_endpoint.return_value = 'http://localhost:80/v2.0/'
         KeystoneManager.resolve_tenant_id.return_value = 'tenant_id'
+        leader_get.return_value = None
 
         utils.add_service_to_keystone(
             relation_id=relation_id,
@@ -690,6 +692,8 @@ class TestKeystoneUtils(CharmTestCase):
         self.assertFalse(utils.ensure_ssl_cert_master())
         self.assertFalse(self.relation_set.called)
 
+    @patch.object(utils, 'leader_set')
+    @patch.object(utils, 'leader_get')
     @patch('charmhelpers.contrib.openstack.ip.unit_get')
     @patch('charmhelpers.contrib.openstack.ip.is_clustered')
     @patch('charmhelpers.contrib.openstack.ip.config')
@@ -704,10 +708,13 @@ class TestKeystoneUtils(CharmTestCase):
                                               _create_keystone_endpoint,
                                               _ip_config,
                                               _is_clustered,
-                                              _unit_get):
+                                              _unit_get,
+                                              _leader_get,
+                                              _leader_set):
         _is_clustered.return_value = False
         _ip_config.side_effect = self.test_config.get
         _unit_get.return_value = '10.0.0.1'
+        _leader_get.return_value = None
         self.test_config.set('os-public-hostname', 'keystone.example.com')
         utils.ensure_initial_admin(self.config)
         _create_keystone_endpoint.assert_called_with(
@@ -860,18 +867,6 @@ class TestKeystoneUtils(CharmTestCase):
             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')
-
     def test_assess_status(self):
         with patch.object(utils, 'assess_status_func') as asf:
             callee = MagicMock()