From bbf1ca7f0b65a0670a01fddd7329caa19b192762 Mon Sep 17 00:00:00 2001 From: Josh Siviour Date: Mon, 3 Aug 2026 00:03:28 +1000 Subject: [PATCH] fix: stop viper AutomaticEnv from shadowing predastore env vars (#659) * fix: stop viper AutomaticEnv from shadowing predastore's own env vars Viper resolves AutomaticEnv before explicit BindEnv, so predastore's bare "config-path", "host" and "base-path" viper keys auto-derived to SPINIFEX_CONFIG_PATH, SPINIFEX_HOST and SPINIFEX_BASE_PATH - coincidentally the same names used elsewhere (the global cluster config path, and root's global --host override). Setting SPINIFEX_CONFIG_PATH for the cluster config (needed by derivePredastoreBind) silently made predastore parse the cluster config as its own S3 config, breaking auth for every signed request. Rename predastore's host/base-path/config-path viper keys to "predastore-"-prefixed keys whose AutomaticEnv-derived name is identical to their explicit BindEnv target, so no shadow is possible regardless of what else is set in the environment. * style: trim predastore env-binding comments to the 3-line limit --- cmd/spinifex/cmd/predastore_bind_test.go | 23 +++++++++++ cmd/spinifex/cmd/service.go | 49 +++++++++++++++--------- 2 files changed, 54 insertions(+), 18 deletions(-) diff --git a/cmd/spinifex/cmd/predastore_bind_test.go b/cmd/spinifex/cmd/predastore_bind_test.go index 88d3493d2..cc010eb21 100644 --- a/cmd/spinifex/cmd/predastore_bind_test.go +++ b/cmd/spinifex/cmd/predastore_bind_test.go @@ -3,6 +3,7 @@ package cmd import ( "os" "path/filepath" + "strings" "testing" "github.com/mulgadc/spinifex/spinifex/config" @@ -136,3 +137,25 @@ func TestDerivePredastoreBind_MissingHostErrors(t *testing.T) { _, err := derivePredastoreBind(clusterConfig) require.Error(t, err) } + +// A bare "config-path" key derives the same AutomaticEnv name as the +// unrelated cluster-config SPINIFEX_CONFIG_PATH, which made predastore load +// the cluster config as its own S3 config. +func TestPredastoreConfigPathNotShadowedByClusterConfigPathEnv(t *testing.T) { + t.Cleanup(func() { viper.Reset() }) + viper.Reset() + + // Mirrors service.go's init() against the freshly-Reset instance. pflags + // survive Reset: they live on predastoreCmd, a package-level singleton. + viper.SetEnvPrefix("SPINIFEX") + viper.SetEnvKeyReplacer(strings.NewReplacer("-", "_")) + viper.AutomaticEnv() + bindPredastoreNamespacedEnv() + + t.Setenv("SPINIFEX_CONFIG_PATH", "/etc/spinifex/spinifex.toml") + t.Setenv("SPINIFEX_PREDASTORE_CONFIG_PATH", "/etc/spinifex/predastore/predastore.toml") + + got := viper.GetString("predastore-config-path") + assert.Equal(t, "/etc/spinifex/predastore/predastore.toml", got, + "predastore-config-path must resolve to the predastore config even when SPINIFEX_CONFIG_PATH is also set") +} diff --git a/cmd/spinifex/cmd/service.go b/cmd/spinifex/cmd/service.go index 5421828c6..366566b0b 100644 --- a/cmd/spinifex/cmd/service.go +++ b/cmd/spinifex/cmd/service.go @@ -141,8 +141,8 @@ var predastoreStartCmd = &cobra.Command{ // Get the port from the flags port := viper.GetInt("port") - host := viper.GetString("host") - basePath := viper.GetString("base-path") + host := viper.GetString("predastore-host") + basePath := viper.GetString("predastore-base-path") debug := viper.GetBool("debug") nodeID := viper.GetInt("node-id") @@ -160,7 +160,7 @@ var predastoreStartCmd = &cobra.Command{ fmt.Println("Error deriving predastore bind config:", err) return } - if !viper.IsSet("host") { + if !viper.IsSet("predastore-host") { host = bind.Host } if !viper.IsSet("port") { @@ -177,7 +177,7 @@ var predastoreStartCmd = &cobra.Command{ return } - configPath := viper.GetString("config-path") + configPath := viper.GetString("predastore-config-path") if configPath == "" { fmt.Println("Config path is not set") @@ -1157,6 +1157,30 @@ var qmpCollectorStatusCmd = &cobra.Command{ }, } +// registerPredastoreNamespacedFlags defines predastore's host, base-path and +// config-path flags and binds them to viper. It must only run once (from +// init()); pflag panics on a redefined flag. +func registerPredastoreNamespacedFlags() { + predastoreCmd.PersistentFlags().String("host", "0.0.0.0", "Predastore (S3) host") + predastoreCmd.PersistentFlags().String("base-path", "", "Predastore (S3) base path") + predastoreCmd.PersistentFlags().String("config-path", "", "Predastore (S3) config path") + bindPredastoreNamespacedEnv() +} + +// bindPredastoreNamespacedEnv uses "predastore-"-prefixed viper keys so each +// key's AutomaticEnv-derived name matches its explicit BindEnv target. Viper +// checks AutomaticEnv first, so a bare key resolves SPINIFEX_HOST instead. +func bindPredastoreNamespacedEnv() { + viper.BindEnv("predastore-host", "SPINIFEX_PREDASTORE_HOST") + viper.BindPFlag("predastore-host", predastoreCmd.PersistentFlags().Lookup("host")) + + viper.BindEnv("predastore-base-path", "SPINIFEX_PREDASTORE_BASE_PATH") + viper.BindPFlag("predastore-base-path", predastoreCmd.PersistentFlags().Lookup("base-path")) + + viper.BindEnv("predastore-config-path", "SPINIFEX_PREDASTORE_CONFIG_PATH") + viper.BindPFlag("predastore-config-path", predastoreCmd.PersistentFlags().Lookup("config-path")) +} + func init() { viper.SetEnvPrefix("SPINIFEX") // Prefix for environment variables viper.SetEnvKeyReplacer(strings.NewReplacer("-", "_")) @@ -1172,20 +1196,9 @@ func init() { viper.BindEnv("port", "SPINIFEX_PREDASTORE_PORT") viper.BindPFlag("port", predastoreCmd.PersistentFlags().Lookup("port")) - // Predastore Host - predastoreCmd.PersistentFlags().String("host", "0.0.0.0", "Predastore (S3) host") - viper.BindEnv("host", "SPINIFEX_PREDASTORE_HOST") - viper.BindPFlag("host", predastoreCmd.PersistentFlags().Lookup("host")) - - // Base path - predastoreCmd.PersistentFlags().String("base-path", "", "Predastore (S3) base path") - viper.BindEnv("base-path", "SPINIFEX_PREDASTORE_BASE_PATH") - viper.BindPFlag("base-path", predastoreCmd.PersistentFlags().Lookup("base-path")) - - // Predastore Config Path - predastoreCmd.PersistentFlags().String("config-path", "", "Predastore (S3) config path") - viper.BindEnv("config-path", "SPINIFEX_PREDASTORE_CONFIG_PATH") - viper.BindPFlag("config-path", predastoreCmd.PersistentFlags().Lookup("config-path")) + // Predastore host, base-path, config-path (namespaced viper keys — + // see registerPredastoreNamespacedFlags doc comment) + registerPredastoreNamespacedFlags() // Predastore Debug predastoreCmd.PersistentFlags().Bool("debug", false, "Predastore (S3) debug")