multitenant: add SQL server startup guardrails

pkg/ccl/serverccl/BUILD.bazel

@@ -39,6 +39,7 @@ go_test(
         "role_authentication_test.go",
         "server_controller_test.go",
         "server_sql_test.go",
+        "server_startup_guardrails_test.go",
         "tenant_decommissioned_host_test.go",
         "tenant_vars_test.go",
     ],
@@ -50,6 +51,7 @@ go_test(
         "//pkg/ccl",
         "//pkg/ccl/kvccl",
         "//pkg/ccl/utilccl/licenseccl",
+        "//pkg/clusterversion",
         "//pkg/kv/kvserver/liveness",
         "//pkg/kv/kvserver/liveness/livenesspb",
         "//pkg/roachpb",
@@ -65,6 +67,7 @@ go_test(
         "//pkg/sql",
         "//pkg/sql/lexbase",
         "//pkg/sql/sem/catconstants",
+        "//pkg/sql/sem/eval",
         "//pkg/sql/sqlinstance/instancestorage",
         "//pkg/sql/tests",
         "//pkg/testutils",

pkg/ccl/serverccl/server_startup_guardrails_test.go

@@ -0,0 +1,123 @@
+// Copyright 2023 The Cockroach Authors.
+//
+// Licensed as a CockroachDB Enterprise file under the Cockroach Community
+// License (the "License"); you may not use this file except in compliance with
+// the License. You may obtain a copy of the License at
+//
+//     https://github.com/cockroachdb/cockroach/blob/master/licenses/CCL.txt
+
+package serverccl
+
+import (
+	"context"
+	"fmt"
+	"testing"
+
+	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
+	"github.com/cockroachdb/cockroach/pkg/roachpb"
+	"github.com/cockroachdb/cockroach/pkg/server"
+	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/eval"
+	"github.com/cockroachdb/cockroach/pkg/testutils"
+	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
+	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+)
+
+// TestServerStartupGuardrails ensures that a SQL server will fail to start if
+// its binary version (TBV) is less than the tenant's logical version (TLV).
+func TestServerStartupGuardrails(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+
+	v := func(major, minor int32) roachpb.Version {
+		return roachpb.Version{Major: clusterversion.DevOffset + major, Minor: minor}
+	}
+
+	tests := []struct {
+		storageBinaryVersion             roachpb.Version
+		storageBinaryMinSupportedVersion roachpb.Version
+		tenantBinaryVersion              roachpb.Version
+		tenantBinaryMinSupportedVersion  roachpb.Version
+		TenantLogicalVersionKey          clusterversion.Key
+		expErrMatch                      string // empty if expecting a nil error
+	}{
+		// First test case ensures that a tenant server can start if the server binary
+		// version is not too low for the tenant logical version.
+		{
+			storageBinaryVersion:             v(22, 2),
+			storageBinaryMinSupportedVersion: v(22, 1),
+			tenantBinaryVersion:              v(22, 2),
+			tenantBinaryMinSupportedVersion:  v(22, 2),
+			TenantLogicalVersionKey:          clusterversion.V22_2,
+			expErrMatch:                      "",
+		},
+		// Second test case ensures that a tenant server is prevented from starting if
+		// its binary version is too low for the current tenant logical version.
+		{
+			storageBinaryVersion:             v(22, 2),
+			storageBinaryMinSupportedVersion: v(22, 1),
+			tenantBinaryVersion:              v(22, 1),
+			tenantBinaryMinSupportedVersion:  v(21, 2),
+			TenantLogicalVersionKey:          clusterversion.V22_2,
+			expErrMatch: fmt.Sprintf("preventing SQL server from starting because its binary version is too low for the tenant active version: "+
+				"server binary version = %v, tenant active version = %v", v(22, 1), v(22, 2)),
+		},
+	}
+
+	for i, test := range tests {
+		storageSettings := cluster.MakeTestingClusterSettingsWithVersions(
+			test.storageBinaryVersion,
+			test.storageBinaryMinSupportedVersion,
+			false, /* initializeVersion */
+		)
+
+		s, _, _ := serverutils.StartServer(t, base.TestServerArgs{
+			// Disable the default test tenant, since we create one explicitly
+			// below.
+			DisableDefaultTestTenant: true,
+			Settings:                 storageSettings,
+			Knobs: base.TestingKnobs{
+				Server: &server.TestingKnobs{
+					BinaryVersionOverride:          test.storageBinaryVersion,
+					DisableAutomaticVersionUpgrade: make(chan struct{}),
+				},
+				SQLEvalContext: &eval.TestingKnobs{
+					TenantLogicalVersionKeyOverride: test.TenantLogicalVersionKey,
+				},
+			},
+		})
+
+		tenantSettings := cluster.MakeTestingClusterSettingsWithVersions(
+			test.tenantBinaryVersion,
+			test.tenantBinaryMinSupportedVersion,
+			true, /* initializeVersion */
+		)
+
+		// The tenant will be created with an active version equal to the version
+		// corresponding to TenantLogicalVersionKey. Tenant creation is expected
+		// to succeed for all test cases but server creation is expected to succeed
+		// only if tenantBinaryVersion is at least equal to the version corresponding
+		// to TenantLogicalVersionKey.
+		tenantServer, err := s.StartTenant(context.Background(),
+			base.TestTenantArgs{
+				Settings: tenantSettings,
+				TenantID: serverutils.TestTenantID(),
+				TestingKnobs: base.TestingKnobs{
+					Server: &server.TestingKnobs{
+						BinaryVersionOverride:          test.tenantBinaryVersion,
+						DisableAutomaticVersionUpgrade: make(chan struct{}),
+					},
+				},
+			})
+
+		if !testutils.IsError(err, test.expErrMatch) {
+			t.Fatalf("test %d: got error %s, wanted error matching '%s'", i, err, test.expErrMatch)
+		}
+
+		// Only attempt to stop the tenant if it was started successfully.
+		if err == nil {
+			tenantServer.Stopper().Stop(context.Background())
+		}
+		s.Stopper().Stop(context.Background())
+	}
+}

pkg/clusterversion/cockroach_versions.go

@@ -422,6 +422,9 @@ func (k Key) String() string {
 // previously referenced a < 22.1 version until that check/gate can be removed.
 const TODOPreV22_1 = V22_1
 
+// Offset every version +1M major versions into the future if this is a dev branch.
+const DevOffset = 1000000
+
 // rawVersionsSingleton lists all historical versions here in chronological
 // order, with comments describing what backwards-incompatible features were
 // introduced.
@@ -750,7 +753,6 @@ var versionsSingleton = func() keyedVersions {
 		// which conceptually is actually back down to 2 -- then back to to 1000003,
 		// then on to 1000004, etc.
 		skipFirst := allowUpgradeToDev
-		const devOffset = 1000000
 		first := true
 		for i := range rawVersionsSingleton {
 			// VPrimordial versions are not offset; they don't matter for the logic
@@ -763,7 +765,7 @@ var versionsSingleton = func() keyedVersions {
 				first = false
 				continue
 			}
-			rawVersionsSingleton[i].Major += devOffset
+			rawVersionsSingleton[i].Major += DevOffset
 		}
 	}
 	return rawVersionsSingleton
@@ -777,6 +779,10 @@ var versionsSingleton = func() keyedVersions {
 // simply need to check is that the cluster has upgraded to 23.1.
 var V23_1 = versionsSingleton[len(versionsSingleton)-1].Key
 
+const (
+	BinaryMinSupportedVersionKey = V22_1
+)
+
 // TODO(irfansharif): clusterversion.binary{,MinimumSupported}Version
 // feels out of place. A "cluster version" and a "binary version" are two
 // separate concepts.
@@ -786,12 +792,13 @@ var (
 	// version than binaryMinSupportedVersion, then the binary will exit with
 	// an error. This typically trails the current release by one (see top-level
 	// comment).
-	binaryMinSupportedVersion = ByKey(V22_1)
+	binaryMinSupportedVersion = ByKey(BinaryMinSupportedVersionKey)
 
+	BinaryVersionKey = V23_1
 	// binaryVersion is the version of this binary.
 	//
 	// This is the version that a new cluster will use when created.
-	binaryVersion = versionsSingleton[len(versionsSingleton)-1].Version
+	binaryVersion = ByKey(BinaryVersionKey)
 )
 
 func init() {

pkg/clusterversion/cockroach_versions_test.go

@@ -90,7 +90,7 @@ func TestClusterVersionPrettyPrint(t *testing.T) {
 		}
 	}
 
-	var tests = []struct {
+	tests := []struct {
 		cv  ClusterVersion
 		exp string
 	}{
@@ -130,7 +130,7 @@ func TestGetVersionsBetween(t *testing.T) {
 		return cvs
 	}
 
-	var tests = []struct {
+	tests := []struct {
 		from, to roachpb.Version
 		exp      []roachpb.Version
 	}{
@@ -155,3 +155,15 @@ func TestGetVersionsBetween(t *testing.T) {
 		}
 	}
 }
+
+// TestEnsureConsistentBinaryVersion ensures that BinaryVersionKey maps to a
+// version equal to binaryVersion.
+func TestEnsureConsistentBinaryVersion(t *testing.T) {
+	require.Equal(t, ByKey(BinaryVersionKey), binaryVersion)
+}
+
+// TestEnsureConsistentMinBinaryVersion ensures that BinaryMinSupportedVersionKey
+// maps to a version equal to binaryMinSupportedVersion.
+func TestEnsureConsistentMinBinaryVersion(t *testing.T) {
+	require.Equal(t, ByKey(BinaryMinSupportedVersionKey), binaryMinSupportedVersion)
+}

pkg/server/server_sql.go

@@ -25,6 +25,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/blobs/blobspb"
 	"github.com/cockroachdb/cockroach/pkg/cloud"
 	"github.com/cockroachdb/cockroach/pkg/cloud/externalconn"
+	"github.com/cockroachdb/cockroach/pkg/clusterversion"
 	"github.com/cockroachdb/cockroach/pkg/featureflag"
 	"github.com/cockroachdb/cockroach/pkg/gossip"
 	"github.com/cockroachdb/cockroach/pkg/jobs"
@@ -1520,6 +1521,25 @@ func (s *SQLServer) preStart(
 
 	log.Infof(ctx, "done ensuring all necessary startup migrations have run")
 
+	// Prevent the server from starting if its binary version is too low
+	// for the current tenant cluster version.
+	// This check needs to run after the "version" setting is set in the
+	// "system.settings" table of this tenant. This includes both system
+	// and secondary tenants.
+	var tenantActiveVersion clusterversion.ClusterVersion
+	if err := s.execCfg.DB.Txn(ctx, func(ctx context.Context, txn *kv.Txn) (err error) {
+		tenantActiveVersion, err = s.settingsWatcher.GetClusterVersionFromStorage(ctx, txn)
+		return err
+	}); err != nil {
+		return err
+	}
+	if s.execCfg.Settings.Version.BinaryVersion().Less(tenantActiveVersion.Version) {
+		return errors.WithHintf(errors.Newf("preventing SQL server from starting because its binary version "+
+			"is too low for the tenant active version: server binary version = %v, tenant active version = %v",
+			s.execCfg.Settings.Version.BinaryVersion(), tenantActiveVersion.Version),
+			"use a tenant binary whose version is at least %v", tenantActiveVersion.Version)
+	}
+
 	// Delete all orphaned table leases created by a prior instance of this
 	// node. This also uses SQL.
 	s.leaseMgr.DeleteOrphanedLeases(ctx, orphanedLeasesTimeThresholdNanos)

pkg/server/settings_cache.go

@@ -144,7 +144,7 @@ func initializeCachedSettings(
 ) error {
 	dec := settingswatcher.MakeRowDecoder(codec)
 	for _, kv := range kvs {
-		settings, val, _, err := dec.DecodeRow(kv)
+		settings, val, _, err := dec.DecodeRow(kv, nil /* alloc */)
 		if err != nil {
 			return errors.Wrap(err, `while decoding settings data
 -this likely indicates the settings table structure or encoding has been altered;

pkg/server/settingswatcher/BUILD.bazel

@@ -13,6 +13,7 @@ go_library(
     deps = [
         "//pkg/clusterversion",
         "//pkg/keys",
+        "//pkg/kv",
         "//pkg/kv/kvclient/rangefeed",
         "//pkg/kv/kvclient/rangefeed/rangefeedbuffer",
         "//pkg/kv/kvclient/rangefeed/rangefeedcache",
@@ -25,6 +26,7 @@ go_library(
         "//pkg/sql/rowenc/valueside",
         "//pkg/sql/sem/tree",
         "//pkg/sql/types",
+        "//pkg/util/encoding",
         "//pkg/util/hlc",
         "//pkg/util/log",
         "//pkg/util/protoutil",
@@ -58,6 +60,7 @@ go_test(
         "//pkg/settings",
         "//pkg/settings/cluster",
         "//pkg/sql",
+        "//pkg/sql/sem/tree",
         "//pkg/testutils",
         "//pkg/testutils/serverutils",
         "//pkg/testutils/sqlutils",

pkg/server/settingswatcher/row_decoder.go

@@ -24,9 +24,9 @@ import (
 )
 
 // RowDecoder decodes rows from the settings table.
+// It is safe for concurrent use.
 type RowDecoder struct {
 	codec   keys.SQLCodec
-	alloc   tree.DatumAlloc
 	columns []catalog.Column
 	decoder valueside.Decoder
 }
@@ -45,17 +45,20 @@ func MakeRowDecoder(codec keys.SQLCodec) RowDecoder {
 // present, the setting key will be returned but the value will be zero and the
 // tombstone bool will be set.
 func (d *RowDecoder) DecodeRow(
-	kv roachpb.KeyValue,
+	kv roachpb.KeyValue, alloc *tree.DatumAlloc,
 ) (setting string, val settings.EncodedValue, tombstone bool, _ error) {
 	// First we need to decode the setting name field from the index key.
+	if alloc == nil {
+		alloc = &tree.DatumAlloc{}
+	}
 	{
 		types := []*types.T{d.columns[0].GetType()}
 		nameRow := make([]rowenc.EncDatum, 1)
 		_, _, err := rowenc.DecodeIndexKey(d.codec, types, nameRow, nil, kv.Key)
 		if err != nil {
 			return "", settings.EncodedValue{}, false, errors.Wrap(err, "failed to decode key")
 		}
-		if err := nameRow[0].EnsureDecoded(types[0], &d.alloc); err != nil {
+		if err := nameRow[0].EnsureDecoded(types[0], alloc); err != nil {
 			return "", settings.EncodedValue{}, false, err
 		}
 		setting = string(tree.MustBeDString(nameRow[0].Datum))
@@ -70,7 +73,7 @@ func (d *RowDecoder) DecodeRow(
 		return "", settings.EncodedValue{}, false, err
 	}
 
-	datums, err := d.decoder.Decode(&d.alloc, bytes)
+	datums, err := d.decoder.Decode(alloc, bytes)
 	if err != nil {
 		return "", settings.EncodedValue{}, false, err
 	}

pkg/server/settingswatcher/row_decoder_external_test.go

@@ -18,6 +18,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/keys"
 	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/server/settingswatcher"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/testutils/testcluster"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
@@ -70,13 +71,14 @@ func TestRowDecoder(t *testing.T) {
 	rows, err := tc.Server(0).DB().Scan(ctx, k, k.PrefixEnd(), 0 /* maxRows */)
 	require.NoError(t, err)
 	dec := settingswatcher.MakeRowDecoder(keys.SystemSQLCodec)
+	var alloc *tree.DatumAlloc
 	for _, row := range rows {
 		kv := roachpb.KeyValue{
 			Key:   row.Key,
 			Value: *row.Value,
 		}
 
-		k, val, tombstone, err := dec.DecodeRow(kv)
+		k, val, tombstone, err := dec.DecodeRow(kv, alloc)
 		require.NoError(t, err)
 		require.False(t, tombstone)
 		if exp, ok := toSet[k]; ok {
@@ -88,7 +90,7 @@ func TestRowDecoder(t *testing.T) {
 		// Test the tombstone logic while we're here.
 		{
 			kv.Value.Reset()
-			tombstoneK, val, tombstone, err := dec.DecodeRow(kv)
+			tombstoneK, val, tombstone, err := dec.DecodeRow(kv, alloc)
 			require.NoError(t, err)
 			require.True(t, tombstone)
 			require.Equal(t, k, tombstoneK)