-
Notifications
You must be signed in to change notification settings - Fork 44
Add support for proxying user connection to Alertmanager #202
Changes from 1 commit
b6dd993
3eb28b3
db0d86f
6aec8bf
2176c44
c8b7b3b
efda91f
983c7f5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
package main | ||
|
||
import ( | ||
"fmt" | ||
"net/http" | ||
"net/http/httputil" | ||
"net/url" | ||
"strings" | ||
|
||
"github.com/cloudflare/unsee/internal/alertmanager" | ||
"github.com/cloudflare/unsee/internal/config" | ||
"github.com/gin-gonic/gin" | ||
|
||
log "github.com/sirupsen/logrus" | ||
) | ||
|
||
func proxyPathPrefix(name string) string { | ||
return fmt.Sprintf("%sproxy/alertmanager/%s", config.Config.Listen.Prefix, name) | ||
} | ||
|
||
// NewAlertmanagerProxy creates a proxy instance for given alertmanager instance | ||
func NewAlertmanagerProxy(alertmanager *alertmanager.Alertmanager) (*httputil.ReverseProxy, error) { | ||
upstreamURL, err := url.Parse(alertmanager.URI) | ||
if err != nil { | ||
return nil, err | ||
} | ||
proxy := httputil.ReverseProxy{ | ||
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)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use |
||
// drop Accept-Encoding header so we always get uncompressed reponses from | ||
// 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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is possible to accept a logger? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see how that would be useful right now There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mostly useful for configuring the logging for proxy specifically. But mostly a nit. |
||
}, | ||
ModifyResponse: func(resp *http.Response) error { | ||
// drop Content-Length header from upstream responses, gzip middleware | ||
// will compress those and that could cause a mismatch | ||
resp.Header.Del("Content-Length") | ||
return nil | ||
}, | ||
} | ||
return &proxy, nil | ||
} | ||
|
||
func setupRouterProxyHandlers(router *gin.Engine, alertmanager *alertmanager.Alertmanager) error { | ||
proxy, err := NewAlertmanagerProxy(alertmanager) | ||
if err != nil { | ||
return err | ||
} | ||
router.POST(fmt.Sprintf("%s/api/v1/silences", proxyPathPrefix(alertmanager.Name)), gin.WrapH(proxy)) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this not clearer without nesting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's not very readable, how about now? |
||
router.DELETE(fmt.Sprintf("%s/api/v1/silence/*id", proxyPathPrefix(alertmanager.Name)), gin.WrapH(proxy)) | ||
return nil | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,121 @@ | ||
package main | ||
|
||
import ( | ||
"net/http" | ||
"net/http/httptest" | ||
"testing" | ||
"time" | ||
|
||
"github.com/cloudflare/unsee/internal/alertmanager" | ||
|
||
httpmock "gopkg.in/jarcoal/httpmock.v1" | ||
) | ||
|
||
// httptest.NewRecorder() doesn't implement http.CloseNotifier | ||
type closeNotifyingRecorder struct { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't see you using the CloseNotify method. Is this recorder necessary? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have time to research myself, but if you eventually figure it out, I'd like to know. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is that blocking this merge? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No. |
||
*httptest.ResponseRecorder | ||
closed chan bool | ||
} | ||
|
||
func newCloseNotifyingRecorder() *closeNotifyingRecorder { | ||
return &closeNotifyingRecorder{ | ||
httptest.NewRecorder(), | ||
make(chan bool, 1), | ||
} | ||
} | ||
|
||
func (c *closeNotifyingRecorder) close() { | ||
c.closed <- true | ||
} | ||
|
||
func (c *closeNotifyingRecorder) CloseNotify() <-chan bool { | ||
return c.closed | ||
} | ||
|
||
type proxyTest struct { | ||
method string | ||
localPath string | ||
upstreamURI string | ||
code int | ||
response string | ||
} | ||
|
||
var proxyTests = []proxyTest{ | ||
// valid alertmanager and methods | ||
proxyTest{ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. struct names are unneeded inside a slice. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. TIL |
||
method: "POST", | ||
localPath: "/proxy/alertmanager/dummy/api/v1/silences", | ||
upstreamURI: "http://localhost:9093/api/v1/silences", | ||
code: 200, | ||
response: "{\"status\":\"success\",\"data\":{\"silenceId\":\"d8a61ca8-ee2e-4076-999f-276f1e986bf3\"}}", | ||
}, | ||
proxyTest{ | ||
method: "DELETE", | ||
localPath: "/proxy/alertmanager/dummy/api/v1/silence/d8a61ca8-ee2e-4076-999f-276f1e986bf3", | ||
upstreamURI: "http://localhost:9093/api/v1/silence/d8a61ca8-ee2e-4076-999f-276f1e986bf3", | ||
code: 200, | ||
response: "{\"status\":\"success\"}", | ||
}, | ||
// invalid alertmanager name | ||
proxyTest{ | ||
method: "POST", | ||
localPath: "/proxy/alertmanager/INVALID/api/v1/silences", | ||
upstreamURI: "", | ||
code: 404, | ||
response: "404 page not found", | ||
}, | ||
proxyTest{ | ||
method: "DELETE", | ||
localPath: "/proxy/alertmanager/INVALID/api/v1/silence/d8a61ca8-ee2e-4076-999f-276f1e986bf3", | ||
upstreamURI: "http://localhost:9093/api/v1/silence/d8a61ca8-ee2e-4076-999f-276f1e986bf3", | ||
code: 404, | ||
response: "404 page not found", | ||
}, | ||
// valid alertmanager name, but invalid method | ||
proxyTest{ | ||
method: "GET", | ||
localPath: "/proxy/alertmanager/dummy/api/v1/silences", | ||
upstreamURI: "", | ||
code: 404, | ||
response: "404 page not found", | ||
}, | ||
proxyTest{ | ||
method: "GET", | ||
localPath: "/proxy/alertmanager/dummy/api/v1/silence/d8a61ca8-ee2e-4076-999f-276f1e986bf3", | ||
upstreamURI: "http://localhost:9093/api/v1/silence/d8a61ca8-ee2e-4076-999f-276f1e986bf3", | ||
code: 404, | ||
response: "404 page not found", | ||
}, | ||
} | ||
|
||
func TestProxy(t *testing.T) { | ||
r := ginTestEngine() | ||
setupRouterProxyHandlers(r, &alertmanager.Alertmanager{ | ||
URI: "http://localhost:9093", | ||
Timeout: time.Second * 5, | ||
Name: "dummy", | ||
ProxyRequests: true, | ||
}) | ||
|
||
httpmock.Activate() | ||
defer httpmock.DeactivateAndReset() | ||
|
||
for _, testCase := range proxyTests { | ||
httpmock.Reset() | ||
if testCase.upstreamURI != "" { | ||
httpmock.RegisterResponder(testCase.method, testCase.upstreamURI, httpmock.NewStringResponder(testCase.code, testCase.response)) | ||
} | ||
req, _ := http.NewRequest(testCase.method, testCase.localPath, nil) | ||
resp := newCloseNotifyingRecorder() | ||
r.ServeHTTP(resp, req) | ||
if resp.Code != testCase.code { | ||
t.Errorf("%s %s proxied to %s returned status %d while %d was expected", | ||
testCase.method, testCase.localPath, testCase.upstreamURI, resp.Code, testCase.code) | ||
} | ||
body := resp.Body.String() | ||
if body != testCase.response { | ||
t.Errorf("%s %s proxied to %s returned content '%s' while '%s' was expected", | ||
testCase.method, testCase.localPath, testCase.upstreamURI, body, testCase.response) | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd suggest using the functional options pattern instead. While reviewing consumer code, it's not clear what the lonesome
false
means.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An example of what this might look like: https://gist.github.com/terinjokes/18c666ec4a52cd5bd10df29b4e6f95a6