From fa6a7e6dd66dbb85ccbcfda156587e4b4e98fc19 Mon Sep 17 00:00:00 2001 From: George Robinson Date: Mon, 15 Jan 2024 10:03:51 +0000 Subject: [PATCH] Fix inconsistent defaults in UTF-8 behavior (#3668) This commit fixes inconsistent UTF-8 behavior if the compat package is not initialized and feature flags are not passed to the API. This can happen when Alertmanager is used as a package in software such as Cortex or Mimir. The inconsistent behavior is that Alertmanager will accept UTF-8 alerts but reject UTF-8 configurations. Since feature flags are optional via api.Options, we cannot force them to be passed to api.New at compile time. Instead, it's better to defer back to the compat package which is consistent even when not initialized. Signed-off-by: George Robinson --- api/api.go | 5 ----- api/v2/api.go | 9 +------- cmd/alertmanager/main.go | 19 ++++++++-------- silence/silence.go | 47 +++++----------------------------------- silence/silence_test.go | 23 +++++++++++++------- types/types.go | 28 +++++------------------- types/types_test.go | 16 +++++++++++++- 7 files changed, 52 insertions(+), 95 deletions(-) diff --git a/api/api.go b/api/api.go index 7d8ece9d7..2e1e1ea42 100644 --- a/api/api.go +++ b/api/api.go @@ -29,7 +29,6 @@ import ( "github.com/prometheus/alertmanager/cluster" "github.com/prometheus/alertmanager/config" "github.com/prometheus/alertmanager/dispatch" - "github.com/prometheus/alertmanager/featurecontrol" "github.com/prometheus/alertmanager/provider" "github.com/prometheus/alertmanager/silence" "github.com/prometheus/alertmanager/types" @@ -68,9 +67,6 @@ type Options struct { Concurrency int // Logger is used for logging, if nil, no logging will happen. Logger log.Logger - // FeatureFlags contains the set of feature flags. If nil, NoopFlags are used, - // and all controlled features are disabled. - FeatureFlags featurecontrol.Flagger // Registry is used to register Prometheus metrics. If nil, no metrics // registration will happen. Registry prometheus.Registerer @@ -121,7 +117,6 @@ func New(opts Options) (*API, error) { opts.Silences, opts.Peer, log.With(l, "version", "v2"), - opts.FeatureFlags, opts.Registry, ) if err != nil { diff --git a/api/v2/api.go b/api/v2/api.go index 9d70625cd..b4f57e75e 100644 --- a/api/v2/api.go +++ b/api/v2/api.go @@ -45,7 +45,6 @@ import ( "github.com/prometheus/alertmanager/cluster" "github.com/prometheus/alertmanager/config" "github.com/prometheus/alertmanager/dispatch" - "github.com/prometheus/alertmanager/featurecontrol" "github.com/prometheus/alertmanager/matchers/compat" "github.com/prometheus/alertmanager/pkg/labels" "github.com/prometheus/alertmanager/provider" @@ -73,7 +72,6 @@ type API struct { logger log.Logger m *metrics.Alerts - ff featurecontrol.Flagger Handler http.Handler } @@ -92,12 +90,8 @@ func NewAPI( silences *silence.Silences, peer cluster.ClusterPeer, l log.Logger, - ff featurecontrol.Flagger, r prometheus.Registerer, ) (*API, error) { - if ff == nil { - ff = featurecontrol.NoopFlags{} - } api := API{ alerts: alerts, getAlertStatus: sf, @@ -106,7 +100,6 @@ func NewAPI( silences: silences, logger: l, m: metrics.NewAlerts(r), - ff: ff, uptime: time.Now(), } @@ -356,7 +349,7 @@ func (api *API) postAlertsHandler(params alert_ops.PostAlertsParams) middleware. for _, a := range alerts { removeEmptyLabels(a.Labels) - if err := a.Validate(api.ff); err != nil { + if err := a.Validate(); err != nil { validationErrs.Add(err) api.m.Invalid().Inc() continue diff --git a/cmd/alertmanager/main.go b/cmd/alertmanager/main.go index d44a69d15..53db23ea0 100644 --- a/cmd/alertmanager/main.go +++ b/cmd/alertmanager/main.go @@ -320,16 +320,15 @@ func run() int { } api, err := api.New(api.Options{ - Alerts: alerts, - Silences: silences, - StatusFunc: marker.Status, - Peer: clusterPeer, - Timeout: *httpTimeout, - Concurrency: *getConcurrency, - Logger: log.With(logger, "component", "api"), - FeatureFlags: ff, - Registry: prometheus.DefaultRegisterer, - GroupFunc: groupFn, + Alerts: alerts, + Silences: silences, + StatusFunc: marker.Status, + Peer: clusterPeer, + Timeout: *httpTimeout, + Concurrency: *getConcurrency, + Logger: log.With(logger, "component", "api"), + Registry: prometheus.DefaultRegisterer, + GroupFunc: groupFn, }) if err != nil { level.Error(logger).Log("err", fmt.Errorf("failed to create API: %w", err)) diff --git a/silence/silence.go b/silence/silence.go index 3a31139c4..7b400bfe7 100644 --- a/silence/silence.go +++ b/silence/silence.go @@ -27,7 +27,6 @@ import ( "sort" "sync" "time" - "unicode/utf8" "github.com/benbjohnson/clock" "github.com/go-kit/log" @@ -39,6 +38,7 @@ import ( "github.com/prometheus/alertmanager/cluster" "github.com/prometheus/alertmanager/featurecontrol" + "github.com/prometheus/alertmanager/matchers/compat" "github.com/prometheus/alertmanager/pkg/labels" pb "github.com/prometheus/alertmanager/silence/silencepb" "github.com/prometheus/alertmanager/types" @@ -193,7 +193,6 @@ type Silences struct { logger log.Logger metrics *metrics - ff featurecontrol.Flagger retention time.Duration mtx sync.RWMutex @@ -340,7 +339,6 @@ func New(o Options) (*Silences, error) { clock: clock.New(), mc: matcherCache{}, logger: log.NewNopLogger(), - ff: featurecontrol.NoopFlags{}, retention: o.Retention, broadcast: func([]byte) {}, st: state{}, @@ -351,10 +349,6 @@ func New(o Options) (*Silences, error) { s.logger = o.Logger } - if o.FeatureFlags != nil { - s.ff = o.FeatureFlags - } - if o.SnapshotFile != "" { if r, err := os.Open(o.SnapshotFile); err != nil { if !os.IsNotExist(err) { @@ -477,9 +471,8 @@ func (s *Silences) GC() (int, error) { return n, nil } -// validateClassicMatcher validates the matcher against the classic rules. -func validateClassicMatcher(m *pb.Matcher) error { - if !model.LabelName(m.Name).IsValid() { +func validateMatcher(m *pb.Matcher) error { + if !compat.IsValidLabelName(model.LabelName(m.Name)) { return fmt.Errorf("invalid label name %q", m.Name) } switch m.Type { @@ -497,29 +490,6 @@ func validateClassicMatcher(m *pb.Matcher) error { return nil } -// validateUTF8Matcher validates the matcher against the UTF-8 rules. -func validateUTF8Matcher(m *pb.Matcher) error { - if !utf8.ValidString(m.Name) { - return fmt.Errorf("invalid label name %q", m.Name) - } - switch m.Type { - case pb.Matcher_EQUAL, pb.Matcher_NOT_EQUAL: - if !utf8.ValidString(m.Pattern) { - return fmt.Errorf("invalid label value %q", m.Pattern) - } - case pb.Matcher_REGEXP, pb.Matcher_NOT_REGEXP: - if !utf8.ValidString(m.Pattern) { - return fmt.Errorf("invalid regular expression %q", m.Pattern) - } - if _, err := regexp.Compile(m.Pattern); err != nil { - return fmt.Errorf("invalid regular expression %q: %w", m.Pattern, err) - } - default: - return fmt.Errorf("unknown matcher type %q", m.Type) - } - return nil -} - func matchesEmpty(m *pb.Matcher) bool { switch m.Type { case pb.Matcher_EQUAL: @@ -532,7 +502,7 @@ func matchesEmpty(m *pb.Matcher) bool { } } -func validateSilence(s *pb.Silence, ff featurecontrol.Flagger) error { +func validateSilence(s *pb.Silence) error { if s.Id == "" { return errors.New("ID missing") } @@ -541,13 +511,8 @@ func validateSilence(s *pb.Silence, ff featurecontrol.Flagger) error { } allMatchEmpty := true - validateFunc := validateUTF8Matcher - if ff.ClassicMode() { - validateFunc = validateClassicMatcher - } - for i, m := range s.Matchers { - if err := validateFunc(m); err != nil { + if err := validateMatcher(m); err != nil { return fmt.Errorf("invalid label matcher %d: %w", i, err) } allMatchEmpty = allMatchEmpty && matchesEmpty(m) @@ -588,7 +553,7 @@ func (s *Silences) setSilence(sil *pb.Silence, now time.Time, skipValidate bool) sil.UpdatedAt = now if !skipValidate { - if err := validateSilence(sil, s.ff); err != nil { + if err := validateSilence(sil); err != nil { return fmt.Errorf("silence invalid: %w", err) } } diff --git a/silence/silence_test.go b/silence/silence_test.go index 0a0797c3b..da39b3a89 100644 --- a/silence/silence_test.go +++ b/silence/silence_test.go @@ -33,6 +33,7 @@ import ( "go.uber.org/atomic" "github.com/prometheus/alertmanager/featurecontrol" + "github.com/prometheus/alertmanager/matchers/compat" pb "github.com/prometheus/alertmanager/silence/silencepb" "github.com/prometheus/alertmanager/types" ) @@ -1060,7 +1061,7 @@ func TestSilenceExpireInvalid(t *testing.T) { UpdatedAt: now.Add(-time.Hour), } // Assert that this silence is invalid. - require.EqualError(t, validateSilence(&silence, featurecontrol.NoopFlags{}), "invalid label matcher 0: unknown matcher type \"-1\"") + require.EqualError(t, validateSilence(&silence), "invalid label matcher 0: unknown matcher type \"-1\"") s.st = state{"active": &pb.MeshSilence{Silence: &silence}} @@ -1241,7 +1242,7 @@ func TestValidateClassicMatcher(t *testing.T) { } for _, c := range cases { - checkErr(t, c.err, validateClassicMatcher(c.m)) + checkErr(t, c.err, validateMatcher(c.m)) } } @@ -1330,8 +1331,18 @@ func TestValidateUTF8Matcher(t *testing.T) { }, } + // Change the mode to UTF-8 mode. + ff, err := featurecontrol.NewFlags(log.NewNopLogger(), featurecontrol.FeatureUTF8StrictMode) + require.NoError(t, err) + compat.InitFromFlags(log.NewNopLogger(), compat.RegisteredMetrics, ff) + + // Restore the mode to classic at the end of the test. + ff, err = featurecontrol.NewFlags(log.NewNopLogger(), featurecontrol.FeatureClassicMode) + require.NoError(t, err) + defer compat.InitFromFlags(log.NewNopLogger(), compat.RegisteredMetrics, ff) + for _, c := range cases { - checkErr(t, c.err, validateUTF8Matcher(c.m)) + checkErr(t, c.err, validateMatcher(c.m)) } } @@ -1455,11 +1466,7 @@ func TestValidateSilence(t *testing.T) { }, } for _, c := range cases { - ff, err := featurecontrol.NewFlags(log.NewNopLogger(), featurecontrol.FeatureClassicMode) - if err != nil { - t.Fatal("unexpected err", err) - } - checkErr(t, c.err, validateSilence(c.s, ff)) + checkErr(t, c.err, validateSilence(c.s)) } } diff --git a/types/types.go b/types/types.go index e10cc6885..54a889ab9 100644 --- a/types/types.go +++ b/types/types.go @@ -18,12 +18,11 @@ import ( "strings" "sync" "time" - "unicode/utf8" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/common/model" - "github.com/prometheus/alertmanager/featurecontrol" + "github.com/prometheus/alertmanager/matchers/compat" "github.com/prometheus/alertmanager/pkg/labels" ) @@ -305,24 +304,9 @@ type Alert struct { Timeout bool } -// validateLs validates the label set against either the classic rules -// or the UTF-8 rules depending on the feature flag. -func validateLs(ls model.LabelSet, ff featurecontrol.Flagger) error { - if ff.ClassicMode() { - return validateClassicLs(ls) - } - return validateUTF8Ls(ls) -} - -// validateClassicLs validates the label set against the classic rules. -func validateClassicLs(ls model.LabelSet) error { - return ls.Validate() -} - -// validateUTF8Ls validates the label set against the UTF-8 rules. -func validateUTF8Ls(ls model.LabelSet) error { +func validateLs(ls model.LabelSet) error { for ln, lv := range ls { - if len(ln) == 0 || !utf8.ValidString(string(ln)) { + if !compat.IsValidLabelName(ln) { return fmt.Errorf("invalid name %q", ln) } if !lv.IsValid() { @@ -334,7 +318,7 @@ func validateUTF8Ls(ls model.LabelSet) error { // Validate overrides the same method in model.Alert to allow UTF-8 labels. // This can be removed once prometheus/common has support for UTF-8. -func (a *Alert) Validate(ff featurecontrol.Flagger) error { +func (a *Alert) Validate() error { if a.StartsAt.IsZero() { return fmt.Errorf("start time missing") } @@ -344,10 +328,10 @@ func (a *Alert) Validate(ff featurecontrol.Flagger) error { if len(a.Labels) == 0 { return fmt.Errorf("at least one label pair required") } - if err := validateLs(a.Labels, ff); err != nil { + if err := validateLs(a.Labels); err != nil { return fmt.Errorf("invalid label set: %w", err) } - if err := validateLs(a.Annotations, ff); err != nil { + if err := validateLs(a.Annotations); err != nil { return fmt.Errorf("invalid annotations: %w", err) } return nil diff --git a/types/types_test.go b/types/types_test.go index 7caab154e..438f35153 100644 --- a/types/types_test.go +++ b/types/types_test.go @@ -20,9 +20,13 @@ import ( "testing" "time" + "github.com/go-kit/log" "github.com/prometheus/client_golang/prometheus" "github.com/prometheus/common/model" "github.com/stretchr/testify/require" + + "github.com/prometheus/alertmanager/featurecontrol" + "github.com/prometheus/alertmanager/matchers/compat" ) func TestMemMarker_Count(t *testing.T) { @@ -334,9 +338,19 @@ func TestValidateUTF8Ls(t *testing.T) { err: "invalid name \"\\xff\"", }} + // Change the mode to UTF-8 mode. + ff, err := featurecontrol.NewFlags(log.NewNopLogger(), featurecontrol.FeatureUTF8StrictMode) + require.NoError(t, err) + compat.InitFromFlags(log.NewNopLogger(), compat.RegisteredMetrics, ff) + + // Restore the mode to classic at the end of the test. + ff, err = featurecontrol.NewFlags(log.NewNopLogger(), featurecontrol.FeatureClassicMode) + require.NoError(t, err) + defer compat.InitFromFlags(log.NewNopLogger(), compat.RegisteredMetrics, ff) + for _, test := range tests { t.Run(test.name, func(t *testing.T) { - err := validateUTF8Ls(test.ls) + err := validateLs(test.ls) if err != nil && err.Error() != test.err { t.Errorf("unexpected err for %s: %s", test.ls, err) } else if err == nil && test.err != "" {