torrust-tracker icon indicating copy to clipboard operation
torrust-tracker copied to clipboard

Use `figment` for configuration

Open josecelano opened this issue 10 months ago • 1 comments

Relates to: https://github.com/torrust/torrust-tracker/pull/790

This PR migrates the configuration implementation to use Figment as suggested by @da2ce7 here.

Context

I was working on a configuration validation in this PR. I wanted to validate things like socket addresses earlier when we parse the configuration instead of waiting until the app launches the service. For example, the UDP tracker configuration contains the bind_address:

[[udp_trackers]]
bind_address = "0.0.0.0:6969"
enabled = false

That configuration maps to a String:

pub struct UdpTracker {
    pub enabled: bool,
    pub bind_address: String,
}

I realized that kind of very basic validation could be actually done just changing the types in the configuration. For example:

pub struct UdpTracker {
    pub enabled: bool,
    pub bind_address: ScoketAddr,
}

There are other functionalities like overwritting values in the config file with env vars that can be implemented with Figment (merge).

So I decided to migrate to Figment the current version and update the configuration after the migration.

Subtasks without changing current config API (toml or struct)

  • Implement a new configuration mod with Figment.
  • Reorganize configuration mods creating new submods for config file sections.
  • Use Default trait for config sections (not only root config).
  • Introduce versioning for configuration, so that we can make breaking changes to the configuration in the future.
  • Replace in production the configuration with the new Figment implementation.

Subtasks changing current config API (structs)

These changes do not change the toml file configuration, only the parsed type.

  • Use reach types like SockeAddr to validate some types without even starting the services.

FUTURE PR: Subtasks changing current config API (toml and structs)

At this point an extra validation could be needed like the one described here. For example, if you enable TSL for the tracker API you must provide both the certificate path and the certificate key path:

[http_api]
bind_address = "127.0.0.1:1212"
enabled = true
ssl_cert_path = ""
ssl_enabled = false
ssl_key_path = ""

I think that type of validation could be implementented after parsing the inyected config or maybe just reorganizcing the toml file structure. For example:

No API enabled:

# No section [http_api]

API enabled but no TSL enabled:

[http_api]
bind_address = "127.0.0.1:1212"

API enabled with TSL enabled:

[http_api]
bind_address = "127.0.0.1:1212"

[http_api.tsl_config]
ssl_cert_path = "./storage/tracker/lib/tls/localhost.crt"
ssl_key_path = "./storage/tracker/lib/tls/localhost.key"

We would not need the enabled field. If the section is present the feature is enabled. If it's not it means that feature is disabled.

These breaking changes will be added in a new v2 configuration in a new PR.

josecelano avatar Apr 22 '24 13:04 josecelano

Codecov Report

Attention: Patch coverage is 94.62366% with 10 lines in your changes are missing coverage. Please review.

Project coverage is 78.76%. Comparing base (31ab3a9) to head (0252f30).

Files Patch % Lines
packages/configuration/src/v1/mod.rs 94.77% 7 Missing :warning:
packages/configuration/src/lib.rs 0.00% 3 Missing :warning:
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #808      +/-   ##
===========================================
- Coverage    78.78%   78.76%   -0.02%     
===========================================
  Files          163      168       +5     
  Lines         9252     9291      +39     
===========================================
+ Hits          7289     7318      +29     
- Misses        1963     1973      +10     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Apr 22 '24 13:04 codecov[bot]

With Figment, you can overwrite all config options using env vars. I have enabled that feature. You only have to do this:

let figment = Figment::new()
    .merge(Toml::file("Config.toml"))
    .merge(Env::prefixed("TORRUST_TRACKER_"));

We were overwriting only one value:

# Please override the admin token setting the
#    `TORRUST_TRACKER_API_ADMIN_TOKEN`
#                     environmental variable!

[http_api.access_tokens]
admin = "MyAccessToken"

With the env var TORRUST_TRACKER_API_ADMIN_TOKEN. This env var does not follow Figment conventions. The Figment name for that value would be TORRUST_TRACKER_HTTP_API.ACCESS_TOKENS.ADMIN. The current name will be deprecated when this PR is merged. We have to replace the old name everywhere with the new one TORRUST_TRACKER_HTTP_API.ACCESS_TOKENS.ADMIN.

josecelano avatar May 08 '24 15:05 josecelano

It seems the en var name generated by Figment (TORRUST_TRACKER_HTTP_API.ACCESS_TOKENS.ADMIN) is not a valid Bash var name.

In the end, you can customize the separator in Figment. I'm using a double underscore to generate a valid Bash name:

TORRUST_TRACKER__HTTP_API__ACCESS_TOKENS__ADMIN

@da2ce7, any other preference for the separator? If we want to generate a valid Bash name we don't have other options.

https://www.gnu.org/software/bash/manual/bash.html#Definitions

image

josecelano avatar May 09 '24 07:05 josecelano

ACK 0252f308183e8ea53988907e7553eda8952ebdc7

josecelano avatar May 09 '24 12:05 josecelano

Finally, I decided to merge this PR at this point with:

  • Migration to Figment done.
  • A new feature. If you are OK with the default values, you can avoid passing config options. I mean you only need to specify the config values you want to overwrite.

I have updated the Overhaul Configuration issue by adding the next steps.

josecelano avatar May 09 '24 13:05 josecelano

@josecelano I think that the double-underscore is the natural choice :)

da2ce7 avatar May 12 '24 10:05 da2ce7

@josecelano I think that the double-underscore is the natural choice :)

Hi @da2ce7, I've extended the question from only "which separator to use" (it seems __ is fine) to a whole re-design of the env var names. See https://github.com/torrust/torrust-tracker/issues/851. I would appreciate your feedback since this is a breaking change, and refactoring all the names in all repos takes quite time.

josecelano avatar May 13 '24 07:05 josecelano