diff --git a/README.md b/README.md index ee2fbd82..5aac7c22 100644 --- a/README.md +++ b/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 ------------- diff --git a/charmhelpers/contrib/openstack/context.py b/charmhelpers/contrib/openstack/context.py index a3d48c41..9b80b6d6 100644 --- a/charmhelpers/contrib/openstack/context.py +++ b/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, diff --git a/charmhelpers/contrib/openstack/policyd.py b/charmhelpers/contrib/openstack/policyd.py index 1adf2472..6541146f 100644 --- a/charmhelpers/contrib/openstack/policyd.py +++ b/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(): - if infolist_item.is_dir(): - continue + 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() diff --git a/charmhelpers/contrib/openstack/utils.py b/charmhelpers/contrib/openstack/utils.py index ac96f844..9ed96f00 100644 --- a/charmhelpers/contrib/openstack/utils.py +++ b/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: - avail_vers = get_os_version_install_source(src) + 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) diff --git a/charmhelpers/contrib/storage/linux/ceph.py b/charmhelpers/contrib/storage/linux/ceph.py index e13dfa8b..a927485f 100644 --- a/charmhelpers/contrib/storage/linux/ceph.py +++ b/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,11 +1260,11 @@ 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, - 'replicas': replica_count, 'pg_num': pg_num, - 'weight': weight, 'group': group, - 'group-namespace': namespace, 'app-name': app_name, - 'max-bytes': max_bytes, 'max-objects': max_objects}) + 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, + 'max-bytes': max_bytes, 'max-objects': max_objects}) def add_op_create_erasure_pool(self, name, erasure_profile=None, weight=None, group=None, app_name=None, @@ -1283,12 +1292,12 @@ class CephBrokerRq(object): :param max_objects: Maximum objects quota to apply :type max_objects: int """ - self.ops.append({'op': 'create-pool', 'name': name, - 'pool-type': 'erasure', - 'erasure-profile': erasure_profile, - 'weight': weight, - 'group': group, 'app-name': app_name, - 'max-bytes': max_bytes, 'max-objects': max_objects}) + self.add_op({'op': 'create-pool', 'name': name, + 'pool-type': 'erasure', + 'erasure-profile': erasure_profile, + 'weight': weight, + 'group': group, 'app-name': app_name, + 'max-bytes': max_bytes, 'max-objects': max_objects}) def set_ops(self, ops): """Set request ops to provided value. diff --git a/config.yaml b/config.yaml index 016ed212..6d42cca6 100644 --- a/config.yaml +++ b/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. diff --git a/hooks/keystone_hooks.py b/hooks/keystone_hooks.py index 811bd711..9acb6cbf 100755 --- a/hooks/keystone_hooks.py +++ b/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() diff --git a/metadata.yaml b/metadata.yaml index 17adff33..de795fa0 100644 --- a/metadata.yaml +++ b/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 diff --git a/tests/tests.yaml b/tests/tests.yaml index be712307..73812fc5 100644 --- a/tests/tests.yaml +++ b/tests/tests.yaml @@ -11,8 +11,9 @@ gate_bundles: - xenial-ocata - xenial-mitaka - trusty-mitaka -comment: - the glance configure job validates operation of identity-service relation +comment: | + 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 diff --git a/unit_tests/test_keystone_hooks.py b/unit_tests/test_keystone_hooks.py index b8f802e1..3995be36 100644 --- a/unit_tests/test_keystone_hooks.py +++ b/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,17 +233,21 @@ 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, - identity_changed, - configs, - mock_cluster_joined, - admin_relation_changed, - mock_log, - mock_is_db_initialised, - mock_run_in_apache, - update, - mock_update_domains, - mock_notify_middleware): + def test_config_changed_no_upgrade_leader( + self, + configure_https, + identity_changed, + configs, + mock_cluster_joined, + admin_relation_changed, + mock_log, + mock_is_db_initialised, + mock_run_in_apache, + update, + mock_update_domains, + 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, - identity_changed, - configs, - mock_cluster_joined, - mock_log, - mock_run_in_apache, update, - mock_update_domains, - mock_is_db_initialised): + 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_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,16 +338,20 @@ 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, - identity_changed, - configs, - cluster_joined, - admin_relation_changed, - mock_log, - mock_is_db_initialised, - mock_run_in_apache, - update, - mock_update_domains): + def test_config_changed_with_openstack_upgrade( + self, + configure_https, + identity_changed, + configs, + cluster_joined, + admin_relation_changed, + mock_log, + mock_is_db_initialised, + mock_run_in_apache, + update, + 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, - config_https, - mock_db_init, - mock_run_in_apache, - os_release, - is_expected_scale): + def test_config_changed_with_openstack_upgrade_action( + self, + config_https, + mock_db_init, + mock_run_in_apache, + os_release, + 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, - mock_stop_manager_instance, - mock_relation_ids, - mock_log, - os_release, update): + def test_upgrade_charm_not_leader_no_packages( + self, + mock_stop_manager_instance, + mock_relation_ids, + mock_log, + 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