Collapse the Config/Request type pairs in natmap models #42

Activity

1. FAZuH

   added

   ready-for-agentFully specified, ready for an AFK agentFully specified, ready for an AFK agent

   on Sep 30, 2026
2. FAZuH

   marked this as blocking 3 issues on Sep 30, 2026

   • Extract the repeated natmap api and table helpers #43
   • Make the unit tests hermetic: no real iptables, Docker, or shared /tmp #44
   • Table-drive the parse_port_mappings and iptables argv tests #45
3. FAZuH commented on Sep 30, 2026

   

   FAZuH

   on Sep 30, 2026

   OwnerAuthor

   More actions

   Acceptance-criteria deviation: lan_cidr no longer serialises as null

   Found in review of #42. The criterion "The JSON on the wire is byte-identical before and after" is not met for one field, and it is recorded here rather than left implicit.

   HairpinRequest (deleted) vs HairpinConfig (the merged replacement):

   // before — HairpinRequest
   #[serde(default)]
   pub lan_cidr: Option<String>,
   
   // after — HairpinConfig
   #[serde(default, skip_serializing_if = "Option::is_none")]
   pub lan_cidr: Option<String>,

   The wire delta

   lan_cidr: None (the common case — lan_cidr only applies to preserve_src_ip hairpin):

   // before
   {"ext_ip":"203.0.113.50","int_ip":"10.0.0.99","ports":"8080","proto":"tcp","lan_cidr":null}
   // after
   {"ext_ip":"203.0.113.50","int_ip":"10.0.0.99","ports":"8080","proto":"tcp"}

   lan_cidr: Some(..) is unchanged:

   {"ext_ip":"203.0.113.50","int_ip":"10.0.0.99","ports":"8080","proto":"tcp","lan_cidr":"192.168.1.0/24"}

   This is the only serialisation delta in #42: skip_serializing_if appears exactly once in crates/natmap/src/models.rs:199.

   No functional break — verified, not assumed

   1. The deserialise side has a default. Both types carry #[serde(default)], so an absent lan_cidr key deserialises to None exactly as an explicit null does. A probe test confirmed the two forms produce equal values:

      absent-> None
      null  -> None
      assert_eq!(absent, null)   // passes
      

   2. Nothing asserts the literal null. rg -n 'null' crates/natmap/ tests/ returns only 2>/dev/null shell redirects and Consul jq null-guards — no test asserts a JSON null in a hairpin body.

   Decision: accept the delta

   Keeping the one-type-per-rule shape that #42 exists to produce is worth more than reproducing a redundant null key. Re-adding a serialisation attribute purely to re-emit "lan_cidr": null would reintroduce exactly the kind of noise this pass is deleting, and both ends of this workspace ship together. The omission is also the better wire format.

   skip_serializing_if is left in place. Flagging so the deviation is auditable rather than hidden.
4. FAZuH commented on Sep 30, 2026

   

   FAZuH

   on Sep 30, 2026

   OwnerAuthor

   More actions

   Implemented in 50db2ad (refactor) and 1c7ffe9 (docs corrections).

   • Cut dead code and unused dependencies #40 — the deletions, the 8 manifest dependencies, and the write-only --state-dir flag. The flag's 15 Docker-test invocations went with it; docs/dev/testing.md:142 was corrected, since the flag was never providing the isolation it claimed.
   • Collapse the Config/Request type pairs in natmap models #42 — the four *Request twins removed, handlers now take *Config. One wire delta, recorded below.
   • Extract the repeated natmap api and table helpers #43 — the err(), header() and push_port_args helpers, the verb-taking route/rule builders, the single runtime, to_lowercase gone, and the unreachable protocol arm removed.

   Deviation from #42's "byte-identical" criterion: POST/DELETE /hairpin now omits lan_cidr instead of sending null, because HairpinConfig carries skip_serializing_if and the deleted HairpinRequest did not. Verified empirically that absent and explicit null both deserialise to None, and that no test asserts a JSON null. Accepted deliberately rather than reintroducing a redundant null key.

   Verified: cargo clippy --workspace --all-targets --all-features --no-deps clean; cargo test -p lab-ops_natmap (189), -p lab-ops_auto-discover (67), -p lab-ops_lab-lib (35), -p lab-ops --lib (65) all pass. The iptables argv tests have no diff, so the generated rules are provably unchanged. A review pass found no remaining defects.

   Not run: the 122+ Docker integration suite, per AGENTS.md. It compiles; you trigger it. ./dev.sh test or ./dev.sh all.

   Not pushed.
5. FAZuH

   closed this as completedon Sep 30, 2026