Athens: envconfig incorrectly overrides config values from file

[oakad]

envconfig will check if appropriate environment variable variable is set, and it not will set the corresponding config value to default, if default tag is present.

However, by doing so it will rewrite the values set via the config file, because it is run after the config file was parsed (consider: https://github.com/gomods/athens/blob/fd10bed60949bb14d612222b3bf600f07808d65d/pkg/config/config.go#L87-L99)

This makes it impossible to set a Port value (the only one with default tag right now) via the config file, as envconfig will revert it to default unconditionally.

[marpio]

@oakad Thanks for taking the time to create the issue!

@gomods/maintainers I think we shouldn't use the envconfig default tag at all. We provide the default values via the config file. If the config file can't be found we will use hardcoded values once https://github.com/gomods/athens/pull/1022 is in.

[marwan-at-work]

From what I can see only Port has a default tag, nothing else does so I'm curious which values do get overridden? @oakad if it's not the server port, can you please provide steps to reproduce this issue. thanks

[marpio]

@marwan-at-work it is the Port that gets overridden.

[jamesgeorge007]

@michalpristas @marwan-at-work @marpio Can I take this up?

[manugupt1]

@jamesgeorge007 Sure! Go ahead and take this up.

[jamesgeorge007]

@manugupt1 I'm not that familiar with the code base.
What changes has to be made here?

[manugupt1]

I am not sure what changes have to be made here (I will have to investigate), but I will start by reproducing the issue if you can. Second, I will look at the config package that is mentioned in the issue above and finally config.dev.toml to see how variables are set.

[fsouza]

This one is tricky. I created the test reproducing the issue here: https://github.com/gomods/athens/commit/53a94b3f57a0a8ab3e903c08a81893868b202126.

I was thinking about approaches to fix this and wanted to check before I send a PR:

1. replace envconfig with some other tool (I only know env and that has the same limitation - and it also sounds like the author may want to converge towards envconfig: https://github.com/caarlos0/env/issues/47). After a quick search, I also found go-config, but that comes with a LOT of dependencies.
2. Manually handle the default value in a static manner. This isn't too bad because the default tag is used only for the port, but can become a pain if there's a plan to support other fields with default values.
3. Dynamically handle the default values: have _3_ Config structs, one loaded from the config file (F), another other one loaded from the environment (E) and the last one with default values (D), then merge the values into a single struct with the following logic: pick the value from E as long as it's not zero and not equal to D, otherwise use the value from F.
4. Discuss the possibility of evolving envconfig to support this use case (some related issues reported over there: https://github.com/kelseyhightower/envconfig/issues/123, https://github.com/kelseyhightower/envconfig/issues/117)
5. ... something else? x)

[marwan-at-work]

@fsouza thanks for the details :)

TL;DR

The quickest fix to this bug is to just remove the default tag from here: https://github.com/gomods/athens/blob/master/pkg/config/config.go#L31

Which is your no.2 option. I think this is okay and once we start having a difficult time to manage it, we can go back and address it.

Background

Currently we have two places for deciding default values, the config.toml file, and the config.go file. This means there are two sources of truth for where default values live. However, it should hopefully make Athens easier to manage because it means you can run it just as a binary (without needing to point it to a config.toml file) and things should work out of the box.

This is how Athens currently work:

1. if there's a TOML file, use it (passed through the -config_file flag to the server). This already has all the defaults, but users can change it up.
2. if there's no TOML file, use the defaults in the config.go file.
3. once 1, or 2 are done, use all the env vars to override anything that needs to be overridden.

[fsouza]

Cool, thanks for the feedback. Here's the PR: https://github.com/gomods/athens/pull/1074 :D