geoipupdate supports environment variables for config options #243
Conversation
The current config code mixes constructing values, setting defaults, validation, etc. which makes it difficult to reason about how a configuration is constructed. This refactors the config package to more clearly separate the various steps of config construction, as well as providing a way for e.g. a Proxy url from one config to work with the ProxyUserPassword from an overriding config (which itself does not provide a Proxy url). It also makes it easier to add additional configuration fields and/or normalize those fields since those steps now come after applying any overrides.
Previous various parts of the config could only be set in a config file (and some parts by the config file or command line options). This updates the config package to also set config options based on environment variables after the config file and before command line options are used.
|
We should also update the changelog |
The various environment variables are referenced in the config file document already, so we can remove these as mostly duplicative.
While it is true we are still creating a configuration file, we aren't actually writing anything to it anymore and thus anyone who reads that message probably wouldn't be interested in it any longer.
Previously a configuration file was required, even if all required configuration was supplied through other means. This removes that requirement, making the configuration file optional.
The original verbose option behavior enabled it if any value was used, and would not error if a value besides 0 or 1 was used. This returns to the original behavior of allowing any value to be used.
| @@ -19,10 +19,15 @@ type Args struct { | |||
| } | |||
|
|
|||
| func getArgs() *Args { | |||
| confFileDefault := vars.DefaultConfigFile | |||
There was a problem hiding this comment.
If I want to configure based on env vars rather than a config file, will that work? I'm thinking that there will always be a non-empty config file specified given the default so we will always try to use it. In the case where I'm wanting to use env vars only, would that error? Not sure how to solve this if so. I guess we'd have to set GEOIPUPDATE_CONF_FILE="" maybe?
We also should think about how docker users will work given they may have been passing that env var already.
There was a problem hiding this comment.
Yeah, GEOIPUPDATE_CONFIG_FILE="" would have to be set to avoid reading the config file altogether. To avoid using any of the values of that config file one would have to supply each option as an environment variable or command line flag -- the file would still get read but the values should be overridden.
The only other thing I can think of is convoluted and doesn't really solve it ideally either. Something like create the config using flags first, then environment variables, and then the config file. Each time it would only update fields that haven't yet been updated. When it reaches the time to set config from the config file, it could check if the current config has any fields that are zero values and if not avoid reading the file altogether. But that would mean every optional value would need to be set explicitly, so I don't think thats what we want to do.
Loading a configuration can fail in ways that do not involve a config file. This removes part of an error message that implies otherwise.
|
If we consider the account ID a secret, I wonder if we should change the error message |
|
Yeah, making that message more generic seems fine. Up to you if you want to add it to this PR or not though :-P |
doc/docker.md
Outdated
| @@ -10,16 +10,22 @@ The source code is available on [GitHub](https://github.com/maxmind/geoipupdate) | |||
| The Docker image is configured by environment variables. The following | |||
| variables are required: | |||
|
|
|||
| * `GEOIPUPDATE_ACCOUNT_ID` - Your MaxMind account ID. | |||
| * `GEOIPUPDATE_LICENSE_KEY` - Your case-sensitive MaxMind license key. | |||
|
|
|||
| * `GEOIPUPDATE_EDITION_IDS` - List of space-separated database edition IDs. | ||
| Edition IDs may consist of letters, digits, and dashes. For example, | ||
| `GeoIP2-City` would download the GeoIP2 City database (`GeoIP2-City`). | ||
|
|
||
| One of: | ||
| * `GEOIPUPDATE_ACCOUNT_ID` - Your MaxMind account ID. |
There was a problem hiding this comment.
Should we have blank lines above this and the start of the next list? Not sure how the docker markdown renderer will show them. I assume it's fine either way though 🤷
There was a problem hiding this comment.
Yeah I'm not sure either. It rendered fine in the VSCode markdown editor, but I changed it just in case.
doc/docker.md
Outdated
| One of: | ||
|
|
||
| * `GEOIPUPDATE_ACCOUNT_ID` - Your MaxMind account ID. | ||
| * `GEOIPUPDATE_ACCOUNT_IDFILE` - A file containing your MaxMind account ID. |
| fi | ||
|
|
||
| mkdir -p $log_dir | ||
|
|
||
| while true; do | ||
| echo "# STATE: Running geoipupdate" | ||
| /usr/bin/geoipupdate -d "$database_dir" -f "$conf_file" $flags 1>$log_file | ||
| /usr/bin/geoipupdate $flags 1>$log_file |
There was a problem hiding this comment.
This didn't work as apparently we still need to provide a GeoIP.conf file.
| if ! [ -z "$GEOIPUPDATE_CONF_FILE" ]; then | ||
| conf_file=$GEOIPUPDATE_CONF_FILE | ||
| if [ -z "$GEOIPUPDATE_DB_DIR" ]; then | ||
| GEOIPUPDATE_DB_DIR="$database_dir" |
There was a problem hiding this comment.
Doesn't this need to be exported?
Previously any configuration of
geoipupdatehad to happen through a config file and command line arguments. This updatesgeoipupdateto understand configuration through environment variables, which can also override values set through the config file (but not those set by command line arguments).