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

@@ -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

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 remove_config(self):
-def render_config(restart_trigger):
+        """
-    """Render the configuration for the charm
+        Remove the domain-specific LDAP configuration file and trigger
-
+        keystone restart.
-    :params: restart_trigger: function to call if configuration file
+        """
-                              changed as a result of rendering
+        if os.path.exists(self.configuration_file):
-    """
+            os.unlink(self.configuration_file)
-    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

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):
+def keystone_departed():
-    reactive.remove_state('domain-name-configured')
+    """
+    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):
+@reactive.when_not('config.rendered')
-    keystone_ldap.render_config(domain.trigger_restart)
+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():
+def assess_status():
-    '''Validate required configuration options at set state'''
+    with charm.provide_charm_instance() as kldap_charm:
-    if keystone_ldap.configuration_complete():
+        kldap_charm.assess_status()
-        reactive.set_state('config.complete')
-    else:
-        reactive.remove_state('config.complete')
-    keystone_ldap.assess_status()

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', ),
+                'keystone_departed': ('domain-name-configured',),
-                'config_changed': ('domain-backend.connected',
+                'config_changed': ('domain-backend.connected',),
-                                   'config.complete',
+                'render_config': ('config.complete',
-                                   'domain-name-configured'),
+                                  'domain-backend.connected',
+                                  'domain-name-configured'),
             },
             'when_not': {
-                'check_configuration': ('always.run', ),
+                'assess_status': ('always.run',),
-                'configure_domain_name': ('domain-name-configured', ),
+                'configure_domain_name': ('domain-name-configured',),
-                'clear_domain_name_configured': ('domain-backend.connected', ),
+                '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):
+    def _patch_provide_charm_instance(self):
-        mocked = mock.patch.object(obj, attr)
+        kldap_charm = mock.MagicMock()
-        self._patches[attr] = mocked
+        self.patch('charms_openstack.charm.provide_charm_instance',
-        started = mocked.start()
+                   name='provide_charm_instance',
-        started.return_value = return_value
+                   new=mock.MagicMock())
-        started.side_effect = side_effect
+        self.provide_charm_instance().__enter__.return_value = kldap_charm
-        self._patches_start[attr] = started
+        self.provide_charm_instance().__exit__.return_value = None
-        setattr(self, attr, started)
+        return kldap_charm
 
     def test_configure_domain_name_application(self):
-        self.patch(handlers.keystone_ldap, 'render_config')
+        self.patch_object(handlers.hookenv, 'config')
-        self.patch(handlers.hookenv, 'config')
-        self.patch(handlers.hookenv, 'service_name')
-        self.patch(handlers.reactive, 'set_state')
         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):
+    def test_keystone_departed(self):
-        self.patch(handlers.reactive, 'remove_state')
+        kldap_charm = self._patch_provide_charm_instance()
-        domain = mock.MagicMock()
+        self.patch_object(kldap_charm, 'remove_config')
-        handlers.clear_domain_name_configured(domain)
+
-        self.remove_state.assert_called_once_with('domain-name-configured')
+        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_object(handlers.hookenv, 'config')
-        self.patch(handlers.hookenv, 'config')
-        self.patch(handlers.hookenv, 'service_name')
         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):
+    def test_config_changed(self):
-        self.patch(handlers.keystone_ldap, 'configuration_complete')
+        kldap_charm = self._patch_provide_charm_instance()
-        self.patch(handlers.reactive, 'set_state')
+        self.patch_object(kldap_charm, 'render_config')
-        self.patch(handlers.reactive, 'remove_state')
+
-        self.patch(handlers.keystone_ldap, 'assess_status')
+        # assume that configuration is complete to test config.rendered
-        self.configuration_complete.return_value = True
+        kldap_charm.configuration_complete.return_value = True
-        handlers.check_configuration()
+
-        self.set_state.assert_called_with('config.complete')
+        self.patch_object(handlers.flags, 'set_flag')
-        self.configuration_complete.return_value = False
+
-        handlers.check_configuration()
+        domain = mock.MagicMock()
-        self.remove_state.assert_called_with('config.complete')
+
-        self.assertTrue(self.assess_status.called)
+        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()

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
+import charms_openstack.test_utils as test_utils
-ch.contrib.openstack.utils.OPENSTACK_RELEASES = ('mitaka', )
+
 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 = {}
+        super().setUp()
-        self._patches_start = {}
+        self.patch_release(keystone_ldap.KeystoneLDAPCharm.release)
-
-    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)
 
 
 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:
+        with provide_charm_instance() as kldap_charm:
-            orig = reply[required_config]
+            self.assertTrue(kldap_charm.configuration_complete())
-            reply[required_config] = None
-            self.assertFalse(keystone_ldap.configuration_complete())
-            reply[required_config] = orig
 
-        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()
+        with provide_charm_instance() as kldap_charm:
-        self.assertEqual('testdomain',
+            self.assertEqual('testdomain',
-                         charm.domain_name)
+                             kldap_charm.domain_name)
-        self.assertEqual('/etc/keystone/domains/keystone.testdomain.conf',
+            self.assertEqual(
-                         charm.configuration_file)
+                '/etc/keystone/domains/keystone.testdomain.conf',
-        config.assert_called_with('domain-name')
+                kldap_charm.configuration_file)
+            config.assert_called_with('domain-name')
 
-        config.return_value = 'userdomain'
+            config.return_value = 'userdomain'
-        self.assertEqual('userdomain',
+            self.assertEqual('userdomain',
-                         charm.domain_name)
+                             kldap_charm.domain_name)
-        self.assertEqual('/etc/keystone/domains/keystone.userdomain.conf',
+            self.assertEqual(
-                         charm.configuration_file)
+                '/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
+        with provide_charm_instance() as kldap_charm:
-        reply['ldap-server'] = None
+            # Check that active status is set correctly
-        keystone_ldap.assess_status()
+            kldap_charm.assess_status()
-        status_set.assert_called_with('blocked', mock.ANY)
+            status_set.assert_called_with('active', mock.ANY)
-        application_version_set.assert_called_with(
+            application_version_set.assert_called_with(
-            keystone_ldap.KeystoneLDAPCharm.singleton.application_version
+                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_object(keystone_ldap.ch_host, 'file_hash')
-        self.patch(keystone_ldap.core.templating, 'render')
+        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
+        with provide_charm_instance() as kldap_charm:
-        keystone_ldap.render_config(mock_trigger)
+            # Ensure a basic level of function from render_config
-        self.render.assert_called_with(
+            kldap_charm.render_config(mock_trigger)
-            source=keystone_ldap.KEYSTONE_CONF_TEMPLATE,
+            self.render.assert_called_with(
-            template_loader=mock.ANY,
+                source=keystone_ldap.KEYSTONE_CONF_TEMPLATE,
-            target='/etc/keystone/domains/keystone.userdomain.conf',
+                template_loader=mock.ANY,
-            context=mock.ANY
+                target='/etc/keystone/domains/keystone.userdomain.conf',
-        )
+                context=mock.ANY
-        self.assertFalse(mock_trigger.called)
+            )
+            self.assertFalse(mock_trigger.called)
 
-        # Ensure that change in file contents results in call
+            # Ensure that change in file contents results in call
-        # to restart trigger function passed to render_config
+            # to restart trigger function passed to render_config
-        self.path_hash.side_effect = ['aaa', 'bbb']
+            self.file_hash.side_effect = ['aaa', 'bbb']
-        keystone_ldap.render_config(mock_trigger)
+            kldap_charm.render_config(mock_trigger)
-        self.assertTrue(mock_trigger.called)
+            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):