Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
211 changes: 130 additions & 81 deletions docs/issues/open/2067-1978-analyze-flat-service-configuration/ISSUE.md

Large diffs are not rendered by default.

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Evidence Ledger: Flat Heterogeneous Service Configuration

> **Status:** Planned
> **Status:** Complete
>
> **Issue contract:** [ISSUE.md](ISSUE.md)
>
Expand All @@ -14,48 +14,133 @@ or runtime change was implemented.

- **Question:** What current configuration, runtime, identity, shared-state, and redaction
contracts constrain the analysis?
- **Status:** TODO
- **Method:** TODO
- **Observation:** TODO
- **Conclusion:** TODO
- **Status:** PASS
- **Method:** Reviewed `packages/configuration/src/v3_0_0/mod.rs`, `http_tracker.rs`,
`udp_tracker.rs`, `tracker_api.rs`, `health_check_api.rs`, and `udp_tracker_server.rs`;
`packages/configuration/src/lib.rs`; `src/bootstrap/app.rs`, `src/app.rs`, and
`src/container.rs`; `packages/primitives/src/configuration_instance_id.rs`,
`service_role.rs`, and `runtime_service_metadata.rs`; `packages/udp-core/src/container.rs`;
and `tests/common/configuration.rs`.
- **Observation:** V3 has optional HTTP/UDP vectors and HTTP API, but defaulted health and shared
UDP server sections. Production global aliases still select v2 until #1980. Startup groups
shared UDP work before UDP instances, then HTTP instances, optional REST, and health. IDs are
role-local; REST and health use ordinal zero. The registry records final post-bind bindings.
The shared UDP ban service takes `max_connection_id_errors_per_ip` from only the first configured
UDP listener. In `src/bootstrap/app.rs::setup`, the exact log expression is
`configuration.clone().mask_secrets().to_json()`: V3 `mask_secrets` masks the database, then
explicitly descends into root `http_api`, and `to_json` serializes only that masked clone.
- **Conclusion:** The split schema encodes cardinality/defaulting structurally and is distinct from
the existing role-grouped runtime lifecycle. Any future normalizer needs one ownership point for
ID allocation, health defaulting, singleton validation, and shared UDP policy. It must retain
post-bind registration and redaction behavior. Any enum-based schema must exhaustively traverse
secret-bearing variants before JSON serialization at this existing log boundary.
- **Report Links:** `analysis.md` sections "Current-State Baseline" and "Runtime and Normalization Model".

## E2: Configuration Representation Feasibility

- **Question:** Which TOML/Rust enum representations parse, serialize, validate, and support
required configuration-source behavior?
- **Status:** TODO
- **Method:** TODO
- **Observation:** TODO
- **Conclusion:** TODO
- **Status:** PASS
- **Method:** Added test-only local types under
`packages/configuration/src/v3_0_0/mod.rs::tests::flat_service_configuration_prototype`.
Ran:

`cargo test -p torrust-tracker-configuration flat_service_configuration_prototype -- --nocapture`

Adjacent-tagged TOML input:

```toml
[[services]]
kind = "http_tracker"
[services.configuration]
bind_address = "127.0.0.1:17070"

[[services]]
kind = "udp_tracker"
[services.configuration]
bind_address = "127.0.0.1:16969"
```

Flat-list indexed override input:

```text
TORRUST_TRACKER_CONFIG_OVERRIDE_SERVICES__0__CONFIGURATION__BIND_ADDRESS=127.0.0.1:18080
```

Equivalent split-list indexed override input:

```text
TORRUST_TRACKER_CONFIG_OVERRIDE_HTTP_TRACKERS__0__BIND_ADDRESS=127.0.0.1:18080
```

- **Observation:** The focused prototype suite covers ten tests. Adjacent, flattened/internal-tagged,
and externally tagged forms round-trip through TOML and Serde. The adjacent fixture round-trips a
nested HTTP tracker `network` block and `tls_config`, plus HTTP API `access_tokens` and `tls_config`.
A separate enum traversal prototype masks the HTTP API token before JSON serialization, proving the
required redaction ordering for that variant. Adjacent tagging rejects an unknown kind. Omitted and
empty lists deserialize as empty; duplicate singleton kinds need semantic validation. Both flat and
equivalent split-list indexed Figment overrides fail extraction with a matched Figment
`InvalidType(Map, "a sequence")`; the current named nested HTTP API override remains covered by an
existing test.
- **Conclusion:** An adjacent enum is technically feasible for the nested v3 fields exercised and
shares the current Figment limitation for indexed listener overrides. Complete secret redaction
remains feasible only with an exhaustive enum traversal before the existing JSON logging boundary.
It transfers singleton/default behavior from structure to custom normalization/validation.
Flattened and external forms are feasible but less operator-friendly.
- **Report Links:** `analysis.md` sections "Candidate Representations" and "Feasibility Results".

## E3: Runtime and Identity Model

- **Question:** Can one normalization model preserve role-local IDs, container lookups, startup
dependencies, registration, and metrics behavior for interleaved services?
- **Status:** TODO
- **Method:** TODO
- **Observation:** TODO
- **Conclusion:** TODO
- **Status:** PASS
- **Method:** Traced `src/container.rs::{initialize,
initialize_http_tracker_instance_containers,initialize_udp_tracker_instance_containers}` and
`src/app.rs::{start_jobs,start_udp_tracker_services,start_the_http_instances,start_the_http_api}`.
Reviewed prototype test `role_local_ids_remain_stable_when_another_role_precedes_a_service`.
- **Observation:** Existing container construction assigns IDs beside per-role containers, and
jobs retrieve those containers by role-local index. The prototype scans interleaved services and
yields `UdpTracker(0)`, `HttpTracker(0)`, `UdpTracker(1)`, `HttpTracker(1)`. Startup is grouped
by dependency rather than declaration order. `Registar` already inventories started listeners;
the job manager includes both listeners and non-listener jobs.
- **Conclusion:** A flat source list can preserve existing IDs only through one scanner with
role-specific counters. Global list positions are incompatible. An internal normalizer is
possible without a flat TOML schema, but no current consumer demonstrates that it is required.
- **Report Links:** `analysis.md` sections "Runtime and Normalization Model" and "Identity, Ordering, and Migration".

## E4: Migration, Schema Lifecycle, and Security
## E4: Successor-Schema Lifecycle and Security

- **Question:** What migration order, schema transition policy, dependency order, and redaction
constraints would a successor schema require?
- **Status:** TODO
- **Method:** TODO
- **Observation:** TODO
- **Conclusion:** TODO
- **Question:** What successor-schema transition policy, dependency order, and redaction constraints
would a future flat layout require after the v3 delivery completes?
- **Status:** PASS
- **Method:** Reviewed `packages/configuration/src/lib.rs`, v3 load/default/version checks,
`src/bootstrap/app.rs`, #2079 at
`docs/issues/open/2079-adopt-secrecy-for-sensitive-configuration.md`, #1490 at
`docs/issues/open/1490-1978-decompose-database-configuration.md`, and #1978 at
`docs/issues/open/1978-configuration-overhaul-epic/EPIC.md`.
- **Observation:** Current v3 loading accepts a single exact schema version, while production
consumers remain v2 until #1980. The #2079 secrecy prerequisite precedes #1490, and both
precede #1980. The current roadmap classifies #2067 as non-blocking post-v3 research. The split
layout has no cross-role ordering; a future flat migration would fabricate the approved HTTP,
UDP, REST, health order. A flat enum would require new redaction traversal, migration guidance,
default files, fixture updates, and an override solution.
- **Conclusion:** A dual loader or migration tool adds cost without an operator benefit. Retaining
the split layout lets #2079, #1490, and #1980 proceed without redoing their consumer migration.
No schema implementation follow-up is warranted.
- **Report Links:** `analysis.md` sections "Identity, Ordering, and Migration" and "Schema Lifecycle, Security, and Compatibility".

## E5: Final Report Review

- **Question:** Does every material recommendation in `analysis.md` have sufficient evidence,
and does the recommendation remain analysis-only?
- **Status:** TODO
- **Method:** TODO
- **Observation:** TODO
- **Conclusion:** TODO
- **Status:** PASS
- **Method:** Checked every required `analysis.md` section against E1–E4 and confirmed the
prototype is restricted to `#[cfg(test)]` test-local types with no production schema/runtime
behavior changes.
- **Observation:** The report compares three TOML representations plus internal normalization,
provides reproducible test input and commands, identifies the Figment limit, preserves the
identity/lifecycle constraints, and issues one recommendation.
- **Conclusion:** The decision record is traceable and remains analysis-only. The recommendation
is to reject a flat TOML schema change and defer any internal normalizer until a real consumer
need exists.
- **Report Links:** `analysis.md` section "Executive Decision" and "Cost, Risks, and Recommendation".
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ not yet demonstrated, while the migration and runtime integration cost is alread
- Current UDP configuration exposes an important semantic mismatch: the shared `BanService` is
initialized from one UDP listener's `max_connection_id_errors_per_ip` value. A flat list could
make this policy less visible without resolving it.
- A v3-to-successor migration cannot recover a meaningful cross-role order, because v3 stores
- Materializing a flat v3 collection from the current split layout cannot recover a meaningful cross-role order, because the split layout stores
independent HTTP and UDP lists rather than one interleaved inventory. Any migration must impose
a canonical order or require operator intervention.
- The change must retain defaulting, environment overrides, configuration serialization, and
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
# Confirmed Bug: UDP Connection-ID Error Limit Is Mis-scoped

> **Status:** Confirmed during issue #2067 analysis; no fix is included here.
>
> **Parent analysis:** [ISSUE.md](ISSUE.md)
>
> **Decision record:** [analysis.md](analysis.md)

## Summary

`max_connection_id_errors_per_ip` is declared on every UDP listener configuration, implying that
each `[[udp_trackers]]` entry can control its own connection-ID error limit. The runtime does not
honor that meaning. It reads only the first configured UDP listener's value, then constructs one
shared `BanService` used by every UDP listener in the process.

This is a configuration-model bug: either a value is listener-specific and every listener must
receive an independent service configured with its own value, or it controls a shared service and
must be represented once as shared/global configuration. The current first-entry-wins behavior is
neither model and makes security behavior depend silently on configuration order.

## Reproduction

The following values imply two different listener policies:

```toml
[[udp_trackers]]
bind_address = "127.0.0.1:6969"
max_connection_id_errors_per_ip = 1

[[udp_trackers]]
bind_address = "127.0.0.1:6970"
max_connection_id_errors_per_ip = 100
```

`src/container.rs` selects the first entry's value:

```rust
let max_connection_id_errors = configuration
.udp_trackers
.as_ref()
.and_then(|trackers| trackers.first())
.map_or(default_max_connection_id_errors, |config| {
config.max_connection_id_errors_per_ip
});
```

It passes that one value to `UdpTrackerCoreServices::initialize_from`. That function creates one
`Arc<RwLock<BanService>>`, and each `UdpTrackerCoreContainer` receives a clone of the same arc.
Consequently both listeners use the limit `1`; the second listener's configured `100` is ignored.
Reordering the TOML entries changes the application-wide limit without changing the shared-service
design.

## Evidence

| Fact | Source |
| ----------------------------------------------- | ------------------------------------------------------------------------------------------------------ |
| Field is placed on each listener | `packages/configuration/src/v2_0_0/udp_tracker.rs`, `packages/configuration/src/v3_0_0/udp_tracker.rs` |
| First UDP listener value is selected | `src/container.rs::AppContainer::initialize` |
| One shared ban service is created | `packages/udp-core/src/container.rs::UdpTrackerCoreServices::initialize_from` |
| All UDP containers clone that service | `packages/udp-core/src/container.rs::UdpTrackerCoreContainer::initialize_from_services` |
| Shared ban state is intentional security design | `docs/adrs/20260727180000_shared_services_across_tracker_instances.md` |

The ADR explicitly states that settings affecting shared services must themselves be global and
uses global `connection_id_validation` as its example. The same reasoning applies to the error
limit held by the shared `BanService`.

## Recommended Follow-up Scope

Create a separate bug sub-issue of EPIC #1978. Its preferred correction is:

1. Move `max_connection_id_errors_per_ip` from `UdpTracker` to the shared
`UdpTrackerServer` configuration.
2. Remove the per-listener field from the active v3 schema, defaults, fixtures, documentation, and
constructors, coordinating the change with the planned v2-to-v3 consumer migration.
3. Make `AppContainer` pass the one shared `udp_tracker_server` value to
`UdpTrackerCoreServices::initialize_from`.
4. Add tests proving that multiple UDP listeners use the same declared global limit and that
configuration order cannot change it.
5. Update the v2-to-v3 migration guidance because the field moves from each listener to the shared
section.

Do not implement this bug fix as part of #2067. The next step is to draft and review a dedicated
sub-issue specification before creating its GitHub issue.

## Rejected Interim Option

Validating that every listener repeats the same value would prevent inconsistent input but would
still duplicate one global policy in every listener block. It is an inferior schema because it
retains ambiguity and raises maintenance cost. The field should be represented once where the
shared service is configured.
Loading
Loading