Analyze flat service configuration

[josecelano]

Summary

Completes the evidence-led analysis for #2067 and recommends retaining the role-specific operator-facing TOML layout.

• Adds isolated TOML/Serde/Figment feasibility prototypes for flat service representations.
• Documents why list declaration order must not control startup and how role-local ConfigurationInstanceId values remain compatible.
• Adds a concrete explanation of the numeric Figment sequence-override limitation.
• Records the confirmed max_connection_id_errors_per_ip configuration-model bug for separate follow-up.

Validation

• RUSTUP_TOOLCHAIN=stable ./contrib/dev-tools/git/hooks/pre-commit.sh --format=text
• Focused configuration representation prototypes

Related to #2067.

[Copilot]

Pull request overview

Analyzes flat heterogeneous service configuration and recommends retaining the role-specific TOML layout without production behavior changes.

Changes:

• Adds TOML/Serde/Figment feasibility prototypes.
• Documents identity, ordering, migration, security, and operator-UX considerations.
• Records the shared UDP connection-ID limit bug for follow-up.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Summary
packages/configuration/src/v3_0_0/mod.rs	Feasibility prototypes and tests; nested configuration coverage and concrete Figment error assertions need completion.
docs/issues/open/2067-1978-analyze-flat-service-configuration/max-connection-id-errors-per-ip-bug.md	Documents the configuration-model bug.
docs/issues/open/2067-1978-analyze-flat-service-configuration/ISSUE.md	Updates issue scope and acceptance tracking; stale v3/v4 execution-contract statements need reconciliation.
docs/issues/open/2067-1978-analyze-flat-service-configuration/evidence.md	Provides the evidence ledger.
docs/issues/open/2067-1978-analyze-flat-service-configuration/analysis.md	Presents the recommendation and trade-offs; logging/JSON serialization analysis and effort estimates need completion.
docs/issues/open/1978-configuration-overhaul-epic/EPIC.md	Updates the configuration roadmap.

Suppressed comments (9)

docs/issues/open/1978-configuration-overhaul-epic/EPIC.md:186

• The delivery strategy places #2067 in Phase 4 after the #1980 integration phase, while these same lines say that an approved implementation must be created after #1490 and before #1980. That makes the roadmap's decision gate occur after the release milestone it is supposed to inform. Please reconcile the phase/order/dependency presentation, or retain the original post-v3 policy consistently.

- **Subissue #13** (#2067) — Analyze whether v3 should retain role-specific operator-facing TOML,
  introduce a heterogeneous listener collection, or retain the TOML layout while normalizing to an
  internal polymorphic service inventory. This analysis does not implement a schema or runtime
  change. If approved, its separately created implementation sub-issue follows #1490 and precedes
  #1980 so that the final consumer migration targets the selected v3 model.

docs/issues/open/2067-1978-analyze-flat-service-configuration/ISSUE.md:296

• The required-section checklist still asks for analysis of loss of cross-role ordering during a v3-to-v4 migration, while the updated scope explicitly moved the candidate into v3.0.0 and says there is no successor schema. Update this checklist item to the selected v3 transition so the completion claim is internally consistent.

7. **Schema Lifecycle, Security, and Compatibility**: v3 loading and transition policy, the #1490 → implementation → #1980 relationship, secret redaction, external configuration consumers, and observability compatibility.

docs/issues/open/2067-1978-analyze-flat-service-configuration/ISSUE.md:383

• The issue marks the analysis deliverables complete, but first-impressions.md still leaves its required Reassessment Record at TODO (line 88). That document says to append the final comparison without rewriting the snapshot; add the outcome entry before checking off analysis completion.

- [x] Analysis completed; no production schema change included
- [x] `analysis.md` completed with an explicit recommendation
- [x] `evidence.md` completed with reproducible evidence for each material conclusion

docs/issues/open/2067-1978-analyze-flat-service-configuration/analysis.md:75

• The external-tagged prototype at packages/configuration/src/v3_0_0/mod.rs:1307-1311 contains only one http_tracker item, so it does not demonstrate round-tripping a heterogeneous/interleaved external-tagged list. Either add another variant to that experiment or narrow this statement to the single-item case; otherwise the representation comparison overstates the verified evidence.

Isolated tests using the repository's `toml`, Serde, and Figment versions confirm that adjacent,
flattened, and externally tagged enum forms parse and serialize an interleaved service document.

docs/issues/open/2067-1978-analyze-flat-service-configuration/max-connection-id-errors-per-ip-bug.md:76

• The proposed correction is not implementable in the stated roadmap sequence as written: src/container.rs still consumes the global v2 Configuration alias, and v2 has no udp_tracker_server field. A follow-up limited to moving the field in the final v3 schema cannot make AppContainer pass udp_tracker_server until consumer migration occurs. Explicitly include the v2/runtime transition or make #1980 a prerequisite/part of the follow-up scope.

1. Move `max_connection_id_errors_per_ip` from `UdpTracker` to the shared
   `UdpTrackerServer` configuration.
2. Remove the per-listener field from the final v3 schema, defaults, fixtures, documentation, and
   constructors.
3. Make `AppContainer` pass the one shared `udp_tracker_server` value to
   `UdpTrackerCoreServices::initialize_from`.

packages/configuration/src/v3_0_0/mod.rs:1179

• This new dead_code suppression has no explanation, even though the type is intentionally used only as the target of a failing extraction and its field is never read. Project review guidance requires #[allow(...)] suppressions to be justified; add a short comment explaining this test-only purpose (or consume the field instead of suppressing the lint).

        #[allow(dead_code)]

packages/configuration/src/v3_0_0/mod.rs:1342

• This only verifies serde(default) on the services field through direct toml::from_str; it never exercises the supported Figment join(Serialized::defaults(...)) path or verifies omitted per-service defaults. E2 and AC3 require Figment defaulting evidence, so add a Figment extraction that asserts the candidate's default behavior (while separately documenting that health materialization belongs to normalization).

            let omitted: AdjacentServicesDocument = toml::from_str("").expect("omitted services should deserialize");
            let empty: AdjacentServicesDocument = toml::from_str("services = []").expect("empty services should deserialize");

            assert!(omitted.services.is_empty());
            assert!(empty.services.is_empty());

packages/configuration/src/v3_0_0/mod.rs:1184

• This comparison fixture is not the current split schema: v3 declares http_trackers as Option<Vec<HttpTracker>> (and the global runtime still consumes the v2 equivalent), whereas this prototype uses a required Vec. Since Figment's merge/extraction behavior can differ for an optional field, E2 does not establish the claimed equivalent current override behavior. Mirror the actual Option<Vec<_>> shape or extract a valid production Configuration instead.

        struct SplitServicesDocument {
            #[serde(default)]
            http_trackers: Vec<HttpTracker>,
        }

packages/configuration/src/v3_0_0/mod.rs:1438

• This split-list experiment has the same evidence gap: is_err() does not establish that the failure comes from Figment treating HTTP_TRACKERS__0__... as a map rather than from some unrelated extraction error. Add a successful no-override baseline and assert or record the concrete error before using it as evidence for the current provider limitation.

                assert!(
                    result.is_err(),
                    "Figment environment maps cannot override a split sequence item by numeric index"
                );

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[josecelano]

Addressed the five Copilot review findings in signed commit 79cd5f82.

• Reconciled Analyze a flat heterogeneous service configuration #2067 and the Overhaul: Configuration (schema v3.0.0) #1978 roadmap to the final-v3, before-Configuration overhaul final cleanup: remove global re-exports, migrate consumers to explicit versioned imports #1980 decision point.
• Added explicit analysis and a test-only proof for masking enum-carried API tokens before JSON configuration logging.
• Added qualitative effort estimates for the rejected flat schema and deferred normalizer.
• Expanded nested TOML round-trip coverage for network, tls_config, and access_tokens.
• Asserted Figment's structured InvalidType(Map, "a sequence") error for both indexed list override variants.

Validation passed: RUSTUP_TOOLCHAIN=stable cargo fmt --check, focused prototype tests (10), RUSTUP_TOOLCHAIN=stable cargo test -p torrust-tracker-configuration (98), and the mandatory pre-commit gate.

[josecelano]

Roadmap correction after merging current develop in signed commit 2b996ab3:

The earlier status comment described the then-current final-v3 / before-#1980 plan. develop is now the source of truth: #2067 is non-blocking post-v3 research and any future implementation must be separately scheduled after #1980 and the #2079 → #1490 prerequisites.

The five review remediations remain present and validated:

• lifecycle wording is now consistent with the post-v3 roadmap;
• the masked to_json logging boundary and enum redaction proof are documented;
• qualitative estimates are included;
• nested network, tls_config, and access_tokens round trips are covered;
• both numeric Figment list override tests match InvalidType(Map, "a sequence").

The follow-up markdown lint fix is in signed commit 11ec94f5; the repository pre-commit gate, including all linters, passed locally.

[josecelano]

ACK 11ec94f