diff --git a/pkg/ccl/serverccl/BUILD.bazel b/pkg/ccl/serverccl/BUILD.bazel index f8e15ae1f7c4..4e1c3c41bff3 100644 --- a/pkg/ccl/serverccl/BUILD.bazel +++ b/pkg/ccl/serverccl/BUILD.bazel @@ -31,7 +31,6 @@ go_test( "//pkg/server/serverpb", "//pkg/sql", "//pkg/sql/distsql", - "//pkg/sql/pgwire/pgcode", "//pkg/sql/tests", "//pkg/testutils/serverutils", "//pkg/testutils/sqlutils", diff --git a/pkg/ccl/serverccl/server_sql_test.go b/pkg/ccl/serverccl/server_sql_test.go index 3c83807b5190..331fd3ddb3ff 100644 --- a/pkg/ccl/serverccl/server_sql_test.go +++ b/pkg/ccl/serverccl/server_sql_test.go @@ -14,6 +14,7 @@ import ( "fmt" "io/ioutil" "net/http" + "strings" "testing" "github.com/cockroachdb/cockroach/pkg/base" @@ -23,7 +24,6 @@ import ( "github.com/cockroachdb/cockroach/pkg/security" "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/distsql" - "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/testutils/testcluster" @@ -86,7 +86,9 @@ func TestTenantCannotSetClusterSetting(t *testing.T) { 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) + if !strings.Contains(pqErr.Message, "unknown cluster setting") { + t.Errorf("unexpected error: %v", err) + } } func TestTenantCanUseEnterpriseFeatures(t *testing.T) { diff --git a/pkg/cli/gen.go b/pkg/cli/gen.go index 47c8686b526c..72811d8ad60c 100644 --- a/pkg/cli/gen.go +++ b/pkg/cli/gen.go @@ -211,8 +211,8 @@ Output the list of cluster settings known to this binary. settings.NewUpdater(&s.SV).ResetRemaining(context.Background()) var rows [][]string - for _, name := range settings.Keys() { - setting, ok := settings.Lookup(name, settings.LookupForLocalAccess) + for _, name := range settings.Keys(settings.ForSystemTenant) { + setting, ok := settings.Lookup(name, settings.LookupForLocalAccess, settings.ForSystemTenant) if !ok { panic(fmt.Sprintf("could not find setting %q", name)) } diff --git a/pkg/server/admin.go b/pkg/server/admin.go index a98d5d6f38d9..a93c950a351a 100644 --- a/pkg/server/admin.go +++ b/pkg/server/admin.go @@ -1662,7 +1662,7 @@ func (s *adminServer) Settings( ) (*serverpb.SettingsResponse, error) { keys := req.Keys if len(keys) == 0 { - keys = settings.Keys() + keys = settings.Keys(settings.ForSystemTenant) } _, isAdmin, err := s.getUserAndRole(ctx) @@ -1686,7 +1686,7 @@ func (s *adminServer) Settings( resp := serverpb.SettingsResponse{KeyValues: make(map[string]serverpb.SettingsResponse_Value)} for _, k := range keys { - v, ok := settings.Lookup(k, lookupPurpose) + v, ok := settings.Lookup(k, lookupPurpose, settings.ForSystemTenant) if !ok { continue } diff --git a/pkg/server/admin_test.go b/pkg/server/admin_test.go index fb37402aeb55..e3b329df7afd 100644 --- a/pkg/server/admin_test.go +++ b/pkg/server/admin_test.go @@ -1074,10 +1074,10 @@ func TestAdminAPISettings(t *testing.T) { // Any bool that defaults to true will work here. const settingKey = "sql.metrics.statement_details.enabled" st := s.ClusterSettings() - allKeys := settings.Keys() + allKeys := settings.Keys(settings.ForSystemTenant) checkSetting := func(t *testing.T, k string, v serverpb.SettingsResponse_Value) { - ref, ok := settings.Lookup(k, settings.LookupForReporting) + ref, ok := settings.Lookup(k, settings.LookupForReporting, settings.ForSystemTenant) if !ok { t.Fatalf("%s: not found after initial lookup", k) } diff --git a/pkg/server/diagnostics/reporter.go b/pkg/server/diagnostics/reporter.go index 0e7b3965c58d..50646a48ecd3 100644 --- a/pkg/server/diagnostics/reporter.go +++ b/pkg/server/diagnostics/reporter.go @@ -209,7 +209,9 @@ func (r *Reporter) CreateReport( for ok, err = it.Next(ctx); ok; ok, err = it.Next(ctx) { row := it.Cur() name := string(tree.MustBeDString(row[0])) - info.AlteredSettings[name] = settings.RedactedValue(name, &r.Settings.SV) + info.AlteredSettings[name] = settings.RedactedValue( + name, &r.Settings.SV, r.TenantID == roachpb.SystemTenantID, + ) } if err != nil { // No need to clear AlteredSettings map since we only make best diff --git a/pkg/server/settingswatcher/settings_watcher.go b/pkg/server/settingswatcher/settings_watcher.go index 5a24b40b2252..b0602d1b12b2 100644 --- a/pkg/server/settingswatcher/settings_watcher.go +++ b/pkg/server/settingswatcher/settings_watcher.go @@ -128,7 +128,7 @@ func (s *SettingsWatcher) Start(ctx context.Context) error { rf, err := s.f.RangeFeed(ctx, "settings", []roachpb.Span{settingsTableSpan}, now, func( ctx context.Context, kv *roachpb.RangeFeedValue, ) { - setting, val, tombstone, err := s.dec.DecodeRow(roachpb.KeyValue{ + name, val, tombstone, err := s.dec.DecodeRow(roachpb.KeyValue{ Key: kv.Key, Value: kv.Value, }) @@ -136,19 +136,32 @@ func (s *SettingsWatcher) Start(ctx context.Context) error { log.Warningf(ctx, "failed to decode settings row %v: %v", kv.Key, err) return } + + if !s.codec.ForSystemTenant() { + setting, ok := settings.Lookup(name, settings.LookupForLocalAccess, s.codec.ForSystemTenant()) + if !ok { + log.Warningf(ctx, "unknown setting %s, skipping update", log.Safe(name)) + return + } + if setting.Class() != settings.TenantWritable { + log.Warningf(ctx, "ignoring read-only setting %s", log.Safe(name)) + return + } + } + s.mu.Lock() defer s.mu.Unlock() - _, hasOverride := s.mu.overrides[setting] + _, hasOverride := s.mu.overrides[name] if tombstone { // This event corresponds to a deletion. - delete(s.mu.values, setting) + delete(s.mu.values, name) if !hasOverride { - s.setDefault(ctx, u, setting) + s.setDefault(ctx, u, name) } } else { - s.mu.values[setting] = val + s.mu.values[name] = val if !hasOverride { - s.set(ctx, u, setting, val) + s.set(ctx, u, name, val) } } }, rangefeed.WithInitialScan(func(ctx context.Context) { @@ -218,7 +231,7 @@ func (s *SettingsWatcher) set(ctx context.Context, u settings.Updater, key strin // setDefault sets a setting to its default value. func (s *SettingsWatcher) setDefault(ctx context.Context, u settings.Updater, key string) { - setting, ok := settings.Lookup(key, settings.LookupForLocalAccess) + setting, ok := settings.Lookup(key, settings.LookupForLocalAccess, s.codec.ForSystemTenant()) if !ok { log.Warningf(ctx, "failed to find setting %s, skipping update", log.Safe(key)) return diff --git a/pkg/server/settingswatcher/settings_watcher_external_test.go b/pkg/server/settingswatcher/settings_watcher_external_test.go index 7405f453d401..23752cfb03a8 100644 --- a/pkg/server/settingswatcher/settings_watcher_external_test.go +++ b/pkg/server/settingswatcher/settings_watcher_external_test.go @@ -52,6 +52,9 @@ func TestSettingWatcherOnTenant(t *testing.T) { "kv.queue.process.guaranteed_time_budget": {"17s", "20s"}, "sql.txn_stats.sample_rate": {.23, .55}, "cluster.organization": {"foobar", "bazbax"}, + // Include a system-only setting to verify that we don't try to change its + // value (which would cause a panic in test builds). + "kv.snapshot_rebalance.max_rate": {1024, 2048}, } fakeTenant := roachpb.MakeTenantID(2) systemTable := keys.SystemSQLCodec.TablePrefix(keys.SettingsTableID) @@ -73,9 +76,12 @@ func TestSettingWatcherOnTenant(t *testing.T) { } } checkSettingsValuesMatch := func(a, b *cluster.Settings) error { - for _, k := range settings.Keys() { - s, ok := settings.Lookup(k, settings.LookupForLocalAccess) + for _, k := range settings.Keys(false /* forSystemTenant */) { + s, ok := settings.Lookup(k, settings.LookupForLocalAccess, false /* forSystemTenant */) require.True(t, ok) + if s.Class() == settings.SystemOnly { + continue + } if av, bv := s.String(&a.SV), s.String(&b.SV); av != bv { return errors.Errorf("values do not match for %s: %s != %s", k, av, bv) } @@ -87,18 +93,19 @@ func TestSettingWatcherOnTenant(t *testing.T) { } copySettingsFromSystemToFakeTenant() s0 := tc.Server(0) - fakeSettings := cluster.MakeTestingClusterSettings() - sw := settingswatcher.New(s0.Clock(), fakeCodec, fakeSettings, + tenantSettings := cluster.MakeTestingClusterSettings() + tenantSettings.SV.SetNonSystemTenant() + sw := settingswatcher.New(s0.Clock(), fakeCodec, tenantSettings, s0.ExecutorConfig().(sql.ExecutorConfig).RangeFeedFactory, tc.Stopper()) require.NoError(t, sw.Start(ctx)) - require.NoError(t, checkSettingsValuesMatch(s0.ClusterSettings(), fakeSettings)) + require.NoError(t, checkSettingsValuesMatch(s0.ClusterSettings(), tenantSettings)) for k, v := range toSet { tdb.Exec(t, "SET CLUSTER SETTING "+k+" = $1", v[1]) } copySettingsFromSystemToFakeTenant() testutils.SucceedsSoon(t, func() error { - return checkSettingsValuesMatch(s0.ClusterSettings(), fakeSettings) + return checkSettingsValuesMatch(s0.ClusterSettings(), tenantSettings) }) } @@ -133,14 +140,14 @@ func TestSettingsWatcherWithOverrides(t *testing.T) { expect := func(setting, value string) { t.Helper() - s, ok := settings.Lookup(setting, settings.LookupForLocalAccess) + s, ok := settings.Lookup(setting, settings.LookupForLocalAccess, settings.ForSystemTenant) require.True(t, ok) require.Equal(t, value, s.String(&st.SV)) } expectSoon := func(setting, value string) { t.Helper() - s, ok := settings.Lookup(setting, settings.LookupForLocalAccess) + s, ok := settings.Lookup(setting, settings.LookupForLocalAccess, settings.ForSystemTenant) require.True(t, ok) testutils.SucceedsSoon(t, func() error { if actual := s.String(&st.SV); actual != value { @@ -189,7 +196,7 @@ func TestSettingsWatcherWithOverrides(t *testing.T) { expectSoon("i1", "10") // Verify that version cannot be overridden. - version, ok := settings.Lookup("version", settings.LookupForLocalAccess) + version, ok := settings.Lookup("version", settings.LookupForLocalAccess, settings.ForSystemTenant) require.True(t, ok) versionValue := version.String(&st.SV) diff --git a/pkg/settings/BUILD.bazel b/pkg/settings/BUILD.bazel index 513fc9a378bc..7f52aef43c52 100644 --- a/pkg/settings/BUILD.bazel +++ b/pkg/settings/BUILD.bazel @@ -22,6 +22,7 @@ go_library( importpath = "github.com/cockroachdb/cockroach/pkg/settings", visibility = ["//visibility:public"], deps = [ + "//pkg/util/buildutil", "//pkg/util/humanizeutil", "//pkg/util/syncutil", "@com_github_cockroachdb_errors//:errors", @@ -35,6 +36,7 @@ go_test( deps = [ ":settings", "//pkg/testutils", + "//pkg/testutils/skip", "//pkg/util/protoutil", "@com_github_cockroachdb_errors//:errors", "@com_github_stretchr_testify//require", diff --git a/pkg/settings/registry.go b/pkg/settings/registry.go index e055c478e8b9..396b945eb5c4 100644 --- a/pkg/settings/registry.go +++ b/pkg/settings/registry.go @@ -27,6 +27,10 @@ import ( // read concurrently by different callers. var registry = make(map[string]internalSetting) +// slotTable stores the same settings as the registry, but accessible by the +// slot index. +var slotTable [MaxSettings]internalSetting + // TestingSaveRegistry can be used in tests to save/restore the current // contents of the registry. func TestingSaveRegistry() func() { @@ -145,16 +149,20 @@ func register(class Class, key, desc string, s internalSetting) { slot := slotIdx(len(registry)) s.init(class, key, desc, slot) registry[key] = s + slotTable[slot] = s } // NumRegisteredSettings returns the number of registered settings. func NumRegisteredSettings() int { return len(registry) } // Keys returns a sorted string array with all the known keys. -func Keys() (res []string) { +func Keys(forSystemTenant bool) (res []string) { res = make([]string, 0, len(registry)) - for k := range registry { - if registry[k].isRetired() { + for k, v := range registry { + if v.isRetired() { + continue + } + if !forSystemTenant && v.Class() == SystemOnly { continue } res = append(res, k) @@ -166,13 +174,18 @@ func Keys() (res []string) { // Lookup returns a Setting by name along with its description. // For non-reportable setting, it instantiates a MaskedSetting // to masquerade for the underlying setting. -func Lookup(name string, purpose LookupPurpose) (Setting, bool) { - v, ok := registry[name] - var setting Setting = v - if ok && purpose == LookupForReporting && !v.isReportable() { - setting = &MaskedSetting{setting: v} +func Lookup(name string, purpose LookupPurpose, forSystemTenant bool) (Setting, bool) { + s, ok := registry[name] + if !ok { + return nil, false } - return setting, ok + if !forSystemTenant && s.Class() == SystemOnly { + return nil, false + } + if purpose == LookupForReporting && !s.isReportable() { + return &MaskedSetting{setting: s}, true + } + return s, true } // LookupPurpose indicates what is being done with the setting. @@ -188,6 +201,10 @@ const ( LookupForLocalAccess ) +// ForSystemTenant can be passed to Lookup for code that runs only on the system +// tenant. +const ForSystemTenant = true + // ReadableTypes maps our short type identifiers to friendlier names. var ReadableTypes = map[string]string{ "s": "string", @@ -204,8 +221,8 @@ var ReadableTypes = map[string]string{ // RedactedValue returns a string representation of the value for settings // types the are not considered sensitive (numbers, bools, etc) or // for those with values could store sensitive things (i.e. strings). -func RedactedValue(name string, values *Values) string { - if setting, ok := Lookup(name, LookupForReporting); ok { +func RedactedValue(name string, values *Values, forSystemTenant bool) string { + if setting, ok := Lookup(name, LookupForReporting, forSystemTenant); ok { return setting.String(values) } return "" diff --git a/pkg/settings/setting.go b/pkg/settings/setting.go index a9b74673e9a9..db8f777c3c20 100644 --- a/pkg/settings/setting.go +++ b/pkg/settings/setting.go @@ -22,6 +22,12 @@ import ( // multiple "instances" (values) for each setting (e.g. for multiple test // servers in the same process). type Setting interface { + // Class returns the scope of the setting in multi-tenant scenarios. + Class() Class + + // Key returns the name of the specific cluster setting. + Key() string + // Typ returns the short (1 char) string denoting the type of setting. Typ() string @@ -30,9 +36,6 @@ type Setting interface { // CLUSTER SETTING `. String(sv *Values) string - // Key returns the name of the specific cluster setting. - Key() string - // Description contains a helpful text explaining what the specific cluster // setting is for. Description() string @@ -41,9 +44,6 @@ type Setting interface { // Reserved settings are still accessible to users, but they don't get listed // out when retrieving all settings. Visibility() Visibility - - // Class returns the scope of the setting in multi-tenant scenarios. - Class() Class } // NonMaskedSetting is the exported interface of non-masked settings. diff --git a/pkg/settings/settings_test.go b/pkg/settings/settings_test.go index c7c9100cd6c2..9e0af4ad256c 100644 --- a/pkg/settings/settings_test.go +++ b/pkg/settings/settings_test.go @@ -20,6 +20,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/settings" "github.com/cockroachdb/cockroach/pkg/testutils" + "github.com/cockroachdb/cockroach/pkg/testutils/skip" "github.com/cockroachdb/cockroach/pkg/util/protoutil" "github.com/cockroachdb/errors" "github.com/stretchr/testify/require" @@ -152,11 +153,11 @@ var strFooA = settings.RegisterStringSetting(settings.TenantWritable, "str.foo", var strBarA = settings.RegisterStringSetting(settings.SystemOnly, "str.bar", "desc", "bar") var i1A = settings.RegisterIntSetting(settings.TenantWritable, "i.1", "desc", 0) var i2A = settings.RegisterIntSetting(settings.TenantWritable, "i.2", "desc", 5) -var fA = settings.RegisterFloatSetting(settings.TenantWritable, "f", "desc", 5.4) +var fA = settings.RegisterFloatSetting(settings.TenantReadOnly, "f", "desc", 5.4) var dA = settings.RegisterDurationSetting(settings.TenantWritable, "d", "desc", time.Second) var duA = settings.RegisterPublicDurationSettingWithExplicitUnit(settings.TenantWritable, "d_with_explicit_unit", "desc", time.Second, settings.NonNegativeDuration) var _ = settings.RegisterDurationSetting(settings.TenantWritable, "d_with_maximum", "desc", time.Second, settings.NonNegativeDurationWithMaximum(time.Hour)) -var eA = settings.RegisterEnumSetting(settings.TenantWritable, "e", "desc", "foo", map[int64]string{1: "foo", 2: "bar", 3: "baz"}) +var eA = settings.RegisterEnumSetting(settings.SystemOnly, "e", "desc", "foo", map[int64]string{1: "foo", 2: "bar", 3: "baz"}) var byteSize = settings.RegisterByteSizeSetting(settings.TenantWritable, "zzz", "desc", mb) var mA = func() *settings.VersionSetting { s := settings.MakeVersionSetting(&dummyVersionSettingImpl{}) @@ -346,36 +347,34 @@ func TestCache(t *testing.T) { // doesn't have one and it would crash. }) - t.Run("lookup", func(t *testing.T) { - if actual, ok := settings.Lookup("i.1", settings.LookupForLocalAccess); !ok || i1A != actual { - t.Fatalf("expected %v, got %v (exists: %v)", i1A, actual, ok) - } - if actual, ok := settings.Lookup("i.Val", settings.LookupForLocalAccess); !ok || iVal != actual { - t.Fatalf("expected %v, got %v (exists: %v)", iVal, actual, ok) - } - if actual, ok := settings.Lookup("f", settings.LookupForLocalAccess); !ok || fA != actual { - t.Fatalf("expected %v, got %v (exists: %v)", fA, actual, ok) - } - if actual, ok := settings.Lookup("fVal", settings.LookupForLocalAccess); !ok || fVal != actual { - t.Fatalf("expected %v, got %v (exists: %v)", fVal, actual, ok) - } - if actual, ok := settings.Lookup("d", settings.LookupForLocalAccess); !ok || dA != actual { - t.Fatalf("expected %v, got %v (exists: %v)", dA, actual, ok) - } - if actual, ok := settings.Lookup("dVal", settings.LookupForLocalAccess); !ok || dVal != actual { - t.Fatalf("expected %v, got %v (exists: %v)", dVal, actual, ok) - } - if actual, ok := settings.Lookup("e", settings.LookupForLocalAccess); !ok || eA != actual { - t.Fatalf("expected %v, got %v (exists: %v)", eA, actual, ok) - } - if actual, ok := settings.Lookup("v.1", settings.LookupForLocalAccess); !ok || mA != actual { - t.Fatalf("expected %v, got %v (exists: %v)", mA, actual, ok) + t.Run("lookup-system", func(t *testing.T) { + for _, s := range []settings.Setting{i1A, iVal, fA, fVal, dA, dVal, eA, mA, duA} { + result, ok := settings.Lookup(s.Key(), settings.LookupForLocalAccess, settings.ForSystemTenant) + if !ok { + t.Fatalf("lookup(%s) failed", s.Key()) + } + if result != s { + t.Fatalf("expected %v, got %v", s, result) + } } - if actual, ok := settings.Lookup("d_with_explicit_unit", settings.LookupForLocalAccess); !ok || duA != actual { - t.Fatalf("expected %v, got %v (exists: %v)", duA, actual, ok) + }) + t.Run("lookup-tenant", func(t *testing.T) { + for _, s := range []settings.Setting{i1A, fA, dA, duA} { + result, ok := settings.Lookup(s.Key(), settings.LookupForLocalAccess, false /* forSystemTenant */) + if !ok { + t.Fatalf("lookup(%s) failed", s.Key()) + } + if result != s { + t.Fatalf("expected %v, got %v", s, result) + } } - if actual, ok := settings.Lookup("dne", settings.LookupForLocalAccess); ok { - t.Fatalf("expected nothing, got %v", actual) + }) + t.Run("lookup-tenant-fail", func(t *testing.T) { + for _, s := range []settings.Setting{iVal, fVal, dVal, eA, mA} { + _, ok := settings.Lookup(s.Key(), settings.LookupForLocalAccess, false /* forSystemTenant */) + if ok { + t.Fatalf("lookup(%s) should have failed", s.Key()) + } } }) @@ -687,10 +686,14 @@ func TestCache(t *testing.T) { } func TestIsReportable(t *testing.T) { - if v, ok := settings.Lookup("bool.t", settings.LookupForLocalAccess); !ok || !settings.TestingIsReportable(v) { + if v, ok := settings.Lookup( + "bool.t", settings.LookupForLocalAccess, settings.ForSystemTenant, + ); !ok || !settings.TestingIsReportable(v) { t.Errorf("expected 'bool.t' to be marked as isReportable() = true") } - if v, ok := settings.Lookup("sekretz", settings.LookupForLocalAccess); !ok || settings.TestingIsReportable(v) { + if v, ok := settings.Lookup( + "sekretz", settings.LookupForLocalAccess, settings.ForSystemTenant, + ); !ok || settings.TestingIsReportable(v) { t.Errorf("expected 'sekretz' to be marked as isReportable() = false") } } @@ -707,7 +710,7 @@ func TestOnChangeWithMaxSettings(t *testing.T) { sv := &settings.Values{} sv.Init(ctx, settings.TestOpaque) var changes int - s, ok := settings.Lookup(maxName, settings.LookupForLocalAccess) + s, ok := settings.Lookup(maxName, settings.LookupForLocalAccess, settings.ForSystemTenant) if !ok { t.Fatalf("expected lookup of %s to succeed", maxName) } @@ -798,6 +801,31 @@ func TestOverride(t *testing.T) { require.Equal(t, 42.0, overrideFloat.Get(sv)) } +func TestSystemOnlyDisallowedOnTenant(t *testing.T) { + skip.UnderNonTestBuild(t) + + ctx := context.Background() + sv := &settings.Values{} + sv.Init(ctx, settings.TestOpaque) + sv.SetNonSystemTenant() + + // Check that we can still read non-SystemOnly settings. + if expected, actual := "", strFooA.Get(sv); expected != actual { + t.Fatalf("expected %v, got %v", expected, actual) + } + + func() { + defer func() { + if r := recover(); r == nil { + t.Error("Get did not panic") + } else if !strings.Contains(fmt.Sprint(r), "attempted to set forbidden setting") { + t.Errorf("received unexpected panic: %v", r) + } + }() + strBarA.Get(sv) + }() +} + func setDummyVersion(dv dummyVersion, vs *settings.VersionSetting, sv *settings.Values) error { // This is a bit round about because the VersionSetting doesn't get updated // through the updater, like most other settings. In order to set it, we set diff --git a/pkg/settings/updater.go b/pkg/settings/updater.go index 12016a41e15c..9499c809ad09 100644 --- a/pkg/settings/updater.go +++ b/pkg/settings/updater.go @@ -136,6 +136,10 @@ func (u updater) Set(ctx context.Context, key, rawValue string, vt string) error // ResetRemaining sets all settings not updated by the updater to their default values. func (u updater) ResetRemaining(ctx context.Context) { for k, v := range registry { + if u.sv.NonSystemTenant() && v.Class() == SystemOnly { + // Don't try to reset system settings on a non-system tenant. + continue + } if _, ok := u.m[k]; !ok { v.setToDefault(ctx, u.sv) } diff --git a/pkg/settings/values.go b/pkg/settings/values.go index 6cc656281ef6..dae86127e37e 100644 --- a/pkg/settings/values.go +++ b/pkg/settings/values.go @@ -14,12 +14,14 @@ import ( "context" "sync/atomic" + "github.com/cockroachdb/cockroach/pkg/util/buildutil" "github.com/cockroachdb/cockroach/pkg/util/syncutil" + "github.com/cockroachdb/errors" ) // MaxSettings is the maximum number of settings that the system supports. // Exported for tests. -const MaxSettings = 512 +const MaxSettings = 511 // Values is a container that stores values for all registered settings. // Each setting is assigned a unique slot (up to MaxSettings). @@ -28,6 +30,8 @@ const MaxSettings = 512 type Values struct { container valuesContainer + nonSystemTenant bool + overridesMu struct { syncutil.Mutex // defaultOverrides maintains the set of overridden default values (see @@ -48,19 +52,55 @@ type Values struct { opaque interface{} } +const numSlots = MaxSettings + 1 + type valuesContainer struct { - intVals [MaxSettings]int64 - genericVals [MaxSettings]atomic.Value + intVals [numSlots]int64 + genericVals [numSlots]atomic.Value + + // If forbidden[slot] is true, that setting is not allowed to be used from the + // current context (i.e. it is a SystemOnly setting and the container is for a + // tenant). Reading or writing such a setting causes panics in test builds. + forbidden [numSlots]bool } func (c *valuesContainer) setGenericVal(slot slotIdx, newVal interface{}) { + if !c.checkForbidden(slot) { + return + } c.genericVals[slot].Store(newVal) } -func (c *valuesContainer) setInt64Val(slot slotIdx, newVal int64) bool { +func (c *valuesContainer) setInt64Val(slot slotIdx, newVal int64) (changed bool) { + if !c.checkForbidden(slot) { + return false + } return atomic.SwapInt64(&c.intVals[slot], newVal) != newVal } +func (c *valuesContainer) getInt64(slot slotIdx) int64 { + c.checkForbidden(slot) + return atomic.LoadInt64(&c.intVals[slot]) +} + +func (c *valuesContainer) getGeneric(slot slotIdx) interface{} { + c.checkForbidden(slot) + return c.genericVals[slot].Load() +} + +// checkForbidden checks if the setting in the given slot is allowed to be used +// from the current context. If not, it panics in test builds and returns false +// in non-test builds. +func (c *valuesContainer) checkForbidden(slot slotIdx) bool { + if c.forbidden[slot] { + if buildutil.CrdbTestBuild { + panic(errors.AssertionFailedf("attempted to set forbidden setting %s", slotTable[slot].Key())) + } + return false + } + return true +} + type testOpaqueType struct{} // TestOpaque can be passed to Values.Init when we are testing the settings @@ -78,6 +118,23 @@ func (sv *Values) Init(ctx context.Context, opaque interface{}) { } } +// SetNonSystemTenant marks this container as pertaining to a non-system tenant, +// after which use of SystemOnly values is disallowed. +func (sv *Values) SetNonSystemTenant() { + sv.nonSystemTenant = true + for slot, setting := range slotTable { + if setting != nil && setting.Class() == SystemOnly { + sv.container.forbidden[slot] = true + } + } +} + +// NonSystemTenant returns true if this container is for a non-system tenant +// (i.e. SetNonSystemTenant() was called). +func (sv *Values) NonSystemTenant() bool { + return sv.nonSystemTenant +} + // Opaque returns the argument passed to Init. func (sv *Values) Opaque() interface{} { return sv.opaque @@ -92,14 +149,6 @@ func (sv *Values) settingChanged(ctx context.Context, slot slotIdx) { } } -func (c *valuesContainer) getInt64(slot slotIdx) int64 { - return atomic.LoadInt64(&c.intVals[slot]) -} - -func (c *valuesContainer) getGeneric(slot slotIdx) interface{} { - return c.genericVals[slot].Load() -} - func (sv *Values) setInt64(ctx context.Context, slot slotIdx, newVal int64) { if sv.container.setInt64Val(slot, newVal) { sv.settingChanged(ctx, slot) diff --git a/pkg/sql/crdb_internal.go b/pkg/sql/crdb_internal.go index 4b61bc889d5a..ce463429ff78 100644 --- a/pkg/sql/crdb_internal.go +++ b/pkg/sql/crdb_internal.go @@ -1411,11 +1411,13 @@ CREATE TABLE crdb_internal.cluster_settings ( "crdb_internal.cluster_settings", roleoption.MODIFYCLUSTERSETTING) } } - for _, k := range settings.Keys() { + for _, k := range settings.Keys(p.ExecCfg().Codec.ForSystemTenant()) { if !hasAdmin && settings.AdminOnly(k) { continue } - setting, _ := settings.Lookup(k, settings.LookupForLocalAccess) + setting, _ := settings.Lookup( + k, settings.LookupForLocalAccess, p.ExecCfg().Codec.ForSystemTenant(), + ) strVal := setting.String(&p.ExecCfg().Settings.SV) isPublic := setting.Visibility() == settings.Public desc := setting.Description() diff --git a/pkg/sql/logictest/testdata/logic_test/cluster_settings b/pkg/sql/logictest/testdata/logic_test/cluster_settings index 8b90abd173bf..6b1cae3081d9 100644 --- a/pkg/sql/logictest/testdata/logic_test/cluster_settings +++ b/pkg/sql/logictest/testdata/logic_test/cluster_settings @@ -98,3 +98,21 @@ query B SHOW CLUSTER SETTING sql.defaults.stub_catalog_tables.enabled ---- true + +skipif config 3node-tenant +statement ok +SET CLUSTER SETTING kv.snapshot_rebalance.max_rate = '10Mib' + +skipif config 3node-tenant +query T +SHOW CLUSTER SETTING kv.snapshot_rebalance.max_rate +---- +10 MiB + +onlyif config 3node-tenant +statement error unknown cluster setting +SET CLUSTER SETTING kv.snapshot_rebalance.max_rate = '10Mib' + +onlyif config 3node-tenant +statement error unknown setting +SHOW CLUSTER SETTING kv.snapshot_rebalance.max_rate diff --git a/pkg/sql/set_cluster_setting.go b/pkg/sql/set_cluster_setting.go index e3083b01b363..0ac165cb9c2f 100644 --- a/pkg/sql/set_cluster_setting.go +++ b/pkg/sql/set_cluster_setting.go @@ -82,7 +82,7 @@ func (p *planner) SetClusterSetting( ) (planNode, error) { name := strings.ToLower(n.Name) st := p.EvalContext().Settings - v, ok := settings.Lookup(name, settings.LookupForLocalAccess) + v, ok := settings.Lookup(name, settings.LookupForLocalAccess, p.ExecCfg().Codec.ForSystemTenant()) if !ok { return nil, errors.Errorf("unknown cluster setting '%s'", name) } diff --git a/pkg/sql/show_cluster_setting.go b/pkg/sql/show_cluster_setting.go index 375fe6144933..511cb122054c 100644 --- a/pkg/sql/show_cluster_setting.go +++ b/pkg/sql/show_cluster_setting.go @@ -119,7 +119,9 @@ func (p *planner) ShowClusterSetting( ) (planNode, error) { name := strings.ToLower(n.Name) st := p.ExecCfg().Settings - val, ok := settings.Lookup(name, settings.LookupForLocalAccess) + val, ok := settings.Lookup( + name, settings.LookupForLocalAccess, p.ExecCfg().Codec.ForSystemTenant(), + ) if !ok { return nil, errors.Errorf("unknown setting: %q", name) } diff --git a/pkg/testutils/lint/lint_test.go b/pkg/testutils/lint/lint_test.go index dbf2fbab3fca..091f7556f3a0 100644 --- a/pkg/testutils/lint/lint_test.go +++ b/pkg/testutils/lint/lint_test.go @@ -1509,6 +1509,7 @@ func TestLint(t *testing.T) { case strings.HasSuffix(s, "protoutil"): case strings.HasSuffix(s, "testutils"): case strings.HasSuffix(s, "syncutil"): + case strings.HasSuffix(s, "buildutil"): case strings.HasSuffix(s, settingsPkgPrefix): default: t.Errorf("%s <- please don't add CRDB dependencies to settings pkg", s) diff --git a/pkg/testutils/skip/BUILD.bazel b/pkg/testutils/skip/BUILD.bazel index fe70040bdd17..98be6766db40 100644 --- a/pkg/testutils/skip/BUILD.bazel +++ b/pkg/testutils/skip/BUILD.bazel @@ -11,6 +11,7 @@ go_library( deps = [ "//pkg/build/bazel", "//pkg/util", + "//pkg/util/buildutil", "//pkg/util/envutil", "//pkg/util/syncutil", ], diff --git a/pkg/testutils/skip/skip.go b/pkg/testutils/skip/skip.go index 34fe926c8976..6be83ff2bb5f 100644 --- a/pkg/testutils/skip/skip.go +++ b/pkg/testutils/skip/skip.go @@ -17,6 +17,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/build/bazel" "github.com/cockroachdb/cockroach/pkg/util" + "github.com/cockroachdb/cockroach/pkg/util/buildutil" "github.com/cockroachdb/cockroach/pkg/util/syncutil" ) @@ -136,6 +137,14 @@ func UnderMetamorphic(t SkippableTest, args ...interface{}) { } } +// UnderNonTestBuild skips this test if the build does not have the crdb_test +// tag. +func UnderNonTestBuild(t SkippableTest) { + if !buildutil.CrdbTestBuild { + t.Skip("crdb_test tag required for this test") + } +} + // UnderBench returns true iff a test is currently running under `go // test -bench`. When true, tests should avoid writing data on // stdout/stderr from goroutines that run asynchronously with the