Skip to content

api: check silence matching by string comparison in getSilences #2443

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
beorn7 merged 4 commits into from
Feb 11, 2021
Merged

api: check silence matching by string comparison in getSilences #2443

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f8c33ab
api: check silence matching by string comparison in getSilences
ajalab Dec 26, 2020
72ce7fd
api: add test for gettableSilenceMatchesFilterLabels
ajalab Dec 26, 2020
b9aae07
Check silence matching with Protobuf models
ajalab Feb 9, 2021
b5ddc5d
Add test cases for silences with negative matchers
ajalab Feb 9, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
34 changes: 26 additions & 8 deletions api/v2/api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -584,14 +584,14 @@ func (api *API) getSilencesHandler(params silence_ops.GetSilencesParams) middlew

sils := open_api_models.GettableSilences{}
for _, ps := range psils {
if !checkSilenceMatchesFilterLabels(ps, matchers) {
continue
}
silence, err := gettableSilenceFromProto(ps)
if err != nil {
level.Error(logger).Log("msg", "Failed to unmarshal silence from proto", "err", err)
return silence_ops.NewGetSilencesInternalServerError().WithPayload(err.Error())
}
if !gettableSilenceMatchesFilterLabels(silence, matchers) {
continue
}
sils = append(sils, &silence)
}

Expand Down Expand Up @@ -638,13 +638,31 @@ func sortSilences(sils open_api_models.GettableSilences) {
})
}

func gettableSilenceMatchesFilterLabels(s open_api_models.GettableSilence, matchers []*labels.Matcher) bool {
sms := make(map[string]string)
for _, m := range s.Matchers {
sms[*m.Name] = *m.Value
// checkSilenceMatchesFilterLabels returns true if
// a given silence matches a list of matchers.
// A silence matches a filter (list of matchers) if
// for all matchers in the filter, there exists a matcher in the silence
// such that their names, types, and values are equivalent.
func checkSilenceMatchesFilterLabels(s *silencepb.Silence, matchers []*labels.Matcher) bool {
for _, matcher := range matchers {
found := false
for _, m := range s.Matchers {
if matcher.Name == m.Name &&
(matcher.Type == labels.MatchEqual && m.Type == silencepb.Matcher_EQUAL ||
matcher.Type == labels.MatchRegexp && m.Type == silencepb.Matcher_REGEXP ||
matcher.Type == labels.MatchNotEqual && m.Type == silencepb.Matcher_NOT_EQUAL ||
matcher.Type == labels.MatchNotRegexp && m.Type == silencepb.Matcher_NOT_REGEXP) &&
matcher.Value == m.Pattern {
found = true
break
}
}
if !found {
return false
}
}

return matchFilterLabels(matchers, sms)
Copy link
Contributor

@hooten hooten Feb 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the new logic you have here were refactored into matchFilterLabels, would it also improve alert filtering (since matchFilterLabels seems to be used by that endpoint too)?

Copy link
Contributor Author

@ajalab ajalab Feb 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, I think it wouldn't. IMO we should apply different logic to filter alerts or silences.

For instance, suppose we'd like to filter alerts coming from *.example.com -- we can use regexp instance=~".*.example.com" for this purpose. In this case, we should rather apply the existing matchFilterLabels logic, which checks whether an alert label matches with the given matcher by using regexp.

On the other hand, unlike alerts, a label of silence may contain regex like instance=~".*.example.com" (silence all alerts from *.example.com). In this case, we should not use regexp to check whether the silence label matches with the given matcher, but apply a simple string comparison.

Copy link
Member

@beorn7 beorn7 Feb 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tend to agree with @ajalab .

return true
}

func (api *API) getSilenceHandler(params silence_ops.GetSilenceParams) middleware.Responder {
Expand Down
104 changes: 104 additions & 0 deletions api/v2/api_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import (
general_ops "github.com/prometheus/alertmanager/api/v2/restapi/operations/general"
"github.com/prometheus/alertmanager/config"
"github.com/prometheus/alertmanager/pkg/labels"
"github.com/prometheus/alertmanager/silence/silencepb"
"github.com/prometheus/alertmanager/types"
)

Expand Down Expand Up @@ -130,6 +131,109 @@ func TestGetSilencesHandler(t *testing.T) {
}
}

func createSilenceMatcher(name string, pattern string, matcherType silencepb.Matcher_Type) *silencepb.Matcher {
return &silencepb.Matcher{
Name: name,
Pattern: pattern,
Type: matcherType,
}
}

func createLabelMatcher(name string, value string, matchType labels.MatchType) *labels.Matcher {
matcher, _ := labels.NewMatcher(matchType, name, value)
return matcher
}

func TestCheckSilenceMatchesFilterLabels(t *testing.T) {
type test struct {
silenceMatchers []*silencepb.Matcher
filterMatchers []*labels.Matcher
expected bool
}

tests := []test{
{
[]*silencepb.Matcher{createSilenceMatcher("label", "value", silencepb.Matcher_EQUAL)},
[]*labels.Matcher{createLabelMatcher("label", "value", labels.MatchEqual)},
true,
},
{
[]*silencepb.Matcher{createSilenceMatcher("label", "value", silencepb.Matcher_EQUAL)},
[]*labels.Matcher{createLabelMatcher("label", "novalue", labels.MatchEqual)},
false,
},
{
[]*silencepb.Matcher{createSilenceMatcher("label", "(foo|bar)", silencepb.Matcher_REGEXP)},
[]*labels.Matcher{createLabelMatcher("label", "(foo|bar)", labels.MatchRegexp)},
true,
},
{
[]*silencepb.Matcher{createSilenceMatcher("label", "foo", silencepb.Matcher_REGEXP)},
[]*labels.Matcher{createLabelMatcher("label", "(foo|bar)", labels.MatchRegexp)},
false,
},

{
[]*silencepb.Matcher{createSilenceMatcher("label", "value", silencepb.Matcher_EQUAL)},
[]*labels.Matcher{createLabelMatcher("label", "value", labels.MatchRegexp)},
false,
},
{
[]*silencepb.Matcher{createSilenceMatcher("label", "value", silencepb.Matcher_REGEXP)},
[]*labels.Matcher{createLabelMatcher("label", "value", labels.MatchEqual)},
false,
},
{
[]*silencepb.Matcher{createSilenceMatcher("label", "value", silencepb.Matcher_NOT_EQUAL)},
[]*labels.Matcher{createLabelMatcher("label", "value", labels.MatchNotEqual)},
true,
},
{
[]*silencepb.Matcher{createSilenceMatcher("label", "value", silencepb.Matcher_NOT_REGEXP)},
[]*labels.Matcher{createLabelMatcher("label", "value", labels.MatchNotRegexp)},
true,
},
{
[]*silencepb.Matcher{createSilenceMatcher("label", "value", silencepb.Matcher_EQUAL)},
[]*labels.Matcher{createLabelMatcher("label", "value", labels.MatchNotEqual)},
false,
},
{
[]*silencepb.Matcher{createSilenceMatcher("label", "value", silencepb.Matcher_REGEXP)},
[]*labels.Matcher{createLabelMatcher("label", "value", labels.MatchNotRegexp)},
false,
},
{
[]*silencepb.Matcher{createSilenceMatcher("label", "value", silencepb.Matcher_NOT_EQUAL)},
[]*labels.Matcher{createLabelMatcher("label", "value", labels.MatchNotRegexp)},
false,
},
{
[]*silencepb.Matcher{createSilenceMatcher("label", "value", silencepb.Matcher_NOT_REGEXP)},
[]*labels.Matcher{createLabelMatcher("label", "value", labels.MatchNotEqual)},
false,
},
{
[]*silencepb.Matcher{
createSilenceMatcher("label", "(foo|bar)", silencepb.Matcher_REGEXP),
createSilenceMatcher("label", "value", silencepb.Matcher_EQUAL),
},
[]*labels.Matcher{createLabelMatcher("label", "(foo|bar)", labels.MatchRegexp)},
true,
},
}

for _, test := range tests {
silence := silencepb.Silence{
Matchers: test.silenceMatchers,
}
actual := checkSilenceMatchesFilterLabels(&silence, test.filterMatchers)
if test.expected != actual {
t.Fatal("unexpected match result between silence and filter. expected:", test.expected, ", actual:", actual)
}
}
}

func convertDateTime(ts time.Time) *strfmt.DateTime {
dt := strfmt.DateTime(ts)
return &dt
Expand Down