MWI: Fix certain --no- flags failing to override config file (#54402)

* MWI: Fix certain `--no-` flags failing to override config file

This fixes a few instances where `--no-` flags were unable to
override config file parameters as we didn't check if the flag was
actually set by the user. This now uses the value parsed by kingpin
directly if an associated `*SetByUser` flag is also set, making it
possible to use, e.g., `--no-oneshot` to disable oneshot mode if
also loading a `tbot.yaml` with the flag enabled.

Most `BoolVar()` uses in tbot don't apply - only those that can
plausibly be overridden by a config file. That means only `start`
type commands, and then only flags which produce configs that can
actually be overridden, which excludes most generated service
definitions.

* Fix tests

Adds a new test for inverted global flags, and fixes a broken test.

* Fix lints

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,6 +63,10 @@ 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
@@ -70,13 +74,18 @@ type GlobalArgs struct {
 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 {