Add support for proxying user connection to Alertmanager #202
Conversation
proxy.go
Outdated
log "github.com/sirupsen/logrus" | ||
) | ||
|
||
func newProxyServer(domain string) *httputil.ReverseProxy { |
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.
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.
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.
Not sure if that's really needed for tests, I got some now and didn't need it for that
proxy.go
Outdated
} else if !upstream.ProxyRequests { | ||
c.String(http.StatusBadRequest, "Alertmanager instance named '%s' does not allow proxying\n", name) | ||
} else { | ||
proxy := newProxyServer(upstream.URI) |
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.
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
.
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.
Updated, is this better? I also got some tests this time
27ac5e8
to
ccfa04f
Compare
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.
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.
Looking better, but I think there's room for some improvements!
proxy.go
Outdated
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Is this not clearer without nesting Sprintf
s?
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.
It's not very readable, how about now?
proxy.go
Outdated
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 comment
The reason will be displayed to describe this comment to others. Learn more.
Use net/http.StripPrefix
instead.
// 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 comment
The 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 comment
The 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 comment
The 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.
proxy_test.go
Outdated
|
||
var proxyTests = []proxyTest{ | ||
// valid alertmanager and methods | ||
proxyTest{ |
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.
struct names are unneeded inside a slice.
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.
TIL
) | ||
|
||
// httptest.NewRecorder() doesn't implement http.CloseNotifier | ||
type closeNotifyingRecorder struct { |
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 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 comment
The 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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
No.
internal/alertmanager/upstream.go
Outdated
@@ -15,7 +15,7 @@ var ( | |||
) | |||
|
|||
// 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 { |
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
internal/alertmanager/upstream.go
Outdated
// a custom timeout for Alertmanager upstream requests | ||
func WithRequestTimeout(timeout time.Duration) Option { | ||
return func(am *Alertmanager) { | ||
am.Timeout = timeout |
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.
Any reason not to change the struct member name as well? Any reason to set a request timeout rather than using a context.WithTimeout
?
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.
- 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
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.
Assuming the CI tests pass, I think this is reasonable to merge. See my note about a good follow-up PR.
internal/alertmanager/upstream.go
Outdated
var ( | ||
upstreams = map[string]*Alertmanager{} | ||
) | ||
|
||
// NewAlertmanager creates a new Alertmanager instance | ||
func NewAlertmanager(name, uri string, timeout time.Duration) error { | ||
func NewAlertmanager(name, uri string, opts ...Option) error { |
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.
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.
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.
Yeah, I'm refactoring this
This makes it easier to test code
If it doesn't passed I can't merge it anyway. 983c7f5 should address last comment |
internal/alertmanager/upstream.go
Outdated
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 { |
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.
Would be great if this was on a collection type, rather than modifying a global variable. Again, happy to see it as another change.
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.