Skip to content

Fix subtle bug in JSON/YAML encoding of inhibition rules #4292

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
SuperQ merged 1 commit into from
Mar 5, 2025
Merged

Fix subtle bug in JSON/YAML encoding of inhibition rules #4292

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
9 changes: 2 additions & 7 deletions config/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -968,11 +968,7 @@ type InhibitRule struct {
TargetMatchers Matchers `yaml:"target_matchers,omitempty" json:"target_matchers,omitempty"`
// A set of labels that must be equal between the source and target alert
// for them to be a match.
Equal model.LabelNames `yaml:"-" json:"-"`
// EqualStr allows us to validate the label depending on whether UTF-8 is
// enabled or disabled. It should be removed when Alertmanager is updated
// to use the validation modes in recent versions of prometheus/common.
EqualStr []string `yaml:"equal,omitempty" json:"equal,omitempty"`
Equal []string `yaml:"equal,omitempty" json:"equal,omitempty"`
}

// UnmarshalYAML implements the yaml.Unmarshaler interface for InhibitRule.
Expand All @@ -994,12 +990,11 @@ func (r *InhibitRule) UnmarshalYAML(unmarshal func(interface{}) error) error {
}
}

for _, l := range r.EqualStr {
for _, l := range r.Equal {
labelName := model.LabelName(l)
if !compat.IsValidLabelName(labelName) {
return fmt.Errorf("invalid label name %q in equal list", l)
}
r.Equal = append(r.Equal, labelName)
}

return nil
Expand Down
6 changes: 3 additions & 3 deletions config/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1461,7 +1461,7 @@ func TestInhibitRuleEqual(t *testing.T) {
// The inhibition rule should have the expected equal labels.
require.Len(t, c.InhibitRules, 1)
r := c.InhibitRules[0]
require.Equal(t, model.LabelNames{"qux", "corge"}, r.Equal)
require.Equal(t, []string{"qux", "corge"}, r.Equal)
Copy link
Member

@SuperQ SuperQ Mar 5, 2025

Choose a reason for hiding this comment

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

Hmm, I thought there was some new changes to provide better UTF-8 support.

CC @bwplotka

Copy link
Collaborator Author

@grobinson-grafana grobinson-grafana Mar 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

There are, but...

The reason we change model.LabelNames to []string is prometheus/common cannot support two modes at the same time (UTF-8 enabled and UTF-8 disabled). This is a blocking issue for projects like Mimir where UTF-8 development is at different stages in the Alertmanager and metrics codebases, and so Mimir depends on a global variable in prometheus/common due to a diamond dependency problem. This change avoids that dependency problem.

tl;dr is Mimir imports prometheus/common via prometheus/alertmanager and also via prometheus/prometheus. Alertmanager and Prometheus needs to set different values for model.NameValidationScheme at the same time, in the same program. Which of course isn't possible as it's a shared global variable. Instead, in Alertmanager we added our own compat package that duplicates the logic from promethues/common to specifically allow Alertmanager and Prometheus to have different levels of UTF-8 support at the same time when linked to the same program.

The problem is here we forgot to use it for inhibition rules equals fields. That's what #4177 solved, but it also introduced this bug. This PR fixes that bug.

Copy link
Member

@SuperQ SuperQ Mar 5, 2025

Choose a reason for hiding this comment

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

Makes sense, thanks!


// Should not be able to load configuration with UTF-8 in equals list.
_, err = LoadFile("testdata/conf.inhibit-equal-utf8.yml")
Expand All @@ -1484,7 +1484,7 @@ func TestInhibitRuleEqual(t *testing.T) {
// The inhibition rule should have the expected equal labels.
require.Len(t, c.InhibitRules, 1)
r = c.InhibitRules[0]
require.Equal(t, model.LabelNames{"qux", "corge"}, r.Equal)
require.Equal(t, []string{"qux", "corge"}, r.Equal)

// Should also be able to load configuration with UTF-8 in equals list.
c, err = LoadFile("testdata/conf.inhibit-equal-utf8.yml")
Expand All @@ -1493,5 +1493,5 @@ func TestInhibitRuleEqual(t *testing.T) {
// The inhibition rule should have the expected equal labels.
require.Len(t, c.InhibitRules, 1)
r = c.InhibitRules[0]
require.Equal(t, model.LabelNames{"qux🙂", "corge"}, r.Equal)
require.Equal(t, []string{"qux🙂", "corge"}, r.Equal)
}
2 changes: 1 addition & 1 deletion inhibit/inhibit.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,7 @@ func NewInhibitRule(cr config.InhibitRule) *InhibitRule {

equal := map[model.LabelName]struct{}{}
for _, ln := range cr.Equal {
equal[ln] = struct{}{}
equal[model.LabelName(ln)] = struct{}{}
}

return &InhibitRule{
Expand Down
10 changes: 5 additions & 5 deletions inhibit/inhibit_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -144,12 +144,12 @@ func TestInhibitRuleMatches(t *testing.T) {
rule1 := config.InhibitRule{
SourceMatch: map[string]string{"s1": "1"},
TargetMatch: map[string]string{"t1": "1"},
Equal: model.LabelNames{"e"},
Equal: []string{"e"},
}
rule2 := config.InhibitRule{
SourceMatch: map[string]string{"s2": "1"},
TargetMatch: map[string]string{"t2": "1"},
Equal: model.LabelNames{"e"},
Equal: []string{"e"},
}

m := types.NewMarker(prometheus.NewRegistry())
Expand Down Expand Up @@ -240,12 +240,12 @@ func TestInhibitRuleMatchers(t *testing.T) {
rule1 := config.InhibitRule{
SourceMatchers: config.Matchers{&labels.Matcher{Type: labels.MatchEqual, Name: "s1", Value: "1"}},
TargetMatchers: config.Matchers{&labels.Matcher{Type: labels.MatchNotEqual, Name: "t1", Value: "1"}},
Equal: model.LabelNames{"e"},
Equal: []string{"e"},
}
rule2 := config.InhibitRule{
SourceMatchers: config.Matchers{&labels.Matcher{Type: labels.MatchEqual, Name: "s2", Value: "1"}},
TargetMatchers: config.Matchers{&labels.Matcher{Type: labels.MatchEqual, Name: "t2", Value: "1"}},
Equal: model.LabelNames{"e"},
Equal: []string{"e"},
}

m := types.NewMarker(prometheus.NewRegistry())
Expand Down Expand Up @@ -374,7 +374,7 @@ func TestInhibit(t *testing.T) {
return config.InhibitRule{
SourceMatch: map[string]string{"s": "1"},
TargetMatch: map[string]string{"t": "1"},
Equal: model.LabelNames{"e"},
Equal: []string{"e"},
}
}
// alertOne is muted by alertTwo when it is active.
Expand Down
Loading