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

@@ -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']
 

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:

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)

templates/ussuri/policy.json

@@ -1,5 +0,0 @@
-{
-    "delete_image_location": "rule:default",
-    "get_image_location": "rule:default",
-    "set_image_location": "rule:default"
-}

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 }}"

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"

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',
-                                              '')])