Repository: canonical/operator
Pull request #809 (2022-08-03 13:46:19)

Documented and checked ActionEvent.set_results with "Stdout" key
Stderr output emitted during an action hook execution gets injected in an action's results by juju.
- documented this in the ActionEvent docstring
- added a check that users don't manually set_results to anything containing "Stdout", because it will either be overridden or override juju's or break something, not sure which one but either way, it ain't good.

## Checklist
 - [x] Do error messages, if any, present charm authors or operators with enough information to solve the problem?

## QA steps

Added test for the new error condition.

## Documentation changes

We should make this clear in the ActionEvent-related documentation.
Already updated: https://discourse.charmhub.io/t/action-name-action-event/6466

## Changelog

*Please include a bulleted list of items here, suitable for inclusion in a release changelog.*

- Added check that `ActionEvent.set_results` will fail if the results contain the 'special' juju key "Stdout"


rwcarlsen: Ha! - I actually fixed a bug in juju a while back related to this: https://github.com/juju/juju/pull/13748 - action-set should give you an error now (in new enough versions of juju).  So we don't need our own framework error, but possibly a framework-level error can be "nicer".
PietroPasotti: > Ha! - I actually fixed a bug in juju a while back related to this: [juju/juju#13748](https://github.com/juju/juju/pull/13748) - action-set should give you an error now (in new enough versions of juju). So we don't need our own framework error, but possibly a framework-level error can be "nicer".

Right, I still think it's best to raise our own error instead to wait for juju to error out.

PietroPasotti: > Here are the known "special" keys: "stdout", "stdout-encoding", "stderr", "stderr-encoding". Let's check for those as well.
I found the [juju source where the keys are being defined/used](https://github.com/juju/juju/blob/fd886a18e169e4741f557d6b574bba06ff0b190d/worker/uniter/runner/runner.go#L329), but there is an issue: 
![image](https://user-images.githubusercontent.com/6230162/183413156-bd2489c9-cbfd-49fd-90e6-7260eadb2b9b.png)
"Stderr" seems to be capitalized in this sample output. I don't see in the juju code where this capitalization is done, I'm wondering if it's an artifact from the juju client displaying the action results, rather than juju itself. It is suspicious that `admin-password` is not capitalized, however.
Should I include the capitalized or the non-capitalized version of "stdout" in the 'reserved' keys? 
And what about the other keys? Is it `Stdout-encoding` or `Stdout-Encoding`, or...?



juanmanuel-tirado: In 2.9 there is a piece of code that changes these keys to start with uppercase when running into compat mode. Check [here](https://github.com/juju/juju/blob/5f82824fc0e87316d48683949e82a0414b3cf0ce/cmd/juju/action/showoutput.go#L416). This has been changed in 3.0 [here](https://github.com/juju/juju/blob/0ffacc32b7bb871879695b1c4515047a09399fd0/cmd/juju/action/common.go#L694)
PietroPasotti: > 

Thanks! From reading that code it seems that ReturnCode is also a reserved key.
PietroPasotti: I just noticed that the regex we use to validate the action result key `'^[a-z0-9](([a-z0-9-.]+)?[a-z0-9])?$'`actually precludes using uppercase letters altogether. This was added I think by @jnsgruk. 
Is the regex too strict, or were you aware of these issues and took a different solution, or something else..?
jnsgruk: > I just noticed that the regex we use to validate the action result key `'^[a-z0-9](([a-z0-9-.]+)?[a-z0-9])?$'`actually precludes using uppercase letters altogether. This was added I think by @jnsgruk. Is the regex too strict, or were you aware of these issues and took a different solution, or something else..?

Interesting. Pretty sure a based the regex off something that lives in the Juju codebase (I can check a bit later if no one beats me to it!)
rwcarlsen: Do the juju checks need to be updated to check for ReturnCode (in addition to the stderr/out, etc.)?
PietroPasotti: > 

Do you mean the code on the juju side?
jnsgruk: > I just noticed that the regex we use to validate the action result key `'^[a-z0-9](([a-z0-9-.]+)?[a-z0-9])?$'`actually precludes using uppercase letters altogether. This was added I think by @jnsgruk. Is the regex too strict, or were you aware of these issues and took a different solution, or something else..?

Here it is: https://github.com/juju/juju/blob/137a772ed339b73b856e9adc0a5624976c2890b2/worker/uniter/runner/jujuc/action-set.go#L85

The `action-set` command validates that keys start with lowercase letters, so that's why the regex is as such in `ops` :)
PietroPasotti: Allright, so it seems that for now we're in the clear -- attempting to set Stdout/Stderr/ReturnCode and company will result into an error, not because they're 'protected keys', but because they contain uppercase characters.
I'll reshape this PR to only include documentation changes.
rwcarlsen: When I last dealt with this trying to solve https://github.com/canonical/operator/issues/689 - somewhere in juju, the lower-case-key names were still being clobbered by the CamelCase special ones (e.g. results "stdout" was clobbered by juju's special "Stdout").
rwcarlsen: I think the conclusion was that checks in juju should cover the relevant clobbering concerns.  So this PR is just doc clarifications and such.

Patch:
diff --git a/ops/charm.py b/ops/charm.py
index ca4ec0e..3bcec04 100755
--- a/ops/charm.py
+++ b/ops/charm.py
@@ -180,7 +180,8 @@ class ActionEvent(EventBase):
         event_action_name = self.handle.kind[:-len('_action')].replace('_', '-')
         if event_action_name != env_action_name:
             # This could only happen if the dev manually emits the action, or from a bug.
-            raise RuntimeError('action event kind does not match current action')
+            raise RuntimeError('action event kind ({}) does not match current '
+                               'action ({})'.format(event_action_name, env_action_name))
         # Params are loaded at restore rather than __init__ because
         # the model is not available in __init__.
         self.params = self.framework.model._backend.action_get()  # pyright: reportPrivateUsage=false  # noqa
@@ -190,6 +191,28 @@ class ActionEvent(EventBase):
 
         Args:
             results: The result of the action as a Dict
+            Juju eventually only accepts a str:str mapping, so we will attempt
+            to flatten any more complex data structure like so:
+            >>> {'a': 'b'} # becomes: 'a'='b'
+            >>> {'a': {'b': 'c'}} # becomes: 'a.b'='c'
+            >>> {'a': {'b': 'c', 'd': 'e'}} # becomes: 'a.b'='c', 'a.d' = 'e'
+            >>> {'a.b': 'c', 'a.d': 'e'} # equivalent to previous
+            Note that duplicate keys are not allowed, so
+            >>> {'a': {'b': 'c'}, 'a.b': 'c'} # invalid!
+
+            Note that the resulting keys must start and end with lowercase
+            alphanumeric, and can only contain lowercase alphanumeric, hyphens
+            and periods.
+
+            If any exceptions occur whilst the action is being handled, juju will
+            gather any stdout/stderr data (and the return code) and inject them into the
+            results object. Thus, the results object might contain the following keys,
+            additionally to those specified by the charm code:
+             - Stdout
+             - Stderr
+             - Stdout-encoding
+             - Stderr-encoding
+             - ReturnCode
         """
         self.framework.model._backend.action_set(results)   # pyright: reportPrivateUsage=false
 
diff --git a/ops/model.py b/ops/model.py
index e511d45..420032a 100644
--- a/ops/model.py
+++ b/ops/model.py
@@ -2102,6 +2102,10 @@ def _format_action_result_dict(input: Dict[str, 'JsonObject'],
 
     for key, value in input.items():
         # Ensure the key is of a valid format, and raise a ValueError if not
+        if not isinstance(key, str):
+            # technically a type error, but for consistency with the
+            # other exceptions raised on key validation...
+            raise ValueError('invalid key {!r}; must be a string'.format(key))
         if not _ACTION_RESULT_KEY_REGEX.match(key):
             raise ValueError("key '{!r}' is invalid: must be similar to 'key', 'some-key2', or "
                              "'some.key'".format(key))


Test patch:
diff --git a/test/test_charm.py b/test/test_charm.py
index fd0da1b..1d1f828 100755
--- a/test/test_charm.py
+++ b/test/test_charm.py
@@ -430,7 +430,15 @@ start:
   description: "Start the unit."
 ''')
 
-    def _test_action_events(self, cmd_type):
+    def _setup_test_action(self):
+        os.environ['JUJU_ACTION_NAME'] = 'foo-bar'
+        fake_script(self, 'action-get', """echo '{"foo-name": "name", "silent": true}'""")
+        fake_script(self, 'action-set', "")
+        fake_script(self, 'action-log', "")
+        fake_script(self, 'action-fail', "")
+        self.meta = self._get_action_test_meta()
+
+    def test_action_events(self):
 
         class MyCharm(CharmBase):
 
@@ -448,13 +456,7 @@ start:
             def _on_start_action(self, event):
                 pass
 
-        fake_script(self, cmd_type + '-get', """echo '{"foo-name": "name", "silent": true}'""")
-        fake_script(self, cmd_type + '-set', "")
-        fake_script(self, cmd_type + '-log', "")
-        fake_script(self, cmd_type + '-fail', "")
-        self.meta = self._get_action_test_meta()
-
-        os.environ['JUJU_{}_NAME'.format(cmd_type.upper())] = 'foo-bar'
+        self._setup_test_action()
         framework = self.create_framework()
         charm = MyCharm(framework)
 
@@ -465,10 +467,10 @@ start:
         charm.on.foo_bar_action.emit()
         self.assertEqual(charm.seen_action_params, {"foo-name": "name", "silent": True})
         self.assertEqual(fake_script_calls(self), [
-            [cmd_type + '-get', '--format=json'],
-            [cmd_type + '-log', "test-log"],
-            [cmd_type + '-set', "res=val with spaces"],
-            [cmd_type + '-fail', "test-fail"],
+            ['action-get', '--format=json'],
+            ['action-log', "test-log"],
+            ['action-set', "res=val with spaces"],
+            ['action-fail', "test-fail"],
         ])
 
         # Make sure that action events that do not match the current context are
@@ -476,8 +478,32 @@ start:
         with self.assertRaises(RuntimeError):
             charm.on.start_action.emit()
 
-    def test_action_events(self):
-        self._test_action_events('action')
+    def test_invalid_action_results(self):
+
+        class MyCharm(CharmBase):
+
+            def __init__(self, *args):
+                super().__init__(*args)
+                self.res = {}
+                framework.observe(self.on.foo_bar_action, self._on_foo_bar_action)
+
+            def _on_foo_bar_action(self, event):
+                event.set_results(self.res)
+
+        self._setup_test_action()
+        framework = self.create_framework()
+        charm = MyCharm(framework)
+
+        for bad_res in (
+                {'a': {'b': 'c'}, 'a.b': 'c'},
+                {'a': {'B': 'c'}},
+                {'a': {(1, 2): 'c'}},
+                {'a': {None: 'c'}},
+                {'aBc': 'd'}):
+            charm.res = bad_res
+
+            with self.assertRaises(ValueError):
+                charm.on.foo_bar_action.emit()
 
     def _test_action_event_defer_fails(self, cmd_type):