TLS configuration for Slack service for Mattermost compatibility #1322
Conversation
seuf
force-pushed
the
slack-tls-options
branch
5 times, most recently
from
da4a899
to
8eb56fc
Compare
server/server.go
Outdated
| srv := slack.NewService(c, l) | ||
| srv, err := slack.NewService(c, l) | ||
| if err != nil { | ||
| s.Logger.Println("W! Slack init :", err) |
There was a problem hiding this comment.
Go ahead and return the error here and check it in NewServer and return it from there. That will cause the server to print the error message and shutdown, which is the behavior we want.
| tlsConfig, err := getTLSConfig(c.SSLCA, c.SSLCert, c.SSLKey, c.InsecureSkipVerify) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Can you add a log warn here if the config specifies using InsecureSkipVerify?
services/slack/service.go
Outdated
| s := &Service{ | ||
| logger: l, | ||
| client: &http.Client{Transport: &http.Transport{TLSClientConfig: tlsConfig}}, |
There was a problem hiding this comment.
The configuration can be changed at runtime via the API, there should be an Update method on the service which is in charge of applying new config changes. Can you update that method to allow for creating a new client when the TLS config changes. See the Alerta service for an example.
|
I've updated the pr to add all your requested changes. Is it good to you ? |
services/slack/service.go
Outdated
| @@ -99,7 +119,8 @@ func (s *Service) Alert(channel, message, username, iconEmoji string, level aler | |||
| if err != nil { | |||
| return err | |||
| } | |||
| resp, err := http.Post(url, "application/json", post) | |||
| s.logger.Println("D! Posting event to Slack", post) | |||
| resp, err := s.client.Post(url, "application/json", post) | |||
There was a problem hiding this comment.
This line races with the lines in Update and New that assign to the client. Either add a mutex to the Sensu service or use the sync.Atomic type to synchronizes access to the client.
Other than that this looks good! Thanks
There was a problem hiding this comment.
Hum. I'm not a Go dev. This looks like a bit tricky for me.. O_ô
I will try to do this next tuesday when i go back to work.
There was a problem hiding this comment.
@seuf No worries, I can help you out too. The Alerta service does something similar if that helps.
There was a problem hiding this comment.
OK, I have updated the PR to use atomic value for the http client like Alerta service.
services/slack/service.go
Outdated
| @@ -99,7 +127,9 @@ func (s *Service) Alert(channel, message, username, iconEmoji string, level aler | |||
| if err != nil { | |||
| return err | |||
| } | |||
| resp, err := http.Post(url, "application/json", post) | |||
| s.logger.Println("D! Posting event to Slack", post) | |||
There was a problem hiding this comment.
Can you remove this debug statement?
…o self hosted Mattermost
|
done |
TLS configuration for Slack service for Mattermost compatibility
|
@seuf Thanks! 🎉 |
Hi,
Mattermost is compatible with slack messages json format so I've patched the slack services to add TLS configuration parameters.
Now alerts can be sent to self hosted Mattermost with custom certificates.
Required for all non-trivial PRs