server: use a shared HTTP listener for all in-memory tenant servers #91744

knz · 2022-11-11T14:55:54Z

Parent PRs:

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

cockroach-teamcity · 2022-11-11T14:56:06Z

This change is

knz · 2022-11-11T14:56:45Z

@dhartunian do you want to have a preliminary look to check if the overall direction is satisfactory?

I'm still on the fence about whether to select the tenant via a custom header or a cookie. Maybe we should do both? Let me know what you think.

dhartunian

I'm still on the fence about whether to select the tenant via a custom header or a cookie. Maybe we should do both? Let me know what you think.

I like supporting multiple methods. Cookie is best for the frontend since it allows all requests to flow reliably to the right place. For instance, requests to JS/CSS/images etc. can't have the header attached because the browser makes them. The only way to ensure they come from the correct tenant is to use a cookie. I know we're not serving different assets at the moment, but it's nice to not worry about it.

Reviewed 11 of 11 files at r1, 12 of 12 files at r2, 5 of 5 files at r3, 4 of 7 files at r4, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @knz, and @srosenberg)

pkg/server/server_controller.go line 174 at r4 (raw file):

// the special HTTP request header.
// If no tenant is specified, the default tenant is used.
func (c *serverController) httpMux(w http.ResponseWriter, r *http.Request) {

love how simple this ends up being!

pkg/server/server_controller.go line 277 at r4 (raw file):

func (t *systemServerWrapper) getHTTPHandlerFn() http.HandlerFunc {
	return t.s.http.baseHandler

nit: can we standardize between the t.s and t.server names here?

knz

I like supporting multiple methods.

👍 Done.

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @herkolategan, and @srosenberg)

pkg/server/server_controller.go line 277 at r4 (raw file):

Previously, dhartunian (David Hartunian) wrote…

nit: can we standardize between the t.s and t.server names here?

Done.

dhartunian

Reviewed 1 of 18 files at r5, 18 of 18 files at r9, 12 of 12 files at r10, 5 of 5 files at r11.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @herkolategan, @knz, and @srosenberg)

pkg/server/server_controller.go line 326 at r12 (raw file):

	// Start a goroutine that watches for shutdown requests issued by
	// the server itself.
	go func() {

are we trying to avoid go keyword in non-test code?

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @dhartunian, @herkolategan, and @srosenberg)

pkg/server/server_controller.go line 326 at r12 (raw file):

Previously, dhartunian (David Hartunian) wrote…

are we trying to avoid go keyword in non-test code?

It's a good question. Alas we can't use stopper.RunAsyncTask here because the call to stopper.Stop inside would then deadlock. Clarified in comment.

andreimatei

I don't have opinions on cookie vs header that you're discussing. However, what I know how to edit is the URL in the address bar. Could we also take the tenant there, like we do with the node id for proxying HTTP requests between nodes?

Reviewable status: complete! 1 of 0 LGTMs obtained (waiting on @dhartunian, @herkolategan, @knz, and @srosenberg)

pkg/server/config.go line 214 at r16 (raw file):

	StartDiagnosticsReporting bool

	// DisableOwnHTTPListener prevents this server from starting a TCP

nit: wrap

pkg/server/config.go line 216 at r16 (raw file):

	// DisableOwnHTTPListener prevents this server from starting a TCP
	// listener for the HTTP service. Instead, it is expected
	// that some other service (typically, the server controller)

s/server controller/serverController

pkg/server/config.go line 218 at r16 (raw file):

	// that some other service (typically, the server controller)
	// will accept and route requests instead.
	DisableOwnHTTPListener bool

s/DisableOwnHTTPListener/DisableHTTPListener

pkg/server/server_controller.go line 326 at r12 (raw file):

Previously, knz (Raphael 'kena' Poss) wrote…

It's a good question. Alas we can't use stopper.RunAsyncTask here because the call to stopper.Stop inside would then deadlock. Clarified in comment.

but the comment was added in the wrong commit :P

pkg/server/server_controller.go line 188 at r16 (raw file):

	}
	if tenantName == "" {
		// TODO(knz): Make the default tenant route for HTTP configurable.

Take it or leave it but, in my opinion, TODOs like this serve little purpose other than distracting and making reading the code harder. There's no need for this code to anticipate what might or might not come in the future; the code makes enough sense as it is (TODOs are mostly for when the code makes less than enough sense as it is).

pkg/server/server_controller.go line 194 at r16 (raw file):

	s, err := c.get(ctx, tenantName)
	if err != nil {
		log.Warningf(ctx, "unable to find tserver for tenant %q: %v", tenantName, err)

tserver ?

pkg/server/server_controller.go line 194 at r16 (raw file):

	s, err := c.get(ctx, tenantName)
	if err != nil {
		log.Warningf(ctx, "unable to find tserver for tenant %q: %v", tenantName, err)

s/unable to find/failed to start ?

pkg/server/server_controller.go line 206 at r16 (raw file):

// Used in tests until https://github.com/cockroachdb/cockroach/issues/84585 is resolved.
func (s *Server) TestingGetSQLAddrForTenant(tenant string) string {
	ts, err := s.serverController.get(context.Background(), tenant)

nit: take in a ctx

pkg/server/tenant.go line 330 at r16 (raw file):

	// If DisableOwnHTTPListener is set, we are relying on the HTTP request
	// routing performed by the server controller.

s/server controller/serverController

Epic: CRDB-14537 This commit lays the groundwork for the ability to run tenant servers in-memory. It introduces a new `serverController` with two roles: - it maps tenant names to server instances. - it only starts servers the first time the tenant name is referenced, and only if that tenant is marked active. As of this commit, no subsystem inside CockroachDB refers to this new server controller; this means there is no signal hooked up to start these servers automatically yet. For testing, a debug HTTP endpoint has been added: `/debug/tickle?name=<tenantname>` Example use: 1. start a server. At this point no secondary tenant server is created yet. 2. create a test tenant, e.g. via `select crdb_internal.create_tenant(123, 'hello');`. At this point, the secondary tenant server is still not running. 3. Perform a HTTP request to the debug endpoint, e.g. to `/debug/tickle?name=hello` 4. Observe (e.g. in logs): the secondary server has been started. One can also observe that the controller also serves the name `system` to refer to the system tenant. For now, the secondary servers created this way use separate network listeners for SQL/HTTP/RPC. NB: This mechanism will be superseded when cockroachdb#84604 is addressed. The port number is assigned randomly. To derive a predictable port number for testing (until issue cockroachdb#84604 is addressed), the operator can pass e.g. `--secondary-tenant-port-offset=100` to the start command (for 100 past the base port number). Release note: None

Release note: None

Prior to this patch, we were using the same netutil.Server object to manage both `net.Conn` created to serve HTTP connections, and `net.Conn` created to serve SQL connections. This was confusing, because a lot of the complexity specific for HTTP connections (integration with the HTTP2 query handling, etc) is not required for raw TCP connections as used by pgwire. This commit clarifies this by separating the two roles. This also cleans up the server initialization. Release note: None

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: - manually in the query URL parameters with `tenant_name=...` - explicitly, with the header `X-Cockroach-Tenant` (preferred for CSRF). - with a `tenant` cookie. Release note: None

dhartunian

thanks for adding the query param option.

Reviewable status: complete! 1 of 0 LGTMs obtained (and 1 stale) (waiting on @andreimatei, @herkolategan, and @srosenberg)

knz · 2022-11-28T18:35:38Z

TFYR!

bors r=andreimatei,dhartunian

craig · 2022-11-28T19:33:09Z

Build succeeded:

Bazel Essential CI (Cockroach)

92574: pgwire: unexport ServerMetrics, rename to tenantSpecificMetrics r=stevendanna a=knz Parent PRs: - [x] #84608 - [x] #91739 - [x] #91744 Epic: CRDB-14537 Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>

Prior to this patch, `cockroach demo --multitenant=true --nodes N` was creating N different secondary tenants, with one SQL server per tenant. This was totally besides the point -- to show multi-node scalability, we want N different SQL servers _for the same tenant_. This commit fixes that. Before: ``` system tenant (webui) http://127.0.0.1:8083/demologin?password=demo36514&username=demo (sql) postgresql://demo:demo36514@127.0.0.1:26260/defaultdb?sslmode=require&sslrootcert=%2Ftmp%2Fdemo1584871889%2Fca.crt (sql/jdbc) jdbc:postgresql://127.0.0.1:26260/defaultdb?password=demo36514&sslmode=require&sslrootcert=%2Ftmp%2Fdemo1584871889%2Fca.crt&user=demo (sql/unix) postgresql://demo:demo36514@/defaultdb?host=%2Ftmp%2Fdemo1584871889&port=26260 tenant 1: (webui) https://127.0.0.1:8080/demologin?password=demo36514&username=demo (sql) postgresql://demo:demo36514@127.0.0.1:26257/movr?sslmode=require&sslrootcert=%2Ftmp%2Fdemo1584871889%2Fca-client-tenant.crt (sql/jdbc) jdbc:postgresql://127.0.0.1:26257/movr?password=demo36514&sslmode=require&sslrootcert=%2Ftmp%2Fdemo1584871889%2Fca-client-tenant.crt&user=demo tenant 2: (webui) https://127.0.0.1:8081/demologin?password=demo36514&username=demo (sql) postgresql://demo:demo36514@127.0.0.1:26258/movr?sslmode=require&sslrootcert=%2Ftmp%2Fdemo1584871889%2Fca-client-tenant.crt (sql/jdbc) jdbc:postgresql://127.0.0.1:26258/movr?password=demo36514&sslmode=require&sslrootcert=%2Ftmp%2Fdemo1584871889%2Fca-client-tenant.crt&user=demo tenant 3: ... ``` After: ``` Application tenant: (webui) https://127.0.0.1:8080/demologin?password=demo41322&username=demo (sql) postgresql://demo:demo41322@127.0.0.1:26257/movr?sslmode=require&sslrootcert=%2Ftmp%2Fdemo3364057791%2Fca-client-tenant.crt (sql/jdbc) jdbc:postgresql://127.0.0.1:26257/movr?password=demo41322&sslmode=require&sslrootcert=%2Ftmp%2Fdemo3364057791%2Fca-client-tenant.crt&user=demo System tenant: (webui) http://127.0.0.1:8083/demologin?password=demo41322&username=demo (sql) postgresql://demo:demo41322@127.0.0.1:26260/defaultdb?sslmode=require&sslrootcert=%2Ftmp%2Fdemo3364057791%2Fca.crt (sql/jdbc) jdbc:postgresql://127.0.0.1:26260/defaultdb?password=demo41322&sslmode=require&sslrootcert=%2Ftmp%2Fdemo3364057791%2Fca.crt&user=demo (sql/unix) postgresql://demo:demo41322@/defaultdb?host=%2Ftmp%2Fdemo3364057791&port=26260 ``` Note: this output will be further simplified once we use a single SQL listener for multiple tenants; see cockroachdb#92580; and also when we support using a shared HTTP listener for the demo servers (i.e. when the work from PR cockroachdb#91744 is extended to `cockroach demo`). (no release note since this functionality is not yet exposed to end-users) Release note: None

92580: server,pgwire: use a single SQL listener for multiple tenants r=rafiss a=knz Parent PRs: - [x] #84608 - [x] #91739 - [x] #91744 - [x] #92574 - [x] #92575 - [x] #92576 - [x] #92577 - [x] #92578 - [x] #92579 - [ ] #94901 Fixes #84585. Informs #94310. Epic: CRDB-14537 This commit implements tenant routing using a single SQL network listener. The tenant name can be specified: - via the client status param `options`, e.g. `options=-ccrdb:tenant=hello` - via the database name, e.g. `crdb:tenant-hello.defaultdb`. Release note (backward-incompatible change): If a SQL database is created with a name that starts with `crdb:tenant-` (e.g. `CREATE DATABASE "crdb:tenant-foo"`, clients will no longer be able to connect to it directly. Co-authored-by: Raphael 'kena' Poss <knz@thaumogen.net>

knz force-pushed the 20221111-shared-http branch from 7ecb9ed to 9463567 Compare November 11, 2022 15:54

knz marked this pull request as ready for review November 11, 2022 15:55

knz requested review from a team as code owners November 11, 2022 15:55

knz requested a review from a team November 11, 2022 15:55

knz requested review from a team as code owners November 11, 2022 15:55

knz requested review from a team, herkolategan, srosenberg and dhartunian and removed request for a team November 11, 2022 15:55

knz force-pushed the 20221111-shared-http branch 2 times, most recently from 7341a90 to c1527b4 Compare November 14, 2022 17:01

dhartunian reviewed Nov 15, 2022

View reviewed changes

knz force-pushed the 20221111-shared-http branch from c1527b4 to 4224843 Compare November 15, 2022 16:14

knz commented Nov 15, 2022

View reviewed changes

knz force-pushed the 20221111-shared-http branch from 4224843 to fb68fc1 Compare November 15, 2022 16:35

This was referenced Nov 16, 2022

server,pgwire: use a single SQL listener for multiple tenants #91982

Closed

[DRAFT] server: allow multiple session cookies side-by-side #92063

Closed

dhartunian reviewed Nov 22, 2022

View reviewed changes

dhartunian mentioned this pull request Nov 22, 2022

demo: set tenant names when multitenant #92329

Closed

knz force-pushed the 20221111-shared-http branch from fb68fc1 to 7c952ab Compare November 22, 2022 13:42

knz commented Nov 22, 2022

View reviewed changes

knz force-pushed the 20221111-shared-http branch 2 times, most recently from 42e73d7 to f88d8ca Compare November 22, 2022 14:56

andreimatei approved these changes Nov 22, 2022

View reviewed changes

knz added 4 commits November 28, 2022 16:06

cli,server: make PreStart responsible for starting diagnostics

65c7eb0

Release note: None

knz force-pushed the 20221111-shared-http branch from 3265b76 to 4dd3f06 Compare November 28, 2022 15:08

dhartunian approved these changes Nov 28, 2022

View reviewed changes

craig bot merged commit 8e4d3df into cockroachdb:master Nov 28, 2022

knz deleted the 20221111-shared-http branch November 28, 2022 19:44

dhartunian mentioned this pull request Dec 5, 2022

server: create http middleware that routes to appropriate in-process tenant #91537

Closed

knz mentioned this pull request Dec 27, 2022

cli/demo: sanitize the creation of secondary tenants #94321

Merged

knz mentioned this pull request Jan 8, 2023

server,cli/demo: make the default tenant configurable #94901

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

server: use a shared HTTP listener for all in-memory tenant servers #91744

server: use a shared HTTP listener for all in-memory tenant servers #91744

knz commented Nov 11, 2022 •

edited

Loading

cockroach-teamcity commented Nov 11, 2022

knz commented Nov 11, 2022 •

edited

Loading

dhartunian left a comment

knz left a comment

dhartunian left a comment

knz left a comment

andreimatei left a comment

dhartunian left a comment

knz commented Nov 28, 2022

craig bot commented Nov 28, 2022

server: use a shared HTTP listener for all in-memory tenant servers #91744

server: use a shared HTTP listener for all in-memory tenant servers #91744

Conversation

knz commented Nov 11, 2022 • edited Loading

cockroach-teamcity commented Nov 11, 2022

knz commented Nov 11, 2022 • edited Loading

dhartunian left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

dhartunian left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

andreimatei left a comment

Choose a reason for hiding this comment

dhartunian left a comment

Choose a reason for hiding this comment

knz commented Nov 28, 2022

craig bot commented Nov 28, 2022

knz commented Nov 11, 2022 •

edited

Loading

knz commented Nov 11, 2022 •

edited

Loading