Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

pgwire: create a framework for pre-serve connection logic #92575

Merged
merged 2 commits into from
Dec 19, 2022

Conversation

@knz knz self-assigned this Nov 28, 2022
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@knz knz changed the title pgwire: simplify writeErr/sendErr pgwire: create a framework for pre-serve connection logic Nov 28, 2022
@knz knz added the A-multitenancy Related to multi-tenancy label Nov 28, 2022
@knz knz marked this pull request as ready for review November 28, 2022 15:42
@knz knz requested a review from a team November 28, 2022 15:42
@knz knz requested a review from a team as a code owner November 28, 2022 15:42
@knz knz requested a review from rafiss November 28, 2022 16:57
Copy link
Collaborator

@rafiss rafiss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz)


-- commits line 15 at r2:
just a question: why is it named sql.pre_serve... and not sql.pre_auth...? i feel that from a client's perspective, the connection is already being "served" once it reaches the business logic inside of CRDB. they shouldn't need to care about the various server objects we have in our code.

also, will this make it harder for serverless users to see where their credits are being used in the case of high authentication load? or do these pre_serve bytes no longer count against them? (if so, that would be worth making explicit in the release notes)


pkg/sql/pgwire/pre_serve.go line 55 at r2 (raw file):

type PreServeConnHandler struct {
	errWriter errWriter
	cfg       struct {

why do we need this empty cfg? explain with a comment? (and maybe don't put the } on a new line)


pkg/sql/pgwire/pre_serve.go line 106 at r2 (raw file):

) (err error) {
	defer func() {
		if err != nil {

when would err ever be non-nil?


pkg/sql/pgwire/server.go line 1402 at r2 (raw file):

// sequence. Later error sends during/after authentication are handled
// in conn.go.
func (s *Server) sendErr(ctx context.Context, conn net.Conn, err error) error {

perhaps the function's job would be more clear if it was renamed sendPreAuthErr or sendPreServeErr?

@knz
Copy link
Contributor Author

knz commented Nov 28, 2022

why is it named sql.pre_serve... and not sql.pre_auth...?

  • why not pre_auth: because there is more remaining tenant-specific logic pre-authentication. The split we're designing is not about pre-auth vs post-auth; it's pre-tenant-assignment vs post-tenant-assignment, with some remaining pre-auth work to do after tenant assignment.

  • why pre_serve: because i'm extracting the work done before ServeConn is called. (it's pre-ServeConn really, hence PreServeConnHandler)

@knz
Copy link
Contributor Author

knz commented Nov 28, 2022

will this make it harder for serverless users to see where their credits are being used in the case of high authentication load?

I don't see it - as you'll see in the PR chain, the amount of pre-serve work is minimal.

@knz knz requested a review from a team November 28, 2022 18:26
@dhartunian dhartunian removed the request for review from a team November 29, 2022 16:48
Copy link
Contributor

@abarganier abarganier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @knz)


pkg/sql/pgwire/pre_serve.go line 75 at r5 (raw file):

// tenantIndependentMetrics is the set of metrics for the
// pre-serve part of connection handling, before the connection
// is routed to a specific tenant.

nit: until tsdb supports tenant cardinality (or maybe I missed it... has this been implemented?) can we specify the semantics of "tenant independent metrics"? Are they stored with tenant-level cardinality, or just available in _status/vars / in-memory?

Code quote:

// tenantIndependentMetrics is the set of metrics for the
// pre-serve part of connection handling, before the connection
// is routed to a specific tenant.

Base automatically changed from 20221116-shared-sql-1 to 20221116-shared-sql-base November 29, 2022 22:39
Copy link
Contributor Author

@knz knz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @abarganier, @knz, and @rafiss)


-- commits line 15 at r2:

Previously, rafiss (Rafi Shamim) wrote…

just a question: why is it named sql.pre_serve... and not sql.pre_auth...? i feel that from a client's perspective, the connection is already being "served" once it reaches the business logic inside of CRDB. they shouldn't need to care about the various server objects we have in our code.

also, will this make it harder for serverless users to see where their credits are being used in the case of high authentication load? or do these pre_serve bytes no longer count against them? (if so, that would be worth making explicit in the release notes)

  • why not pre_auth: because there is more remaining tenant-specific logic pre-authentication. The split we're designing is not about pre-auth vs post-auth; it's pre-tenant-assignment vs post-tenant-assignment, with some remaining pre-auth work to do after tenant assignment.

  • why pre_serve: because i'm extracting the work done before ServeConn is called. (it's pre-ServeConn really, hence PreServeConnHandler)


pkg/sql/pgwire/pre_serve.go line 55 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

why do we need this empty cfg? explain with a comment? (and maybe don't put the } on a new line)

We don't need it. it was dead code. removed.


pkg/sql/pgwire/pre_serve.go line 106 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

when would err ever be non-nil?

This is just a skeleton for the PreServe function that gets populated further in #92577. At that point, err can become non-nil.


pkg/sql/pgwire/pre_serve.go line 75 at r5 (raw file):

Previously, abarganier (Alex Barganier) wrote…

nit: until tsdb supports tenant cardinality (or maybe I missed it... has this been implemented?) can we specify the semantics of "tenant independent metrics"? Are they stored with tenant-level cardinality, or just available in _status/vars / in-memory?

They are tenant-independent so by definition they are not tenant-level. They are just regular metrics and stored alongside the others. What type of specification do you have in mind?


pkg/sql/pgwire/server.go line 1402 at r2 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

perhaps the function's job would be more clear if it was renamed sendPreAuthErr or sendPreServeErr?

I guess that's reasonable. Done.

This commit introduces `PreServeConnHandler`, which will
handle the preparation of a SQL connection prior to handing it off to
a specific tenant server.

Release note (ops changes): The count of network bytes sent
to report re-authentication errors to a SQL client is now
reported via the metric `sql.pre_serve.bytesout` (instead of
`sql.bytesout` previously). The count of pre-authentication errors
is now reported via the metric `sql.pre_serve.conn.failures` (instead
of `sql.conn.failures` previously).

Release note (ops changes): The count of new SQL connections
is now also reported on `sql.pre_serve.new_conns`.
@knz knz merged this pull request into 20221116-shared-sql-base Dec 19, 2022
@knz knz deleted the 20221116-shared-sql-2 branch December 19, 2022 08:41
craig bot pushed a commit that referenced this pull request Jan 9, 2023
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-multitenancy Related to multi-tenancy
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants