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,server: clarify to SQL clients when they select the wrong tenant #96345

Merged
merged 1 commit into from
Feb 2, 2023

Conversation

knz
Copy link
Contributor

@knz knz commented Feb 1, 2023

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

@knz knz added the A-multitenancy Related to multi-tenancy label Feb 1, 2023
@knz knz requested review from stevendanna, rafiss, ecwall and a team February 1, 2023 14:56
@knz knz requested review from a team as code owners February 1, 2023 14:56
@knz knz requested a review from a team February 1, 2023 14:56
@knz knz requested a review from a team as a code owner February 1, 2023 14:56
@cockroach-teamcity
Copy link
Member

This change is Reviewable

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
@knz knz force-pushed the 20230201-tenant-conn-error branch from 491ff13 to 99d6592 Compare February 1, 2023 15:08
@dhartunian dhartunian removed the request for review from a team February 1, 2023 15:48
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.

nice improvement! had one question

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


pkg/sql/pgwire/pre_serve.go line 215 at r1 (raw file):

		pgerror.Newf(pgcode.ConnectionException,
			"service unavailable for target tenant (%v)", tenantName),
		`Double check your "-ccluster=" connection option or your "cluster:" database name prefix.`)

would it be possible to get this error is someone were using our SNI routing feature? if so, this hint would be confusing, as they are not expected to provide a -c cluster option in that case

@knz
Copy link
Contributor Author

knz commented Feb 1, 2023

I have drafted the following answer to your question:

at this point, no CC serverless client would ever see the new error, because those connections would never be routed to a host/storage node.
in the future, we may want to support SNI to route SQL clients to 1 particular tenant out of many "hosted" using shared-process multitenancy on the same process. When that time comes, we can extend the error message to mention SNI.

now asking for serverless friends for confirmation

@knz
Copy link
Contributor Author

knz commented Feb 2, 2023

@rafiss here's the response from Jeff - WDYT?

at this point, no CC serverless client would ever see the new error, because those connections would never be routed to a host/storage node.

This is correct. If network level security is working correctly there should be no way for a Serverless user to reach the storage servers with a sql connection. If they are able to reach the storage servers.

in the future, we may want to support SNI to route SQL clients to 1 particular tenant out of many “hosted” using shared-process multitenancy on the same process. When that time comes, we can extend the error message to mention SNI.

I see a future where this is useful for non-serverless clusters, but I don’t think Serverless would ever offer this in CC. If it were an option we may use it to simplify certain maintenance tasks.

@knz knz requested a review from rafiss February 2, 2023 18:41
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.

thanks for checking!

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

@knz
Copy link
Contributor Author

knz commented Feb 2, 2023

cheers!

bors r=rafiss

@craig
Copy link
Contributor

craig bot commented Feb 2, 2023

This PR was included in a batch that was canceled, it will be automatically retried

@craig
Copy link
Contributor

craig bot commented Feb 2, 2023

Build failed (retrying...):

@craig
Copy link
Contributor

craig bot commented Feb 2, 2023

Build succeeded:

@craig craig bot merged commit 3f73cfb into cockroachdb:master Feb 2, 2023
@knz knz deleted the 20230201-tenant-conn-error branch February 2, 2023 23:31
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
3 participants