-
Notifications
You must be signed in to change notification settings - Fork 47
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
Validate services configuration before starting them #790
Conversation
Codecov ReportAttention: Patch coverage is
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. |
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. |
66c452b
to
4f91133
Compare
… the service It shows a more friendly error message and it's easier to test.
4f91133
to
1033a7d
Compare
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.
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. |
Closed in favour of #808 |
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
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
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:
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:
Subtasks