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
23 changes: 20 additions & 3 deletions docs/issues/287-docker-compose-topology-refactoring-epic.md
Original file line number Diff line number Diff line change
Expand Up @@ -21,18 +21,29 @@ From [docs/refactors/plans/docker-compose-topology-domain-model.md](../refactors
- Derive global network lists from service configurations (single source of truth)
- Move topology decision logic from template conditionals to domain layer
- Enforce invariants like "if a service uses a network, that network must be defined"
- Move port exposure logic from templates to domain (Phase 3 - added post-completion)

## Tasks

Implementation follows a 5-PR strategy:
Implementation follows a 5-PR strategy (original scope), with Phase 3 added as a continuation:

### Original 5-PR Strategy (Completed)

- [x] [#288](https://github.com/torrust/torrust-tracker-deployer/issues/288) - [ADR] Bind Mount Standardization for Docker Compose (ADR-01)
- [x] [#290](https://github.com/torrust/torrust-tracker-deployer/issues/290) - [BUG] Remove invalid "Grafana without Prometheus" template branch (BUG-01)
- [x] [#292](https://github.com/torrust/torrust-tracker-deployer/issues/292) - [Refactor] Phase 0: Convert volumes to bind mounts with domain type (P0.1, P0.2)
- [x] [#294](https://github.com/torrust/torrust-tracker-deployer/issues/294) - [Refactor] Phase 1: Create Network domain types (P1.1, P1.2)
- [ ] [#296](https://github.com/torrust/torrust-tracker-deployer/issues/296) - [Refactor] Phase 2: Create DockerComposeTopology aggregate (P2.1, P2.2)
- [x] [#296](https://github.com/torrust/torrust-tracker-deployer/issues/296) - [Refactor] Phase 2: Create DockerComposeTopology aggregate (P2.1, P2.2) → [PR #297](https://github.com/torrust/torrust-tracker-deployer/pull/297)

### Phase 3: Port Topology (Added Extension)

> **Note**: Phase 3 was part of the original refactoring plan analysis (see PORT-01 through PORT-11 rules in [docker-compose-topology-domain-model.md](../refactors/plans/docker-compose-topology-domain-model.md#port-exposure-rules)) but was not included in the initial 5-PR strategy. It follows the same pattern as networks and completes the topology domain model.

(Tasks will be created and linked as work progresses)
- [ ] Phase 3: Create Port Topology Domain Model (P3.1, P3.2, P3.3, P3.4) → [Draft](drafts/298-phase-3-port-topology-domain-model.md)
- P3.1: Create Port domain types (`PortBinding`, `Protocol`)
- P3.2: Extend `ServiceTopology` with port derivation
- P3.3: Add cross-service port conflict validation
- P3.4: Update template to use derived ports with descriptions

## PR Dependencies

Expand All @@ -47,6 +58,9 @@ PR 4 (Phase 1)
PR 5 (Phase 2)
PR 6 (Phase 3) ◄─── Extension: Port topology (added post-completion)
```

## Key Decisions
Expand All @@ -55,8 +69,11 @@ PR 5 (Phase 2)
- Type-safe `MountOption` enum (ReadOnly, SELinux)
- Domain-driven network derivation (single source of truth)
- ~47 domain rules identified with test specifications
- Network descriptions rendered as YAML comments for sysadmin documentation
- Port topology follows same pattern as networks (Phase 3)

## Related

- Plan Review: #285 (merged)
- Refactoring Plan: [docs/refactors/plans/docker-compose-topology-domain-model.md](../refactors/plans/docker-compose-topology-domain-model.md)
- Phase 3 Draft: [drafts/298-phase-3-port-topology-domain-model.md](drafts/298-phase-3-port-topology-domain-model.md)
11 changes: 6 additions & 5 deletions docs/issues/296-phase-2-topology-aggregate.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# [Refactor] Phase 2: Create DockerComposeTopology Aggregate

**Issue**: [#296](https://github.com/torrust/torrust-tracker-deployer/issues/296)
**Pull Request**: [#297](https://github.com/torrust/torrust-tracker-deployer/pull/297)
**Parent Epic**: [#287](https://github.com/torrust/torrust-tracker-deployer/issues/287) - Docker Compose Topology Domain Model Refactoring
**Related**:

Expand All @@ -13,11 +14,11 @@ This task creates the `DockerComposeTopology` aggregate and derives required net

## Goals

- [ ] Create `DockerComposeTopology` aggregate with service topology collection
- [ ] Create `Service` enum for type-safe service identification
- [ ] Derive required networks from service configurations (single source of truth)
- [ ] Remove conditional network logic from docker-compose template
- [ ] Enforce invariant: "if a service uses a network, that network must be defined"
- [x] Create `DockerComposeTopology` aggregate with service topology collection
- [x] Create `Service` enum for type-safe service identification
- [x] Derive required networks from service configurations (single source of truth)
- [x] Remove conditional network logic from docker-compose template
- [x] Enforce invariant: "if a service uses a network, that network must be defined"

## 🏗️ Architecture Requirements

Expand Down
250 changes: 250 additions & 0 deletions docs/issues/drafts/298-phase-3-port-topology-domain-model.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,250 @@
# [Refactor] Phase 3: Create Port Topology Domain Model

**Epic**: [#287](https://github.com/torrust/torrust-tracker-deployer/issues/287) - Docker Compose Topology Domain Model Refactoring
**Related Plan**: [docs/refactors/plans/docker-compose-topology-domain-model.md](../../refactors/plans/docker-compose-topology-domain-model.md)
**Status**: Draft

## Overview

Move port exposure logic from Tera templates to the domain layer, following the same pattern used for networks in Phase 1-2. This creates type-safe port definitions with descriptions that render as inline YAML comments for sysadmin self-documentation.

## Background

This phase was originally part of the refactoring plan (see PORT-01 through PORT-11 rules in the plan) but was not included in the initial 5-PR strategy. The port exposure rules identified in the plan are currently scattered between Rust code and Tera templates, creating the same dual-source-of-truth problem that was solved for networks.

## Problem Statement

### Current State

Port exposure logic is split across multiple locations:

1. **Template conditionals** - Complex `{%- if %}` blocks for TLS-based port exposure
2. **Rust filtering** - `http_tracker_ports_without_tls` computed in context builder
3. **Hardcoded ports** - Caddy (80, 443), Prometheus (9090), MySQL (none)

Example from template:

```tera
{%- if tracker.needs_ports_section %}
ports:
{%- for udp_tracker in tracker.udp_trackers %}
- "{{ udp_tracker.port }}:{{ udp_tracker.port }}/udp"
{%- endfor %}
{%- for http_tracker_port in tracker.http_tracker_ports_without_tls %}
- "{{ http_tracker_port }}:{{ http_tracker_port }}"
{%- endfor %}
{%- if not tracker.http_api_has_tls %}
- "{{ tracker.http_api_port }}:{{ tracker.http_api_port }}"
{%- endif %}
{%- endif %}
```

### Problems

1. **Logic in templates** - Port exposure decisions based on TLS configuration
2. **No cross-service validation** - Cannot detect host port conflicts across services
3. **Missing documentation** - Rendered YAML lacks context for what each port does
4. **Existing validation is tracker-only** - `TrackerConfig` validates internal socket conflicts but not Docker Compose host port conflicts

### Desired State

```yaml
services:
tracker:
ports:
# BitTorrent UDP announce (external)
- "6969:6969/udp"
# HTTP tracker announce (no TLS)
- "7070:7070"
# REST API for stats/whitelist (no TLS)
- "1212:1212"
```

## Proposed Solution

### Domain Types

Create port topology types that complement the existing network topology:

```rust
// src/domain/topology/port.rs

/// Network protocol for port bindings
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum Protocol {
Tcp,
Udp,
}

/// A port binding in Docker Compose
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct PortBinding {
/// Host port (None = internal only, not exposed)
host_port: Option<u16>,
/// Container port
container_port: u16,
/// Protocol (TCP or UDP)
protocol: Protocol,
/// Host IP to bind to (None = 0.0.0.0)
host_ip: Option<IpAddr>,
}

impl PortBinding {
/// Short description of what this port does
pub fn description(&self) -> &'static str {
// Derived from context (service type + port purpose)
}
}
```

### ServiceTopology Extension

Extend `ServiceTopology` to include port definitions:

```rust
// src/domain/topology/aggregate.rs

pub struct ServiceTopology {
service: Service,
networks: Vec<Network>,
ports: Vec<PortBinding>, // NEW
}

impl ServiceTopology {
/// Derives ports from service configuration
pub fn ports(&self) -> &[PortBinding] {
&self.ports
}
}
```

### Aggregate Validation

Add cross-service port conflict detection to `DockerComposeTopology`:

```rust
impl DockerComposeTopology {
/// Validates no host port conflicts across all services
fn validate_port_uniqueness(&self) -> Result<(), TopologyError> {
// Collect all exposed (host_port, protocol) pairs
// Error if duplicates found
}
}
```

### Relationship with TrackerConfig Validation

| Validation | Location | Scope |
| -------------------------------------------------------------- | ----------------------------------- | ---------------------------- |
| Socket address uniqueness (UDP tracker vs HTTP tracker vs API) | `TrackerConfig::new()` | Tracker application internal |
| Host port uniqueness (Tracker vs Grafana vs Caddy) | `DockerComposeTopology::validate()` | Docker Compose deployment |

**Key insight**: `TrackerConfig` validates what the Torrust Tracker binary can bind to. `DockerComposeTopology` validates what Docker Compose can expose to the host.

## Port Rules from Original Plan

These rules (PORT-01 through PORT-11) will be moved to domain:

| Rule ID | Rule Description | Target Location |
| ------- | ------------------------------------------------------------------------- | --------------------------------------- |
| PORT-01 | Tracker needs ports section if UDP OR HTTP without TLS OR API without TLS | `ServiceTopology::ports()` |
| PORT-02 | UDP ports always exposed (no TLS) | `PortBinding` for UDP tracker |
| PORT-03 | HTTP ports WITHOUT TLS exposed directly | `PortBinding` with host_port |
| PORT-04 | HTTP ports WITH TLS NOT exposed (Caddy handles) | No `PortBinding` created |
| PORT-05 | API exposed only when no TLS | Conditional `PortBinding` |
| PORT-06 | API NOT exposed when TLS | No `PortBinding` created |
| PORT-07 | Grafana 3000 exposed only without TLS | Conditional `PortBinding` |
| PORT-08 | Grafana 3000 NOT exposed with TLS | No `PortBinding` created |
| PORT-09 | Caddy always exposes 80, 443, 443/udp | Fixed `PortBinding`s |
| PORT-10 | Prometheus 9090 on localhost only | `PortBinding` with `host_ip: 127.0.0.1` |
| PORT-11 | MySQL no exposed ports | Empty `ports` in `ServiceTopology` |

## Template Simplification

After refactoring:

```tera
services:
{{ service.name }}:
{%- if service.ports | length > 0 %}
ports:
{%- for port in service.ports %}
# {{ port.description }}
- "{{ port.binding }}"
{%- endfor %}
{%- endif %}
```

## Tasks

### P3.1: Create Port Domain Types

- [ ] Create `Protocol` enum (or reuse from `domain/tracker`)
- [ ] Create `PortBinding` struct with description support
- [ ] Add `PortDefinition` context type for template rendering
- [ ] Add unit tests for port binding creation

### P3.2: Extend ServiceTopology with Ports

- [ ] Add `ports: Vec<PortBinding>` to `ServiceTopology`
- [ ] Implement port derivation for each service type:
- Tracker: UDP ports, HTTP ports (TLS-dependent), API (TLS-dependent)
- Caddy: 80, 443, 443/udp (fixed)
- Prometheus: 9090 (localhost)
- Grafana: 3000 (TLS-dependent)
- MySQL: none
- [ ] Add unit tests for each service's port derivation

### P3.3: Add Cross-Service Port Validation

- [ ] Implement `DockerComposeTopology::validate_port_uniqueness()`
- [ ] Create `TopologyError::PortConflict` variant
- [ ] Add tests for conflict detection scenarios

### P3.4: Update Template and Context

- [ ] Create `PortDefinition` with `binding()` and `description()` for template
- [ ] Update `DockerComposeContext` to use derived ports
- [ ] Simplify `docker-compose.yml.tera` port sections
- [ ] Remove conditional port logic from template

## Acceptance Criteria

- [ ] All PORT-\* rules from refactoring plan are implemented in domain
- [ ] Port descriptions render as YAML comments in output
- [ ] Cross-service port conflicts are detected and reported with actionable error
- [ ] Template has no conditional port logic (just loops)
- [ ] No duplication with `TrackerConfig` validation (different scopes)
- [ ] All existing E2E tests pass
- [ ] Pre-commit checks pass

## Testing Strategy

### Unit Tests

- Port derivation for each service type
- TLS-dependent port inclusion/exclusion
- Port conflict detection
- Description generation

### Integration Tests

- Context builder produces correct port definitions
- Template renders ports with descriptions

### E2E Tests

- Existing tests continue to pass (no behavioral change)

## Out of Scope

- Volume topology (no cross-service invariants currently)
- Service dependency ordering (separate concern)
- Health check configuration

## References

- [Original refactoring plan - Port rules](../../refactors/plans/docker-compose-topology-domain-model.md#port-exposure-rules)
- [TrackerConfig validation](../../../src/domain/tracker/config/mod.rs) - Internal socket conflict validation
- [Network topology implementation (Phase 1-2)](./287-docker-compose-topology-refactoring-epic.md)
- [UDP/TCP port sharing documentation](../../external-issues/tracker/udp-tcp-port-sharing-allowed.md)
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,37 @@ This would require passing the version to the template context.
- [Template System Architecture](../../docs/contributing/templates/template-system-architecture.md)
- [Tera Template Guide](../../docs/contributing/templates/tera.md)

## Already Implemented: Inline Network Descriptions

As a first step toward self-documenting rendered files, we've implemented inline network descriptions in docker-compose.yml. The `Network` enum now has a `description()` method that provides short purpose strings for each network type:

| Network | Description |
| ------------- | ------------------------------------------- |
| Database | `Database isolation: Tracker ↔ MySQL` |
| Metrics | `Metrics scraping: Tracker ↔ Prometheus` |
| Visualization | `Dashboard queries: Prometheus ↔ Grafana` |
| Proxy | `TLS termination: Caddy ↔ backend services` |

These descriptions are rendered as YAML comments in the generated docker-compose.yml:

```yaml
networks:
# Database isolation: Tracker ↔ MySQL
db_network:
driver: bridge
# Metrics scraping: Tracker ↔ Prometheus
metrics_network:
driver: bridge
```

**Implementation details:**

- `src/domain/topology/network.rs` - `Network::description()` method
- `context/network_definition.rs` - `NetworkDefinition.description` field
- `templates/docker-compose/docker-compose.yml.tera` - Renders `# {{ net.description }}`

This pattern can be extended to other configuration elements (services, volumes, etc.) as we implement the full documentation headers.

## Open Questions

1. Should we include the deployer version in the header?
Expand Down
Loading
Loading