StackStorm / hubot-stackstorm

Hubot plugin for integration with StackStorm event-driven infrastructure automation platform.
Apache License 2.0
49 stars 39 forks source link

chatops.post_message seems to be ignoring Slack pretext #162

Open lingfish opened 5 years ago

lingfish commented 5 years ago

Originally logged here: https://github.com/StackStorm/st2/issues/4561, @nzlosh suggested I might re-log it here.

SUMMARY

chatops.post_message seems to be ignoring Slack pretext.

ISSUE TYPE

Pick one below and delete the rest:

STACKSTORM VERSION

st2 2.10.1, on Python 2.7.6

OS / ENVIRONMENT / INSTALL METHOD

Standard docker install.

STEPS TO REPRODUCE
st2 run chatops.post_message channel=testing message='To get a list of commands type: ```!help```' extra='{"slack": {"color":"#f48527","pretext":"Hey <!channel>, Ready for ChatOps?","title": "Ansible and ChatOps. Get started :rocket:","title_link":"https://stackstorm.com/2015/06/24/ansible-chatops-get-started-%f0%9f%9a%80/","author_name":"by StackStorm - IFTTT for Ops","author_link":"https://stackstorm.com/","author_icon":"https://stackstorm.com/wp/wp-content/uploads/2015/01/favicon.png","image_url":"https://i.imgur.com/HWN8T78.png","fields":[{"title":"Documentation","value":"https://docs.stackstorm.com/chatops/","short":true}]}}' > /dev/null
EXPECTED RESULTS

A pretext line in the Slack channel.

ACTUAL RESULTS

Pretext line missing.

blag commented 5 years ago

Is the rest of the Slack message appearing? With the correct color, title, title link, author, author link, author icon, image, and fields?

lingfish commented 5 years ago

Yes, certainly is (sorry, should have mentioned that).

lingfish commented 5 years ago

Any thoughts here? Anything I can do to help?

blag commented 5 years ago

I don't think your usage of the slack key in the extra parameter is correct, or more to the point, I don't think you are using the pretext key correctly.

TL;DR: I think you are interpreting Slack's API incorrectly.

In the linked st2 issue, the following extra parameter data is verified as working correctly with Mattermost (which copies Slack's attachments API):

{
  "color": "#474C5D",
  "mattermost": {
    "attachments": [
      {
        "pretext": "---",
        "actions": [
          {
            "integration": {
              "url": "https://example.com",
              "context": {
                "some": "value"
              }
            },
            "name": "button name"
          }
        ],
        "text": "example"
      }
    ]
  }
}

Note the attachments key, and the fact that pretext appears within an object in a list of attachments. That is what I believe Slack is expecting.

This is the JSON that you are sending in the extra parameter:

{
  "slack": {
    "color": "#f48527",
    "pretext": "Hey <!channel>, Ready for ChatOps?",
    "title": "Ansible and ChatOps. Get started :rocket:",
    "title_link": "https://stackstorm.com/2015/06/24/ansible-chatops-get-started-%f0%9f%9a%80/",
    "author_name": "by StackStorm - IFTTT for Ops",
    "author_link": "https://stackstorm.com/",
    "author_icon": "https://stackstorm.com/wp/wp-content/uploads/2015/01/favicon.png",
    "image_url": "https://i.imgur.com/HWN8T78.png",
    "fields": [
      {
        "title": "Documentation",
        "value": "https://docs.stackstorm.com/chatops/",
        "short": true
      }
    ]
  }
}

See how it's missing the attachments key?

Perusing Slack's messy yet useful documentation, I have not found that pretext is a supported key outside of the attachments API. If you have found otherwise, please point me to it.

In fact, using Slack's own message formatter, I have found that the way you are trying to use pretext is not supported:

We found an error in line 2 Here's what went wrong: we don't recognize the property pretext on message

Using that same formatter, I have verified that pretext is supported as a key of an attachment.

So please try using this data instead:

{
  "color": "#474C5D",
  "slack": {
    "attachments": [
      {
        "color": "#f48527",
        "pretext": "Hey <!channel>, Ready for ChatOps?",
        "title": "Ansible and ChatOps. Get started :rocket:",
        "title_link": "https://stackstorm.com/2015/06/24/ansible-chatops-get-started-%f0%9f%9a%80/",
        "author_name": "by StackStorm - IFTTT for Ops",
        "author_link": "https://stackstorm.com/",
        "author_icon": "https://stackstorm.com/wp/wp-content/uploads/2015/01/favicon.png",
        "image_url": "https://i.imgur.com/HWN8T78.png",
        "fields": [
          {
            "title": "Documentation",
            "value": "https://docs.stackstorm.com/chatops/",
            "short": true
          }
        ]
      }
    ]
  }
}
nzlosh commented 5 years ago

@blag Requiring attachments isn't consistent with StackStorm ChatOps documentation https://docs.stackstorm.com/chatops/aliases.html#passing-attachment-api-parameters-slack-only

Slack formats ChatOps output as attachments, and you can configure the API parameters in the result.extra.slack field.

You can see in the example that extra.slack doest not contain the attachments key, but starts with keys color, fields and image directly below slack.

I assume the Mattermost adapter code handles things differently which is why attachments is required?

What is the correct way Slack extra paramters should be handled? I know you've been working on updating hubot components, has there been changes in the slack adapter that mean the st2 documentation is no longer correct?

lingfish commented 5 years ago

Yeah, basically what @nzlosh said. The CLI example I originally pasted is actually from an example somewhere in the st2 repos, but that said, the doco does indicate that using:

    "slack_extra": {
      "slack": {

Uses attachments in the "backend" when sent to Slack.

An example of my trigger instance (which clearly shows up as a card/attachment in Slack):

    "slack_extra": {
      "slack": {
        "title": "xxx",
        "color": "danger",
        "text": "An alert",
        "author_name": "ServiceNow",
        "title_link": "https://xxx",
        "mrkdwn_in": [
          "fields"
        ],
        "fields": [
          {
            "short": true,
            "value": "xxx",
            "title": "CI"
          },
          {
            "short": true,
            "value": "2 - High",
            "title": "Priority"
          },
          {
            "short": true,
            "value": "01-Mar-2019 00:17:29",
            "title": "First notified"
          },
          {
            "short": true,
            "value": "Active",
            "title": "External status"
          },
          {
            "short": false,
            "value": "yyy",
            "title": "Last update"
          }
        ],
        "pretext": "Something",
        "fallback": "xxx",
        "author_icon": "https://zzz"
      }
    }
lingfish commented 5 years ago

Hi @blag, any further thoughts here?

blag commented 5 years ago

@lingfish Can you find where the docs stated to use slack_extra? That's definitely a bug in our documentation.

Regarding how to us attachments with Mattermost, using extra.mattermost.attachments should work:

Can you see what data is being sent to the Mattermost server?

Adding end-to-end tests to ChatOps with Mattermost is on my list of things to do after 3.0 is released.

lingfish commented 5 years ago

Ok, so a few confusing things going on here, including from me ;)

Firstly, I'm using Slack, not Mattermost.

Secondly, the payload being sent is actually this:

{
  "extra": {
    "slack": {

Not what I pasted above; there is no slack_extra -- sorry about that! I must have gotten confused, because I have a rule that looks like this:

---
    name: "notify_slack"
    pack: "xx_st2"
    description: "Send to Slack."
    enabled: true

    trigger:
      type: "xx_st2.evt_trigger"
      parameters: {}
    action:
      ref: "chatops.post_message"
      parameters:
        message: "{{trigger.message}}"
        channel: "testing"
        extra: "{{trigger.slack_extra}}"

My sensor sends that extra data using self._sensor_service.dispatch.

In the doco, it states:

Everything that’s specified in extra.slack will be passed as-is to the Slack Attachment API.

This implies that anything supplied in extra.slack, gets put into an attachments section in the backend, right?

Inserting the above JSON, from extra.slack, directly into Slack's attachment tester, inside the attachments list, renders it perfectly, including the pretext.

{
    "text": "I am a test message http://slack.com",
    "attachments": [
{
        "title": "xxx",
        "color": "danger",
        "text": "An alert",
        "author_name": "ServiceNow",
        "title_link": "https://xxx",
        "mrkdwn_in": [
          "fields"
        ],
        "fields": [
          {
            "short": true,
            "value": "xxx",
            "title": "CI"
          },
          {
            "short": true,
            "value": "2 - High",
            "title": "Priority"
          },
          {
            "short": true,
            "value": "01-Mar-2019 00:17:29",
            "title": "First notified"
          },
          {
            "short": true,
            "value": "Active",
            "title": "External status"
          },
          {
            "short": false,
            "value": "yyy",
            "title": "Last update"
          }
        ],
        "pretext": "Something",
        "fallback": "xxx",
        "author_icon": "https://zzz"
      }
    ]
}
blag commented 5 years ago

Sorry for the confusion regarding Mattermost vs. Slack!

Unfortunately

I think you have found a bug in our documentation. This statement from our documentation:

Everything that’s specified in extra.slack will be passed as-is to the Slack Attachment API.

is not 100% true. The dictionary that is passed to the Slack Attachment API is NOT passed as-is, it is completely mangled by the hubot-stackstorm Slack adapter.

But there is still hope!

There are two paths in the Slack adapter for handling extra.slack, and they handle attachments slightly differently.

Originally, sending custom attachments was not supported in the adapter (see here). I added the ability to specify your own attachments in 14275e9626021d28a1657acf823de5deec3e6742.

However, I needed to preserve backwards compatibility, so there are now two code paths for sending Slack messages with attachments:

  1. if (data.extra && data.extra.slack && data.extra.slack.attachments)
  2. if (data.extra && data.extra.slack)

But the second (original) code path unconditionally mangled the pretext:

      content.pretext = i === 0 ? pretext + split_message.pretext : null;
      content.text = chunks[i];
      content.fallback = chunks[i];
      robot.adapter.client.send(envelope, {'attachments': [content]});

Really, a better description of what it does is it completely ignores any pretext that is set by you, the user.

Furthermore, if you look through the first code path, you'll notice that it is comparatively incredibly simple:

    var messages_to_send = messages.buildMessages(data.extra.slack);

    var sendMessage = function (i) {
      robot.adapter.client.send(envelope, messages_to_send[i]);

      if (messages_to_send.length > ++i) {
        setTimeout(function(){sendMessage(i);}, 300);
      }
    };

    sendMessage(0);

The first code path does not mangle the pretext at all, so it is a "safer" code path for you, since you wish to specify your own pretext.

What this means for you

Please fully specify the attachments you wish to send, so the code takes the first (safer) code path:

---
    name: "notify_slack"
    pack: "xx_st2"
    description: "Send to Slack."
    enabled: true

    trigger:
      type: "xx_st2.evt_trigger"
      parameters: {}
    action:
      ref: "chatops.post_message"
      parameters:
        message: "{{trigger.message}}"
        channel: "testing"
        extra:
          slack:
            attachments: "{{ trigger.slack_extra_attachments }}"  # Should be an array, see the Slack Attachments API

The value of the extra.slack.attachments key is sent directly to Slack unchanged (except for some intelligent chunking if the message would not fit Slack's message size limit).

I cannot really properly "fix" this bug, because doing so would create issues for other users who do expect pretext to be set/mangled by the Slack adapter, but the above is a way for you to workaround the issue.

Other considerations

Additional pretext mangling elsewhere in the code

There is some additional pretext mangling earlier in the adapter, before either code path:

  if (data.whisper && data.user) {
    recipient = data.user;
    envelope = {
      "user": data.user
    };
  } else {
    recipient = data.channel;
    pretext = (data.user && !data.whisper) ? util.format('@%s: ', data.user) : "";
    envelope = {
      "room": data.channel,
      "id": data.channel,
      "user": data.user,
    };
  }

but that pretext value is not used by the first code path, so the workaround I specified will still workaround this.

Slack message size limits

Ensure that your total message size does not exceed Slack's message size limit (which, IIRC, is 4000 characters in the JSON). If the Slack adapter thinks your message is too large, it will apply some (hopefully) intelligent chunking to break up your message while preserving the intended formatting as much as possible.

Documentation fix

Tomorrow I will put together a pull request updating the documentation so it doesn't mislead users such as yourself. Thank you for pointing out that inaccuracy. :)

lingfish commented 5 years ago

Love it, thanks @blag! That was the most epic and awesome dev issue comment I've read :)

Pity about the constraints though. I'll try your suggestion soon.

Meanwhile, Slack are at it again, improving etc. Now they have a thing called blocks. Perhaps if/when this project moves to it, that non-breaking change can be implemented.

lingfish commented 5 years ago

Hi again @blag ... So, I've tried it as suggested and hit a bit of a wall. It seems that no jinja parsing is done, if done as above?

  "extra": {
    "slack": {
      "attachments": "[{u'title': u'something', u'color': u'#a0a0a0', u'text': u'something', ... ]"
    }
  }

It seems to have just pprint'd the Python representation of the list of dicts?

I've tried variants using to_json_string and yaml, no difference.

Thoughts?

lingfish commented 5 years ago

Pinging @blag 😉

blag commented 5 years ago

@lingfish Can you post your entire alias (feel free to redact/change any sensitive information)? It's difficult to troubleshoot when I don't know what you've written.

lingfish commented 5 years ago

Do you mean my rule? There is no alias. If rule, here it is:

---
    name: "notify_slack"
    pack: "xxx_st2"
    description: "Send to Slack."
    enabled: true

    trigger:
      type: "xxx_st2.evt_trigger"
      parameters: {}

    action:
      ref: "chatops.post_message"
      parameters:
        message: "{{trigger.message}}"
        channel: "testing"
        extra:
          slack:
            attachments: "{{ trigger.slack_extra }}"
blag commented 5 years ago

I'm having a hard time reconstructing what you are doing. Can you post a snapshot of everything you have right now?

Attempting to reconstruct what's going on...

"extra": {
  "slack": {
    "attachments": "[{u'title': u'something', u'color': u'#a0a0a0', u'text': u'something', ... ]"
  }
}

^ This appears to be the data that is being passed into chatops.post_message. Is that correct?

So if this is your rule YAML (copied/pasted from above):

---
    name: "notify_slack"
    pack: "xxx_st2"
    description: "Send to Slack."
    enabled: true

    trigger:
      type: "xxx_st2.evt_trigger"
      parameters: {}

    action:
      ref: "chatops.post_message"
      parameters:
        message: "{{trigger.message}}"
        channel: "testing"
        extra:
          slack:
            attachments: "{{ trigger.slack_extra }}"

It would stand to reason that your trigger data is:

"slack_extra": [
  {
    "title": "something",
    "color": "#a0a0a0",
    "text": "something",
    ...
  }
]

Which doesn't look right.

I would double check what your trigger instance data is. Make sure it is an object/dictionary and not a string. Once you've verified it, please post it here verbatim so I can follow along.

Once you have verified the trigger instance data is what you expect it to be, you can also try to use the from_json_string Jinja filter in your rule:

---
name: "notify_slack"
pack: "xxx_st2"
description: "Send to Slack."
enabled: true

trigger:
  type: "xxx_st2.evt_trigger"
  parameters: {}

action:
  ref: "chatops.post_message"
  parameters:
    message: "{{trigger.message}}"
    channel: "testing"
    extra:
      slack:
        attachments: "{{ trigger.slack_extra | from_json_string }}"
lingfish commented 5 years ago

Sorry, you're right, more data needed from me.

Ok, so... the rule above is right.

Heavily redacted below, but done carefully to not lose quotes etc.

Yes, that's the data that is being passed into chatops.post_message. Here is the action output:

{
  "extra": {
    "slack": {
      "attachments": "[{u'title': u'zzz', u'color': u'danger', u'text': u'NOTIFICATION', u'author_name': u'zzz', u'title_link': u'zzz', u'mrkdwn_in': [u'fields'], u'fields': [{u'short': True, u'value': u'zzz', u'title': u'zzz'}, {u'short': True, u'value': u'zzz', u'title': u'zzz'}, {u'short': True, u'value': u'zzz', u'title': u'zzz'}, {u'short': True, u'value': u'Active', u'title': u'zzz'}, {u'short': True, u'value': u'zzz', u'title': u'zzz'}, {u'short': True, u'value': u'1', u'title': u'zzz'}, {u'short': False, u'value': u'zzz', u'title': u'zzz'}], u'pretext': u'', u'fallback': u'zzz', u'author_icon': u'zzz'}]"
    }
  },
  "whisper": false,
  "user": null,
  "output": {
    "whisper": false,
    "message": "NOTIFICATION",
    "user": null,
    "channel": "testing",
    "extra": {
      "slack": {
        "attachments": "[{u'title': u'zzz', u'color': u'danger', u'text': u'NOTIFICATION', u'author_name': u'zzz', u'title_link': u'zzz', u'mrkdwn_in': [u'fields'], u'fields': [{u'short': True, u'value': u'zzz', u'title': u'zzz'}, {u'short': True, u'value': u'zzz', u'title': u'zzz'}, {u'short': True, u'value': u'zzz', u'title': u'zzz'}, {u'short': True, u'value': u'zzz', u'title': u'zzz'}, {u'short': True, u'value': u'zzz', u'title': u'zzz'}, {u'short': True, u'value': u'1', u'title': u'zzz'}, {u'short': False, u'value': u'zzz', u'title': u'zzz'}], u'pretext': u'', u'fallback': u'zzz', u'author_icon': u'zzz'}]"
      }
    }
  },
  "message": "NOTIFICATION",
  "channel": "testing"
}

Here is the trigger instance:

{
  "status": "processed",
  "occurrence_time": "2019-05-09T13:16:03.000000Z",
  "trigger": "pack.trigger",
  "id": "5cd39b73e8a12c310f1e5ddc",
  "payload": {
    "param": "zzz",
    "param": false,
    "param": 1,
    "param": "zzz",
    "param": "zzz",
    "param": "zzz",
    "param": "zzz",
    "param": "zzz",
    "slack_extra": [
      {
        "title": "zzz",
        "color": "danger",
        "text": "NOTIFICATION",
        "author_name": "zzz",
        "title_link": "zzz",
        "mrkdwn_in": [
          "fields"
        ],
        "fields": [
          {
            "short": true,
            "value": "zzz",
            "title": "zzz"
          },
          {
            "short": true,
            "value": "zzz",
            "title": "zzz"
          },
          {
            "short": true,
            "value": "zzz",
            "title": "zzz"
          },
          {
            "short": true,
            "value": "zzz",
            "title": "zzz"
          },
          {
            "short": true,
            "value": "zzz",
            "title": "zzz"
          },
          {
            "short": true,
            "value": "1",
            "title": "zzz"
          },
          {
            "short": false,
            "value": "zzz",
            "title": "zzz"
          }
        ],
        "pretext": "",
        "fallback": "zzz",
        "author_icon": "zzz"
      }
    ],
    "short_description": "zzz",
    "message": "zzz",
    "cmdb_ci": {
      "link": "zzz",
      "display_value": "zzz"
    }
  }
}

So the trigger instance data looks like an object to me... but as stated above, the attachments field just looks like the data as if Python pretty-printed.

The snippet of my Python code for the sensor:

        payload['slack_extra'] = [
            {   
                'fallback': 'zzz {}: {}'.format(record['zzz'], record['yyy']),
                'author_name': 'zzz',
                'author_icon': 'zzz',
                'title': record['zzz'],
      ...

Finally, I had already tried from_json_string, but did so again, and I get this:

{
  "traceback": "  File \"/opt/stackstorm/st2/local/lib/python2.7/site-packages/st2reactor/rules/enforcer.py\", line 200, in _invoke_action\n    additional_contexts=additional_contexts)\n  File \"/opt/stackstorm/st2/local/lib/python2.7/site-packages/st2reactor/rules/enforcer.py\", line 82, in get_resolved_parameters\n    additional_contexts=additional_contexts)\n  File \"/opt/stackstorm/st2/local/lib/python2.7/site-packages/st2common/util/param.py\", line 297, in render_live_params\n    context = _resolve_dependencies(G)\n  File \"/opt/stackstorm/st2/local/lib/python2.7/site-packages/st2common/util/param.py\", line 215, in _resolve_dependencies\n    raise ParamException(msg)\n",
  "error": "Failed to render parameter \"extra\": Expecting property name enclosed in double quotes: line 1 column 3 (char 2)"
}
blag commented 5 years ago

Can you post the YAML metadata file for your sensor?

lingfish commented 5 years ago

Sure, for this though you'll need non-redacted fields, right?

If so, can I mail it or similar, commercially-sensitive stuff in there.

blag commented 5 years ago

You are welcome to either redact field values (use your best judgment on that) and post it here or you can DM me via our Slack community (I'm also blag on that).

lingfish commented 5 years ago

Alright, so upon trying to modify the sensor schema, I get an error for object, and noticing it's technically an array, tried that too.

Failed to validate payload [...] is not of type u'object', 'null'

When specifying array, it just goes through again as a string :(

lingfish commented 5 years ago

Also just for fun, from the sensor I've send an object {} instead of an array of objects, changed the schema accordingly, and it passes, but still just a flat string.

blag commented 5 years ago

Okay, so since the data being passed to chatops.post_message is a string, it appears that that is not the cause of the issue.

The issue is farther back in the trigger chain - in the rule itself. Because of that, this bug is in the st2 project itself and the original issue StackStorm/st2#4561 is the proper place to handle this.

@lingfish Is that acceptable to you? Sorry to push you back over to that issue, but it looks like there's nothing to do/fix in this project.

I'll keep this open for a week or until you respond, whichever comes first. Then I'll close it and post the information we've figured out here back into the original st2 issue.