setting: prevent use of SystemOnly settings from tenant code

This change implements the `system-only` semantics in the RFC
(#73349).

All SystemOnly setting values are now invisible from tenant code. If
tenant code attempts to get or set a SystemOnly setting by handle, it
results in panics in test builds; in non-test builds, these settings
always report the default value.

Release note (sql change): System-only cluster settings are no longer
visible for non-system tenants.

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",

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) {

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))
 			}

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
 		}

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)
 		}

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

pkg/server/settingswatcher/settings_watcher.go

@@ -128,27 +128,40 @@ 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,
 		})
 		if err != nil {
 			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

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)
 

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",

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
 // <redacted> 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 "<unknown>"

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 <setting-name>`.
 	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.