Merge cockroachdb#73400

73400: server{,testutils}: unify Test{Tenant,Server}Interface r=irfansharif a=irfansharif

TestTenantInterface was intended to be the SQL-only API surface needed
by tests to interface with tenants (both the host and secondary kinds).
TestServerInterface, in contrast, should've been restricted to strictly
host cluster operations. Neither was quite true.

We embed TestTenantInterface into TestServerInterface and move some
symbols into the former. This should help with better test parity,
regardless of tenant kind.

Release note: None

Co-authored-by: irfan sharif <[email protected]>

pkg/ccl/changefeedccl/helpers_tenant_shim_test.go

@@ -69,9 +69,6 @@ func (t *testServerShim) RangeFeedFactory() interface{}         { panic(unsuppor
 func (t *testServerShim) Clock() *hlc.Clock                     { panic(unsupportedShimMethod) }
 func (t *testServerShim) DistSenderI() interface{}              { panic(unsupportedShimMethod) }
 func (t *testServerShim) MigrationServer() interface{}          { panic(unsupportedShimMethod) }
-func (t *testServerShim) SpanConfigAccessor() interface{}       { panic(unsupportedShimMethod) }
-func (t *testServerShim) SpanConfigSQLTranslator() interface{}  { panic(unsupportedShimMethod) }
-func (t *testServerShim) SpanConfigSQLWatcher() interface{}     { panic(unsupportedShimMethod) }
 func (t *testServerShim) SQLServer() interface{}                { panic(unsupportedShimMethod) }
 func (t *testServerShim) SQLLivenessProvider() interface{}      { panic(unsupportedShimMethod) }
 func (t *testServerShim) StartupMigrationsManager() interface{} { panic(unsupportedShimMethod) }

pkg/ccl/sqlproxyccl/proxy_handler_test.go

@@ -207,7 +207,7 @@ func TestProxyAgainstSecureCRDB(t *testing.T) {
 	defer te.Close()
 
 	sql, db, _ := serverutils.StartServer(t, base.TestServerArgs{Insecure: false})
-	sql.(*server.TestServer).PGServer().TestingSetTrustClientProvidedRemoteAddr(true)
+	sql.(*server.TestServer).PGServer().(*pgwire.Server).TestingSetTrustClientProvidedRemoteAddr(true)
 	defer sql.Stopper().Stop(ctx)
 
 	sqlDB := sqlutils.MakeSQLRunner(db)
@@ -323,7 +323,7 @@ func TestProxyTLSClose(t *testing.T) {
 	defer te.Close()
 
 	sql, db, _ := serverutils.StartServer(t, base.TestServerArgs{Insecure: false})
-	sql.(*server.TestServer).PGServer().TestingSetTrustClientProvidedRemoteAddr(true)
+	sql.(*server.TestServer).PGServer().(*pgwire.Server).TestingSetTrustClientProvidedRemoteAddr(true)
 	defer sql.Stopper().Stop(ctx)
 
 	sqlDB := sqlutils.MakeSQLRunner(db)
@@ -368,7 +368,7 @@ func TestProxyModifyRequestParams(t *testing.T) {
 	defer te.Close()
 
 	sql, sqlDB, _ := serverutils.StartServer(t, base.TestServerArgs{Insecure: false})
-	sql.(*server.TestServer).PGServer().TestingSetTrustClientProvidedRemoteAddr(true)
+	sql.(*server.TestServer).PGServer().(*pgwire.Server).TestingSetTrustClientProvidedRemoteAddr(true)
 	defer sql.Stopper().Stop(ctx)
 
 	// Create some user with password authn.
@@ -421,7 +421,7 @@ func TestInsecureProxy(t *testing.T) {
 	defer te.Close()
 
 	sql, db, _ := serverutils.StartServer(t, base.TestServerArgs{Insecure: false})
-	sql.(*server.TestServer).PGServer().TestingSetTrustClientProvidedRemoteAddr(true)
+	sql.(*server.TestServer).PGServer().(*pgwire.Server).TestingSetTrustClientProvidedRemoteAddr(true)
 	defer sql.Stopper().Stop(ctx)
 
 	sqlDB := sqlutils.MakeSQLRunner(db)
@@ -527,7 +527,7 @@ func TestDenylistUpdate(t *testing.T) {
 	require.NoError(t, err)
 
 	sql, sqlDB, _ := serverutils.StartServer(t, base.TestServerArgs{Insecure: false})
-	sql.(*server.TestServer).PGServer().TestingSetTrustClientProvidedRemoteAddr(true)
+	sql.(*server.TestServer).PGServer().(*pgwire.Server).TestingSetTrustClientProvidedRemoteAddr(true)
 	defer sql.Stopper().Stop(ctx)
 
 	// Create some user with password authn.
@@ -596,7 +596,7 @@ func TestDirectoryConnect(t *testing.T) {
 	defer te.Close()
 
 	srv, _, _ := serverutils.StartServer(t, base.TestServerArgs{Insecure: true})
-	srv.(*server.TestServer).PGServer().TestingSetTrustClientProvidedRemoteAddr(true)
+	srv.(*server.TestServer).PGServer().(*pgwire.Server).TestingSetTrustClientProvidedRemoteAddr(true)
 	defer srv.Stopper().Stop(ctx)
 
 	// Create tenant 28.

pkg/server/testserver.go

@@ -47,7 +47,6 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
 	"github.com/cockroachdb/cockroach/pkg/sql"
 	"github.com/cockroachdb/cockroach/pkg/sql/catalog/descpb"
-	"github.com/cockroachdb/cockroach/pkg/sql/pgwire"
 	"github.com/cockroachdb/cockroach/pkg/sql/physicalplan"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
@@ -315,6 +314,8 @@ type TestServer struct {
 	}
 }
 
+var _ serverutils.TestServerInterface = &TestServer{}
+
 // Node returns the Node as an interface{}.
 func (ts *TestServer) Node() interface{} {
 	return ts.node
@@ -421,6 +422,16 @@ func (ts *TestServer) HeartbeatNodeLiveness() error {
 	return err
 }
 
+// SQLInstanceID is part of TestServerInterface.
+func (ts *TestServer) SQLInstanceID() base.SQLInstanceID {
+	return ts.sqlServer.sqlIDContainer.SQLInstanceID()
+}
+
+// StatusServer is part of TestServerInterface.
+func (ts *TestServer) StatusServer() interface{} {
+	return ts.status
+}
+
 // RPCContext returns the rpc context used by the TestServer.
 func (ts *TestServer) RPCContext() *rpc.Context {
 	if ts != nil {
@@ -445,8 +456,9 @@ func (ts *TestServer) DB() *kv.DB {
 	return nil
 }
 
-// PGServer returns the pgwire.Server used by the TestServer.
-func (ts *TestServer) PGServer() *pgwire.Server {
+// PGServer exposes the pgwire.Server instance used by the TestServer as an
+// interface{}.
+func (ts *TestServer) PGServer() interface{} {
 	if ts != nil {
 		return ts.sqlServer.pgServer
 	}
@@ -499,51 +511,93 @@ type TestTenant struct {
 	httpAddr string
 }
 
-// SQLAddr is part of the TestTenantInterface interface.
+var _ serverutils.TestTenantInterface = &TestTenant{}
+
+// SQLAddr is part of TestTenantInterface interface.
 func (t *TestTenant) SQLAddr() string {
 	return t.sqlAddr
 }
 
-// HTTPAddr is part of the TestTenantInterface interface.
+// HTTPAddr is part of TestTenantInterface interface.
 func (t *TestTenant) HTTPAddr() string {
 	return t.httpAddr
 }
 
-// PGServer is part of the TestTenantInterface interface.
+// PGServer is part of TestTenantInterface.
 func (t *TestTenant) PGServer() interface{} {
 	return t.pgServer
 }
 
-// DiagnosticsReporter is part of the TestTenantInterface interface.
+// DiagnosticsReporter is part of TestTenantInterface.
 func (t *TestTenant) DiagnosticsReporter() interface{} {
 	return t.diagnosticsReporter
 }
 
-// StatusServer is part of the TestTenantInterface interface.
+// StatusServer is part of TestTenantInterface.
 func (t *TestTenant) StatusServer() interface{} {
 	return t.execCfg.SQLStatusServer
 }
 
-// DistSQLServer is part of the TestTenantInterface interface.
+// DistSQLServer is part of TestTenantInterface.
 func (t *TestTenant) DistSQLServer() interface{} {
 	return t.SQLServer.distSQLServer
 }
 
-// RPCContext is part of the TestTenantInterface interface
+// RPCContext is part of TestTenantInterface.
 func (t *TestTenant) RPCContext() *rpc.Context {
 	return t.execCfg.RPCContext
 }
 
-// JobRegistry is part of the TestTenantInterface interface.
+// JobRegistry is part of TestTenantInterface.
 func (t *TestTenant) JobRegistry() interface{} {
 	return t.SQLServer.jobRegistry
 }
 
-// TestingKnobs is part of the TestTenantInterface interface.
+// ExecutorConfig is part of TestTenantInterface.
+func (t *TestTenant) ExecutorConfig() interface{} {
+	return *t.SQLServer.execCfg
+}
+
+// RangeFeedFactory is part of TestTenantInterface.
+func (t *TestTenant) RangeFeedFactory() interface{} {
+	return t.SQLServer.execCfg.RangeFeedFactory
+}
+
+// ClusterSettings is part of TestTenantInterface.
+func (t *TestTenant) ClusterSettings() *cluster.Settings {
+	return t.Cfg.Settings
+}
+
+// Stopper is part of TestTenantInterface.
+func (t *TestTenant) Stopper() *stop.Stopper {
+	return t.stopper
+}
+
+// Clock is part of TestTenantInterface.
+func (t *TestTenant) Clock() *hlc.Clock {
+	return t.SQLServer.execCfg.Clock
+}
+
+// TestingKnobs is part TestTenantInterface.
 func (t *TestTenant) TestingKnobs() *base.TestingKnobs {
 	return &t.Cfg.TestingKnobs
 }
 
+// SpanConfigKVAccessor is part TestTenantInterface.
+func (t *TestTenant) SpanConfigKVAccessor() interface{} {
+	return t.SQLServer.tenantConnect
+}
+
+// SpanConfigSQLTranslator is part TestTenantInterface.
+func (t *TestTenant) SpanConfigSQLTranslator() interface{} {
+	return t.SQLServer.spanconfigMgr.SQLTranslator
+}
+
+// SpanConfigSQLWatcher is part TestTenantInterface.
+func (t *TestTenant) SpanConfigSQLWatcher() interface{} {
+	return t.SQLServer.spanconfigMgr.SQLTranslator
+}
+
 // StartTenant starts a SQL tenant communicating with this TestServer.
 func (ts *TestServer) StartTenant(
 	ctx context.Context, params base.TestTenantArgs,
@@ -984,8 +1038,8 @@ func (ts *TestServer) MigrationServer() interface{} {
 	return ts.migrationServer
 }
 
-// SpanConfigAccessor is part of TestServerInterface.
-func (ts *TestServer) SpanConfigAccessor() interface{} {
+// SpanConfigKVAccessor is part of TestServerInterface.
+func (ts *TestServer) SpanConfigKVAccessor() interface{} {
 	return ts.Server.node.spanConfigAccessor
 }
 
@@ -1011,7 +1065,7 @@ func (ts *TestServer) SpanConfigSQLWatcher() interface{} {
 
 // SQLServer is part of TestServerInterface.
 func (ts *TestServer) SQLServer() interface{} {
-	return ts.PGServer().SQLServer
+	return ts.sqlServer.pgServer.SQLServer
 }
 
 // DistSQLServer is part of TestServerInterface.

pkg/spanconfig/spanconfigmanager/manager_test.go

@@ -73,7 +73,7 @@ func TestManagerConcurrentJobCreation(t *testing.T) {
 		ts.InternalExecutor().(*sql.InternalExecutor),
 		ts.Stopper(),
 		ts.ClusterSettings(),
-		ts.SpanConfigAccessor().(spanconfig.KVAccessor),
+		ts.SpanConfigKVAccessor().(spanconfig.KVAccessor),
 		ts.SpanConfigSQLWatcher().(spanconfig.SQLWatcher),
 		ts.SpanConfigSQLTranslator().(spanconfig.SQLTranslator),
 		&spanconfig.TestingKnobs{
@@ -162,7 +162,7 @@ func TestManagerStartsJobIfFailed(t *testing.T) {
 		ts.InternalExecutor().(*sql.InternalExecutor),
 		ts.Stopper(),
 		ts.ClusterSettings(),
-		ts.SpanConfigAccessor().(spanconfig.KVAccessor),
+		ts.SpanConfigKVAccessor().(spanconfig.KVAccessor),
 		ts.SpanConfigSQLWatcher().(spanconfig.SQLWatcher),
 		ts.SpanConfigSQLTranslator().(spanconfig.SQLTranslator),
 		&spanconfig.TestingKnobs{
@@ -238,7 +238,7 @@ func TestManagerCheckJobConditions(t *testing.T) {
 		ts.InternalExecutor().(*sql.InternalExecutor),
 		ts.Stopper(),
 		ts.ClusterSettings(),
-		ts.SpanConfigAccessor().(spanconfig.KVAccessor),
+		ts.SpanConfigKVAccessor().(spanconfig.KVAccessor),
 		ts.SpanConfigSQLWatcher().(spanconfig.SQLWatcher),
 		ts.SpanConfigSQLTranslator().(spanconfig.SQLTranslator),
 		&spanconfig.TestingKnobs{

pkg/sql/pgwire/auth_test.go

@@ -206,7 +206,7 @@ func hbaRunTest(t *testing.T, insecure bool) {
 		// We can't use the cluster settings to do this, because
 		// cluster settings propagate asynchronously.
 		testServer := s.(*server.TestServer)
-		pgServer := s.(*server.TestServer).PGServer()
+		pgServer := s.(*server.TestServer).PGServer().(*pgwire.Server)
 		pgServer.TestingEnableConnLogging()
 		pgServer.TestingEnableAuthLogging()
 
@@ -577,7 +577,7 @@ func TestClientAddrOverride(t *testing.T) {
 	// We can't use the cluster settings to do this, because
 	// cluster settings for booleans propagate asynchronously.
 	testServer := s.(*server.TestServer)
-	pgServer := testServer.PGServer()
+	pgServer := testServer.PGServer().(*pgwire.Server)
 	pgServer.TestingEnableAuthLogging()
 
 	testCases := []struct {

pkg/sql/pgwire/pgwire_test.go

@@ -129,7 +129,7 @@ func TestPGWireDrainClient(t *testing.T) {
 		}
 	}
 
-	if !s.(*server.TestServer).PGServer().IsDraining() {
+	if !s.(*server.TestServer).PGServer().(*pgwire.Server).IsDraining() {
 		t.Fatal("server should be draining, but is not")
 	}
 }
@@ -162,7 +162,7 @@ func TestPGWireDrainOngoingTxns(t *testing.T) {
 	}
 	defer db.Close()
 
-	pgServer := s.(*server.TestServer).PGServer()
+	pgServer := s.(*server.TestServer).PGServer().(*pgwire.Server)
 
 	// Make sure that the server reports correctly the case in which a
 	// connection did not respond to cancellation in time.