api: check silence matching by string comparison in getSilences

[ajalab]

Fixes #2283.

This PR changes how gettableSilenceMatchesFilterLabels in api/v2 checks whether a silence matches a given filter, especially for regex cases. In short,

• before: a silence matches a filter if a pattern value in the silence matches pattern strings of the filter by regex.
• after: a silence matches a filter if a pattern value in the silence and pattern strings of the filter are equivalent.

For instance, as shown in figure #2283, a silence alertname~=(foo|bar) didn't match a filter of the same pattern alertname~=(foo|bar), since the regex match fails. After the fix, the silence and filter will match as we use string comparison instead.

[hooten]

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)?

[ajalab]

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.

[beorn7]

I tend to agree with @ajalab .

[beorn7]

I like it.

@ajalab by now, we support silences with negative matchers, at least in the "backend". Could you check what updates need to be done in your PR to deal with them correctly?

[ajalab]

Sure, I'll work on it!

[ajalab]

Currently, the OpenAPI model for silence doesn't support negative matchers, while the Protobuf model does:

OpenAPI

alertmanager/api/v2/models/matcher.go

Lines 32 to 45 in e6a1bed

	type Matcher struct {
	// is regex
	// Required: true
	IsRegex *bool `json:"isRegex"`
	// name
	// Required: true
	Name *string `json:"name"`
	// value
	// Required: true
	Value *string `json:"value"`
	}

Protobuf

alertmanager/silence/silencepb/silence.pb.go

Lines 65 to 75 in e6a1bed

	type Matcher struct {
	Type Matcher_Type `protobuf:"varint,1,opt,name=type,proto3,enum=silencepb.Matcher_Type" json:"type,omitempty"`
	// The label name in a label set to against which the matcher
	// checks the pattern.
	Name string `protobuf:"bytes,2,opt,name=name,proto3" json:"name,omitempty"`
	// The pattern being checked according to the matcher's type.
	Pattern string `protobuf:"bytes,3,opt,name=pattern,proto3" json:"pattern,omitempty"`
	XXX_NoUnkeyedLiteral struct{} `json:"-"`
	XXX_unrecognized []byte `json:"-"`
	XXX_sizecache int32 `json:"-"`
	}

alertmanager/silence/silencepb/silence.pb.go

Lines 35 to 40 in e6a1bed

	const (
	Matcher_EQUAL Matcher_Type = 0
	Matcher_REGEXP Matcher_Type = 1
	Matcher_NOT_EQUAL Matcher_Type = 2
	Matcher_NOT_REGEXP Matcher_Type = 3
	)

getSilences API checks matching based on OpenAPI models (gettableSilenceMatchesFilterLabels). So updates would be either

1. Update gettableSilenceMatchesFilterLabels to check matching based on Protobuf models, instead of OpenAPI
2. Fix Matcher OpenAPI model so that it supports negation

We may have to work on 2 at some point, but that needs more consideration. I believe the first solution should be reasonable and has less impact for now.

[ajalab]

Update gettableSilenceMatchesFilterLabels to check matching based on Protobuf models, instead of OpenAPI

I tried updating the PR in this direction. Now silences with negative matchers will start working immediately after OpenAPI models and clients/frontends are updated. Could you take a look again? @beorn7

[beorn7]

The changes for alertmanager/api/v2/models/matcher.go are in #2471 (which is not yet merged).

[beorn7]

The test failure might be due to flakiness. I have rerun the test.

(The test_frontend is known to be broken at the moment.)

[beorn7]

OK, now we are down to only the expected test_frontend error.

[beorn7]

I think we should go with this, but I'd much prefer if @simonpasquier could also chime in. I'll leave this open for another day or so to give him the opportunity.

[beorn7]

I take silence as consent and merge this now.