From e1b548fc936b873bf945e92a2f763df69b5d43f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Sat, 4 Apr 2020 19:08:12 +0100 Subject: [PATCH] fix(tests): add more test coverage --- cmd/karma/views.go | 8 +- cmd/karma/views_test.go | 4 +- internal/filters/filter.go | 27 +--- internal/filters/filter_test.go | 52 +++++++ internal/filters/invalid_match_test.go | 47 +++++++ internal/filters/matcher_test.go | 10 ++ internal/mapper/v017/alerts.go | 6 +- internal/mapper/v017/silences.go | 6 +- internal/mapper/v017/status.go | 6 +- internal/uri/uri_test.go | 183 +++++++++++++++++-------- 10 files changed, 252 insertions(+), 97 deletions(-) create mode 100644 internal/filters/invalid_match_test.go diff --git a/cmd/karma/views.go b/cmd/karma/views.go index 57e8e1b09..6236dd56e 100644 --- a/cmd/karma/views.go +++ b/cmd/karma/views.go @@ -38,12 +38,10 @@ func pong(c *gin.Context) { func compressResponse(data []byte) ([]byte, error) { var b bytes.Buffer - gz, err := gzip.NewWriterLevel(&b, 3) - if err != nil { - return nil, fmt.Errorf("failed to create new compression writer: %s", err.Error()) - } + // this only fails if we pass unsupported level (3 is valid) + gz, _ := gzip.NewWriterLevel(&b, 3) - _, err = gz.Write(data) + _, err := gz.Write(data) if err != nil { return nil, fmt.Errorf("failed to compress data: %s", err.Error()) } diff --git a/cmd/karma/views_test.go b/cmd/karma/views_test.go index 68fa6a983..9a145ed72 100644 --- a/cmd/karma/views_test.go +++ b/cmd/karma/views_test.go @@ -30,7 +30,7 @@ var upstreamSetup = false func mockConfig() { log.SetLevel(log.ErrorLevel) os.Setenv("ALERTMANAGER_URI", "http://localhost") - os.Setenv("LABELS_COLOR_UNIQUE", "alertname") + os.Setenv("LABELS_COLOR_UNIQUE", "alertname @receiver @alertmanager") f := pflag.NewFlagSet(".", pflag.ExitOnError) config.SetupFlags(f) @@ -142,7 +142,7 @@ func TestAlerts(t *testing.T) { if len(ur.Filters) != 3 { t.Errorf("[%s] Got %d filter(s) in response, expected %d", version, len(ur.Filters), 3) } - if len(ur.Colors) != 1 { + if len(ur.Colors) != 3 { t.Errorf("[%s] Got %d color(s) in response, expected %d", version, len(ur.Colors), 1) } if len(ur.Grids[0].AlertGroups) != 1 { diff --git a/internal/filters/filter.go b/internal/filters/filter.go index fbb21189a..aec5a5bac 100644 --- a/internal/filters/filter.go +++ b/internal/filters/filter.go @@ -93,12 +93,8 @@ func NewFilter(expression string) FilterT { if matched == "" && operator == "" && value == "" { // no "filter=" part, just the value, use fuzzy filter f := newFuzzyFilter() - matcher, err := newMatcher(regexpOperator) - if err != nil { - f.init("", nil, expression, false, expression) - } else { - f.init("", &matcher, expression, true, expression) - } + matcher, _ := newMatcher(regexpOperator) + f.init("", &matcher, expression, true, expression) return f } @@ -107,11 +103,6 @@ func NewFilter(expression string) FilterT { return &invalid } - if operator == "" { - // no operator, no valid filter here - return &invalid - } - // we have "filter=" part, lookup filter that matches for _, fc := range AllFilters { f := fc.Factory() @@ -122,16 +113,10 @@ func NewFilter(expression string) FilterT { if !slices.StringInSlice(fc.SupportedOperators, operator) { return &invalid } - matcher, err := newMatcher(operator) - if err != nil { - f.init(matched, nil, expression, false, "") - } else { - if value != "" { - f.init(matched, &matcher, expression, true, value) - return f - } - f.init(matched, &matcher, expression, false, "") - } + // we validate operator above, no need to re-check + matcher, _ := newMatcher(operator) + f.init(matched, &matcher, expression, true, value) + return f } return &invalid diff --git a/internal/filters/filter_test.go b/internal/filters/filter_test.go index 0005560b7..a74561965 100644 --- a/internal/filters/filter_test.go +++ b/internal/filters/filter_test.go @@ -83,6 +83,30 @@ var tests = []filterTest{ Alert: models.Alert{State: "suppressed", InhibitedBy: []string{"999"}}, IsMatch: false, }, + { + Expression: "@state==active", + IsValid: false, + }, + { + Expression: "@state!!active", + IsValid: false, + }, + { + Expression: "@state!!", + IsValid: false, + }, + { + Expression: "@state=", + IsValid: false, + }, + { + Expression: "@state==", + IsValid: false, + }, + { + Expression: "@state<=active", + IsValid: false, + }, { Expression: "@silence_id=abcdef", @@ -152,6 +176,13 @@ var tests = []filterTest{ Silence: models.Silence{ID: "1", TicketID: "1"}, IsMatch: true, }, + { + Expression: "@silence_ticket=1", + IsValid: true, + Alert: models.Alert{State: "active", SilencedBy: []string{}}, + Silence: models.Silence{ID: "1", TicketID: "1"}, + IsMatch: false, + }, { Expression: "@silence_ticket=2", IsValid: true, @@ -223,6 +254,13 @@ var tests = []filterTest{ Silence: models.Silence{ID: "1", CreatedBy: "john"}, IsMatch: true, }, + { + Expression: "@silence_author=john", + IsValid: true, + Alert: models.Alert{State: "active", SilencedBy: []string{}}, + Silence: models.Silence{ID: "1", CreatedBy: "john"}, + IsMatch: false, + }, { Expression: "@silence_author=john", IsValid: true, @@ -708,6 +746,20 @@ func TestLimitFilter(t *testing.T) { if f.GetHits() != ft.Hits { t.Errorf("[%s] GetHits() returned %#v hits, expected %d", ft.Expression, f.GetHits(), ft.Hits) } + } else { + func() { + didPanic := false + defer func() { + if r := recover(); r != nil { + didPanic = true + } + }() + alert := models.Alert{} + f.Match(&alert, 0) + if !didPanic { + t.Errorf("[%s] Match() on invalid filter didn't cause panic", ft.Expression) + } + }() } } } diff --git a/internal/filters/invalid_match_test.go b/internal/filters/invalid_match_test.go new file mode 100644 index 000000000..fb1f3f7b7 --- /dev/null +++ b/internal/filters/invalid_match_test.go @@ -0,0 +1,47 @@ +package filters + +import ( + "testing" + + "github.com/prymitive/karma/internal/models" +) + +func TestMatchOnInvalidFilter(t *testing.T) { + for _, ft := range AllFilters { + f := ft.Factory() + m, _ := newMatcher(f.GetMatcher()) + f.init(f.GetName(), &m, f.GetRawText(), false, f.GetValue()) + func() { + didPanic := false + defer func() { + if r := recover(); r != nil { + didPanic = true + } + }() + alert := models.Alert{} + f.Match(&alert, 0) + if !didPanic { + t.Errorf("[%s] Match() on invalid filter didn't cause panic", ft.Label) + } + }() + } +} + +func TestGetMatcherNil(t *testing.T) { + f := alertFilter{} + m := f.GetMatcher() + if m != "" { + t.Errorf("Got %q from empty filter GetMatcher()", m) + } +} + +func TestGetMatcherNotNil(t *testing.T) { + matcher, _ := newMatcher("=") + f := alertFilter{ + Matcher: matcher, + } + m := f.GetMatcher() + if m != "=" { + t.Errorf("Got %q from empty filter GetMatcher()", m) + } +} diff --git a/internal/filters/matcher_test.go b/internal/filters/matcher_test.go index 23bdc0f76..1d6c17b2a 100644 --- a/internal/filters/matcher_test.go +++ b/internal/filters/matcher_test.go @@ -151,5 +151,15 @@ func TestNewMatcher(t *testing.T) { t.Errorf("Got wrong matcher for %s: %s", operator, m.GetOperator()) } } +} +func TestInvalidMatcher(t *testing.T) { + operator := "<>" + m, err := newMatcher(operator) + if err == nil { + t.Errorf("NewMatcher(%s) didn't return any error: %s", operator, m) + } + if m != nil { + t.Errorf("NewMatcher(%s) returned non-nil value: %s", operator, m) + } } diff --git a/internal/mapper/v017/alerts.go b/internal/mapper/v017/alerts.go index 635a1d13c..f5b6c037e 100644 --- a/internal/mapper/v017/alerts.go +++ b/internal/mapper/v017/alerts.go @@ -17,10 +17,8 @@ type AlertMapper struct { // IsSupported returns true if given version string is supported func (m AlertMapper) IsSupported(version string) bool { - versionRange, err := semver.NewConstraint(">=0.17.0") - if err != nil { - panic(err) - } + // no need to check for errors as we pass static value + versionRange, _ := semver.NewConstraint(">=0.17.0") return versionRange.Check(semver.MustParse(version)) } diff --git a/internal/mapper/v017/silences.go b/internal/mapper/v017/silences.go index 8c9741824..eaffacc40 100644 --- a/internal/mapper/v017/silences.go +++ b/internal/mapper/v017/silences.go @@ -16,10 +16,8 @@ type SilenceMapper struct { // IsSupported returns true if given version string is supported func (m SilenceMapper) IsSupported(version string) bool { - versionRange, err := semver.NewConstraint(">=0.17.0") - if err != nil { - panic(err) - } + // no need to check for errors as we pass static value + versionRange, _ := semver.NewConstraint(">=0.17.0") return versionRange.Check(semver.MustParse(version)) } diff --git a/internal/mapper/v017/status.go b/internal/mapper/v017/status.go index 761d3fa20..a3b852fc5 100644 --- a/internal/mapper/v017/status.go +++ b/internal/mapper/v017/status.go @@ -16,10 +16,8 @@ type StatusMapper struct { // IsSupported returns true if given version string is supported func (s StatusMapper) IsSupported(version string) bool { - versionRange, err := semver.NewConstraint(">=0.17.0") - if err != nil { - panic(err) - } + // no need to check for errors as we pass static value + versionRange, _ := semver.NewConstraint(">=0.17.0") return versionRange.Check(semver.MustParse(version)) } diff --git a/internal/uri/uri_test.go b/internal/uri/uri_test.go index 8b948f867..30e77ab3d 100644 --- a/internal/uri/uri_test.go +++ b/internal/uri/uri_test.go @@ -1,9 +1,10 @@ package uri_test import ( + "bytes" + "compress/gzip" "crypto/tls" "crypto/x509" - "fmt" "io" "net/http" "net/http/httptest" @@ -16,30 +17,80 @@ import ( ) type httpTransportTest struct { - timeout time.Duration - tlsConfig *tls.Config - useTLS bool - failed bool - headers map[string]string + name string + timeout time.Duration + tlsConfig *tls.Config + useTLS bool + failed bool + headers map[string]string + responseCode int + responseBody []byte + responseContentLength int64 + responseHeaders map[string]string } var httpTransportTests = []httpTransportTest{ { - // plain HTTP request, should work + name: "plain HTTP request, should work", + responseCode: 200, + responseBody: []byte("1234"), + responseContentLength: 4, }, { - // just enable TLS, will use proper RootCA certs so it should work - useTLS: true, + name: "plain HTTP request, 404, should fail", + responseCode: 404, + failed: true, }, { - // use empty RootCA pool so we fail on verifying server certificate - useTLS: true, - tlsConfig: &tls.Config{RootCAs: x509.NewCertPool()}, - failed: true, + name: "gzipped HTTP response, should work", + responseCode: 200, + responseBody: gzipString("1234"), + responseContentLength: 4, + responseHeaders: map[string]string{ + "Content-Encoding": "gzip", + }, }, { - headers: map[string]string{"X-Auth-Test": "tokenValue"}, + name: "invalid gzipped HTTP response, should fail", + responseCode: 200, + responseBody: []byte("1234"), + responseContentLength: 4, + responseHeaders: map[string]string{ + "Content-Encoding": "gzip", + }, + failed: true, }, + { + name: "enable TLS, will use proper RootCA certs so it should work", + useTLS: true, + responseCode: 200, + responseBody: []byte("1234"), + responseContentLength: 4, + }, + { + name: "use empty RootCA pool so we fail on verifying server certificate", + useTLS: true, + tlsConfig: &tls.Config{RootCAs: x509.NewCertPool()}, + failed: true, + responseCode: 200, + responseBody: []byte("1234"), + responseContentLength: 4, + }, + { + name: "auth headers test", + headers: map[string]string{"X-Auth-Test": "tokenValue"}, + responseCode: 200, + responseBody: []byte("1234"), + responseContentLength: 4, + }, +} + +func gzipString(s string) []byte { + var b bytes.Buffer + gz := gzip.NewWriter(&b) + _, _ = gz.Write([]byte(s)) + _ = gz.Close() + return b.Bytes() } func readAll(source io.ReadCloser) (int64, error) { @@ -59,54 +110,72 @@ func readAll(source io.ReadCloser) (int64, error) { func TestHTTPReader(t *testing.T) { log.SetLevel(log.FatalLevel) - - responseBody := "1234" - handler := func(w http.ResponseWriter, r *http.Request) { - fmt.Fprintln(w, responseBody) - } - plainTS := httptest.NewServer(http.HandlerFunc(handler)) - defer plainTS.Close() - - tlsTS := httptest.NewTLSServer(http.HandlerFunc(handler)) - - defer tlsTS.Close() - caPool := x509.NewCertPool() - caPool.AddCert(tlsTS.Certificate()) - for _, testCase := range httpTransportTests { - var amURI string - if testCase.useTLS { - amURI = tlsTS.URL - } else { - amURI = plainTS.URL - } - - tlsConfig := testCase.tlsConfig - if tlsConfig == nil { - tlsConfig = &tls.Config{RootCAs: caPool} - } - - transp, err := uri.NewReader(amURI, testCase.timeout, &http.Transport{TLSClientConfig: tlsConfig}, testCase.headers) - if err != nil { - t.Errorf("[%v] failed to create new HTTP transport: %s", testCase, err) - } - - source, err := transp.Read(amURI, testCase.headers) - if err != nil { - if !testCase.failed { - t.Errorf("[%v] unexpected failure while creating reader: %s", testCase, err) + t.Run(testCase.name, func(t *testing.T) { + handler := func(w http.ResponseWriter, r *http.Request) { + for k, v := range testCase.responseHeaders { + w.Header().Set(k, v) + } + w.WriteHeader(testCase.responseCode) + _, _ = w.Write(testCase.responseBody) } - continue - } - got, err := readAll(source) - source.Close() - if err != nil { - t.Errorf("[%v] Read() failed: %s", testCase, err) + tlsConfig := testCase.tlsConfig + + var server *httptest.Server + if testCase.useTLS { + server = httptest.NewTLSServer(http.HandlerFunc(handler)) + if tlsConfig == nil { + caPool := x509.NewCertPool() + caPool.AddCert(server.Certificate()) + tlsConfig = &tls.Config{RootCAs: caPool} + } + } else { + server = httptest.NewServer(http.HandlerFunc(handler)) + } + defer server.Close() + + transp, err := uri.NewReader(server.URL, testCase.timeout, &http.Transport{TLSClientConfig: tlsConfig}, testCase.headers) + if err != nil { + t.Errorf("[%v] failed to create new HTTP transport: %s", testCase, err) + } + + source, err := transp.Read(server.URL, testCase.headers) + if err != nil { + if !testCase.failed { + t.Errorf("[%v] unexpected failure while creating reader: %s", testCase, err) + } + } else { + got, err := readAll(source) + source.Close() + + if err != nil { + t.Errorf("[%v] Read() failed: %s", testCase, err) + } + + if got != testCase.responseContentLength { + t.Errorf("[%v] Wrong response size, got %d, expected %d", testCase, got, testCase.responseContentLength) + } + } + }) + } +} + +func TestInvalidNewReaderURI(t *testing.T) { + tests := []string{ + "%gh&%ij", + "httpz://", + } + for _, testCase := range tests { + _, err := uri.NewReader(testCase, time.Second, nil, map[string]string{}) + if err == nil { + t.Errorf("uri.NewReader(%q) didn't trigger any error", testCase) } - if got != int64(len(responseBody)+1) { - t.Errorf("[%v] Wrong response size, got %d, expected %d", testCase, got, len(responseBody)) + r, _ := uri.NewReader("http://localhost", time.Second, nil, map[string]string{}) + _, err = r.Read(testCase, map[string]string{}) + if err == nil { + t.Errorf("Reader.Read(%q) didn't trigger any error", testCase) } } }