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.07k stars 746 forks source link

Add parameter to local/remote shell runner to change delimiter in named parameters #2387

Open jfryman opened 8 years ago

jfryman commented 8 years ago

While assisting some users today, I ran into a problem with optparse. In a nutshell, parsing options is hard in Bash, so there are many tendencies to shortcut. One such example:

while [ -n "$1" ]; do
    case $1 in
        -C|--gecos)    gecos=$2;  shift 2;;
        -e|--email)    email=$2;  shift 2;;
        -u|--userid)   userid=$2; shift 2;;
        -d|--dry-run)  dryrun=1;  shift;;
        -h|--help)     usage;;
        *) shift;;
    esac
done

Today, I can change the kwargs_op to any prefix I so desire. But, in all cases, the input is distilled to a dict, and then rendered as key-value pairs. (e.g.: --email=james@fryman.io). This causes a failure with the above logic.

This is not an uncommon pattern, and it would be good to be able to s/=/ / in the above parameter.

jfryman commented 8 years ago

Some investigation from @m4dcoder: https://github.com/StackStorm/st2/blob/bc60c5508cad6605315daf2af9376a309c8a0754/st2common/st2common/models/system/action.py#L256

https://stackstorm-community.slack.com/archives/community/p1452804623012437

lakshmi-kannan commented 8 years ago

We can write this out as a jinja pattern in cmd today, right?

cmd: --c {{c}} --d {{d}}
manasdk commented 8 years ago

Does the cmd trick work for local-shell-script runner? I don't think the cmd parameter exists

jfryman commented 8 years ago

@lakshmi-kannan @manasdk fwiw, I'm not immediately blocked. The "workaround" was to fix the underlying script.

I avoided using the cmd jinja pattern hack as I think it's certainly an antipattern we need to watch carefully. It is supported (https://docs.stackstorm.com/runners.html?highlight=runner#runner-parameters). It is disjointed to have the ability to modify the kwargs_op option, but not have one for the delimiter between keyword parameters. If anything, dropping to the cmd and writing it out manually all but nullifies the need for kwargs_op, since I could define it in the cmd for that need as well.

manasdk commented 8 years ago

How about we go with kwarg_pattern whose value could take form --%s %s, -%s=%s, -%s %s or --%s=%s.

I am in fact suggesting an enum with a fixed list to avoid some weird shell injection.

powellchristoph commented 5 years ago

I got bit by this today. I don't see a fix.

bigmstone commented 5 years ago

@powellchristoph I'm not certain a patch was added. Can you provide us an example of action parameters you were using and any relevant logs to make sure we're addressing the specific issue you saw.

My current understanding of the issue is this happens when we shell out to action container. If so we might can just take the work done in https://github.com/StackStorm/st2/pull/3976 and just only do parameter input via stdin.

/cc @Kami @lakshmi-kannan for some more context

Kami commented 5 years ago

@powellchristoph What @bigmstone said.

Can you please also share your exact use-case with us, what's the desired input & behavior (ideally a command line string of the arguments you want to pass to your script) and what problems do you have?

powellchristoph commented 5 years ago

I have a simple bash script with named cli arguments that I am trying to use the local-shell-script with. I was able to change the kwarg_op seperator, but the runner still uses the = to separate k/v items.

So alittle script like this below would be called from the command like like: ./script.sh -r foo -w /tmp -s server -c client or ./script.sh -r foo -w /tmp I don't want to use positional args, because they aren't all required.

#!/bin/bash

while getopts r:w:c:s:m: option
do
    case "${option}" in
        r) REGION=${OPTARG//=/};;
        w) WORKDIR=${OPTARG//=/};;
        s) SERVER="ParameterValue=${OPTARG//=/}";;
        c) CLIENT="ParameterValue=${OPTARG//=/}";;
        m) MANIFEST=${OPTARG//=/}
            ;;
    esac
done

But using the following meta parameters. It gets passed by Stackstorm as ./script.sh -r=foo -w=/tmp -s=server -c=client

parameters:
  w:
    type: "string"
    required: true
  r:
    type: "string"
    required: true
  s:
    type: "string"
  c:
    type: "string"
  m:
    type: "string"
  kwarg_op:
    type: "string"
    immutable: true
    default: "-"

So now my args are passed to my script by getopts as =value and I have to use the ${OPTARG//=/} to strip it off.