Add support for proxying user connection to Alertmanager by prymitive · Pull Request #202 · cloudflare/unsee

prymitive · 2017-12-07T06:52:49Z

Fixes #190.

With this feature unsee can be configured to proxy requests to selected Alertmanager instances, if it's enabled unsee silence form will send a request via unsee rather than directly. This allows users to manage silences in environments where they have access to unsee but not to Alertmanager. Only silences endpoints on Alertmanager API are proxied.

terinjokes · 2017-12-08T23:32:33Z

proxy.go

+	log "github.com/sirupsen/logrus"
+)
+
+func newProxyServer(domain string) *httputil.ReverseProxy {


You may wish to take an http.RoundTripper (to set as ReverseProxy.Transport) as an option here so you can observe and influence the behavior of the proxy in test.

Not sure if that's really needed for tests, I got some now and didn't need it for that

terinjokes · 2017-12-08T23:41:06Z

proxy.go

+	} else if !upstream.ProxyRequests {
+		c.String(http.StatusBadRequest, "Alertmanager instance named '%s' does not allow proxying\n", name)
+	} else {
+		proxy := newProxyServer(upstream.URI)


You probably don't want to create a whole new proxy for each request. An alternative architecture could rely on the fact that ReverseProxy satisfies the http.Handler interface, and that you can add an http.Handler to a Gin router with gin.WrapH(http.Handler) gin.HandlerFunc. Then you can direct the two routes to the upstream by implementing ReverseProxy.Director.

Updated, is this better? I also got some tests this time

Fixes #190. With this feature unsee can be configured to proxy requests to selected Alertmanager instances, if it's enabled unsee silence form will send a request via unsee rather than directly. This allows users to manage silences in environments where they have access to unsee but not to Alertmanager. Only silences endpoints on Alertmanager API are proxied.

terinjokes

Looking better, but I think there's room for some improvements!

terinjokes · 2018-01-04T19:48:06Z

proxy.go

+	if err != nil {
+		return err
+	}
+	router.POST(fmt.Sprintf("%s/api/v1/silences", proxyPathPrefix(alertmanager.Name)), gin.WrapH(proxy))


Is this not clearer without nesting Sprintfs?

It's not very readable, how about now?

terinjokes · 2018-01-04T19:49:15Z

proxy.go

+		Director: func(req *http.Request) {
+			req.URL.Scheme = upstreamURL.Scheme
+			req.URL.Host = upstreamURL.Host
+			req.URL.Path = strings.TrimPrefix(req.URL.Path, proxyPathPrefix(alertmanager.Name))


Use net/http.StripPrefix instead.

terinjokes · 2018-01-04T19:50:34Z

proxy.go

+			// upstream, there's a gzip middleware that's global so we don't want it
+			// to gzip twice
+			req.Header.Del("Accept-Encoding")
+			log.Debugf("[%s] Proxy request for %s", alertmanager.Name, req.URL.Path)


It is possible to accept a logger?

I don't see how that would be useful right now

Mostly useful for configuring the logging for proxy specifically. But mostly a nit.

terinjokes · 2018-01-04T19:51:24Z

proxy_test.go

+
+var proxyTests = []proxyTest{
+	// valid alertmanager and methods
+	proxyTest{


struct names are unneeded inside a slice.

terinjokes · 2018-01-04T19:52:43Z

proxy_test.go

+)
+
+// httptest.NewRecorder() doesn't implement http.CloseNotifier
+type closeNotifyingRecorder struct {


I don't see you using the CloseNotify method. Is this recorder necessary?

I'm not 100% sure as I didn't go that deep into the issue, but testes were failing because of it and after some research I wasn't sure if this is gin issue or httptest

I don't have time to research myself, but if you eventually figure it out, I'd like to know.

Is that blocking this merge?

terinjokes · 2018-01-04T19:54:57Z

internal/alertmanager/upstream.go


 // NewAlertmanager creates a new Alertmanager instance
-func NewAlertmanager(name, uri string, timeout time.Duration) error {
+func NewAlertmanager(name, uri string, timeout time.Duration, proxyRequests bool) error {


I'd suggest using the functional options pattern instead. While reviewing consumer code, it's not clear what the lonesome false means.

An example of what this might look like: https://gist.github.com/terinjokes/18c666ec4a52cd5bd10df29b4e6f95a6

terinjokes · 2018-01-08T21:34:31Z

internal/alertmanager/upstream.go

+// a custom timeout for Alertmanager upstream requests
+func WithRequestTimeout(timeout time.Duration) Option {
+	return func(am *Alertmanager) {
+		am.Timeout = timeout


Any reason not to change the struct member name as well? Any reason to set a request timeout rather than using a context.WithTimeout?

None, renamed

I know about context but didn't really use it for anything so lack any experience and obvious patterns I could use it for

terinjokes

Assuming the CI tests pass, I think this is reasonable to merge. See my note about a good follow-up PR.

terinjokes · 2018-01-08T22:06:44Z

internal/alertmanager/upstream.go


 // NewAlertmanager creates a new Alertmanager instance
-func NewAlertmanager(name, uri string, timeout time.Duration) error {
+func NewAlertmanager(name, uri string, opts ...Option) error {


nit: I'd argue this should be AddAlertmanager because while you do allocate you don't actually get the Alertmanager instance back.

For testing reasons (you're creating the Alertmanger manually in test) perhaps it should, and leave it up to the consumer to add it to the upstreams slice/type. I'll accept this as a separate PR though.

Yeah, I'm refactoring this

This makes it easier to test code

prymitive · 2018-01-08T22:42:08Z

If it doesn't passed I can't merge it anyway. 983c7f5 should address last comment

terinjokes · 2018-01-09T00:23:31Z

internal/alertmanager/upstream.go

-	log.Infof("[%s] Configured Alertmanager source at %s", name, uri)
+// RegisterAlertmanager will add an Alertmanager instance to the list of
+// instances used when pulling alerts from upstreams
+func RegisterAlertmanager(am *Alertmanager) error {


Would be great if this was on a collection type, rather than modifying a global variable. Again, happy to see it as another change.

prymitive added the enhancement label Dec 7, 2017

prymitive added this to the v0.9 milestone Dec 7, 2017

prymitive requested review from bobrik, jamesog and terinjokes December 7, 2017 06:52

prymitive mentioned this pull request Dec 7, 2017

Proxy client requests through unsee instance #190

Closed

terinjokes suggested changes Dec 8, 2017

View reviewed changes

prymitive force-pushed the proxy branch 2 times, most recently from 27ac5e8 to ccfa04f Compare December 23, 2017 11:58

prymitive removed this from the v0.9 milestone Dec 29, 2017

prymitive force-pushed the proxy branch from ccfa04f to b6dd993 Compare January 4, 2018 00:18

terinjokes suggested changes Jan 4, 2018

View reviewed changes

prymitive added 5 commits January 6, 2018 17:19

Use functional options pattern when creating Alertmanager instances

3eb28b3

Don't modify path in the proxy, strip prefix in the handler

db0d86f

Cleanup path setup a bit

6aec8bf

Drop names in slices

2176c44

Implement functional options, this time for real

c8b7b3b

terinjokes suggested changes Jan 8, 2018

View reviewed changes

Rename Alertmanager.Timeout -> Alertmanager.RequestTimeout

efda91f

cloudflare deleted a comment from prymitive Jan 8, 2018

terinjokes approved these changes Jan 8, 2018

View reviewed changes

Refactor NewAlertmanager() to not register new instance by default

983c7f5

This makes it easier to test code

terinjokes reviewed Jan 9, 2018

View reviewed changes

prymitive merged commit a3af1ec into master Jan 9, 2018

prymitive deleted the proxy branch January 9, 2018 00:23

Comments

Conversation

prymitive commented Dec 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terinjokes Dec 8, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terinjokes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

terinjokes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

prymitive commented Jan 8, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

terinjokes Dec 8, 2017 •

edited

Loading