Make modifications to domain config atomic
Currently, since creating or updating a domain config is done as a sequence of option creates/updates, it is possible for other keystone threads/processes to see a partial update if the cache timeout occurs in the middle of such an update. This patch makes the creation/updating of a domain config atomic (by doing it all within a common sql session), hence avoiding this issue. This issue has not been reported in the field - and is extremely difficult to test, so we are relying on code inspection to convince ourselves we have solved this issue. Since the domain config API is still marked as experimental, we do not gurantee backward compatability for the driver. Hence, although this patch modifies the driver interface, it does not include a new versioned driver. Closes-bug: 1517038 Change-Id: I16598051ca456cf1c036c412111426b4cd9241b8
This commit is contained in:
Henry Nash
@@ -56,15 +56,23 @@ class DomainConfig(resource.DomainConfigDriverV8):
|
||||
else:
|
||||
return WhiteListedConfig
|
||||
|
||||
@sql.handle_conflicts(conflict_type='domain_config')
|
||||
def create_config_option(self, domain_id, group, option, value,
|
||||
sensitive=False):
|
||||
def _create_config_option(
|
||||
self, session, domain_id, group, option, sensitive, value):
|
||||
config_table = self.choose_table(sensitive)
|
||||
ref = config_table(domain_id=domain_id, group=group, option=option,
|
||||
value=value)
|
||||
session.add(ref)
|
||||
|
||||
def create_config_options(self, domain_id, option_list):
|
||||
with sql.session_for_write() as session:
|
||||
config_table = self.choose_table(sensitive)
|
||||
ref = config_table(domain_id=domain_id, group=group,
|
||||
option=option, value=value)
|
||||
session.add(ref)
|
||||
return ref.to_dict()
|
||||
for config_table in [WhiteListedConfig, SensitiveConfig]:
|
||||
query = session.query(config_table)
|
||||
query = query.filter_by(domain_id=domain_id)
|
||||
query.delete(False)
|
||||
for option in option_list:
|
||||
self._create_config_option(
|
||||
session, domain_id, option['group'],
|
||||
option['option'], option['sensitive'], option['value'])
|
||||
|
||||
def _get_config_option(self, session, domain_id, group, option, sensitive):
|
||||
try:
|
||||
@@ -97,25 +105,17 @@ class DomainConfig(resource.DomainConfigDriverV8):
|
||||
query = query.filter_by(option=option)
|
||||
return [ref.to_dict() for ref in query.all()]
|
||||
|
||||
def update_config_option(self, domain_id, group, option, value,
|
||||
sensitive=False):
|
||||
def update_config_options(self, domain_id, option_list):
|
||||
with sql.session_for_write() as session:
|
||||
ref = self._get_config_option(session, domain_id, group, option,
|
||||
sensitive)
|
||||
ref.value = value
|
||||
return ref.to_dict()
|
||||
for option in option_list:
|
||||
self._delete_config_options(
|
||||
session, domain_id, option['group'], option['option'])
|
||||
self._create_config_option(
|
||||
session, domain_id, option['group'], option['option'],
|
||||
option['sensitive'], option['value'])
|
||||
|
||||
def delete_config_options(self, domain_id, group=None, option=None,
|
||||
sensitive=False):
|
||||
"""Deletes config options that match the filter parameters.
|
||||
|
||||
Since the public API is broken down into calls for delete in both the
|
||||
whitelisted and sensitive methods, we are silent at the driver level
|
||||
if there was nothing to delete.
|
||||
|
||||
"""
|
||||
with sql.session_for_write() as session:
|
||||
config_table = self.choose_table(sensitive)
|
||||
def _delete_config_options(self, session, domain_id, group, option):
|
||||
for config_table in [WhiteListedConfig, SensitiveConfig]:
|
||||
query = session.query(config_table)
|
||||
query = query.filter_by(domain_id=domain_id)
|
||||
if group:
|
||||
@@ -124,6 +124,10 @@ class DomainConfig(resource.DomainConfigDriverV8):
|
||||
query = query.filter_by(option=option)
|
||||
query.delete(False)
|
||||
|
||||
def delete_config_options(self, domain_id, group=None, option=None):
|
||||
with sql.session_for_write() as session:
|
||||
self._delete_config_options(session, domain_id, group, option)
|
||||
|
||||
def obtain_registration(self, domain_id, type):
|
||||
try:
|
||||
with sql.session_for_write() as session:
|
||||
|
||||
+37
-63
@@ -794,7 +794,6 @@ class Manager(manager.Manager):
|
||||
self._delete_project(domain_id, initiator)
|
||||
# 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
|
||||
@@ -1555,18 +1554,16 @@ class DomainConfigManager(manager.Manager):
|
||||
return option in self.sensitive_options[group]
|
||||
|
||||
def _config_to_list(self, config):
|
||||
"""Build whitelisted and sensitive lists for use by backend drivers."""
|
||||
whitelisted = []
|
||||
sensitive = []
|
||||
"""Build list of options for use by backend drivers."""
|
||||
option_list = []
|
||||
for group in config:
|
||||
for option in config[group]:
|
||||
the_list = (sensitive if self._is_sensitive(group, option)
|
||||
else whitelisted)
|
||||
the_list.append({
|
||||
option_list.append({
|
||||
'group': group, 'option': option,
|
||||
'value': config[group][option]})
|
||||
'value': config[group][option],
|
||||
'sensitive': self._is_sensitive(group, option)})
|
||||
|
||||
return whitelisted, sensitive
|
||||
return option_list
|
||||
|
||||
def _list_to_config(self, whitelisted, sensitive=None, req_option=None):
|
||||
"""Build config dict from a list of option dicts.
|
||||
@@ -1626,23 +1623,13 @@ class DomainConfigManager(manager.Manager):
|
||||
|
||||
"""
|
||||
self._assert_valid_config(config)
|
||||
whitelisted, sensitive = self._config_to_list(config)
|
||||
# Delete any existing config
|
||||
self.delete_config_options(domain_id)
|
||||
self.delete_config_options(domain_id, sensitive=True)
|
||||
# ...and create the new one
|
||||
for option in whitelisted:
|
||||
self.create_config_option(
|
||||
domain_id, option['group'], option['option'], option['value'])
|
||||
for option in sensitive:
|
||||
self.create_config_option(
|
||||
domain_id, option['group'], option['option'], option['value'],
|
||||
sensitive=True)
|
||||
option_list = self._config_to_list(config)
|
||||
self.create_config_options(domain_id, option_list)
|
||||
# Since we are caching on the full substituted config, we just
|
||||
# invalidate here, rather than try and create the right result to
|
||||
# cache.
|
||||
self.get_config_with_sensitive_info.invalidate(self, domain_id)
|
||||
return self._list_to_config(whitelisted)
|
||||
return self._list_to_config(self.list_config_options(domain_id))
|
||||
|
||||
def get_config(self, domain_id, group=None, option=None):
|
||||
"""Get config, or partial config, for a domain
|
||||
@@ -1769,17 +1756,6 @@ class DomainConfigManager(manager.Manager):
|
||||
raise exception.DomainConfigNotFound(
|
||||
domain_id=domain_id, group_or_option=msg)
|
||||
|
||||
def _update_or_create(domain_id, option, sensitive):
|
||||
"""Update the option, if it doesn't exist then create it."""
|
||||
try:
|
||||
self.create_config_option(
|
||||
domain_id, option['group'], option['option'],
|
||||
option['value'], sensitive=sensitive)
|
||||
except exception.Conflict:
|
||||
self.update_config_option(
|
||||
domain_id, option['group'], option['option'],
|
||||
option['value'], sensitive=sensitive)
|
||||
|
||||
update_config = config
|
||||
if group and option:
|
||||
# The config will just be a dict containing the option and
|
||||
@@ -1789,12 +1765,8 @@ class DomainConfigManager(manager.Manager):
|
||||
|
||||
_assert_valid_update(domain_id, update_config, group, option)
|
||||
|
||||
whitelisted, sensitive = self._config_to_list(update_config)
|
||||
|
||||
for new_option in whitelisted:
|
||||
_update_or_create(domain_id, new_option, sensitive=False)
|
||||
for new_option in sensitive:
|
||||
_update_or_create(domain_id, new_option, sensitive=True)
|
||||
option_list = self._config_to_list(update_config)
|
||||
self.update_config_options(domain_id, option_list)
|
||||
|
||||
self.get_config_with_sensitive_info.invalidate(self, domain_id)
|
||||
return self.get_config(domain_id)
|
||||
@@ -1836,7 +1808,6 @@ class DomainConfigManager(manager.Manager):
|
||||
domain_id=domain_id, group_or_option=msg)
|
||||
|
||||
self.delete_config_options(domain_id, group, option)
|
||||
self.delete_config_options(domain_id, group, option, sensitive=True)
|
||||
self.get_config_with_sensitive_info.invalidate(self, domain_id)
|
||||
|
||||
def _get_config_with_sensitive_info(self, domain_id, group=None,
|
||||
@@ -1969,18 +1940,28 @@ class DomainConfigDriverV8(object):
|
||||
"""Interface description for a Domain Config driver."""
|
||||
|
||||
@abc.abstractmethod
|
||||
def create_config_option(self, domain_id, group, option, value,
|
||||
sensitive=False):
|
||||
"""Creates a config option for a domain.
|
||||
def create_config_options(self, domain_id, option_list):
|
||||
"""Creates config options for a domain.
|
||||
|
||||
Any existing config options will first be deleted.
|
||||
|
||||
:param domain_id: the domain for this option
|
||||
:param group: the group name
|
||||
:param option: the option name
|
||||
:param value: the value to assign to this option
|
||||
:param sensitive: whether the option is sensitive
|
||||
:param option_list: a list of dicts, each one specifying an option
|
||||
|
||||
:returns: dict containing group, option and value
|
||||
:raises keystone.exception.Conflict: when the option already exists
|
||||
Option schema::
|
||||
|
||||
type: dict
|
||||
properties:
|
||||
group:
|
||||
type: string
|
||||
option:
|
||||
type: string
|
||||
value:
|
||||
type: depends on the option
|
||||
sensitive:
|
||||
type: boolean
|
||||
required: [group, option, value, sensitive]
|
||||
additionalProperties: false
|
||||
|
||||
"""
|
||||
raise exception.NotImplemented() # pragma: no cover
|
||||
@@ -2018,26 +1999,17 @@ class DomainConfigDriverV8(object):
|
||||
raise exception.NotImplemented() # pragma: no cover
|
||||
|
||||
@abc.abstractmethod
|
||||
def update_config_option(self, domain_id, group, option, value,
|
||||
sensitive=False):
|
||||
"""Updates a config option for a domain.
|
||||
def update_config_options(self, domain_id, option_list):
|
||||
"""Updates config options for a domain.
|
||||
|
||||
:param domain_id: the domain for this option
|
||||
:param group: the group option name
|
||||
:param option: the option name
|
||||
:param value: the value to assign to this option
|
||||
:param sensitive: whether the option is sensitive
|
||||
|
||||
:returns: dict containing updated group, option and value
|
||||
:raises keystone.exception.DomainConfigNotFound: the option doesn't
|
||||
exist.
|
||||
:param option_list: a list of dicts, each one specifying an option
|
||||
|
||||
"""
|
||||
raise exception.NotImplemented() # pragma: no cover
|
||||
|
||||
@abc.abstractmethod
|
||||
def delete_config_options(self, domain_id, group=None, option=None,
|
||||
sensitive=False):
|
||||
def delete_config_options(self, domain_id, group=None, option=None):
|
||||
"""Deletes config options for a domain.
|
||||
|
||||
Allows deletion of all options for a domain, all options in a group
|
||||
@@ -2048,7 +2020,9 @@ class DomainConfigDriverV8(object):
|
||||
:param group: optional group option name
|
||||
:param option: optional option name. If group is None, then this
|
||||
parameter is ignored
|
||||
:param sensitive: whether the option is sensitive
|
||||
|
||||
The option is uniquely defined by domain_id, group and option,
|
||||
irrespective of whether it is sensistive ot not.
|
||||
|
||||
"""
|
||||
raise exception.NotImplemented() # pragma: no cover
|
||||
|
||||
@@ -102,29 +102,29 @@ class DomainConfigDriverTests(object):
|
||||
group = uuid.uuid4().hex
|
||||
option = uuid.uuid4().hex
|
||||
value = uuid.uuid4().hex
|
||||
self.driver.create_config_option(
|
||||
domain, group, option, value, sensitive)
|
||||
config = {'group': group, 'option': option, 'value': value,
|
||||
'sensitive': sensitive}
|
||||
self.driver.create_config_options(domain, [config])
|
||||
res = self.driver.get_config_option(
|
||||
domain, group, option, sensitive)
|
||||
config = {'group': group, 'option': option, 'value': value}
|
||||
config.pop('sensitive')
|
||||
self.assertEqual(config, res)
|
||||
|
||||
value = uuid.uuid4().hex
|
||||
self.driver.update_config_option(
|
||||
domain, group, option, value, sensitive)
|
||||
config = {'group': group, 'option': option, 'value': value,
|
||||
'sensitive': sensitive}
|
||||
self.driver.update_config_options(domain, [config])
|
||||
res = self.driver.get_config_option(
|
||||
domain, group, option, sensitive)
|
||||
config = {'group': group, 'option': option, 'value': value}
|
||||
config.pop('sensitive')
|
||||
self.assertEqual(config, res)
|
||||
|
||||
self.driver.delete_config_options(
|
||||
domain, group, option, sensitive)
|
||||
self.driver.delete_config_options(domain, group, option)
|
||||
self.assertRaises(exception.DomainConfigNotFound,
|
||||
self.driver.get_config_option,
|
||||
domain, group, option, sensitive)
|
||||
# ...and silent if we try to delete it again
|
||||
self.driver.delete_config_options(
|
||||
domain, group, option, sensitive)
|
||||
self.driver.delete_config_options(domain, group, option)
|
||||
|
||||
def test_whitelisted_domain_config_crud(self):
|
||||
self._domain_config_crud(sensitive=False)
|
||||
@@ -135,19 +135,19 @@ class DomainConfigDriverTests(object):
|
||||
def _list_domain_config(self, sensitive):
|
||||
"""Test listing by combination of domain, group & option."""
|
||||
config1 = {'group': uuid.uuid4().hex, 'option': uuid.uuid4().hex,
|
||||
'value': uuid.uuid4().hex}
|
||||
'value': uuid.uuid4().hex, 'sensitive': sensitive}
|
||||
# Put config2 in the same group as config1
|
||||
config2 = {'group': config1['group'], 'option': uuid.uuid4().hex,
|
||||
'value': uuid.uuid4().hex}
|
||||
'value': uuid.uuid4().hex, 'sensitive': sensitive}
|
||||
config3 = {'group': uuid.uuid4().hex, 'option': uuid.uuid4().hex,
|
||||
'value': 100}
|
||||
'value': 100, 'sensitive': sensitive}
|
||||
domain = uuid.uuid4().hex
|
||||
|
||||
for config in [config1, config2, config3]:
|
||||
self.driver.create_config_option(
|
||||
domain, config['group'], config['option'],
|
||||
config['value'], sensitive)
|
||||
self.driver.create_config_options(
|
||||
domain, [config1, config2, config3])
|
||||
|
||||
for config in [config1, config2, config3]:
|
||||
config.pop('sensitive')
|
||||
# Try listing all items from a domain
|
||||
res = self.driver.list_config_options(
|
||||
domain, sensitive=sensitive)
|
||||
@@ -178,45 +178,39 @@ class DomainConfigDriverTests(object):
|
||||
def _delete_domain_configs(self, sensitive):
|
||||
"""Test deleting by combination of domain, group & option."""
|
||||
config1 = {'group': uuid.uuid4().hex, 'option': uuid.uuid4().hex,
|
||||
'value': uuid.uuid4().hex}
|
||||
'value': uuid.uuid4().hex, 'sensitive': sensitive}
|
||||
# Put config2 and config3 in the same group as config1
|
||||
config2 = {'group': config1['group'], 'option': uuid.uuid4().hex,
|
||||
'value': uuid.uuid4().hex}
|
||||
'value': uuid.uuid4().hex, 'sensitive': sensitive}
|
||||
config3 = {'group': config1['group'], 'option': uuid.uuid4().hex,
|
||||
'value': uuid.uuid4().hex}
|
||||
'value': uuid.uuid4().hex, 'sensitive': sensitive}
|
||||
config4 = {'group': uuid.uuid4().hex, 'option': uuid.uuid4().hex,
|
||||
'value': uuid.uuid4().hex}
|
||||
'value': uuid.uuid4().hex, 'sensitive': sensitive}
|
||||
domain = uuid.uuid4().hex
|
||||
|
||||
for config in [config1, config2, config3, config4]:
|
||||
self.driver.create_config_option(
|
||||
domain, config['group'], config['option'],
|
||||
config['value'], sensitive)
|
||||
self.driver.create_config_options(
|
||||
domain, [config1, config2, config3, config4])
|
||||
|
||||
for config in [config1, config2, config3, config4]:
|
||||
config.pop('sensitive')
|
||||
# Try deleting by domain, group and option
|
||||
res = self.driver.delete_config_options(
|
||||
domain, group=config2['group'],
|
||||
option=config2['option'], sensitive=sensitive)
|
||||
res = self.driver.list_config_options(
|
||||
domain, sensitive=sensitive)
|
||||
domain, group=config2['group'], option=config2['option'])
|
||||
res = self.driver.list_config_options(domain, sensitive=sensitive)
|
||||
self.assertThat(res, matchers.HasLength(3))
|
||||
for res_entry in res:
|
||||
self.assertIn(res_entry, [config1, config3, config4])
|
||||
|
||||
# Try deleting by domain and group
|
||||
res = self.driver.delete_config_options(
|
||||
domain, group=config4['group'], sensitive=sensitive)
|
||||
res = self.driver.list_config_options(
|
||||
domain, sensitive=sensitive)
|
||||
res = self.driver.delete_config_options(domain, group=config4['group'])
|
||||
res = self.driver.list_config_options(domain, sensitive=sensitive)
|
||||
self.assertThat(res, matchers.HasLength(2))
|
||||
for res_entry in res:
|
||||
self.assertIn(res_entry, [config1, config3])
|
||||
|
||||
# Try deleting all items from a domain
|
||||
res = self.driver.delete_config_options(
|
||||
domain, sensitive=sensitive)
|
||||
res = self.driver.list_config_options(
|
||||
domain, sensitive=sensitive)
|
||||
res = self.driver.delete_config_options(domain)
|
||||
res = self.driver.list_config_options(domain, sensitive=sensitive)
|
||||
self.assertThat(res, matchers.HasLength(0))
|
||||
|
||||
def test_delete_whitelisted_domain_configs(self):
|
||||
@@ -226,18 +220,18 @@ class DomainConfigDriverTests(object):
|
||||
self._delete_domain_configs(True)
|
||||
|
||||
def _create_domain_config_twice(self, sensitive):
|
||||
"""Test conflict error thrown if create the same option twice."""
|
||||
"""Test create the same option twice just overwrites."""
|
||||
config = {'group': uuid.uuid4().hex, 'option': uuid.uuid4().hex,
|
||||
'value': uuid.uuid4().hex}
|
||||
'value': uuid.uuid4().hex, 'sensitive': sensitive}
|
||||
domain = uuid.uuid4().hex
|
||||
|
||||
self.driver.create_config_option(
|
||||
domain, config['group'], config['option'],
|
||||
config['value'], sensitive=sensitive)
|
||||
self.assertRaises(exception.Conflict,
|
||||
self.driver.create_config_option,
|
||||
domain, config['group'], config['option'],
|
||||
config['value'], sensitive=sensitive)
|
||||
self.driver.create_config_options(domain, [config])
|
||||
config['value'] = uuid.uuid4().hex
|
||||
self.driver.create_config_options(domain, [config])
|
||||
res = self.driver.get_config_option(
|
||||
domain, config['group'], config['option'], sensitive)
|
||||
config.pop('sensitive')
|
||||
self.assertEqual(config, res)
|
||||
|
||||
def test_create_whitelisted_domain_config_twice(self):
|
||||
self._create_domain_config_twice(False)
|
||||
@@ -580,8 +574,6 @@ class DomainConfigTests(object):
|
||||
|
||||
# delete, bypassing domain config manager api
|
||||
self.domain_config_api.delete_config_options(self.domain['id'])
|
||||
self.domain_config_api.delete_config_options(self.domain['id'],
|
||||
sensitive=True)
|
||||
|
||||
self.assertDictEqual(
|
||||
res, self.domain_config_api.get_config_with_sensitive_info(
|
||||
|
||||
Reference in New Issue
Block a user