From 9ebc1b506cfca634aec4228ff3dddacc659f5104 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C5=81ukasz=20Mierzwa?= Date: Wed, 9 Sep 2020 15:08:10 +0100 Subject: [PATCH] fix(backend): don't raise false-positive alerts for alertmanagers with no cluster name --- cmd/karma/views_test.go | 333 +++++++++++++++++++++++++++- internal/alertmanager/cluster.go | 2 +- internal/alertmanager/dedup_test.go | 4 +- internal/alertmanager/models.go | 20 +- 4 files changed, 343 insertions(+), 16 deletions(-) diff --git a/cmd/karma/views_test.go b/cmd/karma/views_test.go index 4395b4f8c..4a728bd0f 100644 --- a/cmd/karma/views_test.go +++ b/cmd/karma/views_test.go @@ -1488,7 +1488,7 @@ func TestUpstreamStatus(t *testing.T) { }, }, { - Name: "Broken Cluster Without Name", + Name: "Split Cluster Without Name", mocks: []mockT{ { uri: "http://ha1.example.com/metrics", @@ -1597,7 +1597,7 @@ func TestUpstreamStatus(t *testing.T) { ReadOnly: false, Headers: map[string]string{}, CORSCredentials: "omit", - Error: "missing cluster peers: ha2", + Error: "", Version: "0.20.0", Cluster: "ha1", ClusterMembers: []string{"ha1"}, @@ -1609,7 +1609,7 @@ func TestUpstreamStatus(t *testing.T) { ReadOnly: true, Headers: map[string]string{}, CORSCredentials: "omit", - Error: "missing cluster peers: ha1", + Error: "", Version: "0.19.0", Cluster: "ha2", ClusterMembers: []string{"ha2"}, @@ -1621,6 +1621,147 @@ func TestUpstreamStatus(t *testing.T) { }, }, }, + { + Name: "Broken Cluster Without Name", + mocks: []mockT{ + { + uri: "http://broken1.example.com/metrics", + code: 200, + body: `alertmanager_build_info{version="0.20.0"} 1`, + }, + { + uri: "http://broken2.example.com/metrics", + code: 200, + body: `alertmanager_build_info{version="0.20.0"} 1`, + }, + { + uri: "http://broken1.example.com/api/v2/status", + code: 200, + body: `{ + "cluster": { + "name": "AAAAAAAAAAAAAAAAAAAAAAAAAA", + "peers": [ + { + "address": "10.16.0.1:9094", + "name": "AAAAAAAAAAAAAAAAAAAAAAAAAA" + }, + { + "address": "10.16.0.2:9094", + "name": "BBBBBBBBBBBBBBBBBBBBBBBBBB" + } + ], + "status": "ready" + }, + "versionInfo": { + "version":"0.20.0" + } +}`, + }, + { + uri: "http://broken2.example.com/api/v2/status", + code: 200, + body: `{ + "cluster": { + "name": "BBBBBBBBBBBBBBBBBBBBBBBBBB", + "peers": [ + { + "address": "10.16.0.1:9094", + "name": "AAAAAAAAAAAAAAAAAAAAAAAAAA" + }, + { + "address": "10.16.0.2:9094", + "name": "BBBBBBBBBBBBBBBBBBBBBBBBBB" + } + ], + "status": "ready" + }, + "versionInfo": { + "version":"0.20.0" + } +}`, + }, + { + uri: "http://broken1.example.com/api/v2/alerts/groups", + code: 200, + body: "[]", + }, + { + uri: "http://broken1.example.com/api/v2/silences", + code: 200, + body: "[]", + }, + { + uri: "http://broken2.example.com/api/v2/alerts/groups", + code: 500, + body: "Internal Error\n", + }, + { + uri: "http://broken2.example.com/api/v2/silences", + code: 500, + body: "Internal Error\n", + }, + }, + upstreams: []config.AlertmanagerConfig{ + { + Name: "broken1", + URI: "http://broken1.example.com", + Proxy: false, + ReadOnly: false, + Headers: map[string]string{}, + CORS: config.AlertmanagerCORS{ + Credentials: "omit", + }, + Timeout: time.Second * 10, + }, + { + Name: "broken2", + URI: "http://broken2.example.com", + Proxy: false, + ReadOnly: true, + Headers: map[string]string{}, + CORS: config.AlertmanagerCORS{ + Credentials: "omit", + }, + Timeout: time.Second * 10, + }, + }, + status: models.AlertmanagerAPISummary{ + Counters: models.AlertmanagerAPICounters{ + Total: 2, + Healthy: 1, + Failed: 1, + }, + Instances: []models.AlertmanagerAPIStatus{ + { + Name: "broken1", + URI: "http://broken1.example.com", + PublicURI: "http://broken1.example.com", + ReadOnly: false, + Headers: map[string]string{}, + CORSCredentials: "omit", + Error: "", + Version: "0.20.0", + Cluster: "broken1 | broken2", + ClusterMembers: []string{"broken1", "broken2"}, + }, + { + Name: "broken2", + URI: "http://broken2.example.com", + PublicURI: "http://broken2.example.com", + ReadOnly: true, + Headers: map[string]string{}, + CORSCredentials: "omit", + Error: "invalid character 'I' looking for beginning of value", + Version: "0.20.0", + Cluster: "broken1 | broken2", + ClusterMembers: []string{"broken1", "broken2"}, + }, + }, + Clusters: map[string][]string{ + "broken1 | broken2": {"broken1", "broken2"}, + }, + }, + }, { Name: "Cluster with name and errors", mocks: []mockT{ @@ -1741,10 +1882,10 @@ func TestUpstreamStatus(t *testing.T) { ReadOnly: false, Headers: map[string]string{}, CORSCredentials: "omit", - Error: "missing cluster peers: ha2", + Error: "", Version: "0.20.0", Cluster: "Errors", - ClusterMembers: []string{"ha1"}, + ClusterMembers: []string{"ha1", "ha2"}, }, { Name: "ha2", @@ -1754,13 +1895,189 @@ func TestUpstreamStatus(t *testing.T) { Headers: map[string]string{}, CORSCredentials: "omit", Error: "json: cannot unmarshal array into Go value of type string", - Version: "", + Version: "0.19.0", Cluster: "Errors", - ClusterMembers: []string{"ha2"}, + ClusterMembers: []string{"ha1", "ha2"}, }, }, Clusters: map[string][]string{ - "Errors": {"ha1"}, + "Errors": {"ha1", "ha2"}, + }, + }, + }, + { + Name: "Single alertmanager from HA Cluster Without Name", + mocks: []mockT{ + { + uri: "http://ha1.example.com/metrics", + code: 200, + body: `alertmanager_build_info{version="0.20.0"} 1`, + }, + { + uri: "http://ha2.example.com/metrics", + code: 200, + body: `alertmanager_build_info{version="0.19.0"} 1`, + }, + { + uri: "http://single.example.com/metrics", + code: 200, + body: `alertmanager_build_info{version="0.21.0"} 1`, + }, + { + uri: "http://ha1.example.com/api/v2/status", + code: 200, + body: `{ + "cluster": { + "name": "AAAAAAAAAAAAAAAAAAAAAAAAAA", + "peers": [ + { + "address": "10.16.0.1:9094", + "name": "AAAAAAAAAAAAAAAAAAAAAAAAAA" + }, + { + "address": "10.16.0.2:9094", + "name": "BBBBBBBBBBBBBBBBBBBBBBBBBB" + } + ], + "status": "ready" + }, + "versionInfo": { + "version":"0.20.0" + } +}`, + }, + { + uri: "http://ha2.example.com/api/v2/status", + code: 200, + body: `{ + "cluster": { + "name": "BBBBBBBBBBBBBBBBBBBBBBBBBB", + "peers": [ + { + "address": "10.16.0.1:9094", + "name": "AAAAAAAAAAAAAAAAAAAAAAAAAA" + }, + { + "address": "10.16.0.2:9094", + "name": "BBBBBBBBBBBBBBBBBBBBBBBBBB" + } + ], + "status": "ready" + }, + "versionInfo": { + "version":"0.19.0" + } +}`, + }, + { + uri: "http://single.example.com/api/v2/status", + code: 200, + body: `{ + "cluster": { + "name": "CCCCCCCCCCCCCCCCCCCCCCCCCC", + "peers": [ + { + "address": "10.16.0.3:9094", + "name": "CCCCCCCCCCCCCCCCCCCCCCCCCC" + } + ], + "status": "ready" + }, + "versionInfo": { + "version":"0.21.0" + } +}`, + }, + { + uri: "http://ha1.example.com/api/v2/alerts/groups", + code: 200, + body: "[]", + }, + { + uri: "http://ha1.example.com/api/v2/silences", + code: 200, + body: "[]", + }, + { + uri: "http://ha2.example.com/api/v2/alerts/groups", + code: 200, + body: "[]", + }, + { + uri: "http://ha2.example.com/api/v2/silences", + code: 200, + body: "[]", + }, + { + uri: "http://single.example.com/api/v2/alerts/groups", + code: 200, + body: "[]", + }, + { + uri: "http://single.example.com/api/v2/silences", + code: 200, + body: "[]", + }, + }, + upstreams: []config.AlertmanagerConfig{ + { + Name: "ha1", + URI: "http://ha1.example.com", + Proxy: false, + ReadOnly: false, + Headers: map[string]string{}, + CORS: config.AlertmanagerCORS{ + Credentials: "same-site", + }, + Timeout: time.Second * 10, + }, + { + Name: "single", + URI: "http://single.example.com", + Proxy: false, + ReadOnly: true, + Headers: map[string]string{}, + CORS: config.AlertmanagerCORS{ + Credentials: "same-site", + }, + Timeout: time.Second * 10, + }, + }, + status: models.AlertmanagerAPISummary{ + Counters: models.AlertmanagerAPICounters{ + Total: 2, + Healthy: 2, + Failed: 0, + }, + Instances: []models.AlertmanagerAPIStatus{ + { + Name: "ha1", + URI: "http://ha1.example.com", + PublicURI: "http://ha1.example.com", + ReadOnly: false, + Headers: map[string]string{}, + CORSCredentials: "same-site", + Error: "", + Version: "0.20.0", + Cluster: "ha1", + ClusterMembers: []string{"ha1"}, + }, + { + Name: "single", + URI: "http://single.example.com", + PublicURI: "http://single.example.com", + ReadOnly: true, + Headers: map[string]string{}, + CORSCredentials: "same-site", + Error: "", + Version: "0.21.0", + Cluster: "single", + ClusterMembers: []string{"single"}, + }, + }, + Clusters: map[string][]string{ + "ha1": {"ha1"}, + "single": {"single"}, }, }, }, diff --git a/internal/alertmanager/cluster.go b/internal/alertmanager/cluster.go index ba314be7e..26292c244 100644 --- a/internal/alertmanager/cluster.go +++ b/internal/alertmanager/cluster.go @@ -5,7 +5,7 @@ func clusterMembersFromConfig(am *Alertmanager) []string { upstreams := GetAlertmanagers() for _, upstream := range upstreams { - if upstream.Cluster == am.Cluster { + if upstream.ClusterName() == am.ClusterName() { members = append(members, upstream.Name) } } diff --git a/internal/alertmanager/dedup_test.go b/internal/alertmanager/dedup_test.go index b0bd0c5af..a31b0c57c 100644 --- a/internal/alertmanager/dedup_test.go +++ b/internal/alertmanager/dedup_test.go @@ -210,8 +210,8 @@ func TestClearData(t *testing.T) { mock.RegisterURL(fmt.Sprintf("%s/api/v2/status", uri), version, "api/v2/status") mock.RegisterURL(fmt.Sprintf("%s/api/v2/silences", uri), version, "api/v2/silences") _ = am.Pull() - if am.Version() != "" { - t.Errorf("[%s] Got non-empty version string: %s", am.Name, am.Version()) + if am.Version() == "" { + t.Errorf("[%s] Got empty version string: %s", am.Name, am.Version()) } if am.Error() == "" { t.Errorf("[%s] Got empty error string", am.Name) diff --git a/internal/alertmanager/models.go b/internal/alertmanager/models.go index 0b892bcc1..03fa23413 100644 --- a/internal/alertmanager/models.go +++ b/internal/alertmanager/models.go @@ -114,6 +114,11 @@ func (am *Alertmanager) clearData() { am.colors = models.LabelsColorMap{} am.autocomplete = []models.Autocomplete{} am.knownLabels = []string{} + am.lock.Unlock() +} + +func (am *Alertmanager) clearStatus() { + am.lock.Lock() am.status = models.AlertmanagerStatus{ Version: "", ID: "", @@ -314,10 +319,14 @@ func (am *Alertmanager) Pull() error { status, err := am.fetchStatus(version) if err != nil { am.clearData() + am.clearStatus() am.setError(err.Error()) am.Metrics.Errors[labelValueErrorsSilences]++ return err } + am.lock.Lock() + am.status = *status + am.lock.Unlock() err = am.pullSilences(version) if err != nil { @@ -336,7 +345,6 @@ func (am *Alertmanager) Pull() error { } am.lock.Lock() - am.status = *status am.lastError = "" am.clusterName = "" am.lock.Unlock() @@ -434,6 +442,7 @@ func (am *Alertmanager) Error() string { missing, _ := slices.StringSliceDiff(configPeers, apiPeers) if len(missing) > 0 { + log.Debugf("[%s] cluster peers mismatch, configured: %v, api: %v, missing: %v\n", am.Name, configPeers, apiPeers, missing) return fmt.Sprintf("missing cluster peers: %s", strings.Join(missing, ", ")) } @@ -475,11 +484,12 @@ func (am *Alertmanager) ClusterMemberNames() []string { upstreams := GetAlertmanagers() for _, upstream := range upstreams { - if upstream.Name == am.Name { - continue - } for _, peerID := range upstream.ClusterPeers() { - if slices.StringInSlice(am.status.PeerIDs, peerID) { + // IF + // other alertmanagers peerID is in this alertmanager cluster status + // OR + // this alertmanager peerID is in other alertmanagers cluster status + if slices.StringInSlice(am.status.PeerIDs, peerID) || peerID == am.status.ID { if !slices.StringInSlice(members, upstream.Name) { members = append(members, upstream.Name) }