Skip to content

Commit 4a4f120

Browse files
committed
Merge #258: feat: add socket address uniqueness validation for tracker configuration
dc153f0 refactor: [#255] eliminate duplication in binding collection (Jose Celano) 170ce02 refactor: [#255] extract conflict checking logic into separate method (Jose Celano) 97368e5 refactor: [#255] extract binding collection logic into separate method (Jose Celano) 16ade4a feat: [#255] add socket address uniqueness validation with tiered help system (Jose Celano) 981547b refactor: [#255] move database module into config/core (Jose Celano) eca6dff refactor: [#255] extract tracker config types into submodules (Jose Celano) ab84bbe feat: add configurable Health Check API to tracker configuration (Jose Celano) Pull request description: ## Summary Implements socket address uniqueness validation for tracker configuration as described in issue #255. This prevents configuration errors where multiple services attempt to bind to the same socket address with the same protocol, which would result in runtime failures. ## Changes ### Core Implementation - **BindingAddress value object** (`src/domain/tracker/binding_address.rs`) - Combines `SocketAddr` with `Protocol` for type-safe binding representation - Implements `Hash` and `Eq` for HashMap-based duplicate detection - 7 comprehensive unit tests - **TrackerConfig validation** (`src/domain/tracker/config/mod.rs`) - `validate()` - Main validation orchestrator - `collect_bindings()` - Gathers all socket bindings with service names - `check_for_conflicts()` - Detects duplicate bindings - `register_binding()` / `register_trackers()` - DRY helper methods - `HasBindAddress` trait for generic tracker handling - 8 validation tests covering all scenarios - **TrackerConfigError** with tiered help system - Brief Display with actionable tip - Detailed `.help()` method for troubleshooting - Follows error handling conventions from `docs/contributing/error-handling.md` ### Integration - Validation integrated at DTO boundary (`TrackerSection.to_tracker_config()`) - Error propagation through `CreateConfigError` - 2 integration tests for DTO validation ### Validation Rules ✅ **Rejects**: Same protocol + same socket address - Multiple HTTP services on same TCP port - Multiple UDP trackers on same port - HTTP tracker + HTTP API conflict - HTTP tracker + Health Check API conflict ✅ **Allows**: Different protocols sharing same port - UDP tracker + HTTP tracker on same port (different protocols) - Same port on different IP addresses ## Manual E2E Testing Tested with real environment configurations to verify production behavior: ### Test Case 1: TCP Conflict ❌ (Correctly Rejected) **Configuration**: HTTP Tracker and HTTP API both on `0.0.0.0:7070` (TCP) ```json { "http_trackers": [{"bind_address": "0.0.0.0:7070"}], "http_api": {"bind_address": "0.0.0.0:7070", "admin_token": "..."} } ``` **Result**: ``` ❌ Configuration validation failed: Socket address conflict: 'HTTP Tracker #1', 'HTTP API' cannot bind to 0.0.0.0:7070 (TCP) Tip: Assign different port numbers to each service ``` ### Test Case 2: UDP Conflict ❌ (Correctly Rejected) **Configuration**: Two UDP trackers on same address ```json { "udp_trackers": [ {"bind_address": "0.0.0.0:6969"}, {"bind_address": "0.0.0.0:6969"} ] } ``` **Result**: ``` ❌ Socket address conflict: 'UDP Tracker #1', 'UDP Tracker #2' cannot bind to 0.0.0.0:6969 (UDP) Tip: Assign different port numbers to each service ``` ### Test Case 3: Valid Configuration ✅ (Correctly Accepted) **Configuration**: All services on unique addresses ```json { "udp_trackers": [{"bind_address": "0.0.0.0:6969"}], "http_trackers": [{"bind_address": "0.0.0.0:7070"}], "http_api": {"bind_address": "0.0.0.0:1212"}, "health_check_api": {"bind_address": "127.0.0.1:1313"} } ``` **Result**: ✅ Environment created successfully ### Test Case 4: Cross-Protocol ✅ (Correctly Accepted) **Configuration**: UDP and TCP sharing same port (different protocols) ```json { "udp_trackers": [{"bind_address": "0.0.0.0:7070"}], "http_trackers": [{"bind_address": "0.0.0.0:7070"}] } ``` **Result**: ✅ Environment created successfully (UDP and TCP use separate port spaces) ## Automated Testing - ✅ 1613 unit tests passing - ✅ All linters passing (markdown, YAML, TOML, cspell, clippy, rustfmt, shellcheck) - ✅ Documentation builds successfully - ✅ E2E infrastructure lifecycle tests passing - ✅ E2E deployment workflow tests passing - ✅ Total pre-commit time: 4m 45s ## Code Quality Improvements This PR includes several refactoring commits that improve code quality: 1. **Extract binding collection** - Separate method for gathering bindings 2. **Extract conflict checking** - Focused method for duplicate detection 3. **Eliminate duplication** - DRY helpers for binding registration **Benefits**: - Single Responsibility Principle compliance - Better testability with focused methods - Reduced code duplication (~30 lines → ~15 lines) - Clear separation of concerns - Easier to maintain and extend ## Documentation - Comprehensive rustdoc comments on all public APIs - Error help text with platform-specific troubleshooting - References to `docs/external-issues/tracker/udp-tcp-port-sharing-allowed.md` ## Related - Closes #255 - Related: Socket address validation specification ACKs for top commit: josecelano: ACK dc153f0 Tree-SHA512: e77aae9c25da50fed3aebf6bf513462f5f7b31378920a0b3bd725d454870ab0d8e3f993377a855ae599e96ea63c649f91c8158298f7aca3c7b1f5fff17308372
2 parents 69ae424 + dc153f0 commit 4a4f120

File tree

29 files changed

+1744
-266
lines changed

29 files changed

+1744
-266
lines changed

project-words.txt

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -303,3 +303,5 @@ zeroize
303303
ключ
304304
конфиг
305305
файл
306+
Datagram
307+
connectionless

schemas/environment-config.json

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -155,6 +155,17 @@
155155
"admin_password"
156156
]
157157
},
158+
"HealthCheckApiSection": {
159+
"type": "object",
160+
"properties": {
161+
"bind_address": {
162+
"type": "string"
163+
}
164+
},
165+
"required": [
166+
"bind_address"
167+
]
168+
},
158169
"HetznerProviderSection": {
159170
"description": "Hetzner-specific configuration section\n\nUses raw `String` fields for JSON deserialization. Convert to domain\n`HetznerConfig` via `ProviderSection::to_provider_config()`.\n\n# Examples\n\n```rust\nuse torrust_tracker_deployer_lib::application::command_handlers::create::config::HetznerProviderSection;\n\nlet section = HetznerProviderSection {\n api_token: \"your-api-token\".to_string(),\n server_type: \"cx22\".to_string(),\n location: \"nbg1\".to_string(),\n image: \"ubuntu-24.04\".to_string(),\n};\n```",
160171
"type": "object",
@@ -320,13 +331,17 @@
320331
]
321332
},
322333
"TrackerSection": {
323-
"description": "Tracker configuration section (application DTO)\n\nAggregates all tracker configuration sections: core, UDP trackers,\nHTTP trackers, and HTTP API.\n\n# Examples\n\n```json\n{\n \"core\": {\n \"database\": {\n \"driver\": \"sqlite3\",\n \"database_name\": \"tracker.db\"\n },\n \"private\": false\n },\n \"udp_trackers\": [\n { \"bind_address\": \"0.0.0.0:6969\" }\n ],\n \"http_trackers\": [\n { \"bind_address\": \"0.0.0.0:7070\" }\n ],\n \"http_api\": {\n \"bind_address\": \"0.0.0.0:1212\",\n \"admin_token\": \"MyAccessToken\"\n }\n}\n```",
334+
"description": "Tracker configuration section (application DTO)\n\nAggregates all tracker configuration sections: core, UDP trackers,\nHTTP trackers, and HTTP API.\n\n# Examples\n\n```json\n{\n \"core\": {\n \"database\": {\n \"driver\": \"sqlite3\",\n \"database_name\": \"tracker.db\"\n },\n \"private\": false\n },\n \"udp_trackers\": [\n { \"bind_address\": \"0.0.0.0:6969\" }\n ],\n \"http_trackers\": [\n { \"bind_address\": \"0.0.0.0:7070\" }\n ],\n \"http_api\": {\n \"bind_address\": \"0.0.0.0:1212\",\n \"admin_token\": \"MyAccessToken\"\n },\n \"health_check_api\": {\n \"bind_address\": \"127.0.0.1:1313\"\n }\n}\n```",
324335
"type": "object",
325336
"properties": {
326337
"core": {
327338
"description": "Core tracker configuration (database, privacy mode)",
328339
"$ref": "#/$defs/TrackerCoreSection"
329340
},
341+
"health_check_api": {
342+
"description": "Health Check API configuration",
343+
"$ref": "#/$defs/HealthCheckApiSection"
344+
},
330345
"http_api": {
331346
"description": "HTTP API configuration",
332347
"$ref": "#/$defs/HttpApiSection"
@@ -350,7 +365,8 @@
350365
"core",
351366
"udp_trackers",
352367
"http_trackers",
353-
"http_api"
368+
"http_api",
369+
"health_check_api"
354370
]
355371
},
356372
"UdpTrackerSection": {
@@ -365,4 +381,4 @@
365381
]
366382
}
367383
}
368-
}
384+
}

src/application/command_handlers/create/config/environment_config.rs

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -67,6 +67,9 @@ use super::tracker::TrackerSection;
6767
/// "http_api": {
6868
/// "bind_address": "0.0.0.0:1212",
6969
/// "admin_token": "MyAccessToken"
70+
/// },
71+
/// "health_check_api": {
72+
/// "bind_address": "127.0.0.1:1313"
7073
/// }
7174
/// },
7275
/// "prometheus": {
@@ -417,6 +420,7 @@ impl EnvironmentCreationConfig {
417420
bind_address: "0.0.0.0:1212".to_string(),
418421
admin_token: "MyAccessToken".to_string(),
419422
},
423+
health_check_api: super::tracker::HealthCheckApiSection::default(),
420424
},
421425
prometheus: Some(PrometheusSection::default()),
422426
grafana: Some(GrafanaSection::default()),
@@ -572,6 +576,9 @@ mod tests {
572576
"http_api": {
573577
"bind_address": "0.0.0.0:1212",
574578
"admin_token": "MyAccessToken"
579+
},
580+
"health_check_api": {
581+
"bind_address": "127.0.0.1:1313"
575582
}
576583
}
577584
}"#;
@@ -633,6 +640,9 @@ mod tests {
633640
"http_api": {
634641
"bind_address": "0.0.0.0:1212",
635642
"admin_token": "MyAccessToken"
643+
},
644+
"health_check_api": {
645+
"bind_address": "127.0.0.1:1313"
636646
}
637647
}
638648
}"#;

src/application/command_handlers/create/config/errors.rs

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
use std::path::PathBuf;
88
use thiserror::Error;
99

10+
use crate::domain::tracker::TrackerConfigError;
1011
use crate::domain::EnvironmentNameError;
1112
use crate::domain::ProfileNameError;
1213
use crate::shared::UsernameError;
@@ -105,6 +106,10 @@ pub enum CreateConfigError {
105106
/// Invalid Prometheus configuration
106107
#[error("Invalid Prometheus configuration: {0}")]
107108
InvalidPrometheusConfig(String),
109+
110+
/// Tracker configuration validation failed
111+
#[error("Tracker configuration validation failed: {0}")]
112+
TrackerConfigValidation(#[from] TrackerConfigError),
108113
}
109114

110115
impl CreateConfigError {
@@ -424,6 +429,27 @@ impl CreateConfigError {
424429
Note: The template automatically adds the 's' suffix (e.g., 15 becomes '15s'),\n\
425430
so you only need to specify the numeric value."
426431
}
432+
Self::TrackerConfigValidation(_) => {
433+
"Tracker configuration validation failed.\n\
434+
\n\
435+
This error indicates a problem with the tracker service configuration,\n\
436+
typically related to socket address (IP:Port:Protocol) conflicts.\n\
437+
\n\
438+
The error message above provides specific details about:\n\
439+
- Which services are in conflict\n\
440+
- The conflicting socket addresses\n\
441+
- Why the configuration is invalid\n\
442+
\n\
443+
Common issues:\n\
444+
1. Multiple services on same TCP port (HTTP tracker + API)\n\
445+
2. Duplicate UDP tracker ports\n\
446+
3. Duplicate HTTP tracker ports\n\
447+
\n\
448+
Note: UDP and TCP can share the same port (different protocols),\n\
449+
but this is not recommended for clarity.\n\
450+
\n\
451+
Related: docs/external-issues/tracker/udp-tcp-port-sharing-allowed.md"
452+
}
427453
}
428454
}
429455
}
Lines changed: 125 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,125 @@
1+
use std::net::SocketAddr;
2+
3+
use schemars::JsonSchema;
4+
use serde::{Deserialize, Serialize};
5+
6+
use crate::application::command_handlers::create::config::errors::CreateConfigError;
7+
use crate::domain::tracker::HealthCheckApiConfig;
8+
9+
#[derive(Debug, Clone, Serialize, Deserialize, PartialEq, JsonSchema)]
10+
pub struct HealthCheckApiSection {
11+
pub bind_address: String,
12+
}
13+
14+
impl HealthCheckApiSection {
15+
/// Converts this DTO to a domain `HealthCheckApiConfig`
16+
///
17+
/// # Errors
18+
///
19+
/// Returns `CreateConfigError::InvalidBindAddress` if the bind address cannot be parsed as a valid IP:PORT combination.
20+
/// Returns `CreateConfigError::DynamicPortNotSupported` if port 0 (dynamic port assignment) is specified.
21+
pub fn to_health_check_api_config(&self) -> Result<HealthCheckApiConfig, CreateConfigError> {
22+
// Validate that the bind address can be parsed as SocketAddr
23+
let bind_address = self.bind_address.parse::<SocketAddr>().map_err(|e| {
24+
CreateConfigError::InvalidBindAddress {
25+
address: self.bind_address.clone(),
26+
source: e,
27+
}
28+
})?;
29+
30+
// Reject port 0 (dynamic port assignment)
31+
if bind_address.port() == 0 {
32+
return Err(CreateConfigError::DynamicPortNotSupported {
33+
bind_address: self.bind_address.clone(),
34+
});
35+
}
36+
37+
Ok(HealthCheckApiConfig { bind_address })
38+
}
39+
}
40+
41+
impl Default for HealthCheckApiSection {
42+
fn default() -> Self {
43+
Self {
44+
bind_address: "127.0.0.1:1313".to_string(),
45+
}
46+
}
47+
}
48+
49+
#[cfg(test)]
50+
mod tests {
51+
use super::*;
52+
53+
#[test]
54+
fn it_should_convert_to_domain_config_when_bind_address_is_valid() {
55+
let section = HealthCheckApiSection {
56+
bind_address: "127.0.0.1:1313".to_string(),
57+
};
58+
59+
let config = section.to_health_check_api_config().unwrap();
60+
61+
assert_eq!(
62+
config.bind_address,
63+
"127.0.0.1:1313".parse::<SocketAddr>().unwrap()
64+
);
65+
}
66+
67+
#[test]
68+
fn it_should_fail_when_bind_address_is_invalid() {
69+
let section = HealthCheckApiSection {
70+
bind_address: "invalid".to_string(),
71+
};
72+
73+
let result = section.to_health_check_api_config();
74+
75+
assert!(result.is_err());
76+
assert!(matches!(
77+
result.unwrap_err(),
78+
CreateConfigError::InvalidBindAddress { .. }
79+
));
80+
}
81+
82+
#[test]
83+
fn it_should_reject_dynamic_port_assignment() {
84+
let section = HealthCheckApiSection {
85+
bind_address: "0.0.0.0:0".to_string(),
86+
};
87+
88+
let result = section.to_health_check_api_config();
89+
90+
assert!(result.is_err());
91+
assert!(matches!(
92+
result.unwrap_err(),
93+
CreateConfigError::DynamicPortNotSupported { .. }
94+
));
95+
}
96+
97+
#[test]
98+
fn it_should_allow_ipv6_addresses() {
99+
let section = HealthCheckApiSection {
100+
bind_address: "[::1]:1313".to_string(),
101+
};
102+
103+
let result = section.to_health_check_api_config();
104+
105+
assert!(result.is_ok());
106+
}
107+
108+
#[test]
109+
fn it_should_allow_any_port_except_zero() {
110+
let section = HealthCheckApiSection {
111+
bind_address: "127.0.0.1:8080".to_string(),
112+
};
113+
114+
let result = section.to_health_check_api_config();
115+
116+
assert!(result.is_ok());
117+
}
118+
119+
#[test]
120+
fn it_should_provide_default_localhost_1313() {
121+
let section = HealthCheckApiSection::default();
122+
123+
assert_eq!(section.bind_address, "127.0.0.1:1313");
124+
}
125+
}

src/application/command_handlers/create/config/tracker/mod.rs

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,12 +4,14 @@
44
//! environment creation. These types use raw primitives (String) for
55
//! JSON deserialization and convert to rich domain types (`SocketAddr`).
66
7+
mod health_check_api_section;
78
mod http_api_section;
89
mod http_tracker_section;
910
mod tracker_core_section;
1011
mod tracker_section;
1112
mod udp_tracker_section;
1213

14+
pub use health_check_api_section::HealthCheckApiSection;
1315
pub use http_api_section::HttpApiSection;
1416
pub use http_tracker_section::HttpTrackerSection;
1517
pub use tracker_core_section::{DatabaseSection, TrackerCoreSection};

0 commit comments

Comments
 (0)