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 support for Slack app #245

Merged
merged 3 commits into from
Oct 27, 2021
Merged

Add support for Slack app #245

merged 3 commits into from
Oct 27, 2021

Conversation

dmitriishaburov
Copy link
Contributor

Closes #244

Add token support for Slack notification provider, combined with configuring webhook url to https://slack.com/api/chat.postMessage will allow to send notifications as Slack App.

Do I need to write documentation for Slack notification provider?
Currently project doesn't have any, so it won't be clear that Slack apps are supported.

@dmitriishaburov
Copy link
Contributor Author

@stefanprodan @phillebaba I'm sorry to ping you guys, but is this PR blocked by something, or is there something I can do?

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@stefanprodan
Copy link
Member

@dmitriishaburov have you tested this? How did you generated the token? can the same token be used to post to different channels?

@dmitriishaburov
Copy link
Contributor Author

dmitriishaburov commented Sep 30, 2021

@stefanprodan thanks, will update docs soon.

have you tested this?

Only by building docker image and running it in our cluster, checking that both webhook and app methods working properly.

How did you generated the token?

  • Go to https://api.slack.com/
  • Create an app -> From scratch
  • Add chat:write token scope
  • Install app into your workspace
  • You'll get Bot User OAuth Token
  • Invite app to your channel to allow it to post.

can the same token be used to post to different channels?

Yes, that's the main reason behind PR - you can use same set of credentials to post into different channels.

One main issue - Slack is ignoring the username field right now, so all messages posted from app username, losing the source-controller, kustomize-controller, etc username.
I will check if it's possible to avoid.

Edit: here's an example from Grafana docs, which supports Slack app https://grafana.com/docs/grafana/latest/alerting/old-alerting/notifications/#slack

@stefanprodan
Copy link
Member

@dmitriishaburov thanks for answering my questions. I think the token generation steps and the username omission should be added to the docs along with an example.

@rjhenry
Copy link

rjhenry commented Oct 20, 2021

+1 for this - anything I can do to help, let me know.

@dmitriishaburov
Copy link
Contributor Author

Sorry for being lost for a while.

  • Rebased onto latest main
  • Updated provider.md docs
  • Removed unused token field from SlackPayload

Did some research on username omission with Slack app. Although it is possible to grant apps rights to change username, that brings:

  • Extra step to grant that rights
  • Security concerns of app being able to impersonate every user and change avatars

Probably it's better to avoid this functionality.

Also there is minor (or not?) issue.
If you change the slack secret from webhook to slack app, notification-controller is not able to send message to Slack until restart. Don't know how to fix that, probably issue somewhere in reload functionality, or known issue (?)

Signed-off-by: Dmitrii Shaburov <dmitrii.shaburov@bolt.eu>
Signed-off-by: Dmitrii Shaburov <dmitrii.shaburov@bolt.eu>
Signed-off-by: Dmitrii Shaburov <dmitrii.shaburov@bolt.eu>
@stefanprodan
Copy link
Member

If you change the slack secret from webhook to slack app, notification-controller is not able to send message to Slack until restart.

We read the secret from Kubernetes API each time https://github.com/fluxcd/notification-controller/blob/main/internal/server/event_handlers.go#L143 but the client has an internal cache. Please open an issue if the failure persists.

Copy link
Member

@stefanprodan stefanprodan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Thanks @dmitriishaburov 🏅

@stefanprodan stefanprodan added area/alerting Alerting related issues and PRs enhancement New feature or request labels Oct 27, 2021
@stefanprodan stefanprodan merged commit 4e96e0d into fluxcd:main Oct 27, 2021
@dmitriishaburov dmitriishaburov deleted the slack-app-support branch October 27, 2021 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/alerting Alerting related issues and PRs enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Slack App notifications
3 participants