WIP: Add httppost service (add headers to POST alerts) #1254

desa · 2017-03-10T17:52:49Z

This PR allows users to create POST request endpoints for an alert node where arbitrary HTTP headers may be set. It works as a fix for #117.

Todo:

Add comments
Cleanup
Test how ENV variables work with toml maps

Required for all non-trivial PRs

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

desa

Few changes

desa · 2017-03-10T17:54:57Z

alert.go

+	//	}
+	//	h := alertservice.NewPostHandler(c, l)
+	//	an.handlers = append(an.handlers, h)
+	//}


desa · 2017-03-10T17:56:12Z

alert.go

+		}
+		if p.Endpoint != "" {
+			c.Endpoint = p.Endpoint
+		}


Not necessary.

Just do

c.URL = p.URL c.Endpoint = p.Endpoint

desa · 2017-03-10T17:57:26Z

etc/kapacitor/kapacitor.conf

+#   endpoint = "example"
+#   url = "http://example.com"
+#   headers = { Authorization = "your-key" }
+


How does this work with environment variables?

Good question, have we tried it? I think there is support for maps in our env code but we should test it. Even add a test to the server package for it.

desa · 2017-03-10T17:59:03Z

integrations/streamer_test.go

@@ -9523,6 +9597,7 @@ func testStreamer(
 	tm.HTTPDService = httpdService
 	tm.TaskStore = taskStore{}
 	tm.DeadmanService = deadman{}
+	tm.PostService = alertpost.NewService(nil, logService.NewLogger("[alertpost] ", log.LstdFlags))


Change to

tm.AlertPostService = alertpost.NewService(nil, logService.NewLogger("[alertpost] ", log.LstdFlags))

desa · 2017-03-10T17:59:41Z

pipeline/alert.go

@@ -485,6 +507,9 @@ type PostHandler struct {
 	// The POST URL.
 	// tick:ignore
 	URL string
+
+	// Name of the endpoint to be used, as defined in the configuration file


as is defined by the configuration.

desa · 2017-03-10T18:00:21Z

server/server.go

@@ -457,6 +459,18 @@ func (s *Server) appendPushoverService() {
 	s.AppendService("pushover", srv)
 }

+func (s *Server) appendPostService() {


appendAlertPostService

desa · 2017-03-10T18:01:12Z

services/alert/service.go

@@ -108,6 +109,9 @@ type Service struct {
 	PushoverService interface {
 		Handler(pushover.HandlerConfig, *log.Logger) alert.Handler
 	}
+	PostService interface {


AlertPostService

desa · 2017-03-10T18:02:30Z

services/alertpost/service.go

+
+func (s *Service) TestOptions() interface{} {
+	return &testOptions{}
+}


Can this be removed?

No, it needs to implement the interface. Also would having and endpoint option make sense so you can test each configured endpoint?

desa · 2017-03-10T18:03:00Z

task_master.go

@@ -105,6 +106,9 @@ type TaskMaster struct {
 	PushoverService interface {
 		Handler(pushover.HandlerConfig, *log.Logger) alert.Handler
 	}
+	PostService interface {


AlertPostService

desa · 2017-03-14T15:21:48Z

@nathanielc this should be in a reviewable state.

nathanielc

Overall looks good. I tried to answer some of your questions and had a few of my own.

nathanielc · 2017-03-14T15:23:50Z

etc/kapacitor/kapacitor.conf

+#   endpoint = "example"
+#   url = "http://example.com"
+#   headers = { Authorization = "your-key" }
+


Good question, have we tried it? I think there is support for maps in our env code but we should test it. Even add a test to the server package for it.

nathanielc · 2017-03-14T15:26:38Z

services/alertpost/service.go

+
+func (s *Service) TestOptions() interface{} {
+	return &testOptions{}
+}


No, it needs to implement the interface. Also would having and endpoint option make sense so you can test each configured endpoint?

nathanielc · 2017-03-14T15:29:11Z

services/alertpost/service.go

+
+	// Create the HTTP request
+	var req *http.Request
+	if h.url != "" {


Should we validate in pipeline that you do not have both endpoint and url specified?

How would we validate this in pipeline?

nathanielc · 2017-03-14T15:31:11Z

services/alertpost/service.go

+	Duration time.Duration `json:"duration"`
+	Level    alert.Level   `json:"level"`
+	Data     models.Result `json:"data"`
+}


Hmm, should we move the AlertData struct out of the alert/service package and into the alert package? The purpose of the struct is to provide a consistent view of how alert data is exposed so I'd rather there only be one definition of the type.

Yeah, that sounds like a good idea.

desa · 2017-04-25T19:03:34Z

@nathanielc this should be in a reviewable state.

nathanielc

I have two requested changes.

First, that we separate the different use cases of the httppost service a bit more.

There are two use cases for the service:

Post arbitrary data to an endpoint.
Post alert data to an endpoint.

Both of these uses cases naturally involve posting data. But they also both involve working with endpoints, not handlers.

My suggestion is the have the httppost service expose an Endpoint type that the httpPost node consumes instead of the handler interface.

Then you can create a Handler type that consumes the Endpoint type.

My second request is about how live config updates propagate. In the current state changes to endpoints do not effect running tasks and they should. We need a way for the running alert handlers and httpPost nodes to get these live updates. I think the type separating above will help with this, since we can lock the NewHTTPRequest method and then modify the endpoints instead of having to hand out new ones.

Do those requests make sense? Can I clarify anything?

nathanielc · 2017-04-26T15:41:02Z

http_post.go

-	mu  sync.RWMutex
-	bp  *bufpool.Pool
+	c        *pipeline.HTTPPostNode
+	handlers []*httppost.Handler


It feels odd that the HTTPPost node is dealing with alert handlers. Maybe we should rename the type httppost.Handler to httppost.Endpoint since all it really is is an endpoint. This change also helps clarify the purpose of the type since currently the Handler type is not used for its Handle method, in this context.

nathanielc · 2017-04-26T15:42:47Z

http_post.go

+	if n.Url != "" {
+		c := httppost.HandlerConfig{}
+		c.URL = n.Url
+		h := et.tm.HTTPPostService.Handler(c, l)


The type change is also useful here since you can return the concrete type so you do not have to type assert it. Or event add a new method for getting Endpoints.

nathanielc · 2017-04-26T15:43:19Z

http_post.go

+	// Technically we don't need the wait group, but
+	// having it hear will help contain any leaky go
+	// routines
+	var wg sync.WaitGroup


We should probably defer wg.Wait() here instead of at the end of the function.

nathanielc · 2017-04-26T15:43:50Z

http_post.go

-		h.logger.Printf("E! failed to POST row data: %v", err)
-		return
+			req.Header.Set("Content-Type", "application/json")
+			resp, err := http.DefaultClient.Do(req)


Does this break the HTTP proxy support we added? Or had that not been merged yet?

It hasn't been merged yet, but I also think that it won't break. The default client, I'm pretty sure, does proxy from environment.

nathanielc · 2017-04-26T15:45:28Z

alert/types.go

+
+// AlertData is a structure that contains relevant data about an alert event.
+// The structure is intended to be JSON encoded, providing a consistent data format.
+type AlertData struct {


The move was necessary since we moved the post handler out of the alertservice package correct?

Also should we rename the type to Data to remove the stutter?

Yup. Stutter removed.

nathanielc · 2017-04-26T15:50:16Z

pipeline/node.go

@@ -327,8 +327,8 @@ func (n *chainnode) HttpOut(endpoint string) *HTTPOutNode {
 }

 // Creates an HTTP Post node that POSTS received data to the provided HTTP endpoint.
-func (n *chainnode) HttpPost(url string) *HTTPPostNode {
-	h := newHTTPPostNode(n.provides, url)
+func (n *chainnode) HttpPost(url ...string) *HTTPPostNode {


We should document that the URL is optional if the endpoint is specified, hence the variadic signature.

Where should this be documented?

Added it in the description of HTTPPostNode.

This can also be documented here in the comment for this function. The comment will become part of the auto generated reference docs.

nathanielc · 2017-04-26T15:51:00Z

server/config.go

+		}
+	}
+	return nil
+}


Do we have a test for this code?

Nope, I should add that. Is there a place where the logic for array env elements lives?

nathanielc · 2017-04-26T15:54:35Z

services/httppost/service.go

+	defer s.mu.Unlock()
+	for _, nc := range newConfigs {
+		if c, ok := nc.(Config); ok {
+			s.endpoints[c.Endpoint] = c


This does look like it is going to work.

The config updates need to effect live code, meaning if you change the URL for an endpoint then the existing alert handlers and httpPost nodes need to update to the new URL. This should be easily accomplished by locking the Handler|Endpoint type NewHTTPRequest method etc.

As it currently stands the configs are read once so these changes never propagate to running code.

Let me know if I can help clarify this more.

I could be crazy, but I think this works.

nathanielc · 2017-04-26T15:55:09Z

services/httppost/service.go

+	}
+
+	// Create the HTTP request
+	var req *http.Request


Why not use the NewHTTPRequest method here?

nathanielc · 2017-04-26T15:56:07Z

http_post.go

-	bp  *bufpool.Pool
+	c        *pipeline.HTTPPostNode
+	handlers []*httppost.Handler
+	url      string


I believe this field is now unused.

desa · 2017-04-26T16:33:43Z

@nathanielc

My suggestion is the have the httppost service expose an Endpoint type that the httpPost node consumes instead of the handler interface.

I'll fix that right now.

My second request is about how live config updates propagate. In the current state changes to endpoints do not effect running tasks and they should. We need a way for the running alert handlers and httpPost nodes to get these live updates. I think the type separating above will help with this, since we can lock the NewHTTPRequest method and then modify the endpoints instead of having to hand out new ones.

I think this currently works.
Using this tickscript

var data = stream
    |from()
        .measurement('cpu')
    |window()
        .period(5s)
        .every(5s)
    |httpPost()
        .endpoint('hello')

with configuration

...
[[httppost]]
    endpoint = "hello"
    url = "http://localhost:7070/"
    headers = { Authorization = "your-key" }

and a telegraf reporting every second.

when I first enable the task, the data is written to a mock server I have on 7070. If I then run

 curl -XPOST http://localhost:9092/kapacitor/v1/config/httppost/hello -d '{ "set": { "url": "http://localhost:5050"} }'

the data no longer written to 7070 and is written to 5050.

nathanielc

OK I see how the dynamic update is working now. I have another suggestion to further separate out Endpoints vs Handlers.

nathanielc · 2017-04-26T20:13:56Z

pipeline/http_post.go

 	}
 }

+// tick:ignore
+func (p *HTTPPostNode) Validate() error {


This should be named validate in order to be automatically called in the parsing execution steps.

nathanielc · 2017-04-26T20:14:52Z

pipeline/http_post.go

+	return nil
+}
+
+func (p *HTTPPostNode) URL() (u string, err error) {


While this is a nice clean up we can't do this as the public API of this struct is callable via TICKscript. We have to treat pipeline struct types as config structs with no behavior.

ah dang :( I'll come up with another solution.

Simplest thing is to define this method inside http_post.go like

func nodeURL(p *pipeline.HTTPPostNode) (u string, err error) { }

it needs a better name but you get the idea.

nathanielc · 2017-04-26T20:17:53Z

services/httppost/service.go

+		}
+		return
+	}
+	c, ok := e.s.endpoint(e.name)


This is the bit I missed before, we reread the config on every NewHTTPRequest, which is why it is working.

Although my suggestion above invalidates this way of doing it.

nathanielc · 2017-04-26T20:19:06Z

services/httppost/service.go

+	return c.NewRequest(body)
+}
+
+func (s *Service) Endpoint(c HandlerConfig) *Endpoint {


Why do we need to create a HandlerConfig to get an Endpoint?

nathanielc · 2017-04-26T20:21:34Z

services/httppost/service.go

+type Endpoint struct {
+	s      *Service
+	name   string
+	url    string


As a preference I would like to see Endpoint as having a url field and not know its name. Then for the Endpoints created with just a URL use the URL as the name of the endpoint in the map.

This way all endpoints are the same and just reference a URL and a set of headers.

Also, where are the headers? Are they in the config struct still?

This way all endpoints are the same and just reference a URL and a set of headers.

I like this idea, but I think we might need the name for the case when an endpoint hasn't been defined, but is used in a tickscript. When the task in initialized and the endpoint doesn't currently exist and then we create the endpoint. When the configuration is updated how do we know which httppost.Endpoints need to have their values updated? I could be over thinking this.

Also, where are the headers? Are they in the config struct still?

Yeah they're only in the config struct.

nathanielc

I really like how the Endpoint stuff came together!

I have a few more comments, mostly smaller things I didn't notice before.

nathanielc · 2017-04-27T16:56:51Z

pipeline/node.go

@@ -327,8 +327,8 @@ func (n *chainnode) HttpOut(endpoint string) *HTTPOutNode {
 }

 // Creates an HTTP Post node that POSTS received data to the provided HTTP endpoint.
-func (n *chainnode) HttpPost(url string) *HTTPPostNode {
-	h := newHTTPPostNode(n.provides, url)
+func (n *chainnode) HttpPost(url ...string) *HTTPPostNode {


This can also be documented here in the comment for this function. The comment will become part of the auto generated reference docs.

nathanielc · 2017-04-27T16:57:50Z

server/config_test.go

@@ -70,5 +77,7 @@ boltdb = "/tmp/kapacitor.db"
 		t.Fatalf("unexpected storage boltdb-path: %s", c.Storage.BoltDBPath)
 	} else if c.InfluxDB[0].URLs[0] != "http://localhost:18086" {
 		t.Fatalf("unexpected url 0: %s", c.InfluxDB[0].URLs[0])
+	} else if c.HTTPPost[0].Headers["Authorization"] != "my-key" {
+		t.Fatalf("unexpected header Authorization: %s", c.InfluxDB[0].URLs[0])


Can you add a test case where you add a key to the headers map that isn't already there in the file?

nathanielc · 2017-04-27T17:04:35Z

services/httppost/httpposttest/server.go

+
+type Request struct {
+	MatchingHeaders bool
+	Data            alert.Data


This seems incorrect now. The posted data could be either alert data or just a raw row of data. Should we try and capture both or alternately we could just capture the entire body and let the consumer of this test type decode it.

nathanielc · 2017-04-27T17:08:01Z

services/httppost/service.go

+				s.endpoints[c.Endpoint] = NewEndpoint(c.URL, c.Headers)
+				continue
+			}
+			e.SetURL(c.URL)


I think we want these Set calls to be atomic together. Meaning there is a slight chance that the endpoint is used right after the URL is modified but before the Headers are which could cause issues. Maybe we should just have an Update method that takes a Config? Lots of ways to do it, but all updates should happen with one lock/unlock of the mutex.

nathanielc · 2017-04-27T17:09:04Z

services/httppost/service.go

+	resp.Body.Close()
+}
+
+func alertDataFromEvent(event alert.Event) alert.Data {


Doesn't this function already exist in another package?

yup it is, I'll export and use it in this package.

Great, you can move it to the alert package now too. Maybe even just make it a method of the Event type, but what ever looks right to you.

nathanielc

LGTM 🎉

nathanielc · 2017-05-01T19:21:03Z

CHANGELOG.md

@@ -69,6 +69,7 @@ kapacitor define-handler system aggregate_by_1m.yaml
 - [#507](https://github.com/influxdata/kapacitor/issues/507): Add API endpoint for performing Kapacitor database backups.
 - [#1132](https://github.com/influxdata/kapacitor/issues/1132): Adding source for sensu alert as parameter
 - [#1346](https://github.com/influxdata/kapacitor/pull/1346): Add discovery and scraping services.
+- [#117](https://github.com/influxdata/kapacitor/issues/117): Add headers to alert POST requests.


Can you move this up to the next section now that beta1 was cut?

Which section? the unreleased section?

Add enough to make tests pass Add correct POST alert handling Add tests for .endpoint on .post on alert node Add alertpost config tests Remove NewConfig section Add comments to alertpost/config.go Add fixes suggested in PR Move AlertData to alert package from service/alert Implement Test function Allow override variables to set map values Rename alertpost service to httppost Favorite package name is httpposttest :) Add endpoint property method to httpPost Allow variadic httpPost arguments Update config file for httppost Add endpoint test to http post node Make Fixes suggested in PR Add documentation for variadic arguments it httpPost Add test for map enviornment vars Refactor httppost endpoints Reorder code in httpost service package Add support for basic auth to endpoints Make changes from PR Fix httpPost functionality Allow headers to be set for alerts via tickscript Check headers Unredact headers

desa added the in progress label Mar 10, 2017

desa commented Mar 10, 2017

View reviewed changes

desa force-pushed the md-issue#117 branch 2 times, most recently from 6043675 to a975e89 Compare March 14, 2017 15:21

nathanielc suggested changes Mar 14, 2017

View reviewed changes

desa force-pushed the md-issue#117 branch from a975e89 to 6e4c4e8 Compare April 12, 2017 18:32

nathanielc mentioned this pull request Apr 19, 2017

Add a generic HTTPPost node #1331

Merged

5 tasks

desa force-pushed the md-issue#117 branch 5 times, most recently from c4bd28d to b2ea7e7 Compare April 25, 2017 15:01

desa changed the title ~~WIP: Add alertpost service (add headers to POST alerts)~~ WIP: Add httppost service (add headers to POST alerts) Apr 25, 2017

nathanielc suggested changes Apr 26, 2017

View reviewed changes

desa removed the in progress label Apr 26, 2017

nathanielc suggested changes Apr 26, 2017

View reviewed changes

nathanielc suggested changes Apr 27, 2017

View reviewed changes

desa force-pushed the md-issue#117 branch 2 times, most recently from 513c8a9 to 5f3dec3 Compare May 1, 2017 17:02

nathanielc approved these changes May 1, 2017

View reviewed changes

desa force-pushed the md-issue#117 branch from 5f3dec3 to 40023cd Compare May 1, 2017 19:29

desa merged commit 3f5c354 into master May 1, 2017

desa deleted the md-issue#117 branch May 1, 2017 19:41

WIP: Add httppost service (add headers to POST alerts) #1254

WIP: Add httppost service (add headers to POST alerts) #1254

Conversation

desa commented Mar 10, 2017 • edited Loading

Required for all non-trivial PRs

desa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

desa commented Mar 14, 2017

nathanielc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

desa commented Apr 25, 2017

nathanielc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

desa commented Apr 26, 2017 • edited Loading

nathanielc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanielc Apr 26, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanielc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nathanielc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

desa commented Mar 10, 2017 •

edited

Loading

desa commented Apr 26, 2017 •

edited

Loading

nathanielc Apr 26, 2017 •

edited

Loading