Move alerta api token to header and add option to skip TLS verification. #1053

allen13 · 2016-11-17T20:30:51Z

Required for all non-trivial PRs

Rebased/mergable
Tests pass
CHANGELOG.md updated
Sign CLA (if not already signed)

Resolves issue #1052

I have tested this change live with the custom kapacitor docker image allen13/kapacitor:latest

nathanielc

Overall this is good. Just a few things have been commented below.

In addition the tests in integrations/streamer_test.go will need to be updated to check for the token in the header instead of the query parameters.

Thanks!

nathanielc · 2016-11-17T20:34:35Z

services/alerta/config.go

@@ -11,6 +11,8 @@ type Config struct {
 	Enabled bool `toml:"enabled" override:"enabled"`
 	// The Alerta URL.
 	URL string `toml:"url" override:"url"`
+	// Whether to skip the tls verification of the alerta host
+	TLSSkipVerify bool `toml:"tls_skip_verify" override:"tls_skip_verify"`


Can you rename this option to InsecureSkipVerify and insecure-skip-verify so that it is consistent with other similar config options?

Yup no problem.

nathanielc · 2016-11-17T20:38:30Z

services/alerta/service.go

 	if err != nil {
 		return err
 	}

-	resp, err := http.Post(url, "application/json", post)
+	client := &http.Client{


Can you create the client in the NewService method so that we don't have to create a new client for each new request?

Since the Alerta config can change dynamically you will also need to create a new client in the Update method if the config changed. And that also means that you will need to synchronize access to the client. For that I would recommend using atomic.Value, see how the config field is access for an example.

Let me know if I can clarify any of that better.

Yeah, good catch. I'm pretty sure I understand how the update method works. Let me fix that up.

allen13 · 2016-11-17T21:38:41Z

Updated to address above issues.

nathanielc · 2016-11-18T17:22:00Z

Here is the failing test from server/server_test.go

 TestServer_UpdateConfig (0.25s)
    server_test.go:6230: unexpected defaults for alerta/: alerta: section element 0 are not equal: extra option "insecure-skip-verify" with value <nil>
    server_test.go:6245: unexpected update result 0 for alerta/: alerta: section element 0 are not equal: extra option "insecure-skip-verify" with value <nil>

allen13 · 2016-11-18T19:51:01Z

Tests passes this time!

nathanielc

Changes look good. One last thing is the change to reuse the same client creates a race condition. I have commented in-line below on how to fix it. Should be a simple change.

Thanks again!

nathanielc · 2016-11-19T00:00:28Z

services/alerta/service.go

@@ -17,13 +17,19 @@ import (
 type Service struct {
 	configValue atomic.Value
 	logger      *log.Logger
+	client      *http.Client


The client field here should be an atomic.Value type and to be consistent we should call it clientValue.

nathanielc · 2016-11-19T00:01:29Z

services/alerta/service.go

 }

 func NewService(c Config, l *log.Logger) *Service {
 	s := &Service{
 		logger: l,
 	}
 	s.configValue.Store(c)
+	s.client = &http.Client{


Here, use the Store method to store the new client. Much like the configValue line above.

nathanielc · 2016-11-19T00:01:48Z

services/alerta/service.go

@@ -95,7 +101,13 @@ func (s *Service) Update(newConfig []interface{}) error {
 		return fmt.Errorf("expected config object to be of type %T, got %T", c, newConfig[0])
 	} else {
 		s.configValue.Store(c)
+		s.client = &http.Client{


Same goes here, we should use the Store method.

nathanielc · 2016-11-19T00:02:53Z

services/alerta/service.go

 	if err != nil {
 		return err
 	}

-	resp, err := http.Post(url, "application/json", post)
+	resp, err := s.client.Do(req)


Lastly this becomes:

client := s.clientValue.Get().(*http.Client) resp, err := client.Do(req)

All of these changes avoid a race condition on the client field, synchronizing access to the field.

allen13 · 2016-11-19T01:47:39Z

Race condition resolved.

Move alerta api token to header and add option to skip TLS verification.

allen13 · 2016-11-21T16:29:21Z

Thanks for the help!

nathanielc suggested changes Nov 17, 2016

View reviewed changes

allen13 force-pushed the alerta-tls branch from 14b4262 to b8e6d4b Compare November 17, 2016 21:33

allen13 force-pushed the alerta-tls branch from b8e6d4b to 4920751 Compare November 18, 2016 18:34

nathanielc suggested changes Nov 19, 2016

View reviewed changes

Move alerta api token to header and add option to skip TLS verification.

39b5a8d

allen13 force-pushed the alerta-tls branch from 4920751 to 39b5a8d Compare November 19, 2016 01:30

nathanielc approved these changes Nov 21, 2016

View reviewed changes

nathanielc merged commit 39b5a8d into influxdata:master Nov 21, 2016

nathanielc added a commit that referenced this pull request Nov 21, 2016

Merged pull request #1053 from allen13/alerta-tls

7b2f676

Move alerta api token to header and add option to skip TLS verification.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move alerta api token to header and add option to skip TLS verification. #1053

Move alerta api token to header and add option to skip TLS verification. #1053

allen13 commented Nov 17, 2016 •

edited by nathanielc

Loading

nathanielc left a comment

nathanielc Nov 17, 2016

allen13 Nov 17, 2016

nathanielc Nov 17, 2016

allen13 Nov 17, 2016

allen13 commented Nov 17, 2016

nathanielc commented Nov 18, 2016

allen13 commented Nov 18, 2016

nathanielc left a comment

nathanielc Nov 19, 2016

nathanielc Nov 19, 2016 •

edited

Loading

nathanielc Nov 19, 2016

nathanielc Nov 19, 2016

allen13 commented Nov 19, 2016

allen13 commented Nov 21, 2016

Move alerta api token to header and add option to skip TLS verification. #1053

Move alerta api token to header and add option to skip TLS verification. #1053

Conversation

allen13 commented Nov 17, 2016 • edited by nathanielc Loading

Required for all non-trivial PRs

nathanielc left a comment

Choose a reason for hiding this comment

nathanielc Nov 17, 2016

Choose a reason for hiding this comment

allen13 Nov 17, 2016

Choose a reason for hiding this comment

nathanielc Nov 17, 2016

Choose a reason for hiding this comment

allen13 Nov 17, 2016

Choose a reason for hiding this comment

allen13 commented Nov 17, 2016

nathanielc commented Nov 18, 2016

allen13 commented Nov 18, 2016

nathanielc left a comment

Choose a reason for hiding this comment

nathanielc Nov 19, 2016

Choose a reason for hiding this comment

nathanielc Nov 19, 2016 • edited Loading

Choose a reason for hiding this comment

nathanielc Nov 19, 2016

Choose a reason for hiding this comment

nathanielc Nov 19, 2016

Choose a reason for hiding this comment

allen13 commented Nov 19, 2016

allen13 commented Nov 21, 2016

allen13 commented Nov 17, 2016 •

edited by nathanielc

Loading

nathanielc Nov 19, 2016 •

edited

Loading