Cannot pass pillar data to the slack engine

[mdaffin]

Description of Issue/Question

Pillar data cannot be passed to the slack engine from slack or the engine config resulting in Pillar data must be formatted as a dictionary error message.

I expected it to work the same way as the command line. ie you should be able to copy and paste a command from the command line into slack or the cmd line in the alias.

Setup

/srv/salt/orch/ping.sls

{% set tgt = pillar.get('target', '*') %}
test-ping:
    salt.function:
    - name: test.ping
    - tgt: '{{ tgt }}'

/etc/salt/master.d/slack-engine.conf

engines:
    slack:
       token: '<token>'
       control: True
       valid_users:
           - <someuser>
       valid_commands:
           - state.orchestrate
           - ping-web
       aliases:
           ping-web:
               cmd: |
                   state.orchestrate orch.ping pillar='{"target": "web-*"}'

Steps to Reproduce Issue

From the salt master running salt-run state.orchestrate orch.ping pillar='{"target": "web-*"}' works as expected.

In slack running !state.orchestrate orch.ping pillar='{"target":"web-*"}' or !ping-web produces the following error in slack.

"Exception occurred in runner state.orchestrate: Traceback (most recent call last):\n  File \"/usr/lib/python2.7/site-packages/salt/client/mixins.py\", line 395, in _low\n    data['return'] = self.functions[fun](*args, **kwargs)\n  File \"/usr/lib/python2.7/site-packages/salt/runners/state.py\", line 54, in orchestrate\n    'Pillar data must be formatted as a dictionary'\nSaltInvocationError: Pillar data must be formatted as a dictionary\n"

I have tried a variety of different pillar data, including formatting is as a python dict '{target: "web-*"}'.

Versions Report

We also saw this in the previous version (2016.11.2).

Salt Version:
           Salt: 2016.11.3
 
Dependency Versions:
           cffi: 1.6.0
       cherrypy: 3.2.2
       dateutil: 1.5
          gitdb: 0.6.4
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.7.3
        libgit2: 0.21.4
        libnacl: Not Installed
       M2Crypto: 0.21.1
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.8
   mysql-python: Not Installed
      pycparser: 2.14
       pycrypto: 2.6.1
         pygit2: 0.21.4
         Python: 2.7.5 (default, Nov  6 2016, 00:28:07)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 15.3.0
           RAET: Not Installed
          smmap: 0.9.0
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.1.4
 
System Versions:
           dist: centos 7.3.1611 Core
        machine: x86_64
        release: 3.10.0-514.6.1.el7.x86_64
         system: Linux
        version: CentOS Linux 7.3.1611 Core

[Ch3LL]

@mdaffin looking at the code here and when i just pass that through using the python salt-api as shown below it works:

>>> local.cmd('thecakeisalie', 'state.orchestrate', ['orch.ping'], pillar='{"target": "web-*"}')
{'thecakeisalie': {'outputter': 'highstate', 'data': {'thecakeisalie': {'salt_|-cmd.run_|-cmd.run_|-function': {'comment': 'Function ran successfully. Function cmd.run ran on thecakeisalie.', 'name': 'cmd.run', 'start_time': '16:00:15.792520', 'result': True, 'duration': 152.273, '__run_num__': 0, '__jid__': '20170302160015811048', 'changes': {'ret': {'thecakeisalie': 'test'}, 'out': 'highstate'}, '__id__': 'cmd.run'}}}, 'retcode': 0}}

Can you print out kwargs and args on that command and see how its trying to pass this to localclient?

[whiteinge]

It might not be working because it looks like this engine is doing it's own name=val parsing and at quick glance it's not clear to me why. LocalClient also performs that work in addition to running args through parse_input (which is what turns name=value pairs into a data structure) so I think we could just use that existing behavior....BUT I'm also not clear on why we're relying on the string parsing at all since we can pass in a real data structure from YAML.

I might be missing something, but I think the cmd argument should preserve backward-compat but be changed to also optionally be a dict and bypass the custom parsing entirely. E.g.:

               cmd:
                 - fun: state.orchestrate
                 - kwarg:
                     mods: orch.ping
                     pillar:
                       target: 'web-*'

[mdaffin]

@Ch3LL output as requested:

cmd: state.orchestrate
args: ['orch.ping']
kwargs: {'pillar': '{"target": "web-*"}'}

@whiteinge it does look like that is the case. The above confirms it is just treating the part after the = as a string rather then decoding it like it does on the command line. I do like your suggestion of allowing full yaml support in the config but it does not solve the problem for non aliased messages from slack, such as !state.orchestrate orch.ping pillar='{"target":"web-*"}'. Ideally it should parse these in the same way as they are on the command line.

[mdaffin]

Note that since it is a runner command this is what is passed to the runner.cmd rather then the local.cmd

[whiteinge]

Oh! facepalm I see now why this module is using that format. I was thinking SLS files but it makes sense that it's parsing textual messages from Slack. Thanks, that makes sense now.

In that case, I think we can get this all the way working by simply packing all positional args into LocalClient's arg array. E.g., LocalClient().cmd('*', 'state.sls', arg=['one', 'two=true', 'three={four: Four}']). As of #39472 that will also work happily for RunnerClient too.

[nomeelnoj]

I am struggling to understand the decision to can to--what would be the process by which one would pass inline pillar data to an orchestration command via slack?

[whiteinge]

@nomeelnoj at the moment it looks like only string values can be given as arguments so I don't think it's possible to send Pillar data (which must be a dictionary) from either Slack or as an alias until this change is implemented.

[aphor]

Would it be better if the SlackClient parsed messages for aliases or some other special SlackClient syntax but fell back on using salt.client.caller.cmd() parsing since the slack engine commands seem to set up the expectation this is how it works?

[whiteinge]

@aphor (Stop me if I'm misreading your question) in my opinion:

Commands from Slack should ideally have exactly the same format as the Salt CLI. E.g., typing salt '*' state.sls pillar='{foo: Foo}' into Slack can have all the same parsing, behavior, and semantics as the CLI if the positional args are packed into the arg array (as mentioned above).

The Slack-specific aliases and whitelisting would just be value-adds on top of that native behavior to allow for common shortcuts and some light security.

[aphor]

@whiteinge I think you understood me well. Thanks for the answer.

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

If this issue is closed prematurely, please leave a comment and we will gladly reopen the issue.