From 7f7dd17c0056f6553c7bcecbf4027cb3b24eef8f Mon Sep 17 00:00:00 2001 From: Jay Date: Mon, 1 Nov 2021 01:05:29 -0400 Subject: [PATCH] ccl/sqlproxyccl: provide hints to the user whenever params parsing fails 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 --- pkg/ccl/sqlproxyccl/BUILD.bazel | 1 + pkg/ccl/sqlproxyccl/proxy.go | 2 + pkg/ccl/sqlproxyccl/proxy_handler.go | 49 ++++++++++++++--- pkg/ccl/sqlproxyccl/proxy_handler_test.go | 66 ++++++++++++++--------- 4 files changed, 86 insertions(+), 32 deletions(-) diff --git a/pkg/ccl/sqlproxyccl/BUILD.bazel b/pkg/ccl/sqlproxyccl/BUILD.bazel index c156cb81fd6c..b8e3002f07cc 100644 --- a/pkg/ccl/sqlproxyccl/BUILD.bazel +++ b/pkg/ccl/sqlproxyccl/BUILD.bazel @@ -68,6 +68,7 @@ go_test( "//pkg/server", "//pkg/sql", "//pkg/sql/pgwire", + "//pkg/sql/pgwire/pgerror", "//pkg/testutils", "//pkg/testutils/serverutils", "//pkg/testutils/skip", diff --git a/pkg/ccl/sqlproxyccl/proxy.go b/pkg/ccl/sqlproxyccl/proxy.go index fe09dcf7fb57..ff815f17ca99 100644 --- a/pkg/ccl/sqlproxyccl/proxy.go +++ b/pkg/ccl/sqlproxyccl/proxy.go @@ -54,10 +54,12 @@ func toPgError(err error) *pgproto3.ErrorResponse { } else { pgCode = "08004" // rejected connection } + return &pgproto3.ErrorResponse{ Severity: "FATAL", Code: pgCode, Message: msg, + Hint: errors.FlattenHints(codeErr.err), } } // Return a generic "internal server error" message. diff --git a/pkg/ccl/sqlproxyccl/proxy_handler.go b/pkg/ccl/sqlproxyccl/proxy_handler.go index ad6a5e0b0940..fd5d9b1af474 100644 --- a/pkg/ccl/sqlproxyccl/proxy_handler.go +++ b/pkg/ccl/sqlproxyccl/proxy_handler.go @@ -589,11 +589,15 @@ func clusterNameAndTenantFromParams( } if clusterNameFromDB == "" && clusterNameFromOpt == "" { - return msg, "", roachpb.MaxTenantID, errors.New("missing cluster name in connection string") + err := errors.New("missing cluster identifier in connection string") + err = errors.WithHint(err, strings.TrimLeft(clusterIdentifierHint, "\n")) + return msg, "", roachpb.MaxTenantID, err } if clusterNameFromDB != "" && clusterNameFromOpt != "" { - return msg, "", roachpb.MaxTenantID, errors.New("multiple cluster names provided") + err := errors.New("multiple cluster identifiers provided") + err = errors.WithHint(err, strings.TrimLeft(clusterIdentifierHint, "\n")) + return msg, "", roachpb.MaxTenantID, err } if clusterNameFromDB == "" { @@ -602,27 +606,42 @@ func clusterNameAndTenantFromParams( sepIdx := strings.LastIndex(clusterNameFromDB, clusterTenantSep) - // Cluster name provided without a tenant ID in the end. + // Cluster identifier provided without a tenant ID in the end. if sepIdx == -1 || sepIdx == len(clusterNameFromDB)-1 { - return msg, "", roachpb.MaxTenantID, errors.Errorf("invalid cluster name '%s'", clusterNameFromDB) + err := errors.Errorf("invalid cluster identifier '%s'", clusterNameFromDB) + err = errors.WithHint(err, missingTenantIDHint) + err = errors.WithHint(err, clusterNameFormHint) + return msg, "", roachpb.MaxTenantID, err } clusterNameSansTenant, tenantIDStr := clusterNameFromDB[:sepIdx], clusterNameFromDB[sepIdx+1:] + + // Cluster name does not conform to specified format. if !clusterNameRegex.MatchString(clusterNameSansTenant) { - return msg, "", roachpb.MaxTenantID, errors.Errorf("invalid cluster name '%s'", clusterNameFromDB) + err := errors.Errorf("invalid cluster identifier '%s'", clusterNameFromDB) + err = errors.WithHintf(err, "is '%s' a valid cluster name?", clusterNameSansTenant) + err = errors.WithHint(err, clusterNameFormHint) + return msg, "", roachpb.MaxTenantID, err } + // Tenant ID cannot be parsed. tenID, err := strconv.ParseUint(tenantIDStr, 10, 64) if err != nil { // Log these non user-facing errors. log.Errorf(ctx, "cannot parse tenant ID in %s: %v", clusterNameFromDB, err) - return msg, "", roachpb.MaxTenantID, errors.Errorf("invalid cluster name '%s'", clusterNameFromDB) + err := errors.Errorf("invalid cluster identifier '%s'", clusterNameFromDB) + err = errors.WithHintf(err, "is '%s' a valid tenant ID?", tenantIDStr) + err = errors.WithHint(err, clusterNameFormHint) + return msg, "", roachpb.MaxTenantID, err } + // This case only happens if tenID is 0 or 1 (system tenant). if tenID < roachpb.MinTenantID.ToUint64() { // Log these non user-facing errors. log.Errorf(ctx, "%s contains an invalid tenant ID", clusterNameFromDB) - return msg, "", roachpb.MaxTenantID, errors.Errorf("invalid cluster name '%s'", clusterNameFromDB) + err := errors.Errorf("invalid cluster identifier '%s'", clusterNameFromDB) + err = errors.WithHintf(err, "tenant ID %d is invalid", tenID) + return msg, "", roachpb.MaxTenantID, err } // Make and return a copy of the startup msg so the original is not modified. @@ -717,3 +736,19 @@ func parseOptionsParam(optionsParam string) (clusterName, newOptionsParam string newOptionsParam = strings.TrimSpace(newOptionsParam) return matches[0][1], newOptionsParam, nil } + +const clusterIdentifierHint = ` +ensure that your cluster identifier is specified through only one of the following methods: + +1) database parameter: + Use "." as your database parameter. + (e.g. "active-roach-42.defaultdb") + +2) options parameter: + Use "--cluster=" as the options parameter in your connection string. + (e.g. "postgres://...&options=--cluster=active-roach-42") +` + +const clusterNameFormHint = "Cluster identifiers come in the form of - (e.g. lazy-roach-3)" + +const missingTenantIDHint = "did you forget to include your tenant ID in the cluster identifier?" diff --git a/pkg/ccl/sqlproxyccl/proxy_handler_test.go b/pkg/ccl/sqlproxyccl/proxy_handler_test.go index 00c9ab787d63..b9503c9e2bed 100644 --- a/pkg/ccl/sqlproxyccl/proxy_handler_test.go +++ b/pkg/ccl/sqlproxyccl/proxy_handler_test.go @@ -30,6 +30,7 @@ import ( "github.com/cockroachdb/cockroach/pkg/server" "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/pgwire" + "github.com/cockroachdb/cockroach/pkg/sql/pgwire/pgerror" "github.com/cockroachdb/cockroach/pkg/testutils" "github.com/cockroachdb/cockroach/pkg/testutils/serverutils" "github.com/cockroachdb/cockroach/pkg/testutils/skip" @@ -142,21 +143,21 @@ func TestFailedConnection(t *testing.T) { // TenantID rejected as malformed. te.TestConnectErr( ctx, t, u+"?options=--cluster=dimdog&sslmode="+sslmode, - codeParamsRoutingFailed, "invalid cluster name 'dimdog'", + codeParamsRoutingFailed, "invalid cluster identifier 'dimdog'", ) require.Equal(t, int64(1+(i*3)), s.metrics.RoutingErrCount.Count()) // No cluster name and TenantID. te.TestConnectErr( ctx, t, u+"?sslmode="+sslmode, - codeParamsRoutingFailed, "missing cluster name in connection string", + codeParamsRoutingFailed, "missing cluster identifier in connection string", ) require.Equal(t, int64(2+(i*3)), s.metrics.RoutingErrCount.Count()) // Bad TenantID. Ensure that we don't leak any parsing errors. te.TestConnectErr( ctx, t, u+"?options=--cluster=dim-dog-foo3&sslmode="+sslmode, - codeParamsRoutingFailed, "invalid cluster name 'dim-dog-foo3'", + codeParamsRoutingFailed, "invalid cluster identifier 'dim-dog-foo3'", ) require.Equal(t, int64(3+(i*3)), s.metrics.RoutingErrCount.Count()) } @@ -742,51 +743,58 @@ func TestClusterNameAndTenantFromParams(t *testing.T) { expectedTenantID uint64 expectedParams map[string]string expectedError string + expectedHint string }{ { name: "empty params", params: map[string]string{}, - expectedError: "missing cluster name in connection string", + expectedError: "missing cluster identifier in connection string", + expectedHint: strings.TrimLeft(clusterIdentifierHint, "\n"), }, { - name: "cluster name is not provided", + name: "cluster identifier is not provided", params: map[string]string{ "database": "defaultdb", "options": "--foo=bar", }, - expectedError: "missing cluster name in connection string", + expectedError: "missing cluster identifier in connection string", + expectedHint: strings.TrimLeft(clusterIdentifierHint, "\n"), }, { - name: "multiple similar cluster names", + name: "multiple similar cluster identifiers", params: map[string]string{ "database": "happy-koala-7.defaultdb", "options": "--cluster=happy-koala", }, - expectedError: "multiple cluster names provided", + expectedError: "multiple cluster identifiers provided", + expectedHint: strings.TrimLeft(clusterIdentifierHint, "\n"), }, { - name: "multiple different cluster names", + name: "multiple different cluster identifiers", params: map[string]string{ "database": "happy-koala-7.defaultdb", "options": "--cluster=happy-tiger", }, - expectedError: "multiple cluster names provided", + expectedError: "multiple cluster identifiers provided", + expectedHint: strings.TrimLeft(clusterIdentifierHint, "\n"), }, { - name: "invalid cluster name in database param", + name: "invalid cluster identifier in database param", params: map[string]string{ // Cluster names need to be between 6 to 20 alphanumeric characters. "database": "short-0.defaultdb", }, - expectedError: "invalid cluster name 'short-0'", + expectedError: "invalid cluster identifier 'short-0'", + expectedHint: "is 'short' a valid cluster name?\n--\n" + clusterNameFormHint, }, { - name: "invalid cluster name in options param", + name: "invalid cluster identifier in options param", params: map[string]string{ // Cluster names need to be between 6 to 20 alphanumeric characters. "options": "--cluster=cockroachlabsdotcomfoobarbaz-0", }, - expectedError: "invalid cluster name 'cockroachlabsdotcomfoobarbaz-0'", + expectedError: "invalid cluster identifier 'cockroachlabsdotcomfoobarbaz-0'", + expectedHint: "is 'cockroachlabsdotcomfoobarbaz' a valid cluster name?\n--\n" + clusterNameFormHint, }, { name: "invalid database param (1)", @@ -827,30 +835,35 @@ func TestClusterNameAndTenantFromParams(t *testing.T) { { name: "no tenant id", params: map[string]string{"database": "happy2koala.defaultdb"}, - expectedError: "invalid cluster name 'happy2koala'", + expectedError: "invalid cluster identifier 'happy2koala'", + expectedHint: missingTenantIDHint + "\n--\n" + clusterNameFormHint, }, { name: "missing tenant id", params: map[string]string{"database": "happy2koala-.defaultdb"}, - expectedError: "invalid cluster name 'happy2koala-'", + expectedError: "invalid cluster identifier 'happy2koala-'", + expectedHint: missingTenantIDHint + "\n--\n" + clusterNameFormHint, }, { name: "missing cluster name", params: map[string]string{"database": "-7.defaultdb"}, - expectedError: "invalid cluster name '-7'", + expectedError: "invalid cluster identifier '-7'", + expectedHint: "is '' a valid cluster name?\n--\n" + clusterNameFormHint, }, { name: "bad tenant id", params: map[string]string{"database": "happy-koala-0-7a.defaultdb"}, - expectedError: "invalid cluster name 'happy-koala-0-7a'", + expectedError: "invalid cluster identifier 'happy-koala-0-7a'", + expectedHint: "is '7a' a valid tenant ID?\n--\n" + clusterNameFormHint, }, { name: "zero tenant id", params: map[string]string{"database": "happy-koala-0.defaultdb"}, - expectedError: "invalid cluster name 'happy-koala-0'", + expectedError: "invalid cluster identifier 'happy-koala-0'", + expectedHint: "tenant ID 0 is invalid", }, { - name: "cluster name in database param", + name: "cluster identifier in database param", params: map[string]string{ "database": "happy-koala-7.defaultdb", "foo": "bar", @@ -860,7 +873,7 @@ func TestClusterNameAndTenantFromParams(t *testing.T) { expectedParams: map[string]string{"database": "defaultdb", "foo": "bar"}, }, { - name: "valid cluster name with invalid arrangements", + name: "valid cluster identifier with invalid arrangements", params: map[string]string{ "database": "defaultdb", "options": "-c --cluster=happy-koala-7 -c -c -c", @@ -873,7 +886,7 @@ func TestClusterNameAndTenantFromParams(t *testing.T) { }, }, { - name: "short option: cluster name in options param", + name: "short option: cluster identifier in options param", params: map[string]string{ "database": "defaultdb", "options": "-ccluster=happy-koala-7", @@ -883,7 +896,7 @@ func TestClusterNameAndTenantFromParams(t *testing.T) { expectedParams: map[string]string{"database": "defaultdb"}, }, { - name: "short option with spaces: cluster name in options param", + name: "short option with spaces: cluster identifier in options param", params: map[string]string{ "database": "defaultdb", "options": "-c cluster=happy-koala-7", @@ -893,7 +906,7 @@ func TestClusterNameAndTenantFromParams(t *testing.T) { expectedParams: map[string]string{"database": "defaultdb"}, }, { - name: "long option: cluster name in options param", + name: "long option: cluster identifier in options param", params: map[string]string{ "database": "defaultdb", "options": "--cluster=happy-koala-7\t--foo=test", @@ -906,7 +919,7 @@ func TestClusterNameAndTenantFromParams(t *testing.T) { }, }, { - name: "long option: cluster name in options param with other options", + name: "long option: cluster identifier in options param with other options", params: map[string]string{ "database": "defaultdb", "options": "-csearch_path=public --cluster=happy-koala-7\t--foo=test", @@ -946,6 +959,9 @@ func TestClusterNameAndTenantFromParams(t *testing.T) { require.Equal(t, tc.expectedParams, outMsg.Parameters) } else { require.EqualErrorf(t, err, tc.expectedError, "failed test case\n%+v", tc) + + pgerr := pgerror.Flatten(err) + require.Equal(t, tc.expectedHint, pgerr.Hint) } // Check that the original parameters were not modified.