From 34115a78a3400a856021e995a70715f9703f25cb Mon Sep 17 00:00:00 2001 From: George Robinson Date: Wed, 5 Mar 2025 02:10:04 +0000 Subject: [PATCH] Fix subtle bug in JSON/YAML encoding of inhibition rules This commit fixes a subtle bug introduced in #4177 where external code that imports config.InhibitRule for the purpose of JSON/YAML encoding can encode an inhibition rule with missing equals labels. This bug will happen if someone updates their Go modules, writes labels to the Equal field, and does not also write the same labels to the EqualStr field. To fix this, I propose removing EqualStr and changing Equal from model.LabelNames to []string. This will cause an intentional breaking change, and require the maintainer of downstream code to use []string instead of model.LabelNames. I consider this a better option to the subtle bug explained earlier. This still keeps the fix for #4177, but does so with an explicit breaking change to downstream users. Signed-off-by: George Robinson --- config/config.go | 9 ++------- config/config_test.go | 6 +++--- inhibit/inhibit.go | 2 +- inhibit/inhibit_test.go | 10 +++++----- 4 files changed, 11 insertions(+), 16 deletions(-) diff --git a/config/config.go b/config/config.go index b8e484130a..6f54c52368 100644 --- a/config/config.go +++ b/config/config.go @@ -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. @@ -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 diff --git a/config/config_test.go b/config/config_test.go index 99984827d5..728d7670b1 100644 --- a/config/config_test.go +++ b/config/config_test.go @@ -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) // Should not be able to load configuration with UTF-8 in equals list. _, err = LoadFile("testdata/conf.inhibit-equal-utf8.yml") @@ -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") @@ -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) } diff --git a/inhibit/inhibit.go b/inhibit/inhibit.go index eb1f7752ea..458367cb0b 100644 --- a/inhibit/inhibit.go +++ b/inhibit/inhibit.go @@ -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{ diff --git a/inhibit/inhibit_test.go b/inhibit/inhibit_test.go index 1f6f4ef250..59ff52b195 100644 --- a/inhibit/inhibit_test.go +++ b/inhibit/inhibit_test.go @@ -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()) @@ -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()) @@ -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.