Merge #82020

82020: cli: refactors; make `cockroach-sql`'s CLI parameter handling match `cockroach sql` r=otan,catj-cockroach a=knz

Note to the reviewers:
- The PR has been split into many small commits to ease the review.
- The "main commit" is the last in the sequence; all commits before the last constitute a long-awaited refactor of the connection parameter handling, including moving much of the security-related logic away from `cli` into a sub-package of `security` where it belongs.
- We only have testing code to exercise the command-line handling for `cockroach sql`, which at this point should be considered sufficient given we're using the same config code in `cockroach-sql` now. To make tests work across the two commands, we would need to make progress on #80921 first.

Fixes #81882.
Fixes #82024.
Unblocks progress on #29285.


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

pkg/ccl/cliccl/BUILD.bazel

@@ -24,6 +24,7 @@ go_library(
         "//pkg/ccl/workloadccl/cliccl",
         "//pkg/cli",
         "//pkg/cli/clierrorplus",
+        "//pkg/cli/cliflagcfg",
         "//pkg/cli/cliflags",
         "//pkg/cli/clisqlexec",
         "//pkg/cli/democluster",

pkg/ccl/cliccl/debug.go

@@ -24,6 +24,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/ccl/storageccl/engineccl/enginepbccl"
 	"github.com/cockroachdb/cockroach/pkg/cli"
 	"github.com/cockroachdb/cockroach/pkg/cli/clierrorplus"
+	"github.com/cockroachdb/cockroach/pkg/cli/cliflagcfg"
 	"github.com/cockroachdb/cockroach/pkg/storage"
 	"github.com/cockroachdb/cockroach/pkg/storage/enginepb"
 	"github.com/cockroachdb/cockroach/pkg/util/protoutil"
@@ -86,21 +87,21 @@ AES128_CTR:be235...   # AES-128 encryption with store key ID
 
 	// Add the encryption flag to commands that need it.
 	f := encryptionStatusCmd.Flags()
-	cli.VarFlag(f, &storeEncryptionSpecs, cliflagsccl.EnterpriseEncryption)
+	cliflagcfg.VarFlag(f, &storeEncryptionSpecs, cliflagsccl.EnterpriseEncryption)
 	// And other flags.
 	f.BoolVar(&encryptionStatusOpts.activeStoreIDOnly, "active-store-key-id-only", false,
 		"print active store key ID and exit")
 
 	// Add encryption flag to all OSS debug commands that want it.
 	for _, cmd := range cli.DebugCommandsRequiringEncryption {
 		// storeEncryptionSpecs is in start.go.
-		cli.VarFlag(cmd.Flags(), &storeEncryptionSpecs, cliflagsccl.EnterpriseEncryption)
+		cliflagcfg.VarFlag(cmd.Flags(), &storeEncryptionSpecs, cliflagsccl.EnterpriseEncryption)
 	}
 
 	// init has already run in cli/debug.go since this package imports it, so
 	// DebugPebbleCmd already has all its subcommands. We could traverse those
 	// here. But we don't need to by using PersistentFlags.
-	cli.VarFlag(cli.DebugPebbleCmd.PersistentFlags(),
+	cliflagcfg.VarFlag(cli.DebugPebbleCmd.PersistentFlags(),
 		&storeEncryptionSpecs, cliflagsccl.EnterpriseEncryption)
 
 	cli.PopulateStorageConfigHook = fillEncryptionOptionsForStore

pkg/ccl/cliccl/start.go

@@ -12,6 +12,7 @@ import (
 	"github.com/cockroachdb/cockroach/pkg/ccl/baseccl"
 	"github.com/cockroachdb/cockroach/pkg/ccl/cliccl/cliflagsccl"
 	"github.com/cockroachdb/cockroach/pkg/cli"
+	"github.com/cockroachdb/cockroach/pkg/cli/cliflagcfg"
 	"github.com/spf13/cobra"
 )
 
@@ -22,7 +23,7 @@ var storeEncryptionSpecs baseccl.StoreEncryptionSpecList
 
 func init() {
 	for _, cmd := range cli.StartCmds {
-		cli.VarFlag(cmd.Flags(), &storeEncryptionSpecs, cliflagsccl.EnterpriseEncryption)
+		cliflagcfg.VarFlag(cmd.Flags(), &storeEncryptionSpecs, cliflagsccl.EnterpriseEncryption)
 
 		// Add a new pre-run command to match encryption specs to store specs.
 		cli.AddPersistentPreRunE(cmd, func(cmd *cobra.Command, _ []string) error {

pkg/ccl/workloadccl/fixture_test.go

@@ -165,6 +165,9 @@ func TestFixture(t *testing.T) {
 
 func TestImportFixture(t *testing.T) {
 	defer leaktest.AfterTest(t)()
+
+	skip.WithIssue(t, 81082, "flaky")
+
 	ctx := context.Background()
 
 	defer func(oldRefreshInterval, oldAsOf time.Duration) {

pkg/cli/BUILD.bazel

@@ -100,8 +100,11 @@ go_library(
         "//pkg/ccl/sqlproxyccl",
         "//pkg/ccl/sqlproxyccl/tenantdirsvr",
         "//pkg/cli/clicfg",
+        "//pkg/cli/clientflags",
+        "//pkg/cli/clienturl",
         "//pkg/cli/clierror",
         "//pkg/cli/clierrorplus",
+        "//pkg/cli/cliflagcfg",
         "//pkg/cli/cliflags",
         "//pkg/cli/clisqlcfg",
         "//pkg/cli/clisqlclient",
@@ -132,6 +135,9 @@ go_library(
         "//pkg/roachpb",
         "//pkg/rpc",
         "//pkg/security",
+        "//pkg/security/certnames",
+        "//pkg/security/clientsecopts",
+        "//pkg/security/securityassets",
         "//pkg/security/securitytest",
         "//pkg/security/username",
         "//pkg/server",

pkg/cli/cli.go

@@ -23,6 +23,7 @@ import (
 
 	"github.com/cockroachdb/cockroach/pkg/build"
 	"github.com/cockroachdb/cockroach/pkg/cli/clierror"
+	"github.com/cockroachdb/cockroach/pkg/cli/cliflagcfg"
 	"github.com/cockroachdb/cockroach/pkg/cli/exit"
 	_ "github.com/cockroachdb/cockroach/pkg/cloud/impl" // register cloud storage providers
 	"github.com/cockroachdb/cockroach/pkg/util/log"
@@ -96,7 +97,7 @@ func doMain(cmd *cobra.Command, cmdName string) error {
 		// This must occur before the parameters are parsed by cobra, so
 		// that the command-line flags can override the defaults in
 		// environment variables.
-		if err := processEnvVarDefaults(cmd); err != nil {
+		if err := cliflagcfg.ProcessEnvVarDefaults(cmd); err != nil {
 			return err
 		}