Merge #96282 #96345

96282: logictest: fix user cmd to respect nodeidx r=rafiss a=andyyang890

logictest: change logicTest to store per-node clients for users

This patch changes the `clients` field within `logicTest` from type
`map[string]*gosql.DB` to `map[string]map[int]*gosql.DB` in order to
store per-node clients for each user. This allows the `user` cmd in
logic tests to fetch a client for the specified node instead of the
previously used node.

Release note: None

----

logictest: move paragraph in comment that got separated

Release note: None

----

logictest: fix user cmd to respect nodeidx for cockroach-go testserver

This patch fixes the client creation for logic tests that use the
cockroach-go testserver to use the specified nodeidx when connecting.

Release note: None

----

Informs #92342

96345: pgwire,server: clarify to SQL clients when they select the wrong tenant r=rafiss a=knz

Fixes #92525.
Epic: CRDB-14537

Prior to this patch:
```
$ ./cockroach sql -d cluster:wo
ERROR: server closed the connection.
Is this a CockroachDB node?
unexpected EOF
```

After this patch:
```
$ ./cockroach sql -d cluster:woo
ERROR: service unavailable for target tenant (woo)
SQLSTATE: 08000
HINT: Double check your "-ccluster=" connection parameter or your "cluster:" database name prefix.
```

Release note: None

Co-authored-by: Andy Yang <[email protected]>
Co-authored-by: Raphael 'kena' Poss <[email protected]>

pkg/server/server.go

@@ -914,7 +914,7 @@ func NewServer(cfg Config, stopper *stop.Stopper) (*Server, error) {
 
 	// Initialize the pgwire pre-server, which initializes connections,
 	// sets up TLS and reads client status parameters.
-	pgPreServer := pgwire.MakePreServeConnHandler(
+	pgPreServer := pgwire.NewPreServeConnHandler(
 		cfg.AmbientCtx,
 		cfg.Config,
 		cfg.Settings,
@@ -1040,7 +1040,11 @@ func NewServer(cfg Config, stopper *stop.Stopper) (*Server, error) {
 	sc := newServerController(ctx,
 		node, cfg.BaseConfig.IDContainer,
 		stopper, st,
-		lateBoundServer, &systemServerWrapper{server: lateBoundServer}, systemTenantNameContainer)
+		lateBoundServer,
+		&systemServerWrapper{server: lateBoundServer},
+		systemTenantNameContainer,
+		pgPreServer.SendRoutingError,
+	)
 
 	// Create the debug API server.
 	debugServer := debug.NewServer(
@@ -1116,7 +1120,7 @@ func NewServer(cfg Config, stopper *stop.Stopper) (*Server, error) {
 		protectedtsProvider:    protectedtsProvider,
 		spanConfigSubscriber:   spanConfig.subscriber,
 		spanConfigReporter:     spanConfig.reporter,
-		pgPreServer:            &pgPreServer,
+		pgPreServer:            pgPreServer,
 		sqlServer:              sqlServer,
 		serverController:       sc,
 		externalStorageBuilder: externalStorageBuilder,

pkg/server/server_controller.go

@@ -98,6 +98,10 @@ type serverController struct {
 	// testArgs is used when creating new tenant servers.
 	testArgs map[roachpb.TenantName]base.TestSharedProcessTenantArgs
 
+	// sendSQLRoutingError is a callback to use to report
+	// a tenant routing error to the incoming client.
+	sendSQLRoutingError func(ctx context.Context, conn net.Conn, tenantName roachpb.TenantName)
+
 	mu struct {
 		syncutil.Mutex
 
@@ -122,6 +126,7 @@ func newServerController(
 	tenantServerCreator tenantServerCreator,
 	systemServer onDemandServer,
 	systemTenantNameContainer *roachpb.TenantNameContainer,
+	sendSQLRoutingError func(ctx context.Context, conn net.Conn, tenantName roachpb.TenantName),
 ) *serverController {
 	c := &serverController{
 		nodeID:              parentNodeID,
@@ -130,6 +135,7 @@ func newServerController(
 		testArgs:            make(map[roachpb.TenantName]base.TestSharedProcessTenantArgs),
 		stopper:             parentStopper,
 		tenantServerCreator: tenantServerCreator,
+		sendSQLRoutingError: sendSQLRoutingError,
 	}
 
 	// We make the serverState for the system mock the regular

pkg/server/server_controller_sql.go

@@ -76,17 +76,15 @@ func (c *serverController) sqlMux(
 		})
 
 	case pgwire.PreServeReady:
-		tenantName := status.GetTenantName()
+		tenantName := roachpb.TenantName(status.GetTenantName())
 		if tenantName == "" {
-			tenantName = defaultTenantSelect.Get(&c.st.SV)
+			tenantName = roachpb.TenantName(defaultTenantSelect.Get(&c.st.SV))
 		}
 
-		s, err := c.getServer(ctx, roachpb.TenantName(tenantName))
+		s, err := c.getServer(ctx, tenantName)
 		if err != nil {
 			log.Warningf(ctx, "unable to find server for tenant %q: %v", tenantName, err)
-			// TODO(knz): we might want to send a pg error to the client here.
-			// See: https://github.com/cockroachdb/cockroach/issues/92525
-			_ = conn.Close()
+			c.sendSQLRoutingError(ctx, conn, tenantName)
 			return err
 		}
 

pkg/server/server_controller_test.go

@@ -62,3 +62,24 @@ func TestServerController(t *testing.T) {
 	require.Error(t, err, "tenant connector requires a CCL binary")
 	// TODO(knz): test something about d
 }
+
+func TestSQLErrorUponInvalidTenant(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	ctx := context.Background()
+
+	s, _, _ := serverutils.StartServer(t, base.TestServerArgs{
+		DisableDefaultTestTenant: true,
+	})
+	defer s.Stopper().Stop(ctx)
+
+	sqlAddr := s.ServingSQLAddr()
+	db, err := serverutils.OpenDBConnE(sqlAddr, "cluster:nonexistent", false, s.Stopper())
+	// Expect no error yet: the connection is opened lazily; an
+	// error here means the parameters were incorrect.
+	require.NoError(t, err)
+
+	err = db.Ping()
+	require.Regexp(t, `service unavailable for target tenant \(nonexistent\)`, err.Error())
+}

pkg/server/tenant.go

@@ -253,7 +253,7 @@ func NewTenantServer(
 	if !baseCfg.DisableSQLListener {
 		// Initialize the pgwire pre-server, which initializes connections,
 		// sets up TLS and reads client status parameters.
-		ps := pgwire.MakePreServeConnHandler(
+		pgPreServer = pgwire.NewPreServeConnHandler(
 			baseCfg.AmbientCtx,
 			baseCfg.Config,
 			args.Settings,
@@ -262,10 +262,9 @@ func NewTenantServer(
 			args.monitorAndMetrics.rootSQLMemoryMonitor,
 			false, /* acceptTenantName */
 		)
-		for _, m := range ps.Metrics() {
+		for _, m := range pgPreServer.Metrics() {
 			args.registry.AddMetricStruct(m)
 		}
-		pgPreServer = &ps
 	}
 
 	// Instantiate the SQL server proper.