diff --git a/cmd/spinifex/cmd/service.go b/cmd/spinifex/cmd/service.go index 366566b0b..d04f38d2d 100644 --- a/cmd/spinifex/cmd/service.go +++ b/cmd/spinifex/cmd/service.go @@ -140,11 +140,11 @@ var predastoreStartCmd = &cobra.Command{ fmt.Println("Starting predastore service...") // Get the port from the flags - port := viper.GetInt("port") + port := viper.GetInt("predastore-port") host := viper.GetString("predastore-host") basePath := viper.GetString("predastore-base-path") - debug := viper.GetBool("debug") - nodeID := viper.GetInt("node-id") + debug := viper.GetBool("predastore-debug") + nodeID := viper.GetInt("predastore-node-id") // Derive bind host/port/node-id from spinifex.toml when its path is // known and the caller hasn't explicitly overridden them — replaces @@ -163,10 +163,10 @@ var predastoreStartCmd = &cobra.Command{ if !viper.IsSet("predastore-host") { host = bind.Host } - if !viper.IsSet("port") { + if !viper.IsSet("predastore-port") { port = bind.Port } - if !viper.IsSet("node-id") { + if !viper.IsSet("predastore-node-id") { nodeID = bind.NodeID } } @@ -184,21 +184,21 @@ var predastoreStartCmd = &cobra.Command{ return } - tlsCert := viper.GetString("tls-cert") + tlsCert := viper.GetString("predastore-tls-cert") if tlsCert == "" { fmt.Println("TLS cert is not set") return } - tlsKey := viper.GetString("tls-key") + tlsKey := viper.GetString("predastore-tls-key") if tlsKey == "" { fmt.Println("TLS key is not set") return } - encryptionKeyFile := viper.GetString("encryption-key-file") + encryptionKeyFile := viper.GetString("predastore-encryption-key-file") if encryptionKeyFile == "" { fmt.Println("Encryption key file is not set") @@ -453,9 +453,9 @@ var natsStartCmd = &cobra.Command{ Run: func(cmd *cobra.Command, args []string) { fmt.Println("Starting nats service...") - port := viper.GetInt("port") - host := viper.GetString("host") - debug := viper.GetBool("debug") + port := viper.GetInt("nats-port") + host := viper.GetString("nats-service-host") + debug := viper.GetBool("nats-debug") dataDir := viper.GetString("data-dir") jetStream := viper.GetBool("jetstream") @@ -634,19 +634,19 @@ var awsgwStartCmd = &cobra.Command{ nodeConfig := clusterConfig.Nodes[clusterConfig.Node] // Overwrite defaults (CLI first, config second, env third) - awsgwHost := viper.GetString("host") + awsgwHost := viper.GetString("awsgw-host") if awsgwHost != "" { fmt.Println("Overwriting awsgw host to:", awsgwHost) nodeConfig.AWSGW.Host = awsgwHost } - awsgwTlsCert := viper.GetString("tls-cert") + awsgwTlsCert := viper.GetString("awsgw-tls-cert") if awsgwTlsCert != "" { fmt.Println("Overwriting awsgw tls-cert to:", awsgwTlsCert) nodeConfig.AWSGW.TLSCert = awsgwTlsCert } - awsgwTlsKey := viper.GetString("tls-key") + awsgwTlsKey := viper.GetString("awsgw-tls-key") if awsgwTlsKey != "" { fmt.Println("Overwriting awsgw tls-key to:", awsgwTlsKey) @@ -663,7 +663,7 @@ var awsgwStartCmd = &cobra.Command{ // Apply changes back to cluster config clusterConfig.Nodes[clusterConfig.Node] = nodeConfig - defer initTelemetry("awsgw", viper.GetBool("debug"))() + defer initTelemetry("awsgw", viper.GetBool("awsgw-debug"))() awsgw.SetBuildInfo(Version, Commit) service, err := service.New("awsgw", clusterConfig) @@ -1181,6 +1181,77 @@ func bindPredastoreNamespacedEnv() { viper.BindPFlag("predastore-config-path", predastoreCmd.PersistentFlags().Lookup("config-path")) } +// bindPredastoreCollisionEnv namespaces predastore's port, debug, tls-cert, +// tls-key, encryption-key-file and node-id keys, which nats and awsgw also +// bind bare. Each derived env name now matches its own BindEnv target. +func bindPredastoreCollisionEnv() { + viper.BindEnv("predastore-port", "SPINIFEX_PREDASTORE_PORT") + viper.BindPFlag("predastore-port", predastoreCmd.PersistentFlags().Lookup("port")) + + viper.BindEnv("predastore-debug", "SPINIFEX_PREDASTORE_DEBUG") + viper.BindPFlag("predastore-debug", predastoreCmd.PersistentFlags().Lookup("debug")) + + viper.BindEnv("predastore-tls-cert", "SPINIFEX_PREDASTORE_TLS_CERT") + viper.BindPFlag("predastore-tls-cert", predastoreCmd.PersistentFlags().Lookup("tls-cert")) + + viper.BindEnv("predastore-tls-key", "SPINIFEX_PREDASTORE_TLS_KEY") + viper.BindPFlag("predastore-tls-key", predastoreCmd.PersistentFlags().Lookup("tls-key")) + + viper.BindEnv("predastore-encryption-key-file", "SPINIFEX_PREDASTORE_ENCRYPTION_KEY_FILE") + viper.BindPFlag("predastore-encryption-key-file", predastoreCmd.PersistentFlags().Lookup("encryption-key-file")) + + viper.BindEnv("predastore-node-id", "SPINIFEX_PREDASTORE_NODE_ID") + viper.BindPFlag("predastore-node-id", predastoreCmd.PersistentFlags().Lookup("node-id")) +} + +// bindNatsCollisionEnv namespaces nats's port, host and debug keys, which +// awsgw also binds bare. The host key is "nats-service-host": rootCmd already +// owns "nats-host" for its cluster-wide override, so reusing it would clobber. +func bindNatsCollisionEnv() { + viper.BindEnv("nats-port", "SPINIFEX_NATS_PORT") + viper.BindPFlag("nats-port", natsCmd.PersistentFlags().Lookup("port")) + + viper.BindEnv("nats-service-host", "SPINIFEX_NATS_HOST") + viper.BindPFlag("nats-service-host", natsCmd.PersistentFlags().Lookup("host")) + + viper.BindEnv("nats-debug", "SPINIFEX_NATS_DEBUG") + viper.BindPFlag("nats-debug", natsCmd.PersistentFlags().Lookup("debug")) +} + +// bindAwsgwCollisionEnv namespaces awsgw's host, tls-cert, tls-key and debug +// viper keys, which predastore and/or nats also bind bare. Each +// AutomaticEnv-derived name now matches its own explicit BindEnv target. +func bindAwsgwCollisionEnv() { + viper.BindEnv("awsgw-host", "SPINIFEX_AWSGW_HOST") + viper.BindPFlag("awsgw-host", awsgwCmd.PersistentFlags().Lookup("host")) + + viper.BindEnv("awsgw-tls-cert", "SPINIFEX_AWSGW_TLS_CERT") + viper.BindPFlag("awsgw-tls-cert", awsgwCmd.PersistentFlags().Lookup("tls-cert")) + + viper.BindEnv("awsgw-tls-key", "SPINIFEX_AWSGW_TLS_KEY") + viper.BindPFlag("awsgw-tls-key", awsgwCmd.PersistentFlags().Lookup("tls-key")) + + viper.BindEnv("awsgw-debug", "SPINIFEX_AWSGW_DEBUG") + viper.BindPFlag("awsgw-debug", awsgwCmd.PersistentFlags().Lookup("debug")) +} + +// bindViperblockEnv binds viperblock's S3 and plugin flags. The lookups must +// target viperblockCmd, which declares them; a predastoreCmd lookup yields nil +// and BindPFlag drops it silently, hiding both the flag and its default. +func bindViperblockEnv() { + viper.BindEnv("s3-host", "SPINIFEX_VIPERBLOCK_S3_HOST") + viper.BindPFlag("s3-host", viperblockCmd.PersistentFlags().Lookup("s3-host")) + + viper.BindEnv("s3-bucket", "SPINIFEX_VIPERBLOCK_S3_BUCKET") + viper.BindPFlag("s3-bucket", viperblockCmd.PersistentFlags().Lookup("s3-bucket")) + + viper.BindEnv("s3-region", "SPINIFEX_VIPERBLOCK_S3_REGION") + viper.BindPFlag("s3-region", viperblockCmd.PersistentFlags().Lookup("s3-region")) + + viper.BindEnv("plugin-path", "SPINIFEX_VIPERBLOCK_PLUGIN_PATH") + viper.BindPFlag("plugin-path", viperblockCmd.PersistentFlags().Lookup("plugin-path")) +} + func init() { viper.SetEnvPrefix("SPINIFEX") // Prefix for environment variables viper.SetEnvKeyReplacer(strings.NewReplacer("-", "_")) @@ -1193,8 +1264,6 @@ func init() { // Predastore Port predastoreCmd.PersistentFlags().Int("port", 8443, "Predastore (S3) port") - viper.BindEnv("port", "SPINIFEX_PREDASTORE_PORT") - viper.BindPFlag("port", predastoreCmd.PersistentFlags().Lookup("port")) // Predastore host, base-path, config-path (namespaced viper keys — // see registerPredastoreNamespacedFlags doc comment) @@ -1202,30 +1271,24 @@ func init() { // Predastore Debug predastoreCmd.PersistentFlags().Bool("debug", false, "Predastore (S3) debug") - viper.BindEnv("debug", "SPINIFEX_PREDASTORE_DEBUG") - viper.BindPFlag("debug", predastoreCmd.PersistentFlags().Lookup("debug")) // Predastore TLS Cert predastoreCmd.PersistentFlags().String("tls-cert", "", "Predastore (S3) TLS certificate") - viper.BindEnv("tls-cert", "SPINIFEX_PREDASTORE_TLS_CERT") - viper.BindPFlag("tls-cert", predastoreCmd.PersistentFlags().Lookup("tls-cert")) // Predastore TLS Key predastoreCmd.PersistentFlags().String("tls-key", "", "Predastore (S3) TLS key") - viper.BindEnv("tls-key", "SPINIFEX_PREDASTORE_TLS_KEY") - viper.BindPFlag("tls-key", predastoreCmd.PersistentFlags().Lookup("tls-key")) // Predastore at-rest encryption master key (per node; mode 0600) predastoreCmd.PersistentFlags().String("encryption-key-file", "", "Path to this node's 32-byte AES-256 master key file (required)") - viper.BindEnv("encryption-key-file", "SPINIFEX_PREDASTORE_ENCRYPTION_KEY_FILE") - viper.BindPFlag("encryption-key-file", predastoreCmd.PersistentFlags().Lookup("encryption-key-file")) // Predastore Node ID. Default -1 is dev mode (launch every configured // QUIC node in-process). Production deployments set this to the node's // real ID (>= 1) via SPINIFEX_PREDASTORE_NODE_ID or --node-id. predastoreCmd.PersistentFlags().Int("node-id", -1, "Predastore (S3) node ID (-1 = dev mode, >= 1 = production)") - viper.BindEnv("node-id", "SPINIFEX_PREDASTORE_NODE_ID") - viper.BindPFlag("node-id", predastoreCmd.PersistentFlags().Lookup("node-id")) + + // Namespaced viper keys for port/debug/tls-cert/tls-key/encryption-key-file/node-id + // (see bindPredastoreCollisionEnv doc comment) + bindPredastoreCollisionEnv() // Predastore CPU Profiling predastoreCmd.PersistentFlags().Bool("pprof", false, "Enable CPU profiling (also via PPROF_ENABLED=1)") @@ -1244,20 +1307,10 @@ func init() { serviceCmd.AddCommand(viperblockCmd) viperblockCmd.PersistentFlags().String("s3-host", "", "Predastore (S3) host URI") - viper.BindEnv("s3-host", "SPINIFEX_VIPERBLOCK_S3_HOST") - viper.BindPFlag("s3-host", predastoreCmd.PersistentFlags().Lookup("s3-host")) - viperblockCmd.PersistentFlags().String("s3-bucket", "predastore", "Predastore (S3) bucket") - viper.BindEnv("s3-bucket", "SPINIFEX_VIPERBLOCK_S3_BUCKET") - viper.BindPFlag("s3-bucket", predastoreCmd.PersistentFlags().Lookup("s3-bucket")) - viperblockCmd.PersistentFlags().String("s3-region", "ap-southeast-2", "Predastore (S3) region") - viper.BindEnv("s3-region", "SPINIFEX_VIPERBLOCK_S3_REGION") - viper.BindPFlag("s3-region", predastoreCmd.PersistentFlags().Lookup("s3-region")) - viperblockCmd.PersistentFlags().String("plugin-path", "/opt/spinifex/lib/nbdkit-viperblock-plugin.so", "Pathname to the nbdkit viperblockplugin") - viper.BindEnv("plugin-path", "SPINIFEX_VIPERBLOCK_PLUGIN_PATH") - viper.BindPFlag("plugin-path", predastoreCmd.PersistentFlags().Lookup("plugin-path")) + bindViperblockEnv() // Viperblock at-rest encryption master key (shared with other on-node // services via group ownership; mode 0640 or stricter). Distinct viper @@ -1280,16 +1333,11 @@ func init() { // Add NATS flags natsCmd.PersistentFlags().Int("port", 4222, "NATS server port") - viper.BindEnv("port", "SPINIFEX_NATS_PORT") - viper.BindPFlag("port", natsCmd.PersistentFlags().Lookup("port")) - natsCmd.PersistentFlags().String("host", "0.0.0.0", "NATS server host") - viper.BindEnv("host", "SPINIFEX_NATS_HOST") - viper.BindPFlag("host", natsCmd.PersistentFlags().Lookup("host")) - natsCmd.PersistentFlags().Bool("debug", false, "Enable debug logging") - viper.BindEnv("debug", "SPINIFEX_NATS_DEBUG") - viper.BindPFlag("debug", natsCmd.PersistentFlags().Lookup("debug")) + + // Namespaced viper keys for port/host/debug (see bindNatsCollisionEnv doc comment) + bindNatsCollisionEnv() natsCmd.PersistentFlags().String("data-dir", "", "NATS data directory") viper.BindEnv("data-dir", "SPINIFEX_NATS_DATA_DIR") @@ -1314,22 +1362,17 @@ func init() { serviceCmd.AddCommand(awsgwCmd) awsgwCmd.PersistentFlags().String("host", "0.0.0.0:9999", "AWS Gateway server host") - viper.BindEnv("host", "SPINIFEX_AWSGW_HOST") - viper.BindPFlag("host", awsgwCmd.PersistentFlags().Lookup("host")) // AWS GW TLS Cert awsgwCmd.PersistentFlags().String("tls-cert", "", "AWS Gateway TLS certificate") - viper.BindEnv("tls-cert", "SPINIFEX_AWSGW_TLS_CERT") - viper.BindPFlag("tls-cert", awsgwCmd.PersistentFlags().Lookup("tls-cert")) // AWS GW TLS Key awsgwCmd.PersistentFlags().String("tls-key", "", "AWS Gateway TLS key") - viper.BindEnv("tls-key", "SPINIFEX_AWSGW_TLS_KEY") - viper.BindPFlag("tls-key", awsgwCmd.PersistentFlags().Lookup("tls-key")) awsgwCmd.PersistentFlags().Bool("debug", false, "AWS Gateway Debug") - viper.BindEnv("debug", "SPINIFEX_AWSGW_DEBUG") - viper.BindPFlag("debug", awsgwCmd.PersistentFlags().Lookup("debug")) + + // Namespaced viper keys for host/tls-cert/tls-key/debug (see bindAwsgwCollisionEnv doc comment) + bindAwsgwCollisionEnv() awsgwCmd.AddCommand(awsgwStartCmd) awsgwCmd.AddCommand(awsgwStopCmd) diff --git a/cmd/spinifex/cmd/service_bind_collision_test.go b/cmd/spinifex/cmd/service_bind_collision_test.go new file mode 100644 index 000000000..ffb1eae74 --- /dev/null +++ b/cmd/spinifex/cmd/service_bind_collision_test.go @@ -0,0 +1,156 @@ +package cmd + +import ( + "strings" + "testing" + + "github.com/spf13/viper" + "github.com/stretchr/testify/assert" +) + +// bindAllServiceCollisionEnv mirrors init()'s registration of the +// predastore/nats/awsgw keys against a freshly Reset viper. pflags survive +// Reset: they live on the package-level command singletons. +func bindAllServiceCollisionEnv(t *testing.T) { + t.Helper() + viper.SetEnvPrefix("SPINIFEX") + viper.SetEnvKeyReplacer(strings.NewReplacer("-", "_")) + viper.AutomaticEnv() + bindPredastoreCollisionEnv() + bindNatsCollisionEnv() + bindAwsgwCollisionEnv() +} + +// TestSubcommandDebugKeysResolveIndependently proves predastore, nats and +// awsgw each resolve their own SPINIFEX__DEBUG value instead of +// sharing one last-registration-wins "debug" viper key. +func TestSubcommandDebugKeysResolveIndependently(t *testing.T) { + t.Cleanup(func() { viper.Reset() }) + viper.Reset() + bindAllServiceCollisionEnv(t) + + t.Setenv("SPINIFEX_PREDASTORE_DEBUG", "true") + t.Setenv("SPINIFEX_NATS_DEBUG", "false") + t.Setenv("SPINIFEX_AWSGW_DEBUG", "true") + + assert.True(t, viper.GetBool("predastore-debug")) + assert.False(t, viper.GetBool("nats-debug")) + assert.True(t, viper.GetBool("awsgw-debug")) +} + +// TestSubcommandPortKeysResolveIndependently proves predastore and nats +// each resolve their own SPINIFEX__PORT value instead of sharing +// one last-registration-wins "port" viper key. +func TestSubcommandPortKeysResolveIndependently(t *testing.T) { + t.Cleanup(func() { viper.Reset() }) + viper.Reset() + bindAllServiceCollisionEnv(t) + + t.Setenv("SPINIFEX_PREDASTORE_PORT", "9001") + t.Setenv("SPINIFEX_NATS_PORT", "9002") + + assert.Equal(t, 9001, viper.GetInt("predastore-port")) + assert.Equal(t, 9002, viper.GetInt("nats-port")) +} + +// TestSubcommandHostKeysResolveIndependently proves nats and awsgw each +// resolve their own SPINIFEX__HOST value instead of sharing one +// last-registration-wins "host" viper key. +func TestSubcommandHostKeysResolveIndependently(t *testing.T) { + t.Cleanup(func() { viper.Reset() }) + viper.Reset() + bindAllServiceCollisionEnv(t) + + t.Setenv("SPINIFEX_NATS_HOST", "10.0.0.1") + t.Setenv("SPINIFEX_AWSGW_HOST", "10.0.0.2") + + assert.Equal(t, "10.0.0.1", viper.GetString("nats-service-host")) + assert.Equal(t, "10.0.0.2", viper.GetString("awsgw-host")) +} + +// TestSubcommandTLSKeysResolveIndependently proves predastore and awsgw +// each resolve their own SPINIFEX__TLS_CERT/KEY values instead of +// sharing one last-registration-wins "tls-cert"/"tls-key" viper key. +func TestSubcommandTLSKeysResolveIndependently(t *testing.T) { + t.Cleanup(func() { viper.Reset() }) + viper.Reset() + bindAllServiceCollisionEnv(t) + + t.Setenv("SPINIFEX_PREDASTORE_TLS_CERT", "/etc/predastore/cert.pem") + t.Setenv("SPINIFEX_PREDASTORE_TLS_KEY", "/etc/predastore/key.pem") + t.Setenv("SPINIFEX_AWSGW_TLS_CERT", "/etc/awsgw/cert.pem") + t.Setenv("SPINIFEX_AWSGW_TLS_KEY", "/etc/awsgw/key.pem") + + assert.Equal(t, "/etc/predastore/cert.pem", viper.GetString("predastore-tls-cert")) + assert.Equal(t, "/etc/predastore/key.pem", viper.GetString("predastore-tls-key")) + assert.Equal(t, "/etc/awsgw/cert.pem", viper.GetString("awsgw-tls-cert")) + assert.Equal(t, "/etc/awsgw/key.pem", viper.GetString("awsgw-tls-key")) +} + +// TestGenericSpinifexEnvDoesNotShadowNamespacedKeys proves a generic +// SPINIFEX_DEBUG/SPINIFEX_HOST/SPINIFEX_PORT cannot override a per-subcommand +// value, since no namespaced key derives a generic bare name. +func TestGenericSpinifexEnvDoesNotShadowNamespacedKeys(t *testing.T) { + t.Cleanup(func() { viper.Reset() }) + viper.Reset() + bindAllServiceCollisionEnv(t) + + t.Setenv("SPINIFEX_DEBUG", "true") + t.Setenv("SPINIFEX_HOST", "255.255.255.255") + t.Setenv("SPINIFEX_PORT", "1") + + t.Setenv("SPINIFEX_PREDASTORE_DEBUG", "false") + t.Setenv("SPINIFEX_NATS_DEBUG", "false") + t.Setenv("SPINIFEX_AWSGW_DEBUG", "false") + t.Setenv("SPINIFEX_NATS_HOST", "10.0.0.9") + t.Setenv("SPINIFEX_AWSGW_HOST", "10.0.0.10") + t.Setenv("SPINIFEX_PREDASTORE_PORT", "8443") + t.Setenv("SPINIFEX_NATS_PORT", "4222") + + assert.False(t, viper.GetBool("predastore-debug"), "generic SPINIFEX_DEBUG must not shadow predastore-debug") + assert.False(t, viper.GetBool("nats-debug"), "generic SPINIFEX_DEBUG must not shadow nats-debug") + assert.False(t, viper.GetBool("awsgw-debug"), "generic SPINIFEX_DEBUG must not shadow awsgw-debug") + assert.Equal(t, "10.0.0.9", viper.GetString("nats-service-host"), "generic SPINIFEX_HOST must not shadow nats-service-host") + assert.Equal(t, "10.0.0.10", viper.GetString("awsgw-host"), "generic SPINIFEX_HOST must not shadow awsgw-host") + assert.Equal(t, 8443, viper.GetInt("predastore-port"), "generic SPINIFEX_PORT must not shadow predastore-port") + assert.Equal(t, 4222, viper.GetInt("nats-port"), "generic SPINIFEX_PORT must not shadow nats-port") +} + +// TestViperblockFlagsLookUpTheirOwnCommand guards the receiver on the +// viperblock flag bindings. These are declared on viperblockCmd, so a +// predastoreCmd lookup returns nil and BindPFlag rejects it silently. +func TestViperblockFlagsLookUpTheirOwnCommand(t *testing.T) { + for _, name := range []string{"s3-host", "s3-bucket", "s3-region", "plugin-path"} { + assert.NotNil(t, viperblockCmd.PersistentFlags().Lookup(name), + "%s must be declared on viperblockCmd", name) + assert.Nil(t, predastoreCmd.PersistentFlags().Lookup(name), + "%s is not declared on predastoreCmd, so that lookup yields nil", name) + } +} + +// TestViperblockFlagDefaultsReachViper drives the real bindViperblockEnv, so +// a lookup pointed at the wrong command fails here: an unbound flag leaves +// its default invisible and s3-bucket resolves "" rather than "predastore". +func TestViperblockFlagDefaultsReachViper(t *testing.T) { + t.Cleanup(func() { viper.Reset() }) + viper.Reset() + bindViperblockEnv() + + assert.Equal(t, "predastore", viper.GetString("s3-bucket")) + assert.Equal(t, "ap-southeast-2", viper.GetString("s3-region")) + assert.Equal(t, "/opt/spinifex/lib/nbdkit-viperblock-plugin.so", viper.GetString("plugin-path")) +} + +// TestViperblockEnvBeatsUnchangedFlagDefault pins viper's precedence: wiring +// the defaults correctly must not override a deployment that already sets +// SPINIFEX_VIPERBLOCK_*. +func TestViperblockEnvBeatsUnchangedFlagDefault(t *testing.T) { + t.Cleanup(func() { viper.Reset() }) + viper.Reset() + bindViperblockEnv() + + t.Setenv("SPINIFEX_VIPERBLOCK_S3_BUCKET", "deployed-bucket") + + assert.Equal(t, "deployed-bucket", viper.GetString("s3-bucket"), + "env must win over an unchanged flag default") +}