Fix subtle bug in JSON/YAML encoding of inhibition rules

[grobinson-grafana]

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.

Before #4177:

b, err := json.Marshal(InhibitRule{
    Equals: model.LabelNames{"foo"},
})

{
    "equals": ["foo"]
}

After #4177:

b, err := json.Marshal(InhibitRule{
    Equals: model.LabelNames{"foo"},
})

{}

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.

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.

This still keeps the fix for #4177, but does so with an explicit breaking change to downstream users.

[grobinson-grafana]

@SuperQ as reviewer of #4177, would you also be able to review and merge this?

[grobinson-grafana]

I have not observed or seen this bug in practice, but it's something I reconsidered when updating Mimir, and on reflection, I thought I should change.

[grobinson-grafana]

I also considered two other options where I used a private struct just for decoding YAML. The advantage here is that we get to keep using model.LabelName and also avoid EqualStr. However, in the end I decided against these as I felt it created too much space to introduce new bugs. I have included them here.

Option 1

I first considered a fix based on this:

diff --git a/config/config.go b/config/config.go
index b8e48413..1715b8fe 100644
--- a/config/config.go
+++ b/config/config.go
@@ -968,33 +968,40 @@ 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 model.LabelNames `yaml:"equal,omitempty" json:"equal,omitempty"`
 }

 // UnmarshalYAML implements the yaml.Unmarshaler interface for InhibitRule.
 func (r *InhibitRule) UnmarshalYAML(unmarshal func(interface{}) error) error {
        type plain InhibitRule
-       if err := unmarshal((*plain)(r)); err != nil {
+
+       // tmp allows us to validate the labels in Equals depending on whether UTF-8
+       // is enabled or disabled. It should be deleted hen Alertmanager is updated
+       // to use the validation modes in recent versions of prometheus/common.
+       tmp := struct {
+               plain `yaml:",inline"`
+               Equal []string `yaml:"equal,omitempty" json:"equal,omitempty"`
+       }{}
+
+       if err := unmarshal(&tmp); err != nil {
                return err
        }

However, this causes issues with gopkg.in/yaml.v2 as it doesn't recognize Equal overrides plain.Equal, and instead panics due to duplicates:

plain `yaml:",inline"`
Equal []string `yaml:"equal,omitempty" json:"equal,omitempty"`

panic: Duplicated key 'equal' in struct struct { config.plain "yaml:\",inline\""; Equal []string "yaml:\"equal,omitempty\" json:\"equal,omitempty\"" } [recovered]

Option 2

To avoid the issues with Option 1, I need to duplicate all of the fields like so:

diff --git a/config/config.go b/config/config.go
index b8e48413..f66484be 100644
--- a/config/config.go
+++ b/config/config.go
@@ -968,33 +968,43 @@ 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 model.LabelNames `yaml:"equal,omitempty" json:"equal,omitempty"`
 }

 // UnmarshalYAML implements the yaml.Unmarshaler interface for InhibitRule.
 func (r *InhibitRule) UnmarshalYAML(unmarshal func(interface{}) error) error {
        type plain InhibitRule
-       if err := unmarshal((*plain)(r)); err != nil {
+
+       // tmp allows us to validate the labels in Equals depending on whether UTF-8
+       // is enabled or disabled. It should be deleted hen Alertmanager is updated
+       // to use the validation modes in recent versions of prometheus/common.
+       tmp := struct {
+               SourceMatch map[string]string `yaml:"source_match,omitempty" json:"source_match,omitempty"`
+               SourceMatchRE MatchRegexps `yaml:"source_match_re,omitempty" json:"source_match_re,omitempty"`
+               SourceMatchers Matchers `yaml:"source_matchers,omitempty" json:"source_matchers,omitempty"`
+               TargetMatch map[string]string `yaml:"target_match,omitempty" json:"target_match,omitempty"`
+               TargetMatchRE MatchRegexps `yaml:"target_match_re,omitempty" json:"target_match_re,omitempty"`
+               TargetMatchers Matchers `yaml:"target_matchers,omitempty" json:"target_matchers,omitempty"`
+               Equal []string `yaml:"equal,omitempty" json:"equal,omitempty"`
+       }{}
+
+       if err := unmarshal(&tmp); err != nil {
                return err
        }

which works, but is a much larger change, and I feel more susceptible to error, as I still need to do the following at the end of UnmarshalYAML:

+       r.SourceMatch = tmp.SourceMatch
+       r.SourceMatchRE = tmp.SourceMatchRE
+       r.SourceMatchers = tmp.SourceMatchers
+       r.TargetMatch = tmp.TargetMatch
+       r.TargetMatchRE = tmp.TargetMatchRE
+       r.TargetMatchers = tmp.TargetMatchers

[SuperQ]

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

CC @bwplotka

[grobinson-grafana]

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.

[SuperQ]

Makes sense, thanks!

[SuperQ]

Makes sense, thanks!