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

TLS configuration for Slack service for Mattermost compatibility #1322

Merged
merged 1 commit into from Apr 19, 2017

Conversation

Projects
None yet
2 participants
@seuf
Contributor

seuf commented Apr 13, 2017

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
  • Rebased/mergable
  • Tests pass
  • CHANGELOG.md updated
  • Sign CLA (if not already signed)
@nathanielc

Thanks for this PR! It looks great. I have made a few comments suggestions below.

Show outdated Hide outdated server/server.go
srv := slack.NewService(c, l)
srv, err := slack.NewService(c, l)
if err != nil {
s.Logger.Println("W! Slack init :", err)

This comment has been minimized.

@nathanielc

nathanielc Apr 13, 2017

Contributor

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.

@nathanielc

nathanielc Apr 13, 2017

Contributor

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
}

This comment has been minimized.

@nathanielc

nathanielc Apr 13, 2017

Contributor

Can you add a log warn here if the config specifies using InsecureSkipVerify?

@nathanielc

nathanielc Apr 13, 2017

Contributor

Can you add a log warn here if the config specifies using InsecureSkipVerify?

Show outdated Hide outdated services/slack/service.go
s := &Service{
logger: l,
client: &http.Client{Transport: &http.Transport{TLSClientConfig: tlsConfig}},

This comment has been minimized.

@nathanielc

nathanielc Apr 13, 2017

Contributor

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.

@nathanielc

nathanielc Apr 13, 2017

Contributor

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.

@seuf

This comment has been minimized.

Show comment
Hide comment
@seuf

seuf Apr 14, 2017

Contributor

I've updated the pr to add all your requested changes. Is it good to you ?

Contributor

seuf commented Apr 14, 2017

I've updated the pr to add all your requested changes. Is it good to you ?

Show outdated Hide outdated services/slack/service.go
@@ -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)

This comment has been minimized.

@nathanielc

nathanielc Apr 14, 2017

Contributor

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

@nathanielc

nathanielc Apr 14, 2017

Contributor

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

This comment has been minimized.

@seuf

seuf Apr 14, 2017

Contributor

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.

@seuf

seuf Apr 14, 2017

Contributor

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.

This comment has been minimized.

@nathanielc

nathanielc Apr 14, 2017

Contributor

@seuf No worries, I can help you out too. The Alerta service does something similar if that helps.

@nathanielc

nathanielc Apr 14, 2017

Contributor

@seuf No worries, I can help you out too. The Alerta service does something similar if that helps.

This comment has been minimized.

@seuf

seuf Apr 18, 2017

Contributor

OK, I have updated the PR to use atomic value for the http client like Alerta service.

@seuf

seuf Apr 18, 2017

Contributor

OK, I have updated the PR to use atomic value for the http client like Alerta service.

Show outdated Hide outdated services/slack/service.go
@@ -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)

This comment has been minimized.

@nathanielc

nathanielc Apr 18, 2017

Contributor

Can you remove this debug statement?

@nathanielc

nathanielc Apr 18, 2017

Contributor

Can you remove this debug statement?

@nathanielc

nathanielc approved these changes Apr 18, 2017 edited

LGTM, once the extra log statement is removed.

Before I merge can you rebase/squash and update the CHANGELOG?

Thanks!

@seuf

This comment has been minimized.

Show comment
Hide comment
@seuf

seuf Apr 19, 2017

Contributor

done

Contributor

seuf commented Apr 19, 2017

done

@nathanielc nathanielc merged commit be34a10 into influxdata:master Apr 19, 2017

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

nathanielc added a commit that referenced this pull request Apr 19, 2017

Merged pull request #1322 from seuf/slack-tls-options
TLS configuration for Slack service for Mattermost compatibility
@nathanielc

This comment has been minimized.

Show comment
Hide comment
@nathanielc

nathanielc Apr 19, 2017

Contributor

@seuf Thanks! 🎉

Contributor

nathanielc commented Apr 19, 2017

@seuf Thanks! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment