Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add format string for Slack outputs #40

Merged
merged 2 commits into from
Oct 7, 2019

Conversation

actgardner
Copy link
Contributor

@actgardner actgardner commented Oct 4, 2019

Add a new configuration option for the Slack output, which allows users to customize the text of the Slack message in addition to the existing attachments. The current all and text options are verbose and make it hard to surface specific valuable information. Using Go templates also gives users some flexibility to do some basic string munging.

@Issif Issif added the kind/feature New feature or request label Oct 4, 2019
@Issif
Copy link
Member

Issif commented Oct 4, 2019

This is a really interesting idea, that could be reproduced for other outputs (Teams, SMTP) even all.

For Slack, I choosed to avoid theText field to get all informations beside the colored vertical line, it looks nicer. What do you think to use your MessageFormat template to replace theOutput used for Attachement's Text?

We must also provide more intels about available fields (.Priority, .Rule, etc) in README, some of them are always present but not all.

For example in :

messageformat: "Alert for rule '{{ .Rule }}' from process {{ index .OutputFields \"proc.name\" }}"

proc.name may not be there in all rules' outputs, how do we manage that? I didn't checked what happened if a field used by template is missing (panic?).

If Attachement's Text is overrided, we need a fallback to origin .Output, we can't loose messages.

You edited README.md but forget to do same in config_example.yml.

@actgardner
Copy link
Contributor Author

For Slack, I choosed to avoid theText field to get all informations beside the colored vertical line, it looks nicer. What do you think to use your MessageFormat template to replace theOutput used for Attachement's Text?

I opted to put this in the message Text so it would be easy to pick out - the hope is that surfacing a subset of information makes alerts clearer for the people triaging them? I think putting it in the attachment Text could also work but it loses some differentiation from the body of the message - in a busy channel it gets hard to pick out the individual events. Plus this way it didn't conflict with any existing features, in case people want this and the output value. But I'm open to changing it if you have strong feelings about it.

proc.name may not be there in all rules' outputs, how do we manage that? I didn't checked what happened if a field used by template is missing (panic?).

There's three failure modes:

  • you write an invalid template, that can't be parsed. This logs an error on startup and sends messages with an empty Text field.
  • you write a valid template, but it references a top-level field of the FalcoPayload struct that doesn't exist. This would fall to Execute, we'd log an error and the message would be sent with an empty Text field.
  • you write a valid template that references a map value in .OutputFields that doesn't always exist. This would evaluate to <no value> in the template, and the Text field would still be set ( see https://play.golang.org/p/7mztgw6IWs0).

We must also provide more intels about available fields (.Priority, .Rule, etc) in README, some of them are always present but not all.

You edited README.md but forget to do same in config_example.yml.

I'll write some more explicit documentation and clean this up.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@Issif
Copy link
Member

Issif commented Oct 4, 2019

You convinced me. Ok for your idea.

I proposed some suggestions while reviewing.

I'll test your branch next Monday and merge for 2.9.1 if ok.

@actgardner
Copy link
Contributor Author

Sounds good, thanks for the quick review!

README.md Outdated Show resolved Hide resolved
config_example.yaml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Co-Authored-By: Thomas Labarussias <issif+github@gadz.org>
@Issif Issif merged commit 7360ba4 into falcosecurity:master Oct 7, 2019
@Issif
Copy link
Member

Issif commented Oct 7, 2019

I released 2.9.1 with your PR. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants