Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Validate services configuration before starting them #790

Conversation

josecelano
Copy link
Member

@josecelano josecelano commented Apr 11, 2024

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

  • HTTP tracker
  • UDP tracker
  • Tracker API
  • Health Check API
  • Core tracker

@josecelano josecelano added the Code Cleanup / Refactoring Tidying and Making Neat label Apr 11, 2024
@josecelano josecelano mentioned this pull request Apr 11, 2024
5 tasks
Copy link

codecov bot commented Apr 11, 2024

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 ⚠️
packages/configuration/src/sections/core.rs 0.00% 33 Missing ⚠️
packages/configuration/src/sections/tracker_api.rs 70.83% 28 Missing ⚠️
...ackages/configuration/src/sections/http_tracker.rs 71.59% 25 Missing ⚠️
packages/configuration/src/sections/udp_tracker.rs 45.71% 19 Missing ⚠️
...ges/configuration/src/sections/health_check_api.rs 55.17% 13 Missing ⚠️
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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@josecelano
Copy link
Member Author

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 josecelano requested a review from da2ce7 April 11, 2024 17:05
@josecelano josecelano added this to the v3.0.0 milestone Apr 11, 2024
@josecelano josecelano force-pushed the issue-521-enable-ignored-test-again branch from 66c452b to 4f91133 Compare April 15, 2024 07:49
… the service

It shows a more friendly error message and it's easier to test.
@josecelano josecelano force-pushed the issue-521-enable-ignored-test-again branch from 4f91133 to 1033a7d Compare April 19, 2024 08:39
It shows a more friendly error message and it's easier to test.
It shows a more friendly error message and it's easier to test.
…tarts

It shows a more friendly error message and it's easier to test.
@josecelano
Copy link
Member Author

I decided to migrate to Figment before implementing this validation. See #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
Copy link
Member Author

Closed in favour of #808

@josecelano josecelano closed this May 7, 2024
josecelano added a commit that referenced this pull request May 9, 2024
0252f30 feat: allow users not to provide config option with default values (Jose Celano)
43942ce tests: add test for configuration with deprecated env var name (Jose Celano)
b0c2f9f docs: update env var name in toml config template files (Jose Celano)
69d7939 refactor: implement Default for Configuration sections (Jose Celano)
caae725 feat: use double underscore to split config env var names (Jose Celano)
b3a1442 refactor!: remove unused method in Configuration (Jose Celano)
632c8ba refactor: move Configuration unit test to inner mods (Jose Celano)
146b77d feat: enable overwrite Configuration values using env vars (Jose Celano)
5bd9494 chore: remove unused config dependenciy (Jose Celano)
265d89d refactor: replace Config by Figment in Configuration implementation (Jose Celano)
002fb30 refactor: reexport config versioned config types (Jose Celano)
e7d344c refactor: create new configuration v1 mod with figment (Jose Celano)
636e779 refactor: create new configuration v1 mod with figment (Jose Celano)
157807c chore(deps): enable figment features: env, toml, test (Jose Celano)
f0e0721 test: remove broken example in rustdoc (Jose Celano)
7da52b1 chore(deps): add dependency figment (Jose Celano)

Pull request description:

  Relates to: #790

  This PR migrates the configuration implementation to use [Figment](https://crates.io/crates/figment) as suggested by @da2ce7 [here](#401).

  ### Context

  I was working on a configuration validation in this [PR](#790). 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`:

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

  That configuration maps to a `String`:

  ```rust
  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:

  ```rust
  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)

  - [x] Implement a new configuration mod with Figment.
  - [x] Reorganize configuration mods creating new submods for config file sections.
  - [x] Introduce versioning for configuration, so that we can make breaking changes to the configuration in the future.
  - [x] Replace in production the configuration with the new Figment implementation.
  - [x] Use `Default` trait for config sections (not only root config).
  - [x] Allow users not to provide values when defaults are OK for them.

  ### FUTURE PR: 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](#790). For example, if  you enable TSL for the tracker API you must provide both the certificate path and the certificate key path:

  ```toml
  [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:

  ```toml
  # No section [http_api]
  ```

  API enabled but no TSL enabled:

  ```toml
  [http_api]
  bind_address = "127.0.0.1:1212"
  ```

  API enabled with TSL enabled:

  ```toml
  [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.

ACKs for top commit:
  josecelano:
    ACK 0252f30

Tree-SHA512: 94c7baa8566744c9e79bc9c56aae836d7a9b4272c42965c2dc88ec04b6a297b830c8eb7cb65d318c74e00aa5e27cc7c56784d6b083af04e98aadff4c82af5239
josecelano added a commit that referenced this pull request May 10, 2024
b545b33 refactor: [#852] extract Core configuration type (Jose Celano)
7519ecc refactor: [#852] enrich field types in Configuration struct (Jose Celano)
ceb3074 refactor: [#852] enrich field types in HttpApi config struct (Jose Celano)
3997cfa refactor: [#852] eenrich field types in TslConfig config struct (Jose Celano)
a2e718b chore(deps): add dependency camino (Jose Celano)
fc191f7 refactor: [#852] enrich field types in HttpTracker config struct (Jose Celano)
1475ead refactor: [#852] eenrich field types in UdpTracker config struct (Jose Celano)
384e9f8 refactor: [#852] eenrich field types in HealthCheckApi config struct (Jose Celano)

Pull request description:

  Relates to: #790

  Refactor: enrich field types in configuration structs.

  This will cause the app to fail earlier while loading invalid configurations and simplify the code by reducing conversions to get the rich type from the primitive when it's used.

  - [x] `HealthCheckApi`
  - [x] `UdpTracker`
  - [x] `HttpTracker`
  - [x] `HttpApi`
  - [x] `Configuration`

  Output example when you provide an invalid socket address:

  ```output
  $ cargo run
      Finished `dev` profile [optimized + debuginfo] target(s) in 0.10s
       Running `target/debug/torrust-tracker`
  Loading default configuration file: `./share/default/config/tracker.development.sqlite3.toml` ...
  thread 'main' panicked at src/bootstrap/config.rs:45:32:
  called `Result::unwrap()` on an `Err` value: ConfigError { source: LocatedError { source: Error { tag: Tag(Default, 2), profile: Some(Profile(Uncased { string: "default" })), metadata: Some(Metadata { name: "TOML source string", source: None, provide_location: Some(Location { file: "packages/configuration/src/v1/mod.rs", line: 397, col: 14 }), interpolater:  }), path: ["health_check_api", "bind_address"], kind: Message("invalid socket address syntax"), prev: None }, location: Location { file: "packages/configuration/src/v1/mod.rs", line: 400, col: 41 } } }
  note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
  ```

  You get that error proving this config option:

  ```toml
  [health_check_api]
  bind_address = "127.0.0.1-1313"
  ```

  The error contains all the information needed, although it could be more user-friendly. Maybe we can map that error to a simpler explanation like this:

  ```
  Configuration error: invalid socket address provided in toml file PATH in section `health_check_api` option `bind_address`.
  ```

ACKs for top commit:
  josecelano:
    ACK b545b33

Tree-SHA512: 5d4ca16d882447e9eacc882022346e0cd7484f7b5a0d7aa1754e94cde438e6fe486d690d9977df171c9c792ab5ed3cd814bc500f3fac91cfdf76eb76b5306dc7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Cleanup / Refactoring Tidying and Making Neat
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant