Repository: canonical/operator (language: python)

Issue:
OF should gracefully handle blank application names in relation-broken hooks
See the discussion here: https://bugs.launchpad.net/juju/+bug/1960934

In a relation-broken hook, Juju does not set JUJU_REMOTE_APP. This is Working as Intended for Juju, but it raises an exception in the framework, when iterating through relations.

We need to refactor so that we handle the empty value without throwing the exception. 


Discussion / hints:
I could be convinced to set JUJU_REMOTE_APP but you'd still want to handle
the case in OF for versions of Juju that don't have that set.


On Tue, Feb 15, 2022 at 3:36 PM Pen Gale ***@***.***> wrote:

> See the discussion here: https://bugs.launchpad.net/juju/+bug/1960934
>
> In a relation-broken hook, Juju does not set JUJU_REMOTE_APP. This is
> Working as Intended for Juju, but it raises an exception in the framework,
> when iterating through relations.
>
> We need to refactor so that we handle the empty value without throwing the
> exception.
>
> —
> Reply to this email directly, view it on GitHub
> <https://github.com/canonical/operator/issues/693>, or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AABRQ7J3PYBB5AK7XMNUEMDU3K2MLANCNFSM5OPUSLHQ>
> .
> Triage notifications on the go with GitHub Mobile for iOS
> <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
> or Android
> <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
>
> You are receiving this because you are subscribed to this thread.Message
> ID: ***@***.***>
>

I want to mention that I've encountered this type of errors before [1], and that they still happen for the nginx-ingress-integrator charm when a related application is removed (the charm iterates over its relations in order to establish what Kubernetes Services and Ingress Resources need to be created / removed).

[1] https://github.com/finos/legend-juju-gitlab-integrator/pull/6
If someone is e.g. using the remote app name to access relation data (`relation.data[relation.app].get(...)`), what should happen?  Should we just have `relation.data[relation.app]` return an empty dictionary and log an error?  Or something else...
According to the traceback (why does python reverse the compound word convention from c/c++?) in the linked juju issue, and `relation.data[relation.app].get(...)` is failing in the `.get(...)` call.  And the `relation-get` hook tool is failing with an empty app name.  This means that relation.app is an existing instance with an empty app name.  I've traced this issue to https://github.com/canonical/operator/blob/e3bc30bf8491716d33c8bb6061eb63bd8306ca1d/ops/model.py#L1642 where we end up returning an empty string instead of None.  The calling code for this function only checks for None (not empty string) as a special case.  I would argue that this is a bug that we can fix easily.  However, this doesn't really resolve the question I asked above - users will instead just get a keyerror for e.g. `relation.data[relation.app]`  because now relation.app will be None.  If the actual relation data still does exist and `relation-get` calls can succeed, how can ops get access to the (gone?) remote app's name?

Fix patch:
diff --git a/ops/model.py b/ops/model.py
index 84257366d..45e6a48d8 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -887,6 +887,15 @@ def __iter__(self):
         return iter(self._data)
 
     def __getitem__(self, key: 'UnitOrApplication'):
+        if key is None and self.relation.app is None:
+            # NOTE: if juju gets fixed to set JUJU_REMOTE_APP for relation-broken events, then that
+            # should fix the only case in which we expect key to be None - potentially removing the
+            # need for this error in future ops versions (i.e. if relation.app is guaranteed to not
+            # be None. See https://bugs.launchpad.net/juju/+bug/1960934.
+            raise KeyError(
+                'Cannot index relation data with "None".'
+                ' Are you trying to access remote app data during a relation-broken event?'
+                ' This is not allowed.')
         return self._data[key]
 
     def __repr__(self):
@@ -2081,7 +2090,7 @@ def relation_remote_app_name(self, relation_id: int) -> Optional[str]:
             event_relation_id = int(os.environ['JUJU_RELATION_ID'].split(':')[-1])
             if relation_id == event_relation_id:
                 # JUJU_RELATION_ID is this relation, use JUJU_REMOTE_APP.
-                return os.environ['JUJU_REMOTE_APP']
+                return os.getenv('JUJU_REMOTE_APP') or None
 
         # If caller is asking for information about another relation, use
         # "relation-list --app" to get it.


Test patch:
diff --git a/ops/testing.py b/ops/testing.py
index a116104fb..c11080cca 100755
--- a/ops/testing.py
+++ b/ops/testing.py
@@ -1175,9 +1175,21 @@ def relation_remote_app_name(self, relation_id: int) -> typing.Optional[str]:
         if relation_id not in self._relation_app_and_units:
             # Non-existent or dead relation
             return None
+        if 'relation_broken' in self._hook_is_running:
+            # TODO: if juju ever starts setting JUJU_REMOTE_APP in relation-broken hooks runs,
+            # then we should kill this if clause.
+            # See https://bugs.launchpad.net/juju/+bug/1960934
+            return None
         return self._relation_app_and_units[relation_id]['app']
 
     def relation_get(self, relation_id, member_name, is_app):
+        if 'relation_broken' in self._hook_is_running and not self.relation_remote_app_name(
+                relation_id):
+            # TODO: if juju gets fixed to set JUJU_REMOTE_APP for this case, then we may opt to
+            # allow charms to read/get that (stale) relation data.
+            # See https://bugs.launchpad.net/juju/+bug/1960934
+            raise RuntimeError(
+                'remote-side relation data cannot be accessed during a relation-broken event')
         if is_app and '/' in member_name:
             member_name = member_name.split('/')[0]
         if relation_id not in self._relation_data:
@@ -1185,6 +1197,10 @@ def relation_get(self, relation_id, member_name, is_app):
         return self._relation_data[relation_id][member_name].copy()
 
     def relation_set(self, relation_id, key, value, is_app):
+        if 'relation_broken' in self._hook_is_running and not self.relation_remote_app_name(
+                relation_id):
+            raise RuntimeError(
+                'remote-side relation data cannot be accessed during a relation-broken event')
         relation = self._relation_data[relation_id]
         if is_app:
             bucket_key = self.app_name
diff --git a/test/test_model.py b/test/test_model.py
index b672b84d1..4cd20a039 100755
--- a/test/test_model.py
+++ b/test/test_model.py
@@ -715,12 +715,9 @@ def test_storage(self):
         self.assertEqual(len(model.storages), 2)
         self.assertEqual(model.storages.keys(), meta.storages.keys())
         self.assertIn('disks', model.storages)
-        try:
+
+        with pytest.raises(KeyError, match='Did you mean'):
             model.storages['does-not-exist']
-        except KeyError as err:
-            assert 'Did you mean' in str(err), 'got wrong error message'
-        except Exception as err:
-            assert False, 'got wrong exception type: ' + str(err)
 
         test_cases = {
             0: {'name': 'disks', 'location': pathlib.Path('/var/srv/disks/0')},
@@ -968,7 +965,6 @@ def test_recursive_push_and_pull(case):
 
     errors = []
     try:
-        print(push_path, case.dst)
         c.push_path(push_path, case.dst)
     except ops.model.MultiPushPullError as err:
         if not case.errors:
@@ -978,9 +974,6 @@ def test_recursive_push_and_pull(case):
     assert case.errors == errors, \
         'push_path gave wrong expected errors: want {}, got {}'.format(case.errors, errors)
     for fpath in case.want:
-
-        for f in ops.model.Container._list_recursive(c.list_files, pathlib.Path('/')):
-            print(f)
         assert c.exists(fpath), 'push_path failed: file {} missing at destination'.format(fpath)
 
     # create pull test case filesystem structure
diff --git a/test/test_testing.py b/test/test_testing.py
index 265f27291..32326b9bb 100644
--- a/test/test_testing.py
+++ b/test/test_testing.py
@@ -25,6 +25,7 @@
 import unittest
 from io import BytesIO, StringIO
 
+import pytest
 import yaml
 
 import ops.testing
@@ -318,6 +319,24 @@ def _on_cluster_relation_changed(self, event):
         self.assertTrue(len(harness.charm.observed_events), 1)
         self.assertIsInstance(harness.charm.observed_events[0], RelationEvent)
 
+    def test_relation_get_when_broken(self):
+        harness = Harness(RelationBrokenTester, meta='''
+            name: test-app
+            requires:
+                foo:
+                    interface: foofoo
+            ''')
+        self.addCleanup(harness.cleanup)
+        harness.begin()
+        harness.charm.observe_relation_events('foo')
+
+        # relation remote app is None to mirror production juju behavior where juju doesn't
+        # communicate the remote app to ops.
+        rel_id = harness.add_relation('foo', None)
+
+        with pytest.raises(KeyError, match='trying to access remote app data'):
+            harness.remove_relation(rel_id)
+
     def test_remove_relation(self):
         harness = Harness(RelationEventCharm, meta='''
             name: test-app
@@ -2565,6 +2584,7 @@ def __init__(self, framework):
         self.record_relation_data_on_events = False
 
     def observe_relation_events(self, relation_name):
+        self.relation_name = relation_name
         self.framework.observe(self.on[relation_name].relation_created, self._on_relation_created)
         self.framework.observe(self.on[relation_name].relation_joined, self._on_relation_joined)
         self.framework.observe(self.on[relation_name].relation_changed, self._on_relation_changed)
@@ -2607,6 +2627,16 @@ def _observe_relation_event(self, event_name, event):
         self.changes.append(recording)
 
 
+class RelationBrokenTester(RelationEventCharm):
+    """Access inaccessible relation data."""
+
+    def __init__(self, framework):
+        super().__init__(framework)
+
+    def _on_relation_broken(self, event):
+        print(event.relation.data[event.relation.app]['bar'])
+
+
 class ContainerEventCharm(RecordingCharm):
     """Record events related to container lifecycles."""