[v17] MWI: Fix certain --no- flags failing to override config file

lib/tbot/cli/cli.go

@@ -214,6 +214,21 @@ func BaseConfigWithMutators(mutators ...ConfigMutator) (*config.BotConfig, error
 	return cfg, nil
 }
 
+// TestConfigWithMutators applies mutators to the input config and returns the
+// result. `CheckAndSetDefaults()` will be called before returning. This is
+// meant for use in tests to avoid needing to load or parse a config file.
+func TestConfigWithMutators(cfg *config.BotConfig, mutators ...ConfigMutator) (*config.BotConfig, error) {
+	if err := applyMutators(log, cfg, mutators...); err != nil {
+		return nil, trace.Wrap(err)
+	}
+
+	if err := cfg.CheckAndSetDefaults(); err != nil {
+		return nil, trace.Wrap(err)
+	}
+
+	return cfg, nil
+}
+
 // RemainingArgsList is a custom kingpin parser that consumes all remaining
 // arguments.
 type RemainingArgsList []string

lib/tbot/cli/cli_test.go

@@ -51,7 +51,8 @@ func TestConfigCLIOnlySample(t *testing.T) {
 	}
 
 	cfg, err := LoadConfigWithMutators(&GlobalArgs{
-		Debug: true,
+		Debug:          true,
+		debugSetByUser: true,
 	}, legacy)
 	require.NoError(t, err)
 

lib/tbot/cli/globals.go

@@ -63,20 +63,29 @@ type GlobalArgs struct {
 
 	// staticConfigYAML allows tests to specify a configuration file statically
 	staticConfigYAML string
+
+	fipsSetByUser     bool
+	debugSetByUser    bool
+	insecureSetByUser bool
 }
 
 // NewGlobalArgs appends global flags to the application and returns a struct
 // that will be populated at parse time.
 func NewGlobalArgs(app *kingpin.Application) *GlobalArgs {
 	g := &GlobalArgs{}
 
-	app.Flag("debug", "Verbose logging to stdout.").Short('d').Envar(TBotDebugEnvVar).BoolVar(&g.Debug)
+	app.Flag("debug", "Verbose logging to stdout.").Short('d').Envar(TBotDebugEnvVar).IsSetByUser(&g.debugSetByUser).BoolVar(&g.Debug)
 	app.Flag("config", "Path to a configuration file.").Short('c').Envar(TBotConfigPathEnvVar).StringVar(&g.ConfigPath)
 	app.Flag("config-string", "Base64 encoded configuration string.").Hidden().Envar(TBotConfigEnvVar).StringVar(&g.ConfigString)
-	app.Flag("fips", "Runs tbot in FIPS compliance mode. This requires the FIPS binary is in use.").BoolVar(&g.FIPS)
+	app.Flag("fips", "Runs tbot in FIPS compliance mode. This requires the FIPS binary is in use.").IsSetByUser(&g.fipsSetByUser).BoolVar(&g.FIPS)
 	app.Flag("trace", "Capture and export distributed traces.").Hidden().BoolVar(&g.Trace)
 	app.Flag("trace-exporter", "An OTLP exporter URL to send spans to.").Hidden().StringVar(&g.TraceExporter)
-	app.Flag("insecure", "Insecure configures the bot to trust the certificates from the Auth Server or Proxy on first connect without verification. Do not use in production.").BoolVar(&g.Insecure)
+	app.Flag(
+		"insecure",
+		"Insecure configures the bot to trust the certificates from the Auth "+
+			"Server or Proxy on first connect without verification. Do not use in "+
+			"production.",
+	).IsSetByUser(&g.insecureSetByUser).BoolVar(&g.Insecure)
 	app.Flag("log-format", "Controls the format of output logs. Can be `json` or `text`. Defaults to `text`.").
 		Default(utils.LogFormatText).
 		EnumVar(&g.LogFormat, utils.LogFormatJSON, utils.LogFormatText)
@@ -98,16 +107,16 @@ func (g *GlobalArgs) ApplyConfig(cfg *config.BotConfig, l *slog.Logger) error {
 	// Note: g.ConfigPath is not checked here; the config must have already been
 	// loaded.
 
-	if g.FIPS {
+	if g.fipsSetByUser {
 		cfg.FIPS = g.FIPS
 	}
 
-	if g.Debug {
+	if g.debugSetByUser {
 		cfg.Debug = g.Debug
 	}
 
-	if g.Insecure {
-		cfg.Insecure = true
+	if g.insecureSetByUser {
+		cfg.Insecure = g.Insecure
 	}
 
 	return nil

lib/tbot/cli/globals_test.go

@@ -22,6 +22,8 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+
+	"github.com/gravitational/teleport/lib/tbot/config"
 )
 
 // TestGlobals tests that GlobalArgs initialize and parse properly, and mutate
@@ -63,3 +65,31 @@ func TestGlobalArgs(t *testing.T) {
 	require.True(t, cfg.FIPS)
 	require.True(t, cfg.Insecure)
 }
+
+func TestGlobalInvertedFlags(t *testing.T) {
+	app, _ := buildMinimalKingpinApp("start")
+	globals := NewGlobalArgs(app)
+
+	_, err := app.Parse([]string{
+		"start",
+		"--no-debug",
+		"--no-fips",
+		"--no-insecure",
+		"--config=foo.yaml",
+		"--trace",
+		"--trace-exporter=foo",
+		"--log-format=json",
+	})
+	require.NoError(t, err)
+
+	cfg, err := TestConfigWithMutators(&config.BotConfig{
+		Debug:    true,
+		FIPS:     true,
+		Insecure: true,
+	}, globals)
+	require.NoError(t, err)
+
+	require.False(t, cfg.Debug)
+	require.False(t, cfg.FIPS)
+	require.False(t, cfg.Insecure)
+}

lib/tbot/cli/start_legacy.go

@@ -128,6 +128,8 @@ type LegacyCommand struct {
 	// DiagAddr is the address the diagnostics http service should listen on.
 	// If not set, no diagnostics listener is created.
 	DiagAddr string
+
+	oneshotSetByUser bool
 }
 
 // NewLegacyCommand initializes and returns a command supporting
@@ -151,7 +153,7 @@ func NewLegacyCommand(parentCmd *kingpin.CmdClause, action MutatorAction, mode C
 	c.cmd.Flag("certificate-ttl", "TTL of short-lived machine certificates.").DurationVar(&c.CertificateTTL)
 	c.cmd.Flag("renewal-interval", "Interval at which short-lived certificates are renewed; must be less than the certificate TTL.").DurationVar(&c.RenewalInterval)
 	c.cmd.Flag("join-method", "Method to use to join the cluster. "+joinMethodList).EnumVar(&c.JoinMethod, config.SupportedJoinMethods...)
-	c.cmd.Flag("oneshot", "If set, quit after the first renewal.").BoolVar(&c.Oneshot)
+	c.cmd.Flag("oneshot", "If set, quit after the first renewal.").IsSetByUser(&c.oneshotSetByUser).BoolVar(&c.Oneshot)
 	c.cmd.Flag("diag-addr", "If set and the bot is in debug mode, a diagnostics service will listen on specified address.").StringVar(&c.DiagAddr)
 
 	return c
@@ -183,8 +185,8 @@ func (c *LegacyCommand) ApplyConfig(cfg *config.BotConfig, l *slog.Logger) error
 		}
 	}
 
-	if c.Oneshot {
-		cfg.Oneshot = true
+	if c.oneshotSetByUser {
+		cfg.Oneshot = c.Oneshot
 	}
 
 	if c.CertificateTTL != 0 {

lib/tbot/cli/start_shared.go

@@ -115,6 +115,8 @@ type sharedStartArgs struct {
 
 	Oneshot  bool
 	DiagAddr string
+
+	oneshotSetByUser bool
 }
 
 // newSharedStartArgs initializes shared arguments on the given parent command.
@@ -132,7 +134,7 @@ func newSharedStartArgs(cmd *kingpin.CmdClause) *sharedStartArgs {
 	cmd.Flag("certificate-ttl", "TTL of short-lived machine certificates.").DurationVar(&args.CertificateTTL)
 	cmd.Flag("renewal-interval", "Interval at which short-lived certificates are renewed; must be less than the certificate TTL.").DurationVar(&args.RenewalInterval)
 	cmd.Flag("join-method", "Method to use to join the cluster. "+joinMethodList).EnumVar(&args.JoinMethod, config.SupportedJoinMethods...)
-	cmd.Flag("oneshot", "If set, quit after the first renewal.").BoolVar(&args.Oneshot)
+	cmd.Flag("oneshot", "If set, quit after the first renewal.").IsSetByUser(&args.oneshotSetByUser).BoolVar(&args.Oneshot)
 	cmd.Flag("diag-addr", "If set and the bot is in debug mode, a diagnostics service will listen on specified address.").StringVar(&args.DiagAddr)
 	cmd.Flag("storage", "A destination URI for tbot's internal storage, e.g. file:///foo/bar").StringVar(&args.Storage)
 
@@ -148,8 +150,8 @@ func (s *sharedStartArgs) ApplyConfig(cfg *config.BotConfig, l *slog.Logger) err
 		}
 	}
 
-	if s.Oneshot {
-		cfg.Oneshot = true
+	if s.oneshotSetByUser {
+		cfg.Oneshot = s.Oneshot
 	}
 
 	if s.CertificateTTL != 0 {