From 2769fa96d8e16a7615b456827f14475cf954180e Mon Sep 17 00:00:00 2001 From: tamirms Date: Thu, 17 Jun 2021 11:42:29 +0100 Subject: [PATCH 1/2] Fix bug configuration via environment variables for optional parameters --- services/horizon/CHANGELOG.md | 2 +- support/config/config_option.go | 23 ++++++++++-- support/config/config_option_test.go | 54 ++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 3 deletions(-) diff --git a/services/horizon/CHANGELOG.md b/services/horizon/CHANGELOG.md index 4e5ad55536..dca17bdd18 100644 --- a/services/horizon/CHANGELOG.md +++ b/services/horizon/CHANGELOG.md @@ -20,7 +20,7 @@ file. This project adheres to [Semantic Versioning](http://semver.org/). * Fix bug in `horizon db reingest range` command, which would throw a duplicate entry conflict error from the DB. ([3661](https://github.com/stellar/go/pull/3661)). * Fix bug in DB metrics preventing Horizon from starting when read-only replica middleware is enabled. ([3668](https://github.com/stellar/go/pull/3668)). * Fix bug in the value of `route` in the logs for rate-limited requests (previously it was set to `undefined`). ([3658](https://github.com/stellar/go/pull/3658)). - +* Fix bug where the configuration for `CAPTIVE_CORE_LOG_PATH`, `CAPTIVE_CORE_PEER_PORT`, and `CAPTIVE_CORE_HTTP_PORT` were ignored if they were configured via environment variables instead of command line arguments. ([3702](https://github.com/stellar/go/pull/3702)). ## v2.4.0 diff --git a/support/config/config_option.go b/support/config/config_option.go index 7a8300966d..90b2480056 100644 --- a/support/config/config_option.go +++ b/support/config/config_option.go @@ -5,6 +5,8 @@ import ( "go/types" stdLog "log" "net/url" + "os" + "strings" "time" "github.com/spf13/cobra" @@ -166,7 +168,7 @@ func SetURL(co *ConfigOption) { // value indicates the flag was not explicitly set func SetOptionalUint(co *ConfigOption) { key := co.ConfigKey.(**uint) - if co.flag.Changed { + if isExplicitlySet(co) { *key = new(uint) **key = uint(viper.GetInt(co.Name)) } else { @@ -174,11 +176,28 @@ func SetOptionalUint(co *ConfigOption) { } } +func parseEnvVars(entries []string) map[string]bool { + set := map[string]bool{} + for _, entry := range entries { + key := strings.Split(entry, "=")[0] + set[key] = true + } + return set +} + +var envVars = parseEnvVars(os.Environ()) + +func isExplicitlySet(co *ConfigOption) bool { + // co.flag.Changed is only set to true when the configuration is set via command line parameter. + // In the case where a variable is configured via environment variable we need to check envVars. + return co.flag.Changed || envVars[co.EnvVar] +} + // SetOptionalString converts a command line uint to a *string where the nil // value indicates the flag was not explicitly set func SetOptionalString(co *ConfigOption) { key := co.ConfigKey.(**string) - if co.flag.Changed { + if isExplicitlySet(co) { *key = new(string) **key = viper.GetString(co.Name) } else { diff --git a/support/config/config_option_test.go b/support/config/config_option_test.go index a19e718907..5a82f6570c 100644 --- a/support/config/config_option_test.go +++ b/support/config/config_option_test.go @@ -6,6 +6,7 @@ import ( "testing" "github.com/spf13/cobra" + "github.com/spf13/viper" "github.com/stretchr/testify/assert" ) @@ -71,6 +72,59 @@ func TestConfigOption_optionalFlags_set(t *testing.T) { assert.Equal(t, uint(6), *optUint) } +// Test that optional flags are set to non nil values when they are configured explicitly. +func TestConfigOption_optionalFlags_env_set_empty(t *testing.T) { + var optUint *uint + var optString *string + configOpts := ConfigOptions{ + {Name: "uint", OptType: types.Uint, ConfigKey: &optUint, CustomSetValue: SetOptionalUint, FlagDefault: uint(0)}, + {Name: "string", OptType: types.String, ConfigKey: &optString, CustomSetValue: SetOptionalString}, + } + cmd := &cobra.Command{ + Use: "doathing", + Run: func(_ *cobra.Command, _ []string) { + configOpts.Require() + configOpts.SetValues() + }, + } + configOpts.Init(cmd) + + envVars = map[string]bool{ + "STRING": true, + } + cmd.Execute() + assert.Equal(t, "", *optString) + assert.Equal(t, (*uint)(nil), optUint) +} + +// Test that optional flags are set to non nil values when they are configured explicitly. +func TestConfigOption_optionalFlags_env_set(t *testing.T) { + var optUint *uint + var optString *string + configOpts := ConfigOptions{ + {Name: "uint", OptType: types.Uint, ConfigKey: &optUint, CustomSetValue: SetOptionalUint, FlagDefault: uint(0)}, + {Name: "string", OptType: types.String, ConfigKey: &optString, CustomSetValue: SetOptionalString}, + } + cmd := &cobra.Command{ + Use: "doathing", + Run: func(_ *cobra.Command, _ []string) { + configOpts.Require() + configOpts.SetValues() + }, + } + configOpts.Init(cmd) + + envVars = map[string]bool{ + "STRING": true, + "UINT": true, + } + viper.Set("STRING", "str") + viper.Set("UINT", 6) + cmd.Execute() + assert.Equal(t, "str", *optString) + assert.Equal(t, uint(6), *optUint) +} + // Test that when there are no args the defaults in the config options are // used. func TestConfigOption_getSimpleValue_defaults(t *testing.T) { From 3b9bccb263ff90ad63ed1a0f41f89ea79244f90e Mon Sep 17 00:00:00 2001 From: tamirms Date: Thu, 17 Jun 2021 12:50:21 +0100 Subject: [PATCH 2/2] Fix tests --- support/config/config_option_test.go | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) diff --git a/support/config/config_option_test.go b/support/config/config_option_test.go index 5a82f6570c..30be01424b 100644 --- a/support/config/config_option_test.go +++ b/support/config/config_option_test.go @@ -6,7 +6,6 @@ import ( "testing" "github.com/spf13/cobra" - "github.com/spf13/viper" "github.com/stretchr/testify/assert" ) @@ -89,9 +88,14 @@ func TestConfigOption_optionalFlags_env_set_empty(t *testing.T) { } configOpts.Init(cmd) + prev := envVars envVars = map[string]bool{ "STRING": true, } + defer func() { + envVars = prev + }() + cmd.Execute() assert.Equal(t, "", *optString) assert.Equal(t, (*uint)(nil), optUint) @@ -114,12 +118,20 @@ func TestConfigOption_optionalFlags_env_set(t *testing.T) { } configOpts.Init(cmd) + prev := envVars envVars = map[string]bool{ "STRING": true, "UINT": true, } - viper.Set("STRING", "str") - viper.Set("UINT", 6) + defer func() { + envVars = prev + }() + + defer os.Setenv("STRING", os.Getenv("STRING")) + defer os.Setenv("UINT", os.Getenv("UINT")) + os.Setenv("STRING", "str") + os.Setenv("UINT", "6") + cmd.Execute() assert.Equal(t, "str", *optString) assert.Equal(t, uint(6), *optUint)