Skip to content

Commit

Permalink
Merge pull request juju#13748 from rwcarlsen/action-key-guard
Browse files Browse the repository at this point in the history
juju#13748

When charms set action results/keys via action-set, it is possible that the charm sets "special" action keys (i.e. stdout, stderr) via the key=val args. This caused a race condition between juju collecting stdout/stderr from running the action hook (i.e. worker/uniter/runner runner.updateActionResults) and the hook's call to "action-set [key=val]..." resulting in one of the action-set key+val or the hook stdout winning and clobbering the other in the controller. It also caused other unexpected behavior if the charm set the stdout result key to the empty string (the "stdout" key would be omitted from results entirely) or showing the content of the "stdout" result key under the capitalized "Stdout" key in the final client results printout, etc.

To resolve all these issues this PR adds a check to ensure that none of the keys being set via action-set are in a reserved set that juju uses for special purposes under the hood and errors out accordingly.

This was originally reported and investigated via canonical/operator#689.

## QA steps

I added a unit test:

```
cd worker/uniter/runner/jujuc
go test
```

canonical/operator#689 details how the problem was initially encountered - basically calling the hook tool like this `action-set stdout=42` was enough to trigger unexpected behavior, but should now just cause an error to be emitted with this PR. More specifically, an ops charm with an action that has this code will cause clobbering - and we'd prefer an error (i.e. this PR/patch):

```
 def _on_foo_action(self, event)
 event.set_results({'stdout': 'forty-two'})
 print(42)
```
It is a race between:

* the runner recording `42` from the hook's stdout and
* the action-set called by the ops framework using event.set_results content - `forty-two`. 

This patch causes an error to be emitted when running such an action instead of the race/clobbering.

## Documentation changes

Included.
  • Loading branch information
jujubot authored Mar 9, 2022
2 parents 8cd3338 + e16eb18 commit 64c83e5
Show file tree
Hide file tree
Showing 2 changed files with 21 additions and 4 deletions.
17 changes: 14 additions & 3 deletions worker/uniter/runner/jujuc/action-set.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@ import (

var keyRule = charm.GetActionNameRule()

// list of action result keys that are not allowed to be set by users
var reservedKeys = []string{"stdout", "stdout-encoding", "stderr", "stderr-encoding"}

// ActionSetCommand implements the action-set command.
type ActionSetCommand struct {
cmd.CommandBase
Expand All @@ -30,11 +33,13 @@ func NewActionSetCommand(ctx Context) (cmd.Command, error) {

// Info returns the content for --help.
func (c *ActionSetCommand) Info() *cmd.Info {
doc := `
reservedText := `"` + strings.Join(reservedKeys, `", "`) + `"`
doc := fmt.Sprintf(`
action-set adds the given values to the results map of the Action. This map
is returned to the user after the completion of the Action. Keys must start
and end with lowercase alphanumeric, and contain only lowercase alphanumeric,
hyphens and periods.
hyphens and periods. The following special keys are reserved for internal use:
%s.
Example usage:
action-set outfile.size=10G
Expand All @@ -51,7 +56,7 @@ Example usage:
bar:
zab: "4"
baz: "1"
`
`, reservedText)
return jujucmd.Info(&cmd.Info{
Name: "action-set",
Args: "<key>=<value> [<key>=<value> ...]",
Expand Down Expand Up @@ -79,6 +84,12 @@ func (c *ActionSetCommand) Init(args []string) error {
if valid := keyRule.MatchString(key); !valid {
return fmt.Errorf("key %q must start and end with lowercase alphanumeric, and contain only lowercase alphanumeric, hyphens and periods", key)
}

for _, reserved := range reservedKeys {
if reserved == key {
return fmt.Errorf(`cannot set reserved action key "%s"`, key)
}
}
}
// [key, key, key, key, value]
c.args = append(c.args, append(keySlice, thisArg[1]))
Expand Down
8 changes: 7 additions & 1 deletion worker/uniter/runner/jujuc/action-set_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,11 @@ func (s *ActionSetSuite) TestActionSet(c *gc.C) {
command: []string{"result-Value=5"},
errMsg: "ERROR key \"result-Value\" must start and end with lowercase alphanumeric, and contain only lowercase alphanumeric, hyphens and periods\n",
code: 2,
}, {
summary: "reserved key is an error",
command: []string{"stdout=foo"},
errMsg: "ERROR cannot set reserved action key \"stdout\"\n",
code: 2,
}, {
summary: "empty values are not an error",
command: []string{"result="},
Expand Down Expand Up @@ -153,7 +158,8 @@ Details:
action-set adds the given values to the results map of the Action. This map
is returned to the user after the completion of the Action. Keys must start
and end with lowercase alphanumeric, and contain only lowercase alphanumeric,
hyphens and periods.
hyphens and periods. The following special keys are reserved for internal use:
"stdout", "stdout-encoding", "stderr", "stderr-encoding".
Example usage:
action-set outfile.size=10G
Expand Down

0 comments on commit 64c83e5

Please sign in to comment.