Skip to content

fix: Disable viper auto-env to avoid assigning to parent structs #4893

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Nov 4, 2022
Merged

fix: Disable viper auto-env to avoid assigning to parent structs #4893

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
69326d1
fix: Disable viper auto-env to avoid assigning to parent structs
mafredri Nov 4, 2022
2f4ac30
Add test for wrong-env breakage
mafredri Nov 4, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 18 additions & 5 deletions cli/deployment/config.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you reviewed open issues for the spf13/viper? There are a few open regarding vip.AutomaticEnv. Maybe it's a bug you could raise with the team.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, I thought it'd be good for us to manually control these (we already had all the code in place to do it). But it's a good suggestion, I did a quick browse but nothing stood out. I also just pushed a follow-up PR for this: #4894 in support of not enabling AutomaticEnv.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This might be related, but it's not the exact same issue. Still, looks like a rabbit hole I don't want to dive into 😄 spf13/viper#1243

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good for me, thanks for reviewing those issues! I suspected that this might be a rabbit hole hence the idea of covering it with a basic unit test. It's up to you to decide if you prefer to open a new issue for viper or leave it as it :)


if flg != "" {
vip.SetConfigFile(flg + "/server.yaml")
Expand All @@ -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.
Comment on lines +395 to +397
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for writing the comment here. I'm afraid that we may run into this issue again in the future (with library bump). Do you think that it's possible to prepare a basic unit test to validate the behavior?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm afraid 2f4ac30 (#4893) is the best I can do. I didn't find too many ways to reproduce the issue.

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!
Expand All @@ -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:
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down
10 changes: 10 additions & 0 deletions cli/deployment/config_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down