diff --git a/pkg/cli/client_url.go b/pkg/cli/client_url.go index 3b626b6b4db4..745fb02b98e8 100644 --- a/pkg/cli/client_url.go +++ b/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 { diff --git a/pkg/cli/demo.go b/pkg/cli/demo.go index 91608ee12754..b0b833476a58 100644 --- a/pkg/cli/demo.go +++ b/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") } diff --git a/pkg/cli/flags.go b/pkg/cli/flags.go index 39d15a606ff9..3dcd5a636ef8 100644 --- a/pkg/cli/flags.go +++ b/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,14 +117,14 @@ 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. @@ -142,7 +132,7 @@ 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, diff --git a/pkg/cli/interactive_tests/common.tcl b/pkg/cli/interactive_tests/common.tcl index 9723952412d7..5eb0d1328093 100644 --- a/pkg/cli/interactive_tests/common.tcl +++ b/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" } diff --git a/pkg/cli/interactive_tests/test_flags.tcl b/pkg/cli/interactive_tests/test_flags.tcl index 188c1602138a..24f0f4d7902e 100644 --- a/pkg/cli/interactive_tests/test_flags.tcl +++ b/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 diff --git a/pkg/cli/start.go b/pkg/cli/start.go index 01e9616026ea..9e510fb0d225 100644 --- a/pkg/cli/start.go +++ b/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