Merge #91744

91744: server: use a shared HTTP listener for all in-memory tenant servers r=andreimatei,dhartunian a=knz

Parent PRs:
- [ ] #84608
- [ ] #91739

Fixes #91738.

This commit introduces a HTTP (de)multiplexer for all in-memory tenant
servers.

By default, HTTP requests are routed to the system tenant server.
This can be overridden with the header `X-Cockroach-Tenant` or the `tenant` cookie.

Epic: CRDB-14487

Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>

pkg/base/config.go

@@ -229,6 +229,15 @@ type Config struct {
 	// low.
 	RPCHeartbeatIntervalAndHalfTimeout time.Duration
 
+	// SecondaryTenantPortOffset is the increment to add to the various
+	// addresses to generate the network configuration for the in-memory
+	// secondary tenant. If set to zero (the default), ports are
+	// auto-allocated randomly.
+	// TODO(knz): Remove this mechanism altogether in favor of a single
+	// network listener with protocol routing.
+	// See: https://github.com/cockroachdb/cockroach/issues/84585
+	SecondaryTenantPortOffset int
+
 	// Enables the use of an PTP hardware clock user space API for HLC current time.
 	// This contains the path to the device to be used (i.e. /dev/ptp0)
 	ClockDevicePath string
@@ -281,6 +290,7 @@ func (cfg *Config) InitDefaults() {
 	cfg.DisableClusterNameVerification = false
 	cfg.ClockDevicePath = ""
 	cfg.AcceptSQLWithoutTLS = false
+	cfg.SecondaryTenantPortOffset = 0
 }
 
 // HTTPRequestScheme returns "http" or "https" based on the value of

pkg/base/test_server_args.go

@@ -153,6 +153,12 @@ type TestServerArgs struct {
 	// or if some of the functionality being tested is not accessible from
 	// within tenants.
 	DisableDefaultTestTenant bool
+
+	// StartDiagnosticsReporting checks cluster.TelemetryOptOut(), and
+	// if not disabled starts the asynchronous goroutine that checks for
+	// CockroachDB upgrades and periodically reports diagnostics to
+	// Cockroach Labs. Should remain disabled during unit testing.
+	StartDiagnosticsReporting bool
 }
 
 // TestClusterArgs contains the parameters one can set when creating a test
@@ -311,4 +317,10 @@ type TestTenantArgs struct {
 	// heapprofiler. If empty, no heap profiles will be collected during the test.
 	// If set, this directory should be cleaned up after the test completes.
 	HeapProfileDirName string
+
+	// StartDiagnosticsReporting checks cluster.TelemetryOptOut(), and
+	// if not disabled starts the asynchronous goroutine that checks for
+	// CockroachDB upgrades and periodically reports diagnostics to
+	// Cockroach Labs. Should remain disabled during unit testing.
+	StartDiagnosticsReporting bool
 }

pkg/ccl/serverccl/BUILD.bazel

@@ -37,6 +37,7 @@ go_test(
         "chart_catalog_test.go",
         "main_test.go",
         "role_authentication_test.go",
+        "server_controller_test.go",
         "server_sql_test.go",
         "tenant_decommissioned_host_test.go",
         "tenant_vars_test.go",
@@ -63,6 +64,8 @@ go_test(
         "//pkg/settings/cluster",
         "//pkg/sql",
         "//pkg/sql/distsql",
+        "//pkg/sql/lexbase",
+        "//pkg/sql/sem/catconstants",
         "//pkg/sql/sqlinstance/instancestorage",
         "//pkg/sql/tests",
         "//pkg/testutils",
@@ -73,13 +76,15 @@ go_test(
         "//pkg/util",
         "//pkg/util/contextutil",
         "//pkg/util/envutil",
+        "//pkg/util/httputil",
         "//pkg/util/leaktest",
         "//pkg/util/log",
         "//pkg/util/metric",
         "//pkg/util/protoutil",
         "//pkg/util/randutil",
         "//pkg/util/timeutil",
         "@com_github_cockroachdb_datadriven//:datadriven",
+        "@com_github_cockroachdb_errors//:errors",
         "@com_github_elastic_gosigar//:gosigar",
         "@com_github_lib_pq//:pq",
         "@com_github_prometheus_client_model//go",

pkg/ccl/serverccl/server_controller_test.go

@@ -0,0 +1,172 @@
+// Copyright 2018 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"
+	"io"
+	"net/http"
+	"net/url"
+	"testing"
+	"time"
+
+	"github.com/cockroachdb/cockroach/pkg/base"
+	"github.com/cockroachdb/cockroach/pkg/server"
+	"github.com/cockroachdb/cockroach/pkg/server/serverpb"
+	"github.com/cockroachdb/cockroach/pkg/sql/lexbase"
+	"github.com/cockroachdb/cockroach/pkg/sql/sem/catconstants"
+	"github.com/cockroachdb/cockroach/pkg/testutils/serverutils"
+	"github.com/cockroachdb/cockroach/pkg/util/httputil"
+	"github.com/cockroachdb/cockroach/pkg/util/leaktest"
+	"github.com/cockroachdb/cockroach/pkg/util/log"
+	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
+	"github.com/cockroachdb/errors"
+	"github.com/stretchr/testify/require"
+)
+
+func TestServerControllerHTTP(t *testing.T) {
+	defer leaktest.AfterTest(t)()
+	defer log.Scope(t).Close(t)
+
+	ctx := context.Background()
+
+	s, db, _ := serverutils.StartServer(t, base.TestServerArgs{
+		DisableDefaultTestTenant: true,
+	})
+	defer s.Stopper().Stop(ctx)
+
+	// Retrieve a privileged HTTP client. NB: this also populates
+	// system.web_sessions.
+	aurl := s.AdminURL()
+	client, err := s.GetAdminHTTPClient()
+	require.NoError(t, err)
+
+	// Now retrieve the entry in the system tenant's web sessions.
+	row := db.QueryRow(`SELECT id,"hashedSecret",username,"createdAt","expiresAt" FROM system.web_sessions`)
+	var id int64
+	var secret string
+	var username string
+	var created, expires time.Time
+	require.NoError(t, row.Scan(&id, &secret, &username, &created, &expires))
+
+	// Create our own test tenant with a known name.
+	_, err = db.Exec("SELECT crdb_internal.create_tenant(10, 'hello')")
+	require.NoError(t, err)
+
+	// Get a SQL connection to the test tenant.
+	sqlAddr := s.(*server.TestServer).TestingGetSQLAddrForTenant(ctx, "hello")
+	db2 := serverutils.OpenDBConn(t, sqlAddr, "defaultdb", false, s.Stopper())
+
+	// Instantiate the HTTP test username and privileges into the test tenant.
+	_, err = db2.Exec(fmt.Sprintf(`CREATE USER %s`, lexbase.EscapeSQLIdent(username)))
+	require.NoError(t, err)
+	_, err = db2.Exec(fmt.Sprintf(`GRANT admin TO %s`, lexbase.EscapeSQLIdent(username)))
+	require.NoError(t, err)
+
+	// Copy the session entry to the test tenant.
+	_, err = db2.Exec(`INSERT INTO system.web_sessions(id, "hashedSecret", username, "createdAt", "expiresAt")
+VALUES($1, $2, $3, $4, $5)`, id, secret, username, created, expires)
+	require.NoError(t, err)
+
+	// From this point, we are expecting the ability to access both tenants using
+	// the same cookie jar.
+	// Let's assert this is true by retrieving session lists, asserting
+	// they are different and that each of them contains the appropriate entries.
+
+	// Make our session to the system tenant recognizable in session lists.
+	_, err = db.Exec("SET application_name = 'hello system'")
+	require.NoError(t, err)
+
+	// Ditto for the test tenant.
+	_, err = db2.Exec("SET application_name = 'hello hello'")
+	require.NoError(t, err)
+
+	get := func(req *http.Request) (*serverpb.ListSessionsResponse, error) {
+		req.Header.Set("Content-Type", httputil.ProtoContentType)
+		resp, err := client.Do(req)
+		if err != nil {
+			return nil, err
+		}
+		defer resp.Body.Close()
+		body, err := io.ReadAll(resp.Body)
+		if err != nil {
+			return nil, errors.Newf("request failed: %v", resp.StatusCode)
+		}
+		if resp.StatusCode != http.StatusOK {
+			return nil, errors.Newf("request failed: %v / %q", resp.StatusCode, string(body))
+		}
+		var ls serverpb.ListSessionsResponse
+		if err := protoutil.Unmarshal(body, &ls); err != nil {
+			return nil, err
+		}
+		return &ls, err
+	}
+
+	newreq := func() *http.Request {
+		req, err := http.NewRequest("GET", aurl+"/_status/sessions", nil)
+		require.NoError(t, err)
+		return req
+	}
+
+	// Retrieve the session list for the system tenant.
+	req := newreq()
+	req.Header.Set(server.TenantSelectHeader, catconstants.SystemTenantName)
+	body, err := get(req)
+	require.NoError(t, err)
+	t.Logf("response 1:\n%#v", body)
+	require.Equal(t, len(body.Sessions), 1)
+	require.Equal(t, body.Sessions[0].ApplicationName, "hello system")
+
+	// Ditto for the test tenant.
+	req = newreq()
+	req.Header.Set(server.TenantSelectHeader, "hello")
+	body, err = get(req)
+	require.NoError(t, err)
+	t.Logf("response 2:\n%#v", body)
+	require.Equal(t, len(body.Sessions), 1)
+	require.Equal(t, body.Sessions[0].ApplicationName, "hello hello")
+
+	c := &http.Cookie{
+		Name:     server.TenantSelectCookieName,
+		Value:    catconstants.SystemTenantName,
+		Path:     "/",
+		HttpOnly: true,
+		Secure:   true,
+	}
+	purl, err := url.Parse(aurl)
+	require.NoError(t, err)
+	client.Jar.SetCookies(purl, []*http.Cookie{c})
+
+	req = newreq()
+	body, err = get(req)
+	require.NoError(t, err)
+	t.Logf("response 3:\n%#v", body)
+	require.Equal(t, len(body.Sessions), 1)
+	require.Equal(t, body.Sessions[0].ApplicationName, "hello system")
+
+	c.Value = "hello"
+	client.Jar.SetCookies(purl, []*http.Cookie{c})
+	req = newreq()
+	body, err = get(req)
+	require.NoError(t, err)
+	t.Logf("response 4:\n%#v", body)
+	require.Equal(t, len(body.Sessions), 1)
+	require.Equal(t, body.Sessions[0].ApplicationName, "hello hello")
+
+	// Finally, do it again with both cookie and header. Verify
+	// that the header wins.
+	req = newreq()
+	req.Header.Set(server.TenantSelectHeader, catconstants.SystemTenantName)
+	body, err = get(req)
+	require.NoError(t, err)
+	t.Logf("response 5:\n%#v", body)
+	require.Equal(t, len(body.Sessions), 1)
+	require.Equal(t, body.Sessions[0].ApplicationName, "hello system")
+}

pkg/cli/cliflags/flags.go

@@ -137,6 +137,18 @@ control is not returned to the shell until the server is ready to
 accept requests.`,
 	}
 
+	DisableInMemoryTenant = FlagInfo{
+		Name:        "disable-in-memory-tenant",
+		Description: `Do not start a secondary tenant in-memory.`,
+	}
+
+	// TODO(knz): Remove this once https://github.com/cockroachdb/cockroach/issues/84604
+	// is addressed.
+	SecondaryTenantPortOffset = FlagInfo{
+		Name:        "secondary-tenant-port-offset",
+		Description: "TCP port number offset to use for the secondary in-memory tenant.",
+	}
+
 	SQLMem = FlagInfo{
 		Name: "max-sql-memory",
 		Description: `

pkg/cli/context.go

@@ -615,7 +615,6 @@ func setDemoContextDefaults() {
 	demoCtx.RunWorkload = false
 	demoCtx.Localities = nil
 	demoCtx.GeoPartitionedReplicas = false
-	demoCtx.DisableTelemetry = false
 	demoCtx.DefaultKeySize = defaultKeySize
 	demoCtx.DefaultCALifetime = defaultCALifetime
 	demoCtx.DefaultCertLifetime = defaultCertLifetime

pkg/cli/demo.go

@@ -135,8 +135,6 @@ func checkDemoConfiguration(
 		return nil, errors.Newf("--%s cannot be used with --%s", cliflags.Global.Name, cliflags.DemoNodeLocality.Name)
 	}
 
-	demoCtx.DisableTelemetry = cluster.TelemetryOptOut()
-
 	// Whether or not we enable enterprise feature is a combination of:
 	//
 	// - whether the user wants them (they can disable enterprise
@@ -284,7 +282,7 @@ func runDemoInternal(
 
 		// Only print details about the telemetry configuration if the
 		// user has control over it.
-		if demoCtx.DisableTelemetry {
+		if cluster.TelemetryOptOut() {
 			cliCtx.PrintlnUnlessEmbedded("#\n# Telemetry disabled by configuration.")
 		} else {
 			cliCtx.PrintlnUnlessEmbedded("#\n" +

pkg/cli/democluster/context.go

@@ -35,9 +35,6 @@ type Context struct {
 	// CacheSize is the size of the storage cache for each KV server.
 	CacheSize int64
 
-	// DisableTelemetry requests that telemetry be disabled.
-	DisableTelemetry bool
-
 	// NoExampleDatabase prevents the auto-creation of a demo database
 	// from a workload.
 	NoExampleDatabase bool

pkg/cli/democluster/demo_cluster.go

@@ -476,14 +476,6 @@ func (c *transientCluster) Start(ctx context.Context) (err error) {
 			}
 		}
 
-		// Start up the update check loop.
-		// We don't do this in (*server.Server).Start() because we don't want this
-		// overhead and possible interference in tests.
-		if !c.demoCtx.DisableTelemetry {
-			c.infoLog(ctx, "starting telemetry")
-			c.firstServer.StartDiagnostics(ctx)
-		}
-
 		return nil
 	}(phaseCtx); err != nil {
 		return err
@@ -532,6 +524,8 @@ func (c *transientCluster) createAndAddNode(
 	if idx == 0 {
 		// The first node also auto-inits the cluster.
 		args.NoAutoInitializeCluster = false
+		// The first node also runs diagnostics (unless disabled by cluster.TelemetryOptOut).
+		args.StartDiagnosticsReporting = true
 	}
 
 	serverKnobs := args.Knobs.Server.(*server.TestingKnobs)

pkg/cli/flags.go

@@ -537,6 +537,11 @@ func init() {
 		if backgroundFlagDefined {
 			cliflagcfg.BoolFlag(f, &startBackground, cliflags.Background)
 		}
+
+		// TODO(knz): Remove this port offset mechanism once we implement
+		// a shared listener. See: https://github.com/cockroachdb/cockroach/issues/84585
+		cliflagcfg.IntFlag(f, &baseCfg.SecondaryTenantPortOffset, cliflags.SecondaryTenantPortOffset)
+		_ = f.MarkHidden(cliflags.SecondaryTenantPortOffset.Name)
 	}
 
 	// Multi-tenancy start-sql command flags.
@@ -1138,6 +1143,9 @@ func extraServerFlagInit(cmd *cobra.Command) error {
 	}
 	serverCfg.LocalityAddresses = localityAdvertiseHosts
 
+	// Ensure that diagnostic reporting is enabled for server startup commands.
+	serverCfg.StartDiagnosticsReporting = true
+
 	return nil
 }