diff --git a/pkg/ccl/serverccl/BUILD.bazel b/pkg/ccl/serverccl/BUILD.bazel index 569b67a7282f..abbe55463fb6 100644 --- a/pkg/ccl/serverccl/BUILD.bazel +++ b/pkg/ccl/serverccl/BUILD.bazel @@ -26,6 +26,7 @@ go_test( "//pkg/server", "//pkg/server/serverpb", "//pkg/sql", + "//pkg/sql/pgwire/pgcode", "//pkg/testutils/serverutils", "//pkg/testutils/sqlutils", "//pkg/testutils/testcluster", @@ -35,6 +36,7 @@ go_test( "//pkg/util/log", "//pkg/util/randutil", "//pkg/util/timeutil", + "@com_github_lib_pq//:pq", "@com_github_stretchr_testify//require", "@org_golang_x_crypto//bcrypt", ], diff --git a/pkg/ccl/serverccl/server_sql_test.go b/pkg/ccl/serverccl/server_sql_test.go index 57ebe6603930..3a7f43f3f899 100644 --- a/pkg/ccl/serverccl/server_sql_test.go +++ b/pkg/ccl/serverccl/server_sql_test.go @@ -10,6 +10,7 @@ package serverccl import ( "context" + "errors" "io/ioutil" "net/http" "testing" @@ -17,11 +18,13 @@ import ( "github.com/cockroachdb/cockroach/pkg/base" "github.com/cockroachdb/cockroach/pkg/roachpb" "github.com/cockroachdb/cockroach/pkg/security" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgcode" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/testutils/sqlutils" "github.com/cockroachdb/cockroach/pkg/util/httputil" "github.com/cockroachdb/cockroach/pkg/util/leaktest" "github.com/cockroachdb/cockroach/pkg/util/log" + "github.com/lib/pq" "github.com/stretchr/testify/require" ) @@ -73,13 +76,11 @@ func TestTenantCannotSetClusterSetting(t *testing.T) { defer db.Close() _, err := db.Exec(`SET CLUSTER SETTING sql.defaults.vectorize=off`) require.NoError(t, err) - /* TODO(dt): re-introduce when system-settings are prevented in tenants. _, err = db.Exec(`SET CLUSTER SETTING kv.snapshot_rebalance.max_rate = '2MiB';`) var pqErr *pq.Error ok := errors.As(err, &pqErr) require.True(t, ok, "expected err to be a *pq.Error but is of type %T. error is: %v", err) require.Equal(t, pq.ErrorCode(pgcode.InsufficientPrivilege.String()), pqErr.Code, "err %v has unexpected code", err) - */ } func TestTenantUnauthenticatedAccess(t *testing.T) { diff --git a/pkg/kv/kvserver/store_snapshot.go b/pkg/kv/kvserver/store_snapshot.go index 5860b36394f3..698981798068 100644 --- a/pkg/kv/kvserver/store_snapshot.go +++ b/pkg/kv/kvserver/store_snapshot.go @@ -845,7 +845,7 @@ var rebalanceSnapshotRate = settings.RegisterByteSizeSetting( "the rate limit (bytes/sec) to use for rebalance and upreplication snapshots", envutil.EnvOrDefaultBytes("COCKROACH_PREEMPTIVE_SNAPSHOT_RATE", 8<<20), validatePositive, -).WithPublic() +).WithPublic().WithSystemOnly() // recoverySnapshotRate is the rate at which Raft-initiated spanshots can be // sent. Ideally, one would never see a Raft-initiated snapshot; we'd like all @@ -858,7 +858,7 @@ var recoverySnapshotRate = settings.RegisterByteSizeSetting( "the rate limit (bytes/sec) to use for recovery snapshots", envutil.EnvOrDefaultBytes("COCKROACH_RAFT_SNAPSHOT_RATE", 8<<20), validatePositive, -).WithPublic() +).WithPublic().WithSystemOnly() // snapshotSenderBatchSize is the size that key-value batches are allowed to // grow to during Range snapshots before being sent to the receiver. This limit diff --git a/pkg/settings/bool.go b/pkg/settings/bool.go index ce2af73756b9..31597303e203 100644 --- a/pkg/settings/bool.go +++ b/pkg/settings/bool.go @@ -82,6 +82,15 @@ func (b *BoolSetting) WithPublic() *BoolSetting { return b } +// WithSystemOnly marks this setting as system-only and can be chained. +func (b *BoolSetting) WithSystemOnly() *BoolSetting { + b.common.systemOnly = true + return b +} + +// Defeat the linter. +var _ = (*BoolSetting).WithSystemOnly + // RegisterBoolSetting defines a new setting with type bool. func RegisterBoolSetting(key, desc string, defaultValue bool) *BoolSetting { setting := &BoolSetting{defaultValue: defaultValue} diff --git a/pkg/settings/byte_size.go b/pkg/settings/byte_size.go index 210afcb4ff83..1c88056b7ce3 100644 --- a/pkg/settings/byte_size.go +++ b/pkg/settings/byte_size.go @@ -39,6 +39,12 @@ func (b *ByteSizeSetting) WithPublic() *ByteSizeSetting { return b } +// WithSystemOnly marks this setting as system-only and can be chained. +func (b *ByteSizeSetting) WithSystemOnly() *ByteSizeSetting { + b.common.systemOnly = true + return b +} + // RegisterByteSizeSetting defines a new setting with type bytesize and any // supplied validation function(s). func RegisterByteSizeSetting( diff --git a/pkg/settings/duration.go b/pkg/settings/duration.go index 38545e86bd4a..3fd1d972331a 100644 --- a/pkg/settings/duration.go +++ b/pkg/settings/duration.go @@ -111,6 +111,15 @@ func (d *DurationSetting) WithPublic() *DurationSetting { return d } +// WithSystemOnly marks this setting as system-only and can be chained. +func (d *DurationSetting) WithSystemOnly() *DurationSetting { + d.common.systemOnly = true + return d +} + +// Defeat the linter. +var _ = (*DurationSetting).WithSystemOnly + // RegisterDurationSetting defines a new setting with type duration. func RegisterDurationSetting( key, desc string, defaultValue time.Duration, validateFns ...func(time.Duration) error, diff --git a/pkg/settings/enum.go b/pkg/settings/enum.go index 69d0a0d843e9..c63993accd23 100644 --- a/pkg/settings/enum.go +++ b/pkg/settings/enum.go @@ -109,6 +109,15 @@ func (e *EnumSetting) WithPublic() *EnumSetting { return e } +// WithSystemOnly indicates system-usage only and can be chained. +func (e *EnumSetting) WithSystemOnly() *EnumSetting { + e.common.systemOnly = true + return e +} + +// Defeat the linter. +var _ = (*EnumSetting).WithSystemOnly + // RegisterEnumSetting defines a new setting with type int. func RegisterEnumSetting( key, desc string, defaultValue string, enumValues map[int64]string, diff --git a/pkg/settings/float.go b/pkg/settings/float.go index 90e39481bb93..b623c3026096 100644 --- a/pkg/settings/float.go +++ b/pkg/settings/float.go @@ -94,6 +94,15 @@ func (f *FloatSetting) setToDefault(sv *Values) { } } +// WithSystemOnly indicates system-usage only and can be chained. +func (f *FloatSetting) WithSystemOnly() *FloatSetting { + f.common.systemOnly = true + return f +} + +// Defeat the linter. +var _ = (*FloatSetting).WithSystemOnly + // RegisterFloatSetting defines a new setting with type float. func RegisterFloatSetting( key, desc string, defaultValue float64, validateFns ...func(float64) error, diff --git a/pkg/settings/int.go b/pkg/settings/int.go index 1486e3d645df..b6918f1cce59 100644 --- a/pkg/settings/int.go +++ b/pkg/settings/int.go @@ -123,6 +123,15 @@ func (i *IntSetting) WithPublic() *IntSetting { return i } +// WithSystemOnly system-only usage and can be chained. +func (i *IntSetting) WithSystemOnly() *IntSetting { + i.common.systemOnly = true + return i +} + +// Defeat the linter. +var _ = (*IntSetting).WithSystemOnly + // PositiveInt can be passed to RegisterIntSetting func PositiveInt(v int64) error { if v < 1 { diff --git a/pkg/settings/masked.go b/pkg/settings/masked.go index b67639ca74c1..cc4ebba774a8 100644 --- a/pkg/settings/masked.go +++ b/pkg/settings/masked.go @@ -47,3 +47,8 @@ func (s *MaskedSetting) Description() string { func (s *MaskedSetting) Typ() string { return s.setting.Typ() } + +// SystemOnly returns the underlying setting's SystemOnly. +func (s *MaskedSetting) SystemOnly() bool { + return s.setting.SystemOnly() +} diff --git a/pkg/settings/setting.go b/pkg/settings/setting.go index 4e7ea499021e..3bc3a5140127 100644 --- a/pkg/settings/setting.go +++ b/pkg/settings/setting.go @@ -201,6 +201,9 @@ type Setting interface { // Reserved settings are still accessible to users, but they don't get // listed out when retrieving all settings. Visibility() Visibility + + // SystemOnly indicates if a setting is only applicable to the system tenant. + SystemOnly() bool } // WritableSetting is the exported interface of non-masked settings. @@ -263,6 +266,7 @@ const ( type common struct { description string visibility Visibility + systemOnly bool // Each setting has a slotIdx which is used as a handle with Values. slotIdx int nonReportable bool @@ -298,6 +302,10 @@ func (i common) Visibility() Visibility { return i.visibility } +func (i common) SystemOnly() bool { + return i.systemOnly +} + func (i common) isReportable() bool { return !i.nonReportable } diff --git a/pkg/sql/set_cluster_setting.go b/pkg/sql/set_cluster_setting.go index 5376b7a702cb..bbe34c0400cb 100644 --- a/pkg/sql/set_cluster_setting.go +++ b/pkg/sql/set_cluster_setting.go @@ -93,6 +93,11 @@ func (p *planner) SetClusterSetting( return nil, errors.AssertionFailedf("expected writable setting, got %T", v) } + if setting.SystemOnly() && !p.execCfg.Codec.ForSystemTenant() { + return nil, pgerror.Newf(pgcode.InsufficientPrivilege, + "setting %s is only setable in the system tenant", name) + } + var value tree.TypedExpr if n.Value != nil { // For DEFAULT, let the value reference be nil. That's a RESET in disguise.