From 69326d1fcd97a07d9065819ce32e9e258befdaa5 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 4 Nov 2022 11:50:18 +0200 Subject: [PATCH 1/7] fix: Disable viper auto-env to avoid assigning to parent structs The viper automatic env mapping and BindEnv were both creating mappings like `vip.BindEnv("telemetry", "CODER_TELEMETRY")` which we don't want since `DeploymentConfig.Telemetry` is a struct housing fields. For some reason, this was causing `DeploymentConfig.Telemetry.URL` to **not** be assigned its default value when `CODER_TELEMETRY=false` was set as an environment variable. Potentially we would want `"telemetry.enable"` to be mapped to `"CODER_TELEMETRY"` for simplicity. But that behavior is not changed by this commit. Arguably, we could remove `vip.SetEnvPrefix` and `vip.SetEnvKeyReplacer` as well since we're manually controlling all environment variable names via `formatEnv`. --- cli/deployment/config.go | 23 ++++++++++++++++++----- 1 file changed, 18 insertions(+), 5 deletions(-) diff --git a/cli/deployment/config.go b/cli/deployment/config.go index ab0c0645cea45..a77f687873e0f 100644 --- a/cli/deployment/config.go +++ b/cli/deployment/config.go @@ -370,7 +370,6 @@ func Config(flagset *pflag.FlagSet, vip *viper.Viper) (*codersdk.DeploymentConfi return nil, xerrors.Errorf("get global config from flag: %w", err) } vip.SetEnvPrefix("coder") - vip.AutomaticEnv() if flg != "" { vip.SetConfigFile(flg + "/server.yaml") @@ -393,21 +392,26 @@ func setConfig(prefix string, vip *viper.Viper, target interface{}) { typ = val.Type() } - // Manually bind to env to support CODER_$INDEX_$FIELD format for structured slices. - _ = vip.BindEnv(prefix, formatEnv(prefix)) - + // Ensure that we only bind env variables to proper fields, + // otherwise Viper will get confused if the parent struct is + // assigned a value. if strings.HasPrefix(typ.Name(), "DeploymentConfigField[") { value := val.FieldByName("Value").Interface() switch value.(type) { case string: + vip.MustBindEnv(prefix, formatEnv(prefix)) val.FieldByName("Value").SetString(vip.GetString(prefix)) case bool: + vip.MustBindEnv(prefix, formatEnv(prefix)) val.FieldByName("Value").SetBool(vip.GetBool(prefix)) case int: + vip.MustBindEnv(prefix, formatEnv(prefix)) val.FieldByName("Value").SetInt(int64(vip.GetInt(prefix))) case time.Duration: + vip.MustBindEnv(prefix, formatEnv(prefix)) val.FieldByName("Value").SetInt(int64(vip.GetDuration(prefix))) case []string: + vip.MustBindEnv(prefix, formatEnv(prefix)) // As of October 21st, 2022 we supported delimiting a string // with a comma, but Viper only supports with a space. This // is a small hack around it! @@ -422,6 +426,7 @@ func setConfig(prefix string, vip *viper.Viper, target interface{}) { } val.FieldByName("Value").Set(reflect.ValueOf(value)) case []codersdk.GitAuthConfig: + // Do not bind to CODER_GITAUTH, instead bind to CODER_GITAUTH_0_*, etc. values := readSliceFromViper[codersdk.GitAuthConfig](vip, prefix, value) val.FieldByName("Value").Set(reflect.ValueOf(values)) default: @@ -471,6 +476,11 @@ func readSliceFromViper[T any](vip *viper.Viper, key string, value any) []T { prop = fve.Tag.Get("yaml") } configKey := fmt.Sprintf("%s.%d.%s", key, entry, prop) + + // Ensure the env entry for this key is registered + // before checking value. + vip.MustBindEnv(configKey, formatEnv(configKey)) + value := vip.Get(configKey) if value == nil { continue @@ -502,7 +512,6 @@ func NewViper() *viper.Viper { dc := newConfig() vip := viper.New() vip.SetEnvPrefix("coder") - vip.AutomaticEnv() vip.SetEnvKeyReplacer(strings.NewReplacer("-", "_", ".", "_")) setViperDefaults("", vip, dc) @@ -566,6 +575,10 @@ func setFlags(prefix string, flagset *pflag.FlagSet, vip *viper.Viper, target in hidden := val.FieldByName("Hidden").Bool() value := val.FieldByName("Default").Interface() + // Allow currently set environment variables + // to override default values in help output. + vip.MustBindEnv(prefix, formatEnv(prefix)) + switch value.(type) { case string: _ = flagset.StringP(flg, shorthand, vip.GetString(prefix), usage) From 2f4ac3013ebc62dc123e79bb91afba2fbc498099 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 4 Nov 2022 12:58:55 +0200 Subject: [PATCH 2/7] Add test for wrong-env breakage --- cli/deployment/config_test.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/cli/deployment/config_test.go b/cli/deployment/config_test.go index d86c8e06a29b6..c57f59dc3fc0d 100644 --- a/cli/deployment/config_test.go +++ b/cli/deployment/config_test.go @@ -199,6 +199,16 @@ func TestConfig(t *testing.T) { Regex: "gitlab.com", }}, config.GitAuth.Value) }, + }, { + Name: "Wrong env must not break default values", + Env: map[string]string{ + "CODER_PROMETHEUS_ENABLE": "true", + "CODER_PROMETHEUS": "true", // Wrong env name, must not break prom addr. + }, + Valid: func(config *codersdk.DeploymentConfig) { + require.Equal(t, config.Prometheus.Enable.Value, true) + require.Equal(t, config.Prometheus.Address.Value, config.Prometheus.Address.Default) + }, }} { tc := tc t.Run(tc.Name, func(t *testing.T) { From 64089594b6e44f40a0239b64df801ea3f5dfa38f Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 4 Nov 2022 12:31:17 +0200 Subject: [PATCH 3/7] fix: Allow overriding env variable names, restore CODER_TELEMETRY --- cli/deployment/config.go | 24 +++++++++++++++++------- cli/deployment/config_test.go | 4 ++-- codersdk/deploymentconfig.go | 3 +++ site/src/api/typesGenerated.ts | 1 + 4 files changed, 23 insertions(+), 9 deletions(-) diff --git a/cli/deployment/config.go b/cli/deployment/config.go index a77f687873e0f..58706dddf1517 100644 --- a/cli/deployment/config.go +++ b/cli/deployment/config.go @@ -239,6 +239,7 @@ func newConfig() *codersdk.DeploymentConfig { Name: "Telemetry Enable", Usage: "Whether telemetry is enabled or not. Coder collects anonymized usage data to help improve our product.", Flag: "telemetry", + Env: "CODER_TELEMETRY", // Override default environment. Default: flag.Lookup("test.v") == nil, }, Trace: &codersdk.DeploymentConfigField[bool]{ @@ -396,22 +397,26 @@ func setConfig(prefix string, vip *viper.Viper, target interface{}) { // otherwise Viper will get confused if the parent struct is // assigned a value. if strings.HasPrefix(typ.Name(), "DeploymentConfigField[") { + env := val.FieldByName("Env").String() + if env == "" { + env = formatEnv(prefix) + } value := val.FieldByName("Value").Interface() switch value.(type) { case string: - vip.MustBindEnv(prefix, formatEnv(prefix)) + vip.MustBindEnv(prefix, env) val.FieldByName("Value").SetString(vip.GetString(prefix)) case bool: - vip.MustBindEnv(prefix, formatEnv(prefix)) + vip.MustBindEnv(prefix, env) val.FieldByName("Value").SetBool(vip.GetBool(prefix)) case int: - vip.MustBindEnv(prefix, formatEnv(prefix)) + vip.MustBindEnv(prefix, env) val.FieldByName("Value").SetInt(int64(vip.GetInt(prefix))) case time.Duration: - vip.MustBindEnv(prefix, formatEnv(prefix)) + vip.MustBindEnv(prefix, env) val.FieldByName("Value").SetInt(int64(vip.GetDuration(prefix))) case []string: - vip.MustBindEnv(prefix, formatEnv(prefix)) + vip.MustBindEnv(prefix, env) // As of October 21st, 2022 we supported delimiting a string // with a comma, but Viper only supports with a space. This // is a small hack around it! @@ -569,15 +574,20 @@ func setFlags(prefix string, flagset *pflag.FlagSet, vip *viper.Viper, target in if flg == "" { return } + + env := val.FieldByName("Env").String() + if env == "" { + env = formatEnv(prefix) + } usage := val.FieldByName("Usage").String() - usage = fmt.Sprintf("%s\n%s", usage, cliui.Styles.Placeholder.Render("Consumes $"+formatEnv(prefix))) + usage = fmt.Sprintf("%s\n%s", usage, cliui.Styles.Placeholder.Render("Consumes $"+env)) shorthand := val.FieldByName("Shorthand").String() hidden := val.FieldByName("Hidden").Bool() value := val.FieldByName("Default").Interface() // Allow currently set environment variables // to override default values in help output. - vip.MustBindEnv(prefix, formatEnv(prefix)) + vip.MustBindEnv(prefix, env) switch value.(type) { case string: diff --git a/cli/deployment/config_test.go b/cli/deployment/config_test.go index c57f59dc3fc0d..e6c6a5d75caa5 100644 --- a/cli/deployment/config_test.go +++ b/cli/deployment/config_test.go @@ -35,7 +35,7 @@ func TestConfig(t *testing.T) { "CODER_PROVISIONER_DAEMONS": "5", "CODER_SECURE_AUTH_COOKIE": "true", "CODER_SSH_KEYGEN_ALGORITHM": "potato", - "CODER_TELEMETRY": "false", + "CODER_TELEMETRY": "true", "CODER_TELEMETRY_TRACE": "false", "CODER_WILDCARD_ACCESS_URL": "something-wildcard.com", }, @@ -50,7 +50,7 @@ func TestConfig(t *testing.T) { require.Equal(t, config.ProvisionerDaemons.Value, 5) require.Equal(t, config.SecureAuthCookie.Value, true) require.Equal(t, config.SSHKeygenAlgorithm.Value, "potato") - require.Equal(t, config.Telemetry.Enable.Value, false) + require.Equal(t, config.Telemetry.Enable.Value, true) require.Equal(t, config.Telemetry.Trace.Value, false) require.Equal(t, config.WildcardAccessURL.Value, "something-wildcard.com") }, diff --git a/codersdk/deploymentconfig.go b/codersdk/deploymentconfig.go index a73385670575a..248da8ee23bda 100644 --- a/codersdk/deploymentconfig.go +++ b/codersdk/deploymentconfig.go @@ -131,6 +131,7 @@ type DeploymentConfigField[T Flaggable] struct { Name string `json:"name"` Usage string `json:"usage"` Flag string `json:"flag"` + Env string `json:"env"` Shorthand string `json:"shorthand"` Enterprise bool `json:"enterprise"` Hidden bool `json:"hidden"` @@ -146,6 +147,7 @@ func (f *DeploymentConfigField[T]) MarshalJSON() ([]byte, error) { Name string `json:"name"` Usage string `json:"usage"` Flag string `json:"flag"` + Env string `json:"env"` Shorthand string `json:"shorthand"` Enterprise bool `json:"enterprise"` Hidden bool `json:"hidden"` @@ -156,6 +158,7 @@ func (f *DeploymentConfigField[T]) MarshalJSON() ([]byte, error) { Name: f.Name, Usage: f.Usage, Flag: f.Flag, + Env: f.Env, Shorthand: f.Shorthand, Enterprise: f.Enterprise, Hidden: f.Hidden, diff --git a/site/src/api/typesGenerated.ts b/site/src/api/typesGenerated.ts index 764aab5c6e761..9f613ba4cf5d6 100644 --- a/site/src/api/typesGenerated.ts +++ b/site/src/api/typesGenerated.ts @@ -310,6 +310,7 @@ export interface DeploymentConfigField { readonly name: string readonly usage: string readonly flag: string + readonly env: string readonly shorthand: string readonly enterprise: boolean readonly hidden: boolean From 6aebb3636b9618607788a877a3f98a8198855f51 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 4 Nov 2022 13:27:57 +0200 Subject: [PATCH 4/7] Ensure Env field is populated --- cli/deployment/config.go | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/cli/deployment/config.go b/cli/deployment/config.go index 58706dddf1517..83f8e3b7129a4 100644 --- a/cli/deployment/config.go +++ b/cli/deployment/config.go @@ -385,6 +385,18 @@ func Config(flagset *pflag.FlagSet, vip *viper.Viper) (*codersdk.DeploymentConfi return dc, nil } +func setEnv(prefix string, vip *viper.Viper, val reflect.Value) { + env := val.FieldByName("Env").String() + if env == "" { + env = formatEnv(prefix) + } + // Ensure the Env field is always populated. + val.FieldByName("Env").SetString(env) + + // Ensure the auto-generated or manually named env is bound. + vip.MustBindEnv(prefix, env) +} + func setConfig(prefix string, vip *viper.Viper, target interface{}) { val := reflect.Indirect(reflect.ValueOf(target)) typ := val.Type() @@ -397,26 +409,22 @@ func setConfig(prefix string, vip *viper.Viper, target interface{}) { // otherwise Viper will get confused if the parent struct is // assigned a value. if strings.HasPrefix(typ.Name(), "DeploymentConfigField[") { - env := val.FieldByName("Env").String() - if env == "" { - env = formatEnv(prefix) - } value := val.FieldByName("Value").Interface() switch value.(type) { case string: - vip.MustBindEnv(prefix, env) + setEnv(prefix, vip, val) val.FieldByName("Value").SetString(vip.GetString(prefix)) case bool: - vip.MustBindEnv(prefix, env) + setEnv(prefix, vip, val) val.FieldByName("Value").SetBool(vip.GetBool(prefix)) case int: - vip.MustBindEnv(prefix, env) + setEnv(prefix, vip, val) val.FieldByName("Value").SetInt(int64(vip.GetInt(prefix))) case time.Duration: - vip.MustBindEnv(prefix, env) + setEnv(prefix, vip, val) val.FieldByName("Value").SetInt(int64(vip.GetDuration(prefix))) case []string: - vip.MustBindEnv(prefix, env) + setEnv(prefix, vip, val) // As of October 21st, 2022 we supported delimiting a string // with a comma, but Viper only supports with a space. This // is a small hack around it! From 7cb8a98f0dcb1c7547476a19b3f8cc9d738a4d3c Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 4 Nov 2022 13:30:56 +0200 Subject: [PATCH 5/7] Add test for env field population --- cli/deployment/config_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/cli/deployment/config_test.go b/cli/deployment/config_test.go index e6c6a5d75caa5..dce282e326c38 100644 --- a/cli/deployment/config_test.go +++ b/cli/deployment/config_test.go @@ -209,6 +209,12 @@ func TestConfig(t *testing.T) { require.Equal(t, config.Prometheus.Enable.Value, true) require.Equal(t, config.Prometheus.Address.Value, config.Prometheus.Address.Default) }, + }, { + Name: "Env fields are populated with manual or automatic name", + Valid: func(config *codersdk.DeploymentConfig) { + require.Equal(t, config.Telemetry.Enable.Env, "CODER_TELEMETRY") + require.Equal(t, config.Prometheus.Address.Env, "CODER_PROMETHEUS_ADDRESS") + }, }} { tc := tc t.Run(tc.Name, func(t *testing.T) { From 96785a7ae55ea7d95a4240369a363185dcd085f0 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 4 Nov 2022 13:32:29 +0200 Subject: [PATCH 6/7] Comment --- cli/deployment/config_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cli/deployment/config_test.go b/cli/deployment/config_test.go index dce282e326c38..1812eb6a9a859 100644 --- a/cli/deployment/config_test.go +++ b/cli/deployment/config_test.go @@ -212,8 +212,8 @@ func TestConfig(t *testing.T) { }, { Name: "Env fields are populated with manual or automatic name", Valid: func(config *codersdk.DeploymentConfig) { - require.Equal(t, config.Telemetry.Enable.Env, "CODER_TELEMETRY") - require.Equal(t, config.Prometheus.Address.Env, "CODER_PROMETHEUS_ADDRESS") + require.Equal(t, config.Telemetry.Enable.Env, "CODER_TELEMETRY") // Manual + require.Equal(t, config.Prometheus.Address.Env, "CODER_PROMETHEUS_ADDRESS") // Automatic }, }} { tc := tc From 6aecff815fb07064886c5d268d43684e15fb2468 Mon Sep 17 00:00:00 2001 From: Mathias Fredriksson Date: Fri, 4 Nov 2022 13:58:57 +0200 Subject: [PATCH 7/7] Restore CODER_TRACE as well --- cli/deployment/config.go | 1 + cli/deployment/config_test.go | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/cli/deployment/config.go b/cli/deployment/config.go index 83f8e3b7129a4..1b32a36d6efb8 100644 --- a/cli/deployment/config.go +++ b/cli/deployment/config.go @@ -295,6 +295,7 @@ func newConfig() *codersdk.DeploymentConfig { Name: "Trace Enable", Usage: "Whether application tracing data is collected. It exports to a backend configured by environment variables. See: https://github.com/open-telemetry/opentelemetry-specification/blob/main/specification/protocol/exporter.md", Flag: "trace", + Env: "CODER_TRACE", // Override default environment. }, HoneycombAPIKey: &codersdk.DeploymentConfigField[string]{ Name: "Trace Honeycomb API Key", diff --git a/cli/deployment/config_test.go b/cli/deployment/config_test.go index 1812eb6a9a859..52cae36e17165 100644 --- a/cli/deployment/config_test.go +++ b/cli/deployment/config_test.go @@ -117,7 +117,7 @@ func TestConfig(t *testing.T) { }, { Name: "Trace", Env: map[string]string{ - "CODER_TRACE_ENABLE": "true", + "CODER_TRACE": "true", "CODER_TRACE_HONEYCOMB_API_KEY": "my-honeycomb-key", }, Valid: func(config *codersdk.DeploymentConfig) { @@ -212,6 +212,7 @@ func TestConfig(t *testing.T) { }, { Name: "Env fields are populated with manual or automatic name", Valid: func(config *codersdk.DeploymentConfig) { + require.Equal(t, config.Trace.Enable.Env, "CODER_TRACE") // Manual require.Equal(t, config.Telemetry.Enable.Env, "CODER_TELEMETRY") // Manual require.Equal(t, config.Prometheus.Address.Env, "CODER_PROMETHEUS_ADDRESS") // Automatic },