ccl/sqlproxyccl: provide hints to the user whenever params parsing fails #72277
Conversation
|
This change is |
jaylim-crl
force-pushed
the
jay/sqlproxy-error-messages-with-hints
branch
from
55a0789 to
0646354
Compare
jaylim-crl
requested review from
kernfeld-cockroach,
jeffswenson and
andy-kimball
There was a problem hiding this comment.
With this PR, we'll provide hints whenever a user encounters an error message related to cluster identifiers when connecting through the proxy. Here's an example:
cc: @emily-horing @jordanlewis
Reviewable status:
complete! 0 of 0 LGTMs obtained (waiting on @andy-kimball, @jeffswenson, and @kernfeld-cockroach)
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/sqlproxyccl/proxy.go, line 63 at r1 (raw file):
Message: msg, } hint := errors.FlattenHints(codeErr.err)
nit: This could be simplified to
resp := &pgproto3.ErrorResponse {
Severity: "FATAL",
Code: pgCode,
Message: msg,
Hint: errors.FlattenHints(codeErr.err),
}
pkg/ccl/sqlproxyccl/proxy_handler.go, line 742 at r1 (raw file):
const clusterIdentifierHint = ` ensure that your cluster identifier is specified through only one of the following methods:
I like to link to documentation when an error has a clear doc. What do you think about including a link to:
https://www.cockroachlabs.com/docs/cockroachcloud/connect-to-a-serverless-cluster
There was a problem hiding this comment.
TFTRs!
Reviewable status:
pkg/ccl/sqlproxyccl/proxy.go, line 63 at r1 (raw file):
Previously, JeffSwenson (Jeff Swenson) wrote…
nit: This could be simplified to
resp := &pgproto3.ErrorResponse {
Severity: "FATAL",
Code: pgCode,
Message: msg,
Hint: errors.FlattenHints(codeErr.err),
}
Thanks. Will update.
pkg/ccl/sqlproxyccl/proxy_handler.go, line 742 at r1 (raw file):
Previously, JeffSwenson (Jeff Swenson) wrote…
I like to link to documentation when an error has a clear doc. What do you think about including a link to:
https://www.cockroachlabs.com/docs/cockroachcloud/connect-to-a-serverless-cluster
The problem is that this sqlproxy isn't specific to CC. Users could also use it for their own multi-tenant setup if they wanted to. If we linked a doc to cockroachcloud, that would be incorrect.
jaylim-crl
force-pushed
the
jay/sqlproxy-error-messages-with-hints
branch
from
0646354 to
7f7dd17
Compare
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/sqlproxyccl/proxy.go, line 63 at r1 (raw file):
Previously, jaylim-crl (Jay Lim) wrote…
Thanks. Will update.
Done.
There was a problem hiding this comment.
Reviewable status:
pkg/ccl/sqlproxyccl/proxy_handler.go, line 742 at r1 (raw file):
Previously, jaylim-crl (Jay Lim) wrote…
The problem is that this sqlproxy isn't specific to CC. Users could also use it for their own multi-tenant setup if they wanted to. If we linked a doc to cockroachcloud, that would be incorrect.
I agree that we should link to the cloud docs. It's not reasonable to support all use cases, and we're certainly not expecting many people will use SQLProxy in their own systems - no need to worry about that case.
pkg/ccl/sqlproxyccl/proxy_handler.go, line 741 at r2 (raw file):
const clusterIdentifierHint = ` ensure that your cluster identifier is specified through only one of the following methods:
is it really only one? is there a problem using both?
pkg/ccl/sqlproxyccl/proxy_handler.go, line 743 at r2 (raw file):
ensure that your cluster identifier is specified through only one of the following methods: 1) database parameter:
nit: let's capitalize Database parameter
pkg/ccl/sqlproxyccl/proxy_handler.go, line 747 at r2 (raw file):
(e.g. "active-roach-42.defaultdb") 2) options parameter:
nit: let's capitalize Oatabase parameter
There was a problem hiding this comment.
TFTR!
Reviewable status:
pkg/ccl/sqlproxyccl/proxy_handler.go, line 742 at r1 (raw file):
we're certainly not expecting many people will use SQLProxy in their own systems
Agreed. I'll update this accordingly. Also, the docs are slightly outdated; it references cluster-id, which is incorrect:
I'll sort that out too.
pkg/ccl/sqlproxyccl/proxy_handler.go, line 741 at r2 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
is it really only one? is there a problem using both?
Yes. If we specified both today, we'll get an error:
cockroach/pkg/ccl/sqlproxyccl/proxy_handler.go
Lines 595 to 597 in 199c565
There are no issues with using both. We can change that logic, and add a hint if a user specifies two different cluster names. I think back then we enforced this restriction to keep it simple.
jaylim-crl
force-pushed
the
jay/sqlproxy-error-messages-with-hints
branch
from
7f7dd17 to
03251b8
Compare
There was a problem hiding this comment.
Done. I have addressed all concerns. TFTRs!
Reviewable status:
pkg/ccl/sqlproxyccl/proxy_handler.go, line 742 at r1 (raw file):
Previously, jaylim-crl (Jay Lim) wrote…
we're certainly not expecting many people will use SQLProxy in their own systems
Agreed. I'll update this accordingly. Also, the docs are slightly outdated; it references cluster-id, which is incorrect:
I'll sort that out too.
Done. I updated the hint to include the URL that Jeff linked:
#
# Welcome to the CockroachDB SQL shell.
# All statements must be terminated by a semicolon.
# To exit, type: \q.
#
FATAL: codeParamsRoutingFailed: missing cluster identifier in connection string
SQLSTATE: 08004
HINT: Ensure that your cluster identifier is uniquely specified using any of the
following methods:
1) Database parameter:
Use "<cluster identifier>.<database name>" as your database parameter.
(e.g. "postgres://.../active-roach-42.defaultdb?sslmode=...")
2) Options parameter:
Use "--cluster=<cluster identifier>" as the options parameter.
(e.g. "postgres://...&options=--cluster=active-roach-42")
For more details, please visit our docs site at:
https://www.cockroachlabs.com/docs/cockroachcloud/connect-to-a-serverless-cluster
Failed running "sql"
pkg/ccl/sqlproxyccl/proxy_handler.go, line 741 at r2 (raw file):
Previously, jaylim-crl (Jay Lim) wrote…
Yes. If we specified both today, we'll get an error:
cockroach/pkg/ccl/sqlproxyccl/proxy_handler.go
Lines 595 to 597 in 199c565
There are no issues with using both. We can change that logic, and add a hint if a user specifies two different cluster names. I think back then we enforced this restriction to keep it simple.
Done. There won't be issues if we supplied the identifier through both mechanisms now, provided that they are non-ambiguous. However, if a user did something like options="--cluster=X --cluster=Y -c cluster=Z", the proxy will return an error. We can only specify one cluster key within the options parameter. We enforced this constraint for simplicity since we didn't want to parse the entire options string, and note that this is already the existing behavior of the proxy.
pkg/ccl/sqlproxyccl/proxy_handler.go, line 743 at r2 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
nit: let's capitalize Database parameter
Done.
pkg/ccl/sqlproxyccl/proxy_handler.go, line 747 at r2 (raw file):
Previously, jordanlewis (Jordan Lewis) wrote…
nit: let's capitalize Oatabase parameter
Done.
(I like the term Oatabase 😄)
jaylim-crl
force-pushed
the
jay/sqlproxy-error-messages-with-hints
branch
from
03251b8 to
578635d
Compare
|
@rafiss could you PTAL at this updated info? |
There was a problem hiding this comment.
i just had style comments. the wording lgtm, and thank you!
Reviewable status:
pkg/ccl/sqlproxyccl/proxy.go, line 53 at r6 (raw file):
var pgCode string if codeErr.code == codeIdleDisconnect { pgCode = "57P01" // admin shutdown
nit: change to pgCode = pgcode.AdminShutdown
pkg/ccl/sqlproxyccl/proxy.go, line 55 at r6 (raw file):
pgCode = "57P01" // admin shutdown } else { pgCode = "08004" // rejected connection
nit: change to pgCode = pgcode.SQLserverRejectedEstablishmentOfSQLconnection
pkg/ccl/sqlproxyccl/proxy_handler.go, line 592 at r6 (raw file):
if clusterNameFromDB == "" && clusterNameFromOpt == "" { err := errors.New("missing cluster identifier")
optional nit: all of the errors.New could become
pgerror.New(pgcode.SQLserverRejectedEstablishmentOfSQLconnection, "...")
(or pgerror.Newf)
then you wouldn't need the extra pgCode logic in proxy.go
pkg/ccl/sqlproxyccl/proxy_handler.go, line 740 at r6 (raw file):
} const clusterIdentifierHint = `
nit: i think it would be nicer to avoid the extra newline here, so we can remove all the strings.TrimLeft calls
pkg/ccl/sqlproxyccl/proxy_handler.go, line 757 at r6 (raw file):
const clusterNameFormHint = "Cluster identifiers come in the form of <name>-<tenant ID> (e.g. lazy-roach-3)" const missingTenantIDHint = "did you forget to include your tenant ID in the cluster identifier?"
nit: by convention, hints begin with upper case and use proper punctuation. same comment goes for the other hints above
pkg/ccl/sqlproxyccl/proxy_handler.go, line 601 at r7 (raw file):
err := errors.New("multiple different cluster identifiers provided") err = errors.WithHintf(err, "is '%s' or '%s' the identifier for the cluster that you're connecting to?",
nit: use upper case
Previously, whenever params parsing fails during connection startup, an error message is returned without any hints. Users often get confused on what to do next as the message can be ambiguous and provide no action items. This led to a lot of confusion among end users connecting through the sqlproxy, which can be seen in the recent posts in community forums. This commit updates the error messages to also return hints, and provide users with more information. At the same time, we also relabeled "cluster name" to "cluster identifier", which correctly reflects the format that we're using at the moment: <cluster name>-<tenant ID>. Release note: None
… unique Previously, the proxy will return an error if multiple cluster identifiers were specified in both the database and options parameters, regardless of whether they are different or unique. Ideally we should only return an error if the supplied cluster identifiers are non-ambiguous. We went with the constraint earlier to simplify things, but thinking about it again, it seems fine to specify cluster identifiers through both parameters provided that they are unique. This commit changes the behavior of the proxy such that we now allow multiple cluster identifiers to be supplied within the connection string provided that they are unique so that the proxy knows which cluster the connection is for. Release note: None
…entifiers Previously, we used the term "cluster name" in two different ways, one with the tenant ID, and one without. This can be really confusing to readers who are not familiar with the sqlproxy code. Concretely, cluster name is actually the string without the tenant ID. The combination of cluster name and tenant ID is known as the cluster identifier. This commit is purely mechanical to fix comments and relabel variables. Release note: None
jaylim-crl
force-pushed
the
jay/sqlproxy-error-messages-with-hints
branch
from
578635d to
3b1262b
Compare
There was a problem hiding this comment.
TFTR! Addressed all of them except the optional nit.
Reviewable status:
pkg/ccl/sqlproxyccl/proxy.go, line 53 at r6 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: change to
pgCode = pgcode.AdminShutdown
Done.
pkg/ccl/sqlproxyccl/proxy.go, line 55 at r6 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: change to
pgCode = pgcode.SQLserverRejectedEstablishmentOfSQLconnection
Done.
pkg/ccl/sqlproxyccl/proxy_handler.go, line 592 at r6 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
optional nit: all of the errors.New could become
pgerror.New(pgcode.SQLserverRejectedEstablishmentOfSQLconnection, "...")(or
pgerror.Newf)then you wouldn't need the extra pgCode logic in proxy.go
I left it as-is without this suggestion since the proxy only cares about two different types of codes: admin shutdown, and server rejected. If the proxy does end up needing more codes in the future, we can revisit this. I felt that pgcode.SQLserverRejectedEstablishmentOfSQLconnection was too long, and I don't think callers would need to know about it, a least for now.
pkg/ccl/sqlproxyccl/proxy_handler.go, line 740 at r6 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: i think it would be nicer to avoid the extra newline here, so we can remove all the
strings.TrimLeftcalls
Done.
pkg/ccl/sqlproxyccl/proxy_handler.go, line 757 at r6 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: by convention, hints begin with upper case and use proper punctuation. same comment goes for the other hints above
Done.
pkg/ccl/sqlproxyccl/proxy_handler.go, line 601 at r7 (raw file):
Previously, rafiss (Rafi Shamim) wrote…
nit: use upper case
Done.
|
TFTR! bors r=JeffSwenson,rafiss |
|
Build failed (retrying...): |
|
Build succeeded: |
Previously, whenever params parsing fails during connection startup, an error
message is returned without any hints. Users often get confused on what to
do next as the message can be ambiguous and provide no action items. This
led to a lot of confusion among end users connecting through the sqlproxy,
which can be seen in the recent posts in community forums.
This commit updates the error messages to also return hints, and provide users
with more information. At the same time, we also relabeled "cluster name" to
"cluster identifier", which correctly reflects the format that we're using at
the moment: -.
Release note: None