config: Fix overriding options with env vars

[ansasaki]

Read the configuration options set via environment variables separately
to initialize an EnvConfig structure and use this structure as a
Source to override the values of the KeylimeConfig obtained from the
configuration files.

For this, change the prefix for the environment variables to
KEYLIME_AGENT with the 'prefix_separator' as '_', and 'separator' as
'.'.

Then the configuration options obtained from the environment variables
are used to initialize an EnvConfig structure, which does not contain
sub-structures.

The EnvConfig structure is a duplicate of AgentConfig structure, but
contains Option for all configuration options, allowing values to not
be set via environment variables.

The remaining optional fields in AgentConfig were replaced to be
required (not Option).

Note that the verification of required/non-required options
was not used since all values are set with the default values and then
successively overriden by values from the configuration files and
environment variables.

The Source trait was implemented for the EnvConfig structure in a way
that it is possible to use it as the source for KeylimeConfig structure.

Fixes: #510

Signed-off-by: Anderson Toshiyuki Sasaki [email protected]

[aplanas]

OK for me, but maybe we can drop concept of "configuration as a dictionary", and use the struct directly

[ansasaki]

We use the struct directly, the collection to get the map is only used to implement Source to allow overriding options with the values obtained from the environment variable.

[aplanas]

Now everything is optional, can this be a problem?

[ansasaki]

It is not a problem because since the last big configuration change to TOML format and to use the config crate, we always base the configuration from the defaults. So basically there is no case when an option is not set.

[ansasaki]

/packit retest-failed

[THS-on]

Can we move the default value part into the KeylimeConfig struct? I don't really like that we now have checks for the defaults spread all over the code base.

[ansasaki]

Can we move the default value part into the KeylimeConfig struct? I don't really like that we now have checks for the defaults spread all over the code base.

Actually all of those are dead code, as the options are always set. The problem is that we need the internal AgentConfig structure to be made of Option<T> to allow config to be partially filled from the environment variables, then use it to override the options from the files.

I can replace all of them with except() or similar that would panic if not set, but usually this is not the preferred way.

We can alternatively make the AgentConfig structure to not contain any Option<T> and create a new structure duplicating the AgentConfig struct but made of Option<T> to read from the environment (e.g. EnvConfig). I tried to avoid this when writing this changes, but maybe it is the best way since it makes it clear the purposes of each structure: The EnvConfig which may be partially set and used only to read config from the environment and the AgentConfig which is the actual configuration where all the fields are required to be set.

[THS-on]

Yeah I think the last approach is probably the best. Then we have the duplication only in one place.

[ansasaki]

@THS-on I rewrote the code to add EnvConfig which is a duplicate of AgentConfig but with all fields optional. The remaining optional fields in AgentConfig were also replaced by required fields (i.e. not Option<T>).

I removed some checks for options set throughout the code as the options are all required. I had to keep checks for empty strings is some cases, though.

I also added instructions to the configuration file on how to override options using env vars.

[THS-on]

@ansasaki thank you for changing it :)

I think the empty strings checks are fine, because that are not that many.