From b5fe0ef6c9a999cf5f622de6a6bd0727ccf5459c Mon Sep 17 00:00:00 2001 From: Dmitrii Shcherbakov Date: Sat, 6 Jan 2018 19:37:57 +0300 Subject: [PATCH] refactor keystone-ldap * replace singletons with provide_charm_instance (in the code and tests) * select an openstack release based on keystone package due to the lack of openstack-origin from the principal layer (this is a subordinate, see https://git.io/vNTyx) * use reactive triggers to drop config.complete (new configuration completeness) and config.rendered (non-stale config is rendered) on config.changed * do not check config completeness on every event - only when config has actually changed * remove the domain configuration file when relation with keystone is removed (service restart should be performed on the keystone charm side) * replace path_hash with file_hash (path_hash returns a new dict) * modify unit tests to reflect the changes Change-Id: Ied4b6ed64354e3de3c78e6ac809666ee9ae29d1a Closes-Bug: #1741661 --- src/layer.yaml | 2 +- src/lib/charm/openstack/keystone_ldap.py | 64 ++++--- src/reactive/keystone_ldap_handlers.py | 50 ++++-- unit_tests/test_keystone_ldap_handlers.py | 128 +++++++++----- .../test_lib_charm_openstack_keystone_ldap.py | 161 ++++++++++-------- 5 files changed, 245 insertions(+), 160 deletions(-) diff --git a/src/layer.yaml b/src/layer.yaml index 4d052a6..4708ae2 100644 --- a/src/layer.yaml +++ b/src/layer.yaml @@ -1,4 +1,4 @@ -includes: ['layer:openstack', 'interface:keystone-domain-backend'] +includes: ['layer:openstack', 'interface:keystone-domain-backend', 'interface:juju-info'] options: basic: use_venv: True diff --git a/src/lib/charm/openstack/keystone_ldap.py b/src/lib/charm/openstack/keystone_ldap.py index f501909..8a95c1c 100644 --- a/src/lib/charm/openstack/keystone_ldap.py +++ b/src/lib/charm/openstack/keystone_ldap.py @@ -23,11 +23,36 @@ import charmhelpers.contrib.openstack.utils as os_utils import charms_openstack.charm import charms_openstack.adapters +import os + +# release detection is done via keystone package given that +# openstack-origin is not present in the subordinate charm +# see https://github.com/juju/charm-helpers/issues/83 +import charmhelpers.core.unitdata as unitdata +from charms_openstack.charm.core import ( + register_os_release_selector +) +OPENSTACK_RELEASE_KEY = 'charmers.openstack-release-version' DOMAIN_CONF = "/etc/keystone/domains/keystone.{}.conf" KEYSTONE_CONF_TEMPLATE = "keystone.conf" +@register_os_release_selector +def select_release(): + """Determine the release based on the keystone package version. + + Note that this function caches the release after the first install so + that it doesn't need to keep going and getting it from the package + information. + """ + release_version = unitdata.kv().get(OPENSTACK_RELEASE_KEY, None) + if release_version is None: + release_version = os_utils.os_release('keystone') + unitdata.kv().set(OPENSTACK_RELEASE_KEY, release_version) + return release_version + + class KeystoneLDAPConfigurationAdapter( charms_openstack.adapters.ConfigurationAdapter): '''Charm specific configuration adapter to deal with ldap @@ -66,7 +91,8 @@ class KeystoneLDAPCharm(charms_openstack.charm.OpenStackCharm): """ return hookenv.config('domain-name') or hookenv.service_name() - def configuration_complete(self): + @staticmethod + def configuration_complete(): """Determine whether sufficient configuration has been provided to configure keystone for use with a LDAP backend @@ -98,38 +124,20 @@ class KeystoneLDAPCharm(charms_openstack.charm.OpenStackCharm): def render_config(self, restart_trigger): """Render the domain specific LDAP configuration for the application """ - checksum = ch_host.path_hash(self.configuration_file) + checksum = ch_host.file_hash(self.configuration_file) core.templating.render( source=KEYSTONE_CONF_TEMPLATE, template_loader=os_templating.get_loader( 'templates/', self.release), target=self.configuration_file, context=self.adapters_instance) - if checksum != ch_host.path_hash(self.configuration_file): + if checksum != ch_host.file_hash(self.configuration_file): restart_trigger() - -def render_config(restart_trigger): - """Render the configuration for the charm - - :params: restart_trigger: function to call if configuration file - changed as a result of rendering - """ - KeystoneLDAPCharm.singleton.render_config(restart_trigger) - - -def assess_status(): - """Just call the KeystoneLDAPCharm.singleton.assess_status() command - to update status on the unit. - """ - KeystoneLDAPCharm.singleton.assess_status() - - -def configuration_complete(): - """Determine whether charm configuration is actually complete""" - return KeystoneLDAPCharm.singleton.configuration_complete() - - -def configuration_file(): - """Configuration file for current domain configuration""" - return KeystoneLDAPCharm.singleton.configuration_file + def remove_config(self): + """ + Remove the domain-specific LDAP configuration file and trigger + keystone restart. + """ + if os.path.exists(self.configuration_file): + os.unlink(self.configuration_file) diff --git a/src/reactive/keystone_ldap_handlers.py b/src/reactive/keystone_ldap_handlers.py index fcfb232..1375cd2 100644 --- a/src/reactive/keystone_ldap_handlers.py +++ b/src/reactive/keystone_ldap_handlers.py @@ -13,10 +13,13 @@ # See the License for the specific language governing permissions and # limitations under the License. +# import to trigger openstack charm metaclass init +import charm.openstack.keystone_ldap # noqa + import charms_openstack.charm as charm import charms.reactive as reactive -import charm.openstack.keystone_ldap as keystone_ldap # noqa +import charms.reactive.flags as flags import charmhelpers.core.hookenv as hookenv @@ -24,35 +27,54 @@ charm.use_defaults( 'charm.installed', 'update-status') +# if config has been changed we need to re-evaluate flags +# config.changed is set and cleared (atexit) in layer-basic +flags.register_trigger(when='config.changed', + clear_flag='config.rendered') +flags.register_trigger(when='config.changed', + clear_flag='config.complete') + @reactive.when('domain-backend.connected') @reactive.when_not('domain-name-configured') @reactive.when('config.complete') def configure_domain_name(domain): - keystone_ldap.render_config(domain.trigger_restart) domain.domain_name(hookenv.config('domain-name') or hookenv.service_name()) - reactive.set_state('domain-name-configured') + flags.set_flag('domain-name-configured') @reactive.when_not('domain-backend.connected') @reactive.when('domain-name-configured') -def clear_domain_name_configured(*args): - reactive.remove_state('domain-name-configured') +def keystone_departed(): + """ + Service restart should be handled on the keystone side + in this case. + """ + flags.clear_flag('domain-name-configured') + with charm.provide_charm_instance() as kldap_charm: + kldap_charm.remove_config() + + +@reactive.when('domain-backend.connected') +@reactive.when_not('config.complete') +def config_changed(domain): + with charm.provide_charm_instance() as kldap_charm: + if kldap_charm.configuration_complete(): + flags.set_flag('config.complete') @reactive.when('domain-backend.connected') @reactive.when('domain-name-configured') @reactive.when('config.complete') -def config_changed(domain): - keystone_ldap.render_config(domain.trigger_restart) +@reactive.when_not('config.rendered') +def render_config(domain): + with charm.provide_charm_instance() as kldap_charm: + kldap_charm.render_config(domain.trigger_restart) + flags.set_flag('config.rendered') @reactive.when_not('always.run') -def check_configuration(): - '''Validate required configuration options at set state''' - if keystone_ldap.configuration_complete(): - reactive.set_state('config.complete') - else: - reactive.remove_state('config.complete') - keystone_ldap.assess_status() +def assess_status(): + with charm.provide_charm_instance() as kldap_charm: + kldap_charm.assess_status() diff --git a/unit_tests/test_keystone_ldap_handlers.py b/unit_tests/test_keystone_ldap_handlers.py index d86ed93..b3018ce 100644 --- a/unit_tests/test_keystone_ldap_handlers.py +++ b/unit_tests/test_keystone_ldap_handlers.py @@ -32,15 +32,18 @@ class TestRegisteredHooks(test_utils.TestRegisteredHooks): 'when': { 'configure_domain_name': ('domain-backend.connected', 'config.complete'), - 'clear_domain_name_configured': ('domain-name-configured', ), - 'config_changed': ('domain-backend.connected', - 'config.complete', - 'domain-name-configured'), + 'keystone_departed': ('domain-name-configured',), + 'config_changed': ('domain-backend.connected',), + 'render_config': ('config.complete', + 'domain-backend.connected', + 'domain-name-configured'), }, 'when_not': { - 'check_configuration': ('always.run', ), - 'configure_domain_name': ('domain-name-configured', ), - 'clear_domain_name_configured': ('domain-backend.connected', ), + 'assess_status': ('always.run',), + 'configure_domain_name': ('domain-name-configured',), + 'keystone_departed': ('domain-backend.connected',), + 'config_changed': ('config.complete',), + 'render_config': ('config.rendered',), } } # test that the hooks were registered via the @@ -50,62 +53,93 @@ class TestRegisteredHooks(test_utils.TestRegisteredHooks): class TestKeystoneLDAPCharmHandlers(test_utils.PatchHelper): - def patch(self, obj, attr, return_value=None, side_effect=None): - mocked = mock.patch.object(obj, attr) - self._patches[attr] = mocked - started = mocked.start() - started.return_value = return_value - started.side_effect = side_effect - self._patches_start[attr] = started - setattr(self, attr, started) + def _patch_provide_charm_instance(self): + kldap_charm = mock.MagicMock() + self.patch('charms_openstack.charm.provide_charm_instance', + name='provide_charm_instance', + new=mock.MagicMock()) + self.provide_charm_instance().__enter__.return_value = kldap_charm + self.provide_charm_instance().__exit__.return_value = None + return kldap_charm def test_configure_domain_name_application(self): - self.patch(handlers.keystone_ldap, 'render_config') - self.patch(handlers.hookenv, 'config') - self.patch(handlers.hookenv, 'service_name') - self.patch(handlers.reactive, 'set_state') + self.patch_object(handlers.hookenv, 'config') self.config.return_value = None + + self.patch_object(handlers.hookenv, 'service_name') self.service_name.return_value = 'keystone-ldap' + + self.patch_object(handlers.flags, 'set_flag') + domain = mock.MagicMock() + handlers.configure_domain_name(domain) - self.render_config.assert_called_with( - domain.trigger_restart - ) + domain.domain_name.assert_called_with( 'keystone-ldap' ) - self.set_state.assert_called_once_with('domain-name-configured') + self.set_flag.assert_called_once_with('domain-name-configured') - def test_clear_domain_name_configured(self): - self.patch(handlers.reactive, 'remove_state') - domain = mock.MagicMock() - handlers.clear_domain_name_configured(domain) - self.remove_state.assert_called_once_with('domain-name-configured') + def test_keystone_departed(self): + kldap_charm = self._patch_provide_charm_instance() + self.patch_object(kldap_charm, 'remove_config') + + self.patch_object(handlers.flags, 'clear_flag') + + handlers.keystone_departed() + + self.clear_flag.assert_called_once_with('domain-name-configured') + + kldap_charm.remove_config.assert_called_once() def test_configure_domain_name_config(self): - self.patch(handlers.keystone_ldap, 'render_config') - self.patch(handlers.hookenv, 'config') - self.patch(handlers.hookenv, 'service_name') + self.patch_object(handlers.hookenv, 'config') self.config.return_value = 'mydomain' - self.service_name.return_value = 'keystone-ldap' + domain = mock.MagicMock() + handlers.configure_domain_name(domain) - self.render_config.assert_called_with( - domain.trigger_restart - ) + domain.domain_name.assert_called_with( 'mydomain' ) - def test_check_configuration(self): - self.patch(handlers.keystone_ldap, 'configuration_complete') - self.patch(handlers.reactive, 'set_state') - self.patch(handlers.reactive, 'remove_state') - self.patch(handlers.keystone_ldap, 'assess_status') - self.configuration_complete.return_value = True - handlers.check_configuration() - self.set_state.assert_called_with('config.complete') - self.configuration_complete.return_value = False - handlers.check_configuration() - self.remove_state.assert_called_with('config.complete') - self.assertTrue(self.assess_status.called) + def test_config_changed(self): + kldap_charm = self._patch_provide_charm_instance() + self.patch_object(kldap_charm, 'render_config') + + # assume that configuration is complete to test config.rendered + kldap_charm.configuration_complete.return_value = True + + self.patch_object(handlers.flags, 'set_flag') + + domain = mock.MagicMock() + + handlers.config_changed(domain) + + self.set_flag.assert_called_once_with('config.complete') + self.render_config.assert_not_called() + + def test_render_config(self): + kldap_charm = self._patch_provide_charm_instance() + self.patch_object(kldap_charm, 'render_config') + + self.patch_object(handlers.flags, 'set_flag') + + domain = mock.MagicMock() + + handlers.render_config(domain) + + self.set_flag.assert_called_once_with('config.rendered') + + kldap_charm.render_config.assert_called_with( + domain.trigger_restart + ) + + def test_assess_status(self): + kldap_charm = self._patch_provide_charm_instance() + self.patch_object(kldap_charm, 'assess_status') + + handlers.assess_status() + + kldap_charm.assess_status.assert_called_once() diff --git a/unit_tests/test_lib_charm_openstack_keystone_ldap.py b/unit_tests/test_lib_charm_openstack_keystone_ldap.py index 0adf1c0..d63efc1 100644 --- a/unit_tests/test_lib_charm_openstack_keystone_ldap.py +++ b/unit_tests/test_lib_charm_openstack_keystone_ldap.py @@ -12,35 +12,20 @@ from __future__ import absolute_import from __future__ import print_function -import unittest - import mock -from charms_openstack.test_mocks import charmhelpers as ch -ch.contrib.openstack.utils.OPENSTACK_RELEASES = ('mitaka', ) +import charms_openstack.test_utils as test_utils + import charm.openstack.keystone_ldap as keystone_ldap +from charms_openstack.charm import provide_charm_instance -class Helper(unittest.TestCase): + +class Helper(test_utils.PatchHelper): def setUp(self): - self._patches = {} - self._patches_start = {} - - def tearDown(self): - for k, v in self._patches.items(): - v.stop() - setattr(self, k, None) - self._patches = None - self._patches_start = None - - def patch(self, obj, attr, return_value=None, **kwargs): - mocked = mock.patch.object(obj, attr, **kwargs) - self._patches[attr] = mocked - started = mocked.start() - started.return_value = return_value - self._patches_start[attr] = started - setattr(self, attr, started) + super().setUp() + self.patch_release(keystone_ldap.KeystoneLDAPCharm.release) class TestKeystoneLDAPCharm(Helper): @@ -59,15 +44,17 @@ class TestKeystoneLDAPCharm(Helper): return reply.get(key) return reply config.side_effect = mock_config - self.assertTrue(keystone_ldap.configuration_complete()) - for required_config in reply: - orig = reply[required_config] - reply[required_config] = None - self.assertFalse(keystone_ldap.configuration_complete()) - reply[required_config] = orig + with provide_charm_instance() as kldap_charm: + self.assertTrue(kldap_charm.configuration_complete()) - self.assertTrue(keystone_ldap.configuration_complete()) + for required_config in reply: + orig = reply[required_config] + reply[required_config] = None + self.assertFalse(kldap_charm.configuration_complete()) + reply[required_config] = orig + + self.assertTrue(kldap_charm.configuration_complete()) @mock.patch('charmhelpers.core.hookenv.service_name') @mock.patch('charmhelpers.core.hookenv.config') @@ -75,18 +62,20 @@ class TestKeystoneLDAPCharm(Helper): service_name): config.return_value = None service_name.return_value = 'testdomain' - charm = keystone_ldap.KeystoneLDAPCharm() - self.assertEqual('testdomain', - charm.domain_name) - self.assertEqual('/etc/keystone/domains/keystone.testdomain.conf', - charm.configuration_file) - config.assert_called_with('domain-name') + with provide_charm_instance() as kldap_charm: + self.assertEqual('testdomain', + kldap_charm.domain_name) + self.assertEqual( + '/etc/keystone/domains/keystone.testdomain.conf', + kldap_charm.configuration_file) + config.assert_called_with('domain-name') - config.return_value = 'userdomain' - self.assertEqual('userdomain', - charm.domain_name) - self.assertEqual('/etc/keystone/domains/keystone.userdomain.conf', - charm.configuration_file) + config.return_value = 'userdomain' + self.assertEqual('userdomain', + kldap_charm.domain_name) + self.assertEqual( + '/etc/keystone/domains/keystone.userdomain.conf', + kldap_charm.configuration_file) @mock.patch('charmhelpers.contrib.openstack.utils.snap_install_requested') @mock.patch('charmhelpers.core.hookenv.config') @@ -110,25 +99,27 @@ class TestKeystoneLDAPCharm(Helper): config.side_effect = mock_config snap_install_requested.return_value = False - # Check that active status is set correctly - keystone_ldap.assess_status() - status_set.assert_called_with('active', mock.ANY) - application_version_set.assert_called_with( - keystone_ldap.KeystoneLDAPCharm.singleton.application_version - ) - # Check that blocked status is set correctly - reply['ldap-server'] = None - keystone_ldap.assess_status() - status_set.assert_called_with('blocked', mock.ANY) - application_version_set.assert_called_with( - keystone_ldap.KeystoneLDAPCharm.singleton.application_version - ) + with provide_charm_instance() as kldap_charm: + # Check that active status is set correctly + kldap_charm.assess_status() + status_set.assert_called_with('active', mock.ANY) + application_version_set.assert_called_with( + kldap_charm.application_version + ) + + # Check that blocked status is set correctly + reply['ldap-server'] = None + kldap_charm.assess_status() + status_set.assert_called_with('blocked', mock.ANY) + application_version_set.assert_called_with( + kldap_charm.application_version + ) @mock.patch('charmhelpers.core.hookenv.config') def test_render_config(self, config): - self.patch(keystone_ldap.ch_host, 'path_hash') - self.patch(keystone_ldap.core.templating, 'render') + self.patch_object(keystone_ldap.ch_host, 'file_hash') + self.patch_object(keystone_ldap.core.templating, 'render') reply = { 'ldap-server': 'myserver', @@ -144,24 +135,54 @@ class TestKeystoneLDAPCharm(Helper): return reply config.side_effect = mock_config - self.path_hash.side_effect = ['aaa', 'aaa'] + self.file_hash.side_effect = ['aaa', 'aaa'] mock_trigger = mock.MagicMock() - # Ensure a basic level of function from render_config - keystone_ldap.render_config(mock_trigger) - self.render.assert_called_with( - source=keystone_ldap.KEYSTONE_CONF_TEMPLATE, - template_loader=mock.ANY, - target='/etc/keystone/domains/keystone.userdomain.conf', - context=mock.ANY - ) - self.assertFalse(mock_trigger.called) + with provide_charm_instance() as kldap_charm: + # Ensure a basic level of function from render_config + kldap_charm.render_config(mock_trigger) + self.render.assert_called_with( + source=keystone_ldap.KEYSTONE_CONF_TEMPLATE, + template_loader=mock.ANY, + target='/etc/keystone/domains/keystone.userdomain.conf', + context=mock.ANY + ) + self.assertFalse(mock_trigger.called) - # Ensure that change in file contents results in call - # to restart trigger function passed to render_config - self.path_hash.side_effect = ['aaa', 'bbb'] - keystone_ldap.render_config(mock_trigger) - self.assertTrue(mock_trigger.called) + # Ensure that change in file contents results in call + # to restart trigger function passed to render_config + self.file_hash.side_effect = ['aaa', 'bbb'] + kldap_charm.render_config(mock_trigger) + self.assertTrue(mock_trigger.called) + + @mock.patch('charmhelpers.core.hookenv.config') + @mock.patch('os.path.exists') + @mock.patch('os.unlink') + def test_remove_config(self, unlink, exists, config): + exists.return_value = True + + self.patch_object(keystone_ldap.ch_host, 'file_hash') + + reply = { + 'ldap-server': 'myserver', + 'ldap-user': 'myusername', + 'ldap-password': 'mypassword', + 'ldap-suffix': 'suffix', + 'domain-name': 'userdomain', + } + + def mock_config(key=None): + if key: + return reply.get(key) + return reply + config.side_effect = mock_config + + with provide_charm_instance() as kldap_charm: + # Ensure a basic level of function from render_config + cf = keystone_ldap.DOMAIN_CONF.format(reply['domain-name']) + kldap_charm.remove_config() + exists.assert_called_once_with(cf) + unlink.assert_called_once_with(cf) class TestKeystoneLDAPAdapters(Helper):