Standardize reactive practices

Rely on updated interfaces.
Use standard practices for endpoint handling.
Remove unused code.

.gitignore

@@ -4,3 +4,5 @@
 *.pyc
 build
 src/tests/bundles/overlays/local-charm-overlay.yaml.j2
+interfaces
+layers

src/lib/charm/openstack/keystone_saml_mellon.py

@@ -16,14 +16,16 @@
 import charmhelpers.core as core
 import charmhelpers.core.host as ch_host
 import charmhelpers.core.hookenv as hookenv
-import charmhelpers.core.unitdata as unitdata
 
 import charmhelpers.contrib.openstack.templating as os_templating
-import charmhelpers.contrib.openstack.utils as os_utils
 
 import charms_openstack.charm
 import charms_openstack.adapters
 
+from charms.reactive.relations import (
+    endpoint_from_flag,
+)
+
 import os
 import subprocess
 
@@ -31,14 +33,6 @@ from lxml import etree
 from cryptography.hazmat.backends import default_backend
 from cryptography.hazmat.primitives import serialization
 
-# 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
-from charms_openstack.charm.core import (
-    register_os_release_selector
-)
-OPENSTACK_RELEASE_KEY = 'charmers.openstack-release-version'
-
 CONFIGS = (IDP_METADATA, SP_METADATA, SP_PRIVATE_KEY,
            SP_LOCATION_CONFIG,) = [
                os.path.join('/etc/apache2/mellon',
@@ -48,20 +42,7 @@ CONFIGS = (IDP_METADATA, SP_METADATA, SP_PRIVATE_KEY,
                                 'sp-pk.{}.pem',
                                 'sp-location.{}.conf']]
 
-
+KEYSTONE_FID_ENDPOINT = "keystone-fid-service-provider.connected"
-@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 KeystoneSAMLMellonConfigurationAdapter(
@@ -73,6 +54,7 @@ class KeystoneSAMLMellonConfigurationAdapter(
         self._sp_private_key = None
         self._sp_signing_keyinfo = None
         self._validation_errors = {}
+        self._fid_data = self.get_fid_data()
 
     @property
     def validation_errors(self):
@@ -101,17 +83,24 @@ class KeystoneSAMLMellonConfigurationAdapter(
     def sp_location_config(self):
         return SP_LOCATION_CONFIG
 
+    def get_fid_data(self):
+        fid_sp = endpoint_from_flag(KEYSTONE_FID_ENDPOINT)
+        if fid_sp:
+            return fid_sp.all_joined_units.received
+        else:
+            return {}
+
     @property
     def keystone_host(self):
-        return unitdata.kv().get('hostname')
+        return self.get_fid_data().get("hostname")
 
     @property
     def keystone_port(self):
-        return unitdata.kv().get('port')
+        return self.get_fid_data().get("port")
 
     @property
     def tls_enabled(self):
-        return unitdata.kv().get('tls-enabled')
+        return self.get_fid_data().get("tls-enabled")
 
     @property
     def keystone_base_url(self):
@@ -256,6 +245,13 @@ class KeystoneSAMLMellonCharm(charms_openstack.charm.OpenStackCharm):
     # First release supported
     release = 'mitaka'
 
+    release_pkg = 'keystone-common'
+
+    # Required relations
+    required_relations = [
+        'keystone-fid-service-provider',
+        'websso-fid-service-provider']
+
     # List of packages to install for this charm
     packages = ['libapache2-mod-auth-mellon']
 
@@ -275,6 +271,13 @@ class KeystoneSAMLMellonCharm(charms_openstack.charm.OpenStackCharm):
     # ownership.
     group = 'www-data'
 
+    @property
+    def restart_map(self):
+        _map = {}
+        for config in CONFIGS:
+            _map[config] = []
+        return _map
+
     def configuration_complete(self):
         """Determine whether sufficient configuration has been provided
         via charm config options and resources.
@@ -292,19 +295,20 @@ class KeystoneSAMLMellonCharm(charms_openstack.charm.OpenStackCharm):
 
         return all(required_config.values())
 
-    def assess_status(self):
+    def custom_assess_status_check(self):
-        """Determine the current application status for the charm"""
+        """Custom asses status.
-        hookenv.application_version_set(self.application_version)
+
+        Check the configuration is complete.
+        """
         if not self.configuration_complete():
             errors = [
                 '{}: {}'.format(k, v)
                 for k, v in self.options.validation_errors.items() if v]
             status_msg = 'Configuration is incomplete. {}'.format(
                 ','.join(errors))
-            hookenv.status_set('blocked', status_msg)
+            return 'blocked', status_msg
-        else:
+        # Nothing to report
-            hookenv.status_set('active',
+        return None, None
-                               'Unit is ready')
 
     def render_config(self):
         """

src/reactive/keystone_saml_mellon_handlers.py

@@ -13,16 +13,11 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import uuid
-
 # import to trigger openstack charm metaclass init
-import charm.openstack.keystone_saml_mellon # noqa
+import charm.openstack.keystone_saml_mellon as keystone_saml_mellon  # noqa
 
 import charms_openstack.charm as charm
 import charms.reactive as reactive
-import charms.reactive.flags as flags
-
-import charmhelpers.core.unitdata as unitdata
 
 from charms.reactive.relations import (
     endpoint_from_flag,
@@ -30,34 +25,11 @@ from charms.reactive.relations import (
 
 charm.use_defaults(
     'charm.installed',
-    'update-status')
+    'update-status',
-
+    'upgrade-charm')
-# 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='upgraded', clear_flag='config.rendered')
-flags.register_trigger(when='config.changed',
-                       clear_flag='config.complete')
-flags.register_trigger(
-    when='endpoint.keystone-fid-service-provider.changed',
-    clear_flag='keystone-data.complete'
-)
 
 
-@reactive.hook('upgrade-charm')
+@reactive.when_not('keystone-fid-service-provider.connected')
-def default_upgrade_charm():
-    """Default handler for the 'upgrade-charm' hook.
-    This calls the charm.singleton.upgrade_charm() function as a default.
-    """
-    reactive.set_state('upgraded')
-
-
-# clear the upgraded state once config.rendered is set again
-flags.register_trigger(when='config.rendered', clear_flag='upgraded')
-
-
-@reactive.when_not('endpoint.keystone-fid-service-provider.joined')
 def keystone_departed():
     """
     Service restart should be handled on the keystone side
@@ -67,69 +39,46 @@ def keystone_departed():
         charm_instance.remove_config()
 
 
-@reactive.when('endpoint.keystone-fid-service-provider.joined')
+@reactive.when('keystone-fid-service-provider.connected')
-@reactive.when_not('config.complete')
+def publish_sp_fid():
-def config_changed():
-    with charm.provide_charm_instance() as charm_instance:
-        if charm_instance.configuration_complete():
-            flags.set_flag('config.complete')
-
-
-@reactive.when('endpoint.keystone-fid-service-provider.joined')
-@reactive.when_not('keystone-data.complete')
-def keystone_data_changed(fid_sp):
-    primary_data = fid_sp.all_joined_units[0].received
-    if primary_data:
-        hostname = primary_data.get('hostname')
-        port = primary_data.get('port')
-        tls_enabled = primary_data.get('tls-enabled')
-        # a basic check on the fact that keystone provided us with
-        # hostname and port information
-        if hostname and port:
-            # save hostname and port data in local storage for future
-            # use - in case config is incomplete but a relation is
-            # we need to store this across charm hook invocations
-            unitdb = unitdata.kv()
-            unitdb.set('hostname', hostname)
-            unitdb.set('port', port)
-            unitdb.set('tls-enabled', tls_enabled)
-            flags.set_flag('keystone-data.complete')
-
-
-@reactive.when('endpoint.keystone-fid-service-provider.joined')
-@reactive.when('config.complete')
-@reactive.when('keystone-data.complete')
-@reactive.when_not('config.rendered')
-def render_config():
     # don't always have a relation context - obtain from the flag
     fid_sp = endpoint_from_flag(
-        'endpoint.keystone-fid-service-provider.joined')
+        keystone_saml_mellon.KEYSTONE_FID_ENDPOINT)
     with charm.provide_charm_instance() as charm_instance:
-        charm_instance.render_config()
+        fid_sp.publish(charm_instance.options.protocol_name,
-        flags.set_flag('config.rendered')
-        # Trigger keystone restart. The relation is container-scoped
-        # so a per-unit db of a remote unit will only contain a nonce
-        # of a single subordinate
-        restart_nonce = str(uuid.uuid4())
-        fid_sp.publish(restart_nonce,
-                       charm_instance.options.protocol_name,
                        charm_instance.options.remote_id_attribute)
 
 
-@reactive.when('endpoint.websso-fid-service-provider.joined')
+@reactive.when('keystone-fid-service-provider.available')
-@reactive.when('config.complete')
+def render_config():
-@reactive.when('keystone-data.complete')
+    # don't always have a relation context - obtain from the flag
-@reactive.when('config.rendered')
+    fid_sp = endpoint_from_flag(
+        keystone_saml_mellon.KEYSTONE_FID_ENDPOINT)
+    with charm.provide_charm_instance() as charm_instance:
+        if charm_instance.configuration_complete():
+            print("COMPLETE")
+            charm_instance.render_config()
+            # Trigger keystone restart. The relation is container-scoped
+            # so a per-unit db of a remote unit will only contain a nonce
+            # of a single subordinate
+            print("CHECK_anyfile")
+            if reactive.any_file_changed(keystone_saml_mellon.CONFIGS):
+                print("TRUE_anyfile")
+                fid_sp.request_restart()
+
+
+@reactive.when('websso-fid-service-provider.connected')
 def configure_websso():
     # don't always have a relation context - obtain from the flag
     websso_fid_sp = endpoint_from_flag(
-        'endpoint.websso-fid-service-provider.joined')
+        'websso-fid-service-provider.connected')
     with charm.provide_charm_instance() as charm_instance:
-        # publish config options for all remote units of a given rel
+        if charm_instance.configuration_complete():
-        options = charm_instance.options
+            # publish config options for all remote units of a given rel
-        websso_fid_sp.publish(options.protocol_name,
+            options = charm_instance.options
-                              options.idp_name,
+            websso_fid_sp.publish(options.protocol_name,
-                              options.user_facing_name)
+                                  options.idp_name,
+                                  options.user_facing_name)
 
 
 @reactive.when_not('always.run')

unit_tests/test_keystone_saml_mellon_handlers.py

@@ -34,26 +34,16 @@ class TestRegisteredHooks(test_utils.TestRegisteredHooks):
                 'default_upgrade_charm': ('upgrade-charm',),
             },
             'when': {
+                'publish_sp_fid': (
+                    'keystone-fid-service-provider.connected',),
                 'render_config': (
-                    'endpoint.keystone-fid-service-provider.joined',
+                    'keystone-fid-service-provider.available',),
-                    'config.complete',
-                    'keystone-data.complete',),
-                'config_changed': (
-                    'endpoint.keystone-fid-service-provider.joined',),
-                'keystone_data_changed': (
-                    'endpoint.keystone-fid-service-provider.joined',),
                 'configure_websso': (
-                    'endpoint.websso-fid-service-provider.joined',
+                    'websso-fid-service-provider.connected',),
-                    'config.complete',
-                    'keystone-data.complete',
-                    'config.rendered',),
             },
             'when_not': {
-                'config_changed': ('config.complete',),
                 'keystone_departed': (
-                    'endpoint.keystone-fid-service-provider.joined',),
+                    'keystone-fid-service-provider.connected',),
-                'keystone_data_changed': ('keystone-data.complete',),
-                'render_config': ('config.rendered',),
                 'assess_status': ('always.run',),
             },
         }
@@ -75,16 +65,8 @@ class TestKeystoneSAMLMellonHandlers(test_utils.PatchHelper):
             self.keystone_saml_mellon_charm)
         self.provide_charm_instance().__exit__.return_value = None
 
-        self.patch_object(handlers, 'flags')
+        self.patch_object(handlers.reactive, 'any_file_changed',
-
-        self.uuid = 'uuid-uuid'
-        self.patch_object(handlers.uuid, 'uuid4')
-        self.uuid4.return_value = self.uuid
-
-        self.patch_object(handlers, 'unitdata',
                           new=mock.MagicMock())
-        self.kv = mock.MagicMock()
-        self.unitdata.kv.return_value = self.kv
 
         self.patch_object(handlers, 'endpoint_from_flag',
                           new=mock.MagicMock())
@@ -116,30 +98,25 @@ class TestKeystoneSAMLMellonHandlers(test_utils.PatchHelper):
         handlers.keystone_departed()
         self.keystone_saml_mellon_charm.remove_config.assert_called_once_with()
 
-    def test_keystone_data_changed(self):
+    def test_publish_sp_fid(self):
-        kv_set_calls = [
+        handlers.publish_sp_fid()
-            mock.call("tls-enabled", True),
+        self.endpoint.publish.assert_called_once_with(
-            mock.call("port", "5000"),
+            self.protocol_name, self.remote_id_attribute)
-            mock.call("hostname", "keystone-0"),
-        ]
-
-        handlers.keystone_data_changed(self.endpoint)
-
-        self.kv.set.has_calls(kv_set_calls)
-        self.flags.set_flag.assert_called_once_with('keystone-data.complete')
 
     def test_render_config(self):
+        # No restart
+        self.any_file_changed.return_value = False
+        (self.keystone_saml_mellon_charm
+            .configuration_complete.return_value) = True
+
         handlers.render_config()
         self.keystone_saml_mellon_charm.render_config.assert_called_once_with()
-        self.flags.set_flag.assert_called_once_with('config.rendered')
+        self.endpoint.request_restart.assert_not_called()
-        self.endpoint.publish.assert_called_once_with(
-            self.uuid, self.protocol_name, self.remote_id_attribute)
 
-    def test_config_changed(self):
+        # Restart
-        handlers.config_changed()
+        self.any_file_changed.return_value = True
-        (self.keystone_saml_mellon_charm.configuration_complete
+        handlers.render_config()
-            .return_value) = True
+        self.endpoint.request_restart.assert_called_once_with()
-        self.flags.set_flag.assert_called_once_with('config.complete')
 
     def test_configure_websso(self):
         handlers.configure_websso()

unit_tests/test_lib_charm_openstack_keystone_saml_mellon.py

@@ -45,13 +45,10 @@ class Helper(test_utils.PatchHelper):
         self.patch_release(
             keystone_saml_mellon.KeystoneSAMLMellonCharm.release)
 
-        self.patch_object(keystone_saml_mellon, 'unitdata',
+        self.patch_object(keystone_saml_mellon, 'endpoint_from_flag',
-                          new=mock.MagicMock())
-        self.kv = mock.MagicMock()
-        self.unitdata.kv.return_value = self.kv
-
-        self.patch_object(keystone_saml_mellon.os_utils, 'os_release',
                           new=mock.MagicMock())
+        self.endpoint = mock.MagicMock()
+        self.endpoint_from_flag.return_value = self.endpoint
 
         self.idp_name = "samltest"
         self.protocol_name = "mapped"
@@ -100,19 +97,6 @@ class Helper(test_utils.PatchHelper):
         self.open.return_value = self.fileobj
 
 
-class TestKeystoneSAMLMellonUtils(Helper):
-
-    def test_select_release(self):
-        self.kv.get.return_value = 'mitaka'
-        self.assertEqual(
-            keystone_saml_mellon.select_release(), 'mitaka')
-
-        self.kv.get.return_value = None
-        self.os_release.return_value = 'rocky'
-        self.assertEqual(
-            keystone_saml_mellon.select_release(), 'rocky')
-
-
 class TestKeystoneSAMLMellonConfigurationAdapter(Helper):
 
     def setUp(self):
@@ -120,12 +104,13 @@ class TestKeystoneSAMLMellonConfigurationAdapter(Helper):
         self.hostname = "keystone-sp.local"
         self.port = "5000"
         self.tls_enabled = True
-        self.unitdata_data = {
+        self.endpoint_data = {
             "hostname": self.hostname,
             "port": self.port,
             "tls-enabled": self.tls_enabled,
         }
-        self.kv.get.side_effect = FakeConfig(self.unitdata_data)
+        self.endpoint.all_joined_units.received.get.side_effect = (
+            FakeConfig(self.endpoint_data))
         self.base_url = "https://{}:{}".format(self.hostname, self.port)
 
     def test_validation_errors(self):
@@ -338,19 +323,20 @@ class TestKeystoneSAMLMellonCharm(Helper):
         self.sp_signing_keyinfo.__bool__.return_value = False
         self.assertFalse(ksm.configuration_complete())
 
-    def test_assess_status(self):
+    def test_custom_assess_status_check(self):
         ksm = keystone_saml_mellon.KeystoneSAMLMellonCharm()
-        ksm.assess_status()
+        self.assertEqual(
-        self.application_version_set.asert_called_once_with()
+            ksm.custom_assess_status_check(),
-        self.status_set.assert_called_once_with("active", "Unit is ready")
+            (None, None))
 
         # One option not ready
         self.status_set.reset_mock()
         self.sp_signing_keyinfo.__bool__.return_value = False
         ksm.options._validation_errors = {"idp-metadata": "malformed"}
-        ksm.assess_status()
+        self.assertEqual(
-        self.status_set.assert_called_once_with(
+            ksm.custom_assess_status_check(),
-            "blocked", "Configuration is incomplete. idp-metadata: malformed")
+            ("blocked",
+             "Configuration is incomplete. idp-metadata: malformed"))
 
     def test_render_config(self):
         ksm = keystone_saml_mellon.KeystoneSAMLMellonCharm()