action_ref referential integrity not validated for action-alias

[nzlosh]

SUMMARY

When action-alias files are parsed and inserted into the database, invalid action_ref references are silently accepted.

ISSUE TYPE

Pick one below and delete the rest:

• Bug Report

STACKSTORM VERSION

st2 2.9dev, on Python 2.7.6```:


##### OS / ENVIRONMENT / INSTALL METHOD
```lsb_release -a
No LSB modules are available.
Distributor ID: Ubuntu
Description:    Ubuntu 14.04.5 LTS
Release:        14.04
Codename:       trusty

Install method: dev source installation

STEPS TO REPRODUCE

Add an action-alias to a pack that references a non-existent action and reload st2.

Simple example

---
name: "disable_upgrade"
description: "Disable the automatic package upgrade for a farm."
action_ref: "st2dm_upgrade_pkg.disable_upgrade"
formats:
  - display: "apu disable <role>"
    representation:
      - "apu disable {{role}}"
ack:
  enabled: true
  format: "StackStorm execution id `{{ execution.id }}`."
  append_url: true
result:
  enabled: true
  format: "{{ execution }}"

EXPECTED RESULTS

It would be useful to report a referential integrity error/warning when the action-alias file is parsed and references a non-existent action.

ACTUAL RESULTS

We see there is no pack named st2dm_upgrade_pkg.

st2 pack list 
+----------------------+----------------------+-------------------------------+---------+-------------------------------+
| ref                  | name                 | description                   | version | author                        |
+----------------------+----------------------+-------------------------------+---------+-------------------------------+
| chatops              | chatops              | Chatops integration pack      | 0.2.0   | Kirill Enykeev                |
| core                 | core                 | Basic core actions.           | 1.0.0   | StackStorm, Inc.              |
| linux                | linux                | Generic linux actions         | 1.0.0   | Stackstorm Inc                |
| packs                | packs                | Pack management               | 1.0.0   | StackStorm, Inc.              |
|                      |                      | functionality.                |         |                               |
| st2dm_apu            | st2dm_apu            | Automatic Package Upgrade     | 2.0.0   | Infrastructure Squad          |
|                      |                      | (APU) proccess.               |         |                               |
+----------------------+----------------------+-------------------------------+---------+-------------------------------+

Here we see the action-aliases defined to st2dm_apu of which disable_upgrade is used to show the problem.

st2 action-alias list                                                                    
+-----------------------------------+------------+--------------------------------------+---------+
| ref                               | pack       | description                          | enabled |
+-----------------------------------+------------+--------------------------------------+---------+
| st2dm_apu.disable_upgrade         | st2dm_apu  | Disable the automatic package        | True    |
|                                   |            | upgrade for a farm.                  |         |
| st2dm_apu.enable_upgrade          | st2dm_apu  | Enable the automatic package upgrade | True    |
|                                   |            | for a farm.                          |         |
| st2dm_apu.reset_upgrade           | st2dm_apu  | Reset the automatic package upgrade  | True    |
|                                   |            | on a farm.                           |         |
| st2dm_apu.start_upgrade           | st2dm_apu  | Start the automatic package upgrade  | True    |
|                                   |            | on a farm.                           |         |
| st2dm_apu.stop_upgrade            | st2dm_apu  | Stop the automatic package upgrade   | True    |
|                                   |            | on a farm.                           |         |
+-----------------------------------+------------+--------------------------------------+---------+

Here we see st2dm_apu.disable_upgade action alias is available with an action_ref that points to a non-existent action.

st2 action-alias get st2dm_apu.disable_upgrade
+-------------+--------------------------------------------------------------+
| Property    | Value                                                        |
+-------------+--------------------------------------------------------------+
| id          | 5bb37fb7d506c3cfe9c26276                                     |
| ref         | st2dm_apu.disable_upgrade                                    |
| pack        | st2dm_apu                                                    |
| name        | disable_upgrade                                              |
| description | Disable the automatic package upgrade for a farm.            |
| enabled     | True                                                         |
| action_ref  | st2dm_upgrade_pkg.disable_upgrade                            |
| formats     | [                                                            |
|             |     {                                                        |
|             |         "representation": [                                  |
|             |             "apu disable {{role}}"                           |
|             |         ],                                                   |
|             |         "display": "apu disable <role>"                      |
|             |     }                                                        |
|             | ]                                                            |
| ack         | {                                                            |
|             |     "append_url": true,                                      |
|             |     "enabled": true,                                         |
|             |     "format": "StackStorm execution id `{{ execution.id      |
|             | }}`."                                                        |
|             | }                                                            |
| extra       |                                                              |
| result      | {                                                            |
|             |     "enabled": true,                                         |
|             |     "format": "{{ execution }}"                              |
|             | }                                                            |
| uid         | action:st2dm_apu:disable_upgrade                             |
+-------------+--------------------------------------------------------------+

[Kami]

Thanks for reporting this.

IIRC, we had a discussion on this in the past (I would need to find it) and it was intentional - we allow users to create / register aliases for actions which are not yet registered when alias is registered.

Having said that, I will try to find that discussion and re-evaluate the decision.

At the very least (if there is a valid use case for the scenario I mentioned above), I do agree that we should at least warn on such scenario on alias register.

[LindsayHill]

I guess the problem is that you can't guarantee that packs are installed in a certain order (or indeed, if a dependent pack is installed at all).

If it threw a fatal error, you could be stuck in a circular dependency hell, unable to install certain combinations of packs. Perhaps needs some WARNING-level, or at least something more graceful?

Same situation applies to workflows or rules that reference other packs.

[nzlosh]

Thanks for the feedback. I can appreciate dependency hell complexity isn't easily solved and I tend to think a warning should be enough rather than cause fatal errors.

I don't how feasible or expensive such an operation would be, however, perhaps another option could be to perform post-import referential integrity in the DB that checks action-aliases, workflows and rules.

My thinking is: the user will need to pay for the invalid reference, either at run time or in performing referential checks up front and catching these earlier (at startup) is also how other coherency/syntax errors are reported.

[Kami]

@LindsayHill Yeah that's part of the problem - unlike other resources, aliases usually cross-reference actions from other packs.

@nzlosh I agree and in 99% of the cases I'm advocating for "fail early" (register / check time vs run-time) since it almost always provides better user experience and saves time troubleshooting and debugging things.

Having said that - would warning be a good compromise to begin with?

[LindsayHill]

So maybe this is the right UX:

At the end of all registration steps, do cross-reference of all workflows, rules, aliases to identify anything that refers to non-existent actions.

Raise non-fatal WARNING message if missing content found

Sound right?

[nzlosh]

That sounds like a reasonable UX to me. Would this change require an additional flag? Sort of along the lines of a -Wall for people who don't actually care about invalid references?

[lakshmi-kannan]

I'll pick this task as a warm up task for myself. No, I don't think a -Wall option is a good idea.

[lakshmi-kannan]

So I opened #4388 to address this and there are different registration paths.

• There is a tool registration path using st2ctl reload --register-all - This will spit out a warning.
• There is a CLI binding registration path st2 action-alias register /path/to/action/alias.yaml - This is tricky to introduce the warning because this would involve a change in the API model and the CLI.
• Then there is st2 pack register foo which is another path - Even here, including the warning would require change to API model (I think?)

@nzlosh Where do you think the warning makes most sense for you as a user? Do you expect to see a warning on all 3 ways to register that I outlined above? Just making sure we get the UX before I make those changes (they are significant). cc: @Kami, @LindsayHill

[nzlosh]

Thanks for the analysis @lakshmi-kannan. A naive response would be as a user I'd expect all paths to behave in the same way since it would be confusing to have one path warn on invalid references and another to silently accept them.

That said I've only ever used st2ctl reload --register-all so don't really have any expectation or direct experience with the other paths you mentioned.

[lakshmi-kannan]

@nzlosh With that, I'll just open a PR to spit out a warning for st2ctl reload --register-all for now but keep the issue open. We should address the other paths as part of v2 API overhaul. Opened an issue #4395 and tracking label for v2 APIs.

[nzlosh]

thanks 👍