From 7e3e7fdb55f8977ab25d3199b8805f11b44988ad Mon Sep 17 00:00:00 2001 From: SungJin1212 Date: Tue, 27 Oct 2026 21:58:36 +0900 Subject: [PATCH] validate merged tenant limits in POST /api/v1/user-overrides Signed-off-by: SungJin1212 --- CHANGELOG.md | 1 + docs/api/_index.md | 2 + docs/guides/overrides.md | 5 +++ integration/overrides_test.go | 44 ++++++++++++++++++- pkg/cortex/modules.go | 2 +- pkg/cortex/runtime_config.go | 12 +++-- pkg/overrides/api.go | 14 ++++++ pkg/overrides/overrides.go | 14 ++++-- pkg/overrides/overrides_test.go | 78 ++++++++++++++++++++++++++++++--- 9 files changed, 157 insertions(+), 15 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 969e6c8f3b5..fdd219a813c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,7 @@ * [ENHANCEMENT] Ruler: Add rule information to query parameters sent to the Query Frontend when `-ruler.frontend-address` is configured, so the Query Frontend query stats logs (`-frontend.enabled-ruler-query-stats`) can identify the evaluated rule. In this mode, the Ruler query stats log (`-ruler.query-stats-enabled`) omits the fetched statistics fields, which are always zero. #6539 * [ENHANCEMENT] gRPC: When `-auth.enabled=true`, a gRPC request without the `X-Scope-OrgID` metadata is now rejected with the `UNAUTHENTICATED` status code instead of `UNKNOWN`. #7873 * [ENHANCEMENT] Upgrade promql-engine and Thanos to latest. #7889 +* [ENHANCEMENT] Overrides API: Validate the merged tenant limits in `POST /api/v1/user-overrides` with the same checks used when loading the runtime config, and reject invalid values with 400. #7897 * [BUGFIX] Update prometheus alertmanager version to v0.34.1 to apply upstream inhibit rule bug fixes. #7877 * [BUGFIX] Tenant Federation: Fix regex tenant federation not working in single binary mode, and failing with `multiple org IDs present` when a matched tenant ID contains regex metacharacters and also matches other tenants as a regex. #7869 * [BUGFIX] Query Frontend: Fix the data selection time window computed for an instant query sent without a `time` parameter. The default evaluation time was passed to `util.ParseTimeParam` in milliseconds, but that helper formats the default and parses it as seconds, so the value overflowed `UnixNano()` and wrapped to a timestamp decades away from `now`. This made the `data_select_min_time` and `data_select_max_time` query stats wrong, and stopped the `time_window` query attribute of query priority and query rejection from matching such queries correctly. #7880 diff --git a/docs/api/_index.md b/docs/api/_index.md index da9153646ca..70b64a74ba4 100644 --- a/docs/api/_index.md +++ b/docs/api/_index.md @@ -981,6 +981,8 @@ The following limits can be modified via the API: Overrides are validated against hard limits defined in the runtime configuration file. If a requested override exceeds the hard limit for the tenant, the request will be rejected with a 400 status code. +The merged limits of the tenant are also validated with the same checks used when loading the runtime configuration. If the validation fails, the request will be rejected with a 400 status code. + ## Store-gateway ### Store-gateway ring status diff --git a/docs/guides/overrides.md b/docs/guides/overrides.md index 0c7d78df13e..af6365ab4f5 100644 --- a/docs/guides/overrides.md +++ b/docs/guides/overrides.md @@ -203,6 +203,7 @@ Returns success with no body. **Response (400 Bad Request):** - Invalid limit names (not in `api_allowed_limits`) - Values exceeding hard limits +- Values failing limits validation (see [Limits Validation](#limits-validation)) - Invalid JSON format ### Delete User Overrides @@ -280,6 +281,10 @@ Limit names correspond to fields in the [`limits_config`](../configuration/confi - `max_label_name_length` - `max_label_value_length` +### Limits Validation + +Before writing to the runtime configuration file, the merged limits of the tenant are validated with the same checks that Cortex components run when they load the runtime configuration. For example, setting `max_global_series_per_user` is rejected when `-distributor.shard-by-all-labels` is disabled. + ## Hard Limits Hard limits provide per-tenant upper bounds for override values. They prevent tenants from setting limits that exceed their allocated capacity. diff --git a/integration/overrides_test.go b/integration/overrides_test.go index 066236d955c..ce28b601aee 100644 --- a/integration/overrides_test.go +++ b/integration/overrides_test.go @@ -174,6 +174,44 @@ func TestOverridesAPIWithRunningCortex(t *testing.T) { assert.Equal(t, http.StatusBadRequest, resp.StatusCode) }) + t.Run("POST overrides failing limits validation", func(t *testing.T) { + // max_global_series_per_user requires shard-by-all-labels, which is disabled by default. + invalidOverrides := map[string]interface{}{ + "max_global_series_per_user": 10000, + } + requestBody, err := json.Marshal(invalidOverrides) + require.NoError(t, err) + + req, err := http.NewRequest("POST", "http://"+cortexSvc.HTTPEndpoint()+"/api/v1/user-overrides", bytes.NewReader(requestBody)) + require.NoError(t, err) + req.Header.Set("X-Scope-OrgID", "user1") + req.Header.Set("Content-Type", "application/json") + + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, http.StatusBadRequest, resp.StatusCode) + + // The runtime config must be left unchanged. + req, err = http.NewRequest("GET", "http://"+cortexSvc.HTTPEndpoint()+"/api/v1/user-overrides", nil) + require.NoError(t, err) + req.Header.Set("X-Scope-OrgID", "user1") + + resp, err = http.DefaultClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + + assert.Equal(t, http.StatusOK, resp.StatusCode) + + var savedOverrides map[string]interface{} + err = json.NewDecoder(resp.Body).Decode(&savedOverrides) + require.NoError(t, err) + + assert.Equal(t, float64(5000), savedOverrides["ingestion_rate"]) + assert.Equal(t, float64(0), savedOverrides["max_global_series_per_user"]) + }) + t.Run("DELETE overrides", func(t *testing.T) { req, err := http.NewRequest("DELETE", "http://"+cortexSvc.HTTPEndpoint()+"/api/v1/user-overrides", nil) require.NoError(t, err) @@ -223,7 +261,11 @@ func TestOverridesAPIHardLimits(t *testing.T) { } uploadRuntimeConfig(t, minio, runtimeConfig) - cortexSvc := e2ecortex.NewSingleBinary("cortex-overrides-hard-limits", defaultArgsOverrides(minio), "") + // max_global_series_per_user is rejected by the limits validation unless shard-by-all-labels is enabled. + flags := e2e.MergeFlags(defaultArgsOverrides(minio), map[string]string{ + "-distributor.shard-by-all-labels": "true", + }) + cortexSvc := e2ecortex.NewSingleBinary("cortex-overrides-hard-limits", flags, "") require.NoError(t, s.StartAndWaitReady(cortexSvc)) t.Run("POST overrides within hard limits", func(t *testing.T) { diff --git a/pkg/cortex/modules.go b/pkg/cortex/modules.go index d787a6b2429..9e9b400b5b4 100644 --- a/pkg/cortex/modules.go +++ b/pkg/cortex/modules.go @@ -218,7 +218,7 @@ func (t *Cortex) initOverridesConfig() (services.Service, error) { func (t *Cortex) initOverrides() (services.Service, error) { - overridesAPI, err := overrides.New(t.Cfg.RuntimeConfig, util_log.Logger, prometheus.DefaultRegisterer) + overridesAPI, err := overrides.New(t.Cfg.RuntimeConfig, runtimeConfigLoader{cfg: t.Cfg}.validateTenantLimits, util_log.Logger, prometheus.DefaultRegisterer) if err != nil { return nil, fmt.Errorf("failed to create overrides API: %w", err) } diff --git a/pkg/cortex/runtime_config.go b/pkg/cortex/runtime_config.go index 8031b6be5c4..ae04a8b4a31 100644 --- a/pkg/cortex/runtime_config.go +++ b/pkg/cortex/runtime_config.go @@ -77,10 +77,7 @@ func (l runtimeConfigLoader) load(r io.Reader) (any, error) { // refer to https://github.com/cortexproject/cortex/issues/6741#issuecomment-3067244929 if overrides != nil { for userID, ul := range overrides.TenantLimits { - if err := ul.Validate(l.cfg.NameValidationScheme, l.cfg.Distributor.ShardByAllLabels, l.cfg.Ingester.ActiveSeriesMetricsEnabled, l.cfg.Distributor.HATrackerConfig.UpdateTimeout, l.cfg.Distributor.HATrackerConfig.UpdateTimeoutJitterMax); err != nil { - return nil, err - } - if err := ul.ValidateQueryLimits(userID, l.cfg.BlocksStorage.TSDB.CloseIdleTSDBTimeout); err != nil { + if err := l.validateTenantLimits(userID, ul); err != nil { return nil, err } } @@ -91,6 +88,13 @@ func (l runtimeConfigLoader) load(r io.Reader) (any, error) { return overrides, nil } +func (l runtimeConfigLoader) validateTenantLimits(userID string, ul *validation.Limits) error { + if err := ul.Validate(l.cfg.NameValidationScheme, l.cfg.Distributor.ShardByAllLabels, l.cfg.Ingester.ActiveSeriesMetricsEnabled, l.cfg.Distributor.HATrackerConfig.UpdateTimeout, l.cfg.Distributor.HATrackerConfig.UpdateTimeoutJitterMax); err != nil { + return err + } + return ul.ValidateQueryLimits(userID, l.cfg.BlocksStorage.TSDB.CloseIdleTSDBTimeout) +} + func multiClientRuntimeConfigChannel(manager *runtimeconfig.Manager) func() <-chan kv.MultiRuntimeConfig { if manager == nil { return nil diff --git a/pkg/overrides/api.go b/pkg/overrides/api.go index bbbea84dd50..73b620ee82f 100644 --- a/pkg/overrides/api.go +++ b/pkg/overrides/api.go @@ -26,6 +26,8 @@ const ( ErrRuntimeConfig = "runtime config read error" ) +var errInvalidOverrides = errors.New("invalid overrides") + // getAllowedLimitsFromBucket reads allowed limits from the runtime config file func (a *API) getAllowedLimitsFromBucket(ctx context.Context) ([]string, error) { reader, err := a.bucketClient.Get(ctx, a.runtimeConfigPath) @@ -117,6 +119,11 @@ func (a *API) SetOverrides(w http.ResponseWriter, r *http.Request) { // Write overrides to bucket storage if err := a.setOverridesToBucket(r.Context(), userID, overrides); err != nil { + if errors.Is(err, errInvalidOverrides) { + level.Error(a.logger).Log("msg", "limits validation failed", "userID", userID, "err", err) + http.Error(w, "Invalid overrides", http.StatusBadRequest) + return + } level.Error(a.logger).Log("msg", "failed to set overrides to bucket", "userID", userID, "err", err) http.Error(w, "Internal server error", http.StatusInternalServerError) return @@ -248,6 +255,13 @@ func (a *API) setOverridesToBucket(ctx context.Context, userID string, overrides return fmt.Errorf("invalid overrides format: %w", err) } + // Reject limits that would make the runtime config fail to load in other components. + if a.validateLimits != nil { + if err := a.validateLimits(userID, &limits); err != nil { + return fmt.Errorf("%w: %w", errInvalidOverrides, err) + } + } + config.TenantLimits[userID] = &limits data, err := yaml.Marshal(config) diff --git a/pkg/overrides/overrides.go b/pkg/overrides/overrides.go index e6e3e0e6c19..06db91ee68c 100644 --- a/pkg/overrides/overrides.go +++ b/pkg/overrides/overrides.go @@ -12,6 +12,7 @@ import ( "github.com/cortexproject/cortex/pkg/storage/bucket" "github.com/cortexproject/cortex/pkg/util/runtimeconfig" "github.com/cortexproject/cortex/pkg/util/services" + "github.com/cortexproject/cortex/pkg/util/validation" ) const ( @@ -19,6 +20,9 @@ const ( ErrFailedToCreateBucketClient = "failed to create bucket client for overrides" ) +// LimitsValidator validates the merged limits of a tenant before they are written to the runtime config. +type LimitsValidator func(userID string, limits *validation.Limits) error + type API struct { services.Service cfg runtimeconfig.Config @@ -26,17 +30,19 @@ type API struct { registerer prometheus.Registerer bucketClient objstore.Bucket runtimeConfigPath string + validateLimits LimitsValidator } -func New(cfg runtimeconfig.Config, logger log.Logger, registerer prometheus.Registerer) (*API, error) { +func New(cfg runtimeconfig.Config, validateLimits LimitsValidator, logger log.Logger, registerer prometheus.Registerer) (*API, error) { if err := cfg.StorageConfig.Validate(); err != nil { return nil, fmt.Errorf("%s: %w", ErrInvalidOverridesConfiguration, err) } api := &API{ - cfg: cfg, - logger: logger, - registerer: registerer, + cfg: cfg, + logger: logger, + registerer: registerer, + validateLimits: validateLimits, } api.Service = services.NewBasicService(api.starting, api.running, api.stopping) return api, nil diff --git a/pkg/overrides/overrides_test.go b/pkg/overrides/overrides_test.go index 3cc505855c6..6c1beb1eb71 100644 --- a/pkg/overrides/overrides_test.go +++ b/pkg/overrides/overrides_test.go @@ -3,11 +3,13 @@ package overrides import ( "bytes" "encoding/json" + "errors" "flag" "fmt" "io" "net/http" "net/http/httptest" + "strings" "testing" "github.com/prometheus/client_golang/prometheus" @@ -22,6 +24,7 @@ import ( "github.com/cortexproject/cortex/pkg/util/log" "github.com/cortexproject/cortex/pkg/util/runtimeconfig" "github.com/cortexproject/cortex/pkg/util/services" + "github.com/cortexproject/cortex/pkg/util/validation" ) func TestConfig_Validate(t *testing.T) { @@ -111,7 +114,7 @@ func TestNew(t *testing.T) { for testName, testData := range tests { t.Run(testName, func(t *testing.T) { - api, err := New(testData.cfg, log.Logger, prometheus.DefaultRegisterer) + api, err := New(testData.cfg, nil, log.Logger, prometheus.DefaultRegisterer) if testData.expectError { assert.Error(t, err) @@ -140,7 +143,7 @@ func TestOverridesModuleServiceInterface(t *testing.T) { } // Set default values before validation flagext.DefaultValues(&cfg.StorageConfig.S3) - api, err := New(cfg, log.Logger, prometheus.DefaultRegisterer) + api, err := New(cfg, nil, log.Logger, prometheus.DefaultRegisterer) require.NoError(t, err) require.NotNil(t, api) @@ -417,7 +420,7 @@ api_allowed_limits: } // Set default values before validation flagext.DefaultValues(&cfg.StorageConfig.S3) - api, err := New(cfg, log.Logger, prometheus.DefaultRegisterer) + api, err := New(cfg, nil, log.Logger, prometheus.DefaultRegisterer) require.NoError(t, err) require.NotNil(t, api) @@ -515,7 +518,7 @@ func TestAPITenantExtraction(t *testing.T) { } // Set default values before validation flagext.DefaultValues(&cfg.StorageConfig.S3) - api, err := New(cfg, log.Logger, prometheus.DefaultRegisterer) + api, err := New(cfg, nil, log.Logger, prometheus.DefaultRegisterer) require.NoError(t, err) require.NotNil(t, api) @@ -612,7 +615,7 @@ api_allowed_limits: } // Set default values before validation flagext.DefaultValues(&cfg.StorageConfig.S3) - api, err := New(cfg, log.Logger, prometheus.DefaultRegisterer) + api, err := New(cfg, nil, log.Logger, prometheus.DefaultRegisterer) require.NoError(t, err) require.NotNil(t, api) @@ -652,3 +655,68 @@ api_allowed_limits: }) } } + +func TestSetOverridesLimitsValidation(t *testing.T) { + const runtimeConfig = `overrides: + user1: + ingestion_rate: 1000 + max_global_series_per_user: 100000 +api_allowed_limits: + - ingestion_rate + - max_global_series_per_user` + + tests := map[string]struct { + requestBody string + validateLimits LimitsValidator + expectedStatus int + expectedBody string + expectUpload bool + }{ + "validator receives merged limits": { + requestBody: `{"ingestion_rate": 2000}`, + validateLimits: func(userID string, limits *validation.Limits) error { + assert.Equal(t, "user1", userID) + assert.Equal(t, float64(2000), limits.IngestionRate) + assert.Equal(t, 100000, limits.MaxGlobalSeriesPerUser) + return nil + }, + expectedStatus: http.StatusOK, + expectUpload: true, + }, + "validator error rejects the request": { + requestBody: `{"max_global_series_per_user": 200000}`, + validateLimits: func(string, *validation.Limits) error { + return errors.New("max_global_series_per_user is not supported") + }, + expectedStatus: http.StatusBadRequest, + expectedBody: "Invalid overrides", + }, + } + + for name, tc := range tests { + t.Run(name, func(t *testing.T) { + mockBucket := &bucket.ClientMock{} + mockBucket.MockGet("runtime.yaml", runtimeConfig, nil) + mockBucket.MockUpload("runtime.yaml", nil) + + api, err := New(runtimeconfig.Config{StorageConfig: bucket.Config{Backend: bucket.Filesystem}}, tc.validateLimits, log.Logger, prometheus.NewRegistry()) + require.NoError(t, err) + api.bucketClient = mockBucket + api.runtimeConfigPath = "runtime.yaml" + + req := httptest.NewRequest(http.MethodPost, "/api/v1/user-overrides", strings.NewReader(tc.requestBody)) + req.Header.Set("X-Scope-OrgID", "user1") + recorder := httptest.NewRecorder() + + api.SetOverrides(recorder, req) + + assert.Equal(t, tc.expectedStatus, recorder.Code) + assert.Contains(t, recorder.Body.String(), tc.expectedBody) + if tc.expectUpload { + mockBucket.AssertCalled(t, "Upload", mock.Anything, "runtime.yaml", mock.Anything) + } else { + mockBucket.AssertNotCalled(t, "Upload", mock.Anything, "runtime.yaml", mock.Anything) + } + }) + } +}