invalid action parameters are silently ignored

[Samuel Allan]

When running an action with the juju command line client, invalid/unknown action parameters are silently ignored. An example scenario:

```
# typo: pol-name instead of pool-name
juju run woodpecker/0 fio pol-name=mypool
```

The above will happily run, but against the default pool from config, not mypool from typoed the command line.

Silently ignoring undefined parameters in this way can lead to subtle issues like the above scenario. Juju client should display an error message to the user when undefined parameters are used.

juju version: 3.3.1

[Joseph Phillips (manadart)]

Did the action result or logs indicate anything regarding the errant parameter name?

[Samuel Allan (samuelallan)]

No, the only place I can see mention of the extra params is in the 'show-operation' output. For example:

```
$ juju run tempest-k8s/0 get-lists test=true aribtrary-arg=yes foo=bar --format yaml
Running operation 25 with 1 task
  - task 26 on unit-tempest-k8s-0

Waiting for task 26...
tempest-k8s/0:
  id: "26"
  results:
    return-code: 0
    stdout: |
      readonly-quick
      refstack-2022.11
  status: completed
  timing:
    completed: 2024-01-29 00:52:43 +0000 UTC
    enqueued: 2024-01-29 00:52:42 +0000 UTC
    started: 2024-01-29 00:52:42 +0000 UTC
  unit: tempest-k8s/0

$ juju show-operation 25 --format yaml
summary: get-lists run on unit-tempest-k8s-0
status: completed
action:
  name: get-lists
  parameters:
    aribtrary-arg: true
    foo: bar
    test: true
...
```

[Alastair Flynn (aflynn50)]

After some investigation this behavior is because in the actions.yaml of the charm, there is a field that is true by default:

additionalProperties:true

It is possible to set this in the actions.yaml to false in which case an error will be thrown when an invalid key is passed to the action.

A fix for this bug would be to set the default value of this setting to false, however this will be a breaking change. This can be done in one of the upcoming minor version releases of juju.

[Samuel Allan (samuelallan)]

Thanks for investigating Alastair. :) I went looking for where this is documented, and I found a reference to additionalProperties in point 3 under https://juju.is/docs/sdk/actions-yaml#heading--structure . It doesn't specify what the default is or how additionalProperties there affects the behaviour though.

Does it make sense to allow additional properties? Can the charm even access them? It doesn't seem to make sense to me for the charm to access properties that weren't defined in the action params. Unless perhaps there is a special case of an action allowing to set arbitrary keys (eg. juju run mydb/0 set-values key1=value1 key2=anothervalue ...)?

[Simon Richardson (simonrichardson)]

This is unfortunately a breaking change.

> Can the charm even access them?

The additional arguments are indeed passed through to the `events.params` (if you're using the ops framework).

> Does it make sense to allow additional properties?

At the very least that's how it works now. We'd have to roll out a way to let people know that they need to opt-in to allowing additionalProperties.

For your current circumstance I believe `required` would be the better option. Using `required` forces that all the properties are correct and a spelling mistake won't break.

[Simon Richardson (simonrichardson)]

I'm currently discussing the path forward on this one, hence why I've taken my name off it. We can look into improving the documentation to highlight this better.

[Samuel Allan (samuelallan)]

Thanks, yes this makes sense. Agreed that the documentation can be improved to make it clear what the behaviour is.

> Using `required` forces that all the properties are correct and a spelling mistake won't break.

Wouldn't `required` force the properties to be provided? eg. then the params aren't optional.

For the project we're working on, `additionalProperties: false` gave the desired behaviour: https://review.opendev.org/c/openstack/sunbeam-charms/+/907692