From b717b5174fc43efe64a6cd72d3020987079f2d9e Mon Sep 17 00:00:00 2001 From: Tim Buckley Date: Thu, 8 May 2025 20:32:09 -0600 Subject: [PATCH] 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 | 15 +++++++++++++++ lib/tbot/cli/cli_test.go | 3 ++- lib/tbot/cli/globals.go | 23 ++++++++++++++++------- lib/tbot/cli/globals_test.go | 30 ++++++++++++++++++++++++++++++ lib/tbot/cli/start_legacy.go | 8 +++++--- lib/tbot/cli/start_shared.go | 8 +++++--- 6 files changed, 73 insertions(+), 14 deletions(-) diff --git a/lib/tbot/cli/cli.go b/lib/tbot/cli/cli.go index ff82aae5699..c85ebbef47f 100644 --- a/lib/tbot/cli/cli.go +++ b/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 diff --git a/lib/tbot/cli/cli_test.go b/lib/tbot/cli/cli_test.go index 6e60e8c7b18..27b47ebe85f 100644 --- a/lib/tbot/cli/cli_test.go +++ b/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) diff --git a/lib/tbot/cli/globals.go b/lib/tbot/cli/globals.go index 8796e6e0ba4..644fcd5ce37 100644 --- a/lib/tbot/cli/globals.go +++ b/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 diff --git a/lib/tbot/cli/globals_test.go b/lib/tbot/cli/globals_test.go index 06fdc5256a8..aa49942dead 100644 --- a/lib/tbot/cli/globals_test.go +++ b/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) +} diff --git a/lib/tbot/cli/start_legacy.go b/lib/tbot/cli/start_legacy.go index e765c0d99b4..7a392d3632a 100644 --- a/lib/tbot/cli/start_legacy.go +++ b/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 { diff --git a/lib/tbot/cli/start_shared.go b/lib/tbot/cli/start_shared.go index e8c2b895d0d..9d567500bf3 100644 --- a/lib/tbot/cli/start_shared.go +++ b/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 {