Policyd override implementation

This patchset implements policy overrides for keystone.  It uses the
code in charmhelpers.

Closed-Bug: #1741723
Change-Id: I187f4493392178d87ef7dbd67de841bbeae0c65d

README.md

@@ -158,12 +158,57 @@ configuration:
         internal: internal-space
         shared-db: internal-space
 
-NOTE: Spaces must be configured in the underlying provider prior to attempting to use them.
+NOTE: Spaces must be configured in the underlying provider prior to attempting
+to use them.
 
 NOTE: Existing deployments using `os\-\*-network` configuration options will
 continue to function; these options are preferred over any network space
 binding provided if set.
 
+Policy Overrides
+----------------
+
+This feature allows for policy overrides using the `policy.d` directory.  This
+is an **advanced** feature and the policies that the OpenStack service supports
+should be clearly and unambiguously understood before trying to override, or
+add to, the default policies that the service uses.  The charm also has some
+policy defaults.  They should also be understood before being overridden.
+
+> **Caution**: It is possible to break the system (for tenants and other
+  services) if policies are incorrectly applied to the service.
+
+Policy overrides are YAML files that contain rules that will add to, or
+override, existing policy rules in the service.  The `policy.d` directory is
+a place to put the YAML override files.  This charm owns the
+`/etc/keystone/policy.d` directory, and as such, any manual changes to it will
+be overwritten on charm upgrades.
+
+Overrides are provided to the charm using a Juju resource called
+`policyd-override`.  The resource is a ZIP file.  This file, say
+`overrides.zip`, is attached to the charm by:
+
+
+    juju attach-resource keystone policyd-override=overrides.zip
+
+The policy override is enabled in the charm using:
+
+    juju config keystone use-policyd-override=true
+
+When `use-policyd-override` is `True` the status line of the charm will be
+prefixed with `PO:` indicating that policies have been overridden.  If the
+installation of the policy override YAML files failed for any reason then the
+status line will be prefixed with `PO (broken):`.  The log file for the charm
+will indicate the reason.  No policy override files are installed if the `PO
+(broken):` is shown.  The status line indicates that the overrides are broken,
+not that the policy for the service has failed. The policy will be the defaults
+for the charm and service.
+
+Policy overrides on one service may affect the functionality of another
+service. Therefore, it may be necessary to provide policy overrides for
+multiple service charms to achieve a consistent set of policies across the
+OpenStack system.  The charms for the other services that may need overrides
+should be checked to ensure that they support overrides before proceeding.
+
 Token Support
 -------------
 

charmhelpers/contrib/openstack/context.py

@@ -1940,7 +1940,7 @@ class VolumeAPIContext(InternalEndpointContext):
         as well as the catalog_info string that would be supplied. Returns
         a dict containing the volume_api_version and the volume_catalog_info.
         """
-        rel = os_release(self.pkg, base='icehouse')
+        rel = os_release(self.pkg)
         version = '2'
         if CompareOpenStackReleases(rel) >= 'pike':
             version = '3'
@@ -2140,7 +2140,7 @@ class VersionsContext(OSContextGenerator):
         self.pkg = pkg
 
     def __call__(self):
-        ostack = os_release(self.pkg, base='icehouse')
+        ostack = os_release(self.pkg)
         osystem = lsb_release()['DISTRIB_CODENAME'].lower()
         return {
             'openstack_release': ostack,

charmhelpers/contrib/openstack/policyd.py

@@ -299,10 +299,17 @@ def maybe_do_policyd_overrides(openstack_release,
     config = hookenv.config()
     try:
         if not config.get(POLICYD_CONFIG_NAME, False):
-            remove_policy_success_file()
             clean_policyd_dir_for(service, blacklist_paths)
+            if (os.path.isfile(_policy_success_file()) and
+                    restart_handler is not None and
+                    callable(restart_handler)):
+                restart_handler()
+            remove_policy_success_file()
             return
-    except Exception:
+    except Exception as e:
+        print("Exception is: ", str(e))
+        import traceback
+        traceback.print_exc()
         return
     if not is_policyd_override_valid_on_this_release(openstack_release):
         return
@@ -348,8 +355,12 @@ def maybe_do_policyd_overrides_on_config_changed(openstack_release,
     config = hookenv.config()
     try:
         if not config.get(POLICYD_CONFIG_NAME, False):
-            remove_policy_success_file()
             clean_policyd_dir_for(service, blacklist_paths)
+            if (os.path.isfile(_policy_success_file()) and
+                    restart_handler is not None and
+                    callable(restart_handler)):
+                restart_handler()
+            remove_policy_success_file()
             return
     except Exception:
         return
@@ -430,8 +441,13 @@ def _yamlfiles(zipfile):
     """
     l = []
     for infolist_item in zipfile.infolist():
+        try:
             if infolist_item.is_dir():
                 continue
+        except AttributeError:
+            # fallback to "old" way to determine dir entry for pre-py36
+            if infolist_item.filename.endswith('/'):
+                continue
         _, name_ext = os.path.split(infolist_item.filename)
         name, ext = os.path.splitext(name_ext)
         ext = ext.lower()

charmhelpers/contrib/openstack/utils.py

@@ -531,7 +531,7 @@ def reset_os_release():
     _os_rel = None
 
 
-def os_release(package, base='essex', reset_cache=False):
+def os_release(package, base=None, reset_cache=False):
     '''
     Returns OpenStack release codename from a cached global.
 
@@ -542,6 +542,8 @@ def os_release(package, base='essex', reset_cache=False):
     the installation source, the earliest release supported by the charm should
     be returned.
     '''
+    if not base:
+        base = UBUNTU_OPENSTACK_RELEASE[lsb_release()['DISTRIB_CODENAME']]
     global _os_rel
     if reset_cache:
         reset_os_release()
@@ -670,7 +672,10 @@ def openstack_upgrade_available(package):
         codename = get_os_codename_install_source(src)
         avail_vers = get_os_version_codename_swift(codename)
     else:
+        try:
             avail_vers = get_os_version_install_source(src)
+        except:
+            avail_vers = cur_vers
     apt.init()
     return apt.version_compare(avail_vers, cur_vers) >= 1
 
@@ -1693,7 +1698,7 @@ def enable_memcache(source=None, release=None, package=None):
     if release:
         _release = release
     else:
-        _release = os_release(package, base='icehouse')
+        _release = os_release(package)
     if not _release:
         _release = get_os_codename_install_source(source)
 

charmhelpers/contrib/storage/linux/ceph.py

@@ -1185,6 +1185,15 @@ class CephBrokerRq(object):
             self.request_id = str(uuid.uuid1())
         self.ops = []
 
+    def add_op(self, op):
+        """Add an op if it is not already in the list.
+
+        :param op: Operation to add.
+        :type op: dict
+        """
+        if op not in self.ops:
+            self.ops.append(op)
+
     def add_op_request_access_to_group(self, name, namespace=None,
                                        permission=None, key_name=None,
                                        object_prefix_permissions=None):
@@ -1198,7 +1207,7 @@ class CephBrokerRq(object):
                 'rwx': ['prefix1', 'prefix2'],
                 'class-read': ['prefix3']}
         """
-        self.ops.append({
+        self.add_op({
             'op': 'add-permissions-to-key', 'group': name,
             'namespace': namespace,
             'name': key_name or service_name(),
@@ -1251,7 +1260,7 @@ class CephBrokerRq(object):
         if pg_num and weight:
             raise ValueError('pg_num and weight are mutually exclusive')
 
-        self.ops.append({'op': 'create-pool', 'name': name,
+        self.add_op({'op': 'create-pool', 'name': name,
                      'replicas': replica_count, 'pg_num': pg_num,
                      'weight': weight, 'group': group,
                      'group-namespace': namespace, 'app-name': app_name,
@@ -1283,7 +1292,7 @@ class CephBrokerRq(object):
         :param max_objects: Maximum objects quota to apply
         :type max_objects: int
         """
-        self.ops.append({'op': 'create-pool', 'name': name,
+        self.add_op({'op': 'create-pool', 'name': name,
                      'pool-type': 'erasure',
                      'erasure-profile': erasure_profile,
                      'weight': weight,

config.yaml

@@ -408,4 +408,11 @@ options:
     description: |
       A comma-separated list of nagios servicegroups.
       If left empty, the nagios_context will be used as the servicegroup
-
+  use-policyd-override:
+    type: boolean
+    default: False
+    description: |
+      If True then use the resource file named 'policyd-override' to install
+      override YAML files in the service's policy.d directory.  The resource
+      file should be a ZIP file containing at least one yaml file with a .yaml
+      or .yml extension.  If False then remove the overrides.

hooks/keystone_hooks.py

@@ -164,6 +164,12 @@ from charmhelpers.contrib.openstack.cert_utils import (
     process_certificates,
 )
 
+from charmhelpers.contrib.openstack.policyd import (
+    maybe_do_policyd_overrides,
+    maybe_do_policyd_overrides_on_config_changed,
+)
+
+
 hooks = Hooks()
 CONFIGS = register_configs()
 
@@ -196,6 +202,8 @@ def install():
         if run_in_apache():
             disable_unused_apache_sites()
             service_pause('keystone')
+    # call the policy overrides handler which will install any policy overrides
+    maybe_do_policyd_overrides(os_release('keystone'), 'keystone')
 
 
 @hooks.hook('config-changed')
@@ -222,6 +230,10 @@ def config_changed():
     for r_id in relation_ids('cluster'):
         cluster_joined(rid=r_id)
 
+    # call the policy overrides handler which will install any policy overrides
+    maybe_do_policyd_overrides_on_config_changed(os_release('keystone'),
+                                                 'keystone')
+
     config_changed_postupgrade()
 
 
@@ -686,6 +698,9 @@ def upgrade_charm():
             'date', level=DEBUG)
         update_all_identity_relation_units()
 
+    # call the policy overrides handler which will install any policy overrides
+    maybe_do_policyd_overrides(os_release('keystone'), 'keystone')
+
 
 @hooks.hook('update-status')
 @harden()

metadata.yaml

@@ -53,3 +53,8 @@ requires:
 peers:
   cluster:
     interface: keystone-ha
+resources:
+  policyd-override:
+    type: file
+    filename: policyd-override.zip
+    description: The policy.d overrides file

tests/tests.yaml

@@ -11,8 +11,9 @@ gate_bundles:
 - xenial-ocata
 - xenial-mitaka
 - trusty-mitaka
-comment:
+comment: |
-  the glance configure job validates operation of identity-service relation
+  the glance configure job validates operation of identity-service relation.
+  The policyd test is generic and validates the policy.d overrides work
 configure:
 - zaza.openstack.charm_tests.glance.setup.add_lts_image
 - zaza.openstack.charm_tests.keystone.setup.add_demo_user
@@ -20,3 +21,7 @@ tests:
 - zaza.openstack.charm_tests.keystone.tests.AuthenticationAuthorizationTest
 - zaza.openstack.charm_tests.keystone.tests.CharmOperationTest
 - zaza.openstack.charm_tests.keystone.tests.SecurityTests
+- zaza.openstack.charm_tests.policyd.tests.KeystoneTests
+tests_options:
+  policyd:
+    service: keystone

unit_tests/test_keystone_hooks.py

@@ -16,7 +16,7 @@ import importlib
 import os
 import sys
 
-from mock import call, patch, MagicMock
+from mock import call, patch, MagicMock, ANY
 from test_utils import CharmTestCase
 
 # python-apt is not installed as part of test-requirements but is imported by
@@ -115,10 +115,13 @@ class KeystoneRelationTests(CharmTestCase):
         self.ssh_user = 'juju_keystone'
         self.snap_install_requested.return_value = False
 
+    @patch.object(hooks, 'maybe_do_policyd_overrides')
     @patch.object(utils, 'os_release')
     @patch.object(hooks, 'service_stop', lambda *args: None)
     @patch.object(hooks, 'service_start', lambda *args: None)
-    def test_install_hook(self, os_release):
+    def test_install_hook(self,
+                          os_release,
+                          mock_maybe_do_policyd_overrides):
         os_release.return_value = 'havana'
         self.run_in_apache.return_value = False
         repo = 'cloud:precise-grizzly'
@@ -132,11 +135,16 @@ class KeystoneRelationTests(CharmTestCase):
              'python-keystoneclient', 'python-mysqldb', 'python-psycopg2',
              'python3-six', 'uuid'], fatal=True)
         self.disable_unused_apache_sites.assert_not_called()
+        mock_maybe_do_policyd_overrides.assert_called_once_with(
+            ANY, "keystone")
 
+    @patch.object(hooks, 'maybe_do_policyd_overrides')
     @patch.object(utils, 'os_release')
     @patch.object(hooks, 'service_stop', lambda *args: None)
     @patch.object(hooks, 'service_start', lambda *args: None)
-    def test_install_hook_apache2(self, os_release):
+    def test_install_hook_apache2(self,
+                                  os_release,
+                                  mock_maybe_do_policyd_overrides):
         os_release.return_value = 'havana'
         self.run_in_apache.return_value = True
         repo = 'cloud:xenial-newton'
@@ -150,6 +158,9 @@ class KeystoneRelationTests(CharmTestCase):
              'python-keystoneclient', 'python-mysqldb', 'python-psycopg2',
              'python3-six', 'uuid'], fatal=True)
         self.disable_unused_apache_sites.assert_called_with()
+        mock_maybe_do_policyd_overrides.assert_called_once_with(
+            ANY, "keystone")
+
     mod_ch_openstack_utils = 'charmhelpers.contrib.openstack.utils'
 
     @patch.object(utils, 'os_release')
@@ -210,6 +221,7 @@ class KeystoneRelationTests(CharmTestCase):
                          configs.write.call_args_list)
         self.assertTrue(leader_init.called)
 
+    @patch.object(hooks, 'maybe_do_policyd_overrides_on_config_changed')
     @patch.object(hooks, 'notify_middleware_with_release_version')
     @patch.object(hooks, 'update_all_domain_backends')
     @patch.object(hooks, 'update_all_identity_relation_units')
@@ -221,7 +233,9 @@ class KeystoneRelationTests(CharmTestCase):
     @patch.object(hooks, 'CONFIGS')
     @patch.object(hooks, 'identity_changed')
     @patch.object(hooks, 'configure_https')
-    def test_config_changed_no_upgrade_leader(self, configure_https,
+    def test_config_changed_no_upgrade_leader(
+        self,
+        configure_https,
         identity_changed,
         configs,
         mock_cluster_joined,
@@ -231,7 +245,9 @@ class KeystoneRelationTests(CharmTestCase):
         mock_run_in_apache,
         update,
         mock_update_domains,
-                                              mock_notify_middleware):
+        mock_notify_middleware,
+        mock_maybe_do_policyd_overrides_on_config_changed
+    ):
         def fake_relation_ids(relation):
             rids = {'cluster': ['cluster:1'],
                     'identity-service': ['identity-service:0']}
@@ -259,6 +275,10 @@ class KeystoneRelationTests(CharmTestCase):
         self.assertTrue(mock_update_domains.called)
         self.assertTrue(mock_notify_middleware.called_once)
 
+        (mock_maybe_do_policyd_overrides_on_config_changed
+         .assert_called_once_with(ANY, "keystone"))
+
+    @patch.object(hooks, 'maybe_do_policyd_overrides_on_config_changed')
     @patch.object(hooks, 'is_db_initialised')
     @patch.object(hooks, 'update_all_domain_backends')
     @patch.object(hooks, 'update_all_identity_relation_units')
@@ -268,14 +288,18 @@ class KeystoneRelationTests(CharmTestCase):
     @patch.object(hooks, 'CONFIGS')
     @patch.object(hooks, 'identity_changed')
     @patch.object(hooks, 'configure_https')
-    def test_config_changed_no_upgrade_not_leader(self, configure_https,
+    def test_config_changed_no_upgrade_not_leader(
+        self,
+        configure_https,
         identity_changed,
         configs,
         mock_cluster_joined,
         mock_log,
         mock_run_in_apache, update,
         mock_update_domains,
-                                                  mock_is_db_initialised):
+        mock_is_db_initialised,
+        mock_maybe_do_policyd_overrides_on_config_changed
+    ):
 
         def fake_relation_ids(relation):
             rids = {}
@@ -300,6 +324,10 @@ class KeystoneRelationTests(CharmTestCase):
         self.assertTrue(update.called)
         self.assertTrue(mock_update_domains.called)
 
+        (mock_maybe_do_policyd_overrides_on_config_changed
+         .assert_called_once_with(ANY, "keystone"))
+
+    @patch.object(hooks, 'maybe_do_policyd_overrides_on_config_changed')
     @patch.object(hooks, 'update_all_domain_backends')
     @patch.object(hooks, 'update_all_identity_relation_units')
     @patch.object(hooks, 'run_in_apache')
@@ -310,7 +338,9 @@ class KeystoneRelationTests(CharmTestCase):
     @patch.object(hooks, 'CONFIGS')
     @patch.object(hooks, 'identity_changed')
     @patch.object(hooks, 'configure_https')
-    def test_config_changed_with_openstack_upgrade(self, configure_https,
+    def test_config_changed_with_openstack_upgrade(
+        self,
+        configure_https,
         identity_changed,
         configs,
         cluster_joined,
@@ -319,7 +349,9 @@ class KeystoneRelationTests(CharmTestCase):
         mock_is_db_initialised,
         mock_run_in_apache,
         update,
-                                                   mock_update_domains):
+        mock_update_domains,
+        mock_maybe_do_policyd_overrides_on_config_changed
+    ):
         def fake_relation_ids(relation):
             rids = {'identity-service': ['identity-service:0']}
             return rids.get(relation, [])
@@ -345,17 +377,24 @@ class KeystoneRelationTests(CharmTestCase):
         self.assertTrue(update.called)
         self.assertTrue(mock_update_domains.called)
 
+        (mock_maybe_do_policyd_overrides_on_config_changed
+         .assert_called_once_with(ANY, "keystone"))
+
+    @patch.object(hooks, 'maybe_do_policyd_overrides_on_config_changed')
     @patch.object(hooks, 'is_expected_scale')
     @patch.object(hooks, 'os_release')
     @patch.object(hooks, 'run_in_apache')
     @patch.object(hooks, 'is_db_initialised')
     @patch.object(hooks, 'configure_https')
-    def test_config_changed_with_openstack_upgrade_action(self,
+    def test_config_changed_with_openstack_upgrade_action(
+        self,
         config_https,
         mock_db_init,
         mock_run_in_apache,
         os_release,
-                                                          is_expected_scale):
+        is_expected_scale,
+        mock_maybe_do_policyd_overrides_on_config_changed
+    ):
         os_release.return_value = 'ocata'
         self.enable_memcache.return_value = False
         mock_run_in_apache.return_value = False
@@ -368,6 +407,9 @@ class KeystoneRelationTests(CharmTestCase):
 
         self.assertFalse(self.do_openstack_upgrade_reexec.called)
 
+        (mock_maybe_do_policyd_overrides_on_config_changed
+         .assert_called_once_with(ANY, "keystone"))
+
     @patch.object(hooks, 'is_db_initialised')
     @patch('keystone_utils.log')
     @patch.object(hooks, 'send_notifications')
@@ -537,6 +579,7 @@ class KeystoneRelationTests(CharmTestCase):
         cmd = ['a2dissite', 'openstack_https_frontend']
         self.check_call.assert_called_with(cmd)
 
+    @patch.object(hooks, 'maybe_do_policyd_overrides')
     @patch.object(hooks, 'update_all_identity_relation_units')
     @patch.object(utils, 'os_release')
     @patch.object(hooks, 'is_db_ready')
@@ -551,7 +594,8 @@ class KeystoneRelationTests(CharmTestCase):
                                   mock_is_db_initialised,
                                   mock_is_db_ready,
                                   os_release,
-                                  update):
+                                  update,
+                                  mock_maybe_do_policyd_overrides):
         os_release.return_value = 'havana'
         mock_is_db_initialised.return_value = True
         mock_is_db_ready.return_value = True
@@ -566,7 +610,10 @@ class KeystoneRelationTests(CharmTestCase):
         self.remove_old_packages.assert_called_once_with()
         self.service_restart.assert_called_with('apache2')
         mock_stop_manager_instance.assert_called_once_with()
+        mock_maybe_do_policyd_overrides.assert_called_once_with(
+            ANY, "keystone")
 
+    @patch.object(hooks, 'maybe_do_policyd_overrides')
     @patch.object(hooks, 'update_all_identity_relation_units')
     @patch.object(utils, 'os_release')
     @patch.object(hooks, 'is_db_ready')
@@ -581,7 +628,8 @@ class KeystoneRelationTests(CharmTestCase):
                                               mock_is_db_initialised,
                                               mock_is_db_ready,
                                               os_release,
-                                              update):
+                                              update,
+                                              mock_maybe_do_policyd_overrides):
         os_release.return_value = 'havana'
         mock_is_db_initialised.return_value = True
         mock_is_db_ready.return_value = True
@@ -596,6 +644,8 @@ class KeystoneRelationTests(CharmTestCase):
         self.remove_old_packages.assert_called_once_with()
         self.service_restart.assert_called_with('apache2')
         mock_stop_manager_instance.assert_called_once_with()
+        mock_maybe_do_policyd_overrides.assert_called_once_with(
+            ANY, "keystone")
 
     @patch.object(hooks, 'update_all_identity_relation_units')
     @patch.object(hooks, 'is_db_initialised')
@@ -737,6 +787,7 @@ class KeystoneRelationTests(CharmTestCase):
         # Still updates relations
         self.assertTrue(self.relation_ids.called)
 
+    @patch.object(hooks, 'maybe_do_policyd_overrides')
     @patch.object(hooks, 'update_all_identity_relation_units')
     @patch.object(utils, 'os_release')
     @patch('keystone_utils.log')
@@ -746,7 +797,8 @@ class KeystoneRelationTests(CharmTestCase):
                                       mock_stop_manager_instance,
                                       mock_relation_ids,
                                       mock_log,
-                                      os_release, update):
+                                      os_release, update,
+                                      mock_maybe_do_policyd_overrides):
         os_release.return_value = 'havana'
 
         self.filter_installed_packages.return_value = ['something']
@@ -756,17 +808,24 @@ class KeystoneRelationTests(CharmTestCase):
         self.assertTrue(self.log.called)
         self.assertFalse(update.called)
         mock_stop_manager_instance.assert_called_once()
+        mock_maybe_do_policyd_overrides.assert_called_once_with(
+            ANY, "keystone")
 
+    @patch.object(hooks, 'maybe_do_policyd_overrides')
     @patch.object(hooks, 'update_all_identity_relation_units')
     @patch.object(utils, 'os_release')
     @patch('keystone_utils.log')
     @patch('keystone_utils.relation_ids')
     @patch.object(hooks, 'stop_manager_instance')
-    def test_upgrade_charm_not_leader_no_packages(self,
+    def test_upgrade_charm_not_leader_no_packages(
+        self,
         mock_stop_manager_instance,
         mock_relation_ids,
         mock_log,
-                                                  os_release, update):
+        os_release,
+        update,
+        mock_maybe_do_policyd_overrides
+    ):
         os_release.return_value = 'havana'
 
         self.filter_installed_packages.return_value = []
@@ -776,6 +835,8 @@ class KeystoneRelationTests(CharmTestCase):
         self.assertTrue(self.log.called)
         self.assertFalse(update.called)
         mock_stop_manager_instance.assert_called_once()
+        mock_maybe_do_policyd_overrides.assert_called_once_with(
+            ANY, "keystone")
 
     def test_domain_backend_changed_v2(self):
         self.get_api_version.return_value = 2