Skip to content

Commit 68e55d1

Browse files
juliusmhjuliusmh
committed
feat(querier): validate label/metric names with per-tenant naming scheme
1 parent 8ec6abb commit 68e55d1

21 files changed

+254
-107
lines changed

‎pkg/cardinality/request.go‎

Lines changed: 22 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import (
1313
"github.com/pkg/errors"
1414
"github.com/prometheus/common/model"
1515
"github.com/prometheus/prometheus/model/labels"
16+
"github.com/prometheus/prometheus/model/validation"
1617
"github.com/prometheus/prometheus/promql/parser"
1718
)
1819

@@ -145,27 +146,38 @@ func (r *LabelValuesRequest) String() string {
145146

146147
// DecodeLabelValuesRequest decodes the input http.Request into a LabelValuesRequest.
147148
// The input http.Request can either be a GET or POST with URL-encoded parameters.
148-
func DecodeLabelValuesRequest(r *http.Request, tenantMaxLimit int) (*LabelValuesRequest, error) {
149+
func DecodeLabelValuesRequest(
150+
r *http.Request,
151+
tenantMaxLimit int,
152+
tenantNameValidationScheme validation.NamingScheme,
153+
) (*LabelValuesRequest, error) {
149154
if err := r.ParseForm(); err != nil {
150155
return nil, err
151156
}
152-
153-
return DecodeLabelValuesRequestFromValuesWithTenantMaxLimit(r.Form, tenantMaxLimit)
157+
return DecodeLabelValuesRequestFromValuesWithTenantLimits(
158+
r.Form,
159+
tenantMaxLimit,
160+
tenantNameValidationScheme,
161+
)
154162
}
155163

156164
// DecodeLabelValuesRequestFromValues is like DecodeLabelValuesRequest but takes url.Values in input.
157165
func DecodeLabelValuesRequestFromValues(values url.Values) (*LabelValuesRequest, error) {
158-
return DecodeLabelValuesRequestFromValuesWithTenantMaxLimit(values, 0)
166+
return DecodeLabelValuesRequestFromValuesWithTenantLimits(values, 0, validation.LegacyNamingScheme)
159167
}
160168

161-
// DecodeLabelValuesRequestFromValuesWithTenantMaxLimit is like DecodeLabelValuesRequestFromValues but also accepts the tenantMaxLimit
162-
func DecodeLabelValuesRequestFromValuesWithTenantMaxLimit(values url.Values, tenantMaxLimit int) (*LabelValuesRequest, error) {
169+
// DecodeLabelValuesRequestFromValuesWithTenantLimits decodes label values from url.Values, respecting tenant limits.
170+
func DecodeLabelValuesRequestFromValuesWithTenantLimits(
171+
values url.Values,
172+
tenantMaxLimit int,
173+
tenantNameValidationScheme validation.NamingScheme,
174+
) (*LabelValuesRequest, error) {
163175
var (
164176
parsed = &LabelValuesRequest{}
165177
err error
166178
)
167179

168-
parsed.LabelNames, err = extractLabelNames(values)
180+
parsed.LabelNames, err = extractLabelNames(values, tenantNameValidationScheme)
169181
if err != nil {
170182
return nil, err
171183
}
@@ -254,18 +266,18 @@ func extractLimit(values url.Values, tenantMaxLimit int) (limit int, err error)
254266
}
255267

256268
// extractLabelNames parses and gets label_names query parameter containing an array of label values
257-
func extractLabelNames(values url.Values) ([]model.LabelName, error) {
269+
func extractLabelNames(values url.Values, nameValidationScheme validation.NamingScheme) ([]model.LabelName, error) {
258270
labelNamesParams := values["label_names[]"]
259271
if len(labelNamesParams) == 0 {
260272
return nil, fmt.Errorf("'label_names[]' param is required")
261273
}
262274

263275
labelNames := make([]model.LabelName, 0, len(labelNamesParams))
264276
for _, labelNameParam := range labelNamesParams {
265-
labelName := model.LabelName(labelNameParam)
266-
if !labelName.IsValid() {
277+
if !nameValidationScheme.IsValidLabelName(labelNameParam) {
267278
return nil, fmt.Errorf("invalid 'label_names' param '%v'", labelNameParam)
268279
}
280+
labelName := model.LabelName(labelNameParam)
269281
labelNames = append(labelNames, labelName)
270282
}
271283

‎pkg/cardinality/request_test.go‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010

1111
"github.com/prometheus/common/model"
1212
"github.com/prometheus/prometheus/model/labels"
13+
"github.com/prometheus/prometheus/model/validation"
1314
"github.com/stretchr/testify/assert"
1415
"github.com/stretchr/testify/require"
1516
)
@@ -98,7 +99,7 @@ func TestDecodeLabelValuesRequest(t *testing.T) {
9899
req, err := http.NewRequest("GET", "http://localhost?"+params.Encode(), nil)
99100
require.NoError(t, err)
100101

101-
actual, err := DecodeLabelValuesRequest(req, 0)
102+
actual, err := DecodeLabelValuesRequest(req, 0, validation.LegacyNamingScheme)
102103
require.NoError(t, err)
103104

104105
assert.Equal(t, expected, actual)
@@ -109,7 +110,7 @@ func TestDecodeLabelValuesRequest(t *testing.T) {
109110
require.NoError(t, err)
110111
req.Header.Set("Content-Type", "application/x-www-form-urlencoded")
111112

112-
actual, err := DecodeLabelValuesRequest(req, 0)
113+
actual, err := DecodeLabelValuesRequest(req, 0, validation.LegacyNamingScheme)
113114
require.NoError(t, err)
114115

115116
assert.Equal(t, expected, actual)

‎pkg/mimir/modules.go‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -827,6 +827,8 @@ func (t *Mimir) initQueryFrontendTripperware() (serv services.Service, err error
827827
panic(fmt.Sprintf("invalid config not caught by validation: unknown PromQL engine '%s'", t.Cfg.Querier.QueryEngine))
828828
}
829829

830+
eng = streamingpromqlcompat.NameValidatingEngine(eng, t.Overrides)
831+
830832
tripperware, err := querymiddleware.NewTripperware(
831833
t.Cfg.Frontend.QueryMiddleware,
832834
util_log.Logger,

‎pkg/querier/cardinality_analysis_handler.go‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -67,7 +67,8 @@ func LabelValuesCardinalityHandler(distributor Distributor, limits *validation.O
6767
return
6868
}
6969
tenantMaxLimit := limits.CardinalityAnalysisMaxResults(tenantID)
70-
cardinalityRequest, err := cardinality.DecodeLabelValuesRequest(r, tenantMaxLimit)
70+
tenantValidationScheme := limits.NameValidationScheme(tenantID)
71+
cardinalityRequest, err := cardinality.DecodeLabelValuesRequest(r, tenantMaxLimit, tenantValidationScheme)
7172
if err != nil {
7273
http.Error(w, err.Error(), http.StatusBadRequest)
7374
return

‎pkg/querier/engine/config.go‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,12 @@ func (cfg *Config) RegisterFlags(f *flag.FlagSet) {
5353
}
5454

5555
// NewPromQLEngineOptions returns the PromQL engine options based on the provided config.
56-
func NewPromQLEngineOptions(cfg Config, activityTracker *activitytracker.ActivityTracker, logger log.Logger, reg prometheus.Registerer) (promql.EngineOpts, streamingpromql.EngineOpts) {
56+
func NewPromQLEngineOptions(
57+
cfg Config,
58+
activityTracker *activitytracker.ActivityTracker,
59+
logger log.Logger,
60+
reg prometheus.Registerer,
61+
) (promql.EngineOpts, streamingpromql.EngineOpts) {
5762
commonOpts := promql.EngineOpts{
5863
Logger: util_log.SlogFromGoKit(logger),
5964
Reg: reg,

‎pkg/querier/querier.go‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -197,6 +197,7 @@ func New(cfg Config, limits *validation.Overrides, distributor Distributor, quer
197197
panic(fmt.Sprintf("invalid config not caught by validation: unknown PromQL engine '%s'", cfg.QueryEngine))
198198
}
199199

200+
eng = compat.NameValidatingEngine(eng, limits)
200201
return NewSampleAndChunkQueryable(lazyQueryable), exemplarQueryable, eng, nil
201202
}
202203

‎pkg/querier/stats_renderer_test.go‎

Lines changed: 0 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,6 @@ import (
1313
"github.com/grafana/dskit/user"
1414
"github.com/grafana/regexp"
1515
"github.com/prometheus/client_golang/prometheus"
16-
"github.com/prometheus/common/model"
1716
"github.com/prometheus/common/promslog"
1817
"github.com/prometheus/common/route"
1918
"github.com/prometheus/prometheus/config"
@@ -26,12 +25,6 @@ import (
2625
mimir_stats "github.com/grafana/mimir/pkg/querier/stats"
2726
)
2827

29-
func init() {
30-
// Mimir doesn't support Prometheus' UTF-8 metric/label name scheme yet.
31-
// nolint:staticcheck
32-
model.NameValidationScheme = model.LegacyValidation
33-
}
34-
3528
func TestStatsRenderer(t *testing.T) {
3629

3730
testCases := map[string]struct {

‎pkg/streamingpromql/compat/name_validating_engine.go‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,73 @@
1+
// SPDX-License-Identifier: AGPL-3.0-only
2+
3+
package compat
4+
5+
import (
6+
"context"
7+
"time"
8+
9+
"github.com/grafana/dskit/tenant"
10+
prom_validation "github.com/prometheus/prometheus/model/validation"
11+
"github.com/prometheus/prometheus/promql"
12+
"github.com/prometheus/prometheus/storage"
13+
14+
"github.com/grafana/mimir/pkg/util/validation"
15+
)
16+
17+
type nameValidatingEngine struct {
18+
engine promql.QueryEngine
19+
limits *validation.Overrides
20+
}
21+
22+
// NameValidatingEngine creates a new promql.QueryEngine that wraps engine and overrides query options
23+
// with the tenants naming scheme from limits.
24+
func NameValidatingEngine(engine promql.QueryEngine, limits *validation.Overrides) promql.QueryEngine {
25+
return &nameValidatingEngine{engine: engine, limits: limits}
26+
}
27+
28+
type optsWithNamingScheme struct {
29+
promql.QueryOpts
30+
namingScheme prom_validation.NamingScheme
31+
}
32+
33+
func (o optsWithNamingScheme) EnablePerStepStats() bool {
34+
return o.QueryOpts.EnablePerStepStats()
35+
}
36+
37+
func (o optsWithNamingScheme) LookbackDelta() time.Duration {
38+
return o.QueryOpts.LookbackDelta()
39+
}
40+
41+
func (o optsWithNamingScheme) NameValidationScheme() prom_validation.NamingScheme {
42+
return o.namingScheme
43+
}
44+
45+
func (e nameValidatingEngine) NewInstantQuery(ctx context.Context, q storage.Queryable, opts promql.QueryOpts, qs string, ts time.Time) (promql.Query, error) {
46+
userID, err := tenant.TenantID(ctx)
47+
if err != nil {
48+
return nil, err
49+
}
50+
if opts == nil {
51+
opts = promql.NewPrometheusQueryOpts(false, 0, prom_validation.UTF8NamingScheme)
52+
}
53+
opts = &optsWithNamingScheme{
54+
QueryOpts: opts,
55+
namingScheme: e.limits.NameValidationScheme(userID),
56+
}
57+
return e.engine.NewInstantQuery(ctx, q, opts, qs, ts)
58+
}
59+
60+
func (e nameValidatingEngine) NewRangeQuery(ctx context.Context, q storage.Queryable, opts promql.QueryOpts, qs string, start, end time.Time, interval time.Duration) (promql.Query, error) {
61+
userID, err := tenant.TenantID(ctx)
62+
if err != nil {
63+
return nil, err
64+
}
65+
if opts == nil {
66+
opts = promql.NewPrometheusQueryOpts(false, 0, prom_validation.UTF8NamingScheme)
67+
}
68+
opts = &optsWithNamingScheme{
69+
QueryOpts: opts,
70+
namingScheme: e.limits.NameValidationScheme(userID),
71+
}
72+
return e.engine.NewRangeQuery(ctx, q, opts, qs, start, end, interval)
73+
}

‎pkg/streamingpromql/engine_test.go‎

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import (
2424
"github.com/prometheus/prometheus/model/histogram"
2525
"github.com/prometheus/prometheus/model/labels"
2626
"github.com/prometheus/prometheus/model/timestamp"
27+
"github.com/prometheus/prometheus/model/validation"
2728
"github.com/prometheus/prometheus/promql"
2829
"github.com/prometheus/prometheus/promql/parser"
2930
"github.com/prometheus/prometheus/promql/parser/posrange"
@@ -3207,7 +3208,7 @@ func TestQueryStats(t *testing.T) {
32073208
runQueryAndGetSamplesStats := func(t *testing.T, engine promql.QueryEngine, expr string, isInstantQuery bool) *promstats.QuerySamples {
32083209
var q promql.Query
32093210
var err error
3210-
opts := promql.NewPrometheusQueryOpts(true, 0)
3211+
opts := promql.NewPrometheusQueryOpts(true, 0, validation.LegacyNamingScheme)
32113212
if isInstantQuery {
32123213
q, err = engine.NewInstantQuery(context.Background(), storage, opts, expr, end)
32133214
} else {
@@ -3501,7 +3502,7 @@ func TestQueryStatsUpstreamTestCases(t *testing.T) {
35013502
runQueryAndGetSamplesStats := func(t *testing.T, engine promql.QueryEngine, expr string, start, end time.Time, interval time.Duration) *promstats.QuerySamples {
35023503
var q promql.Query
35033504
var err error
3504-
opts := promql.NewPrometheusQueryOpts(true, 0)
3505+
opts := promql.NewPrometheusQueryOpts(true, 0, validation.LegacyNamingScheme)
35053506

35063507
if interval == 0 {
35073508
// Instant query
@@ -3882,7 +3883,7 @@ func TestQueryStatementLookbackDelta(t *testing.T) {
38823883
require.NoError(t, err)
38833884

38843885
t.Run("lookback delta not set in query options", func(t *testing.T) {
3885-
queryOpts := promql.NewPrometheusQueryOpts(false, 0)
3886+
queryOpts := promql.NewPrometheusQueryOpts(false, 0, validation.LegacyNamingScheme)
38863887
runTest(t, engine, queryOpts, defaultLookbackDelta)
38873888
})
38883889

@@ -3891,7 +3892,7 @@ func TestQueryStatementLookbackDelta(t *testing.T) {
38913892
})
38923893

38933894
t.Run("lookback delta set in query options", func(t *testing.T) {
3894-
queryOpts := promql.NewPrometheusQueryOpts(false, 14*time.Minute)
3895+
queryOpts := promql.NewPrometheusQueryOpts(false, 14*time.Minute, validation.LegacyNamingScheme)
38953896
runTest(t, engine, queryOpts, 14*time.Minute)
38963897
})
38973898
})
@@ -3903,7 +3904,7 @@ func TestQueryStatementLookbackDelta(t *testing.T) {
39033904
require.NoError(t, err)
39043905

39053906
t.Run("lookback delta not set in query options", func(t *testing.T) {
3906-
queryOpts := promql.NewPrometheusQueryOpts(false, 0)
3907+
queryOpts := promql.NewPrometheusQueryOpts(false, 0, validation.LegacyNamingScheme)
39073908
runTest(t, engine, queryOpts, 12*time.Minute)
39083909
})
39093910

@@ -3912,7 +3913,7 @@ func TestQueryStatementLookbackDelta(t *testing.T) {
39123913
})
39133914

39143915
t.Run("lookback delta set in query options", func(t *testing.T) {
3915-
queryOpts := promql.NewPrometheusQueryOpts(false, 14*time.Minute)
3916+
queryOpts := promql.NewPrometheusQueryOpts(false, 14*time.Minute, validation.LegacyNamingScheme)
39163917
runTest(t, engine, queryOpts, 14*time.Minute)
39173918
})
39183919
})

‎pkg/streamingpromql/operators/aggregations/aggregation_test.go‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@ import (
1010
"github.com/prometheus/prometheus/model/histogram"
1111
"github.com/prometheus/prometheus/model/labels"
1212
"github.com/prometheus/prometheus/model/timestamp"
13+
"github.com/prometheus/prometheus/model/validation"
1314
"github.com/prometheus/prometheus/promql"
1415
"github.com/prometheus/prometheus/promql/parser"
1516
"github.com/prometheus/prometheus/promql/parser/posrange"
@@ -337,7 +338,16 @@ func TestAggregations_ReturnIncompleteGroupsOnEarlyClose(t *testing.T) {
337338
"count_values": {
338339
createOperator: func(inner types.InstantVectorOperator, queryTimeRange types.QueryTimeRange, memoryConsumptionTracker *limiter.MemoryConsumptionTracker) (types.InstantVectorOperator, error) {
339340
labelName := operators.NewStringLiteral("value", posrange.PositionRange{})
340-
return NewCountValues(inner, labelName, queryTimeRange, []string{"group"}, false, memoryConsumptionTracker, posrange.PositionRange{}), nil
341+
return NewCountValues(
342+
inner,
343+
labelName,
344+
queryTimeRange,
345+
[]string{"group"},
346+
false,
347+
memoryConsumptionTracker,
348+
posrange.PositionRange{},
349+
validation.LegacyNamingScheme,
350+
), nil
341351
},
342352
instant: true,
343353
allowExpectedSeriesInAnyOrder: true,

0 commit comments

Comments
 (0)