pgwire: add server.cockroach_cloud.max_client_connections_per_gateway cluster setting #100200

alyshanjahani-crl · 2023-03-30T19:40:56Z

This setting specifies a maximum number of external connections that a server can have open at any given time. External is defined as connections that are not root connections. In the future, we can have the definition of external be a configurable list of users.
<0 - Connections are unlimited (existing behavior)
=0 - External connections are disabled

0 - External connections are limited
If a new connection would exceed this limit, an error message is returned as postgres: "cluster connections are limited" with the 53300 error code that corresponds to "too many connections".

This functionality is required for serverless.
Part of: https://cockroachlabs.atlassian.net/browse/CC-9288

Release note: None

cockroach-teamcity · 2023-03-30T19:41:06Z

This change is

knz

Reviewed 4 of 4 files at r1, all commit messages.
Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @alyshanjahani-crl, @jeffswenson, and @rafiss)

-- commits line 18 at r1:
Looks like this deserves a release note.

pkg/sql/conn_executor.go line 788 at r1 (raw file):

// IncrementConnectionCount increases connectionCount by 1.
// If isRoot is true then it also increases rootConnectionCount by 1.

I recommend you make two functions (root vs non-root) side by side. Bool arguments make reading the code harder.

pkg/sql/pgwire/conn.go line 721 at r1 (raw file):

		}

		if retErr = c.checkMaxConnections(ctx, sqlServer); retErr != nil {

Q for @rafiss unrelated to this PR: should we check the max nr of connections before starting authentication? This way we can also contrain the amount of server-side authn work via the conn limit.

pkg/sql/pgwire/server.go line 104 at r1 (raw file):

// Note(alyshan): This setting is not public.
var maxNumExternalConnections = settings.RegisterIntSetting(
	settings.TenantReadOnly,

I don't understand why it's tenantreadonly. Why not tenantwritable like the other one?

(The cc control plane can still override as usual)

alyshanjahani-crl

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

-- commits line 18 at r1:
I felt that this change was not intended for external users, but just Cockroach Cloud.

Do NOT write release note texts for internal changes, such as:
Functionality that is not accessible to, or intended for, external users (e.g., multi-tenant capabilities)

pkg/sql/conn_executor.go line 788 at r1 (raw file):

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

I recommend you make two functions (root vs non-root) side by side. Bool arguments make reading the code harder.

In this case of an incoming root connection, are you imaging the caller make two calls IncrementConnectionCount and IncrementRootConnectionCount ?

Or one call to IncrementRootConnectionCount that increments both connectionCount and rootConnectionCount?

pkg/sql/pgwire/server.go line 104 at r1 (raw file):

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

I don't understand why it's tenantreadonly. Why not tenantwritable like the other one?

(The cc control plane can still override as usual)

I do not want serverless users to be able to discover and alter this setting on their own.

CC will set this setting for individual tenants through the system tenant.

knz

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @alyshanjahani-crl, @jeffswenson, and @rafiss)

pkg/sql/conn_executor.go line 788 at r1 (raw file):

Previously, alyshanjahani-crl wrote…

In this case of an incoming root connection, are you imaging the caller make two calls IncrementConnectionCount and IncrementRootConnectionCount ?

Or one call to IncrementRootConnectionCount that increments both connectionCount and rootConnectionCount?

the latter.

pkg/sql/pgwire/server.go line 104 at r1 (raw file):

I do not want serverless users to be able to discover and alter this setting on their own.

They would not be able to alter it if CC orchestration sets it via an override. Overrides cannot be altered by users.

IMHO there is no good reason to make it TenantReadOnly since it's the same type as the other one just above.

alyshanjahani-crl

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

pkg/sql/conn_executor.go line 788 at r1 (raw file):

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

the latter.

Done.

pkg/sql/pgwire/server.go line 104 at r1 (raw file):

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

I do not want serverless users to be able to discover and alter this setting on their own.

They would not be able to alter it if CC orchestration sets it via an override. Overrides cannot be altered by users.

IMHO there is no good reason to make it TenantReadOnly since it's the same type as the other one just above.

Oh gotchya, that makes sense to me now.

Changed to TenantWritable

rafiss

just had minor comments

i was wondering - is there a risk for confusion if this new setting and the old max_per_gateway setting are used at the same time? does one take precedence?

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @alyshanjahani-crl, @jeffswenson, and @knz)

pkg/sql/conn_executor.go line 804 at r2 (raw file):

// DecrementConnectionCount decreases connectionCount by 1.
// If isRoot is true then it also decreases rootConnectionCount by 1.

nit: this comment seems outdated

pkg/sql/pgwire/conn.go line 721 at r1 (raw file):

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

Q for @rafiss unrelated to this PR: should we check the max nr of connections before starting authentication? This way we can also contrain the amount of server-side authn work via the conn limit.

i believe we'd discussed this before, and decided no. this would allow someone who has no access to the cluster to spam connection attempts, and thereby lock out legitimate users from ever being able to begin authentication attempts.

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

// Note(alyshan): One might suggest this cluster setting be named server.max_non_root_connections_per_gateway
// as that reflects its current behaviour of excluding root connections from being limited. However, I chose
// to use the term "external" so that this setting can be extended to exclude connections from an arbitrary

the name "external connection" is unfortunate here. that already has a definition in our product https://www.cockroachlabs.com/docs/stable/create-external-connection.html

can we just name this maxNumNonRootConnections? and name the setting server.max_non_root_connections_per_gateway?

can you say more about how you imagine extending the setting?

alyshanjahani-crl

does one take precedence?

The one im adding maxNonRoot is checked first since it is more restrictive. But they won't interfere with each other.

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

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

Previously, rafiss (Rafi Shamim) wrote…

the name "external connection" is unfortunate here. that already has a definition in our product https://www.cockroachlabs.com/docs/stable/create-external-connection.html

can we just name this maxNumNonRootConnections? and name the setting server.max_non_root_connections_per_gateway?

can you say more about how you imagine extending the setting?

Currently when CockroachCloud performs operations on serverless clusters (like the console page listing sql users, creating databases etc, doing a CRDB version upgrade etc..) they connect as root.

The intention behind this cluster setting is to let an administrator access the cluster but limit other users. Currently the root user is this administrator. But that might not always be the case, we might have more fine grained access control and so i was imagining extending this setting one day with another setting where one can provide which users should be exempted / treated like root in this case.

I wanted to avoid a renaming of cluster setting in the future, and so i thought of maxExternalConnections.

I suppose i would be fine with server.max_non_root_connections_per_gateway, and if we ever needed to extend the functionality of the setting as i described above, we would make it explicit/clear in the description of this setting that it can permit more than just root connections via using an additional setting.

jeffswenson

From a high level architecture standpoint, I agree that we should implement this in the sql server instead of the sql proxy. The one downside is adding a setting to the sql server means we need to support it O(forever) because removing the setting will be a breaking change. That means we need to take some time and make sure the setting makes sense outside of the context of CC. We also need to think about how throttling will interact with a user that has configured the setting themselves.

As for the design, I'm hesitant to add a new setting that has a behavior very similar to an existing setting, because there are already a lot of settings and I don't want to make things more confusing.

Here are a few ideas:

We add a setting which defines which users are exempt from checkMaxConnections. This feature would be more useful for non CC users and we can override the two settings together
We stick with the somewhat hacky sql proxy change, because we can remove the feature if we no longer want to support it the future. E.g. once we have a GUI sql shell, maybe we limit all connections at the sql proxy and only allow connections through the GUI shell.

alyshanjahani-crl

I spoke w. Jeff offline, the setting being private/reserved actually addresses his concerns. With the caveat of including in the description of the setting a disclaimer/warning that this is intended to be used by Cockroach Cloud, as well as a comment in the usage of the setting in checkMaxConnections

And on a similar note, a request to include cloud somewhere in the name of the setting.

Before I go ahead and change the name of the setting again, can we agree upon a name here first?
Is there any rules around what can be the prefix of the cluster setting name @rafiss ? for ex. instead of server.max_conn... could we do cloud.max_conn... ?
If we can that my preferred cluster setting name is cloud.max_connections_per_gateway with a description like so

This setting is intended to be used by Cockroach Cloud for limiting connections to serverless clusters.
The maximum number of connections per gateway allowed at a given time
(note: this will only limit future connection attempts and will not affect already established connections).
Negative values result in unlimited number of connections. Cockroach cloud internal users (including root user) are not affected by this limit."

Alternatively: server.max_non_cloud_connections_per_gateway

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

rafiss · 2023-04-05T19:03:04Z

what about server.max_cockroach_cloud_connections_per_gateway. (i want to use cockroach_cloud instead of cloud, since cloud is used in other parts of CRDB code to refer to the idea of connecting to resources like S3 or GCE)

the general convention is that the first part of the cluster setting name relates to the code/component that is being configured.

alyshanjahani-crl

ack, i've used server.max_non_cockroach_cloud_connections_per_gateway . It is a mouthful, but to re-iterate, this is a setting that is private, only intended for cockroach cloud, and will possibly be removed in the future when we have a better way to limit connections to serverless clusters.

TFTR

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

jeffswenson

LGTM

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @alyshanjahani-crl and @knz)

pkg/sql/pgwire/server.go line 104 at r6 (raw file):

var maxNumNonRootConnections = settings.RegisterIntSetting(
	settings.TenantWritable,
	"server.max_non_cockroach_cloud_connections_per_gateway",

nit: it should be server.max_cockroach_cloud_connections_per_gateway

sorry for the review churn! feel free to merge after fixing this.

rafiss

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @alyshanjahani-crl and @knz)

pkg/sql/pgwire/server.go line 104 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…

nit: it should be server.max_cockroach_cloud_connections_per_gateway

sorry for the review churn! feel free to merge after fixing this.

well, my idea was "cockroach cloud connection" refers to someone connecting to cockroach cloud

but i see in your description that you are thinking of it differently maybe?

jeffswenson · 2023-04-06T21:04:21Z

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @alyshanjahani-crl and @knz)

pkg/sql/pgwire/server.go line 104 at r6 (raw file):

Previously, rafiss (Rafi Shamim) wrote…
well, my idea was "cockroach cloud connection" refers to someone connecting to cockroach cloud

but i see in your description that you are thinking of it differently maybe?

The intent of the setting is to differentiate between connections from the CC control plane vs connections from the CC user. What about the name server.cockroach_cloud_client_connection_limit?

jeffswenson · 2023-04-06T21:08:08Z

Reviewable status: complete! 0 of 0 LGTMs obtained (waiting on @alyshanjahani-crl and @knz)

pkg/sql/pgwire/server.go line 104 at r6 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
well, my idea was "cockroach cloud connection" refers to someone connecting to cockroach cloud
but i see in your description that you are thinking of it differently maybe?

The intent of the setting is to differentiate between connections from the CC control plane vs connections from the CC user. What about the name server.cockroach_cloud_client_connection_limit?

Or maybe
server.cockroach_cloud.max_client_connections.

rafiss · 2023-04-07T00:12:41Z

server.cockroach_cloud.max_client_connections_per_gateway sounds like a good one to me. (the per_gateway part is important, i think)

… cluster setting This setting specifies a maximum number of non root connections that a server can have open at any given time. <0 - Connections are unlimited (existing behavior) =0 - Non root connections are disabled >0 - Non root connections are limited If a new connection would exceed this limit, an error message is returned as postgres: "cluster connections are limited" with the 53300 error code that corresponds to "too many connections". This functionality is required for serverless. Part of: https://cockroachlabs.atlassian.net/browse/CC-9288 Release note: None

rafiss

lgtm! thanks for the attention to detail

rafiss · 2023-04-10T19:02:20Z

pkg/sql/pgwire/server.go

@@ -82,7 +82,11 @@ var logSessionAuth = settings.RegisterBoolSetting(
 	"if set, log SQL session login/disconnection events (note: may hinder performance on loaded nodes)",
 	false).WithPublic()

-var maxNumConnections = settings.RegisterIntSetting(
+// TODO(alyshan): This setting is enforcing max number of connections with superusers not being affected by


good call on the TODO. we should do this one:

update the description to say "the maximum number of connections per gateway ... Superusers are not affected by this limit"

(our team can do this in a later PR if that's easier)

In spirit of breather week:

#101177

rafiss · 2023-04-10T19:03:09Z

pkg/sql/pgwire/server.go

+)
+
+// maxNumNonRootConnectionsReason is used to supplement the error message for connections that denied due to
+// server.max_non_root_connections_per_gateway.


super nit: this comment refers to the old name

thanks for catching, fixed

…_reason cluster setting This setting can be used to customize the error message returned when connections are denied due to a limit specified via server.cockroach_cloud.max_client_connections_per_gateway. This functionality is required for serverless. Being able to indicate to the user why a limit is placed on their number of connections will support a better UX. Specifically, indicating to the user which resource limits they have hit. Part of: https://cockroachlabs.atlassian.net/browse/CC-9288 Release note: None

alyshanjahani-crl · 2023-04-11T17:32:58Z

bors r+

craig · 2023-04-11T18:04:10Z

Build succeeded:

Bazel Essential CI (Cockroach)

alyshanjahani-crl · 2023-04-11T18:34:27Z

blathers backport 22.2 23.1

alyshanjahani-crl requested a review from a team March 30, 2023 19:40

alyshanjahani-crl requested a review from a team as a code owner March 30, 2023 19:40

alyshanjahani-crl requested review from rafiss and jeffswenson March 30, 2023 19:47

knz reviewed Mar 31, 2023

View reviewed changes

alyshanjahani-crl commented Mar 31, 2023

View reviewed changes

knz reviewed Apr 1, 2023

View reviewed changes

alyshanjahani-crl force-pushed the max-external-conns-cluster-setting branch from a05ec83 to bce5c02 Compare April 3, 2023 16:14

alyshanjahani-crl commented Apr 3, 2023

View reviewed changes

alyshanjahani-crl force-pushed the max-external-conns-cluster-setting branch from 7e71266 to 50d374e Compare April 4, 2023 18:18

rafiss requested changes Apr 4, 2023

View reviewed changes

alyshanjahani-crl force-pushed the max-external-conns-cluster-setting branch 2 times, most recently from 5b021bf to 3fb40b6 Compare April 4, 2023 20:02

alyshanjahani-crl commented Apr 4, 2023

View reviewed changes

jeffswenson reviewed Apr 4, 2023

View reviewed changes

alyshanjahani-crl commented Apr 5, 2023

View reviewed changes

alyshanjahani-crl force-pushed the max-external-conns-cluster-setting branch from 3fb40b6 to 0e6ff41 Compare April 6, 2023 20:38

alyshanjahani-crl commented Apr 6, 2023

View reviewed changes

jeffswenson approved these changes Apr 6, 2023

View reviewed changes

rafiss approved these changes Apr 6, 2023

View reviewed changes

rafiss reviewed Apr 6, 2023

View reviewed changes

alyshanjahani-crl force-pushed the max-external-conns-cluster-setting branch from 0e6ff41 to 660a05a Compare April 7, 2023 23:58

alyshanjahani-crl force-pushed the max-external-conns-cluster-setting branch from 660a05a to d3a9bf0 Compare April 10, 2023 17:08

rafiss approved these changes Apr 10, 2023

View reviewed changes

alyshanjahani-crl force-pushed the max-external-conns-cluster-setting branch from d3a9bf0 to 02f420a Compare April 10, 2023 22:04

rafiss changed the title ~~pgwire: add server.max_external_connections_per_gateway cluster setting~~ pgwire: add server.cockroach_cloud.max_client_connections_per_gateway cluster setting Apr 11, 2023

craig bot merged commit 5fd45da into cockroachdb:master Apr 11, 2023

alyshanjahani-crl added backport-22.2.x backport-23.1.x Flags PRs that need to be backported to 23.1 labels Apr 11, 2023

This was referenced Apr 11, 2023

release-22.2: pgwire: add server.cockroach_cloud.max_client_connections_per_gateway cluster setting #101239

Merged

release-23.1: pgwire: add server.cockroach_cloud.max_client_connections_per_gateway cluster setting #101241

Merged

alyshanjahani-crl mentioned this pull request Apr 19, 2023

sqlproxyccl: Add support for limiting non root connections #99056

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pgwire: add server.cockroach_cloud.max_client_connections_per_gateway cluster setting #100200

pgwire: add server.cockroach_cloud.max_client_connections_per_gateway cluster setting #100200

alyshanjahani-crl commented Mar 30, 2023

cockroach-teamcity commented Mar 30, 2023

knz left a comment

alyshanjahani-crl left a comment

knz left a comment

alyshanjahani-crl left a comment

rafiss left a comment

alyshanjahani-crl left a comment

jeffswenson left a comment

alyshanjahani-crl left a comment

rafiss commented Apr 5, 2023

alyshanjahani-crl left a comment

jeffswenson left a comment

rafiss left a comment

rafiss left a comment

jeffswenson commented Apr 6, 2023

jeffswenson commented Apr 6, 2023

rafiss commented Apr 7, 2023

rafiss left a comment

rafiss Apr 10, 2023

alyshanjahani-crl Apr 10, 2023

rafiss Apr 10, 2023

alyshanjahani-crl Apr 10, 2023

alyshanjahani-crl commented Apr 11, 2023

craig bot commented Apr 11, 2023

alyshanjahani-crl commented Apr 11, 2023

pgwire: add server.cockroach_cloud.max_client_connections_per_gateway cluster setting #100200

pgwire: add server.cockroach_cloud.max_client_connections_per_gateway cluster setting #100200

Conversation

alyshanjahani-crl commented Mar 30, 2023

cockroach-teamcity commented Mar 30, 2023

knz left a comment

Choose a reason for hiding this comment

alyshanjahani-crl left a comment

Choose a reason for hiding this comment

knz left a comment

Choose a reason for hiding this comment

alyshanjahani-crl left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

alyshanjahani-crl left a comment

Choose a reason for hiding this comment

jeffswenson left a comment

Choose a reason for hiding this comment

alyshanjahani-crl left a comment

Choose a reason for hiding this comment

rafiss commented Apr 5, 2023

alyshanjahani-crl left a comment

Choose a reason for hiding this comment

jeffswenson left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

rafiss left a comment

Choose a reason for hiding this comment

jeffswenson commented Apr 6, 2023

jeffswenson commented Apr 6, 2023

rafiss commented Apr 7, 2023

rafiss left a comment

Choose a reason for hiding this comment

rafiss Apr 10, 2023

Choose a reason for hiding this comment

alyshanjahani-crl Apr 10, 2023

Choose a reason for hiding this comment

rafiss Apr 10, 2023

Choose a reason for hiding this comment

alyshanjahani-crl Apr 10, 2023

Choose a reason for hiding this comment

alyshanjahani-crl commented Apr 11, 2023

craig bot commented Apr 11, 2023

alyshanjahani-crl commented Apr 11, 2023