From 848eb535a43b16ae9c26ccac0cdf5dfcc7fb6d51 Mon Sep 17 00:00:00 2001 From: Alex Kavanagh Date: Tue, 31 Mar 2020 21:47:04 +0100 Subject: [PATCH] Remove policy.json from charm for ussuri Glance (in ussuri) uses policy-in-code, and so that policy.json file doesn't ship with the package. This means that the charm can't rely on the file existing ussuri onwards. This patchset changes the way the charm uses policy.json by switching it to a charm determined policy.yaml file (preferred format) with the only 3 options that the charm determines to enforce. Also add yaml vars to focal-ussuri bundle This brings it into line with the other charms that are part of the the enable-focal topic. This makes it easier to add a new bundle just by changing a couple of variables. Closes-Bug: #1872996 Change-Id: I47f19272a4e0af3781843608b76304ce8ba1e2b8 --- hooks/glance_contexts.py | 29 +++++++++++++ hooks/glance_relations.py | 7 ++- hooks/glance_utils.py | 77 +++++++++++++++++++++++++++------ templates/ussuri/policy.json | 5 --- templates/ussuri/policy.yaml | 5 +++ tests/bundles/focal-ussuri.yaml | 33 +++++++++++--- unit_tests/test_glance_utils.py | 26 +++++------ 7 files changed, 141 insertions(+), 41 deletions(-) delete mode 100644 templates/ussuri/policy.json create mode 100644 templates/ussuri/policy.yaml diff --git a/hooks/glance_contexts.py b/hooks/glance_contexts.py index 04c9ff98..eb547475 100644 --- a/hooks/glance_contexts.py +++ b/hooks/glance_contexts.py @@ -71,6 +71,35 @@ class GlanceContext(OSContextGenerator): return ctxt +class GlancePolicyContext(OSContextGenerator): + """This Context is only used from Ussuri onwards. At Ussuri, Glance + implemented policy-in-code, and thus didn't ship with a policy.json. + Therefore, the charm introduces a 'policy.yaml' file that is used to + provide the override here. + + Note that this is separate from policy overrides as it's a charm config + option that has existed prior to its introduction. + + Update *_image_location policy to restrict to admin role. + + We do this unconditonally and keep a record of the original as installed by + the package. + """ + + def __call__(self): + if config('restrict-image-location-operations'): + policy_value = 'role:admin' + else: + policy_value = '' + + ctxt = { + "get_image_location": policy_value, + "set_image_location": policy_value, + "delete_image_location": policy_value, + } + return ctxt + + class CephGlanceContext(OSContextGenerator): interfaces = ['ceph-glance'] diff --git a/hooks/glance_relations.py b/hooks/glance_relations.py index 8c234908..ac24b180 100755 --- a/hooks/glance_relations.py +++ b/hooks/glance_relations.py @@ -272,7 +272,7 @@ def object_store_joined(): return [image_service_joined(rid) for rid in relation_ids('image-service')] - update_image_location_policy() + update_image_location_policy(CONFIGS) CONFIGS.write(GLANCE_API_CONF) @@ -400,7 +400,7 @@ def config_changed(): # NOTE(jamespage): trigger any configuration related changes # for cephx permissions restrictions ceph_changed() - update_image_location_policy() + update_image_location_policy(CONFIGS) # call the policy overrides handler which will install any policy overrides maybe_do_policyd_overrides_on_config_changed( @@ -443,6 +443,9 @@ def upgrade_charm(): reinstall_paste_ini(force_reinstall=packages_removed) configure_https() update_nrpe_config() + # NOTE(ajkavanagh) the update_image_location_policy() call below isn't + # called with CONFIGS as the config files all get re-written after the + # call. update_image_location_policy() CONFIGS.write_all() if packages_removed: diff --git a/hooks/glance_utils.py b/hooks/glance_utils.py index 23cbaf4e..8e3e8168 100644 --- a/hooks/glance_utils.py +++ b/hooks/glance_utils.py @@ -36,6 +36,7 @@ from charmhelpers.core.hookenv import ( config, log, INFO, + WARNING, relation_ids, service_name, ) @@ -119,6 +120,11 @@ GLANCE_REGISTRY_PASTE = os.path.join(GLANCE_CONF_DIR, GLANCE_API_PASTE = os.path.join(GLANCE_CONF_DIR, 'glance-api-paste.ini') GLANCE_POLICY_FILE = os.path.join(GLANCE_CONF_DIR, "policy.json") +# NOTE(ajkavanagh): from Ussuri, glance switched to policy-in-code; this is the +# policy.yaml file (as there is not packaged policy.json or .yaml) that is used +# to provide the image_location override config value: +# 'restrict-image-location-operations' +GLANCE_POLICY_YAML = os.path.join(GLANCE_CONF_DIR, "policy.yaml") CEPH_CONF = "/etc/ceph/ceph.conf" CHARM_CEPH_CONF = '/var/lib/charm/{}/ceph.conf' @@ -220,6 +226,10 @@ CONFIG_FILES = OrderedDict([ 'contexts': [], 'services': ['apache2'], }), + (GLANCE_POLICY_YAML, { + 'hook_contexts': [glance_contexts.GlancePolicyContext()], + 'services': [], + }), ]) @@ -270,8 +280,8 @@ def register_configs(): CONFIG_FILES[GLANCE_SWIFT_CONF]['hook_contexts']) if cmp_release >= 'ussuri': - configs.register(GLANCE_POLICY_FILE, - CONFIG_FILES[GLANCE_POLICY_FILE]['hook_contexts']) + configs.register(GLANCE_POLICY_YAML, + CONFIG_FILES[GLANCE_POLICY_YAML]['hook_contexts']) return configs @@ -599,38 +609,79 @@ def is_api_ready(configs): return (not incomplete_relation_data(configs, REQUIRED_INTERFACES)) -def update_image_location_policy(): +def update_image_location_policy(configs=None): """Update *_image_location policy to restrict to admin role. We do this unconditonally and keep a record of the original as installed by the package. + + For ussuri, the charm updates/writes the policy.yaml file. The configs + param is optional as the caller may already be writing all the configs. + From ussuri onwards glance is policy-in-code (rather than using a + policy.json) and, therefore, policy files are essentially all overrides. + + From ussuri, this function deletes the policy.json file and alternatively + writes the GLANCE_POLICY_YAML file via the configs object. + + :param configs: The configs for the charm + :type configs: Optional[:class:templating.OSConfigRenderer()] """ - if CompareOpenStackReleases(os_release('glance-common')) < 'kilo': + _res = os_release('glance-common') + cmp = CompareOpenStackReleases(_res) + if cmp < 'kilo': # NOTE(hopem): at the time of writing we are unable to do this for # earlier than Kilo due to LP: #1502136 return + if cmp >= 'ussuri': + # If the policy.json exists, then remove it as it's the packaged + # version from a previous version of OpenStack, and thus not used. + if os.path.isfile(GLANCE_POLICY_FILE): + try: + os.remove(GLANCE_POLICY_FILE) + except Exception as e: + log("Problem removing file: {}: {}" + .format(GLANCE_POLICY_FILE, str(e))) + # if the caller supplied a configs param then update the + # GLANCE_POLICY_FILE using its context. + if configs is not None: + configs.write(GLANCE_POLICY_YAML) + return + # otherwise the OpenStack release after kilo and before ussuri, so continue + # modifying the existing policy.json file. db = kv() policies = ["get_image_location", "set_image_location", "delete_image_location"] + + try: + with open(GLANCE_POLICY_FILE) as f: + pmap = json.load(f) + except IOError as e: + log("Problem opening glance policy file: {}. Error was:{}" + .format(GLANCE_POLICY_FILE, str(e)), + level=WARNING) + return + for policy_key in policies: # Save original value at time of first install in case we ever need to # revert. db_key = "policy_{}".format(policy_key) if db.get(db_key) is None: - p = json.loads(open(GLANCE_POLICY_FILE).read()) - if policy_key in p: - db.set(db_key, p[policy_key]) + if policy_key in pmap: + db.set(db_key, pmap[policy_key]) db.flush() else: log("key '{}' not found in policy file".format(policy_key), level=INFO) - if config('restrict-image-location-operations'): - policy_value = 'role:admin' - else: - policy_value = '' + if config('restrict-image-location-operations'): + policy_value = 'role:admin' + else: + policy_value = '' + new_policies = {k: policy_value for k in policies} + for policy_key, policy_value in new_policies.items(): log("Updating Glance policy file setting policy " - "'{}':'{}'".format(policy_key, policy_value), level=INFO) - update_json_file(GLANCE_POLICY_FILE, {policy_key: policy_value}) + "'{}': '{}'".format(policy_key, policy_value), level=INFO) + + update_json_file(GLANCE_POLICY_FILE, new_policies) diff --git a/templates/ussuri/policy.json b/templates/ussuri/policy.json deleted file mode 100644 index 9ccdb5c9..00000000 --- a/templates/ussuri/policy.json +++ /dev/null @@ -1,5 +0,0 @@ -{ - "delete_image_location": "rule:default", - "get_image_location": "rule:default", - "set_image_location": "rule:default" -} diff --git a/templates/ussuri/policy.yaml b/templates/ussuri/policy.yaml new file mode 100644 index 00000000..f15efec5 --- /dev/null +++ b/templates/ussuri/policy.yaml @@ -0,0 +1,5 @@ +# The file is controlled by Juju -- do not edit by hand as it will be +# overwritten +"get_image_location": "{{ get_image_location }}" +"set_image_location": "{{ set_image_location }}" +"delete_image_location": "{{ delete_image_location }}" diff --git a/tests/bundles/focal-ussuri.yaml b/tests/bundles/focal-ussuri.yaml index 54c9bcac..1f4d9d9e 100644 --- a/tests/bundles/focal-ussuri.yaml +++ b/tests/bundles/focal-ussuri.yaml @@ -1,3 +1,6 @@ +variables: + openstack-origin: &openstack-origin distro + series: focal comment: @@ -13,32 +16,48 @@ machines: '3': '4': -relations: - - ["keystone:shared-db", "keystone-mysql-router:shared-db"] - - ["glance:shared-db", "glance-mysql-router:shared-db"] - - ["glance:identity-service", "keystone:identity-service"] - - ["glance-mysql-router:db-router", "mysql-innodb-cluster:db-router"] - - ["keystone-mysql-router:db-router", "mysql-innodb-cluster:db-router"] - applications: + glance-mysql-router: charm: cs:~openstack-charmers-next/mysql-router keystone-mysql-router: charm: cs:~openstack-charmers-next/mysql-router + mysql-innodb-cluster: charm: cs:~openstack-charmers-next/mysql-innodb-cluster num_units: 3 + options: + source: *openstack-origin to: - '0' - '1' - '2' + keystone: charm: cs:~openstack-charmers-next/keystone num_units: 1 + options: + openstack-origin: *openstack-origin to: - '3' + glance: charm: ../../../glance num_units: 1 + options: + openstack-origin: *openstack-origin to: - '4' + +relations: + + - - "keystone:shared-db" + - "keystone-mysql-router:shared-db" + - - "glance:shared-db" + - "glance-mysql-router:shared-db" + - - "glance:identity-service" + - "keystone:identity-service" + - - "glance-mysql-router:db-router" + - "mysql-innodb-cluster:db-router" + - - "keystone-mysql-router:db-router" + - "mysql-innodb-cluster:db-router" diff --git a/unit_tests/test_glance_utils.py b/unit_tests/test_glance_utils.py index 78346fa5..f5d016a4 100644 --- a/unit_tests/test_glance_utils.py +++ b/unit_tests/test_glance_utils.py @@ -471,32 +471,30 @@ class TestGlanceUtils(CharmTestCase): utils.update_image_location_policy() self.assertFalse(mock_kv.called) + # Function has no effect in ussuri onwards + mock_os_release.return_value = 'ussuri' + utils.update_image_location_policy() + self.assertFalse(mock_kv.called) + mock_os_release.return_value = 'kilo' utils.update_image_location_policy() self.assertTrue(mock_kv.called) + mock_update_json_file.assert_has_calls([ call('/etc/glance/policy.json', - {'get_image_location': ''}), - call('/etc/glance/policy.json', - {'set_image_location': ''}), - call('/etc/glance/policy.json', - {'delete_image_location': ''})]) + {'get_image_location': '', + 'set_image_location': '', + 'delete_image_location': ''})]) mock_update_json_file.reset_mock() config['restrict-image-location-operations'] = True utils.update_image_location_policy() mock_update_json_file.assert_has_calls([ call('/etc/glance/policy.json', - {'get_image_location': 'role:admin'}), - call('/etc/glance/policy.json', - {'set_image_location': 'role:admin'}), - call('/etc/glance/policy.json', - {'delete_image_location': 'role:admin'})]) + {'get_image_location': 'role:admin', + 'set_image_location': 'role:admin', + 'delete_image_location': 'role:admin'})]) db_obj.get.assert_has_calls([call('policy_get_image_location'), call('policy_set_image_location'), call('policy_delete_image_location')]) - db_obj.set.assert_has_calls([call('policy_get_image_location', ''), - call('policy_set_image_location', ''), - call('policy_delete_image_location', - '')])