StackStorm / st2

StackStorm (aka "IFTTT for Ops") is event-driven automation for auto-remediation, incident responses, troubleshooting, deployments, and more for DevOps and SREs. Includes rules engine, workflow, 160 integration packs with 6000+ actions (see https://exchange.stackstorm.org) and ChatOps. Installer at https://docs.stackstorm.com/install/index.html
https://stackstorm.com/
Apache License 2.0
6.04k stars 745 forks source link

Immutable/default action parameters for aliases (ChatOps) #1704

Closed arm4b closed 4 years ago

arm4b commented 9 years ago

There is already mechanism to set optional parameter in alias:

formats:
  - "google {{query=StackStorm}}"

Problem

But what if we want to add simple hardcoded chatops command and hide unneeded logic behind the default/immutable action parameters? I think that good ChatOps commands could be simple and easy to remember, without MANY optional parameters. Example case: deploy chatops

To do that, we need to create additional action with default parameters, which will trigger another action. I do this way almost everywhere (bad way): https://github.com/armab/st2-chatops-aliases/tree/master/actions

Proposal

The proposal is to allow setting from alias default/immutable action parameters.

Here is an example how it could look:


---
name: chatops.deploy
action_ref: packs.install
description: Deploy ChatOps pack from `armab/st2-ansible-chatops` GitHub repo
formats:
  - "deploy chatops"
# these lines were added
# naming/structure is doubtful, but you got the idea
action_parameters:
  packs: "st2-ansible-chatops"
  repo_url: "armab/st2-ansible-chatops"

Here you can see that behind simple and immutable/hardcoded ChatOps command:

!deploy chatops

stands:

st2 run packs.install packs=st2-ansible-chatops repo_url=armab/st2-ansible-chatops

This simplifies things a lot, making existing actions more reusable by aliases.

arm4b commented 9 years ago

Another limitation is when there is need to edit parameter value coming from chat. Here is an example when I need to trim unneeded http:// from parameter (added by slack chat).

Instead of writing this action: https://github.com/armab/st2-chatops-aliases/blob/master/actions/server_status.yaml#L20 and use action in alias: https://github.com/armab/st2-chatops-aliases/blob/master/aliases/server_status.yaml

I could do everything in single alias:

---
name: chatops.server_status
action_ref: ansible.command
description: Show status for hosts
formats:
  - "status {{host}}"
# added
action_parameters:
  hosts: "{{ host|replace('http://', '') }}"
  module-name: ping

The intent is to avoid ugly behavior when one action should be created to trigger another action, because alias is limited.

Before: alias -> helper action (pre-processing) -> core action -> After: alias -> core action

manasdk commented 9 years ago

@armab - appreciate the report.

Immutable parameters

My initial gut is that this make the ActionAlias look too much like an Action definition so I wanted to avoid doing so however after I think about it more it pushes towards a fewer core actions and some more aliases.

The intent is to avoid ugly behavior when one action should be created to trigger another action, because alias is limited.

In reality adding an extra action to support specialized default or immutable values is perfectly reasonable but it does mean that an Alias author is also required to be an action author. Seeing that this adds flexibility I believe there is value in supporting immutable param in an ActionAlias.

Transforming parameter values

This one I am on the fence about. This really makes ActionAliases way more complex and to some extent takes away from the intent of an ActionAlias.

In the case of {{ host|replace('http://', '') }} is an issue with the medium i.e. slack so the best place to handle is the adapter. the ActionAlias has no context about the specific text based application that is being used here and not all chat clients will behave the same therefore the normalization would have to come from the medium adapter.

Generally though I am unsure if we should support such transformations. I would want to see atleast a handful(2 maybe) of usecases to be convinced about this one.

manasdk commented 9 years ago

@jfryman - Thoughts? You may have had similar experiences using ActionAlias and can share some data points to help figure out transformation support.

arm4b commented 9 years ago

Of course, under {{ host|replace('http://', '') }} it could be any Jinja-powered expression like conditional (what is pretty flexible). Creating additional action to do that - looks like overcomplication for me.

^^ that was just real example.

arm4b commented 9 years ago

Case 3. Parameter renaming for ChatOps

Imagine you don't like parameter name inside some action. For ChatOps you want something more readable, because ChatOps commands are simple shortcuts/representation for more complex things.

Let's rename it. For the moment it's possible to do only by creating additional action in the middle. Here is an example how it could be with single action alias:

---
name: chatops.server_status
action_ref: ansible.command
description: Show status for hosts
formats:
  # avoid this
  #- "status {{some_strange_and_long_parameter}}"
  - "status {{host}}"
# added
action_parameters:
  # note parameter name
  some_strange_and_long_parameter: "{{ host }}"

Case 4. Inject several alias parameters into acton parameter value

This action and this alias could be transformed into single alias:

---
name: chatops.ansible_package_update
action_ref: ansible.command
description: Update package on remote hosts
formats:
  - "update {{package}} on {{hosts}}"
# added
action_parameters:
  # note 2 variables coming from chatops
  # and injected into one action parameter
  extra-vars: "hosts={{ hosts }} package={{ package }}"
manasdk commented 9 years ago

Parameter renaming

The user sitting in chat is never really exposed to parameters except help. The person writing the ActionAlias is but there is no getting around that in either case. Also, if a param is poorly named/inconvenient then fixing the action is perhaps the better approach - right? It is true that not actions will be editable and/or the action could be from a curated and safe to consume as-is pack with no intent to modify the pack.

Case 4. Inject several alias parameters into acton parameter value

Alright this is beginning to get a little extreme but I hear your overall point.

My primary push-back is that an ActionAlias is not an action and is quicky beginning to pick up action semantics.

However, I see that having an ability to define/re-define parameters does provide flexibility. So maybe you won me over :smile: . The impl may have limitations along the lines of once an ActionAlias contains a parameters section then those are the only parameters used for execution.

arm4b commented 9 years ago

Personally I found this approach with creating middle-man actions pretty confusing.

This looked ugly for me: action alias -> helper action -> ansible integration pack action -> ansible binary

So I removed ansible pack from the chain to reduce number of agents: action alias -> helper action -> ansible binary what means that I don't need ansible pack anymore (felt strange here). What's the reason to use pack if I can use binary here and reduce complexity?

So after all, good logic for me looked like: action alias -> ansible integration pack action

Thus core and integration packs could be more reusable. Alias could be something like 1-click shortcut behind the real action, without middle-man.

manasdk commented 9 years ago

This would definitely inject more capabilities in ActionAlias. I suspect it might have impact on Action and ActionAlias interplay; lets see how this ends up.

jfryman commented 9 years ago

To @armab's point, there are still a lot of layers to wire something up to ChatOps. I ran into a similar problem, where I had to create an alias, create a new workflow, and then tie those together to wrap my chatops command up in sufficient way. (see https://github.com/frymanio/st2-frymanio). I sort of reconciled this at first as it allows me to have workflows I can expand upon later, and the scaffolding is there. But, this quickly became burdensome for the simple and quick actions.

I like this proposal, because it's cutting down on friction for the beginning uses.

In terms of transforming data: it's clear we need an answer here. It keeps coming up, and shelling out to some helper action to transform data isn't ideal. I would be open to explore the transformation in an alias. Initially I thought the ActionAlias isn't the place for this, but if we cut out Workflow, then it's the only logical place for it to be.

The way I look at this proposal is exactly the same thing we talk about when we express workflow. There needs to be a clear way to restrict the actions of a user to select actions. Creates trust and enforces safety. If there is a way to do this without having all the scaffolding, I think that's a win.

pixelrebel commented 8 years ago

I’m working on an action alias that wraps an ansible.playbook action. I need to specify the playbook and inventory files in the alias yaml file. I tried adding a parameters block as described here: https://docs.stackstorm.com/chatops/aliases.html#key-value-parameters , but this fails to load with error:

ValidationError: Additional properties are not allowed ('parameters' was unexpected)

Is this the crux of this issue, or am I just doing it wrong?

manasdk commented 8 years ago

@pixelrebel What version of stackstorm are you using?

Please run st2 --version

pixelrebel commented 8 years ago

Typo in my original comment, here's the version:

$ st2 --version
st2 ('1.2.0.8',)
manasdk commented 8 years ago

@pixelrebel

Did you by any chance add parameters to one of you alias? In that case it may be some misleading docs.

What the doc say is the even if your format is google {{query}} and you type google StackStorm limit=10 we will automatically assume limit is a parameter you would like your action to receive.

You do not have to define parameters anywhere explicitly.

pixelrebel commented 8 years ago

So how would I hardcode the inventory_file for an ansible.playbook alias? I don't want to have to type the full path to this file in my chatops command.

Here's my alias for example:

---
name: "show_arp"
description: "Show arp for host or inventory group."
action_ref: "ansible.playbook"
parameters:
  module_path: "/opt/stackstorm/packs/net/ansible/library"
  inventory_file: "/opt/stackstorm/packs/net/ansible/inventory"
  playbook: "/opt/stackstorm/packs/net/ansible/show_arp.yml"
formats:
  - display: "net show arp for <host|all>"
    # Workaround for optional arguments in an ({{optional}} regex)? failing to validate as null
    representation:
      - "net show arp for {{limit}}[!.]?"
      - "net show arp[!.]?"
ack: 
  format: "Showing arp tables for `{{execution.parameters['limit']}}`. ({{execution.id}})"
  append_url: false
result:
  format: "{{execution.result.result}}"
pixelrebel commented 8 years ago

I was able to work around the issue by moving the parameters into the optional regex with a default values:

representation:
  - "net show arp( for {{limit=all}})?( '{{playbook=/opt/stackstorm/packs/net/ansible/show_arp.yml}}' '{{inventory_file=/opt/stackstorm/packs/net/ansible/inventory}}')?"
manasdk commented 8 years ago

Nice yeah jinja hackery to the max!

With this issue we are actually working out if parameters should be added to ActionAlias. Some of what is here can also be pushed down to Action definition. However, adding parameters to the alias definition might be the only option if there is no control over an existing action.

emedvedev commented 8 years ago

I'd just like to point out that this hackery relies on the current parser implementation which could change in the future, so it's not reliable.

That said, I'm using something similar myself. To make your parameters truly immutable you can do OH MY GOD SUPER HACKY DANGER ZONE something like restart hubot( {{ cmd="sudo docker restart hubot" }}){0}(?=$) to pass a parameter and forbid its modification with an end-of-string lookahead.

pixelrebel commented 8 years ago

For an action like ansible.playbook it makes much more sense to set parameters in the action-alias. Consider two playbooks: show_arp.yml and show_vlans.yml These could be executed with separate action-aliases referencing the same action. But the chatops command should be a simple ! show vlans I shouldn't have to remember the full path to the playbooks when executing a chatops command.

Like you said, I could create a new action for each playbook, but that seems like a lot of repetitive code.

pixelrebel commented 8 years ago

@emedvedev: Agreed, I opened an issue https://github.com/StackStorm/st2/issues/2295 regarding flaws in the current approach. These two issues are very much related.

Also, that's some nice regex-fu!

hd-deman commented 8 years ago

Any updates on this?

emedvedev commented 8 years ago

You can reasonably expect it in one of the next two releases. Since the latest one was focused on BWC and some other stuff, the feature didn't make it into the shortlist, but it's long due, so, again, one of the next two. :)

On 12 Sep 2016 18:51 -0500, Dmitry Tsoy notifications@github.com, wrote:

Any updates on this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub (https://github.com/StackStorm/st2/issues/1704#issuecomment-246531405), or mute the thread (https://github.com/notifications/unsubscribe-auth/AAI3QqQgrpw26Kib5TqsMDlPGZ8FykYlks5qpeWIgaJpZM4FUw6q).

estee-tew commented 7 years ago

https://stackstorm.slack.com/archives/community/p1493039417041051

namachieli commented 6 years ago

Hi,

I have a similar use case.

I have an action alias I would like to be able have users call, that uses the same parameter values.

The proposed "action-parameters" would solve my use case perfectly.

action_ref: "servicenow.get"
description: "list unassigned Tickets in the Network team's Queue"
formats:
  - display: "list unassigned servicenow"
    representation:
    - "(unassigned|network|network team|networking|ticket|tickets).*(servicenow|service now|service-now|tickets)"
action-parameters:
  query: {\"active:\"true\",\"assignment_group\":\"aebf964c4f8a3e00c92a2cee0210c734\"\"assigned_to\":\"^state!=6^EQ\"
  table: "incident"
nicholasamorim commented 5 years ago

Same use case here, pretty much:

When using chatops, I started defining certain action aliases. One alias I set up was basically deploy app, that in turn would trigger an ansible-playbook action with certain parameters.

Those parameters are static (playbook X, folder Y) and shouldn't be changed with user input.

I was then surprised to find that action aliases do not support default/static parameters. You can define in the format such as deploy web {{playbook="someplay"}} but there are valid use-cases for simply having static parameters being passed to the action. Having a default in the format looks like a workaround for the time being but that's all that it is.

I think Action Aliases could benefit from something like action_parameters where we define certain parameters to be passed to the action that do not depend on user input and should not be changed.

nicholasamorim commented 5 years ago

I've created a PR to support this feature in https://github.com/StackStorm/st2/pull/4786

I'd appreciate reviews and suggestions in order to get this merged.