From b3d516b6da0f1896093791dbcd05fe3aa85169f8 Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 12 Aug 2026 14:50:30 +0100 Subject: [PATCH 1/2] fix(config): detect explicitly passed flags in collector and alerter Both services documented the precedence of built-in defaults, then the configuration file, then command-line flags, but neither could tell an explicitly passed flag from one left at its registered default: each decided the question by comparing the flag's current value against its own hardcoded default. The collector did this for `-pg-port` and `-pg-sslmode`, whose defaults are real values rather than zero values, and the alerter did the same for its whole `applyFlagOverrides` set, where the zero-value defaults mostly masked the problem. That comparison conflates two genuinely different situations, and it gets both of them wrong. A configuration file value that happens to equal a flag's default was overwritten even though nothing was passed on the command line, and a flag passed explicitly with its default value was ignored: a file setting `datastore.port: 6000` survived `-pg-port 5432`, which is precisely the case an operator would reach for to force the standard port back. The new `pkg/flagutil` helper records the flags a `flag.FlagSet` actually saw, via `flag.Visit`, and both binaries now consult that set instead of comparing values. The collector threads the set through `loadConfiguration` into `ApplyFlags`, whilst the alerter carries it in the `flagOverrides` bundle that both startup and the SIGHUP reload path already share, so overrides survive a reload exactly as before. Flag names became constants in both binaries so registration and lookup cannot drift apart. Closes #389 --- .claude/golang-expert/testing-strategy.md | 32 ++++ .../cmd/ai-dba-alerter/flagoverrides_test.go | 95 ++++++++++++ alerter/src/cmd/ai-dba-alerter/main.go | 107 ++++++++----- alerter/src/cmd/ai-dba-alerter/main_test.go | 50 ++++-- collector/src/config.go | 32 ++-- collector/src/config_test.go | 144 ++++++++++++++++-- collector/src/main.go | 48 ++++-- collector/src/main_test.go | 23 +-- docs/changelog.md | 12 ++ docs/getting-started/configuration/alerter.md | 6 + .../configuration/collector.md | 6 + pkg/flagutil/flagutil.go | 47 ++++++ pkg/flagutil/flagutil_test.go | 95 ++++++++++++ 13 files changed, 597 insertions(+), 100 deletions(-) create mode 100644 alerter/src/cmd/ai-dba-alerter/flagoverrides_test.go create mode 100644 pkg/flagutil/flagutil.go create mode 100644 pkg/flagutil/flagutil_test.go diff --git a/.claude/golang-expert/testing-strategy.md b/.claude/golang-expert/testing-strategy.md index 49ba4faf..26a537a3 100644 --- a/.claude/golang-expert/testing-strategy.md +++ b/.claude/golang-expert/testing-strategy.md @@ -307,6 +307,38 @@ func skipIfNoDatabase(t *testing.T) *pgxpool.Pool { } ``` +### Testing Command-Line Flag Precedence + +Configuration is layered as defaults, then configuration file, then +command-line flags. Whether a flag overrides the file is decided by +whether the operator actually passed it, never by comparing the +flag's value against its default; a value comparison silently drops +an explicitly passed default value and clobbers a file value that +happens to match a default. + +The shared helper `pkg/flagutil` provides `Passed(fs *flag.FlagSet) +Set`, backed by `flag.FlagSet.Visit`, and `Set.Has(name)`. The +collector threads a `flagutil.Set` into `loadConfiguration` and +`(*Config).ApplyFlags`; the alerter carries one in the `Passed` +field of its `flagOverrides` struct, which `applyFlagOverrides` and +the SIGHUP reload path both consume. Both binaries declare their +flag names as constants, so registration and lookup cannot drift. + +Tests construct the set directly rather than parsing a command +line, so both branches of every override are reachable: + +```go +// The flag was passed, carrying the value that is also its default. +cfg.ApplyFlags(flagutil.Set{flagPGPort: true}) + +// Nothing was passed, so the config file value must survive. +cfg.ApplyFlags(nil) +``` + +Any new flag needs both cases covered: a configuration value that +coincides with the flag's default must survive when the flag is +absent, and an explicitly passed default value must still apply. + ## Database Testing ### Test Database Lifecycle diff --git a/alerter/src/cmd/ai-dba-alerter/flagoverrides_test.go b/alerter/src/cmd/ai-dba-alerter/flagoverrides_test.go new file mode 100644 index 00000000..f823693d --- /dev/null +++ b/alerter/src/cmd/ai-dba-alerter/flagoverrides_test.go @@ -0,0 +1,95 @@ +/*------------------------------------------------------------------------- + * + * pgEdge AI DBA Workbench + * + * Copyright (c) 2025 - 2026, pgEdge, Inc. + * This software is released under The PostgreSQL License + * + *------------------------------------------------------------------------- + */ +package main + +import ( + "testing" + + "github.com/pgedge/ai-workbench/alerter/internal/config" + "github.com/pgedge/ai-workbench/pkg/flagutil" +) + +// TestApplyFlagOverrides_ExplicitDefaultValueWins covers the case the +// old value-comparison logic could not express: the operator passes a +// flag whose value equals the flag's registered default, and the flag +// must still override the configuration file. +func TestApplyFlagOverrides_ExplicitDefaultValueWins(t *testing.T) { + cfg := config.NewConfig() + // Values as if they had come from a configuration file. + cfg.Datastore.Host = "file-host" + cfg.Datastore.Port = 6000 + cfg.Datastore.SSLMode = "require" + + // Every value below is the corresponding flag's registered + // default, yet each flag was explicitly passed, so each must + // still be applied. + err := applyFlagOverrides(cfg, flagOverrides{ + DBHost: "", + DBPort: 0, + DBSSLMode: "", + Passed: flagutil.Set{ + flagDBHost: true, + flagDBPort: true, + flagDBSSLMode: true, + }, + }) + if err != nil { + t.Fatalf("applyFlagOverrides: %v", err) + } + if cfg.Datastore.Host != "" { + t.Errorf("Host = %q, want the explicitly passed empty value", cfg.Datastore.Host) + } + if cfg.Datastore.Port != 0 { + t.Errorf("Port = %d, want the explicitly passed 0", cfg.Datastore.Port) + } + if cfg.Datastore.SSLMode != "" { + t.Errorf("SSLMode = %q, want the explicitly passed empty value", cfg.Datastore.SSLMode) + } +} + +// TestApplyFlagOverrides_ConfigMatchingFlagDefaultSurvives is the +// mirror image: configuration values that coincide with the flags' +// registered defaults must survive when no flag was passed. +func TestApplyFlagOverrides_ConfigMatchingFlagDefaultSurvives(t *testing.T) { + cfg := config.NewConfig() + cfg.Datastore.Host = "" + cfg.Datastore.Port = 0 + cfg.Datastore.SSLMode = "" + + if err := applyFlagOverrides(cfg, flagOverrides{ + DBHost: "unused-host", + DBPort: 9999, + DBSSLMode: "unused-mode", + }); err != nil { + t.Fatalf("applyFlagOverrides: %v", err) + } + if cfg.Datastore.Host != "" || cfg.Datastore.Port != 0 || + cfg.Datastore.SSLMode != "" { + t.Errorf("unpassed flags overrode the config: %+v", cfg.Datastore) + } +} + +// TestApplyFlagOverrides_ExplicitEmptyPasswordFile verifies that an +// explicitly empty -db-password-file is treated as "no password +// file" rather than as a path to read, which would fail. +func TestApplyFlagOverrides_ExplicitEmptyPasswordFile(t *testing.T) { + cfg := config.NewConfig() + cfg.Datastore.Password = "from-config" + + if err := applyFlagOverrides(cfg, flagOverrides{ + DBPasswordFile: "", + Passed: flagutil.Set{flagDBPasswordFile: true}, + }); err != nil { + t.Fatalf("applyFlagOverrides: %v", err) + } + if cfg.Datastore.Password != "from-config" { + t.Errorf("Password = %q, want it left alone", cfg.Datastore.Password) + } +} diff --git a/alerter/src/cmd/ai-dba-alerter/main.go b/alerter/src/cmd/ai-dba-alerter/main.go index 77d251e8..adbf646f 100644 --- a/alerter/src/cmd/ai-dba-alerter/main.go +++ b/alerter/src/cmd/ai-dba-alerter/main.go @@ -22,6 +22,7 @@ import ( "github.com/pgedge/ai-workbench/alerter/internal/database" "github.com/pgedge/ai-workbench/alerter/internal/engine" "github.com/pgedge/ai-workbench/pkg/fileutil" + "github.com/pgedge/ai-workbench/pkg/flagutil" ) // Version information @@ -60,17 +61,37 @@ func resolveConfigPath(flagValue string) resolveConfigPathResult { } } -// reloadFlagOverrides bundles the CLI flag values that survive -// across a SIGHUP reload and must be reapplied to the freshly -// loaded config so operators do not lose their command-line -// overrides on every reload. -type reloadFlagOverrides struct { +// Names of the database connection flags. They are named constants +// so the registration in main and the override logic in +// applyFlagOverrides cannot drift apart; the override logic looks a +// flag up by name to decide whether the operator actually passed it. +const ( + flagDBHost = "db-host" + flagDBPort = "db-port" + flagDBName = "db-name" + flagDBUser = "db-user" + flagDBPasswordFile = "db-password-file" + flagDBSSLMode = "db-sslmode" +) + +// flagOverrides bundles the CLI flag values that override the +// configuration file, together with the set of flags the operator +// actually passed. The same bundle is reapplied to the freshly +// loaded config across a SIGHUP reload so operators do not lose +// their command-line overrides on every reload. +// +// Passed is what makes an override unambiguous: a flag value on its +// own cannot distinguish "the operator asked for this" from "this is +// the registered default", so a config file value that happens to +// match a default would otherwise be overwritten. +type flagOverrides struct { DBHost string DBPort int DBName string DBUser string DBPasswordFile string DBSSLMode string + Passed flagutil.Set } // reloadConfigOnSignal builds a fresh *config.Config from the same @@ -95,7 +116,7 @@ func reloadConfigOnSignal( logOut io.Writer, prevPath string, explicit bool, - overrides reloadFlagOverrides, + overrides flagOverrides, ) (*config.Config, error) { reloadPath := prevPath if !explicit { @@ -119,10 +140,7 @@ func reloadConfigOnSignal( return nil, fmt.Errorf("failed to reload config: %w", err) } - if err := applyFlagOverrides(newCfg, - overrides.DBHost, overrides.DBPort, overrides.DBName, - overrides.DBUser, overrides.DBPasswordFile, overrides.DBSSLMode, - ); err != nil { + if err := applyFlagOverrides(newCfg, overrides); err != nil { return nil, fmt.Errorf("failed to apply overrides on reload: %w", err) } @@ -156,15 +174,27 @@ func main() { debug := flag.Bool("debug", false, "Enable debug logging") // Database connection flags - dbHost := flag.String("db-host", "", "Database host (overrides config)") - dbPort := flag.Int("db-port", 0, "Database port (overrides config)") - dbName := flag.String("db-name", "", "Database name (overrides config)") - dbUser := flag.String("db-user", "", "Database user (overrides config)") - dbPasswordFile := flag.String("db-password-file", "", "Path to file containing the database password") - dbSSLMode := flag.String("db-sslmode", "", "Database SSL mode (overrides config)") + dbHost := flag.String(flagDBHost, "", "Database host (overrides config)") + dbPort := flag.Int(flagDBPort, 0, "Database port (overrides config)") + dbName := flag.String(flagDBName, "", "Database name (overrides config)") + dbUser := flag.String(flagDBUser, "", "Database user (overrides config)") + dbPasswordFile := flag.String(flagDBPasswordFile, "", "Path to file containing the database password") + dbSSLMode := flag.String(flagDBSSLMode, "", "Database SSL mode (overrides config)") flag.Parse() + // Record which flags the operator actually passed, so overrides + // are driven by intent rather than by a value comparison. + overrides := flagOverrides{ + DBHost: *dbHost, + DBPort: *dbPort, + DBName: *dbName, + DBUser: *dbUser, + DBPasswordFile: *dbPasswordFile, + DBSSLMode: *dbSSLMode, + Passed: flagutil.Passed(flag.CommandLine), + } + // Resolve the config path: an explicit flag wins, otherwise the // shared discovery helper picks the highest-priority path that // exists. If neither exists, the resolved path is "" and the @@ -202,7 +232,7 @@ func main() { } // Apply command line overrides - if err := applyFlagOverrides(cfg, *dbHost, *dbPort, *dbName, *dbUser, *dbPasswordFile, *dbSSLMode); err != nil { + if err := applyFlagOverrides(cfg, overrides); err != nil { fmt.Fprintf(os.Stderr, "ERROR: %v\n", err) os.Exit(1) } @@ -262,14 +292,7 @@ func main() { os.Stderr, resolvedConfigPath, explicitConfigPath, - reloadFlagOverrides{ - DBHost: *dbHost, - DBPort: *dbPort, - DBName: *dbName, - DBUser: *dbUser, - DBPasswordFile: *dbPasswordFile, - DBSSLMode: *dbSSLMode, - }, + overrides, ) if err != nil { fmt.Fprintf(os.Stderr, @@ -305,28 +328,36 @@ func main() { // applyFlagOverrides applies CLI flag values to the configuration, allowing // command-line arguments to take precedence over the configuration file. -func applyFlagOverrides(cfg *config.Config, dbHost string, dbPort int, dbName, dbUser, dbPasswordFile, dbSSLMode string) error { - if dbHost != "" { - cfg.Datastore.Host = dbHost +// +// Only the flags recorded in o.Passed are applied, so a value read +// from the configuration file survives unless the operator asked for +// something else, and an explicitly passed flag wins even when its +// value coincides with the flag's registered default. +func applyFlagOverrides(cfg *config.Config, o flagOverrides) error { + if o.Passed.Has(flagDBHost) { + cfg.Datastore.Host = o.DBHost } - if dbPort != 0 { - cfg.Datastore.Port = dbPort + if o.Passed.Has(flagDBPort) { + cfg.Datastore.Port = o.DBPort } - if dbName != "" { - cfg.Datastore.Database = dbName + if o.Passed.Has(flagDBName) { + cfg.Datastore.Database = o.DBName } - if dbUser != "" { - cfg.Datastore.Username = dbUser + if o.Passed.Has(flagDBUser) { + cfg.Datastore.Username = o.DBUser } - if dbPasswordFile != "" { - password, err := fileutil.ReadSecretFile(dbPasswordFile) + // An explicitly empty -db-password-file means "no password file", + // so there is nothing to read; only a non-empty path triggers a + // read, which would otherwise fail on the empty path. + if o.Passed.Has(flagDBPasswordFile) && o.DBPasswordFile != "" { + password, err := fileutil.ReadSecretFile(o.DBPasswordFile) if err != nil { return fmt.Errorf("failed to read password file: %w", err) } cfg.Datastore.Password = password } - if dbSSLMode != "" { - cfg.Datastore.SSLMode = dbSSLMode + if o.Passed.Has(flagDBSSLMode) { + cfg.Datastore.SSLMode = o.DBSSLMode } return nil } diff --git a/alerter/src/cmd/ai-dba-alerter/main_test.go b/alerter/src/cmd/ai-dba-alerter/main_test.go index 5ac66abd..d599fa3b 100644 --- a/alerter/src/cmd/ai-dba-alerter/main_test.go +++ b/alerter/src/cmd/ai-dba-alerter/main_test.go @@ -18,6 +18,7 @@ import ( "github.com/pgedge/ai-workbench/alerter/internal/config" "github.com/pgedge/ai-workbench/pkg/fileutil" + "github.com/pgedge/ai-workbench/pkg/flagutil" ) // minimalValidYAML is a config payload that satisfies @@ -111,7 +112,7 @@ func TestReloadConfigOnSignal_ExplicitMissing(t *testing.T) { &buf, "/definitely/not/a/real/path.yaml", true, // explicit - reloadFlagOverrides{}, + flagOverrides{}, ) if err != nil { t.Fatalf("reloadConfigOnSignal: unexpected error %v", err) @@ -141,7 +142,7 @@ func TestReloadConfigOnSignal_NoCandidateFile(t *testing.T) { &buf, "", // no previously-resolved path either false, - reloadFlagOverrides{}, + flagOverrides{}, ) if err != nil { t.Fatalf("reloadConfigOnSignal: unexpected error %v", err) @@ -168,7 +169,7 @@ func TestReloadConfigOnSignal_HappyPath(t *testing.T) { &buf, cfgPath, true, - reloadFlagOverrides{}, + flagOverrides{}, ) if err != nil { t.Fatalf("reloadConfigOnSignal: %v", err) @@ -185,7 +186,7 @@ func TestReloadConfigOnSignal_HappyPath(t *testing.T) { } // TestReloadConfigOnSignal_FlagOverridesApplied verifies that the -// CLI flag overrides bundled in reloadFlagOverrides are applied +// CLI flag overrides bundled in flagOverrides are applied // to the freshly loaded config so SIGHUP does not silently drop // command-line settings. func TestReloadConfigOnSignal_FlagOverridesApplied(t *testing.T) { @@ -200,10 +201,15 @@ func TestReloadConfigOnSignal_FlagOverridesApplied(t *testing.T) { &buf, cfgPath, true, - reloadFlagOverrides{ + flagOverrides{ DBHost: "override-host", DBPort: 9999, DBSSLMode: "require", + Passed: flagutil.Set{ + flagDBHost: true, + flagDBPort: true, + flagDBSSLMode: true, + }, }, ) if err != nil { @@ -243,7 +249,7 @@ func TestReloadConfigOnSignal_InvalidConfig(t *testing.T) { &buf, cfgPath, true, - reloadFlagOverrides{}, + flagOverrides{}, ) if err == nil { t.Fatal("expected validation error, got nil") @@ -270,7 +276,7 @@ func TestReloadConfigOnSignal_MalformedYAML(t *testing.T) { &buf, cfgPath, true, - reloadFlagOverrides{}, + flagOverrides{}, ) if err == nil { t.Fatal("expected YAML parse error, got nil") @@ -298,7 +304,7 @@ func TestReloadConfigOnSignal_APIKeyWarning(t *testing.T) { &buf, cfgPath, true, - reloadFlagOverrides{}, + flagOverrides{}, ) if err != nil { t.Fatalf("reloadConfigOnSignal: %v", err) @@ -333,7 +339,7 @@ func TestReloadConfigOnSignal_PasswordFileMissing(t *testing.T) { &buf, cfgPath, true, - reloadFlagOverrides{}, + flagOverrides{}, ) if err == nil { t.Fatal("expected password-load error, got nil") @@ -352,7 +358,14 @@ func TestReloadConfigOnSignal_PasswordFileMissing(t *testing.T) { func TestApplyFlagOverrides(t *testing.T) { t.Run("all scalar overrides applied", func(t *testing.T) { cfg := config.NewConfig() - if err := applyFlagOverrides(cfg, "h", 5433, "db", "user", "", "require"); err != nil { + if err := applyFlagOverrides(cfg, flagOverrides{ + DBHost: "h", DBPort: 5433, DBName: "db", DBUser: "user", + DBSSLMode: "require", + Passed: flagutil.Set{ + flagDBHost: true, flagDBPort: true, flagDBName: true, + flagDBUser: true, flagDBSSLMode: true, + }, + }); err != nil { t.Fatalf("unexpected error: %v", err) } if cfg.Datastore.Host != "h" || cfg.Datastore.Port != 5433 || @@ -365,7 +378,7 @@ func TestApplyFlagOverrides(t *testing.T) { t.Run("no overrides leaves config untouched", func(t *testing.T) { cfg := config.NewConfig() host := cfg.Datastore.Host - if err := applyFlagOverrides(cfg, "", 0, "", "", "", ""); err != nil { + if err := applyFlagOverrides(cfg, flagOverrides{}); err != nil { t.Fatalf("unexpected error: %v", err) } if cfg.Datastore.Host != host { @@ -379,7 +392,10 @@ func TestApplyFlagOverrides(t *testing.T) { t.Fatalf("write: %v", err) } cfg := config.NewConfig() - if err := applyFlagOverrides(cfg, "", 0, "", "", pwFile, ""); err != nil { + if err := applyFlagOverrides(cfg, flagOverrides{ + DBPasswordFile: pwFile, + Passed: flagutil.Set{flagDBPasswordFile: true}, + }); err != nil { t.Fatalf("unexpected error: %v", err) } if cfg.Datastore.Password != "flag-password" { @@ -389,7 +405,10 @@ func TestApplyFlagOverrides(t *testing.T) { t.Run("missing password file is an error", func(t *testing.T) { cfg := config.NewConfig() - if err := applyFlagOverrides(cfg, "", 0, "", "", "/nonexistent/pw", ""); err == nil { + if err := applyFlagOverrides(cfg, flagOverrides{ + DBPasswordFile: "/nonexistent/pw", + Passed: flagutil.Set{flagDBPasswordFile: true}, + }); err == nil { t.Error("expected error for missing password file") } }) @@ -400,7 +419,10 @@ func TestApplyFlagOverrides(t *testing.T) { t.Fatalf("write: %v", err) } cfg := config.NewConfig() - if err := applyFlagOverrides(cfg, "", 0, "", "", pwFile, ""); err == nil { + if err := applyFlagOverrides(cfg, flagOverrides{ + DBPasswordFile: pwFile, + Passed: flagutil.Set{flagDBPasswordFile: true}, + }); err == nil { t.Error("expected error for empty password file") } }) diff --git a/collector/src/config.go b/collector/src/config.go index 3f1f174f..7c917d8b 100644 --- a/collector/src/config.go +++ b/collector/src/config.go @@ -16,6 +16,7 @@ import ( "github.com/pgedge/ai-workbench/pkg/datastoreconfig" "github.com/pgedge/ai-workbench/pkg/fileutil" + "github.com/pgedge/ai-workbench/pkg/flagutil" "gopkg.in/yaml.v3" ) @@ -89,36 +90,43 @@ func (c *Config) LoadFromFile(filename string) error { return nil } -// ApplyFlags applies command line flags to override config values -func (c *Config) ApplyFlags() { - if *pgHost != "" { +// ApplyFlags applies command line flags to override config values. +// +// The passed set names the flags the operator actually supplied on +// the command line, as reported by flagutil.Passed. Only those flags +// override the configuration file, so a file value that happens to +// equal a flag's registered default survives, and a flag explicitly +// passed with its default value still wins. Deciding on the value +// alone cannot tell those two cases apart. +func (c *Config) ApplyFlags(passed flagutil.Set) { + if passed.Has(flagPGHost) { c.Datastore.Host = *pgHost } - if *pgHostAddr != "" { + if passed.Has(flagPGHostAddr) { c.Datastore.HostAddr = *pgHostAddr } - if *pgDatabase != "" { + if passed.Has(flagPGDatabase) { c.Datastore.Database = *pgDatabase } - if *pgUsername != "" { + if passed.Has(flagPGUsername) { c.Datastore.Username = *pgUsername } - if *pgPasswordFile != "" { + if passed.Has(flagPGPasswordFile) { c.Datastore.PasswordFile = *pgPasswordFile } - if *pgPort != 5432 { + if passed.Has(flagPGPort) { c.Datastore.Port = *pgPort } - if *pgSSLMode != "prefer" { + if passed.Has(flagPGSSLMode) { c.Datastore.SSLMode = *pgSSLMode } - if *pgSSLCert != "" { + if passed.Has(flagPGSSLCert) { c.Datastore.SSLCert = *pgSSLCert } - if *pgSSLKey != "" { + if passed.Has(flagPGSSLKey) { c.Datastore.SSLKey = *pgSSLKey } - if *pgSSLRootCert != "" { + if passed.Has(flagPGSSLRootCert) { c.Datastore.SSLRootCert = *pgSSLRootCert } } diff --git a/collector/src/config_test.go b/collector/src/config_test.go index af22848b..3d6bb2de 100644 --- a/collector/src/config_test.go +++ b/collector/src/config_test.go @@ -16,6 +16,7 @@ import ( "github.com/pgedge/ai-workbench/pkg/datastoreconfig" "github.com/pgedge/ai-workbench/pkg/fileutil" + "github.com/pgedge/ai-workbench/pkg/flagutil" ) func TestNewConfig(t *testing.T) { @@ -469,6 +470,24 @@ func TestConfigLoadFromFile_InvalidYAML(t *testing.T) { } } +// allDatastoreFlagsPassed returns a flagutil.Set naming every +// datastore connection flag, as if the operator had supplied all of +// them on the command line. +func allDatastoreFlagsPassed() flagutil.Set { + return flagutil.Set{ + flagPGHost: true, + flagPGHostAddr: true, + flagPGDatabase: true, + flagPGUsername: true, + flagPGPasswordFile: true, + flagPGPort: true, + flagPGSSLMode: true, + flagPGSSLCert: true, + flagPGSSLKey: true, + flagPGSSLRootCert: true, + } +} + func TestConfigApplyFlags(t *testing.T) { // Save and restore the package-level flag pointers so this test is // isolated from other tests in the file. @@ -502,8 +521,9 @@ func TestConfigApplyFlags(t *testing.T) { *pgSSLKey = "/flag/key" *pgSSLRootCert = "/flag/root" + // Every flag is marked as passed, so every override applies. config := NewConfig() - config.ApplyFlags() + config.ApplyFlags(allDatastoreFlagsPassed()) if config.Datastore.Host != "flag-host" { t.Errorf("Host: got %q, want flag-host", config.Datastore.Host) @@ -537,7 +557,7 @@ func TestConfigApplyFlags(t *testing.T) { } } -func TestConfigApplyFlags_EmptyFlagsPreserveDefaults(t *testing.T) { +func TestConfigApplyFlags_UnpassedFlagsPreserveConfig(t *testing.T) { // Save and restore flag pointers. origHost, origHostAddr := *pgHost, *pgHostAddr origDB, origUser := *pgDatabase, *pgUsername @@ -557,18 +577,20 @@ func TestConfigApplyFlags_EmptyFlagsPreserveDefaults(t *testing.T) { *pgSSLRootCert = origSSLRootCert }) - // Set all flags to their default values: ApplyFlags should not - // overwrite any config field in this case. - *pgHost = "" - *pgHostAddr = "" - *pgDatabase = "" - *pgUsername = "" - *pgPasswordFile = "" + // Give every flag a value that differs from the configuration + // below, but mark none of them as passed. ApplyFlags must ignore + // the values entirely, because nothing was supplied on the + // command line. + *pgHost = "flag-host" + *pgHostAddr = "10.0.0.5" + *pgDatabase = "flag-db" + *pgUsername = "flag-user" + *pgPasswordFile = "/flag/password" *pgPort = 5432 *pgSSLMode = "prefer" - *pgSSLCert = "" - *pgSSLKey = "" - *pgSSLRootCert = "" + *pgSSLCert = "/flag/cert" + *pgSSLKey = "/flag/key" + *pgSSLRootCert = "/flag/root" config := NewConfig() // Pre-set all fields so we can detect any unwanted overwrite. @@ -583,7 +605,7 @@ func TestConfigApplyFlags_EmptyFlagsPreserveDefaults(t *testing.T) { config.Datastore.SSLKey = "/preserved/key" config.Datastore.SSLRootCert = "/preserved/root" - config.ApplyFlags() + config.ApplyFlags(nil) if config.Datastore.Host != "preserved" { t.Errorf("Host should not have changed, got %q", config.Datastore.Host) @@ -617,6 +639,102 @@ func TestConfigApplyFlags_EmptyFlagsPreserveDefaults(t *testing.T) { } } +// TestConfigApplyFlags_ExplicitDefaultValueWins covers the case the +// old value-comparison logic could not express: the operator passes a +// flag whose value happens to equal the flag's registered default, so +// the flag must still override the configuration file. +func TestConfigApplyFlags_ExplicitDefaultValueWins(t *testing.T) { + origPort, origSSLMode := *pgPort, *pgSSLMode + t.Cleanup(func() { + *pgPort = origPort + *pgSSLMode = origSSLMode + }) + + // Both values are the flags' registered defaults. + *pgPort = 5432 + *pgSSLMode = "prefer" + + config := NewConfig() + // Values as if they had come from a configuration file. + config.Datastore.Port = 6000 + config.Datastore.SSLMode = "require" + + config.ApplyFlags(flagutil.Set{flagPGPort: true, flagPGSSLMode: true}) + + if config.Datastore.Port != 5432 { + t.Errorf("Port: got %d, want 5432 from the explicit flag", config.Datastore.Port) + } + if config.Datastore.SSLMode != "prefer" { + t.Errorf("SSLMode: got %q, want prefer from the explicit flag", config.Datastore.SSLMode) + } +} + +// TestConfigApplyFlags_ConfigMatchingFlagDefaultSurvives covers the +// mirror image: a configuration file value that coincides with a +// flag's registered default must survive when the flag is not passed. +func TestConfigApplyFlags_ConfigMatchingFlagDefaultSurvives(t *testing.T) { + origPort, origSSLMode := *pgPort, *pgSSLMode + t.Cleanup(func() { + *pgPort = origPort + *pgSSLMode = origSSLMode + }) + + *pgPort = 5432 + *pgSSLMode = "prefer" + + config := NewConfig() + config.Datastore.Port = 5432 + config.Datastore.SSLMode = "prefer" + + config.ApplyFlags(nil) + + if config.Datastore.Port != 5432 { + t.Errorf("Port: got %d, want the config value 5432", config.Datastore.Port) + } + if config.Datastore.SSLMode != "prefer" { + t.Errorf("SSLMode: got %q, want the config value prefer", config.Datastore.SSLMode) + } +} + +// TestLoadConfigurationFlagPrecedence drives the whole load path: a +// configuration file sets a non-default port, and an explicitly +// passed -pg-port carrying the flag's own default value must still +// win, which is precisely the case the old logic got wrong. +func TestLoadConfigurationFlagPrecedence(t *testing.T) { + tmpDir := t.TempDir() + configPath := filepath.Join(tmpDir, "collector.yaml") + secretPath := filepath.Join(tmpDir, "collector.secret") + + yaml := "datastore:\n host: config-host\n port: 6000\n sslmode: require\n" + + "secret_file: " + secretPath + "\n" + if err := os.WriteFile(configPath, []byte(yaml), 0600); err != nil { + t.Fatalf("write config: %v", err) + } + if err := os.WriteFile(secretPath, []byte("test-secret\n"), 0600); err != nil { + t.Fatalf("write secret: %v", err) + } + + defer saveAndClearFlags(t)() + *configFile = configPath + *pgPort = 5432 + *pgSSLMode = "prefer" + + // Only -pg-port was passed; -pg-sslmode was not. + cfg, err := loadConfiguration(flagutil.Set{flagPGPort: true}) + if err != nil { + t.Fatalf("loadConfiguration: %v", err) + } + if cfg.Datastore.Port != 5432 { + t.Errorf("Port: got %d, want 5432 from the explicit flag", cfg.Datastore.Port) + } + if cfg.Datastore.SSLMode != "require" { + t.Errorf("SSLMode: got %q, want require from the config file", cfg.Datastore.SSLMode) + } + if cfg.Datastore.Host != "config-host" { + t.Errorf("Host: got %q, want config-host from the config file", cfg.Datastore.Host) + } +} + func TestConfigLoadPassword_AlreadySet(t *testing.T) { config := NewConfig() config.Datastore.Password = "already-set" diff --git a/collector/src/main.go b/collector/src/main.go index cb67c30e..6bf53081 100644 --- a/collector/src/main.go +++ b/collector/src/main.go @@ -12,6 +12,7 @@ package main import ( "github.com/pgedge/ai-workbench/collector/src/database" "github.com/pgedge/ai-workbench/collector/src/scheduler" + "github.com/pgedge/ai-workbench/pkg/flagutil" "github.com/pgedge/ai-workbench/pkg/logger" "context" @@ -40,16 +41,33 @@ var ( "Print the latest datastore schema version this collector knows about and exit") // Datastore connection flags - pgHost = flag.String("pg-host", "", "PostgreSQL server hostname or IP address") - pgHostAddr = flag.String("pg-hostaddr", "", "PostgreSQL server IP address") - pgDatabase = flag.String("pg-database", "", "PostgreSQL database name") - pgUsername = flag.String("pg-username", "", "PostgreSQL username") - pgPasswordFile = flag.String("pg-password-file", "", "Path to file containing PostgreSQL password") - pgPort = flag.Int("pg-port", 5432, "PostgreSQL server port") - pgSSLMode = flag.String("pg-sslmode", "prefer", "PostgreSQL SSL mode") - pgSSLCert = flag.String("pg-sslcert", "", "Path to PostgreSQL client SSL certificate") - pgSSLKey = flag.String("pg-sslkey", "", "Path to PostgreSQL client SSL key") - pgSSLRootCert = flag.String("pg-sslrootcert", "", "Path to PostgreSQL root SSL certificate") + pgHost = flag.String(flagPGHost, "", "PostgreSQL server hostname or IP address") + pgHostAddr = flag.String(flagPGHostAddr, "", "PostgreSQL server IP address") + pgDatabase = flag.String(flagPGDatabase, "", "PostgreSQL database name") + pgUsername = flag.String(flagPGUsername, "", "PostgreSQL username") + pgPasswordFile = flag.String(flagPGPasswordFile, "", "Path to file containing PostgreSQL password") + pgPort = flag.Int(flagPGPort, 5432, "PostgreSQL server port") + pgSSLMode = flag.String(flagPGSSLMode, "prefer", "PostgreSQL SSL mode") + pgSSLCert = flag.String(flagPGSSLCert, "", "Path to PostgreSQL client SSL certificate") + pgSSLKey = flag.String(flagPGSSLKey, "", "Path to PostgreSQL client SSL key") + pgSSLRootCert = flag.String(flagPGSSLRootCert, "", "Path to PostgreSQL root SSL certificate") +) + +// Names of the datastore connection flags. They are named constants +// so the registration above and the override logic in ApplyFlags +// cannot drift apart; ApplyFlags looks a flag up by name to decide +// whether the operator actually passed it. +const ( + flagPGHost = "pg-host" + flagPGHostAddr = "pg-hostaddr" + flagPGDatabase = "pg-database" + flagPGUsername = "pg-username" + flagPGPasswordFile = "pg-password-file" + flagPGPort = "pg-port" + flagPGSSLMode = "pg-sslmode" + flagPGSSLCert = "pg-sslcert" + flagPGSSLKey = "pg-sslkey" + flagPGSSLRootCert = "pg-sslrootcert" ) func main() { @@ -76,7 +94,7 @@ func main() { logger.Startupf("pgEdge AI DBA Workbench Collector v%s starting...", Version) // Load configuration - config, err := loadConfiguration() + config, err := loadConfiguration(flagutil.Passed(flag.CommandLine)) if err != nil { logger.Fatalf("Failed to load configuration: %v", err) } @@ -162,12 +180,16 @@ func maybePrintSchemaVersion(w io.Writer, enabled bool) (bool, error) { // loadConfiguration loads configuration from file, environment, and command line. // Priority (highest to lowest): CLI flags > environment variables > config file > defaults. // +// The passed set names the flags the operator supplied on the command +// line; it is threaded in from main rather than read from the global +// flag set here so tests can drive both branches of every override. +// // When --config is not given, the function consults the shared // helper which searches the per-user config directory first and // /etc/pgedge second. When neither exists the function logs an // informational message and proceeds with compiled-in defaults // rather than failing. -func loadConfiguration() (*Config, error) { +func loadConfiguration(passed flagutil.Set) (*Config, error) { config := NewConfig() // Determine config file path. The empty string from the helper @@ -213,7 +235,7 @@ func loadConfiguration() (*Config, error) { } // Override with command line flags (highest priority) - config.ApplyFlags() + config.ApplyFlags(passed) // Load password from file if specified if err := config.LoadPassword(); err != nil { diff --git a/collector/src/main_test.go b/collector/src/main_test.go index faf09290..e4dcdb16 100644 --- a/collector/src/main_test.go +++ b/collector/src/main_test.go @@ -22,6 +22,7 @@ import ( "github.com/pgedge/ai-workbench/collector/src/database" "github.com/pgedge/ai-workbench/pkg/fileutil" + "github.com/pgedge/ai-workbench/pkg/flagutil" ) // saveAndClearFlags resets the package-level flag pointers and returns a @@ -68,7 +69,7 @@ func TestLoadConfiguration_ExplicitConfigMissing(t *testing.T) { *configFile = "/nonexistent/path/to/config.yaml" - cfg, err := loadConfiguration() + cfg, err := loadConfiguration(nil) if err == nil { t.Fatal("expected error when explicit config file does not exist") } @@ -87,7 +88,7 @@ func TestLoadConfiguration_ExplicitConfigMalformed(t *testing.T) { } *configFile = configPath - _, err := loadConfiguration() + _, err := loadConfiguration(nil) if err == nil { t.Fatal("expected error for malformed YAML") } @@ -105,7 +106,9 @@ func TestLoadConfiguration_PasswordFileMissing(t *testing.T) { *configFile = configPath *pgPasswordFile = "/definitely/missing/password-file" - _, err := loadConfiguration() + // -pg-password-file is marked as passed, so the override applies + // and loadConfiguration then fails to read the named file. + _, err := loadConfiguration(flagutil.Set{flagPGPasswordFile: true}) if err == nil { t.Fatal("expected error because password file is missing") } @@ -129,14 +132,14 @@ func TestLoadConfiguration_SecretFileMissing(t *testing.T) { } *configFile = configPath - _, err := loadConfiguration() + _, err := loadConfiguration(nil) if err == nil { t.Fatal("expected error because server secret file is missing") } } // TestLoadConfiguration_Success exercises the full happy path of -// loadConfiguration(): it writes a config file, a password file, and a +// loadConfiguration: it writes a config file, a password file, and a // secret file all within the temp directory and explicitly references // them in the config YAML. func TestLoadConfiguration_Success(t *testing.T) { @@ -166,7 +169,7 @@ func TestLoadConfiguration_Success(t *testing.T) { *configFile = configPath - cfg, err := loadConfiguration() + cfg, err := loadConfiguration(nil) if err != nil { t.Fatalf("loadConfiguration error: %v", err) } @@ -198,7 +201,7 @@ func TestLoadConfiguration_NoConfigFileUsesDefaults(t *testing.T) { // loadConfiguration must surface an error from LoadSecret since // there is no secret file available in either default location. - _, err := loadConfiguration() + _, err := loadConfiguration(nil) if err == nil { t.Fatal("expected error because no secret file exists on default paths") } @@ -224,7 +227,7 @@ func TestLoadConfiguration_ExplicitConfigPermissionError(t *testing.T) { } *configFile = cfgPath - _, err := loadConfiguration() + _, err := loadConfiguration(nil) if err == nil { t.Fatal("expected error when explicit config path is unreadable") } @@ -264,7 +267,7 @@ func TestLoadConfiguration_AutoDiscoveredUnreadable(t *testing.T) { t.Fatalf("Mkdir: %v", err) } - _, err = loadConfiguration() + _, err = loadConfiguration(nil) if err == nil { t.Fatal("expected error from non-IsNotExist auto-discovery branch") } @@ -309,7 +312,7 @@ func TestLoadConfiguration_DefaultConfigFromUserDir(t *testing.T) { t.Fatalf("write cfg: %v", err) } - cfg, err := loadConfiguration() + cfg, err := loadConfiguration(nil) if err != nil { t.Fatalf("loadConfiguration: %v", err) } diff --git a/docs/changelog.md b/docs/changelog.md index 1d847e9f..46e19830 100644 --- a/docs/changelog.md +++ b/docs/changelog.md @@ -61,6 +61,18 @@ project adheres to ### Fixed +- Fix the collector and the alerter overriding a configuration file + value whenever that value happened to match a command-line flag's + built-in default, and ignoring a flag that an operator passed + explicitly with its default value. Both services decided whether a + flag had been supplied by comparing the flag's value against its + own default, which cannot tell the two cases apart; for example, + a collector configuration file setting `datastore.port: 6000` + ignored `-pg-port 5432` on the command line. Both services now + detect the flags that were actually present on the command line, + so the documented precedence of defaults, then configuration file, + then flags holds in every case. (#389) + - Fix every chat request that included a tool list failing with `anthropic (400): tools.0.custom.input_schema: Input does not match the expected shape`, which broke Ask Ellie and the Server, diff --git a/docs/getting-started/configuration/alerter.md b/docs/getting-started/configuration/alerter.md index 5fb1212d..564c8e7d 100644 --- a/docs/getting-started/configuration/alerter.md +++ b/docs/getting-started/configuration/alerter.md @@ -13,6 +13,12 @@ following order; later sources override earlier ones: 2. Configuration file settings (YAML format). 3. Command-line flag overrides. +The alerter overrides a configuration file setting only when you +actually pass the corresponding flag; a flag you omit never +overrides the file, even though the flag has a built-in default. A +flag you do pass always wins, including when the value you give it +happens to equal that flag's default. + ## Configuration File The alerter searches for its configuration file in the diff --git a/docs/getting-started/configuration/collector.md b/docs/getting-started/configuration/collector.md index 9f1891fc..399264f0 100644 --- a/docs/getting-started/configuration/collector.md +++ b/docs/getting-started/configuration/collector.md @@ -8,6 +8,12 @@ order; later sources override earlier ones: 2. Configuration file. 3. Command-line flags. +The collector overrides a configuration file setting only when you +actually pass the corresponding flag; a flag you omit never +overrides the file, even though the flag has a built-in default. A +flag you do pass always wins, including when the value you give it +happens to equal that flag's default, such as `-pg-port 5432`. + ## File Location The collector searches for its configuration file in the following diff --git a/pkg/flagutil/flagutil.go b/pkg/flagutil/flagutil.go new file mode 100644 index 00000000..d2cafff9 --- /dev/null +++ b/pkg/flagutil/flagutil.go @@ -0,0 +1,47 @@ +/*------------------------------------------------------------------------- + * + * pgEdge AI DBA Workbench + * + * Copyright (c) 2025 - 2026, pgEdge, Inc. + * This software is released under The PostgreSQL License + * + *------------------------------------------------------------------------- + */ + +// Package flagutil provides helpers for reasoning about command line +// flags. Configuration in the Workbench is layered (built-in defaults, +// then the configuration file, then command line flags), which means a +// caller must be able to tell "the operator passed this flag" from "the +// flag is sitting at its registered default". Comparing a flag's value +// against its default cannot make that distinction, because an operator +// may legitimately pass a value that happens to equal the default. +package flagutil + +import "flag" + +// Set records the names of the flags that were explicitly present on +// the command line. The zero value is usable: a nil Set simply reports +// that no flag was passed. +type Set map[string]bool + +// Passed returns the Set of flags that fs saw on the command line. +// Flags left at their registered defaults are absent from the result, +// so callers can apply an override only when the operator asked for +// it. A nil FlagSet yields an empty Set rather than a panic, which +// keeps callers free of nil checks. +func Passed(fs *flag.FlagSet) Set { + passed := make(Set) + if fs == nil { + return passed + } + fs.Visit(func(f *flag.Flag) { + passed[f.Name] = true + }) + return passed +} + +// Has reports whether the named flag was explicitly passed on the +// command line. +func (s Set) Has(name string) bool { + return s[name] +} diff --git a/pkg/flagutil/flagutil_test.go b/pkg/flagutil/flagutil_test.go new file mode 100644 index 00000000..d282149f --- /dev/null +++ b/pkg/flagutil/flagutil_test.go @@ -0,0 +1,95 @@ +/*------------------------------------------------------------------------- + * + * pgEdge AI DBA Workbench + * + * Copyright (c) 2025 - 2026, pgEdge, Inc. + * This software is released under The PostgreSQL License + * + *------------------------------------------------------------------------- + */ +package flagutil + +import ( + "flag" + "io" + "testing" +) + +// newTestFlagSet builds a flag set with a string and an int flag whose +// defaults mirror the collector's, so tests can pass a value that +// coincides with a default. +func newTestFlagSet() (*flag.FlagSet, *string, *int) { + fs := flag.NewFlagSet("test", flag.ContinueOnError) + fs.SetOutput(io.Discard) + sslMode := fs.String("pg-sslmode", "prefer", "SSL mode") + port := fs.Int("pg-port", 5432, "port") + return fs, sslMode, port +} + +func TestPassed_NoFlags(t *testing.T) { + fs, _, _ := newTestFlagSet() + if err := fs.Parse(nil); err != nil { + t.Fatalf("Parse: %v", err) + } + + passed := Passed(fs) + if len(passed) != 0 { + t.Errorf("Passed() = %v, want an empty set", passed) + } + if passed.Has("pg-port") { + t.Error("Has(pg-port) = true, want false when nothing was passed") + } +} + +// TestPassed_ExplicitDefaultValue is the case that motivates the +// package: a flag passed with the value that is also its registered +// default must still be reported as passed. +func TestPassed_ExplicitDefaultValue(t *testing.T) { + fs, sslMode, port := newTestFlagSet() + if err := fs.Parse([]string{"-pg-port", "5432", "-pg-sslmode", "prefer"}); err != nil { + t.Fatalf("Parse: %v", err) + } + + passed := Passed(fs) + if !passed.Has("pg-port") { + t.Error("Has(pg-port) = false, want true for an explicitly passed flag") + } + if !passed.Has("pg-sslmode") { + t.Error("Has(pg-sslmode) = false, want true for an explicitly passed flag") + } + if *port != 5432 || *sslMode != "prefer" { + t.Errorf("parsed values changed unexpectedly: port=%d sslmode=%q", *port, *sslMode) + } +} + +func TestPassed_SubsetOfFlags(t *testing.T) { + fs, _, _ := newTestFlagSet() + if err := fs.Parse([]string{"-pg-port", "6000"}); err != nil { + t.Fatalf("Parse: %v", err) + } + + passed := Passed(fs) + if !passed.Has("pg-port") { + t.Error("Has(pg-port) = false, want true") + } + if passed.Has("pg-sslmode") { + t.Error("Has(pg-sslmode) = true, want false for an unpassed flag") + } +} + +func TestPassed_NilFlagSet(t *testing.T) { + passed := Passed(nil) + if passed == nil { + t.Fatal("Passed(nil) = nil, want an empty set") + } + if passed.Has("anything") { + t.Error("Has() on an empty set = true, want false") + } +} + +func TestSet_HasOnNilSet(t *testing.T) { + var s Set + if s.Has("pg-port") { + t.Error("Has() on a nil Set = true, want false") + } +} From 2ff60c0a186508a92e511dad6dec1f84530de17e Mon Sep 17 00:00:00 2001 From: Dave Page Date: Wed, 12 Aug 2026 15:02:23 +0100 Subject: [PATCH 2/2] fix(alerter): clear the configured password file on an explicit empty flag An explicitly empty -db-password-file skipped the immediate read but left the path the configuration file named in place, so the later cfg.LoadPassword during startup and on every SIGHUP reload still read that file. The flag now always replaces the configured path, including with an empty value, whilst only a non-empty path is read. --- .../cmd/ai-dba-alerter/flagoverrides_test.go | 50 +++++++++++++++++-- alerter/src/cmd/ai-dba-alerter/main.go | 21 +++++--- 2 files changed, 58 insertions(+), 13 deletions(-) diff --git a/alerter/src/cmd/ai-dba-alerter/flagoverrides_test.go b/alerter/src/cmd/ai-dba-alerter/flagoverrides_test.go index f823693d..679deb95 100644 --- a/alerter/src/cmd/ai-dba-alerter/flagoverrides_test.go +++ b/alerter/src/cmd/ai-dba-alerter/flagoverrides_test.go @@ -10,6 +10,8 @@ package main import ( + "os" + "path/filepath" "testing" "github.com/pgedge/ai-workbench/alerter/internal/config" @@ -77,11 +79,13 @@ func TestApplyFlagOverrides_ConfigMatchingFlagDefaultSurvives(t *testing.T) { } // TestApplyFlagOverrides_ExplicitEmptyPasswordFile verifies that an -// explicitly empty -db-password-file is treated as "no password -// file" rather than as a path to read, which would fail. +// explicitly empty -db-password-file clears the path the +// configuration file named, rather than being read as a path (which +// would fail) or silently leaving the configured file in place for +// cfg.LoadPassword to read later. func TestApplyFlagOverrides_ExplicitEmptyPasswordFile(t *testing.T) { cfg := config.NewConfig() - cfg.Datastore.Password = "from-config" + cfg.Datastore.PasswordFile = "/from/config/password" if err := applyFlagOverrides(cfg, flagOverrides{ DBPasswordFile: "", @@ -89,7 +93,43 @@ func TestApplyFlagOverrides_ExplicitEmptyPasswordFile(t *testing.T) { }); err != nil { t.Fatalf("applyFlagOverrides: %v", err) } - if cfg.Datastore.Password != "from-config" { - t.Errorf("Password = %q, want it left alone", cfg.Datastore.Password) + if cfg.Datastore.PasswordFile != "" { + t.Errorf("PasswordFile = %q, want it cleared by the explicit empty flag", + cfg.Datastore.PasswordFile) + } + + // The configured file must not be read on a later LoadPassword + // either; it does not exist, so a read attempt would error. + if err := cfg.LoadPassword(); err != nil { + t.Errorf("LoadPassword after clearing the path: %v", err) + } + if cfg.Datastore.Password != "" { + t.Errorf("Password = %q, want it left empty", cfg.Datastore.Password) + } +} + +// TestApplyFlagOverrides_PasswordFileReplacesConfigured verifies that +// a non-empty -db-password-file both loads the password and replaces +// the path the configuration file named. +func TestApplyFlagOverrides_PasswordFileReplacesConfigured(t *testing.T) { + pwFile := filepath.Join(t.TempDir(), "pw.txt") + if err := os.WriteFile(pwFile, []byte("flag-password\n"), 0600); err != nil { + t.Fatalf("write: %v", err) + } + + cfg := config.NewConfig() + cfg.Datastore.PasswordFile = "/from/config/password" + + if err := applyFlagOverrides(cfg, flagOverrides{ + DBPasswordFile: pwFile, + Passed: flagutil.Set{flagDBPasswordFile: true}, + }); err != nil { + t.Fatalf("applyFlagOverrides: %v", err) + } + if cfg.Datastore.PasswordFile != pwFile { + t.Errorf("PasswordFile = %q, want %q", cfg.Datastore.PasswordFile, pwFile) + } + if cfg.Datastore.Password != "flag-password" { + t.Errorf("Password = %q, want flag-password", cfg.Datastore.Password) } } diff --git a/alerter/src/cmd/ai-dba-alerter/main.go b/alerter/src/cmd/ai-dba-alerter/main.go index adbf646f..4a227357 100644 --- a/alerter/src/cmd/ai-dba-alerter/main.go +++ b/alerter/src/cmd/ai-dba-alerter/main.go @@ -346,15 +346,20 @@ func applyFlagOverrides(cfg *config.Config, o flagOverrides) error { if o.Passed.Has(flagDBUser) { cfg.Datastore.Username = o.DBUser } - // An explicitly empty -db-password-file means "no password file", - // so there is nothing to read; only a non-empty path triggers a - // read, which would otherwise fail on the empty path. - if o.Passed.Has(flagDBPasswordFile) && o.DBPasswordFile != "" { - password, err := fileutil.ReadSecretFile(o.DBPasswordFile) - if err != nil { - return fmt.Errorf("failed to read password file: %w", err) + // The flag always replaces whatever the configuration file named, + // including with an explicit empty value meaning "no password + // file"; otherwise cfg.LoadPassword would still read the + // configured file later. Only a non-empty path is read here, + // because reading the empty path would fail. + if o.Passed.Has(flagDBPasswordFile) { + cfg.Datastore.PasswordFile = o.DBPasswordFile + if o.DBPasswordFile != "" { + password, err := fileutil.ReadSecretFile(o.DBPasswordFile) + if err != nil { + return fmt.Errorf("failed to read password file: %w", err) + } + cfg.Datastore.Password = password } - cfg.Datastore.Password = password } if o.Passed.Has(flagDBSSLMode) { cfg.Datastore.SSLMode = o.DBSSLMode