Conversation
c9c52d3 to
c32a184
Compare
While it's possible to configure Slack app bot tokens using a combination of http_configs authorization credentials and setting the Slack API URL to the a specific endpoint, doing so at a global level leaks the token to any other notification receiver configured, as http_configs are not specific to notifiers. Slack has also restricted webhook URLs to only be able to post to a single channel (with the legacy webhooks being [marked as deprecated and not recommended][1]), which reduces their usefulness when set at the global level. This PR adds a way of easily setting Slack App bot tokens at the global level, as well as overriding at the individual receiver level, while keeping compatibility with existing configurations. The decision to have a separate config field for the URL was to be able to provide a default API URL for Slack apps as well as differentiate when a webhook url is provided. Ideally we'd change the `slack_api_url` to be `slack_webhook_url` so as to avoid confusion, but that would be an unnecessary breaking change. More context in issue prometheus#2513 [1]: https://api.slack.com/legacy/custom-integrations/messaging/webhooks Signed-off-by: Pedro Araujo <pedro.araujo@teya.com>
c32a184 to
d6d3160
Compare
|
So we do also need BlockKit support, and I think this feature, along with BlockKit, would be perfect for a Slack V2 integration, just like we did for Microsoft Teams. |
I'm a bit confused. Is this feedback for me, or for other reviewers? Are you saying I should implement this under a new slack integration, or close this PR since someone else is implementing it together with Block Kit? To be clear, I'm able to change the PR to kickstart the new slack integration and plug internals into the current integration, but I don't want to extend the scope of this PR to add such a large piece of work such as Block Kit. What's the recommendation here? |
The [Slack app support issue][1] suggested setting the slack API URL to the `chat.PostMessage` endpoint instead of a webhook URL, and so people migrating from this workaround to the the new configuration might encounter a situation where they want to set the `slack_app_token` at the global level while still retaining the `slack_api_url` while dynamically configured receivers (such as those set by prometheus operator) are migrated. Signed-off-by: Pedro Araujo <pedro.araujo@teya.com> [1]: prometheus#2513
Signed-off-by: Pedro Araujo <pedro.araujo@teya.com>
Spaceman1701
left a comment
There was a problem hiding this comment.
This seems like a great change to make to bridge us to a V2 slack integration in the future. Since webhook URLs don't really work anymore, I think something like this is necessary for the existing slack integration to remain useful.
| if sc.HTTPConfig == nil { | ||
| sc.HTTPConfig = c.Global.HTTPConfig | ||
| if sc.AppURL == nil { | ||
| if c.Global.SlackAppURL == nil { |
There was a problem hiding this comment.
Does this change cause existing configs to error unless they set the new app* fields?
There was a problem hiding this comment.
Thanks for looking into this! No, it shouldn't, because we're setting the default SlackAppURL in line 781, so it just works out-of-the-box.
Spaceman1701
left a comment
There was a problem hiding this comment.
Thanks for your patience!
I Took some time to go through this a little more carefully. Overall, this seems good to me. It looks like there shouldn't be any behavior changes for anybody who's already using the slack integration (because they would've needed to set slack_api_url already), which is my biggest concern.
My second concern is whether the slack_app_url is necessary - is there a reason to create the new field rather than just set slack_api_url's default to https://slack.com/api/chat.postMessage? I don't think this would be a breaking change because existing users have already set this to something.
My final (and least important) concern is whether this actually works with slack. It looks like it does, but we just don't have any automated testing for that as far as I can tell. That being said, this is counter-intuitively the easiest part to fix if there's a problem 😆 .
Thank you for reviewing!
Yeah, the way it's configured, the slack app token logic is opt in, and everything keeps working for existing users that use the legacy webhook url. The logic that's a bit more complex is to guard against breaking users that set the slack app token through http_configs, as suggested here—given the amount of reactions for that comment, I think it might be fairly widespread.
I think I addressed this in my original PR description; the decision to have a separate config field for the Slack app URL was to be able to provide a default API URL for Slack apps as well as differentiate when a webhook url is provided. I originally tried reusing the field, but it became very hairy to know whether the URL was for use with app token or for legacy URL, and I thought the added complexity wasn't worth it.
Well, I can say for certain that it works with Slack Apps, because we've been running an alertmanager fork with this patch for the last ~9 months with no issues 😄 |
Spaceman1701
left a comment
There was a problem hiding this comment.
I think I addressed this in my original PR description; the decision to have a separate config field for the Slack app URL was to be able to provide a default API URL for Slack apps as well as differentiate when a webhook url is provided. I originally tried reusing the field, but it became very hairy to know whether the URL was for use with app token or for legacy URL, and I thought the added complexity wasn't worth it.
Ah, yeah, I guess it becomes unclear what the behavior should be when there's a global http_config too. And there's a more clear distinction for users moving to the new mechanism. At some point we'll have to deprecate the old slack_api_url.
Given that slack receivers are extraordinarily ergonomic today, I think this is definitely worth merging.
cc @grobinson-grafana and @SuperQ for a sanity check on the config changes.
|
A couple of linter errors to fix. |
Fixed by running `make common-lint-fix` Signed-off-by: Pedro Araujo <pedro.araujo@teya.com>
|
@SuperQ the linter errors should be fixed, please check again when you can 😃 |
|
@pharaujo I wanted to thank you for this PR to improve the Slack integration. We are deploying AlertManager as part of the kube-prometheus-stack helm chart. I'm trying to use the kube-prometheus-helm chart v80.4.2, which includes AlertManager v0.30.0 I'm not sure where PrometheusOperator does the config validation, or if it's related to Prometheus CRDs |
|
That is most likely an issue in prometheus-operator. To me it looks like they have there own copy of the config types |
|
Yes, prometheus-operator imports alertmanager, so the next step is to contribute the slack app support to it. Since we use kube-prometheus internally as well, I've already done this here, and we've been running this fork since the beginning of the year. Now that this feature is released, I'll send a PR to prometheus-operator with it (and then, hopefully, to kube-prometheus if/when it's accepted 🙂) EDIT: I don't recommend other people running my fork though, it's still using my alertmanager fork; I haven't updated it yet to use the newly-released 0.30.0. |
Feature released in alertmanager [v0.30.0](https://github.com/prometheus/alertmanager/releases/tag/v0.30.0), implemented in prometheus/alertmanager#4211 Signed-off-by: Pedro Araujo <pedro.araujo@teya.com>
Slack app support released in alertmanager [v0.30.0](https://github.com/prometheus/alertmanager/releases/tag/v0.30.0), (implemented in prometheus/alertmanager#4211), missing support in prometheus-operator. Signed-off-by: Pedro Araujo <pedro.araujo@teya.com>
Slack app support released in alertmanager [v0.30.0](https://github.com/prometheus/alertmanager/releases/tag/v0.30.0), (implemented in prometheus/alertmanager#4211), missing support in prometheus-operator. Signed-off-by: Pedro Araujo <pedro.araujo@teya.com>
Slack app support released in alertmanager [v0.30.0](https://github.com/prometheus/alertmanager/releases/tag/v0.30.0), (implemented in prometheus/alertmanager#4211), missing support in prometheus-operator. Signed-off-by: Pedro Araujo <pedro.araujo@teya.com>
Slack app support released in alertmanager [v0.30.0](https://github.com/prometheus/alertmanager/releases/tag/v0.30.0), (implemented in prometheus/alertmanager#4211). Supporting the new fields in the Alertmanager config was done in prometheus-operator#8238, and now this one adds support for them in the operator CRs. Additionally added SlackConfig tests I had around. Signed-off-by: Pedro Araujo <pedro.araujo@teya.com>
Slack app support released in alertmanager [v0.30.0](https://github.com/prometheus/alertmanager/releases/tag/v0.30.0), (implemented in prometheus/alertmanager#4211). Supporting the new fields in the Alertmanager config was done in prometheus-operator#8238, and now this one adds support for them in the operator CRs. Additionally added SlackConfig tests I had around. Signed-off-by: Pedro Araujo <pedro.araujo@teya.com>
Slack app support released in alertmanager [v0.30.0](https://github.com/prometheus/alertmanager/releases/tag/v0.30.0), (implemented in prometheus/alertmanager#4211). Supporting the new fields in the Alertmanager config was done in prometheus-operator#8238, and now this one adds support for them in the operator CRs. Additionally added SlackConfig tests I had around. Signed-off-by: Pedro Araujo <pedro.araujo@teya.com>
Slack app support released in alertmanager [v0.30.0](https://github.com/prometheus/alertmanager/releases/tag/v0.30.0), (implemented in prometheus/alertmanager#4211). Supporting the new fields in the Alertmanager config was done in prometheus-operator#8238, and now this one adds support for them in the operator CRs. Additionally added SlackConfig tests I had around. Signed-off-by: Pedro Araujo <pedro.araujo@teya.com>
Slack app support released in alertmanager [v0.30.0](https://github.com/prometheus/alertmanager/releases/tag/v0.30.0), (implemented in prometheus/alertmanager#4211). Supporting the new fields in the Alertmanager config was done in prometheus-operator#8238, and now this one adds support for them in the operator CRs. Additionally added SlackConfig tests I had around. Signed-off-by: Pedro Araujo <pedro.araujo@teya.com>
While it's possible to configure Slack app bot tokens using a combination of http_configs authorization credentials and setting the Slack API URL to the a specific endpoint, doing so at a global level leaks the token to any other notification receiver configured, as http_configs are not specific to notifiers. Slack has also restricted webhook URLs to only be able to post to a single channel (with the legacy webhooks being marked as deprecated and not recommended), which reduces their usefulness when set at the global level.
This PR adds a way of easily setting Slack App bot tokens at the global level, as well as overriding at the individual receiver level, while keeping compatibility with existing configurations.
The decision to have a separate config field for the URL was to be able to provide a default API URL for Slack apps as well as differentiate when a webhook url is provided. Ideally we'd change the
slack_api_urlto beslack_webhook_urlso as to avoid confusion, but that would be an unnecessary breaking change.More context in issue #2513