Merge "Relax newly imposed sql driver restriction for domain config"

8 files changed, 357 insertions, 17 deletions

diff --git a/keystone/common/sql/migrate_repo/versions/075_confirm_config_registration.py b/keystone/common/sql/migrate_repo/versions/075_confirm_config_registration.py
new file mode 100644
index 000000000..576842c69
--- /dev/null
+++ b/keystone/common/sql/migrate_repo/versions/075_confirm_config_registration.py
@@ -0,0 +1,29 @@
+#    Licensed under the Apache License, Version 2.0 (the "License"); you may
+#    not use this file except in compliance with the License. You may obtain
+#    a copy of the License at
+#
+#         http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
+#    WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
+#    License for the specific language governing permissions and limitations
+#    under the License.
+
+import sqlalchemy as sql
+
+REGISTRATION_TABLE = 'config_register'
+
+
+def upgrade(migrate_engine):
+    meta = sql.MetaData()
+    meta.bind = migrate_engine
+
+    registration_table = sql.Table(
+        REGISTRATION_TABLE,
+        meta,
+        sql.Column('type', sql.String(64), primary_key=True),
+        sql.Column('domain_id', sql.String(64), nullable=False),
+        mysql_engine='InnoDB',
+        mysql_charset='utf8')
+    registration_table.create(migrate_engine, checkfirst=True)
diff --git a/keystone/exception.py b/keystone/exception.py
index 2940d5083..f5c0e1cdf 100644
--- a/keystone/exception.py
+++ b/keystone/exception.py
@@ -357,6 +357,13 @@ class DomainConfigNotFound(NotFound):
                        'configuration for domain %(domain_id)s')
 
 
+class ConfigRegistrationNotFound(Exception):
+    # This is used internally between the domain config backend and the
+    # manager, so should not escape to the client.  If it did, it is a coding
+    # error on our part, and would end up, appropriately, as a 500 error.
+    pass
+
+
 class Conflict(Error):
     message_format = _("Conflict occurred attempting to store %(type)s -"
                        " %(details)s")
diff --git a/keystone/identity/core.py b/keystone/identity/core.py
index a619e770d..061b82e12 100644
--- a/keystone/identity/core.py
+++ b/keystone/identity/core.py
@@ -44,6 +44,14 @@ MEMOIZE = cache.get_memoization_decorator(section='identity')
 DOMAIN_CONF_FHEAD = 'keystone.'
 DOMAIN_CONF_FTAIL = '.conf'
 
+# The number of times we will attempt to register a domain to use the SQL
+# driver, if we find that another process is in the middle of registering or
+# releasing at the same time as us.
+REGISTRATION_ATTEMPTS = 10
+
+# Config Registration Types
+SQL_DRIVER = 'SQL'
+
 
 def filter_user(user_ref):
     """Filter out private items in a user dict.
@@ -67,7 +75,7 @@ def filter_user(user_ref):
     return user_ref
 
 
-@dependency.requires('domain_config_api')
+@dependency.requires('domain_config_api', 'resource_api')
 class DomainConfigs(dict):
     """Discover, store and provide access to domain specific configs.
 
@@ -95,7 +103,7 @@ class DomainConfigs(dict):
 
     def _assert_no_more_than_one_sql_driver(self, domain_id, new_config,
                                             config_file=None):
-        """Ensure there is more than one sql driver.
+        """Ensure there is no more than one sql driver.
 
         Check to see if the addition of the driver in this new config
         would cause there to now be more than one sql driver.
@@ -108,6 +116,13 @@ class DomainConfigs(dict):
                 (self.driver.is_sql or self._any_sql)):
             # The addition of this driver would cause us to have more than
             # one sql driver, so raise an exception.
+
+            # TODO(henry-nash): This method is only used in the file-based
+            # case, so has no need to worry about the database/API case. The
+            # code that overrides config_file below is therefore never used
+            # and should be removed, and this method perhaps moved inside
+            # _load_config_from_file(). This is raised as bug #1466772.
+
             if not config_file:
                 config_file = _('Database at /domains/%s/config') % domain_id
             raise exception.MultipleSQLDriversInConfig(source=config_file)
@@ -177,19 +192,93 @@ class DomainConfigs(dict):
 
     def _load_config_from_database(self, domain_id, specific_config):
 
-        def _assert_not_sql_driver(domain_id, new_config):
-            """Ensure this is not an sql driver.
+        def _assert_no_more_than_one_sql_driver(domain_id, new_config):
+            """Ensure adding driver doesn't push us over the limit of 1
 
-            Due to multi-threading safety concerns, we do not currently support
-            the setting of a specific identity driver to sql via the Identity
-            API.
+            The checks we make in this method need to take into account that
+            we may be in a multiple process configuration and ensure that
+            any race conditions are avoided.
 
             """
-            if new_config['driver'].is_sql:
-                reason = _('Domain specific sql drivers are not supported via '
-                           'the Identity API. One is specified in '
-                           '/domains/%s/config') % domain_id
-                raise exception.InvalidDomainConfig(reason=reason)
+            if not new_config['driver'].is_sql:
+                self.domain_config_api.release_registration(domain_id)
+                return
+
+            # To ensure the current domain is the only SQL driver, we attempt
+            # to register our use of SQL. If we get it we know we are good,
+            # if we fail to register it then we should:
+            #
+            # - First check if another process has registered for SQL for our
+            #   domain, in which case we are fine
+            # - If a different domain has it, we should check that this domain
+            #   is still valid, in case, for example, domain deletion somehow
+            #   failed to remove its registration (i.e. we self heal for these
+            #   kinds of issues).
+
+            domain_registered = 'Unknown'
+            for attempt in range(REGISTRATION_ATTEMPTS):
+                if self.domain_config_api.obtain_registration(
+                        domain_id, SQL_DRIVER):
+                    LOG.debug('Domain %s successfully registered to use the '
+                              'SQL driver.', domain_id)
+                    return
+
+                # We failed to register our use, let's find out who is using it
+                try:
+                    domain_registered = (
+                        self.domain_config_api.read_registration(
+                            SQL_DRIVER))
+                except exception.ConfigRegistrationNotFound:
+                    msg = ('While attempting to register domain %(domain)s to '
+                           'use the SQL driver, another process released it, '
+                           'retrying (attempt %(attempt)s).')
+                    LOG.debug(msg, {'domain': domain_id,
+                                    'attempt': attempt + 1})
+                    continue
+
+                if domain_registered == domain_id:
+                    # Another process already registered it for us, so we are
+                    # fine. In the race condition when another process is
+                    # in the middle of deleting this domain, we know the domain
+                    # is already disabled and hence telling the caller that we
+                    # are registered is benign.
+                    LOG.debug('While attempting to register domain %s to use '
+                              'the SQL driver, found that another process had '
+                              'already registered this domain. This is normal '
+                              'in multi-process configurations.', domain_id)
+                    return
+
+                # So we don't have it, but someone else does...let's check that
+                # this domain is still valid
+                try:
+                    self.resource_api.get_domain(domain_registered)
+                except exception.DomainNotFound:
+                    msg = ('While attempting to register domain %(domain)s to '
+                           'use the SQL driver, found that it was already '
+                           'registered to a domain that no longer exists '
+                           '(%(old_domain)s). Removing this stale '
+                           'registration and retrying (attempt %(attempt)s).')
+                    LOG.debug(msg, {'domain': domain_id,
+                                    'old_domain': domain_registered,
+                                    'attempt': attempt + 1})
+                    self.domain_config_api.release_registration(
+                        domain_registered, type=SQL_DRIVER)
+                    continue
+
+                # The domain is valid, so we really do have an attempt at more
+                # than one SQL driver.
+                details = (
+                    _('Config API entity at /domains/%s/config') % domain_id)
+                raise exception.MultipleSQLDriversInConfig(source=details)
+
+            # We fell out of the loop without either registering our domain or
+            # being able to find who has it...either we were very very very
+            # unlucky or something is awry.
+            msg = _('Exceeded attempts to register domain %(domain)s to use '
+                    'the SQL driver, the last  domain that appears to have '
+                    'had it is %(last_domain)s, giving up') % {
+                        'domain': domain_id, 'last_domain': domain_registered}
+            raise exception.UnexpectedError(msg)
 
         domain_config = {}
         domain_config['cfg'] = cfg.ConfigOpts()
@@ -207,7 +296,7 @@ class DomainConfigs(dict):
 
         domain_config['cfg_overrides'] = specific_config
         domain_config['driver'] = self._load_driver(domain_config)
-        _assert_not_sql_driver(domain_id, domain_config)
+        _assert_no_more_than_one_sql_driver(domain_id, domain_config)
         self[domain_id] = domain_config
 
     def _setup_domain_drivers_from_database(self, standard_driver,
diff --git a/keystone/resource/config_backends/sql.py b/keystone/resource/config_backends/sql.py
index 846423b2d..7c296074a 100644
--- a/keystone/resource/config_backends/sql.py
+++ b/keystone/resource/config_backends/sql.py
@@ -42,6 +42,12 @@ class SensitiveConfig(sql.ModelBase, sql.ModelDictMixin):
         return d
 
 
+class ConfigRegister(sql.ModelBase, sql.ModelDictMixin):
+    __tablename__ = 'config_register'
+    type = sql.Column(sql.String(64), primary_key=True)
+    domain_id = sql.Column(sql.String(64), nullable=False)
+
+
 class DomainConfig(resource.DomainConfigDriverV8):
 
     def choose_table(self, sensitive):
@@ -117,3 +123,30 @@ class DomainConfig(resource.DomainConfigDriverV8):
                 if option:
                     query = query.filter_by(option=option)
             query.delete(False)
+
+    def obtain_registration(self, domain_id, type):
+        try:
+            with sql.transaction() as session:
+                ref = ConfigRegister(type=type, domain_id=domain_id)
+                session.add(ref)
+            return True
+        except sql.DBDuplicateEntry:
+            pass
+        return False
+
+    def read_registration(self, type):
+        with sql.transaction() as session:
+            ref = session.query(ConfigRegister).get(type)
+            if not ref:
+                raise exception.ConfigRegistrationNotFound()
+        return ref.domain_id
+
+    def release_registration(self, domain_id, type=None):
+        """Silently delete anything registered for the domain specified."""
+
+        with sql.transaction() as session:
+            query = session.query(ConfigRegister)
+            if type:
+                query = query.filter_by(type=type)
+            query = query.filter_by(domain_id=domain_id)
+            query.delete(False)
diff --git a/keystone/resource/core.py b/keystone/resource/core.py
index f2de22e2a..6015107d2 100644
--- a/keystone/resource/core.py
+++ b/keystone/resource/core.py
@@ -460,6 +460,7 @@ class Manager(manager.Manager):
         # Delete any database stored domain config
         self.domain_config_api.delete_config_options(domain_id)
         self.domain_config_api.delete_config_options(domain_id, sensitive=True)
+        self.domain_config_api.release_registration(domain_id)
         # TODO(henry-nash): Although the controller will ensure deletion of
         # all users & groups within the domain (which will cause all
         # assignments for those users/groups to also be deleted), there
@@ -1373,5 +1374,44 @@ class DomainConfigDriverV8(object):
         """
         raise exception.NotImplemented()  # pragma: no cover
 
+    @abc.abstractmethod
+    def obtain_registration(self, domain_id, type):
+        """Try and register this domain to use the type specified.
+
+        :param domain_id: the domain required
+        :param type: type of registration
+        :returns: True if the domain was registered, False otherwise. Failing
+                  to register means that someone already has it (which could
+                  even be the domain being requested).
+
+        """
+        raise exception.NotImplemented()  # pragma: no cover
+
+    @abc.abstractmethod
+    def read_registration(self, type):
+        """Get the domain ID of who is registered to use this type.
+
+        :param type: type of registration
+        :returns: domain_id of who is registered.
+        :raises: keystone.exception.ConfigRegistrationNotFound: nobody is
+                 registered.
+
+        """
+        raise exception.NotImplemented()  # pragma: no cover
+
+    @abc.abstractmethod
+    def release_registration(self, domain_id, type=None):
+        """Release registration if it is held by the domain specified.
+
+        If the specified domain is registered for this domain then free it,
+        if it is not then do nothing - no exception is raised.
+
+        :param domain_id: the domain in question
+        :param type: type of registration, if None then all registrations
+                     for this domain will be freed
+
+        """
+        raise exception.NotImplemented()  # pragma: no cover
+
 
 DomainConfigDriver = manager.create_legacy_driver(DomainConfigDriverV8)
diff --git a/keystone/tests/unit/backend/domain_config/core.py b/keystone/tests/unit/backend/domain_config/core.py
index e0bb44114..7bbbf313e 100644
--- a/keystone/tests/unit/backend/domain_config/core.py
+++ b/keystone/tests/unit/backend/domain_config/core.py
@@ -549,3 +549,53 @@ class DomainConfigTests(object):
             {},
             self.domain_config_api.get_config_with_sensitive_info(
                 self.domain['id']))
+
+    def test_config_registration(self):
+        type = uuid.uuid4().hex
+        self.domain_config_api.obtain_registration(
+            self.domain['id'], type)
+        self.domain_config_api.release_registration(
+            self.domain['id'], type=type)
+
+        # Make sure that once someone has it, nobody else can get it.
+        # This includes the domain who already has it.
+        self.domain_config_api.obtain_registration(
+            self.domain['id'], type)
+        self.assertFalse(
+            self.domain_config_api.obtain_registration(
+                self.domain['id'], type))
+
+        # Make sure we can read who does have it
+        self.assertEqual(
+            self.domain['id'],
+            self.domain_config_api.read_registration(type))
+
+        # Make sure releasing it is silent if the domain specified doesn't
+        # have the registration
+        domain2 = {'id': uuid.uuid4().hex, 'name': uuid.uuid4().hex}
+        self.resource_api.create_domain(domain2['id'], domain2)
+        self.domain_config_api.release_registration(
+            domain2['id'], type=type)
+
+        # If nobody has the type registered, then trying to read it should
+        # raise ConfigRegistrationNotFound
+        self.domain_config_api.release_registration(
+            self.domain['id'], type=type)
+        self.assertRaises(exception.ConfigRegistrationNotFound,
+                          self.domain_config_api.read_registration,
+                          type)
+
+        # Finally check multiple registrations are cleared if you free the
+        # registration without specifying the type
+        type2 = uuid.uuid4().hex
+        self.domain_config_api.obtain_registration(
+            self.domain['id'], type)
+        self.domain_config_api.obtain_registration(
+            self.domain['id'], type2)
+        self.domain_config_api.release_registration(self.domain['id'])
+        self.assertRaises(exception.ConfigRegistrationNotFound,
+                          self.domain_config_api.read_registration,
+                          type)
+        self.assertRaises(exception.ConfigRegistrationNotFound,
+                          self.domain_config_api.read_registration,
+                          type2)
diff --git a/keystone/tests/unit/test_backend_ldap.py b/keystone/tests/unit/test_backend_ldap.py
index 8bdc8145b..16d54973e 100644
--- a/keystone/tests/unit/test_backend_ldap.py
+++ b/keystone/tests/unit/test_backend_ldap.py
@@ -2943,15 +2943,100 @@ class MultiLDAPandSQLIdentityDomainConfigsInSQL(MultiLDAPandSQLIdentity):
             domain_cfgs.get_domain_conf(CONF.identity.default_domain_id))
         self.assertEqual(CONF.ldap.url, default_config.ldap.url)
 
-    def test_setting_sql_driver_raises_exception(self):
-        """Ensure setting of domain specific sql driver is prevented."""
+    def test_setting_multiple_sql_driver_raises_exception(self):
+        """Ensure setting multiple domain specific sql drivers is prevented."""
 
         new_config = {'identity': {'driver': 'sql'}}
         self.domain_config_api.create_config(
             CONF.identity.default_domain_id, new_config)
-        self.assertRaises(exception.InvalidDomainConfig,
+        self.identity_api.domain_configs.get_domain_conf(
+            CONF.identity.default_domain_id)
+        self.domain_config_api.create_config(self.domains['domain1']['id'],
+                                             new_config)
+        self.assertRaises(exception.MultipleSQLDriversInConfig,
                           self.identity_api.domain_configs.get_domain_conf,
-                          CONF.identity.default_domain_id)
+                          self.domains['domain1']['id'])
+
+    def test_same_domain_gets_sql_driver(self):
+        """Ensure we can set an SQL driver if we have had it before."""
+
+        new_config = {'identity': {'driver': 'sql'}}
+        self.domain_config_api.create_config(
+            CONF.identity.default_domain_id, new_config)
+        self.identity_api.domain_configs.get_domain_conf(
+            CONF.identity.default_domain_id)
+
+        # By using a slightly different config, we cause the driver to be
+        # reloaded...and hence check if we can reuse the sql driver
+        new_config = {'identity': {'driver': 'sql'},
+                      'ldap': {'url': 'fake://memory1'}}
+        self.domain_config_api.create_config(
+            CONF.identity.default_domain_id, new_config)
+        self.identity_api.domain_configs.get_domain_conf(
+            CONF.identity.default_domain_id)
+
+    def test_delete_domain_clears_sql_registration(self):
+        """Ensure registration is deleted when a domain is deleted."""
+
+        domain = {'id': uuid.uuid4().hex, 'name': uuid.uuid4().hex}
+        domain = self.resource_api.create_domain(domain['id'], domain)
+        new_config = {'identity': {'driver': 'sql'}}
+        self.domain_config_api.create_config(domain['id'], new_config)
+        self.identity_api.domain_configs.get_domain_conf(domain['id'])
+
+        # First show that trying to set SQL for another driver fails
+        self.domain_config_api.create_config(self.domains['domain1']['id'],
+                                             new_config)
+        self.assertRaises(exception.MultipleSQLDriversInConfig,
+                          self.identity_api.domain_configs.get_domain_conf,
+                          self.domains['domain1']['id'])
+        self.domain_config_api.delete_config(self.domains['domain1']['id'])
+
+        # Now we delete the domain
+        domain['enabled'] = False
+        self.resource_api.update_domain(domain['id'], domain)
+        self.resource_api.delete_domain(domain['id'])
+
+        # The registration should now be available
+        self.domain_config_api.create_config(self.domains['domain1']['id'],
+                                             new_config)
+        self.identity_api.domain_configs.get_domain_conf(
+            self.domains['domain1']['id'])
+
+    def test_orphaned_registration_does_not_prevent_getting_sql_driver(self):
+        """Ensure we self heal an orphaned sql registration."""
+
+        domain = {'id': uuid.uuid4().hex, 'name': uuid.uuid4().hex}
+        domain = self.resource_api.create_domain(domain['id'], domain)
+        new_config = {'identity': {'driver': 'sql'}}
+        self.domain_config_api.create_config(domain['id'], new_config)
+        self.identity_api.domain_configs.get_domain_conf(domain['id'])
+
+        # First show that trying to set SQL for another driver fails
+        self.domain_config_api.create_config(self.domains['domain1']['id'],
+                                             new_config)
+        self.assertRaises(exception.MultipleSQLDriversInConfig,
+                          self.identity_api.domain_configs.get_domain_conf,
+                          self.domains['domain1']['id'])
+
+        # Now we delete the domain by using the backend driver directly,
+        # which causes the domain to be deleted without any of the cleanup
+        # that is in the manager (this is simulating a server process crashing
+        # in the middle of a delete domain operation, and somehow leaving the
+        # domain config settings in place, but the domain is deleted). We
+        # should still be able to set another domain to SQL, since we should
+        # self heal this issue.
+
+        self.resource_api.driver.delete_domain(domain['id'])
+        # Invalidate cache (so we will see the domain has gone)
+        self.resource_api.get_domain.invalidate(
+            self.resource_api, domain['id'])
+
+        # The registration should now be available
+        self.domain_config_api.create_config(self.domains['domain1']['id'],
+                                             new_config)
+        self.identity_api.domain_configs.get_domain_conf(
+            self.domains['domain1']['id'])
 
 
 class DomainSpecificLDAPandSQLIdentity(
diff --git a/keystone/tests/unit/test_sql_upgrade.py b/keystone/tests/unit/test_sql_upgrade.py
index 7b14fc677..d617d445c 100644
--- a/keystone/tests/unit/test_sql_upgrade.py
+++ b/keystone/tests/unit/test_sql_upgrade.py
@@ -636,6 +636,13 @@ class SqlUpgradeTests(SqlMigrateBase):
                                  'enabled', 'domain_id', 'parent_id',
                                  'is_domain'])
 
+    def test_add_config_registration(self):
+        config_registration = 'config_register'
+        self.upgrade(74)
+        self.assertTableDoesNotExist(config_registration)
+        self.upgrade(75)
+        self.assertTableColumns(config_registration, ['type', 'domain_id'])
+
     def populate_user_table(self, with_pass_enab=False,
                             with_pass_enab_domain=False):
         # Populate the appropriate fields in the user