From e099791091f439322def893ab83225734a42cb63 Mon Sep 17 00:00:00 2001 From: Radu Berinde Date: Sun, 12 Sep 2021 13:31:55 +0000 Subject: [PATCH] tenantcostclient: restrict allowed configuration from the tenant side This change restricts the configuration of tenant cost control from the tenant side. In the future, we will want to have settings where the values come from the host cluster but we don't have that infrastructure today. With tenants being able to set their own settings, they could easily sabotage the cost control mechanisms. This change restricts the allowed values for the target period and the CPU usage allowance, and fixes the cost model configuration to the default. Release note: None Release justification: Necessary fix for the distributed rate limiting functionality, which is vital for the upcoming Serverless MVP release. It allows CRDB to throttle clusters that have run out of free or paid request units (which measure CPU and I/O usage). This functionality is only enabled in multi-tenant scenarios and should have no impact on our dedicated customers. --- .../tenantcostclient/tenant_side.go | 23 +++++++++++++++---- pkg/multitenant/tenantcostmodel/settings.go | 4 ++++ 2 files changed, 23 insertions(+), 4 deletions(-) diff --git a/pkg/ccl/multitenantccl/tenantcostclient/tenant_side.go b/pkg/ccl/multitenantccl/tenantcostclient/tenant_side.go index 87091a401198..124da1ed8148 100644 --- a/pkg/ccl/multitenantccl/tenantcostclient/tenant_side.go +++ b/pkg/ccl/multitenantccl/tenantcostclient/tenant_side.go @@ -32,7 +32,7 @@ var TargetPeriodSetting = settings.RegisterDurationSetting( "tenant_cost_control_period", "target duration between token bucket requests from tenants (requires restart)", 10*time.Second, - settings.PositiveDuration, + checkDurationInRange(5*time.Second, 120*time.Second), ) // CPUUsageAllowance is exported for testing purposes. @@ -42,9 +42,22 @@ var CPUUsageAllowance = settings.RegisterDurationSetting( "doesn't contribute to consumption; for example, if it is set to 10ms, "+ "that corresponds to 1% of a CPU", 10*time.Millisecond, - settings.NonNegativeDuration, + checkDurationInRange(0, 10*time.Millisecond), ) +// checkDurationInRange returns a function used to validate duration cluster +// settings. Because these values are currently settable by the tenant, we need +// to restrict the allowed values to avoid possible sabotage of the cost control +// mechanisms. +func checkDurationInRange(min, max time.Duration) func(v time.Duration) error { + return func(v time.Duration) error { + if v < min || v > max { + return errors.Errorf("value %s out of range (%s, %s)", v, min, max) + } + return nil + } +} + // mainLoopUpdateInterval is the period at which we collect CPU usage and // evaluate whether we need to send a new token request. const mainLoopUpdateInterval = 1 * time.Second @@ -93,8 +106,10 @@ func newTenantSideCostController( } c.limiter.Init(c.timeSource, c.lowRUNotifyChan) - // TODO(radu): support changing the tenant configuration at runtime. - c.costCfg = tenantcostmodel.ConfigFromSettings(&st.SV) + // TODO(radu): these settings can currently be changed by the tenant (see + // #47918), which would made it very easy to evade cost control. For now, use + // the hardcoded default values. + c.costCfg = tenantcostmodel.DefaultConfig() return c, nil } diff --git a/pkg/multitenant/tenantcostmodel/settings.go b/pkg/multitenant/tenantcostmodel/settings.go index c121d3eac816..536188c24c8a 100644 --- a/pkg/multitenant/tenantcostmodel/settings.go +++ b/pkg/multitenant/tenantcostmodel/settings.go @@ -21,6 +21,10 @@ import ( // // The KV operation parameters are set based on experiments, where 1000 Request // Units correspond to one CPU second of usage on the host cluster. +// +// TODO(radu): these settings are not currently used on the tenant side; there, +// only the defaults are used. Ideally, the tenant would always get the values +// from the host cluster. var ( readRequestCost = settings.RegisterFloatSetting( "tenant_cost_model.kv_read_request_cost",