From 05bcaa1a8161df25a97732d5bb3765a9ce5b26eb Mon Sep 17 00:00:00 2001 From: Raphael 'kena' Poss Date: Thu, 23 Feb 2023 20:57:03 +0100 Subject: [PATCH] cli: warn the user for common mistakes around `options=-ccluster` Examples: ``` % ./cockroach sql --url='postgres://localhost:26257/?-ccluster=foo' warning: found raw URL parameter "-ccluster"; are you sure you did not mean to use "options=-ccluster" instead? % ./cockroach sql --url='postgres://localhost:26257/?options=-cluster=foo' warning: found "-cluster=" in URL "options" field; are you sure you did not mean to use "options=-ccluster" instead? ``` Release note: None --- pkg/cli/clientflags/client_flags.go | 8 +- pkg/cli/clienturl/client_url.go | 37 +++++++ pkg/cli/interactive_tests/test_flags.tcl | 13 +++ pkg/server/pgurl/BUILD.bazel | 1 + pkg/server/pgurl/extended_options.go | 116 +++++++++++++++++++++ pkg/server/pgurl/pgurl.go | 26 ++++- pkg/server/pgurl/pgurl_test.go | 22 ++++ pkg/sql/pgwire/BUILD.bazel | 1 + pkg/sql/pgwire/pre_serve_options.go | 122 +++-------------------- 9 files changed, 235 insertions(+), 111 deletions(-) create mode 100644 pkg/server/pgurl/extended_options.go diff --git a/pkg/cli/clientflags/client_flags.go b/pkg/cli/clientflags/client_flags.go index 8cc651226c43..d362d1420283 100644 --- a/pkg/cli/clientflags/client_flags.go +++ b/pkg/cli/clientflags/client_flags.go @@ -11,6 +11,9 @@ package clientflags import ( + "fmt" + "os" + "github.com/cockroachdb/cockroach/pkg/cli/clienturl" "github.com/cockroachdb/cockroach/pkg/cli/cliflagcfg" "github.com/cockroachdb/cockroach/pkg/cli/cliflags" @@ -71,7 +74,10 @@ func AddSQLFlags( _ = f.MarkHidden(cliflags.ClientHost.Name) // --url. - cliflagcfg.VarFlagDepth(1, f, clienturl.NewURLParser(cmd, clientOpts, false /* strictTLS */, nil /* warnFn */), cliflags.URL) + warnFn := func(format string, args ...interface{}) { + fmt.Fprintf(os.Stderr, format, args...) + } + cliflagcfg.VarFlagDepth(1, f, clienturl.NewURLParser(cmd, clientOpts, false /* strictTLS */, warnFn), cliflags.URL) // --user/-u cliflagcfg.StringFlagDepth(1, f, &clientOpts.User, cliflags.User) diff --git a/pkg/cli/clienturl/client_url.go b/pkg/cli/clienturl/client_url.go index 8877fcc7dada..47c6f65759ea 100644 --- a/pkg/cli/clienturl/client_url.go +++ b/pkg/cli/clienturl/client_url.go @@ -13,6 +13,7 @@ package clienturl import ( "fmt" "strconv" + "strings" "github.com/cockroachdb/cockroach/pkg/cli/cliflagcfg" "github.com/cockroachdb/cockroach/pkg/cli/cliflags" @@ -136,6 +137,8 @@ func (u *urlParser) Set(v string) error { return fmt.Errorf("%s", msg) } + u.checkMistakes(purl) + // Store the parsed URL for later. u.clientOpts.ExplicitURL = purl return err @@ -274,3 +277,37 @@ func MakeURLParserFn( return clientOpts.ExplicitURL, nil } } + +// checkMistakes reports likely mistakes to the user via warning messages. +func (u *urlParser) checkMistakes(purl *pgurl.URL) { + if u.warnFn == nil { + return + } + + // Check mistaken use of -c as direct option, instead of using + // options=-c... + opts := purl.GetExtraOptions() + for optName := range opts { + if strings.HasPrefix(optName, "-c") { + u.warnFn("\nwarning: found raw URL parameter \"%[1]s\"; "+ + "are you sure you did not mean to use \"options=%[1]s\" instead?\n\n", optName) + } + if optName == "cluster" { + u.warnFn("\nwarning: found raw URL parameter \"%[1]s\"; "+ + "are you sure you did not mean to use \"options=-c%[1]s\" instead?\n\n", optName) + } + } + // For tenant selection, the option is `-ccluster=`, not `-cluster=`. + if extraOpts := opts.Get("options"); extraOpts != "" { + opts, err := pgurl.ParseExtendedOptions(extraOpts) + if err != nil { + u.warnFn("\nwarning: invalid syntax in options: %v\n\n", err) + } else { + if opts.Has("luster") { + // User entered options=-cluster. + u.warnFn("\nwarning: found \"-cluster=\" in URL \"options\" field; " + + "are you sure you did not mean to use \"options=-ccluster=\" instead?\n\n") + } + } + } +} diff --git a/pkg/cli/interactive_tests/test_flags.tcl b/pkg/cli/interactive_tests/test_flags.tcl index 2d29b4b3f0cc..197c7ddf385c 100644 --- a/pkg/cli/interactive_tests/test_flags.tcl +++ b/pkg/cli/interactive_tests/test_flags.tcl @@ -134,8 +134,21 @@ eexpect "setting --url from COCKROACH_URL" eexpect "invalid argument" eexpect "unrecognized URL scheme" eexpect ":/# " +send "unset COCKROACH_URL\r" +eexpect ":/# " end_test +start_test "Check that common URL mistakes are detected and the user is informed" +send "$argv sql --no-line-editor --url='postgres://invalid:0/?-cinvalid'\r" +eexpect "warning: found raw URL parameter \"-cinvalid" +eexpect "are you sure" +eexpect ":/# " + +send "$argv sql --no-line-editor --url='postgres://invalid:0/?options=-cluster=foo'\r" +eexpect "warning: found \"-cluster=\" in URL \"options\" field" +eexpect "are you sure" +eexpect ":/# " +end_test stop_server $argv diff --git a/pkg/server/pgurl/BUILD.bazel b/pkg/server/pgurl/BUILD.bazel index 5b7eb0de2c9e..b6ce53968e46 100644 --- a/pkg/server/pgurl/BUILD.bazel +++ b/pkg/server/pgurl/BUILD.bazel @@ -4,6 +4,7 @@ load("@io_bazel_rules_go//go:def.bzl", "go_library", "go_test") go_library( name = "pgurl", srcs = [ + "extended_options.go", "generate.go", "parse.go", "pgurl.go", diff --git a/pkg/server/pgurl/extended_options.go b/pkg/server/pgurl/extended_options.go new file mode 100644 index 000000000000..cbee2e6c1014 --- /dev/null +++ b/pkg/server/pgurl/extended_options.go @@ -0,0 +1,116 @@ +// Copyright 2023 The Cockroach Authors. +// +// Use of this software is governed by the Business Source License +// included in the file licenses/BSL.txt. +// +// As of the Change Date specified in that file, in accordance with +// the Business Source License, use of this software will be governed +// by the Apache License, Version 2.0, included in the file +// licenses/APL.txt. + +package pgurl + +import ( + "net/url" + "strings" + "unicode" + + "github.com/cockroachdb/errors" +) + +// ParseExtendedOptions is a parser for the value part of the special +// option "options". +// +// The options must be separated by space and have one of the +// following patterns: '-c key=value', '-ckey=value', '--key=value' +func ParseExtendedOptions(optionsString string) (res url.Values, err error) { + res = url.Values{} + lastWasDashC := false + opts := splitOptions(optionsString) + + for i := 0; i < len(opts); i++ { + prefix := "" + if len(opts[i]) > 1 { + prefix = opts[i][:2] + } + + switch { + case opts[i] == "-c": + lastWasDashC = true + continue + case lastWasDashC: + lastWasDashC = false + // if the last option was '-c' parse current option with no regard to + // the prefix + prefix = "" + case prefix == "--" || prefix == "-c": + lastWasDashC = false + default: + return nil, errors.Newf( + "option %q is invalid, must have prefix '-c' or '--'", opts[i]) + } + + key, value, err := splitOption(opts[i], prefix) + if err != nil { + return nil, err + } + res.Set(key, value) + } + return res, nil +} + +// splitOptions slices the given string into substrings separated by space +// unless the space is escaped using backslashes '\\'. It also skips multiple +// subsequent spaces. +func splitOptions(options string) []string { + var res []string + var sb strings.Builder + i := 0 + for i < len(options) { + sb.Reset() + // skip leading spaces. + for i < len(options) && unicode.IsSpace(rune(options[i])) { + i++ + } + if i == len(options) { + break + } + + lastWasEscape := false + + for i < len(options) { + if unicode.IsSpace(rune(options[i])) && !lastWasEscape { + break + } + if !lastWasEscape && options[i] == '\\' { + lastWasEscape = true + } else { + lastWasEscape = false + sb.WriteByte(options[i]) + } + i++ + } + + res = append(res, sb.String()) + } + + return res +} + +// splitOption splits the given opt argument into substrings separated by '='. +// It returns an error if the given option does not comply with the pattern +// "key=value" and the number of elements in the result is not two. +// splitOption removes the prefix from the key and replaces '-' with '_' so +// "--option-name=value" becomes [option_name, value]. +func splitOption(opt, prefix string) (string, string, error) { + kv := strings.Split(opt, "=") + + if len(kv) != 2 { + return "", "", errors.Newf( + "option %q is invalid, check '='", opt) + } + + kv[0] = strings.TrimPrefix(kv[0], prefix) + + return strings.ReplaceAll(kv[0], "-", "_"), kv[1], nil +} diff --git a/pkg/server/pgurl/pgurl.go b/pkg/server/pgurl/pgurl.go index fef7d51b6eb5..1010411e0065 100644 --- a/pkg/server/pgurl/pgurl.go +++ b/pkg/server/pgurl/pgurl.go @@ -103,6 +103,20 @@ func (u *URL) GetDatabase() string { // AddOptions adds key=value options to the URL. // Certain combinations are checked and an error is returned // if a combination is found invalid. +// +// Note that AddOptions supports the "main" client driver options like +// "database", "host", "application_name" etc. Separately, certain +// client drivers also support an extended "options" field with +// additional key/value pairs, e.g. datestyle. To set those, use +// either: +// +// AddOptions(url.Values{"options":[]string{"--key=value"}}) +// +// or +// +// SetOption("options", "--key=value"). +// +// See also ParseExtendedOptions() in the top level of this package. func (u *URL) AddOptions(opts url.Values) error { return u.parseOptions(opts) } @@ -112,6 +126,12 @@ func (u *URL) AddOptions(opts url.Values) error { // given option. // Certain combinations are checked and an error is returned // if a combination is found invalid. +// +// Note: this method only sets the "main" client-side parameters, such +// as "database", "application_name" etc. To set the extended options +// field, use SetOption("options", "--key=value") instead. +// +// See also ParseExtendedOptions() in the top level of this package. func (u *URL) SetOption(key, value string) error { vals := []string{value} opts := url.Values{key: vals} @@ -129,7 +149,11 @@ func (u *URL) GetOption(opt string) string { return getVal(u.extraOptions, opt) } -// GetExtraOptions retrieves all of the extra options. +// GetExtraOptions retrieves all of the extra options. These are all +// top-level k=v client-side parameters. If the "options" extended +// parameters are present, they will be returned as a _single_ pair +// with the key equal to "options". +// See also ParseExtendedOptions() at the top of this package. func (u *URL) GetExtraOptions() url.Values { return u.extraOptions } diff --git a/pkg/server/pgurl/pgurl_test.go b/pkg/server/pgurl/pgurl_test.go index a74ea8225017..c5345c4e9989 100644 --- a/pkg/server/pgurl/pgurl_test.go +++ b/pkg/server/pgurl/pgurl_test.go @@ -144,3 +144,25 @@ var _ = ProtoUndefined var _ = TLSVerifyCA var _ = TLSPrefer var _ = TLSAllow + +func TestParseExtendedOptions(t *testing.T) { + u, err := Parse("postgres://localhost?options= " + + "--user=test " + + "-c search_path=public,testsp %20%09 " + + "--default-transaction-isolation=read\\ uncommitted " + + "-capplication_name=test " + + "--DateStyle=ymd\\ ,\\ iso\\ " + + "-c intervalstyle%3DISO_8601 " + + "-ccustom_option.custom_option=test2") + require.NoError(t, err) + opts := u.GetOption("options") + kvs, err := ParseExtendedOptions(opts) + require.NoError(t, err) + require.Equal(t, kvs.Get("user"), "test") + require.Equal(t, kvs.Get("search_path"), "public,testsp") + require.Equal(t, kvs.Get("default_transaction_isolation"), "read uncommitted") + require.Equal(t, kvs.Get("application_name"), "test") + require.Equal(t, kvs.Get("DateStyle"), "ymd , iso ") + require.Equal(t, kvs.Get("intervalstyle"), "ISO_8601") + require.Equal(t, kvs.Get("custom_option.custom_option"), "test2") +} diff --git a/pkg/sql/pgwire/BUILD.bazel b/pkg/sql/pgwire/BUILD.bazel index 4efe5db5513a..082d329d6c4d 100644 --- a/pkg/sql/pgwire/BUILD.bazel +++ b/pkg/sql/pgwire/BUILD.bazel @@ -31,6 +31,7 @@ go_library( "//pkg/security/password", "//pkg/security/sessionrevival", "//pkg/security/username", + "//pkg/server/pgurl", "//pkg/server/serverpb", "//pkg/server/telemetry", "//pkg/settings", diff --git a/pkg/sql/pgwire/pre_serve_options.go b/pkg/sql/pgwire/pre_serve_options.go index de6490cd810e..5a5b242b604a 100644 --- a/pkg/sql/pgwire/pre_serve_options.go +++ b/pkg/sql/pgwire/pre_serve_options.go @@ -17,9 +17,9 @@ import ( "regexp" "strconv" "strings" - "unicode" "github.com/cockroachdb/cockroach/pkg/security/username" + "github.com/cockroachdb/cockroach/pkg/server/pgurl" "github.com/cockroachdb/cockroach/pkg/server/telemetry" "github.com/cockroachdb/cockroach/pkg/sql" "github.com/cockroachdb/cockroach/pkg/sql/catalog/catalogkeys" @@ -143,16 +143,20 @@ func parseClientProvidedSessionParameters( args.RemoteAddr = &net.TCPAddr{IP: ip, Port: port} case "options": - opts, err := parseOptions(value) + opts, err := pgurl.ParseExtendedOptions(value) if err != nil { - return args, err + return args, pgerror.WithCandidateCode(err, pgcode.ProtocolViolation) } - for _, opt := range opts { + for opt, values := range opts { + var optvalue string + if len(values) > 0 { + optvalue = values[0] + } // crdb:jwt_auth_enabled must be passed as an option in order for us to support non-CRDB // clients. jwt_auth_enabled is not a session variable. We extract it separately here. - switch strings.ToLower(opt.key) { + switch strings.ToLower(opt) { case "crdb:jwt_auth_enabled": - b, err := strconv.ParseBool(opt.value) + b, err := strconv.ParseBool(optvalue) if err != nil { return args, pgerror.Wrapf(err, pgcode.InvalidParameterValue, "crdb:jwt_auth_enabled") } @@ -164,7 +168,7 @@ func parseClientProvidedSessionParameters( return args, pgerror.Newf(pgcode.InvalidParameterValue, "cannot specify system identity via options") } - args.SystemIdentity, _ = username.MakeSQLUsernameFromUserInput(opt.value, username.PurposeValidation) + args.SystemIdentity, _ = username.MakeSQLUsernameFromUserInput(optvalue, username.PurposeValidation) continue case "cluster": @@ -174,12 +178,12 @@ func parseClientProvidedSessionParameters( } // The syntax after '.' will be extended in later versions. // The period itself cannot occur in a tenant name. - parts := strings.SplitN(opt.value, ".", 2) + parts := strings.SplitN(optvalue, ".", 2) args.tenantName = parts[0] hasTenantSelectOption = true continue } - err = loadParameter(ctx, opt.key, opt.value, &args.SessionArgs) + err = loadParameter(ctx, opt, optvalue, &args.SessionArgs) if err != nil { return args, pgerror.Wrapf(err, pgerror.GetPGCode(err), "options") } @@ -257,106 +261,6 @@ func loadParameter(ctx context.Context, key, value string, args *sql.SessionArgs return nil } -// option represents an option argument passed in the connection URL. -type option struct { - key string - value string -} - -// parseOptions parses the given string into the options. The options must be -// separated by space and have one of the following patterns: -// '-c key=value', '-ckey=value', '--key=value' -func parseOptions(optionsString string) (res []option, err error) { - lastWasDashC := false - opts := splitOptions(optionsString) - - for i := 0; i < len(opts); i++ { - prefix := "" - if len(opts[i]) > 1 { - prefix = opts[i][:2] - } - - switch { - case opts[i] == "-c": - lastWasDashC = true - continue - case lastWasDashC: - lastWasDashC = false - // if the last option was '-c' parse current option with no regard to - // the prefix - prefix = "" - case prefix == "--" || prefix == "-c": - lastWasDashC = false - default: - return nil, pgerror.Newf(pgcode.ProtocolViolation, - "option %q is invalid, must have prefix '-c' or '--'", opts[i]) - } - - opt, err := splitOption(opts[i], prefix) - if err != nil { - return nil, err - } - res = append(res, opt) - } - return res, nil -} - -// splitOptions slices the given string into substrings separated by space -// unless the space is escaped using backslashes '\\'. It also skips multiple -// subsequent spaces. -func splitOptions(options string) []string { - var res []string - var sb strings.Builder - i := 0 - for i < len(options) { - sb.Reset() - // skip leading space - for i < len(options) && unicode.IsSpace(rune(options[i])) { - i++ - } - if i == len(options) { - break - } - - lastWasEscape := false - - for i < len(options) { - if unicode.IsSpace(rune(options[i])) && !lastWasEscape { - break - } - if !lastWasEscape && options[i] == '\\' { - lastWasEscape = true - } else { - lastWasEscape = false - sb.WriteByte(options[i]) - } - i++ - } - - res = append(res, sb.String()) - } - - return res -} - -// splitOption splits the given opt argument into substrings separated by '='. -// It returns an error if the given option does not comply with the pattern -// "key=value" and the number of elements in the result is not two. -// splitOption removes the prefix from the key and replaces '-' with '_' so -// "--option-name=value" becomes [option_name, value]. -func splitOption(opt, prefix string) (option, error) { - kv := strings.Split(opt, "=") - - if len(kv) != 2 { - return option{}, pgerror.Newf(pgcode.ProtocolViolation, - "option %q is invalid, check '='", opt) - } - - kv[0] = strings.TrimPrefix(kv[0], prefix) - - return option{key: strings.ReplaceAll(kv[0], "-", "_"), value: kv[1]}, nil -} - // Note: Usage of an env var here makes it possible to unconditionally // enable this feature when cluster settings do not work reliably, // e.g. in multi-tenant setups in v20.2. This override mechanism can