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

Validate services configuration before starting them

Open josecelano opened this issue 1 year ago • 3 comments

These refactors allow proper configuration validation with unit tests. It also improves the error messages when the service configuration is wrong.

For example, this is the current error message if you enable SSL for the HTTP tracker but you don't provide the certificate and the key files:

Loading default configuration file: `./share/default/config/tracker.development.sqlite3.toml` ...
2024-04-11T17:13:28.016834819+01:00 [torrust_tracker::bootstrap::logging][INFO] logging initialized.
2024-04-11T17:13:28.017498045+01:00 [UDP TRACKER][INFO] Starting on: udp://0.0.0.0:6969
thread 'main' panicked at /home/developer/Documents/git/committer/me/github/torrust/torrust-tracker/src/app.rs:96:25:
Invalid HTTP Tracker configuration: missing SSL cert path, got: none
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

This is also related to something we have discussed in the past. The application should fail as soon as possible. If the provided configuration is wrong, it should panic before starting the service.

The idea is to progressively convert the current configuration structs into DTOs (plain configuration). Those DTOs are validated and converted into rich domain objects. For example:

// DTO: it's just the config toml file section parsed into Rust struct.
pub struct HttpTracker {
    pub enabled: bool,
    pub bind_address: String,
    pub ssl_enabled: bool,
    pub ssl_cert_path: Option<String>,
    pub ssl_key_path: Option<String>,
}

// Validated configuration:
// - private fields
// - Domain types: SocketAddr, Path, ...
pub struct Config {
    enabled: bool,
    bind_address: SocketAddr,
    ssl_enabled: bool,
    ssl_cert_path: Option<Path>,
    ssl_key_path: Option<Path>,
}

Subtasks

  • [x] HTTP tracker
  • [x] UDP tracker
  • [x] Tracker API
  • [x] Health Check API
  • [ ] Core tracker

josecelano avatar Apr 11 '24 16:04 josecelano

Codecov Report

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

Project coverage is 78.15%. Comparing base (9a1ce0e) to head (81b81ff).

Files Patch % Lines
src/app.rs 0.00% 41 Missing :warning:
packages/configuration/src/sections/core.rs 0.00% 33 Missing :warning:
packages/configuration/src/sections/tracker_api.rs 70.83% 28 Missing :warning:
...ackages/configuration/src/sections/http_tracker.rs 71.59% 25 Missing :warning:
packages/configuration/src/sections/udp_tracker.rs 45.71% 19 Missing :warning:
...ges/configuration/src/sections/health_check_api.rs 55.17% 13 Missing :warning:
Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #790      +/-   ##
===========================================
- Coverage    78.93%   78.15%   -0.78%     
===========================================
  Files          162      167       +5     
  Lines         9186     9486     +300     
===========================================
+ Hits          7251     7414     +163     
- Misses        1935     2072     +137     

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

codecov[bot] avatar Apr 11 '24 16:04 codecov[bot]

Hi, @da2ce7, this is a significant change in the configuration. However, in this PR, I'm just creating the new configuration type for validation and converting it back to the plain structure (the current one) to avoid changing the configuration structures for the whole app.

It would be nice if you review it, because you are also working on the configuration overhaul.

josecelano avatar Apr 11 '24 17:04 josecelano

I decided to migrate to Figment before implementing this validation. See https://github.com/torrust/torrust-tracker/pull/808. This validation could be needed anyway, but all the validation added here could be implemented by just adding reacher types to the configuration structures.

josecelano avatar Apr 22 '24 13:04 josecelano

Closed in favour of https://github.com/torrust/torrust-tracker/pull/808

josecelano avatar May 07 '24 11:05 josecelano