Merge #29467

29467: cli: make all client commands recognize `--url` r=knz a=knz

Fixes #4984.
Fixes #29567.

Prior to this patch, the client commands using a SQL connection under
the hood would accept `--url` in addition to discrete
parameters (`--host`, `--port`, etc); however, non-SQL client commands
did not recognize `--url`.

This was causing UX pain/fatigue because users had to remember which
commands accept a URL and which commands do not. It also made
automation more difficult.

(For context, it was not reasonable to propose to users to _never_ use
`--url` and always stick to discrete flags, because some parameters
can only be specified via `--url`, for example the application name.)

This patch resolves the UX problem by ensuring that all client
commands accept `--url`, in addition to the discrete flags. This
effectively makes all CockroachDB commands (*) homogeneous/consistent
wrt their connection flags.

The value provided via `--url` and via the other discrete flags mesh
as follows:

```
//            flags parser
//                /    \
//         .-----'      `-------.
//         |                    |
//       --url               --host, --port, etc
//         |                    |
//   urlParser.Set()            |
//         |                    |
//         `-------.    .-------'
//                  \  /
//          sqlCtx/cliCtx/baseCtx
//                   |
//                  / \
//         .-------'   `--------.
//         |                    |
//         |                    |
//      non-SQL           makeClientConnURL()
//     commands                 |
//    (quit, init, etc)         |
//                          SQL commands
//                        (user, zone, etc)
```

(*) except for `cockroach workload`, which still has the status an
adopted appendage with its separate argument handling.

Release note (cli change): All `cockroach` client sub-commands (except
for `cockroach workload`) now support the `--url` flag.

Co-authored-by: Raphael 'kena' Poss <knz@cockroachlabs.com>

pkg/base/config.go

@@ -215,21 +215,40 @@ func (cfg *Config) GetCACertPath() (string, error) {
 // already contained SSL config options.
 func (cfg *Config) LoadSecurityOptions(options url.Values, username string) error {
 	if cfg.Insecure {
-		options.Add("sslmode", "disable")
+		options.Set("sslmode", "disable")
+		options.Del("sslrootcert")
+		options.Del("sslcert")
+		options.Del("sslkey")
 	} else {
-		// Fetch CA cert. This is required.
-		caCertPath, err := cfg.GetCACertPath()
-		if err != nil {
-			return wrapError(err)
+		sslMode := options.Get("sslmode")
+		if sslMode == "" || sslMode == "disable" {
+			options.Set("sslmode", "verify-full")
+		}
+
+		if sslMode != "require" {
+			// verify-ca and verify-full need a CA certificate.
+			if options.Get("sslrootcert") == "" {
+				// Fetch CA cert. This is required.
+				caCertPath, err := cfg.GetCACertPath()
+				if err != nil {
+					return wrapError(err)
+				}
+				options.Set("sslrootcert", caCertPath)
+			}
+		} else {
+			// require does not check the CA.
+			options.Del("sslrootcert")
 		}
-		options.Add("sslmode", "verify-full")
-		options.Add("sslrootcert", caCertPath)
 
 		// Fetch certs, but don't fail, we may be using a password.
 		certPath, keyPath, err := cfg.GetClientCertPaths(username)
 		if err == nil {
-			options.Add("sslcert", certPath)
-			options.Add("sslkey", keyPath)
+			if options.Get("sslcert") == "" {
+				options.Set("sslcert", certPath)
+			}
+			if options.Get("sslkey") == "" {
+				options.Set("sslkey", keyPath)
+			}
 		}
 	}
 	return nil

pkg/cli/cli_test.go

@@ -83,6 +83,32 @@ func (c *cliTest) fail(err interface{}) {
 	}
 }
 
+func createTestCerts(certsDir string) (cleanup func() error) {
+	// Copy these assets to disk from embedded strings, so this test can
+	// run from a standalone binary.
+	// Disable embedded certs, or the security library will try to load
+	// our real files as embedded assets.
+	security.ResetAssetLoader()
+
+	assets := []string{
+		filepath.Join(security.EmbeddedCertsDir, security.EmbeddedCACert),
+		filepath.Join(security.EmbeddedCertsDir, security.EmbeddedCAKey),
+		filepath.Join(security.EmbeddedCertsDir, security.EmbeddedNodeCert),
+		filepath.Join(security.EmbeddedCertsDir, security.EmbeddedNodeKey),
+		filepath.Join(security.EmbeddedCertsDir, security.EmbeddedRootCert),
+		filepath.Join(security.EmbeddedCertsDir, security.EmbeddedRootKey),
+	}
+
+	for _, a := range assets {
+		securitytest.RestrictedCopy(nil, a, certsDir, filepath.Base(a))
+	}
+
+	return func() error {
+		security.SetAssetLoader(securitytest.EmbeddedAssets)
+		return os.RemoveAll(certsDir)
+	}
+}
+
 func newCLITest(params cliTestParams) cliTest {
 	c := cliTest{t: params.t}
 
@@ -100,30 +126,8 @@ func newCLITest(params cliTestParams) cliTest {
 
 	if !params.noServer {
 		if !params.insecure {
-			// Copy these assets to disk from embedded strings, so this test can
-			// run from a standalone binary.
-			// Disable embedded certs, or the security library will try to load
-			// our real files as embedded assets.
-			security.ResetAssetLoader()
-
-			assets := []string{
-				filepath.Join(security.EmbeddedCertsDir, security.EmbeddedCACert),
-				filepath.Join(security.EmbeddedCertsDir, security.EmbeddedCAKey),
-				filepath.Join(security.EmbeddedCertsDir, security.EmbeddedNodeCert),
-				filepath.Join(security.EmbeddedCertsDir, security.EmbeddedNodeKey),
-				filepath.Join(security.EmbeddedCertsDir, security.EmbeddedRootCert),
-				filepath.Join(security.EmbeddedCertsDir, security.EmbeddedRootKey),
-			}
-
-			for _, a := range assets {
-				securitytest.RestrictedCopy(nil, a, certsDir, filepath.Base(a))
-			}
+			c.cleanupFunc = createTestCerts(certsDir)
 			baseCfg.SSLCertsDir = certsDir
-
-			c.cleanupFunc = func() error {
-				security.SetAssetLoader(securitytest.EmbeddedAssets)
-				return os.RemoveAll(c.certsDir)
-			}
 		}
 
 		s, err := serverutils.StartServerRaw(base.TestServerArgs{