From 309236c42c218c4f23f7eadee8f41c06da4c9aba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Thu, 15 Sep 2022 14:50:45 +0100 Subject: [PATCH] fix(backend): workaround handling of am instances with slashes The issue is really with go-chi, but this will make it work for now. Fixes #4674. --- CHANGELOG.md | 7 ++ cmd/karma/main.go | 1 + cmd/karma/proxy.go | 12 +++ cmd/karma/proxy_test.go | 201 +++++++++++++++++++++++----------------- cmd/karma/views_test.go | 1 + 5 files changed, 138 insertions(+), 84 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f96ecc667..661e30524 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,12 @@ # Changelog +## v0.109 + +### Fixed + +- Managing silences for alertmanager instances with `/` in the name was failing. + This release adds a workaround for it - #4674. + ## v0.108 ### Fixed diff --git a/cmd/karma/main.go b/cmd/karma/main.go index 3e127274b..3cb206954 100644 --- a/cmd/karma/main.go +++ b/cmd/karma/main.go @@ -80,6 +80,7 @@ func getViewURL(sub string) string { func setupRouter(router *chi.Mux, historyPoller *historyPoller) { _ = mime.AddExtensionType(".ico", "image/x-icon") + router.Use(proxyPathFixMiddleware) router.Use(promMiddleware) router.Use(middleware.RealIP) diff --git a/cmd/karma/proxy.go b/cmd/karma/proxy.go index 426ad65b5..5eacdef82 100644 --- a/cmd/karma/proxy.go +++ b/cmd/karma/proxy.go @@ -10,6 +10,7 @@ import ( "strings" "github.com/go-chi/chi/v5" + "github.com/go-chi/chi/v5/middleware" "github.com/prymitive/karma/internal/alertmanager" "github.com/prymitive/karma/internal/config" @@ -177,3 +178,14 @@ func setupRouterProxyHandlers(router *chi.Mux, alertmanager *alertmanager.Alertm h.ServeHTTP(w, r) })) } + +// this fixes a problem with go-chi usage of RawPath +// see https://github.com/prymitive/karma/issues/4674 +// and https://github.com/go-chi/chi/issues/641 +func proxyPathFixMiddleware(next http.Handler) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + r.URL.RawPath = "" + ww := middleware.NewWrapResponseWriter(w, r.ProtoMajor) + next.ServeHTTP(ww, r) + }) +} diff --git a/cmd/karma/proxy_test.go b/cmd/karma/proxy_test.go index 6e27ced26..a7de62f04 100644 --- a/cmd/karma/proxy_test.go +++ b/cmd/karma/proxy_test.go @@ -40,60 +40,6 @@ 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 - { - method: "POST", - localPath: "/proxy/alertmanager/dummy%20with%20%28space%29/api/v2/silences", - upstreamURI: "http://localhost:9093/api/v2/silences", - code: 200, - response: "{\"status\":\"success\",\"data\":{\"silenceId\":\"d8a61ca8-ee2e-4076-999f-276f1e986bf3\"}}", - }, - { - method: "DELETE", - localPath: "/proxy/alertmanager/dummy%20with%20%28space%29/api/v2/silence/d8a61ca8-ee2e-4076-999f-276f1e986bf3", - upstreamURI: "http://localhost:9093/api/v2/silence/d8a61ca8-ee2e-4076-999f-276f1e986bf3", - code: 200, - response: "{\"status\":\"success\"}", - }, - // invalid alertmanager name - { - method: "POST", - localPath: "/proxy/alertmanager/INVALID/api/v2/silences", - upstreamURI: "", - code: 404, - response: "404 page not found\n", - }, - { - method: "DELETE", - localPath: "/proxy/alertmanager/INVALID/api/v2/silence/d8a61ca8-ee2e-4076-999f-276f1e986bf3", - upstreamURI: "http://localhost:9093/api/v2/silence/d8a61ca8-ee2e-4076-999f-276f1e986bf3", - code: 404, - response: "404 page not found\n", - }, - // valid alertmanager name, but invalid method - { - method: "GET", - localPath: "/proxy/alertmanager/dummy%20with%20%28space%29/api/v2/silences", - upstreamURI: "", - code: 405, - }, - { - method: "GET", - localPath: "/proxy/alertmanager/dummy%20with%20%28space%29/api/v2/silence/d8a61ca8-ee2e-4076-999f-276f1e986bf3", - upstreamURI: "http://localhost:9093/api/v2/silence/d8a61ca8-ee2e-4076-999f-276f1e986bf3", - code: 405, - }, -} - func TestProxy(t *testing.T) { dummySilence := `{ "comment": "comment", @@ -105,40 +51,127 @@ func TestProxy(t *testing.T) { config.Config.Listen.Prefix = "" - r := testRouter() - am, err := alertmanager.NewAlertmanager( - "cluster", - "dummy with (space)", - "http://localhost:9093", - alertmanager.WithRequestTimeout(time.Second*5), - alertmanager.WithProxy(true), - ) - if err != nil { - t.Error(err) + type proxyTest struct { + amName string + amCluster string + amURI string + method string + localPath string + upstreamURI string + code int + response string } - setupRouterProxyHandlers(r, am) - - httpmock.Activate() - defer httpmock.DeactivateAndReset() + proxyTests := []proxyTest{ + // valid alertmanager and methods + { + amCluster: "cluster", + amName: "dummy with (space)", + amURI: "http://localhost:9093", + method: "POST", + localPath: "/proxy/alertmanager/dummy%20with%20%28space%29/api/v2/silences", + upstreamURI: "http://localhost:9093/api/v2/silences", + code: 200, + response: "{\"status\":\"success\",\"data\":{\"silenceId\":\"d8a61ca8-ee2e-4076-999f-276f1e986bf3\"}}", + }, + { + amCluster: "cluster", + amName: "ha/1", + amURI: "http://localhost:9093", + method: "POST", + localPath: "/proxy/alertmanager/ha%2F1/api/v2/silences", + upstreamURI: "http://localhost:9093/api/v2/silences", + code: 200, + response: "{\"status\":\"success\",\"data\":{\"silenceId\":\"d8a61ca8-ee2e-4076-999f-276f1e986bf3\"}}", + }, + { + amCluster: "cluster", + amName: "dummy with (space)", + amURI: "http://localhost:9093", + method: "DELETE", + localPath: "/proxy/alertmanager/dummy%20with%20%28space%29/api/v2/silence/d8a61ca8-ee2e-4076-999f-276f1e986bf3", + upstreamURI: "http://localhost:9093/api/v2/silence/d8a61ca8-ee2e-4076-999f-276f1e986bf3", + code: 200, + response: "{\"status\":\"success\"}", + }, + // invalid alertmanager name + { + amCluster: "cluster", + amName: "dummy with (space)", + amURI: "http://localhost:9093", + method: "POST", + localPath: "/proxy/alertmanager/INVALID/api/v2/silences", + upstreamURI: "", + code: 404, + response: "404 page not found\n", + }, + { + amCluster: "cluster", + amName: "dummy with (space)", + amURI: "http://localhost:9093", + method: "DELETE", + localPath: "/proxy/alertmanager/INVALID/api/v2/silence/d8a61ca8-ee2e-4076-999f-276f1e986bf3", + upstreamURI: "http://localhost:9093/api/v2/silence/d8a61ca8-ee2e-4076-999f-276f1e986bf3", + code: 404, + response: "404 page not found\n", + }, + // valid alertmanager name, but invalid method + { + amCluster: "cluster", + amName: "dummy with (space)", + amURI: "http://localhost:9093", + method: "GET", + localPath: "/proxy/alertmanager/dummy%20with%20%28space%29/api/v2/silences", + upstreamURI: "", + code: 405, + }, + { + amCluster: "cluster", + amName: "dummy with (space)", + amURI: "http://localhost:9093", + method: "GET", + localPath: "/proxy/alertmanager/dummy%20with%20%28space%29/api/v2/silence/d8a61ca8-ee2e-4076-999f-276f1e986bf3", + upstreamURI: "http://localhost:9093/api/v2/silence/d8a61ca8-ee2e-4076-999f-276f1e986bf3", + code: 405, + }, + } for _, testCase := range proxyTests { - httpmock.Reset() - if testCase.upstreamURI != "" { - httpmock.RegisterResponder(testCase.method, testCase.upstreamURI, httpmock.NewStringResponder(testCase.code, testCase.response)) - } - req := httptest.NewRequest(testCase.method, testCase.localPath, strings.NewReader(dummySilence)) - 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) - } + t.Run(testCase.amName, func(t *testing.T) { + httpmock.Activate() + defer httpmock.DeactivateAndReset() + + am, err := alertmanager.NewAlertmanager( + testCase.amCluster, + testCase.amName, + testCase.amURI, + alertmanager.WithRequestTimeout(time.Second*5), + alertmanager.WithProxy(true), + ) + if err != nil { + t.Error(err) + } + t.Logf("InternalURI: %s", am.InternalURI()) + + r := testRouter() + setupRouterProxyHandlers(r, am) + + if testCase.upstreamURI != "" { + httpmock.RegisterResponder(testCase.method, testCase.upstreamURI, httpmock.NewStringResponder(testCase.code, testCase.response)) + } + req := httptest.NewRequest(testCase.method, testCase.localPath, strings.NewReader(dummySilence)) + 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) + } + }) } } diff --git a/cmd/karma/views_test.go b/cmd/karma/views_test.go index 900b17f23..cb1ea8604 100644 --- a/cmd/karma/views_test.go +++ b/cmd/karma/views_test.go @@ -62,6 +62,7 @@ func mockConfig(setenv setenvFunc) { func testRouter() *chi.Mux { router := chi.NewRouter() + router.Use(proxyPathFixMiddleware) err := loadTemplates() if err != nil {