search

grafana / alerting

Set of libraries used to build alerting systems at Grafana - including the Alertmanager.
GNU Affero General Public License v3.0
48 stars 42 forks source link

Slack mentions should check length of the rendered template instead of the template source #129

Open chris-barbour-as opened 1 year ago

chris-barbour-as commented 1 year ago

The slack user and group mentions should check the length of the rendered go template when deciding whether or not to add a mention to the slack pretext. Currently, the logic checks the length of the source string for it's conditional logic, and renders the template when interpolating it into the slack pretext.

This is a problem, because if the go template renders into an empty string, a broken mention will be added to the pretext.

To reproduce, use {{- /* empty */ -}} as the "Mention Groups" configuration setting.

https://github.com/grafana/alerting/blob/939f55779e5ff663b9f557a34e73191625ac1d85/receivers/slack/slack.go#L368-L380

DGuhr commented 8 months ago

Hey @armandgrillet - For reasons, I checked the codebase a bit, and found this bug. If it is still valid, I have a fix here (I think, if I understood everything correctly): https://github.com/grafana/alerting/compare/main...DGuhr:alerting:fix_slack_broken_mentions - see tests for details. Happy to provide a PR if you want.

Alternatively, I think, the check could be added to the config_util.go method for comma separated strings, but this is also used elsewhere and I wanted to avoid impacting other behaviour.

edit: on second glance, I saw that splitCommaSeparatedString in config_util does exactly what I did here, so I guess that the bug doesn't exist anymore or I am missing some context currently. sry for the noise so-far.