diff --git a/execution/binary/utils.go b/execution/binary/utils.go index a11557ca..db086930 100644 --- a/execution/binary/utils.go +++ b/execution/binary/utils.go @@ -42,9 +42,18 @@ func newManyToManyMatchError(matching *parser.VectorMatching, original, duplicat func (e *errManyToManyMatch) Error() string { group := e.original.MatchLabels(e.matching.On, e.matching.MatchingLabels...) + // The choice of which series is reported as "original" vs "duplicate" is + // driven by upstream StepVector / map iteration order and is therefore + // non-deterministic across runs. Sort the rendered label-set strings so + // the error message is stable; downstream consumers (e.g. Cortex, + // see cortexproject/cortex#7546) compare these messages. + original, duplicate := e.original.String(), e.duplicate.String() + if duplicate < original { + original, duplicate = duplicate, original + } msg := "found duplicate series for the match group %s on the %s hand-side of the operation: [%s, %s]" + ";many-to-many matching not allowed: matching labels must be unique on one side" - return fmt.Sprintf(msg, group, e.side, e.original.String(), e.duplicate.String()) + return fmt.Sprintf(msg, group, e.side, original, duplicate) } func shouldDropMetricName(op parser.ItemType, returnBool bool) bool { diff --git a/execution/binary/utils_test.go b/execution/binary/utils_test.go new file mode 100644 index 00000000..e8098a1b --- /dev/null +++ b/execution/binary/utils_test.go @@ -0,0 +1,96 @@ +// Copyright (c) The Thanos Community Authors. +// Licensed under the Apache License 2.0. + +package binary + +import ( + "strings" + "testing" + + "github.com/efficientgo/core/testutil" + "github.com/prometheus/prometheus/model/labels" + "github.com/prometheus/prometheus/promql/parser" +) + +// TestErrManyToManyMatchDeterministicOrder verifies that errManyToManyMatch.Error() +// produces a byte-identical error string regardless of which of the two duplicate +// series happened to land in the "original" vs "duplicate" slot. +// +// Background: the choice of which series becomes original vs duplicate is driven +// by upstream StepVector ordering, which is in turn fed by map iteration (e.g. +// in execution/scan/subquery.go). That made the error message non-deterministic +// across runs and across parallel engine workers, breaking error-string equality +// checks in downstream consumers (see cortexproject/cortex#7546). +// +// Fix: lexicographically sort the two rendered label-set strings before +// formatting the message. +func TestErrManyToManyMatchDeterministicOrder(t *testing.T) { + matching := &parser.VectorMatching{ + On: true, + MatchingLabels: []string{"job"}, + } + + for _, tc := range []struct { + name string + side binOpSide + a labels.Labels + b labels.Labels + }{ + { + name: "simple labels right side", + side: rhBinOpSide, + a: labels.FromStrings("__name__", "requests_total", "instance", "a", "job", "api"), + b: labels.FromStrings("__name__", "requests_total", "instance", "b", "job", "api"), + }, + { + name: "simple labels left side", + side: lhBinOpSide, + a: labels.FromStrings("zone", "us-east-1", "job", "api"), + b: labels.FromStrings("zone", "us-west-2", "job", "api"), + }, + { + // Adversarial values that include the same punctuation we use to + // frame the rendered list ([, ], comma, braces, quotes). Sorting + // the rendered label-set strings — not the labels themselves — + // keeps the comparison robust against these characters. + name: "adversarial label values", + side: rhBinOpSide, + a: labels.FromStrings( + "detail", `left[,{}"]`, + "instance", `a[,{}"]`, + "job", `api[,{}"]`, + ), + b: labels.FromStrings( + "detail", `right[,{}"]`, + "instance", `b[,{}"]`, + "job", `api[,{}"]`, + ), + }, + } { + t.Run(tc.name, func(t *testing.T) { + aFirst := (&errManyToManyMatch{ + matching: matching, + side: tc.side, + original: tc.a, + duplicate: tc.b, + }).Error() + bFirst := (&errManyToManyMatch{ + matching: matching, + side: tc.side, + original: tc.b, + duplicate: tc.a, + }).Error() + + testutil.Equals(t, aFirst, bFirst) + + // Sanity-check that both rendered label sets are still present and + // that the existing prefix/format expected by downstream consumers + // (e.g. Prometheus-compatible error matchers) is preserved. + testutil.Assert(t, strings.Contains(aFirst, tc.a.String()), "expected %q to contain %q", aFirst, tc.a.String()) + testutil.Assert(t, strings.Contains(aFirst, tc.b.String()), "expected %q to contain %q", aFirst, tc.b.String()) + testutil.Assert(t, strings.Contains(aFirst, "found duplicate series for the match group"), "missing expected prefix in %q", aFirst) + testutil.Assert(t, strings.Contains(aFirst, "many-to-many matching not allowed"), "missing expected suffix in %q", aFirst) + testutil.Assert(t, strings.Contains(aFirst, string(tc.side)+" hand-side"), "missing expected side marker in %q", aFirst) + }) + } +}