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 <[email protected]>

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 {

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

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

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

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

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

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 != "" {