Merge cockroachdb#40824

40824: cli: make COCKROACH_URL work as intended r=bdarnell a=knz

Fixes cockroachdb#40747.

Release justification: see individual commits

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

pkg/cli/client_url.go

@@ -97,12 +97,13 @@ func (u urlParser) Set(v string) error {
 	}
 
 	cliCtx := u.cliCtx
+	fl := flagSetForCmd(u.cmd)
 
 	// If user name / password information is available, forward it to
 	// --user. We store the password for later re-collection by
 	// makeClientConnURL().
 	if parsedURL.User != nil {
-		f := u.cmd.Flags().Lookup(cliflags.User.Name)
+		f := fl.Lookup(cliflags.User.Name)
 		if f == nil {
 			// A client which does not support --user will also not use
 			// makeClientConnURL(), so we can ignore/forget about the
@@ -141,7 +142,7 @@ func (u urlParser) Set(v string) error {
 	// If a database path is available, forward it to --database.
 	if parsedURL.Path != "" {
 		dbPath := strings.TrimLeft(parsedURL.Path, "/")
-		f := u.cmd.Flags().Lookup(cliflags.Database.Name)
+		f := fl.Lookup(cliflags.Database.Name)
 		if f == nil {
 			// A client which does not support --database does not need this
 			// bit of information, so we can ignore/forget about it. We do
@@ -169,8 +170,6 @@ func (u urlParser) Set(v string) error {
 
 		cliCtx.extraConnURLOptions = options
 
-		fl := u.cmd.Flags()
-
 		switch sslMode := options.Get("sslmode"); sslMode {
 		case "disable":
 			if err := fl.Set(cliflags.ClientInsecure.Name, "true"); err != nil {

pkg/cli/demo.go

@@ -140,8 +140,9 @@ func setupTransientServers(
 
 	// Set up logging. For demo/transient server we use non-standard
 	// behavior where we avoid file creation if possible.
-	df := cmd.Flags().Lookup(cliflags.LogDir.Name)
-	sf := cmd.Flags().Lookup(logflags.LogToStderrName)
+	fl := flagSetForCmd(cmd)
+	df := fl.Lookup(cliflags.LogDir.Name)
+	sf := fl.Lookup(logflags.LogToStderrName)
 	if !df.Changed && !sf.Changed {
 		// User did not request logging flags; shut down all logging.
 		// Otherwise, the demo command would cause a cockroach-data
@@ -374,7 +375,7 @@ func runDemo(cmd *cobra.Command, gen workload.Generator) error {
 	}
 
 	// If the geo-partitioned replicas flag was given and the nodes have changed, throw an error.
-	if demoCtx.geoPartitionedReplicas && cmd.Flags().Lookup(cliflags.DemoNodes.Name).Changed {
+	if demoCtx.geoPartitionedReplicas && flagSetForCmd(cmd).Lookup(cliflags.DemoNodes.Name).Changed {
 		return errors.New("--nodes cannot be used with --geo-partitioned-replicas")
 	}
 

pkg/cli/flags.go

@@ -90,35 +90,25 @@ func AddPersistentPreRunE(cmd *cobra.Command, fn func(*cobra.Command, []string)
 	}
 }
 
-func setFlagFromEnv(f *pflag.FlagSet, flagInfo cliflags.FlagInfo) {
-	if flagInfo.EnvVar != "" {
-		if value, set := envutil.EnvString(flagInfo.EnvVar, 2); set {
-			if err := f.Set(flagInfo.Name, value); err != nil {
-				panic(err)
-			}
-		}
-	}
-}
-
 // StringFlag creates a string flag and registers it with the FlagSet.
 func StringFlag(f *pflag.FlagSet, valPtr *string, flagInfo cliflags.FlagInfo, defaultVal string) {
 	f.StringVarP(valPtr, flagInfo.Name, flagInfo.Shorthand, defaultVal, flagInfo.Usage())
 
-	setFlagFromEnv(f, flagInfo)
+	registerEnvVarDefault(f, flagInfo)
 }
 
 // IntFlag creates an int flag and registers it with the FlagSet.
 func IntFlag(f *pflag.FlagSet, valPtr *int, flagInfo cliflags.FlagInfo, defaultVal int) {
 	f.IntVarP(valPtr, flagInfo.Name, flagInfo.Shorthand, defaultVal, flagInfo.Usage())
 
-	setFlagFromEnv(f, flagInfo)
+	registerEnvVarDefault(f, flagInfo)
 }
 
 // BoolFlag creates a bool flag and registers it with the FlagSet.
 func BoolFlag(f *pflag.FlagSet, valPtr *bool, flagInfo cliflags.FlagInfo, defaultVal bool) {
 	f.BoolVarP(valPtr, flagInfo.Name, flagInfo.Shorthand, defaultVal, flagInfo.Usage())
 
-	setFlagFromEnv(f, flagInfo)
+	registerEnvVarDefault(f, flagInfo)
 }
 
 // DurationFlag creates a duration flag and registers it with the FlagSet.
@@ -127,22 +117,22 @@ func DurationFlag(
 ) {
 	f.DurationVarP(valPtr, flagInfo.Name, flagInfo.Shorthand, defaultVal, flagInfo.Usage())
 
-	setFlagFromEnv(f, flagInfo)
+	registerEnvVarDefault(f, flagInfo)
 }
 
 // VarFlag creates a custom-variable flag and registers it with the FlagSet.
 func VarFlag(f *pflag.FlagSet, value pflag.Value, flagInfo cliflags.FlagInfo) {
 	f.VarP(value, flagInfo.Name, flagInfo.Shorthand, flagInfo.Usage())
 
-	setFlagFromEnv(f, flagInfo)
+	registerEnvVarDefault(f, flagInfo)
 }
 
 // StringSlice creates a string slice flag and registers it with the FlagSet.
 func StringSlice(
 	f *pflag.FlagSet, valPtr *[]string, flagInfo cliflags.FlagInfo, defaultVal []string,
 ) {
 	f.StringSliceVar(valPtr, flagInfo.Name, defaultVal, flagInfo.Usage())
-	setFlagFromEnv(f, flagInfo)
+	registerEnvVarDefault(f, flagInfo)
 }
 
 // aliasStrVar wraps a string configuration option and is meant
@@ -229,8 +219,21 @@ const maxClusterNameLength = 256
 
 const backgroundEnvVar = "COCKROACH_BACKGROUND_RESTART"
 
+// flagSetForCmd is a replacement for cmd.Flag() that properly merges
+// persistent and local flags, until the upstream bug
+// https://github.com/spf13/cobra/issues/961 has been fixed.
+func flagSetForCmd(cmd *cobra.Command) *pflag.FlagSet {
+	_ = cmd.LocalFlags() // force merge persistent+local flags
+	return cmd.Flags()
+}
+
 func init() {
 	initCLIDefaults()
+	defer func() {
+		if err := processEnvVarDefaults(); err != nil {
+			panic(err)
+		}
+	}()
 
 	// Every command but start will inherit the following setting.
 	AddPersistentPreRunE(cockroachCmd, func(cmd *cobra.Command, _ []string) error {
@@ -555,7 +558,7 @@ func init() {
 	// Make the other non-SQL client commands also recognize --url in
 	// strict SSL mode.
 	for _, cmd := range clientCmds {
-		if f := cmd.Flags().Lookup(cliflags.URL.Name); f != nil {
+		if f := flagSetForCmd(cmd).Lookup(cliflags.URL.Name); f != nil {
 			// --url already registered above, nothing to do.
 			continue
 		}
@@ -624,6 +627,51 @@ func init() {
 	}
 }
 
+// processEnvVarDefaults injects the current value of flag-related
+// environment variables into the initial value of the settings linked
+// to the flags, during initialization and before the command line is
+// actually parsed. For example, it will inject the value of
+// $COCKROACH_URL into the urlParser object linked to the --url flag.
+func processEnvVarDefaults() error {
+	for envVar, d := range envVarDefaults {
+		if err := d.flagSet.Set(d.flagName, d.envValue); err != nil {
+			return errors.Wrapf(err, "setting --%s from %s", d.flagName, envVar)
+		}
+	}
+	return nil
+}
+
+// envVarDefault describes a delayed default initialization of the
+// setting covered by a flag from the value of an environment
+// variable.
+type envVarDefault struct {
+	envValue string
+	flagName string
+	flagSet  *pflag.FlagSet
+}
+
+// envVarDefaults records the initializations from environment variables
+// for processing at the end of initialization, before flag parsing.
+var envVarDefaults = map[string]envVarDefault{}
+
+// registerEnvVarDefault registers a deferred initialization of a flag
+// from an environment variable.
+func registerEnvVarDefault(f *pflag.FlagSet, flagInfo cliflags.FlagInfo) {
+	if flagInfo.EnvVar == "" {
+		return
+	}
+	value, set := envutil.EnvString(flagInfo.EnvVar, 2)
+	if !set {
+		// Env var not set. Nothing to do.
+		return
+	}
+	envVarDefaults[flagInfo.EnvVar] = envVarDefault{
+		envValue: value,
+		flagName: flagInfo.Name,
+		flagSet:  f,
+	}
+}
+
 func extraServerFlagInit(cmd *cobra.Command) error {
 	// Construct the main RPC listen address.
 	serverCfg.Addr = net.JoinHostPort(startCtx.serverListenAddr, serverListenPort)
@@ -645,11 +693,11 @@ func extraServerFlagInit(cmd *cobra.Command) error {
 		serverSQLPort = serverListenPort
 	}
 	serverCfg.SQLAddr = net.JoinHostPort(serverSQLAddr, serverSQLPort)
-	serverCfg.SplitListenSQL = cmd.Flags().Lookup(cliflags.ListenSQLAddr.Name).Changed
+	serverCfg.SplitListenSQL = flagSetForCmd(cmd).Lookup(cliflags.ListenSQLAddr.Name).Changed
 
 	// Fill in the defaults for --advertise-sql-addr.
-	advSpecified := cmd.Flags().Lookup(cliflags.AdvertiseAddr.Name).Changed ||
-		cmd.Flags().Lookup(cliflags.AdvertiseHost.Name).Changed
+	advSpecified := flagSetForCmd(cmd).Lookup(cliflags.AdvertiseAddr.Name).Changed ||
+		flagSetForCmd(cmd).Lookup(cliflags.AdvertiseHost.Name).Changed
 	if serverSQLAdvertiseAddr == "" {
 		if advSpecified {
 			serverSQLAdvertiseAddr = serverAdvertiseAddr
@@ -704,9 +752,7 @@ func extraClientFlagInit() {
 }
 
 func setDefaultStderrVerbosity(cmd *cobra.Command, defaultSeverity log.Severity) error {
-	pf := cmd.Flags()
-
-	vf := pf.Lookup(logflags.LogToStderrName)
+	vf := flagSetForCmd(cmd).Lookup(logflags.LogToStderrName)
 
 	// if `--logtostderr` was not specified and no log directory was
 	// set, or `--logtostderr` was specified but without explicit level,

pkg/cli/interactive_tests/common.tcl

@@ -89,7 +89,7 @@ proc send_eof {} {
 # in `server_pid`.
 proc start_server {argv} {
     report "BEGIN START SERVER"
-    system "$argv start-single-node --insecure --pid-file=server_pid --background -s=path=logs/db >>logs/expect-cmd.log 2>&1;
+    system "$argv start-single-node --insecure --pid-file=server_pid --listening-url-file=server_url --background -s=path=logs/db >>logs/expect-cmd.log 2>&1;
             $argv sql --insecure -e 'select 1'"
     report "START SERVER DONE"
 }

pkg/cli/interactive_tests/test_flags.tcl

@@ -80,5 +80,19 @@ interrupt
 eexpect ":/# "
 end_test
 
+
+start_server $argv
+
+start_test "Check that a client can connect using the URL env var"
+send "export COCKROACH_URL=`cat server_url`;\r"
+eexpect ":/# "
+send "$argv sql\r"
+eexpect "defaultdb>"
+interrupt
+eexpect ":/# "
+end_test
+
+stop_server $argv
+
 send "exit 0\r"
 eexpect eof

pkg/cli/start.go

@@ -414,7 +414,7 @@ func initTempStorageConfig(
 var errCannotUseJoin = errors.New("cannot use --join with 'cockroach start-single-node' -- use 'cockrach start' instead")
 
 func runStartSingleNode(cmd *cobra.Command, args []string) error {
-	joinFlag := cmd.Flags().Lookup(cliflags.Join.Name)
+	joinFlag := flagSetForCmd(cmd).Lookup(cliflags.Join.Name)
 	if joinFlag.Changed {
 		return errCannotUseJoin
 	}
@@ -501,8 +501,7 @@ func runStart(cmd *cobra.Command, args []string, disableReplication bool) error
 	grpcutil.SetSeverity(log.Severity_WARNING)
 
 	// Check the --join flag.
-	pf := cmd.Flags()
-	if !pf.Lookup(cliflags.Join.Name).Changed {
+	if !flagSetForCmd(cmd).Lookup(cliflags.Join.Name).Changed {
 		log.Shout(ctx, log.Severity_WARNING,
 			"running 'cockroach start' without --join is deprecated.\n"+
 				"Consider using 'cockroach start-single-node' or 'cockroach init' instead.")
@@ -981,7 +980,7 @@ If problems persist, please see ` + base.DocsURL("cluster-setup-troubleshooting.
 }
 
 func hintServerCmdFlags(ctx context.Context, cmd *cobra.Command) {
-	pf := cmd.Flags()
+	pf := flagSetForCmd(cmd)
 
 	listenAddrSpecified := pf.Lookup(cliflags.ListenAddr.Name).Changed || pf.Lookup(cliflags.ServerHost.Name).Changed
 	advAddrSpecified := pf.Lookup(cliflags.AdvertiseAddr.Name).Changed || pf.Lookup(cliflags.AdvertiseHost.Name).Changed