settings: assert that SystemOnly settings are not accessed in virtual…

… clusters

TLDR: tests will now fail (with a panic) if code running in virtual
clusters mistakenly attempts to access a SystemOnly cluster setting.

Prior to this patch, it was possible for code (e.g. in SQL) running
inside a virtual cluster to access a `SystemOnly` setting. This resulted
in silently incorrect behavior: the default value of the setting would
be observed always, without any relationship to values explicitly set
in the storage layer.

When the setting mechanisms for virtual clusters had been implemented
orignally, this concern was known. We even had implemented a mechanism
on `settings.Values` (`SetNonSystemTenant()`) by which we could cause
further `.Get()` calls to fail with an error in tests if accessing a
`SystemOnly` setting from a virtual cluster.

Sadly, this mechanism was never hooked up anywhere.

This patch closes that loop by annotating the `settings.Values`
properly during server initialization.

Since the property is stored inside the `settings.Values`, this means
it is not any more possible to share a single `settings.Values`
instance (nor `cluster.Settings`) between the system tenant and a
secondary tenant.

Release note: None

pkg/kv/kvserver/replica_follower_read.go

@@ -27,7 +27,7 @@ import (
 // information is collected and passed around, regardless of the value of this
 // setting.
 var FollowerReadsEnabled = settings.RegisterBoolSetting(
-	settings.SystemOnly,
+	settings.TenantReadOnly, // needed for planning in SQL
 	"kv.closed_timestamp.follower_reads_enabled",
 	"allow (all) replicas to serve consistent historical reads based on closed timestamp information",
 	true,

pkg/server/server.go

@@ -235,6 +235,10 @@ func NewServer(cfg Config, stopper *stop.Stopper) (serverctl.ServerStartupInterf
 
 	st := cfg.Settings
 
+	// Ensure that we don't mistakenly reuse the same Values container
+	// across servers (e.g. misuse of TestServer API).
+	st.SV.SpecializeForSystemInterface()
+
 	if cfg.AmbientCtx.Tracer == nil {
 		panic(errors.New("no tracer set in AmbientCtx"))
 	}

pkg/server/settingswatcher/settings_watcher_external_test.go

@@ -152,7 +152,7 @@ func TestSettingWatcherOnTenant(t *testing.T) {
 	copySettingsFromSystemToFakeTenant()
 
 	tenantSettings := cluster.MakeTestingClusterSettings()
-	tenantSettings.SV.SetNonSystemTenant()
+	tenantSettings.SV.SpecializeForVirtualCluster()
 
 	// Needed for backward-compat on crdb_internal.ranges{_no_leases}.
 	// Remove in v23.2.

pkg/server/tenant.go

@@ -1012,6 +1012,10 @@ func makeTenantSQLServerArgs(
 ) (sqlServerArgs, error) {
 	st := baseCfg.Settings
 
+	// Ensure that the settings accessor bark if an attempt is made
+	// to use a SystemOnly setting.
+	st.SV.SpecializeForVirtualCluster()
+
 	// We want all log messages issued on behalf of this SQL instance to report
 	// the instance ID (once known) as a tag.
 	startupCtx = baseCfg.AmbientCtx.AnnotateCtx(startupCtx)

pkg/settings/settings_test.go

@@ -802,13 +802,13 @@ func TestOverride(t *testing.T) {
 	require.Equal(t, 42.0, overrideFloat.Get(sv))
 }
 
-func TestSystemOnlyDisallowedOnTenant(t *testing.T) {
+func TestSystemOnlyDisallowedOnVirtualCluster(t *testing.T) {
 	skip.UnderNonTestBuild(t)
 
 	ctx := context.Background()
 	sv := &settings.Values{}
 	sv.Init(ctx, settings.TestOpaque)
-	sv.SetNonSystemTenant()
+	sv.SpecializeForVirtualCluster()
 
 	// Check that we can still read non-SystemOnly settings.
 	if expected, actual := "", strFooA.Get(sv); expected != actual {

pkg/settings/updater.go

@@ -175,7 +175,7 @@ func (u updater) ResetRemaining(ctx context.Context) {
 			u.sv.setValueOrigin(ctx, v.getSlot(), OriginDefault)
 		}
 
-		if u.sv.NonSystemTenant() && v.Class() == SystemOnly {
+		if u.sv.SpecializedToVirtualCluster() && v.Class() == SystemOnly {
 			// Don't try to reset system settings on a non-system tenant.
 			continue
 		}

pkg/settings/values.go

@@ -30,7 +30,7 @@ const MaxSettings = 1023
 type Values struct {
 	container valuesContainer
 
-	nonSystemTenant bool
+	classCheck classCheck
 
 	defaultOverridesMu struct {
 		syncutil.Mutex
@@ -51,6 +51,22 @@ type Values struct {
 	opaque interface{}
 }
 
+type classCheck int8
+
+const (
+	// classCheckUndefined is used when the settings.Values hasn't been
+	// specialized yet.
+	classCheckUndefined classCheck = iota
+	// classCheckSystemInterface is used when the settings.Values is
+	// specialized for the system interface and SystemOnly settings can
+	// be used.
+	classCheckSystemInterface
+	// classCheckVirtualCluster is used when the settings.Values is
+	// specialized for a virtual cluster and SystemOnly settings cannot
+	// be used.
+	classCheckVirtualCluster
+)
+
 const numSlots = MaxSettings + 1
 
 type valuesContainer struct {
@@ -119,21 +135,34 @@ 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
+// SpecializeForSystemInterface marks the values container as
+// pertaining to the system interface.
+func (sv *Values) SpecializeForSystemInterface() {
+	if sv.classCheck != classCheckUndefined && sv.classCheck != classCheckSystemInterface {
+		panic(errors.AssertionFailedf("setting value container is already specialized"))
+	}
+	sv.classCheck = classCheckSystemInterface
+}
+
+// SpecializeForVirtualCluster marks this container as pertaining to
+// a virtual cluster, after which use of SystemOnly values is
+// disallowed.
+func (sv *Values) SpecializeForVirtualCluster() {
+	if sv.classCheck != classCheckUndefined && sv.classCheck != classCheckVirtualCluster {
+		panic(errors.AssertionFailedf("setting value container is already specialized"))
+	}
+	sv.classCheck = classCheckVirtualCluster
 	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
+// SpecializedToVirtualCluster returns true if this container is for a
+// virtual cluster (i.e. SpecializeToVirtualCluster() was called).
+func (sv *Values) SpecializedToVirtualCluster() bool {
+	return sv.classCheck == classCheckVirtualCluster
 }
 
 // Opaque returns the argument passed to Init.

pkg/sql/vars.go

@@ -1613,7 +1613,8 @@ var varGen = map[string]sessionVar{
 			// - The session var is named "disable_" because we want the Go
 			//   default value (false) to mean that tenant deletion is enabled.
 			//   This is needed for backward-compatibility with Cockroach Cloud.
-			return formatBoolAsPostgresSetting(!enableDropVirtualCluster.Get(sv))
+			enabled := !sv.SpecializedToVirtualCluster() && !enableDropVirtualCluster.Get(sv)
+			return formatBoolAsPostgresSetting(enabled)
 		},
 	},
 

pkg/storage/pebble.go

@@ -123,7 +123,7 @@ var UseEFOS = settings.RegisterBoolSetting(
 //
 // This cluster setting will be removed in a subsequent release.
 var IngestAsFlushable = settings.RegisterBoolSetting(
-	settings.SystemOnly,
+	settings.TenantWritable, // used to init temp storage in virtual cluster servers.
 	"storage.ingest_as_flushable.enabled",
 	"set to true to enable lazy ingestion of sstables",
 	util.ConstantWithMetamorphicTestBool(

pkg/util/schedulerlatency/sampler.go

@@ -34,7 +34,7 @@ import (
 // requests in work queues as part of this tick). Might be worth checking for
 // grantees more frequently independent of this sample period.
 var samplePeriod = settings.RegisterDurationSetting(
-	settings.SystemOnly,
+	settings.TenantWritable, // used in virtual clusters
 	"scheduler_latency.sample_period",
 	"controls the duration between consecutive scheduler latency samples",
 	100*time.Millisecond,
@@ -47,7 +47,7 @@ var samplePeriod = settings.RegisterDurationSetting(
 )
 
 var sampleDuration = settings.RegisterDurationSetting(
-	settings.SystemOnly,
+	settings.TenantWritable, // used in virtual clusters
 	"scheduler_latency.sample_duration",
 	"controls the duration over which each scheduler latency sample is a measurement over",
 	2500*time.Millisecond,