Skip to content

Commit

Permalink
ccl/sqlproxyccl: provide hints to the user whenever params parsing fails
Browse files Browse the repository at this point in the history 
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
  • Loading branch information
@jaylim-crl
jaylim-crl committed Nov 1, 2021
1 parent 039ed5f commit 7f7dd17
Show file tree
Hide file tree
Showing 4 changed files with 86 additions and 32 deletions.
1 change: 1 addition & 0 deletions pkg/ccl/sqlproxyccl/BUILD.bazel
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
2 changes: 2 additions & 0 deletions pkg/ccl/sqlproxyccl/proxy.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
49 changes: 42 additions & 7 deletions pkg/ccl/sqlproxyccl/proxy_handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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 == "" {
Expand All @@ -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.
Expand Down Expand Up @@ -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 "<cluster identifier>.<database name>" as your database parameter.
(e.g. "active-roach-42.defaultdb")
2) options parameter:
Use "--cluster=<cluster identifier>" 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 <name>-<tenant ID> (e.g. lazy-roach-3)"

const missingTenantIDHint = "did you forget to include your tenant ID in the cluster identifier?"
66 changes: 41 additions & 25 deletions pkg/ccl/sqlproxyccl/proxy_handler_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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())
}
Expand Down Expand Up @@ -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)",
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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.
Expand Down

0 comments on commit 7f7dd17

Please sign in to comment.