testserver,testtenant: fix SQLConn()

Prior to this patch, the (recently introduced) `SQLConn()` method was
blind to the concept of a shared SQL port where connections get
redirected to a secondary tenant based on the `cluster` URL option.

Because of this, two bad things were happening:

- `.SQLConn()` on the `ApplicationLayerInterface` of a secondary
  tenant with a shared SQL port would fail to connect to the right
  tenant.

- `.SystemLayer().SQLConn()` would fail if the cluster setting
  `server.controller.default_tenant` was set.

This patch fixes it.

Release note: None

Author: knz

pkg/server/BUILD.bazel

@@ -491,6 +491,7 @@ go_test(
         "//pkg/kv/kvserver/kvserverpb",
         "//pkg/kv/kvserver/kvstorage",
         "//pkg/kv/kvserver/liveness/livenesspb",
+        "//pkg/multitenant",
         "//pkg/roachpb",
         "//pkg/rpc",
         "//pkg/security",

pkg/server/server_controller_test.go

@@ -15,10 +15,14 @@ import (
 	"testing"
 
 	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/multitenant"
+	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/settings/cluster"
+	"github.com/cockroachdb/cockroach/pkg/sql/isql"
 	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
 	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 )
 
@@ -81,6 +85,7 @@ func TestSQLErrorUponInvalidTenant(t *testing.T) {
 	require.NoError(t, err)
 
 	err = db.Ping()
+	require.NotNil(t, err)
 	require.Regexp(t, `service unavailable for target tenant \(nonexistent\)`, err.Error())
 }
 
@@ -109,3 +114,57 @@ func TestSharedProcessServerInheritsTempStorageLimit(t *testing.T) {
 	tss := ts.(*testTenant)
 	require.Equal(t, int64(specialSize), tss.SQLCfg.TempStorageConfig.Mon.Limit())
 }
+
+// TestServerSQLConn checks that the SQLConn() method on the
+// SystemLayer() of TestServerInterface works even when non-specific
+// SQL connection requests are redirected to a secondary tenant.
+func TestServerSQLConn(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	ctx := context.Background()
+	s := serverutils.StartServerOnly(t, base.TestServerArgs{
+		DefaultTestTenant: base.TestControlsTenantsExplicitly,
+	})
+	defer s.Stopper().Stop(ctx)
+	systemTenant := s.SystemLayer()
+
+	// Start some secondary tenant servers.
+	secondaryTenantExtNoName, err := s.StartTenant(ctx, base.TestTenantArgs{
+		TenantID: roachpb.MustMakeTenantID(2),
+	})
+	require.NoError(t, err)
+
+	secondaryTenantExtNamed, err := s.StartTenant(ctx, base.TestTenantArgs{
+		TenantName: "hello",
+		TenantID:   roachpb.MustMakeTenantID(10),
+	})
+	require.NoError(t, err)
+
+	secondaryTenantSh, _, err := s.StartSharedProcessTenant(ctx, base.TestSharedProcessTenantArgs{
+		TenantName: "world",
+	})
+	require.NoError(t, err)
+	multitenant.DefaultTenantSelect.Override(ctx, &systemTenant.ClusterSettings().SV, "world")
+
+	for _, tc := range []struct {
+		testName     string
+		tbName       string
+		sqlInterface serverutils.ApplicationLayerInterface
+	}{
+		{"system", "foo", systemTenant},
+		{"secondary-external-noname", "bar", secondaryTenantExtNoName},
+		{"secondary-external-named", "baz", secondaryTenantExtNamed},
+		{"secondary-shared", "qux", secondaryTenantSh},
+	} {
+		t.Run(tc.testName, func(t *testing.T) {
+			_, err = tc.sqlInterface.InternalExecutor().(isql.Executor).
+				Exec(ctx, "create-table", nil, "CREATE TABLE defaultdb."+tc.tbName+" (i INT)")
+			require.NoError(t, err)
+
+			conn := tc.sqlInterface.SQLConn(t, "defaultdb")
+			var unused int
+			assert.NoError(t, conn.QueryRowContext(ctx, "SELECT count(*) FROM "+tc.tbName).Scan(&unused))
+		})
+	}
+}

pkg/server/testserver.go

@@ -54,6 +54,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/sql/deprecatedshowranges"
 	"github.com/cockroachdb/cockroach/pkg/sql/pgwire"
 	"github.com/cockroachdb/cockroach/pkg/sql/physicalplan"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants"
 	"github.com/cockroachdb/cockroach/pkg/sql/sem/tree"
 	"github.com/cockroachdb/cockroach/pkg/sql/sessiondata"
 	"github.com/cockroachdb/cockroach/pkg/storage"
@@ -495,7 +496,9 @@ func (ts *testServer) SQLConnE(dbName string) (*gosql.DB, error) {
 
 // SQLConnForUserE is part of the serverutils.ApplicationLayerInterface.
 func (ts *testServer) SQLConnForUserE(userName string, dbName string) (*gosql.DB, error) {
-	return openTestSQLConn(userName, dbName, ts.Stopper(),
+	return openTestSQLConn(
+		userName, dbName, catconstants.SystemTenantName,
+		ts.Stopper(),
 		ts.topLevelServer.loopbackPgL,
 		ts.cfg.SQLAdvertiseAddr,
 		ts.cfg.Insecure,
@@ -787,7 +790,15 @@ func (t *testTenant) SQLConnE(dbName string) (*gosql.DB, error) {
 
 // SQLConnForUserE is part of the serverutils.ApplicationLayerInterface.
 func (t *testTenant) SQLConnForUserE(userName string, dbName string) (*gosql.DB, error) {
-	return openTestSQLConn(userName, dbName, t.Stopper(),
+	tenantName := t.t.tenantName
+	if !t.Cfg.DisableSQLListener {
+		// This tenant server has its own SQL listener. It will not accept
+		// a "cluster" connection parameter.
+		tenantName = ""
+	}
+	return openTestSQLConn(
+		userName, dbName, tenantName,
+		t.Stopper(),
 		t.pgL,
 		t.Cfg.SQLAdvertiseAddr,
 		t.Cfg.Insecure,

pkg/server/testserver_sqlconn.go

@@ -13,10 +13,13 @@ package server
 import (
 	"context"
 	gosql "database/sql"
+	"fmt"
 	"net"
 	"net/url"
+	"strings"
 	"time"
 
+	"github.com/cockroachdb/cockroach/pkg/roachpb"
 	"github.com/cockroachdb/cockroach/pkg/testutils/sqlutils"
 	"github.com/cockroachdb/cockroach/pkg/util/log"
 	"github.com/cockroachdb/cockroach/pkg/util/netutil"
@@ -43,6 +46,7 @@ const useLoopbackListener = false
 // of serverutils.ApplicationLayerInterface.
 func openTestSQLConn(
 	userName, dbName string,
+	tenantName roachpb.TenantName,
 	stopper *stop.Stopper,
 	// When useLoopbackListener is set, only this is used:
 	pgL *netutil.LoopbackListener,
@@ -53,13 +57,21 @@ func openTestSQLConn(
 	cleanupFn := func() {}
 	var goDB *gosql.DB
 
+	opts := url.Values{}
+	if tenantName != "" && !strings.HasPrefix(dbName, "cluster:") {
+		opts.Add("options", fmt.Sprintf("-ccluster=%s", tenantName))
+	}
+	if insecure || useLoopbackListener {
+		opts.Add("sslmode", "disable")
+	}
+
 	if useLoopbackListener {
 		pgurl := url.URL{
 			Scheme:   "postgres",
 			User:     url.User(userName),
 			Host:     "unused",
 			Path:     dbName,
-			RawQuery: "sslmode=disable",
+			RawQuery: opts.Encode(),
 		}
 		// TODO(sql): consider using pgx for tests instead of lib/pq.
 		connector, err := pq.NewConnector(pgurl.String())
@@ -76,9 +88,16 @@ func openTestSQLConn(
 			return nil, err
 		}
 		pgURL.Path = dbName
-		if insecure {
-			pgURL.RawQuery = "sslmode=disable"
+
+		// Add the common query options decided above to those prepared by
+		// PGUrlE().
+		qv := pgURL.Query()
+		for k, v := range opts {
+			qv[k] = v
 		}
+		pgURL.RawQuery = qv.Encode()
+
+		// Open the connection.
 		goDB, err = gosql.Open("postgres", pgURL.String())
 		if err != nil {
 			cleanupFn()