Skip to content

Commit 746de92

Browse files
authored
feat: CommonDevice → OpnSenseDocument reverse serializer (#9)
## Summary opnConfigGenerator now implements the **reverse serializer** that opnDossier doesn't ship. opnDossier parses `config.xml → *model.CommonDevice`; this PR delivers the missing inverse direction: `faker → *model.CommonDevice → *opnsense.OpnSenseDocument → config.xml`. Zero-argument `opnConfigGenerator generate` emits a valid OPNsense `config.xml` that round-trips through opnDossier's `ConvertDocument` with zero warnings. **Impact:** 57 files changed, +2,513 / −3,615 lines (net −1,102). One new dependency (`gofakeit/v7`). Two new packages, one rewritten package, two deleted packages. **Risk level:** 🟡 Medium — architectural pivot with breaking CLI changes; mitigated by round-trip acceptance test, byte-stability tests, fuzz tests, and cross-platform CI green. **Review time:** ~60–90 min (11 commits; recommend reading commit-by-commit). ## Type of Change - [x] **New feature** — the `CommonDevice → OpnSense` serializer is a net-new deliverable - [x] **Refactor** — CLI reshape + transport-only `opnsensegen` - [x] **Breaking change** — CLI flag surface changed (see below) - [x] **Documentation** — README, CONTRIBUTING, GOTCHAS, and new `docs/solutions/` entry ## What Changed ### 🔧 New packages - **`internal/faker/`** (7 files + tests) — produces `*model.CommonDevice` via `NewCommonDevice(opts...)`. Functional options: `WithSeed`, `WithVLANCount`, `WithFirewallRules`, `WithHostname`, `WithDomain`, `WithDeviceType`. Target-neutral by default. Uses `gofakeit/v7` + `math/rand/v2` PCG for seeded determinism. - **`internal/serializer/opnsense/`** (7 files + tests) — `Serialize(device) → *opnsense.OpnSenseDocument` and `Overlay(base, device) → *opnsense.OpnSenseDocument` for the `--base-config` path. One file per CommonDevice subsystem; package layout reserves an `internal/serializer/pfsense/` sibling for a future plan. ### 🔧 Rewritten packages - **`internal/opnsensegen/template.go`** — transport-only (`LoadBaseConfig`, `ParseConfig`, `MarshalConfig`). `MarshalConfig` post-processes XML to sort children of map-backed sections (`<interfaces>`, `<dhcpd>`) alphabetically so output is byte-stable under a fixed seed. Atomic single-write contract: encode errors leave the destination untouched. - **`internal/csvio/csvio.go`** — consumes `*model.CommonDevice` instead of `[]generator.VlanConfig`. German headers preserved. - **`cmd/generate.go`** — zero-argument emission, new flag surface, overlay routing helper. ### 🗑️ Deleted packages - `internal/generator/` (14 files) — dead code under the new pipeline. - `internal/validate/` (2 files) — only validated `generator.VlanConfig`. ### 📝 Documentation - `README.md` reframes the project as the opnDossier reverse serializer. - `CONTRIBUTING.md` adds an "Adding a New CommonDevice Subsystem" playbook. - `GOTCHAS.md` §7.1 documents the map-backed-section ordering workaround; §7.2 documents the round-trip-must-assert-per-field lesson. ## Breaking Changes ### Removed CLI flags `--count`, `--csv-file`, `--firewall-nr`, `--opt-counter`, `--include-firewall-rules`, `--firewall-rule-complexity`, `--vlan-range`, `--vpn-count`, `--nat-mappings`, `--wan-assignments`. ### New CLI surface | Flag | Default | Purpose | |------|---------|---------| | `--format` | `xml` (was required) | `xml` or `csv` | | `--vlan-count`/`-n` | `10` | Number of VLANs (0–4093) | | `--base-config` | *(unset)* | Optional overlay source; xml-only | | `--firewall-rules` | `false` | Emit default allow-per-interface rules | | `--seed` | `0` (random) | Deterministic PCG seed | | `--hostname`, `--domain` | *(unset)* | Override faker-generated values | ## Testing ### Round-trip contract `TestRoundTrip` in `internal/serializer/opnsense/serializer_test.go` is the primary acceptance gate. It exercises the full pipeline — `faker → Serialize → MarshalConfig → ParseConfig → ConvertDocument` — and asserts **per-field parity** (not just counts) on: - System: Hostname, Domain, Timezone, Language - Interfaces: Type, Virtual, Description, IPAddress, Subnet (keyed by Name) - VLANs: VLANIf, PhysicalIf, Description (keyed by Tag) - DHCP: Range.From/To, Gateway, DNSServer (keyed by Interface) - FirewallRules: Type, Description, IPProtocol, Direction, Protocol, Log, Disabled, Tracker, Source.Address/Port/Negated, Destination.Address/Port/Negated opnDossier returns zero `ConversionWarning`s on every round-trip. ### Byte stability `TestRoundTripByteStable` re-marshals the same input 20 times and asserts byte-identical output — a direct guard for `sortMapBackedSections` against Go's randomized map iteration. ### Determinism `TestGenerateDeterministicSeed` compares two `--seed 42` runs byte-for-byte. `TestNewRandZeroSeedIsRandom` samples 8 draws to drive flake probability to ~2⁻⁵¹². ### Fuzz + boundaries `TestNewCommonDeviceFuzzSeeds` iterates 200 distinct seeds at `--vlan-count 8` to catch adversarial-seed regressions. CLI boundary tests cover `--vlan-count 0`, `1`, `4094` (reject), malformed `--base-config` XML, and overlay wholesale-replace. ### Cross-platform CI green on `ubuntu-latest`, `macos-latest`, `windows-latest`, plus CodeQL and DCO checks. ## Review Checklist ### Architecture - [ ] `*model.CommonDevice` is the single intermediate representation; no parallel types - [ ] Faker is target-neutral (`WithDeviceType` option, not hardcoded) - [ ] Serializer package layout (`internal/serializer/opnsense/`) reserves pfSense sibling - [ ] Transport (`internal/opnsensegen/`) has no generation or serialization logic ### Correctness - [ ] Round-trip test asserts per-field parity on every Phase 1 subsystem - [ ] `sortMapBackedSections` covers every map-backed section in `OpnSenseDocument` (currently `interfaces`, `dhcpd`) - [ ] `MarshalConfig` buffers fully before first `Write` — no partial output on encode failure - [ ] `pickUnique*` loops return errors on exhaustion, not panic ### CLI - [ ] Zero-argument `generate` emits valid XML to stdout - [ ] `--base-config` only accepted with `--format xml` - [ ] `--seed N` produces byte-identical output across invocations - [ ] Flag help text matches actual bounds (0–4093) ### Tests - [ ] Per-subsystem unit tests assert `require.True` on map key presence before reading fields - [ ] Overlay test seeds Dhcpd + Filter in base with sentinel values and asserts they're dropped - [ ] Consumer round-trip test enables firewall rules and asserts they survive ### Docs - [ ] README describes current behavior (CLI hardwires OPNsense; DeviceType routing is planned) - [ ] GOTCHAS captures the two non-obvious behaviors (map ordering, silent round-trip drops) - [ ] CONTRIBUTING has a concrete "Adding a New CommonDevice Subsystem" procedure ## Risk Assessment | Factor | Rating | Details | |--------|--------|---------| | Size | 🟡 Medium | 57 files / ~6K line-diff; offset by 1,500+ deleted dead-code lines | | Complexity | 🟡 Medium | New token-stream XML stabilizer + functional-options faker | | Test coverage | 🟢 Low | Round-trip + byte-stability + 200-seed fuzz + boundary tests | | Dependencies | 🟢 Low | One new pure-Go lib (`gofakeit/v7`), no CGO | | Security | 🟢 Low | Tool generates synthetic data locally; no network calls, no secrets | | Breaking | 🟠 High | Removes 10 CLI flags; new project with no external consumers — acceptable | **Mitigations:** 1. Round-trip acceptance test fails loud on any field-level regression (learned the hard way — see `GOTCHAS §7.2`). 2. Byte-stability test defeats Go's map-iteration randomization. 3. Fuzz test covers 200 distinct seeds at moderate VLAN count. 4. Cross-platform CI (Linux/macOS/Windows) validates stdlib behavior differences. 5. Deferred subsystems (NAT, VPN, Users, Certs, …) are explicitly scoped out via pending todos — see "Known residuals" below. ## Pipeline Architecture ```mermaid flowchart LR CLI[cmd/generate] --> F[faker.NewCommonDevice] F --> CD[*model.CommonDevice] CD --> S[serializer/opnsense.Serialize] CD -- "--format csv" --> CSV[csvio.WriteVlanCSV] S --> D[*opnsense.OpnSenseDocument] BASE[--base-config file] --> LOAD[opnsensegen.LoadBaseConfig] LOAD --> OV[serializer/opnsense.Overlay] CD --> OV OV --> D D --> M[opnsensegen.MarshalConfig] M --> XML[(config.xml)] CSV --> OUT[(csv)] ``` ## Known Residuals (Pending Todos) Captured in `.context/compound-engineering/todos/` (gitignored, local-only) for follow-up triage. **None block merge** — they were surfaced by ce:review as `gated_auto` or `manual` and need a product call before implementation: | Priority | Area | What | |----------|------|------| | P1 | `cmd/root.go` | Orphan output file on serialize error — atomic temp-file + rename pattern | | P1 | `serializer/opnsense/overlay.go` | Overlay wholesale-replaces `System.User/Group` (merge vs warn+document decision) | | P2 | `serializer/opnsense/overlay.go` | Shallow-copy aliases caller's base (document or deep-copy) | | P2 | `cmd/validate.go` | `validate` subcommand still stub; wire to opnDossier or hide | | P2 | `cmd/generate.go` | Distinct exit codes per failure class | | P2 | `internal/faker/rand.go` | `--seed 0` sentinel ambiguity | | P3 | `internal/csvio/csvio.go` | Unconditional UTF-8 BOM breaks Go `csv.Reader` | | P3 | `cmd/root.go` | `--output -` stdout convention | Each will be addressed in a focused follow-up PR. ## Deferred Subsystems (Follow-up Plans) Phase 1 covers System, Interfaces, VLANs, DHCP, and default firewall rules. One plan per subsystem, landing as separate PRs: NAT · VPN (OpenVPN / WireGuard / IPsec) · Users/Groups · Certificates/CAs · IDS · HighAvailability · VirtualIPs · Bridges · GIF/GRE/LAGG · PPP · CaptivePortal · Kea DHCP · Monit · Netflow · TrafficShaper · Syslog forwarding · **pfSense target**. ## Test Plan (reviewer verification) - [x] `just ci-check` passes locally - [x] CI green on ubuntu/macos/windows + CodeQL + DCO + codecov - [ ] Reviewer manually confirms `go run . generate | head` emits valid XML - [ ] Reviewer manually confirms `go run . generate --seed 42` twice produces identical bytes - [ ] Reviewer manually confirms `go run . generate --base-config testdata/base-config.xml` preserves base fields outside Phase 1 scope - [ ] Reviewer reads GOTCHAS §7.1 + §7.2 before approving — these are load-bearing ## Commits (chronological) | SHA | Subject | |-----|---------| | `53131f3` | chore(deps): add gofakeit/v7 for CommonDevice faker | | `d5d8cae` | feat(faker): CommonDevice faker package | | `0460e3d` | feat(serializer/opnsense): CommonDevice to OpnSenseDocument serializer | | `cd3b179` | refactor: transport-only opnsensegen; csvio consumes CommonDevice | | `dd0f27c` | feat(cmd/generate): zero-arg config.xml emission | | `de4ffba` | refactor: delete generator and validate packages | | `b100924` | docs: reframe README as CommonDevice reverse serializer | | `54787a6` | fix: round-trip fidelity, stability hardening, and docs from ce:review | | `3756d96` | fix: surface faker exhaustion as error; harden XML stabilizer and tests | | `6b660fb` | Address PR review feedback (#9) — round 1 | | `2f615de` | Address PR review feedback (#9) — round 2 | --------- Signed-off-by: UncleSp1d3r <unclesp1d3r@evilbitlabs.io>
1 parent 0e8244b commit 746de92

57 files changed

Lines changed: 2515 additions & 3615 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

CONTRIBUTING.md

Lines changed: 46 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -81,49 +81,63 @@ opnConfigGenerator uses a layered architecture separating data generation from d
8181

8282
```text
8383
opnConfigGenerator/
84-
├── cmd/ # CLI commands (Cobra)
85-
│ ├── root.go # Root command, global flags
86-
│ ├── generate.go # generate command (csv + xml)
87-
│ ├── validate.go # validate command (stub)
88-
│ └── completion.go # Shell completion
84+
├── cmd/ # CLI commands (Cobra)
85+
│ ├── root.go # Root command, global flags
86+
│ ├── generate.go # generate command (xml + csv)
87+
│ ├── validate.go # validate command (stub)
88+
│ └── completion.go # Shell completion
8989
├── internal/
90-
│ ├── errors/ # Typed errors (ConfigError, VlanError)
91-
│ ├── netutil/ # RFC 1918 address generation/validation
92-
│ ├── generator/ # Device-agnostic data generators
93-
│ │ ├── vlan.go # VLAN generation with uniqueness tracking
94-
│ │ ├── dhcp.go # DHCP config derivation
95-
│ │ ├── firewall.go # Firewall rule generation (3 complexity levels)
96-
│ │ ├── nat.go # NAT mapping generation
97-
│ │ ├── vpn.go # VPN config generation (OpenVPN/WireGuard/IPsec)
98-
│ │ ├── departments.go # 20 departments with lease time mappings
99-
│ │ └── types.go # Shared generator types
100-
│ ├── opnsensegen/ # OPNsense-specific serializer (uses opnDossier schema)
101-
│ ├── csvio/ # CSV read/write with German headers
102-
│ └── validate/ # Cross-object consistency checks
103-
├── testdata/ # Test fixtures
104-
├── main.go # Entry point
90+
│ ├── errors/ # Typed errors
91+
│ ├── netutil/ # RFC 1918 address generation/validation
92+
│ ├── faker/ # *model.CommonDevice populator
93+
│ │ ├── device.go # NewCommonDevice entry point
94+
│ │ ├── options.go # Functional options
95+
│ │ ├── rand.go # Seeded *rand.Rand + *gofakeit.Faker
96+
│ │ ├── system.go # model.System populator
97+
│ │ ├── network.go # WAN/LAN/VLAN interfaces + VLAN list
98+
│ │ ├── dhcp.go # []model.DHCPScope populator
99+
│ │ └── firewall.go # []model.FirewallRule populator
100+
│ ├── serializer/
101+
│ │ └── opnsense/ # CommonDevice → OpnSenseDocument
102+
│ │ ├── serializer.go # Serialize entry point + ErrNilDevice
103+
│ │ ├── overlay.go # Overlay onto a base config
104+
│ │ ├── system.go # SerializeSystem
105+
│ │ ├── interfaces.go # SerializeInterfaces
106+
│ │ ├── vlans.go # SerializeVLANs
107+
│ │ ├── dhcp.go # SerializeDHCP
108+
│ │ └── firewall.go # SerializeFilter
109+
│ ├── opnsensegen/ # Transport only: load/parse/marshal XML
110+
│ └── csvio/ # CSV output derived from CommonDevice
111+
├── testdata/ # Test fixtures (base-config.xml)
112+
├── main.go # Entry point
105113
├── go.mod / go.sum
106-
├── .golangci.yml # Linter configuration (50+ linters)
107-
└── justfile # Task runner recipes
114+
├── .golangci.yml # Linter configuration
115+
└── justfile # Task runner recipes
108116
```
109117

110118
### Key Design Decisions
111119

112-
**Generators are device-agnostic.** The `internal/generator/` package produces abstract configuration data (`VlanConfig`, `FirewallRule`, `NatMapping`, etc.) with no knowledge of OPNsense XML structure. This data flows into device-specific serializers.
120+
**`*model.CommonDevice` is the single intermediate representation.** opnDossier defines the model; this project populates it (via `internal/faker/`) and serializes it (via `internal/serializer/opnsense/`). There is no parallel type or wrapper.
113121

114-
**Serializers are device-specific.** `internal/opnsensegen/` maps generator output to opnDossier's `OpnSenseDocument` schema type and marshals to XML. Future device types (pfSense) would get their own serializer package (e.g., `internal/pfsensegen/`).
122+
**Package layout reserves a pfSense sibling (planned).** `internal/serializer/opnsense/` is organized so a future `internal/serializer/pfsense/` can plug in alongside without restructuring shared code. When that sibling lands, the CLI will route by `device.DeviceType`; today it hardwires the OPNsense serializer.
115123

116-
**Schema types are imported, not duplicated.** The `opnsensegen` package imports types from `github.com/EvilBit-Labs/opnDossier/pkg/schema/opnsense` directly. This guarantees the mock configs match the production schema.
124+
**Transport is separate from serialization.** `internal/opnsensegen/` only loads, parses, and marshals XML. It does not generate or serialize. `MarshalConfig` post-processes to sort map-backed sections alphabetically (see GOTCHAS §7.1) so output is byte-stable under a fixed seed.
117125

118-
### Adding a New Device Type
126+
**Schema types are imported, not duplicated.** The serializer imports types from `github.com/EvilBit-Labs/opnDossier/pkg/schema/opnsense` directly. Generated configs are structurally identical to real device exports.
119127

120-
When opnDossier adds a new device parser (e.g., pfSense):
128+
### Adding a New CommonDevice Subsystem (NAT, VPN, Users, …)
121129

122-
1. Create `internal/pfsensegen/` with serialization logic
123-
2. Import the appropriate schema types from opnDossier
124-
3. Wire up a new `--device-type pfsense` flag in `cmd/generate.go`
125-
4. Add device-specific tests with testdata fixtures
126-
5. The existing generators (`generator/vlan.go`, etc.) should work unchanged
130+
1. **Faker** — add `internal/faker/<subsystem>.go` that returns the corresponding `model.*` type, and wire it into `internal/faker/device.go`'s `NewCommonDevice`.
131+
2. **Serializer** — add `internal/serializer/opnsense/<subsystem>.go` exposing `Serialize<Subsystem>(in) opnsense.<Type>`, and wire it into both `Serialize` (`serializer.go`) and `Overlay` (`overlay.go`) so overlay replaces the subsystem wholesale.
132+
3. **Round-trip test** — extend `TestRoundTrip` in `serializer_test.go` with per-field parity assertions on the new subsystem. A new subsystem without round-trip assertions is not in-scope for CI.
133+
4. **GOTCHAS §7.1** — if the schema type is `map[string]T`, add the parent element name to `mapBackedSections` in `internal/opnsensegen/template.go`.
134+
135+
### Adding a New Device Type (pfSense, …)
136+
137+
1. Create `internal/serializer/pfsense/` mirroring `internal/serializer/opnsense/`.
138+
2. Import the appropriate schema types (e.g., `github.com/EvilBit-Labs/opnDossier/pkg/schema/pfsense`).
139+
3. Route from `cmd/generate.go` based on `device.DeviceType` (or an explicit flag).
140+
4. Round-trip tests must go through opnDossier's corresponding parser.
127141

128142
## Code Style
129143

GOTCHAS.md

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,20 @@ It is also worthwhile to check the [opnDossier GOTCHAS](https://raw.githubuserco
1818

1919
## 7. CommonDevice to Device Serializer
2020

21+
### 7.1 Map-backed XML sections emit in randomized order
22+
23+
`opnsense.Interfaces` and `opnsense.Dhcpd` in `github.com/EvilBit-Labs/opnDossier/pkg/schema/opnsense` are defined as `map[string]T` with a custom `MarshalXML` that iterates the map directly. Go map iteration is randomized per encode, so a naive `xml.Marshal(doc)` emits `<interfaces>` and `<dhcpd>` children in a different order on every call — even with a fixed RNG seed.
24+
25+
- **Symptom:** `generate --seed 42` produces byte-different output across runs; `TestGenerateDeterministicSeed` flakes; downstream diff tooling registers spurious changes.
26+
- **Fix:** `internal/opnsensegen/template.go` `MarshalConfig` post-processes the marshaled XML via `sortMapBackedSections`, which walks the token stream and re-emits the children of any element in `mapBackedSections` in alphabetical order by tag name.
27+
- **When adding a new subsystem:** if the opnDossier schema type is `map[string]T`, add the parent element name to `mapBackedSections` in `internal/opnsensegen/template.go`. Slice-backed sections (VLANs, Filter.Rule, CAs, Certs) are emitted in struct-field order and do not need this treatment.
28+
29+
### 7.2 Serializer must propagate every round-trip field or fail CI silently
30+
31+
opnDossier's `opnsenseparser.ConvertDocument` does **not** warn on structurally-valid but semantically-empty output. If `SerializeInterfaces` drops `Interface.Type` or `Interface.Virtual`, the parser reads them back as the zero value and produces zero `ConversionWarning`s. Round-trip assertions based on counts alone will pass silently while every generated VLAN interface loses its `Virtual: true` flag.
32+
33+
- **Fix:** `TestRoundTrip` in `internal/serializer/opnsense/serializer_test.go` asserts per-field parity on `Interface.Type`, `Virtual`, `Description`, `IPAddress`, `Subnet`; on `VLAN.Tag`, `VLANIf`, `PhysicalIf`, `Description`; on `DHCPScope.Range`, `Gateway`, `DNSServer`. Any new subsystem must extend this test or CI will not gate its round-trip fidelity.
34+
2135
## 8. Git Tagging
2236

2337
### 8.1 Tag the Squash-Merge Commit on Main

0 commit comments

Comments
 (0)