policy.d override support for reactive OpenStack charms

This patchset provides support for policy.d overrides in
charms.openstack based charms.  It is essentially an adapter for the
charm-helpers policyd module which does the heavy lifting for all of the
policy.d overrides.

This change depends on the charm-helpers change:
https://github.com/juju/charm-helpers/pull/368

Change-Id: I495ce9f5be1815c78a2295d33bab4b9a774d805c

charms_openstack/charm/core.py

@@ -6,6 +6,7 @@ import os
 import re
 import subprocess
 
+import charmhelpers.contrib.openstack.policyd as os_policyd
 import charmhelpers.contrib.openstack.templating as os_templating
 import charmhelpers.contrib.openstack.utils as os_utils
 import charmhelpers.core.hookenv as hookenv
@@ -1080,11 +1081,23 @@ class BaseOpenStackCharmAssessStatus(object):
         workload status in juju.
         """
         # set the application version when we set the status (always)
-        # NOTE(tinwood) this is not, strictly speaking, good code organisation,
-        # as the 'application_version' property is in the classes.py file.
-        # However, as this is ALWAYS a mixin on that class, we can get away
-        # with this.
+        # NOTE(ajkavanagh) this is not, strictly speaking, good code
+        # organisation, as the 'application_version' property is in the
+        # classes.py file.  However, as this is ALWAYS a mixin on that class,
+        # we can get away with this.
         hookenv.application_version_set(self.application_version)
+
+        # NOTE(ajkavanagh) we check for the Policyd override here, even though
+        # most of the work is done in the plugin class PolicydOverridePlugin.
+        # This is a consequence of how the assess status is implemented; we
+        # simply have to get the prefix sorted here and there's no easy way to
+        # get it in without a complete refactor.
+        if self.config.get(os_policyd.POLICYD_CONFIG_NAME, False):
+            os_policyd_prefix = "{} ".format(
+                os_policyd.policyd_status_message_prefix())
+        else:
+            os_policyd_prefix = ""
+
         for f in [self.check_if_paused,
                   self.custom_assess_status_check,
                   self.check_interfaces,
@@ -1092,10 +1105,10 @@ class BaseOpenStackCharmAssessStatus(object):
                   self.check_services_running]:
             state, message = f()
             if state is not None:
-                hookenv.status_set(state, message)
+                hookenv.status_set(state, os_policyd_prefix + message)
                 return
         # No state was particularly set, so assume the unit is active
-        hookenv.status_set('active', 'Unit is ready')
+        hookenv.status_set('active', os_policyd_prefix + 'Unit is ready')
 
     def assess_status(self):
         """This is a deferring version of _assess_status that only runs during

charms_openstack/plugins/__init__.py

@@ -19,10 +19,12 @@ from charms_openstack.plugins.adapters import (
 from charms_openstack.plugins.classes import (
     BaseOpenStackCephCharm,
     CephCharm,
+    PolicydOverridePlugin,
 )
 
 __all__ = (
     "BaseOpenStackCephCharm",
     "CephCharm",
     "CephRelationAdapter",
+    "PolicydOverridePlugin",
 )

charms_openstack/plugins/classes.py

@@ -22,6 +22,7 @@ import charms_openstack.charm
 from charms_openstack.charm.classes import SNAP_PATH_PREFIX_FORMAT
 
 import charmhelpers.core as ch_core
+import charmhelpers.contrib.openstack.policyd as ch_policyd
 
 
 class BaseOpenStackCephCharm(object):
@@ -288,3 +289,122 @@ class CephCharm(charms_openstack.charm.OpenStackCharm,
         """
         self.configure_source()
         super().install()
+
+
+class PolicydOverridePlugin(object):
+    """The PolicydOverridePlugin is provided to manage the policy.d overrides
+    to charms.openstack charms.  It heavily leans on the
+    charmhelpers.contrib.openstack.policyd to provide the functionality.  The
+    methods provided in this class simply use the functions from charm-helpers
+    so that charm authors can simply include this plugin class into the
+    inheritance list of the charm class.
+
+    It's very important that the PolicyOverridePlugin class appear FIRST in the
+    list of classes when declaring the charm class.  This is to ensure that the
+    config_changed() method in this class gets called first, and it then calls
+    other classes.  Otherwise, the config_changed method in the base class will
+    need to call the config_changed() method in this class manually.  e.g. from
+    Designate:
+
+        class DesignateCharm(ch_plugins.PolicydOverridePlugin,
+                             openstack_charm.HAOpenStackCharm):
+
+    Note that this feature is only available with OpenStack versions of
+    'queens' and later, and Ubuntu versions of 'bionic' and later.  Prior to
+    those versions, the feature will not activate.  This is checked in the
+    charm-helpers policyd implementation functions which are called from this
+    class' implementation.
+
+    This should be read in conjunction with the module
+    charmhelpers.contrib.openstack.policyd which provides further details on
+    the changes that need to be made to a charm to enable this feature.
+
+    Note that the metadata.yaml and config.yaml needs to be updated for the
+    charm to actually be able to use this class.  See the
+    charmhelpers.contrib.openstack.policyd module for further details.
+
+    The following class variables are used to drive the plugin and should be
+    declared on the class:
+
+       policyd_service_name = str
+       policyd_blacklist_paths = Union[None, List[str]]
+       policyd_blacklist_keys = Union[None, List[str]]
+       policyd_template_function = Union[None, Callable[[str], str]]
+       policyd_restart_on_change = Union[None, bool]
+
+    These have the following meanings:
+
+    policyd_service_name:
+        This is the name of the payload that is having an override.  e.g.
+        keystone.  It is used to construct the policy.d directory:
+        /etc/keystone/policy.d/
+
+    policyd_blacklist_paths: (Optional)
+        These are other policyd overrides that exist in the above directory
+        that should not be touched.  It is a list of the FULL path.  e.g.
+        /etc/keystone/policy.d/charm-overrides.yaml
+
+    policyd_blacklist_keys: (Optional)
+        These are keys that should not appear in the YAML files.  e.g. admin.
+
+    policyd_template_function: (Optional)
+        This is an callable that takes a string that returns another string
+        that tis then loaded as the yaml file.  This is intended to allow a
+        charm to modify the proposed yaml file to allow substitution of rules
+        and values under the control of the charm.  The charm needs to supply
+        the substitution function (and thus the variables that will be used).
+
+    policyd_restart_on_change: Optional
+        If set to True, then the service will be restarted using the charm
+        class'  `restart_services` method.
+    """
+
+    def _policyd_function_args(self):
+        """Returns the parameters that need to be passed to the charm-helpers
+        policyd implemenation functions.
+
+        :returns: ([openstack_release, payload_name],
+                   {blacklist_paths=...,
+                    blacklist_keys=...,
+                    template_function=...,
+                    restart_handler=...,})
+        :rtype: Tuple[List[str,str], Dict[str,str]]
+        """
+        blacklist_paths = getattr(self, 'policyd_blacklist_paths', None)
+        blacklist_keys = getattr(self, 'policyd_blacklist_keys', None)
+        template_function = getattr(self, 'policyd_template_function', None)
+        if getattr(self, 'policyd_restart_on_change', False):
+            restart_handler = self.restart_services
+        else:
+            restart_handler = None
+        return ([self.release, self.policyd_service_name],
+                dict(blacklist_paths=blacklist_paths,
+                     blacklist_keys=blacklist_keys,
+                     template_function=template_function,
+                     restart_handler=restart_handler))
+
+    def _maybe_policyd_overrides(self):
+        args, kwargs = self._policyd_function_args()
+        ch_policyd.maybe_do_policyd_overrides(*args, **kwargs)
+
+    def install(self):
+        """Hook into the install"""
+        super().install()
+        self._maybe_policyd_overrides()
+
+    def upgrade_charm(self):
+        """Check the policyd during an upgrade_charm"""
+        super().upgrade_charm()
+        self._maybe_policyd_overrides()
+
+    def config_changed(self):
+        """Note that this is usually a nop, and is only called from the default
+        handler.  Please check that the charm implementation actually uses it.
+        """
+        try:
+            super().config_changed()
+        except Exception:
+            pass
+        args, kwargs = self._policyd_function_args()
+        ch_policyd.maybe_do_policyd_overrides_on_config_changed(
+            *args, **kwargs)

charms_openstack/test_mocks.py

@@ -30,6 +30,8 @@ def mock_charmhelpers():
         charmhelpers.contrib.openstack.cert_utils)
     sys.modules['charmhelpers.contrib.openstack.templating'] = (
         charmhelpers.contrib.openstack.templating)
+    sys.modules['charmhelpers.contrib.openstack.policyd'] = (
+        charmhelpers.contrib.openstack.policyd)
     sys.modules['charmhelpers.contrib.network'] = charmhelpers.contrib.network
     sys.modules['charmhelpers.contrib.network.ip'] = (
         charmhelpers.contrib.network.ip)

tox.ini

@@ -43,4 +43,4 @@ basepython = python3
 commands = {posargs}
 
 [flake8]
-ignore = E402,E226
+ignore = E402,E226,W504

unit_tests/__init__.py

@@ -35,6 +35,8 @@ sys.modules['charmhelpers.contrib.openstack.cert_utils'] = (
     charmhelpers.contrib.openstack.cert_utils)
 sys.modules['charmhelpers.contrib.openstack.utils'] = (
     charmhelpers.contrib.openstack.utils)
+sys.modules['charmhelpers.contrib.openstack.policyd'] = (
+    charmhelpers.contrib.openstack.policyd)
 sys.modules['charmhelpers.contrib.openstack.templating'] = (
     charmhelpers.contrib.openstack.templating)
 sys.modules['charmhelpers.contrib.openstack.context'] = (

unit_tests/charms_openstack/plugins/test_classes.py

@@ -191,3 +191,101 @@ class TestCephCharm(BaseOpenStackCharmTest):
         self.target.install()
         self.target.configure_source.assert_called()
         self.check_output.assert_called()
+
+
+class MockCharmForPolicydOverrid(object):
+
+    def __init__(self, *args, **kwargs):
+        self._restart_services = False
+        self._install = False
+        self._upgrade_charm = False
+        self._config_changed = False
+        self.release = 'mitaka'
+        self.policyd_service_name = 'aservice'
+        super().__init__(*args, **kwargs)
+
+    def restart_services(self):
+        self._restart_services = True
+
+    def install(self):
+        self._install = True
+
+    def upgrade_charm(self):
+        self._upgrade_charm = True
+
+    def config_changed(self):
+        self._config_changed = True
+
+
+class FakeConsumingPolicydOverride(cpl.PolicydOverridePlugin,
+                                   MockCharmForPolicydOverrid):
+
+    pass
+
+
+class TestPolicydOverridePlugin(BaseOpenStackCharmTest):
+
+    def setUp(self):
+        super(TestPolicydOverridePlugin, self).setUp(
+            FakeConsumingPolicydOverride, TEST_CONFIG)
+
+    def test__policyd_function_args_no_defines(self):
+        args, kwargs = self.target._policyd_function_args()
+        self.assertEqual(args, ['mitaka', 'aservice'])
+        self.assertEqual(kwargs, {
+            'blacklist_paths': None,
+            'blacklist_keys': None,
+            'template_function': None,
+            'restart_handler': None
+        })
+
+    def test__policyd_function_args_with_defines(self):
+        def my_template_fn(s):
+            return "done"
+
+        self.target.policyd_blacklist_paths = ['p1']
+        self.target.policyd_blacklist_keys = ['k1']
+        self.target.policyd_template_function = my_template_fn
+        self.target.policyd_restart_on_change = True
+        args, kwargs = self.target._policyd_function_args()
+        self.assertEqual(args, ['mitaka', 'aservice'])
+        self.assertEqual(kwargs, {
+            'blacklist_paths': ['p1'],
+            'blacklist_keys': ['k1'],
+            'template_function': my_template_fn,
+            'restart_handler': self.target.restart_services
+        })
+
+    def test__maybe_policyd_overrides(self):
+        self.patch_target('_policyd_function_args',
+                          return_value=(["args"], {"kwargs": 1}))
+        self.patch_object(cpl.ch_policyd,
+                          'maybe_do_policyd_overrides',
+                          name='mock_policyd_call')
+        self.target._maybe_policyd_overrides()
+        self.mock_policyd_call.assert_called_once_with(
+            "args", kwargs=1)
+
+    def test_install_calls_policyd(self):
+        self.patch_target('_maybe_policyd_overrides')
+        self.target.install()
+        self.assertTrue(self.target._install)
+        self._maybe_policyd_overrides.assert_called_once_with()
+
+    def test_upgrade_charm_calls_policyd(self):
+        self.patch_target('_maybe_policyd_overrides')
+        self.target.upgrade_charm()
+        self.assertTrue(self.target._upgrade_charm)
+        self._maybe_policyd_overrides.assert_called_once_with()
+
+    def test_config_changed_calls_into_policyd_library(self):
+        self.patch_target('_policyd_function_args',
+                          return_value=(["args"], {"kwargs": 1}))
+        self.patch_object(cpl.ch_policyd,
+                          'maybe_do_policyd_overrides_on_config_changed',
+                          name='mock_policyd_call')
+        self.target.config_changed()
+        self.assertTrue(self.target._config_changed)
+        self._policyd_function_args.assert_called_once_with()
+        self.mock_policyd_call.assert_called_once_with(
+            "args", kwargs=1)