Merge cockroachdb#97588

97588: cli: warn the user for common mistakes around `options=-ccluster` r=rafiss a=knz

Fixes cockroachdb#97586.
Epic: CRDB-23559

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

Co-authored-by: Raphael 'kena' Poss <[email protected]>

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)

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")
+			}
+		}
+	}
+}

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
 

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",

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
+}

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
 }

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")
+}

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",