userspace-dp: resolve the to-zone of a MAC-less egress interface (IPsec xfrmi) - #6722
userspace-dp: resolve the to-zone of a MAC-less egress interface (IPsec xfrmi)#6722psaab wants to merge 30 commits into
Conversation
An IPsec secure tunnel (`st0`, an xfrmi) is `ARPHRD_NONE`, so
`forwarding_build::populate_egress` never builds an `EgressInterface`
for it: its `src_mac` gate is unsatisfiable for such a device.
`hardware_addr` is empty (netlink reports `hw_len=0`),
`mac_by_ifindex[bind_ifindex]` is absent because the parent is itself a
MAC-less xfrmi, and `iface.tunnel` means a Junos
`tunnel { source destination }` stanza that `st0` does not have.
The to-zone of a forwarding decision was read from `state.egress`
alone, so a correctly-zoned tunnel resolved to zone id 0 — the reserved
"unknown zone" sentinel that `evaluate_policy_result_l3_aware`
deliberately refuses to match ANY exact, wildcard or `junos-global` rule
against. Every LAN->tunnel packet was therefore adjudicated as
`(lan, 0)`: no operator-authored permit could apply — not
`from-zone lan to-zone vpn permit`, not `global ... permit` — the packet
fell to the implicit default policy, and under the default deny it was
dropped before reinjection. The tunnel IS correctly zoned in
`ifindex_to_zone_id` (which the INGRESS half of the same zone pair
already reads); the egress half simply did not consult it.
The fix is at the read, not at `populate_egress`.
`ForwardingState::egress_zone_id` now falls back to the authoritative
`ifindex_to_zone_id` when the interface has no `egress` row, and it
becomes the single egress-zone resolver: the zone-pair resolver (both
the production u16 form and its test-only String twin), the #3651
per-zone traffic counter, and the filter-log egress-zone field all route
through it, so the adjudicated zone and the logged/counted zone cannot
disagree.
Admitting the tunnel INTO `state.egress` — the other candidate
direction — was rejected. An `EgressInterface` carries `src_mac` and
`bind_ifindex`: it asserts that an Ethernet frame can be built for the
interface and handed to an AF_XDP bind target. That is false for a
link-layer-less xfrmi, and `session_glue::populate_egress_resolution`
would act on it, setting `resolution.src_mac = Some([0; 6])` and
`tx_ifindex = <xfrmi>` where today it correctly leaves `src_mac = None`.
It also changes what roughly thirty other `state.egress` consumers see —
MSS clamping, interface SNAT source selection, ICMP/PTB reply
generation, `zone_to_rgs`, WireGuard, fabric — none of which this defect
requires. Fixing the read touches only the zone value.
The fallback is scoped to the absent-row case on purpose. A row that
exists carrying `zone_id == 0` stays 0: `ifindex_to_zone_id` also holds
the zone PROPAGATED from a child unit onto its physical parent, and
inheriting that onto an interface the operator deliberately left unzoned
would widen the adjudicated zone pair for ordinary VLAN trunks. For
every ifindex that has an egress row the resolved to-zone is unchanged.
Validation. Measured end to end through the real chain — real Go
snapshot (`buildSnapshotWithSchedulerStateAndNATCounters` with only
`buildLinkSnapshot` stubbed to a faithful MAC-less xfrmi) -> real
`build_forwarding_state` -> real FIB -> real policy evaluator — over
2 bind spellings x 3 next-hop shapes x 2 destinations x 3 policy shapes.
With an explicit `from-zone trust to-zone vpn permit` (or a `global`
permit) and default-deny, master drops 8 of the bare `bind-interface
st0` spelling's 12 permitted cells with `policy_id=4294967295`
(the default-policy sentinel); at this change all 12 are Permit
`policy_id=0` and reinject. The zone pair moves from `(trust, 0)` to
`(trust, vpn)` in every MissingNeighbor cell. The no-matching-permit
control still denies at both revisions, and `state.egress` still has no
row for the tunnel — nothing on the TX path moved.
Four regression tests, each proven to fire by mutation with a clean
build first (rc 0): removing the fallback reds three of them on the
to-zone (left 0, right 7); redirecting the fallback to the
`junos-global` sentinel reds the same three (left 65535); widening the
fallback to also fire on `Some(0)` reds the scoping guard (left 1,
right 0) while leaving the other three green.
A cluster smoke is owed: this is a dataplane forwarding change.
Issue: #6713
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
…wn zone The #6713 fallback resolved the to-zone of a row-less interface through `ifindex_to_zone_id`, which is not a pure "this interface's zone" map: `populate_interfaces` also propagates a zoned child unit's zone onto `parent_ifindex` when the parent has no entry of its own. The scope claim shipped alongside it -- an existing row carrying `zone_id == 0` stays 0, so an unzoned interface cannot inherit a propagated zone -- held for only half the domain. A MAC-FUL parent HAS a row carrying 0, so the fallback never fires and the claim is true. A MAC-LESS parent has NO row, so the propagated zone WAS inherited, which is exactly the widening the claim said it prevented, on precisely the interface class #6713 newly routed through the fallback. The reachable shape is two secure tunnels on one `st0` -- `bind-interface st0` plus `bind-interface st0.1`, both legal spellings per `pkg/config/xfrmi.go` -- with unit 0 addressed and routed but deliberately left in no security zone. Driven through the real `build_forwarding_state` -> real FIB -> real policy evaluator, the unzoned unit resolved its sibling's zone and the operator's `from-zone lan to-zone vpnb permit` MATCHED, with `policy_id = 0`, a real rule index rather than `DEFAULT_POLICY_SENTINEL_ID`. The disposition is `MissingNeighbor`, where a permit is `is_slow_path_eligible` and is reinjected to the kernel while a deny breaks to `RecycleAndContinue` -- so this forwarded transit that the previous release denied, out a different IPsec SA than the operator authorised. The ingress half is not symmetrically exposed, because an interface with an empty zone is never an AF_XDP ingress bind target, so #6713 made the widening reachable for the first time. Fix at the source of truth rather than at the read: `populate_interfaces` now records the own-zone value a second time in a new `ForwardingState::ifindex_own_zone_id`, written immediately BEFORE the parent propagation, and `egress_zone_id`'s fallback reads that map. This shape was chosen over a propagated-flag or a `(u16, bool)` value type because it records only ground truth: it needs no remove-on-own-insert semantics, so it does not depend on snapshot iteration order, and it touches no existing reader -- the ingress half still wants the propagated value (#921/#3618). Both branches of `egress_zone_id` now answer the same question, since `populate_egress` likewise derives `EgressInterface.zone_id` from the interface's own `iface.zone`. Two further items from the same review. `tunnel.rs`'s local-origin tunnel TX path still open-coded the `egress`-only read, contradicting the "single egress-zone resolver" invariant; it is swept through `egress_zone_id`, with no runtime difference today because GRE and WireGuard both carry a `tunnel` stanza and so always have a row -- the comment records why, since no test can bind a state the snapshot builder cannot produce. And the two #6713 call sites shipped with no test at all: reverting either `filter_log_egress_zone_id` or `forward_request`'s own independent `egress_zone_id` call left the entire suite green, because every existing filter-log assertion uses a MAC-ful interface where both reads agree. They are separate call sites, not one helper, and each now has its own binding. Validation. Parent-RED at b73679a with only the test additions applied: `cargo build --release --bins` and `--bins --tests` both rc 0, then exactly one failure, an assertion (`left: 7, right: 0`) rather than a build break, with all five #6713 guards green -- so the red is real and precisely scoped. Mutations, each with build rc 0: pointing the fallback back at `ifindex_to_zone_id` reds exactly 2 of 4244 tests, the two new #6722 guards, and nothing else; reverting `filter_log_egress_zone_id` reds exactly 1; reverting `forward_request`'s call reds exactly 1, and neither reds the other's test, confirming they needed separate bindings; reverting the `tunnel.rs` sweep leaves the suite green, the honest result for a state that is unreachable today. The preserved `unzoned_interface_with_egress_row_stays_zone_zero_6713` no longer reds on the lone "fire on Some(0)" mutation -- the own-zone scoping makes that harmless -- but still reds on the full widening; that is recorded in the guard's own comment so its greenness is not misread as lost coverage. Gates: cargo test --release --bins --tests -- --test-threads=1 rc 0 (4242 passed, 2 ignored, plus 60 + 8 + 22 + 31), go build, go vet, and go test ./... all rc 0 across 59 packages. The architecture doc's scope paragraph asserted the false claim; it is rewritten, and the NPTv6, fabric zone-stamp, per-zone half-open-window and default-permit-upgrade consequences of resolving a real to-zone are now named there rather than left for a bisect to rediscover. Advances #6713. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
Folds four MAJORs from a hostile review of ebe3707..f754bee. The fifth (a MAC-less xfrmi egress resolving to zone 0) is a pre-existing defect tracked as #6713 and fixed by open PR #6722, which owns the two Rust files it lives in; it is not touched here, and the documentation claims that silently depended on it being fixed are corrected instead. junos-host denies were scoped to the wrong kernel device (FAIL-OPEN). This one the branch created. junosHostLinuxName resolves the iifname scope for `to-zone junos-host ... then deny` nft rules and its doc claimed to mirror snapshotLinuxName exactly, but it still performed the generic unit-zero collapse. Before this branch both sides collapsed `st0.0` to `st0` and agreed on a wrong name; fixing only the snapshot made them disagree on a right one, so with `bind-interface st0.0` the renderer emitted `iifname st0` while the decrypted plaintext arrives on `st0.0`. The deny could never match — a security guard rendered unable to fire, which `show` still reports as configured. The fix is not to add a fourth copy of the rule. Config. SecureTunnelUnitNetdev is now the single resolver, and ResolveKernelIfName, snapshotLinuxName and junosHostLinuxName all call it. That rule in particular cannot be re-derived per caller: the netdev comes from the AUTHORED bind-interface, and junosHostLinuxName does not otherwise read the IPsec config at all. The secure-tunnel predicate admitted names that cannot be xfrmis. XFRMIfNameAndID bounds the index to [0, 65536) — the if_id is `stIndex<<16 | unit+1`, so a larger or negative index has no room — while IsSecureTunnelIfName ran a bare Atoi with no bounds. So `st-3` and `st65536` classified as secure tunnels. Interface names are wildcard- authorable with no `st` reservation, making `st65536` an ordinary data interface, and the new exclusion therefore removed a live interface from the ingress-adjudication map, the AF_XDP binding plan and the RSS allowlist: a traffic outage. Both now share one unexported secureTunnelIndex, so a classifier can no longer admit what the constructor rejects. The Rust mirror is_secure_tunnel_ifname takes the identical bound. The collision fallback contradicted the routing fail-closed contract. Two distinct bind-interface strings deriving one if_id made SecureTunnelNetdevForRef return the lexicographically smallest name for determinism. pkg/routing/xfrm.go deletes BOTH colliding devices from its desired set, so neither exists on the box; naming one is not deterministic-and-correct but deterministically wrong, and it attaches forwarding state to a device the reconciler has guaranteed is absent. It now returns ("", false). Two VPNs authoring the SAME string remain one device, not a collision, and still resolve. TestSecureTunnelNetdevForRefIsDeterministicUnderCollision required the old behaviour, so it was rewritten. It was changed because it pinned a defect, not because it was inconvenient: the property it asserted (determinism) was real but was the wrong property to hold. Resolver divergence is scoped deliberately. The three resolvers this rule belongs to are unified above. Four pre-existing divergences in live code — reached through CompileUserspaceShim -> CompileConfig -> compileZones, not retired-eBPF-only — were verified firsthand and filed rather than half-fixed: #6728, #6729, #6730, #6731. The search scope is stated in userspace-dp/src/server/README.md so the claim is no broader than the sweep, including the sites deliberately not audited. Six documentation claims were verified against the code and corrected rather than softened. The xfrmi does not enter the egress map: populate_egress needs a MAC, a parent's MAC or the tunnel flag, and an ARPHRD_NONE secure-tunnel unit carries none of the three. A matching permit does not preserve delivery today, because evaluate_policy_result_l3_aware wraps its whole walk — exact, wildcard AND junos-global — in `if from_id != 0 && to_id != 0`; that behaviour depends on #6722 landing, and the README and both code comments now say so. An any/any policy does not match zone zero, by the same guard. A bare `next-hop st0.0` still does not resolve. And a deployment with no bound VPN is not unchanged: the merge base collapsed an unbound unit zero to `st0` while head returns `st0.0`, and the exclusion keys on the interface NAME, not on whether a VPN is configured. Advances #5619. Validation: go build and go vet clean; full Go suite green (59 packages, zero failures) under a fresh GOCACHE with TMPDIR=/tmp, with each new test name confirmed present in -v output. cargo build and the Rust suite green for the mirrored predicate. Every behavioural fix was proven by mutating the fix out and confirming build+vet stayed CLEAN while the specific guard went RED on a real assertion. Two of the six mutations sit at the EDGE of the claim rather than its centre: an off-by-one bound (`>` for `>=`) keeps st65535 green while st65536 reds, and an over-strict collision test reds only the same-bind-interface case. One mutation moved the SHARED resolver so both planes agreed on the wrong name — the parity test stayed GREEN and only the new absolute test went RED, which is why an absolute assertion was added rather than trusting parity.
…findex zone Round 2 of #6722 added `ForwardingState::ifindex_own_zone_id` to keep the #6713 egress-zone fallback from handing an "unzoned" `st0.0` the zone of its zoned sibling `st0.1`. That map could not have the effect it claimed. It is built from `InterfaceSnapshot.zone`, which is `zoneByInterface[name]`, and the Go builder has already propagated by then: `buildInterfaceZoneMap` (pkg/dataplane/userspace/zones.go) writes `out[base]` for a unit-suffixed zone reference, and `snapshotLinuxName` collapses a non-VLAN unit 0 onto the base netdev. So zoning `st0.1` puts `vpnb` on the BASE row, on the very ifindex unit 0 forwards out of, before Rust sees the snapshot. Measured with the real builders on the fixture's own config (`buildLinkSnapshot` is a package var so this is testable): zoneByInterface = map[ge-0/0/1:lan ge-0/0/1.0:lan st0:vpnb st0.1:vpnb] snap name="st0" ifindex=42 parent=0 zone="vpnb" snap name="st0.0" ifindex=42 parent=42 zone="" snap name="st0.1" ifindex=43 parent=42 zone="vpnb" The round-2 Rust fixture gave the base row no zone -- a snapshot the builder never emits. The general case follows: a Rust child->parent propagation can only add an entry for `parent_ifindex(U)`, which is the base row's ifindex, and that row's own `Zone` is non-empty in both zone-ref spellings. The two maps were therefore identical on every producible snapshot, and the round-2 fix was a runtime no-op. Resolution: accept the propagated behaviour instead of carrying an own-vs-inherited flag across the Go->Rust boundary. The ingress half already resolves that ifindex to `vpnb` (`ifindex_to_zone_id` is the from-zone source), so scoping only the egress half would make one ifindex answer two zones by direction -- and the narrower answer is the 0 sentinel, which matches no exact, wildcard or `junos-global` rule, i.e. #6713 again for that config. Junos zones logical UNITS, so `st0.0` and `st0.1` sharing a zone is a real parity gap; it needs per-unit identity end to end (the unit-0 ifindex collapse included) and is filed separately rather than papered over at this one read. Deleted `ifindex_own_zone_id`, its insert and the two #6722 guards; the fallback reads `ifindex_to_zone_id` again. The Rust child->parent propagation is kept as a helper-boundary backstop with a comment saying it is unreachable for a Go-produced snapshot. Also corrected a real coverage hole round 2 introduced. It called `egress_zone_id`'s `Some(0)` short-circuit "redundant rather than load-bearing". It is load-bearing: `populate_egress` is last-write-wins across snapshot rows, so a zoned trunk with a declared-but-unzoned unit 0 (`ge-0/0/9` zoned `lan`, `ge-0/0/9.0` in no zone, both MAC-ful, both ifindex 90) gets `egress[90].zone_id == 0` while `ifindex_to_zone_id[90] == lan`, and the short-circuit is the only thing holding the to-zone at 0. The guard meant to catch its removal had been modelling an unzoned physical parent carrying a zoned VLAN unit, which the builder never emits -- the parent arrives zoned -- so it was green on an impossible shape and had stopped binding. Re-pointed at the producible shape. New Go guard pkg/dataplane/userspace/zone_propagation_6722_test.go pins the two cross-boundary facts the userspace-dp fixtures encode, so a Rust fixture cannot drift back to a snapshot the Go builder cannot emit. Fixture drift is what cost rounds 1 and 2. Validation. Full suite green under a timeout wrapper (so a wedge would surface as rc 124 rather than silence): `timeout 3600 cargo test --release --bins --tests -- --test-threads=1` rc 0, 4241 + 60/8/22/31/1/2 passed, 2 ignored, and the eight #6713/#6722 tests confirmed as RUN rather than skipped; go build/vet/test rc 0. Seven mutations, each applied alone against a sha256-verified baseline with the build asserted rc 0 first so a build break cannot be misread as a red. Six tests reach the resolver through the real `build_forwarding_state`; two hand-build a `ForwardingState`, and several rows turn on that distinction: M-A widen the egress branch to fire on `Some(0)` -> 1 RED (the re-pointed #6713 scoping guard; this is the round-2 hole closed) M-B delete the fallback (undo #6713) -> 7 RED M-C filter_log_egress_zone_id -> egress-only -> 1 RED M-D forward_request's own call -> egress-only -> 1 RED (M-C and M-D do not red each other's test: the two call sites stay independently bound) M-E restore round-2's own-zone scoping VERBATIM -> 2 RED, and ONLY the two hand-built fixtures, which no longer populate the map that code reads. All six real-builder tests stayed GREEN: round 2 changed nothing on any producible snapshot -- measured, not argued. M-F Go: drop out[base] = zoneName -> 3 RED (both new Go guards plus the pre-existing #5699 test, which depends on the same write) M-G option-(a) scoping that really excludes inherited -> 3 RED, including the coherence test; the other five real-builder tests stay green, so that test is the only thing that would catch a future re-scoping of the egress half #6713 is not re-broken: the plain `bind-interface st0` matrix (2 zone-ref spellings x 3 next-hops x 2 destinations x 3 policy shapes = 36 cells) run through the real snapshot -> build_forwarding_state -> FIB -> policy chain shows permitted_dropped=0/12, every permitted cell resolving from=lan to=vpn under an operator rule id, and control_denied=24/24. Advances #6713. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
…index
Round 3 justified resolving a MAC-less egress interface's to-zone from
`ifindex_to_zone_id` by arguing "directional coherence" -- that ingress
and egress must answer the same zone for one ifindex. That argument is
wrong. Several logical units collapse onto one netdev
(`snapshotLinuxName` maps a non-VLAN unit 0 back onto its base), so an
ifindex is not a unit identity, and `ifindex_to_zone_id` holds the LAST
zoned row on it plus the child->parent propagation. Reading it as the
egress answer hands an interface a zone the operator never configured
there -- and a NONZERO to-zone is exactly what makes an operator's
permit MATCH, so that direction is fail-OPEN.
Three producible shapes do it:
1. Zone only `st0.1`; `buildInterfaceZoneMap` still stamps the `st0`
BASE row with that zone, and `st0.0` -- which the operator left in
NO zone -- shares the base's ifindex.
2. Two units in DIFFERENT zones on one `st0` with unit 0 unzoned. The
`out[base]` write is first-write-wins over SORTED zone names, so
unit 0's ifindex carries the alphabetically-first sibling's zone.
3. StableZoneID quarantine. `quarantineCollidingZones` blanks `Zone`
on a colliding zone's interfaces AFTER `buildInterfaceSnapshots`
ran, precisely so they fail CLOSED. The base then arrives unzoned
beside a surviving zoned child, the Rust child->parent propagation
re-zones the parent ifindex, and reading it would hand the
quarantine's deliberate default-deny back the survivor's zone.
New `ForwardingState::ifindex_unambiguous_zone_id`, built in
`populate_interfaces` over ALL snapshot rows -- zoned and unzoned alike,
because an unzoned row's "no zone" is an opinion that must be able to
conflict with a zoned sibling's. An ifindex lands in it only when EVERY
row sharing it named the same nonzero zone; disagreement leaves it
absent and `egress_zone_id` resolves the 0 sentinel, the pre-#6713
answer, against which no rule matches and the default policy decides.
The two directions now deliberately disagree for an ambiguous ifindex.
The justification is DIRECTIONAL, not a claim that the ingress surface
is unreachable: only in shape 3 is every row unzoned, which
`interfaces.go`'s `if iface.Zone == "" { continue }` keeps off the AF_XDP
bind list entirely. In shapes 1 and 2 the base row is zoned and ingress
really does answer that zone. What differs is that ingress answering
wide is pre-existing (#921/#3618) and untouched here, while egress
answering wide is NEW. Whether the ingress half should be narrowed the
same way is left explicitly unsettled.
#6713 is untouched: in every #6713 shape the rows on the tunnel's
ifindex agree, so the ifindex stays in the unambiguous map.
Two claims are labelled as design rationale rather than guards, because
mutating them leaves the suite green: keeping the propagation out of the
agreement ledger, and the `zone_id != 0` skip at the flush (a stored
`Some(0)` and an absent key both resolve 0 through `.unwrap_or(0)`).
`row_zone_id != 0` is exactly the pre-existing `!zone.is_empty()`
condition -- `zone_name_to_id_from_snapshot` skips `zone.id == 0`, so a
name that resolves resolves nonzero.
Also corrects `tunnel.rs`: only gre/ip6gre reach the local-origin loop,
not WireGuard -- `endpoint_attachment_valid` parks every other mode.
Advances #6722.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
… not
The round-3 coherence test
`macless_unit_on_a_shared_ifindex_resolves_one_zone_both_directions_6722`
did not bind anything. Its fixture already gave the `st0` base row
`vpnb`, so restoring the map the PR had deleted left it GREEN -- it
could only ever agree with the resolver by coincidence. It is deleted,
not re-pointed, and replaced by four tests that adjudicate through the
real FIB and the real policy evaluator:
- unzoned_macless_unit_does_not_inherit_a_zoned_siblings_zone_6722
- divergently_zoned_sibling_units_do_not_pick_a_zone_6722
- quarantine_unzoned_base_does_not_inherit_the_surviving_childs_zone_6722
- reused_ifindex_across_two_zoned_interfaces_resolves_no_zone_6722
Each asserts to-zone 0 AND names the specific wrong nonzero value the
fail-open would produce, so none can pass by resolving some other zone;
each first asserts that `ifindex_to_zone_id` -- the map the egress half
must NOT read -- carries a real nonzero zone for that ifindex, so
"to-zone is 0" is never indistinguishable from an empty state; and each
requires the verdict to come from `DEFAULT_POLICY_SENTINEL_ID` rather
than the sibling's permit.
`unanimously_zoned_shared_ifindex_still_reaches_policy_6713` is the
scope control: `set security zones security-zone vpnb interfaces st0`
fans out to every unit, both rows on the ifindex agree, and the fallback
must still resolve. That is why the gate keys on DISAGREEMENT and not on
"more than one row shares this ifindex".
All five snapshot fixtures move to `afxdp::test_fixtures` and are driven
through the real `build_forwarding_state`. Round 3 kept hand-built
`ForwardingState`s in `poll_descriptor::filter` and
`frame::tests_ports_live_forward` and claimed independently maintained
fixtures could not drift; they can and did -- both populated
`ifindex_to_zone_id` alone, so they encoded a map layout instead of a
snapshot and went red on a builder change that was correct. Both now
build from the shared fixture.
Both log sites gained an ambiguous-ifindex case. `forward_request.rs`
calls the resolver independently of `filter_log_egress_zone_id`, so the
gate has to be proven at each: a log field naming `vpnb` for transit the
firewall denied under the default policy sends an operator hunting a
`lan->vpnb` rule that never ran.
Red-on-revert, `cargo test --release -- 6722 6713` (14 tests, baseline
14/14 green):
- fallback re-pointed at `ifindex_to_zone_id`: 6 RED (all four
forwarding tests + both log sites), e.g. "left: 7 right: 0". All
EIGHT #6713 tests stayed GREEN, so the gate is scoped to ambiguous
ifindexes and does not re-break #6713.
- agreement ledger fed only by ZONED rows: 4 RED (the two sibling
shapes + both log sites); the quarantine and reused-ifindex tests
correctly stay green, their ambiguity having a different source. The
two ledger properties are independently bound.
Advances #6722.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
…napshot The Go guard called only `buildInterfaceZoneMap` + `buildInterfaceSnapshots`, never `buildSnapshot` -- which is exactly why it could assert that the Rust child->parent zone propagation is unreachable for a Go-produced snapshot. `quarantineCollidingZones` runs AFTER `buildInterfaceSnapshots` (builder.go) and blanks `Zone` on every row bound to a colliding zone, so a base whose zone lost a StableZoneID collision arrives UNZONED beside a surviving zoned child, and the propagation fires. The claim is corrected and `TestQuarantineUnzonesTheBaseRow_6722` emits the counterexample. Zone names are picked for their SORT order, which drives two independent mechanisms: z174/z214 collide and the later-sorting name (z214) is the one quarantined; and `buildInterfaceZoneMap`'s `out[base]` write is first-write-wins over sorted names, and z214 sorts before zzzz, so the doomed zone is the one that lands on the `st0` base row. The test pins that pre-quarantine placement before running the pass, so an empty Zone afterwards is a scrub rather than the failure default. It also pins that the STRICT compiler REJECTS the collision (#3075), so the lenient boot / HA-sync / pre-#3075-persisted path is the only way a colliding snapshot can reach the quarantine at all. Two failure-default assertions fixed. `unit0.Ifindex != base.Ifindex` passes as `0 == 0` when the `buildLinkSnapshot` stub resolves neither row, so both cases now pin the primed values (42/43, 90/91). And the empty-HardwareAddr assertions equal what the stub returns for an unresolvable link, so case A gained a positive control: the LAN row must carry its MAC through the SAME stub, or the empty xfrmi MACs prove nothing. Advances #6722. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
The architecture doc still described the round-3 design, and three of its claims are now false: that both zone-resolution halves read the same authoritative source, that the deleted own-zone map was inert, and that independently maintained test fixtures cannot drift. It also asserted the Rust child->parent propagation is unreachable for a Go-produced snapshot, which the StableZoneID quarantine disproves. The section now documents `ifindex_unambiguous_zone_id`, the three producible shapes an ifindex-wide answer would mis-adjudicate, and the deliberate ingress/egress asymmetry -- including the part that is NOT true: the ingress surface is only unreachable in the quarantine shape, where every row is unzoned. In the sibling and divergent shapes the base row is zoned and ingress really does answer that zone, so the asymmetry rests on direction (ingress-wide is pre-existing, egress-wide is new and turns a deny into a permit), not on reachability. The downstream-consumer list is scoped to an unambiguous ifindex, since an ambiguous one still resolves 0 and none of those consumers fire for it. Advances #6722. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
The gate's claim is "an ifindex resolves a to-zone only when EVERY
snapshot row on it named the same nonzero zone", but every fixture in
this issue put at most TWO rows on a shared ifindex. That drives the
agreement fold `Vacant -> Occupied-same` and `Vacant -> Occupied-different`
and never exercises a third row arriving AFTER a conflict was recorded,
so the guard fired while being scoped narrower than its claim.
The gap admits a producible fail-open. Rewriting the fold's comparison
against an unwrapped zone id instead of the whole `Option` --
if let Some(existing) = *slot.get() {
if existing != row_zone_id { slot.insert(None); }
} else {
slot.insert(Some(row_zone_id)); // re-arms after a conflict
}
-- is green on all fourteen existing tests, and on `st0` with units
0/1/2 where unit 0 is unzoned it resolves `vpnb` for an ifindex whose
rows arrive `vpnb` -> none -> `vpnb`. That is the original #6722
fail-open, reachable again through an ordinary three-unit config.
`conflict_then_agreement_snapshot_6722` is the first fixture here with
three rows on one ifindex, and it asserts the ORDER before adjudicating
so the absorbing-conflict path is exercised rather than assumed.
`None` is absorbing because the `!=` is written against the whole
`Option`; the comment now says so and names the mutation.
Red-on-revert: the mutation above yields exactly 1 RED --
`a_conflicted_ifindex_is_not_rearmed_by_a_later_agreeing_row_6722`,
"ifindex 42 is shared by rows that disagree about its zone, so it must
not appear in the unambiguous map at all" -- with the other 14 green,
which is precisely the point. Restored (with `touch`, so cargo could not
re-run the mutated binary) and 15/15 green.
Advances #6722.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
Applying the guard-D lens to the rest of the change. A two-element fixture is structurally incapable of testing an absorbing state -- it can only ENTER the state, never attempt to re-arm it -- and "EVERY row on that ifindex agrees" is a universal that a pair cannot exercise. Two more gaps found, one of them with a mutation invisible to the entire suite. Zero sentinel arriving FIRST. Every prior fixture ran nonzero-then-sentinel on the shared ifindex; nothing tested the mirror order. A fold that reads a recorded `Some(0)` as "no opinion yet" and lets a later nonzero row upgrade it is GREEN on all sixteen other tests and resolves `vpnb` for an ifindex two unrelated interfaces share. Order-dependence is a defect even where it happens to yield the operator's answer: `buildInterfaceSnapshots` row order is not a contract the Rust side may lean on. Pinned by `a_zero_sentinel_row_is_not_upgraded_by_a_later_zoned_row_6722`; the mutation above yields exactly 1 RED. Unanimity over THREE rows rather than a pair, so the positive direction is tested as a universal too. NEGATIVE RESULT, recorded as such: no mutation was found that this test catches and the two-row unanimous case misses. It is coverage of the universal, not a proven guard, and is described that way rather than credited as one. Quarantine with more than one blanked row was ALREADY covered -- the fixture blanks both the base and the unit-0 row, and the Go test asserts both. No change. FIXTURE PROVENANCE, and a defect in the guard-D test from the previous commit. It put a third row on the shared ifindex by giving `st0.2` `linux_name = "st0"`. `snapshotLinuxName` collapses only a non-VLAN unit ZERO onto the base, and `TunnelNameMap` gives unit N>0 its own device (`gr-0-0-0u1`), so that row is one `buildInterfaceSnapshots` never emits -- the evidence-free-fixture class this PR exists to fight, with a docstring claiming a config that does not produce it. A third row on one ifindex is producible ONLY by ifindex RECYCLING within one snapshot, the mechanism `reused_ifindex_snapshot_6722` already rests on. All three new fixtures are rebuilt on base + unit-0 collapse plus a recycled `st1.0`, the provenance is written into each fixture, and guard D was re-proven on the corrected shape. Last absolute phrasing corrected. `egress_zone_id`'s "the logged/counted zone can NEVER disagree" is a completeness claim over CALL SITES that nothing enforces. It now says "do not disagree", states that this holds BY ENUMERATION rather than by construction, names which three sites are pinned by tests, and warns that the zone-accounting readers are not -- so a new direct `state.egress` read there would not be caught here. Advances #6722. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
Independent gate at
|
…dger
The ledger was correct; the resolver did not depend on it.
`egress_zone_id` reads `state.egress` FIRST and only falls back to
`ifindex_unambiguous_zone_id`, and `populate_egress` wrote
`egress[ifindex].zone_id` from the ROW's own zone, last-write-wins per
ifindex. On an ifindex that several differently-zoned rows share, the
LAST row therefore decided the to-zone and the ledger was never
consulted. The four mutations proven in the previous rounds all mutate
ledger arithmetic, so none of them could catch this.
Reachable from a stable, non-racy config:
set interfaces wg0 tunnel mode wireguard
set interfaces wg0 unit 0 family inet address 10.5.5.1/30
set interfaces wg0 unit 1 family inet address 10.6.6.1/30
set security zones security-zone vpnb interfaces wg0.1
`TunnelNameMap` (`pkg/config/types.go`) maps every unit WITHOUT its own
tunnel stanza onto the interface device -- the branch admits WireGuard
despite its empty GRE-style `source` -- so `wg0`, `wg0.0` and `wg0.1`
are one netdev and one ifindex, as `tunnels_test.go` already pins. All
three carry `tunnel = true`, so `populate_egress` admits them through
`iface.tunnel.then_some([0; 6])` and they DO get egress rows.
`buildInterfaceZoneMap` stamps the base row `vpnb`, `wg0.0` is left
unzoned, and the zoned `wg0.1` is emitted last -- so transit routed out
the deliberately-unzoned `wg0.0` matched `lan -> vpnb permit`. That is
the defect this PR exists to close, intact underneath a green suite:
every ambiguity test asserted the ifindex has NO egress row, which is
true of the MAC-less secure tunnels the fixtures used and false here.
`populate_egress` now takes `zone_id` from `ifindex_unambiguous_zone_id`,
so both arms of the resolver derive from one source and cannot disagree.
The alternative -- gating the read inside `egress_zone_id` -- was
rejected: it adds a second map lookup to a per-packet path for no
semantic gain. The #2391 unknown-zone check is retained as an explicit
reject so an unresolvable zone NAME still fails the snapshot closed.
This also makes the `Some(0)` short-circuit correct BY CONSTRUCTION.
`unzoned_interface_with_egress_row_stays_zone_zero_6713` held only
because the unzoned unit-0 row happened to be emitted last; reverse the
order, as the WireGuard shape does, and the zone won instead.
Fixture producibility, the same lesson one layer down. The round-5 claim
that a third row on one ifindex cannot come from another unit cited
`TunnelNameMap`'s per-unit branch (`gr-0-0-0u1`) and missed the
interface-level branch directly above it, which does the opposite. The
recycling-based multi-row fixtures were unproducible for a second reason
too: they omitted base rows `buildInterfaceSnapshots` necessarily emits.
All three are removed -- the WireGuard shapes subsume them exactly,
giving `[Z, 0, Z]`, `[0, 0, Z]` and `[Z, Z, Z]` from real config and
WITH egress rows. `reused_ifindex_snapshot_6722` is kept, two unrelated
interfaces on a recycled index being a distinct shape, with its missing
base rows added.
Red-on-revert: sourcing the egress row's zone from the row again yields
2 RED -- `unzoned_iface_tunnel_unit_does_not_inherit_a_siblings_zone_via_egress_row_6722`
("left: 7 right: 0") and
`iface_tunnel_egress_row_is_not_upgraded_by_a_later_zoned_unit_6722` --
with the other 15 green. Both tests were written and watched RED on the
parent before the fix. Scope control
`unanimous_iface_tunnel_units_still_reach_policy_6722` keeps resolving
`vpnb` and matching the operator's permit, so an ordinary WireGuard
deployment still forwards.
Blast radius: none on the existing suite (4248 passed; the 2 failures
are the #6819 poisoned-mutex flake). The change is observable only where
rows on one ifindex disagree.
Advances #6722.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
…he helper The comment claimed the helper-level tests "close" the production call site. They do not drive it. Every assertion called `filter_log_egress_zone_id` directly, so re-opening the pre-#6713 `state.egress`-only read INSIDE `emit_cached_output_filter_log_tail` -- its only production caller -- left the whole suite green: the helper is still correct, it just is not the thing being called. `cached_output_filter_log_reports_the_adjudicated_zone_6722` drives the real caller and asserts the emitted event. It adjudicates TWO ifindexes, and the pairing is what makes it bind: - the AMBIGUOUS ifindex must log 0 (the #6722 gate), and - the MAC-less ZONED tunnel must log its zone (the #6713 fallback). The second is the discriminator. An ambiguous ifindex alone does NOT bind the consumer: since the egress row's `zone_id` became ledger-derived, an ifindex that HAS an egress row gets the same answer from the helper and from a raw `state.egress` read, so the mutation stays green. Only an ifindex with NO egress row separates them. The first version of this test used the ambiguous index alone and was green under the mutation; that is recorded in the test comment so the pairing is not "simplified" away later. Each emission uses a fresh event-stream handle. Reusing one across both made the second `try_recv` return `Empty` -- the stream is stateful per handle -- which would have made the assertion depend on stream internals rather than on the zone. Two claim corrections, both cases of the artifact being stronger than the code: - The architecture doc said the logged and counted zones "cannot disagree" without the enumeration caveat the source comment already carried. It now says "do not disagree", states that this holds by enumeration rather than by construction, names the four sites tests pin, and names the zone-accounting readers that are not pinned. - PROVENANCE. The WireGuard ambiguity was already latent in the index-keyed `egress` map before this branch: on `origin/master` `egress_zone_id` is an `egress`-only read and `populate_egress` already took the row's own zone last-write-wins, so that shape already adjudicated `vpnb` there. #6713 did not create the defect; it added the fallback and routed more consumers through the same incomplete resolver. Recorded in the source and the doc so a bisect is not misled about what this branch introduced versus inherited. Red-on-revert: re-opening the `state.egress`-only read inside `emit_cached_output_filter_log_tail` yields 1 RED -- the new consumer test, "left: 0 right: 7" -- with BOTH direct-helper tests staying GREEN. That green is the evidence for the finding. Advances #6722. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
…est-scope limit Text only; no logic changes. Three claims in the change were wrong or stale, two of them in a way that would mislead a future reader. The architecture doc claimed every ifindex with an egress row resolves a to-zone "bit-identical to the pre-#6713 read". That is false, and `[Z, 0, Z]` is the direct counterexample: the old read returned the last row's `Z`, the ledger returns `0`. Since that change is the POINT of #6722, the doc now says so rather than denying it -- bit-identical wherever the rows AGREE, which is every ordinary single-unit interface, and deliberately different where they disagree. A second instance of the same false claim, in the downstream-consumer preamble, is corrected too: "resolves 0" matches pre-#6713 only for an ambiguous ifindex with NO egress row. `ifindex_unambiguous_zone_id` was documented "Read only by `egress_zone_id`". That went stale when `populate_egress` began sourcing `EgressInterface::zone_id` from it; the doc comment now names both arms and says why the row is the wrong source. `forwarding/mod.rs` still described the resolver as falling back to `ifindex_to_zone_id`. It falls back to `ifindex_unambiguous_zone_id`; the comment now says which, and why the from-zone map is the wrong source for a to-zone. Also records a test-scope limit next to the ledger rather than leaving it to be discovered: no public test can distinguish an erroneous re-arm to `Some(0)` from a genuine conflict, because the flush omits both from the map and `egress_zone_id` ends in `.unwrap_or(0)`, so both resolve to the same 0. The observable security property -- an ambiguous ifindex never adjudicates a zone -- is pinned in BOTH resolver arms, but this internal representation is not exhaustively mutation-tested, and a mutation turning a conflict into `Some(0)` will not red. The note says what to do about it: add an accessor before relying on the distinction. Two further claims flagged against the previous head were already corrected in `1a7ff02d3` -- the doc's enumeration caveat for the "cannot disagree" wording, and the filter.rs helper-vs-consumer scope, which also gained a test driving the production `emit_cached_output_filter_log_tail`. Advances #6722. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
`userspace-dp/src/afxdp/poll_descriptor/filter.rs` crossed the 1500-LOC boundary that puts a file INTO the audit (1603 at head) and was never added, so `TestHeatmapNotStale` failed. That is a GO test failing because of a RUST file, which is why it went unnoticed for several rounds: the Rust gates on this branch are green and there is no reason to run the Go suite on a Rust-only change. Regenerated with `bash scripts/refactoring-audit.sh`, not hand-edited. The script also refreshes within-tier LOC numbers for files this branch never touched (compiler_system.go, daemon_nft.go, neighbor.rs, session/mod.rs, compiler_opts.go, compiler_validate_warn.go) and reorders them accordingly. That drift is what the generator produces — the gate compares the audited file SET and TIER assignment, not the LOC figures, which is why master is green while carrying it. A hand-narrowed edit would disagree with the script and be reverted by the next regeneration. `filter.rs` is the only file whose tier changes; it enters at [WATCH]. `userspace-dp/src/afxdp/test_fixtures.rs` also crossed 1500 (995 -> 1548) and correctly gets NO entry: `AUDIT_SKIP_RE` in scripts/refactoring-audit-lib.sh excludes `(^|/)test_[^/]*\.rs$`, and that library's own comment names test_fixtures.rs as one of the files that pattern subsumed in #6232. Confirmed from the rule rather than inferred from its absence in the failure output. Kept as its own commit so the mechanical regeneration is separable from the logic and documentation changes on this branch. Advances #6722. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
# Conflicts: # _Log.md
Two conflicts, resolved by their different natures. _Log.md is union-resolved: every entry from both sides is retained and none is rewritten. The file is no longer append-ordered, so line counts and prefix checks prove nothing about it; the resolution was instead verified structurally, by confirming each pre-merge side diffs into the result with add-hunks only -- 4 hunks from this branch, 1 from master, 0 changed-or-deleted on either side. docs/refactoring-audit-current.txt is a GENERATED artifact and was regenerated from the merged tree with scripts/refactoring-audit.sh rather than union-resolved. Unioning a generated file would have manufactured a heatmap corresponding to no tree that exists: master and this branch each recorded a snapshot of their own source, and the merged source is neither. The regenerated file differs from master's by exactly one line -- a [WATCH] entry for this branch's new userspace-dp/src/afxdp/poll_descriptor/filter.rs at 1603 lines -- and the authoritative pkg/refactoraudit drift canary passes against it. Validation on the merged tree: go build ./... clean; cargo build --release clean; the full userspace-dp cargo suite green (4269 passed, 0 failed, plus the five auxiliary suites), no panics and no wedge. Advances #6713.
Two files conflicted. `_Log.md` is union-resolved: both sides' entries are kept and only the three conflict-marker lines are dropped. Verified after resolving that all 492 branch-only lines and all 6286 master-only lines are present, so neither side's history was lost. `docs/refactoring-audit-current.txt` is a GENERATED artifact and both sides had regenerated it, so a textual merge would have produced a file matching neither tree. It is instead regenerated from the merged worktree with `scripts/refactoring-audit.sh`, and the result is byte-identical to a fresh generation — which is the only resolution that keeps `make audit-check` meaningful. Validation: the anchored conflict-marker sweep over the whole worktree returns nothing, and the regenerated audit file diffs clean against a second independent run of the generator.
Independent re-gate at
|
| this head | :456-460 reverted (master's behaviour) |
|
|---|---|---|
egress[24].zone_id |
Some(0) |
Some(1) |
ledger[24] |
None |
— |
ifindex_to_zone_id[24] |
Some(1) |
Some(1) |
egress_zone_id(24) |
0 | 1 |
| zone pair | (7 wan, 0) |
(7 wan, 1 lan) |
| verdict | Deny, policy_id=4294967295 |
Permit, policy_id=0 |
policy.rs:2679 gates the entire rule walk — exact pair, from-any, to-any, both-any, and junos-global — on from_id != 0 && to_id != 0. With to-zone 0 no rule can match and default-policy deny-all drops the packet. Every WAN→LAN, sfmix→LAN and tunnel→LAN transit flow on a bondless-RETH cluster blackholes.
LAN→WAN survives, because egress ifindex 27 carries a single row — which is precisely why a plain iperf3 smoke would come back green. That is the part worth sitting with: the reference smoke for this project would not have caught it.
Why the obvious refutations fail
Seven were attempted and all fail. The member row is not filtered before Rust (builder.go:41 passes the slice untouched; quarantineCollidingZones only blanks zones). It is not MAC-less in production, and even a MAC-less row would still poison the ledger. reth1.0 does not get a synthetic ifindex — interfaces.go:73-81 requires VlanID > 0 and unit 0 has VlanID 0. And it is not pre-existing: master's populate_egress took the row's own zone last-write-wins, rows are emitted in sorted-name order (ge-0/0/1 < reth1 < reth1.0), so the final write was lan. The regression is introduced by ad4f0c113 — the fix for the previous round's finding.
The asymmetry is the tell: ifindex_to_zone_id[24] is still Some(1), so from-zone lan still works while to-zone lan is unreachable. Only the egress half regresses.
And zone 0 does not fall through to a global policy: JUNOS_GLOBAL_ZONE_ID = u16::MAX, distinct from 0, and the global tier lives inside the to_id != 0 gate.
The remediation is bounded
The member row must not cast an independent vote. Smallest correct change is Go-side: stamp an interface with RedundantParent != "" with its RETH's zone — they are literally one kernel netdev, and nothing can egress ge-0/0/1 that is not reth1.0 traffic. The "disagreement" is an artefact of describing one device with three snapshot rows, not an operator ambiguity. That restores unanimity on ifindex 24/25 without weakening the #6722 guard, because wg0.0 and st0.0 remain genuinely distinct logical units and keep voting.
A Rust-side alternative needs a new InterfaceSnapshot field — pkg/dataplane/userspace/protocol.go:244-269 carries RedundancyGroup but not RedundantParent.
The fix IS bound — that part is not in question
| mutation | result | tests |
|---|---|---|
revert interfaces.rs:456-460 to the row's own zone (the whole fix) |
RED, 8 passed / 2 failed, assertions | unzoned_iface_tunnel_unit_does_not_inherit_a_siblings_zone_via_egress_row_6722, iface_tunnel_egress_row_is_not_upgraded_by_a_later_zoned_unit_6722 |
break the absorbing None |
RED, 8 passed / 2 failed, assertions | unzoned_iface_tunnel_unit_... (ledger-ambiguous precondition), reused_ifindex_across_two_zoned_interfaces_resolves_no_zone_6722 |
Non-blocking, all falsified-by-B1 or adjacent
interfaces.rs:436-438claims "an ordinary single-unit interface is unaffected".reth1is one, and it is affected — this PR's third retracted-class claim.types/forwarding.rs:566-569enumerates the sharing mechanisms and omitsResolveReth; the 209 new architecture-doc lines mentionrethonce, at a pre-existing line, never in the ambiguity section.- Under
default-policy permit-allthe same change is fail-OPEN: zone 0 skips every tier including DENY, so an operator DENY that previously matched by emission-order luck no longer does. Consistent with the pre-existing userspace-dp: zone-less interface (zone-id 0) still evaluates GLOBAL policies — a permit global leaks transit on unzoned ingress/egress #3110 decision, but the new docs argue only the closed direction. - Per-zone egress counters vanish for a conflicted ifindex (
zone_counters.rs:136filtersz != 0), and RT_FLOW logs to-zone 0 for RETH egress — pointing an operator at the wrong place, which is the exact failure mode userspace-dp: a MAC-less xfrmi is dropped by populate_egress, so LAN->tunnel traffic evaluates zone pair (lan, 0) and an operator's explicit permit is DENIED — live on master for bind-interface st0 #6713 set out to fix.
Re-dispatching for the fix plus a regression fixture pinning the three-row RETH shape on both sides.
#6722 B1 (ad4f0c1) made the egress row take its zone_id from the agreement ledger rather than from the row's own zone. The ledger's model of "how can several rows share one ifindex" covered the non-VLAN unit-0 collapse and interface-level tunnels, but missed a third mechanism -- and it is the only one that reaches a shipped topology. ResolveReth (pkg/config/types.go) resolves a RETH to its PHYSICAL MEMBER, and snapshotLinuxName applies it to the reth base row AND its units, so ge-0/0/1, reth1 and reth1.0 are ONE kernel netdev. Junos zones the RETH and never the member, so the member's rows arrive UNZONED and their "no zone" was counted as a dissenting vote. Measured through the full buildSnapshot on docs/ha-cluster-userspace.conf (node 0 -- the topology test/incus/loss-userspace-cluster.env points every HA smoke test at): ifindex 24: [ge-0/0/1="" reth1="lan" reth1.0="lan"] <-- DISAGREE ifindex 25: [ge-0/0/2="" reth0="wan"] <-- DISAGREE DefaultPolicy="deny" With the ledger ambiguous, egress_zone_id(24) returned 0 instead of 1, the zone pair became (wan, 0), and policy.rs's `from_id != 0 && to_id != 0` gate skipped every tier -- exact, from-any, to-any, both-any and junos-global -- so default-policy deny-all dropped the packet. Every WAN->LAN, sfmix->LAN and tunnel->LAN transit flow on a bondless-RETH cluster blackholed. LAN->WAN survived because its egress ifindex has a single row, which is exactly why an iperf3 smoke in the usual direction came back green. The INGRESS half was unaffected throughout (ifindex_to_zone_id[24] still carried lan); that asymmetry was the tell. A ledger is only sound if every row voting on an ifindex is an INDEPENDENT observer of it. A RETH member's row is a PROJECTION of the RETH's netdev, not an observer -- nothing can egress ge-0/0/1 that is not reth1.0 traffic. So carry the member relationship on the wire as a new additive `redundant_parent` field and have populate_interfaces exempt a row that carries it AND has no zone of its own from voting. Stamped on the member's base row AND its unit rows: a member's units alias the matching reth unit too, since a VLAN unit resolves to LinuxIfName(ResolveReth(base)).<vlan>. Measured -- a member carrying `unit 0` + `unit 100 vlan-id 100` puts {ge-0/0/1, ge-0/0/1.0, reth1} on one ifindex and {ge-0/0/1.100, reth1.100} on another, so stamping only the base row would have left the second pair ambiguous. The zone.is_empty() half of the gate is load-bearing, not defensive: a member the operator EXPLICITLY zoned differently from its RETH is a real statement about a real conflict and must keep failing closed. Route not taken, and why. Stamping the member with the RETH's zone in buildInterfaceZoneMap was prototyped and MEASURED to reintroduce #5699. The bondless-RETH address lives on the member netdev, so with a zone the member row enters BuildZoneHostInboundViews and the single live address 10.0.61.1 lands in TWO views with DIFFERENT admit sets ([ssh ping] from reth1.0's per-interface override vs [ssh] from the zone default). The kernel host-inbound chain matches destination address only, so the verdict is order-dependent -- the deterministic false-deny the #5699 comment exists to prevent. Its existing guard cannot fire because it keys on ifc.Units[0] != nil and a RETH member has no units. The full pkg/dataplane suite passes WITH that defect present. Alias-mechanism audit, since the general lesson is to enumerate every projection: the unit-0 collapse and interface-level tunnels are GENUINE logical units and still vote; fab0 is not an alias at all (measured as its own netdev/ifindex -- snapshotLinuxName never calls ResolveFab); bondless-RETH VLAN synthetic ifindexes are unique by construction; and a recycled ifindex across two unrelated interfaces is two genuinely distinct observers that must keep voting. The field is additive both directions -- omitempty on the Go side, serde(default) on the Rust side, and no deny_unknown_fields anywhere in userspace-dp/src/protocol -- and the DEGRADED direction is the safe one: an old helper ignores the key, an old Go binary omits it, and in both cases the member votes and the ifindex stays ambiguous, i.e. the fail-CLOSED behaviour. The wire fixture regen adds exactly one key. Validation: the new binder unzoned_reth_member_row_does_not_strip_the_reths_egress_zone_6722 reds at the unmodified PR head 886ad86 with an ASSERTION (left: 0, right: 1), not a build break. Two over-reach controls stay GREEN there and are each proven to FIRE under their own mutation -- explicitly_zoned_reth_member_still_makes_the_ifindex_ambiguous_6722 reds when the zone.is_empty() half is dropped, and reth_exemption_does_not_leak_to_iface_tunnel_units_6722 reds when the exemption is widened. Both pre-existing #6722 mutation cells still red after this change (revert the ledger-sourced egress zone_id -> 4 red; break the absorbing None -> 4 red), so the B1 ledger guard remains bound. Go row-shape tests bind the producible snapshot -- base stamp, unit stamp, over-reach, JSON round-trip -- and each reds on its own revert. cargo test --release rc=0 (4419 passed), go build ./... rc=0, go test ./pkg/dataplane/... ./pkg/config/... rc=0, gofmt clean on every touched file. Docs: corrected three claims this falsified. interfaces.rs's "an ordinary single-unit interface is unaffected" is true again but now states WHY (the member casts no vote) rather than being deleted; types/forwarding.rs now enumerates all THREE ifindex-sharing mechanisms and names ResolveReth as the one that reaches a shipped topology; the architecture doc gains the B2 section and states BOTH directions of the 0 sentinel -- fail-CLOSED under deny-all (what the reference cluster runs) and fail-OPEN under permit-all, where zone 0 skips the operator's DENY rules too, consistent with the pre-existing #3110 decision to treat zone 0 as unmatchable rather than as a wildcard. Advances #6713. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
Route B implemented at
|
Full re-gate at
|
ifindex_unambiguous_zone_id[42] |
egress_zone_id(42) |
|
|---|---|---|
HEAD df19a5743 |
Some(7) |
7 (= vpnb) |
| exemption reverted | None |
0 (fail-closed) |
st0.0 — a unit the operator deliberately left in no zone — now adjudicates to vpnb, so from-zone lan to-zone vpnb permit matches traffic that previously fell to deny-all. That is bit-for-bit the shape unzoned_macless_unit_does_not_inherit_a_zoned_siblings_zone_6722 exists to hold at 0, and the shape reth_exemption_does_not_leak_to_iface_tunnel_units_6722 claims to guard. The control only holds while redundant_parent happens to be empty, and the operator can set it.
(b) A dangling parent, no tunnel needed. redundant-parent reth1 on ge-0/0/1 with reth1 never defined: ledger goes None → Some(1), egress 0 → dmz. Master answers 0 here, so this commit introduces the flip — not #6713, not master.
(c) Two local members of one RETH — same row shape, same result for the non-projection member.
Refutations attempted and defeated
"Only a physical RETH member can carry it" — refuted by (a): st0 carries it and strict commit accepts. "The RETH's own rows keep it honest" — true only when the reth is defined and RethToPhysical resolves to this member; (a)/(b)/(c) are the three ways that breaks, all commit-accepted. "Some(0) and absent are equivalent so the exemption is a no-op" — true only when the exempt row is alone on its ifindex; all three scenarios have a zoned co-resident. Peer-node member rows were checked and are genuinely harmless.
The fix, and why Rust-side
Encode "this row is a projection of another row's netdev": pre-pass the interface list and exempt a row only if some other row on the same ifindex has name == iface.redundant_parent or name.starts_with(&format!("{}.", iface.redundant_parent)). Three lines, no new wire field. I prefer this to the Go-side narrowing because this function already treats the helper boundary as a fail-closed backstop (#2391/#2409/#2706), so it stays correct against a drifted or hostile snapshot rather than trusting the producer. Plus the (a) over-reach control as a committed fixture.
I am implementing this myself. I directed route B, so its regression is mine to close, and it will be gated by a reviewer who did not write it.
The quarantine probe: reachable, and correctly ruled NOT a regression
I flagged this as the weakest link and asked for it to be measured. It was, and the answer is better than I expected. The construction works exactly as I described — zones_quarantine.go:96-99 blanks the member's zone and thereby manufactures the exemption's own trigger, moving the outcome from 0 to z174. But it restores master rather than exceeding it: buildInterfaceSnapshots emits names sorted, so master's last-write-wins populate_egress already answered z174; the intermediate 0 was the anomaly. Post-fix egress also now agrees with ingress. And the quarantine's own contract is that the colliding zone is dropped as if it never existed, so resolving the surviving RETH statement is coherent.
The residue is that the control test's premise — a deliberately-zoned member is an operator statement — is conditional on that zone not having been quarantined, and nothing pins it. The reverse direction was also checked: quarantine can only turn nonzero into 0 (fail-closed) except through this member exemption. One test with the reasoning written down, so a later reader does not "fix" it back to 0.
Two universal claims are now false and need the qualifier
interfaces.rs:158-162 ("the other ways two rows share an ifindex are all genuine independent observers and still vote") and docs/userspace-dataplane-architecture.md:948 ("reaches nothing else"). st0.0 votes only while st0 carries no gigether-options redundant-parent. The proper fix is B1: make the sentences true rather than qualifying them.
Also docs:954-957 calls the degraded direction "the safe one" while the next bullet correctly says refusing to guess is safe only to the extent the default policy is — under permit-all it is the fail-open direction. And interfaces.rs:135 states "Junos zones the RETH, never the member" as a builder property; zones.go:43-91 has no such rule (measured zoneByInterface[ge-0/0/1] = "z214"). It is operator convention, and the code correctly does not depend on it.
What held up
The B2 diagnosis and the fix's direction are confirmed by measurement against the real docs/ha-cluster-userspace.conf: the row composition, the zones, DefaultPolicy="deny", and the LAN→WAN-survives asymmetry all reproduce. Zone is genuinely untouched — host-inbound, NAT, binding plan, ifindex_to_zone_id and zone_to_rgs bit-identical. fab0 is confirmed not an alias. The three-mechanism enumeration is correct, including the ge-0/0/1.100↔reth1.100 alias, and the unit-row stamp is measurably load-bearing for it (M2 reds only the unit-row test). buildInterfaceSnapshots is confirmed the sole producer, with #6480 partial-republish inheriting next.Interfaces verbatim. Wire compat holds. All four Go mutation cells and the Rust cell are assertion failures, not build breaks. All 13 Rust 6722 tests and the Go 6722 tests green at HEAD.
The #6722 B2 exemption let an unzoned physical RETH member's row abstain from the egress-zone agreement ledger, because that row is a projection of the RETH's own netdev rather than an independent observer of it. The predicate chosen to express "is a projection" was !iface.redundant_parent.is_empty() && iface.zone.is_empty() and that is not the same claim. `redundant_parent` is an unvalidated operator string: `schema_interfaces.go` accepts `gigether-options` under any interface name, and no compiler pass requires the interface it names to exist, to be a `reth*`, or to resolve back to the row carrying it. The Go builder then copies the string onto the base row and every unit row unconditionally. So the gate read "this interface mentioned a redundant-parent", and the operator controls whether that is true. Measured, through the real strict compiler and the real builder: set interfaces st0 gigether-options redundant-parent reth1 set interfaces st0 unit 0 family inet address 10.5.5.1/30 set interfaces st0 unit 1 family inet address 10.6.6.1/30 set security zones security-zone vpnb interfaces st0.1 set security policies default-policy deny-all is ACCEPTED by `CompileConfig`. Under the name-only gate `st0.0` — a unit the operator deliberately left in no zone — is exempted, casts no vote, and `egress_zone_id` moves from the fail-closed 0 sentinel to `vpnb`, so `from-zone lan to-zone vpnb permit` matches traffic that previously fell to `deny-all`. That is the original #6722 fail-open, in the exact shape `reth_exemption_does_not_leak_to_iface_tunnel_units_6722` was written to guard; the control only held while `redundant_parent` happened to be empty. A dangling `redundant-parent` on a physical interface produces the same flip, and there master answers 0 — so the name-only form regressed master rather than merely under-fixing. Narrow the gate to the invariant that actually makes a projection a projection: some OTHER row on the same ifindex must be the parent, either `<parent>` or `<parent>.<unit>`. A pre-pass collects the row names per ifindex, because a projection's parent row may be emitted before or after it — the Go builder walks names sorted, so `ge-0/0/1` precedes `reth1` but `st0` does not, and a backwards-only scan would be order-dependent. Checked on the Rust side rather than in Go because this function already treats the helper boundary as a fail-closed backstop (see #2391, #2409, #2706), so the gate stays sound against a drifted or hostile snapshot instead of trusting the producer. This also makes two claims true that the narrow gate had falsified: the SCOPE comment's "the other ways two rows share an ifindex are all genuine independent observers and still vote", and the architecture doc's "the exemption reaches nothing else". Both are restored as guarantees rather than qualified into accuracy. Validation: reverting only the parent-row requirement reds exactly one test — the new control — as an assertion, rc=101 with zero `error[E...]` lines, 13 passed / 1 failed. Every other 6722 test stays green under that mutation, including the B2 binder `unzoned_reth_member_row_does_not_strip_the_reths_egress_zone_6722`, so the narrowing does not weaken what the exemption fixed and the new control is the only thing that distinguishes the two gates. Full `cargo test --release` 4420 passed / 0 failed; `go build ./...` clean; rustfmt clean within both edited ranges, with no crate-wide `cargo fmt`.
B1 CLOSED at
|
| result | |
|---|---|
| control (fix in place) | 14 passed / 0 failed |
| revert the parent-row clause | 13 passed / 1 failed, rc=101, 0 error[E…] lines |
The single failure is an assertion carrying its own reasoning:
---- dangling_redundant_parent_does_not_exempt_a_genuine_observer_6722 stdout ----
panicked at src/afxdp/forwarding/tests.rs:6060:5:
no row named `reth1` shares this ifindex, so `st0.0` is not a projection of
anything -- it is a genuine unzoned logical unit and must still make the
ifindex ambiguous
Two things that matter more than the red itself:
- The new control is the ONLY test that distinguishes the two gates. All 13 others stay green under the mutation, so nothing pre-existing could have caught this — which is why the hole shipped past a hostile review, an AGY pass, and my own reasoning.
- The B2 binder
unzoned_reth_member_row_does_not_strip_the_reths_egress_zone_6722stays GREEN under the mutation, so the narrowing does not weaken what route B fixed. The fix is strictly a narrowing, not a partial revert.
The two universal claims are now true rather than qualified
The reviewer offered the choice of qualifying interfaces.rs:158-162 ("the other ways two rows share an ifindex are all genuine independent observers and still vote") and docs:948 ("reaches nothing else"), or making them true. I took the second. Both sentences stand unchanged; the architecture doc gains a bullet stating the parent-row requirement, why the name alone is not the invariant, and that a name-only gate would have regressed master on the dangling-parent shape rather than merely under-fixing it.
Gates
cargo test --release 4420 passed / 0 failed · go build ./... rc=0 · rustfmt clean inside both edited ranges — the remaining crate diffs sit at :100/:428/:480/:513, outside my ranges (47-65, 185-224) and pre-existing. No crate-wide cargo fmt was run.
Still open from the re-gate, and my reading
N1 (quarantine) — I am adding the test, not changing behaviour. The reviewer measured that it restores master rather than exceeding it, and that the only way quarantine turns 0 into nonzero is through this member exemption. That reasoning holds. What is missing is a test pinning the resolved-Y outcome with the reasoning written down, so a later reader does not "fix" it back to 0. That is next.
N3/N4 (two doc nits) — mechanical, riding with N1: the "degraded direction is the safe one" sentence needs the permit-all qualifier the very next bullet already supplies, and "Junos zones the RETH, never the member" is operator convention, not a builder property (zones.go:43-91 has no such rule — measured zoneByInterface[ge-0/0/1] = "z214").
Head is 9c6cddc70. The re-gate that found B1 should not gate its own fix, so the next hostile leg goes to a reviewer who did not write this.
Round 7 at
|
| # | parent P | candidate X | equality tested | resolved to |
|---|---|---|---|---|
| 1 | reth | non-reth | L(R(P)) = L(X) |
NON-EMPTY — the designed case. Branch A. |
| 2 | non-reth | non-reth | L(P) = L(X) |
EMPTY on strict (#5832 rejects); non-empty lenient. Branch B / cell K. |
| 3 | reth | reth | L(R(P)) = L(R(X)) |
NON-EMPTY on strict. Covered by NEITHER branch. |
| 4 | non-reth | reth | L(P) = L(R(X)) |
NON-EMPTY on strict. Covered by NEITHER branch. |
Branch A covered row 1, branch B covered row 2. Rows 3 and 4 — the ones where the candidate side takes the ResolveReth arm — were never covered by either. The reduction did not have a gap at case 4; it had a gap at every case where the candidate is a reth name, which is half the table.
Measured, with master as a control on the same configs
set interfaces reth1 gigether-options redundant-parent reth0— strict ACCEPTED. P=reth0, X=reth1, S(P)=S(X)="reth1"→ mark=true. reth1 is marked a projection of reth0, and reth0's own rows land on a netdev name no NIC carries.- Two-cycle
ge-0/0/1 redundant-parent reth1+reth1 redundant-parent ge-0/0/1— strict ACCEPTED, mark=true on BOTH rows of one ifindex. Every row on that ifindex declares itself a non-observer of it; the zone survives only because the zoned row's mark is inert under the Rust gate'szone.is_empty(). reth1 redundant-parent ge-0/0/1(no cycle) — marks nothing, butResolveKernelIfNamereadsRethToPhysicalungated for a dotted ref, soge-0/0/1.0displays asreth1while the dataplane bindsge-0-0-1.
Control: master accepts all three and marks nothing. So the first two are a delta this PR introduces — an ifindex that was ambiguous on master (fail-closed at the 0 sentinel) now resolves a zone. The third is a resolver split master shares: pre-existing, not a regression.
The fix is at the gate, not a fourth conjunct
validateRethMemberStrict gains a clause rejecting any reth* interface that declares gigether-options redundant-parent, placed after the self-parent clause so that message is unchanged, and testing strings.HasPrefix(name, "reth") — the identical test snapshotLinuxName uses, so the two cannot drift.
Why this empties rows 3 and 4 as a property of the code rather than a failed search: rethProjectionMembers only ever considers a candidate that declares a redundant-parent. Once no reth* name may declare one, no candidate it sees is a reth* name, so snapshotLinuxName(name) is unconditionally LinuxIfName(name) and neither row is representable. Only after that is the two-branch reading true. That is the shape I asked for and did not get in five prior rounds — the emptiness now follows from a clause, not from looking and not finding.
Deliberately not widened to "a redundant-parent must name a reth": that would also close row 2, but row 2 is already closed by #5832, and cell K is the only fixture binding that cross-gate dependency — a second gate rejecting K's config would leave K green with #5832 relaxed. Correct call.
Mutation proof
Neutering the new clause reds exactly L1/L2/L3, with H, I and K staying green — no over-reach into the sibling gates. Neutering the self clause still reds H1, so the new clause has not taken over the fixture that binds it. That second control is the one that matters: a new gate that silently subsumes an older gate's coverage leaves the older one vacuous.
Gates and merge
go build/go vet/go test ./... rc 0 (62 packages). cargo test --release --bins --tests 4419 passed / 0 failed. make audit-check up to date.
The _Log.md union proof is worth noting: predicted 1589 ## headings and 3005 - **Timestamp** entries and got exactly that, but the line count came to 80341 against a naive 80342 — because both sides begin their insertion at the same base line with the same blank separator, which git anchored outside the conflict. The arithmetic was not trusted; the structural invariant was checked directly, confirming every line of both parents is still an in-order subsequence of the result. That is the right response to a count that disagrees with a prediction by one.
Superseded framing removed everywhere it appeared — the predicate doc comment, validateRethMemberStrict's doc, the Rust ledger comment, the test-file header, and docs/userspace-dataplane-architecture.md's "exactly two branches" paragraph. The PR body's tail cited rethProjectionNetdevs, a symbol retired two spellings ago, as the current mechanism; corrected.
Flagged and not from this work: TestRetiredLegNeverGainsARotatedCredential_5561 (pkg/api, arrived with the #6645 merge) failed once in a full-tree run and did not reproduce in 8 subsequent runs. This branch touches zero files under pkg/api/. Master-side flake candidate.
Full re-gate owed at this head.
Parent mutation proof at
|
Hostile Claude at
|
Round 8 of #6722, folding four claim/test items from the hostile review at e17de05. Zero blocking; the four-case answer itself was verified and holds. This changes no runtime behaviour on any accepted config -- the one production edit is a commit-check error MESSAGE. F1. The stated tolerant-path bound was false on both halves, at three sites: the `rethProjectionMembers` doc comment, cell M's doc block, and the round-7 `_Log.md` entry. All three said the marked reth "carries no units (the gate's unit clause covers it) and no zone". The parenthetical is the load-bearing part and it is wrong: on the TOLERANT path the unit clause is downgraded to a warning exactly like the reth clause, so it covers nothing there. Measured with the real `CompileConfigLenient` and the real `buildInterfaceSnapshots`, `reth1 gigether-options redundant-parent reth0` beside `reth1 unit 0 family inet address 10.0.61.1/24` compiles, marks `reth1`, and emits a `reth1.0` row. "and no zone" is wrong too -- adding `security-zone dmz interfaces reth1` yields a marked row carrying `dmz`, and cell M's own 2-cycle sub-case already has `ge-0/0/1` marked AND zoned, which the block contradicted two lines further down. The bound is not a property of what the marked row carries. It is two structural facts, and both are bound by assertions rather than by the shape of a fixture: - A withheld vote is always an EMPTY one. The Rust gate is `reth_projection && zone.is_empty()`, so a marked row that names a zone still votes. Withholding can never discard a zone the operator wrote -- it can only let the ifindex resolve a zone another row on it named, or leave it with no contributing row and answer the 0 sentinel. Bound Rust-side with disjoint reds. - UNIT rows are never marked. `buildInterfaceSnapshots` stamps `RethProjection: false` on every unit row unconditionally, so a grandfathered unit-carrying reth keeps voting through its units. F2. Cell M's "no `reth1.*` row exists" loop asserted a property of its own fixture -- that config declares no units, so no production edit could red it -- under a comment claiming the opposite ("The bound, asserted rather than asserted-about"), justified by F1's false parenthetical. Replaced with a sub-case whose marked reth DOES carry a unit: the base row must be marked and the unit row must not be. Measured, stamping the unit row from the projection map instead of the constant `false` now reds cell M as well as cell F; the old loop stayed green under that mutation. F3. The new commit-check message asserted unconditionally that the builder "then marks %q as a PROJECTION of %q and withholds its egress-zone vote". That is false for one of the gate's own shapes: `reth1 gigether-options redundant-parent ge-0/0/1` -- cell L's `reth-names-a-physical` sub-case -- marks nothing, because S(reth1)="reth1" and S(ge-0/0/1)="ge-0-0-1". An operator hitting the non-cycling shape was told a consequence their config does not have. The message now splits the three cases: a reth parent lands the parent's rows on a name no NIC carries and marks the reth; the two-name cycle marks BOTH rows on the shared device; the non-cycling case marks nothing but splits the resolvers. F5. "an ifindex that was AMBIGUOUS -- fail-closed against the 0 sentinel" attributed this branch's agreement ledger to master. Master has no ledger: `populate_egress` inserts one `egress` entry per snapshot row keyed by ifindex, so the LAST row wins, and `egress_zone_id` reads that map and answers 0 when the last row on the ifindex is unzoned. The number was right, the mechanism was not, and the Rust provenance comment on this branch already stated master's mechanism correctly -- so this was an internal inconsistency. Corrected in cell L's block and in the architecture doc, which now say what makes the ledger different: 0 as the principled answer to DISAGREEMENT rather than an artifact of row order. Also corrected in `_Log.md`, since a commit message cannot be edited: the round-7 message (3f99ba4) conceded "four pre-existing failures in pkg/dataplane/userspace" as the `sun_path` 108-byte limit. Under a SHORT TMPDIR there are no failures at all -- the package is ok and the full `go test ./...` exits 0. They were an artifact of the long GOTMPDIR that run used, not a property of the tree, and the message should not have conceded them. Validation. `go build ./...`, `go vet ./...`, `go test ./...` all clean, 62 packages ok, rc 0, under a short TMPDIR. `cargo test --release --bins --tests` in userspace-dp: 4419 passed, 0 failed. gofmt clean on the three Go files. Mutation: stamping the unit row from the projection map reds the new cell-M sub-case; neutering the reth clause still reds exactly L1/L2/L3, so the F3 message rewrite kept the rejection fragment those cells match on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
Round 8 at
|
Hostile Claude interim at
|
Round 9 of #6722, folding two measured claim defects plus three sibling sites found by sweeping the predicate rather than the instance. No runtime behaviour change on any accepted config, and no `.rs` file is touched; the one production edit is a commit-check error MESSAGE. ITEM 1. The tolerant-path bound's fact (2) said a grandfathered reth carrying units "keeps voting through them, so its unzoned units still hold the shared ifindex ambiguous". That holds only for a non-VLAN unit 0, which collapses onto the base netdev. `snapshotLinuxName` sends a VLAN unit to `L(R(name)).<vlan>` -- a different netdev -- so it bears on its own ifindex and says nothing about the base's. Measured with the real lenient compiler and the real builder: reth1 redundant-parent reth0 + reth1 unit 100 vlan-id 100 ifindex 31: [reth0 "lan", reth1 "" MARKED] -> resolves lan ifindex 32: [reth1.100 ""] the same with unit 0 ifindex 31: [reth0 "lan", reth1 "" MARKED, reth1.0 ""] -> resolves 0 Fact (1) -- a withheld vote is always an EMPTY one -- is sound and unchanged: `zone.is_empty()` is a conjunct of the only consumer of the flag, so "resolve a zone another row named, or answer the 0 sentinel" stays exhaustive. The replacement is a STRONGER bound, not a weaker one: a row-3 mark is RUNTIME-INERT. A row-3 mark needs `S(parent) == S(name)` with both `reth*`. `S(name)` is the marked reth's own name unless something declares it as a redundant parent, and if something does, `R(parent)` is a member of `parent` while `R(name)` is a member of `name` -- two different interfaces, so the equality would need a canonicalization collision, which is #5832's shape rather than this one. So the marked netdev is the literal string `rethN`, and on the bondless-RETH model this whole mechanism exists for, a reth is not a kernel device: `buildLinkSnapshot` answers ifindex 0 and both `populate_interfaces` and `populate_egress` skip `ifindex <= 0`. The row never reaches the ledger. Row 4 is NOT inert -- its netdev is a real physical member's -- and is bounded by fact (1) instead. ITEM 2. The reth-parent branch of the new commit-check message asserted unconditionally that the parent's rows land on the netdev name `rethN` and that the builder marks the reth a projection. Measured false when the parent already has a real physical member: ge-0/0/2 redundant-parent reth0 reth1 redundant-parent reth0 -> RethToPhysical[reth0] = ge-0/0/2, marks = {ge-0/0/2} -> reth1 is not marked at all SWEEP, not reported: the predicate behind that finding is "an unconditional sentence inside a multi-branch message", so all three branches were checked rather than the one named. The CYCLE branch is conditional too -- a two-name cycle whose reth has a lower-named third member gives `RethToPhysical[reth1] = ge-0/0/0` and marks NEITHER cycle row, marking the third member instead. The message now asserts only what is unconditional -- the line enters the reth into `RethToPhysical`'s scoring against the parent's real physical ports -- and states each consequence as a possibility, with an inline note recording why, so the next edit does not re-introduce an outcome claim. SWEEP on item 1's predicate found the same over-general inference at three more sites, all now carrying the unit-0 qualifier: cell M's doc block, the tail of `validateRethMemberStrict`'s own doc comment, and fact 5 of the test-file header together with the matching paragraph in docs/userspace-dataplane-architecture.md. Cell F's sentence is left as written -- its config is a non-VLAN unit 0, so it is true as scoped. New cell-M sub-case `reth-carrying-a-vlan-unit` pins the split: the base row marked, the unit row exempt (as in the unit-0 sub-case), and the unit row on a DIFFERENT ifindex with LinuxName `reth1.100`. Validation. `go build ./...`, `go vet ./...`, `go test ./...` clean -- 62 packages ok, rc 0, short TMPDIR. gofmt clean on the three Go files. No `.rs` file is touched this round. Mutations: stamping the unit row from the projection map reds BOTH cell-M sub-cases; collapsing the VLAN unit onto the base netdev reds the new ifindex and LinuxName assertions; neutering the reth clause still reds exactly L1/L2/L3 with H, I and K green, so the message rewrite kept the fragment those cells match on. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
Hostile Claude FINAL at
|
Round 9 at
|
| mutation | result |
|---|---|
| stamp the unit row from the projection map | reds both cell-M sub-cases, and cell F |
| collapse the VLAN unit onto the base netdev | reds the new ifindex + LinuxName assertions |
| neuter the reth clause | still reds exactly L1/L2/L3; H, I, K green |
The third row is the over-reach control and it still holds — the new work has not widened the clause's blast radius.
Gates
go build / go vet / go test ./... rc 0 — 62 packages, short TMPDIR. cargo test --release --bins --tests 4419 passed / 0 failed. gofmt clean.
One process note worth carrying to other lanes
I used
git checkout -- <file>to restore a mutation and it silently discarded an uncommitted edit in that same file.
Redone and verified. The safe restore in a worktree that carries uncommitted work is a file copy, not git checkout — the same class as the restore-clobbers-real-work failure already recorded this session, reached through a different command.
Gate at
|
AGY leg at
|
Round 10 answered the design question before writing the fifth patch — the enumeration IS the defect, and it is being replacedI asked round 10 not to patch a fifth case until it said whether the enumeration approach was
Measured, not asserted — The replacement: stop reconstructing, carry the answerGo computes the egress zone per ifindex, because Go is the only place holding both missing Rust then deletes the entire What it makes unrepresentable: there is no per-row classification predicate left in the What it does not, stated by the round rather than dragged out of it: "a reth member is a Three additions I sent back
Predicted, to be reported as measured: B1 → lan, C1 → lan (both = master), C2 → 0, C3 → 0, |
Round 10 of #6722 arrived with FOUR more runtime spellings of one defect, from two independent reviewer legs — the sixth through ninth across the PR's life, four of them in a single round. Each earlier spelling had been closed by adding a case to a predicate. This round replaces the predicate instead, because the count is the finding. WHY THE ENUMERATION KEPT FAILING The Rust agreement ledger asked "do the rows sharing this ifindex agree about its zone?" and grew an exemption list for the rows whose agreement or dissent turned out to be an artefact. It cannot answer that soundly: a row's Zone is the OUTCOME of two Go-side derivations whose inputs the rows no longer carry. buildInterfaceZoneMap fans one authored reference up to a base and down onto units. Measured on the real builder: for `ge-0/0/1` with unit 0 in `lan` and unit 1 in `dmz`, the BASE row carries "dmz" — a zone nothing on that netdev was ever put in, picked because "dmz" sorts before "lan". snapshotLinuxName then collapses several configured identities onto one netdev. By the time a row exists, "the operator zoned this identity" and "another identity was zoned and this row inherited the words" look identical. Every spelling was an attempt to reconstruct that provenance downstream, and provenance is not recoverable from the outcome. THE REPLACEMENT authoredZoneRefs (zones.go) records the operator's literal `security-zone <z> interfaces <ref>` bindings before any derivation. stampEgressZones (interfaces.go) resolves them through the same aliasing the builder performs and decides, per ifindex, the zone that ifindex EGRESSES into. Three rules, in order: 1. CONTESTED OWNERSHIP -> no zone. Two egress identities on one netdev with no valid reth membership between them. 2. AUTHORED -> that zone. Exactly one distinct literal binding wins; two or more is a real conflict about a real device. 3. TRUNK CARRIER -> the units' unanimous zone, for an ifindex with no authored binding and no unit row on it. This is what keeps the reference cluster's `reth0` base zoned `wan`, matching origin/master. The answer ships as InterfaceSnapshot.EgressZone. rethProjectionMembers and the reth_projection wire field are deleted: there is no per-row classification predicate on the dataplane side any more, so there is nothing left for a new config shape to disagree with. What the helper still does is CORROBORATE — it honours the answer only where a row on that ifindex literally names that zone, preserving the #2391/#2409/#2706 property that a drifted or hostile snapshot cannot conjure a zone no row named. MEASURED, through the real CompileConfig + buildInterfaceSnapshots shape c9b0206 now master B1 ge-0/0/1 units 0/1 lan/dmz, ifindex 10 0 lan lan B1 control: rename dmz->aaa (sorts first) green lan lan B1 control: single unit 0 lan lan lan C1 authored ge-0/0/1.100 aliases reth1.100 0 lan lan C2 WireGuard wg0 as a reth member lan OPEN none none C3 reth-as-member + #5832 collision lan OPEN none none reference HA cluster ifindex 24 / 25 lan / wan same same WHAT IS NOT MADE UNREPRESENTABLE, stated plainly "A reth member is a bare L2 port — no logical units, no tunnel, not itself a reth" is a model rule imported from Junos, not something derivable from the config. It stays a definition, but it now lives in ONE place, stated positively, and is read by both validateRethMemberStrict (hard reject at commit) and egressMemberIsBarePort (the runtime half that holds the line on the tolerant load / peer-sync path, where the rejection is a warning per #1960). C2's missing clause was a hole in that definition — a WireGuard interface configures no logical unit, so the existing unit clause could not see it — not a tenth reconstruction. THE VACUOUS BINDER egress_zone_id's Some(0) short-circuit went vacuous at ad4f0c1, when populate_egress began sourcing EgressInterface::zone_id from the same ledger the fallback read: both arms then returned the same number for every state, and the claimed binder's mutation (filter zero before or_else) still returned 0. The resolver is now a single map read — exactly equivalent for every state, with no branch left to mutate — and the doc says so instead of calling the short-circuit load-bearing. WIRE ENCODING EgressZone is emitted unconditionally and decoded as Option<String>, because ABSENT (a Go binary predating the field) and EMPTY (a decision that the ifindex identifies no zone) must be distinguishable. Absent falls back to the pre-#6722 row-unanimity rule, which has NO exemption list — so every shared-netdev shape this issue was holed by resolves to nothing under it rather than to a wrong zone, and it is strictly narrower than origin/master. CLAIM CORRECTIONS FOLDED The round-9 commit-check remedy pointed the operator at the wrong interface (the %q had moved from `name` to `parent`, so "remove the redundant-parent line from it" named a physical port that carries no such line), and its first consequence was asserted across a branch where it is unreachable. Both are fixed and each consequence now names its sub-branch. Three near-verbatim restatements of the corrected predicate — in compiler_opts.go, compiler_uniformgates_routing_rib_rpm.go and the architecture doc — are rewritten rather than patched, because the mechanism they describe is gone. A trailing comment block in userspace-dp/src/afxdp/forwarding/tests.rs announced "the four tests below" binding a three-conjunct projection gate; there were no tests below it at all. VALIDATION Full Go suite for ./pkg/... + ./cmd/... and the full userspace-dp cargo suite, both green; the protocol wire golden regenerated via XPF_PROTOCOL_WIRE_REGEN=1; make audit-check refreshed and pkg/refactoraudit green. A 15-cell mutation matrix reverts each production hunk individually — 11 Go, 4 Rust — and every cell reds on a named assertion; two cells that first came back green were fixtures that could not distinguish the branch they named, and both were rewritten rather than accepted. Not smoked on the cluster: the ha-cluster-userspace.conf measurement above is a Go-side measurement, not a traffic test.
Round 10 at
|
| head before | now | |
|---|---|---|
| B1 multi-unit base | 0 |
lan (= master) |
| C1 authored dotted base | 0 |
lan (= master) |
| C2 WireGuard member | lan (fail-OPEN) |
"" (= master) |
| C3 two lenient gates | lan |
"" (= master) |
Both B1 controls are in the test, not just reproduced — the dmz→aaa rename asserted as a
precondition so the fixture cannot silently stop distinguishing, and the single-unit-0 case.
C2 was fixed at both ends: the runtime rule and the strict validator, which now rejects a
member carrying its own tunnel — the unit clause could not see it because WireGuard configures
no logical unit.
C5 agreed and fixed. The Some(0) short-circuit went vacuous exactly as Codex described;
the resolver now collapses to a single map read, with no branch left to mutate, and its doc says
so rather than calling the short-circuit load-bearing.
Two things the round found in itself and reported rather than quietly fixing, which is the
part I'd highlight: a trailing comment announcing "the four tests below" with no tests below
it — the file ended at the comment; and two of its own mutation cells came back GREEN,
meaning two fixtures could not distinguish the branch they named. One zoned a member as
ge-0/0/1.0 where that interface configures no unit, so the ref named a row that does not
exist; the other used a fixture whose member row is unzoned, so no row-based rule resolves
anything there either. Both rewritten.
BLOCKING — the wire contract changed at an unchanged version
I asked for the mixed-version matrix to be measured in both directions. It is not in the
report, so I checked:
master protocol.go:37 ProtocolVersion = 4 head protocol.go:37 ProtocolVersion = 4
master control.rs VERSION = 4 head control.rs VERSION = 4
A field was deleted and another added, and both sides still advertise 4 — the version
running in the field. Two binaries that both say "4" now interpret the same bytes differently,
and no gate can distinguish them.
The PR's own Rust doc states the consequence: egress_zone is Option with
#[serde(default)], and the fallback arm is "strictly narrower than origin/master and can
only fail closed." Narrower is right for a bug and wrong for an unsignalled upgrade — an old
control plane with a new helper sends no egress_zone, the helper defaults to None, takes the
narrower arm, and traffic master would forward lands on the default policy. A partial outage
with nothing in the logs pointing at a version.
I am not accepting "the field is optional and unknown fields are ignored, so both directions
degrade rather than corrupt." That argument was made on #6691 this afternoon in nearly the same
words and Codex broke it — and #6691's case was weaker, since it bumped 4→5 and the colliding
version had never shipped. This one collides with the version in the field.
Sent back: measure both directions as measurements; bump to 5 on both sides with the assertion
pinned to equality rather than > 4 (the shape that stays green at exactly the colliding
value); add a required-capability sentinel; and say in the commit message that the wire contract
changed and what upgrade ordering it implies.
Deliberate deltas, stated rather than folded in
Rule 3 (trunk carrier) exists because without it the reference cluster's ifindex 25 fails closed
against both master and head; it fires only when no authored binding and no unit row sit on the
ifindex, so it cannot reopen B1. And the #5832 canonical-collision shape now fails closed
where the previous head resolved the operator's zone from the other name — retiring a fail-open
the old doc admitted.
Smoke
The round says plainly: "a green smoke would not be evidence about B1/C1/C2/C3, the tests
are." That is correct and stays in the PR body. I am scheduling one anyway, because the
resolver and the wire both moved — it is evidence about the reference topology and the
forwarding path, not about the four spellings.
Round 10 of #6722 arrived with FOUR more runtime spellings of one defect, from two independent reviewer legs — the sixth through ninth across the PR's life, four of them in a single round. Each earlier spelling had been closed by adding a case to a predicate. This round replaces the predicate instead, because the count is the finding. WHY THE ENUMERATION KEPT FAILING The Rust agreement ledger asked "do the rows sharing this ifindex agree about its zone?" and grew an exemption list for the rows whose agreement or dissent turned out to be an artefact. It cannot answer that soundly: a row's Zone is the OUTCOME of two Go-side derivations whose inputs the rows no longer carry. buildInterfaceZoneMap fans one authored reference up to a base and down onto units. Measured on the real builder: for `ge-0/0/1` with unit 0 in `lan` and unit 1 in `dmz`, the BASE row carries "dmz" — a zone nothing on that netdev was ever put in, picked because "dmz" sorts before "lan". snapshotLinuxName then collapses several configured identities onto one netdev. By the time a row exists, "the operator zoned this identity" and "another identity was zoned and this row inherited the words" look identical. Every spelling was an attempt to reconstruct that provenance downstream, and provenance is not recoverable from the outcome. THE REPLACEMENT authoredZoneRefs (zones.go) records the operator's literal `security-zone <z> interfaces <ref>` bindings before any derivation. stampEgressZones (interfaces.go) resolves them through the same aliasing the builder performs and decides, per ifindex, the zone that ifindex EGRESSES into. Three rules, in order: 1. CONTESTED OWNERSHIP -> no zone. Two egress identities on one netdev with no valid reth membership between them. 2. AUTHORED -> that zone. Exactly one distinct literal binding wins; two or more is a real conflict about a real device. 3. TRUNK CARRIER -> the units' unanimous zone, for an ifindex with no authored binding and no unit row on it. This is what keeps the reference cluster's `reth0` base zoned `wan`, matching origin/master. The answer ships as InterfaceSnapshot.EgressZone. rethProjectionMembers and the reth_projection wire field are deleted: there is no per-row classification predicate on the dataplane side any more, so there is nothing left for a new config shape to disagree with. What the helper still does is CORROBORATE — it honours the answer only where a row on that ifindex literally names that zone, preserving the #2391/#2409/#2706 property that a drifted or hostile snapshot cannot conjure a zone no row named. MEASURED, through the real CompileConfig + buildInterfaceSnapshots shape c9b0206 now master B1 ge-0/0/1 units 0/1 lan/dmz, ifindex 10 0 lan lan B1 control: rename dmz->aaa (sorts first) green lan lan B1 control: single unit 0 lan lan lan C1 authored ge-0/0/1.100 aliases reth1.100 0 lan lan C2 WireGuard wg0 as a reth member lan OPEN none none C3 reth-as-member + #5832 collision lan OPEN none none reference HA cluster ifindex 24 / 25 lan / wan same same WHAT IS NOT MADE UNREPRESENTABLE, stated plainly "A reth member is a bare L2 port — no logical units, no tunnel, not itself a reth" is a model rule imported from Junos, not something derivable from the config. It stays a definition, but it now lives in ONE place, stated positively, and is read by both validateRethMemberStrict (hard reject at commit) and egressMemberIsBarePort (the runtime half that holds the line on the tolerant load / peer-sync path, where the rejection is a warning per #1960). C2's missing clause was a hole in that definition — a WireGuard interface configures no logical unit, so the existing unit clause could not see it — not a tenth reconstruction. THE VACUOUS BINDER egress_zone_id's Some(0) short-circuit went vacuous at ad4f0c1, when populate_egress began sourcing EgressInterface::zone_id from the same ledger the fallback read: both arms then returned the same number for every state, and the claimed binder's mutation (filter zero before or_else) still returned 0. The resolver is now a single map read — exactly equivalent for every state, with no branch left to mutate — and the doc says so instead of calling the short-circuit load-bearing. WIRE ENCODING EgressZone is emitted unconditionally and decoded as Option<String>, because ABSENT (a Go binary predating the field) and EMPTY (a decision that the ifindex identifies no zone) must be distinguishable. Absent falls back to the pre-#6722 row-unanimity rule, which has NO exemption list — so every shared-netdev shape this issue was holed by resolves to nothing under it rather than to a wrong zone, and it is strictly narrower than origin/master. CLAIM CORRECTIONS FOLDED The round-9 commit-check remedy pointed the operator at the wrong interface (the %q had moved from `name` to `parent`, so "remove the redundant-parent line from it" named a physical port that carries no such line), and its first consequence was asserted across a branch where it is unreachable. Both are fixed and each consequence now names its sub-branch. Three near-verbatim restatements of the corrected predicate — in compiler_opts.go, compiler_uniformgates_routing_rib_rpm.go and the architecture doc — are rewritten rather than patched, because the mechanism they describe is gone. A trailing comment block in userspace-dp/src/afxdp/forwarding/tests.rs announced "the four tests below" binding a three-conjunct projection gate; there were no tests below it at all. MIXED-VERSION WIRE MATRIX, MEASURED This round REMOVES `reth_projection` from a contract both sides read, which is not the same as removing an internal predicate, so both upgrade directions were measured on the real binaries rather than argued. CONFIG_SNAPSHOT_PROTOCOL_VERSION stays at 4. The repo bumps it when an old reader MISREADS a snapshot into a wrong answer — #5488's ErrScopedGlobalZoneSetProtocolIncompatible is the model: an old helper reads only the singular match_from_zone and NARROWS a global deny, a fail-OPEN. Neither direction here misreads. new Go -> OLD helper. The helper at c9b0206, fed the wire shape this builder emits, deserializes it — there is no `deny_unknown_fields` anywhere in userspace-dp/src/protocol, at that commit or this one — reads reth_projection = false from serde's default, and answers ledger[24] = None, to_zone = 0, action = Deny. old Go -> NEW helper. The retired key is ignored, egress_zone arrives None, the compatibility arm requires unanimity, the member's unzoned row dissents, and the answer is 0. Pinned by old_go_wire_shape_into_new_helper_fails_closed_6722. Both directions LOSE the fix in a mixed window and neither invents a zone: a partially-upgraded bondless-RETH cluster keeps blackholing until both halves land, which is what it did already. That is what makes the pair additive. A DELIBERATE BEHAVIOUR CHANGE BEYOND THE FOUR FINDINGS The #5832 canonical-collision-without-a-reth shape — two names that merely canonicalize onto one device, rejected at commit but ADMITTED on the tolerant load / peer-sync path — measured on all three trees: origin/master (edefb75) egress_zone_id(24) = 0 PR head c9b0206 resolves `lan` <-- fail-OPEN here egress_zone_id(24) = 0 At the previous head the collision row is marked a projection (measured: RethProjection = true), its empty vote is withheld, and the ledger resolves the zone the operator wrote on the OTHER name for that device — a fail-OPEN the PR's own doc admitted in passing. `egressRethMemberOf` requires the PARENT to be a `reth*`, so neither name is the other's member port and the ownership is contested. The net effect RESTORES master and retires a delta an earlier round of this PR introduced. It is called out as its own change, not as a side effect of the refactor, because an operator holding such a config sees the difference. THE SHIPPED CLUSTER CONFIG IS NOW A FIXTURE Rule 3 makes docs/ha-cluster-userspace.conf a live dependency of a rule this round introduces: without it, ifindex 25 (reth0's untagged base) fails closed against both master and the previous head. TestShippedClusterConfigResolvesBothRethIfindexes_6722 parses that file through the real parser, the real `${node}` group expansion and the real CompileConfig, and asserts ifindex 24 = lan / 25 = wan — so an edit that moves a zone binding off reth0.50/reth0.80, or adds an untagged unit to reth0, fails here instead of on a cluster. VALIDATION Full Go suite for ./pkg/... + ./cmd/... and the full userspace-dp cargo suite, both green; the protocol wire golden regenerated via XPF_PROTOCOL_WIRE_REGEN=1; make audit-check refreshed and pkg/refactoraudit green. A 15-cell mutation matrix reverts each production hunk individually — 11 Go, 4 Rust — and every cell reds on a named assertion; two cells that first came back green were fixtures that could not distinguish the branch they named, and both were rewritten rather than accepted. The retained AF_XDP shim object is UNCHANGED (no diff under userspace-xdp/ or any .o), so this round adds no shim-ABI risk. Not smoked on the cluster: every measurement above is a Go/Rust unit measurement, not a traffic test, and the forwarding path moves enough here that a real DUT run is owed before merge.
3270fcb to
f03a73c
Compare
Wire-version item: withdrawn. No bump owed — the round measured it and I was applying the wrong criterion.I called this blocking. It is not, and the correction is mine. The matrix, measured on real binaries rather than reasoned about from serde attributes — the Neither direction produces a wrong zone. Both degrade to the same fail-closed answer the The repo's own bump criterion, which I should have asked for instead of importing one. This The decisive argument, which I did not have: every non-test caller of The round also stated plainly what a bump would buy, rather than burying it: it converts a I was generalising from two other PRs today where a bump was owed, and applying that Two other corrections to my earlier commentThe #5832 item is a restoration, not a new behaviour change. I described it as a deliberate
An earlier round of this PR introduced the fail-open; this round retires it. Versus master it C5 stays. I said delete it if no binder can distinguish it. The round measured that it still And rule 3 now has the fixture I asked for
Cluster smoke is mine and is being scheduled at |
Cluster smoke at
|
Round 10 of #6722 arrived with FOUR more runtime spellings of one defect, from two independent reviewer legs — the sixth through ninth across the PR's life, four of them in a single round. Each earlier spelling had been closed by adding a case to a predicate. This round replaces the predicate instead, because the count is the finding. WHY THE ENUMERATION KEPT FAILING The Rust agreement ledger asked "do the rows sharing this ifindex agree about its zone?" and grew an exemption list for the rows whose agreement or dissent turned out to be an artefact. It cannot answer that soundly: a row's Zone is the OUTCOME of two Go-side derivations whose inputs the rows no longer carry. buildInterfaceZoneMap fans one authored reference up to a base and down onto units. Measured on the real builder: for `ge-0/0/1` with unit 0 in `lan` and unit 1 in `dmz`, the BASE row carries "dmz" — a zone nothing on that netdev was ever put in, picked because "dmz" sorts before "lan". snapshotLinuxName then collapses several configured identities onto one netdev. By the time a row exists, "the operator zoned this identity" and "another identity was zoned and this row inherited the words" look identical. Every spelling was an attempt to reconstruct that provenance downstream, and provenance is not recoverable from the outcome. THE REPLACEMENT authoredZoneRefs (zones.go) records the operator's literal `security-zone <z> interfaces <ref>` bindings before any derivation. stampEgressZones (interfaces.go) resolves them through the same aliasing the builder performs and decides, per ifindex, the zone that ifindex EGRESSES into. Three rules, in order: 1. CONTESTED OWNERSHIP -> no zone. Two egress identities on one netdev with no valid reth membership between them. 2. AUTHORED -> that zone. Exactly one distinct literal binding wins; two or more is a real conflict about a real device. 3. TRUNK CARRIER -> the units' unanimous zone, for an ifindex with no authored binding and no unit row on it. This is what keeps the reference cluster's `reth0` base zoned `wan`, matching origin/master. The answer ships as InterfaceSnapshot.EgressZone. rethProjectionMembers and the reth_projection wire field are deleted: there is no per-row classification predicate on the dataplane side any more, so there is nothing left for a new config shape to disagree with. What the helper still does is CORROBORATE — it honours the answer only where a row on that ifindex literally names that zone, preserving the #2391/#2409/#2706 property that a drifted or hostile snapshot cannot conjure a zone no row named. MEASURED, through the real CompileConfig + buildInterfaceSnapshots shape c9b0206 now master B1 ge-0/0/1 units 0/1 lan/dmz, ifindex 10 0 lan lan B1 control: rename dmz->aaa (sorts first) green lan lan B1 control: single unit 0 lan lan lan C1 authored ge-0/0/1.100 aliases reth1.100 0 lan lan C2 WireGuard wg0 as a reth member lan OPEN none none C3 reth-as-member + #5832 collision lan OPEN none none reference HA cluster ifindex 24 / 25 lan / wan same same WHAT IS NOT MADE UNREPRESENTABLE, stated plainly "A reth member is a bare L2 port — no logical units, no tunnel, not itself a reth" is a model rule imported from Junos, not something derivable from the config. It stays a definition, but it now lives in ONE place, stated positively, and is read by both validateRethMemberStrict (hard reject at commit) and egressMemberIsBarePort (the runtime half that holds the line on the tolerant load / peer-sync path, where the rejection is a warning per #1960). C2's missing clause was a hole in that definition — a WireGuard interface configures no logical unit, so the existing unit clause could not see it — not a tenth reconstruction. THE VACUOUS BINDER egress_zone_id's Some(0) short-circuit went vacuous at ad4f0c1, when populate_egress began sourcing EgressInterface::zone_id from the same ledger the fallback read: both arms then returned the same number for every state, and the claimed binder's mutation (filter zero before or_else) still returned 0. The resolver is now a single map read — exactly equivalent for every state, with no branch left to mutate — and the doc says so instead of calling the short-circuit load-bearing. WIRE ENCODING EgressZone is emitted unconditionally and decoded as Option<String>, because ABSENT (a Go binary predating the field) and EMPTY (a decision that the ifindex identifies no zone) must be distinguishable. Absent falls back to the pre-#6722 row-unanimity rule, which has NO exemption list — so every shared-netdev shape this issue was holed by resolves to nothing under it rather than to a wrong zone, and it is strictly narrower than origin/master. CLAIM CORRECTIONS FOLDED The round-9 commit-check remedy pointed the operator at the wrong interface (the %q had moved from `name` to `parent`, so "remove the redundant-parent line from it" named a physical port that carries no such line), and its first consequence was asserted across a branch where it is unreachable. Both are fixed and each consequence now names its sub-branch. Three near-verbatim restatements of the corrected predicate — in compiler_opts.go, compiler_uniformgates_routing_rib_rpm.go and the architecture doc — are rewritten rather than patched, because the mechanism they describe is gone. A trailing comment block in userspace-dp/src/afxdp/forwarding/tests.rs announced "the four tests below" binding a three-conjunct projection gate; there were no tests below it at all. THE WIRE CONTRACT CHANGED. ConfigSnapshotProtocolVersion 4 -> 5. OPERATORS: xpfd and the userspace dataplane helper must be upgraded TOGETHER. `make cluster-deploy` / `make test-deploy` already push and restart both, so the supported path needs no new step. A PARTIAL upgrade — one half moved, the other still on a v4 build — is now refused loudly instead of silently mis-forwarding: the helper rejects the snapshot (its apply_snapshot and bump_fib_generation gates are exact-equality) and the commit ABORTS with ErrEgressZoneProtocolIncompatible naming the observed version and the remedy, while the running helper keeps forwarding its previous-good image. WHY A BUMP, when this repo does not bump for additive fields. This is a field DELETION (`reth_projection`) paired with an addition (`egress_zone`), and a deletion cannot ride an unchanged version: two binaries built either side of it both advertise the same number and read the same bytes differently, with nothing on the wire to tell them apart. The version that would have collided is 4 — the one master ships — so the collision is with binaries that are deployed. And the mixed pairing is not merely "not yet fixed". MEASURED, feeding the v4 Go builder's rows to the v5 helper on docs/ha-cluster-userspace.conf (node 0): ifindex 24 egress zone 0 (origin/master and the matched v5 pair: lan) ifindex 25 egress zone 0 (origin/master and the matched v5 pair: wan) Ifindex 25 settles it: the mixed pairing loses a zone even the PRE-#6722 helper resolved, so it is strictly worse than either endpoint rather than an intermediate state. Under `default-policy deny-all` that is a silent transit outage carrying a version number both sides agree on. An earlier revision of this work claimed both directions "degrade to the same fail-closed answer the pairing gave before this PR"; that claim was too strong and this measurement is what refuted it. The other direction was measured on the real binary too: the v5 Go builder's rows fed to the helper at c9b0206 deserialize (there is no `deny_unknown_fields` anywhere in userspace-dp/src/protocol, at that commit or this one), read `reth_projection = false` from serde's default, and answer `ledger[24] = None, to_zone = 0, action = Deny`. THE GATE IS KEYED ON EQUALITY, NOT `>=` ensureEgressZoneProtocolLocked (manager_compile.go) refuses to commit against a running helper whose advertised ConfigSnapshotProtocolVersion is not exactly this binary's. `>=` would pass a helper NEWER than xpfd, whose own exact-equality gate would then refuse our snapshot anyway — and a `> N` spelling stays green at precisely the value that collides, which is the shape this gate exists to catch. A dedicated test cell drives a helper at ProtocolVersion + 1 and reds under `>=`. Two properties the gate deliberately does NOT have. It takes no config, because every snapshot carries EgressZone and there is no shape to test. And it is conditional on having actually OBSERVED a helper version: `lastStatus` is zero before the first handshake, so firing on "version unknown" would abort every commit made while the helper is down or starting — a brick, not a fence (#1960). When no version can be learned it returns nil and the pre-existing behaviour stands. A DELIBERATE BEHAVIOUR CHANGE BEYOND THE FOUR FINDINGS The #5832 canonical-collision-without-a-reth shape — two names that merely canonicalize onto one device, rejected at commit but ADMITTED on the tolerant load / peer-sync path — measured on all three trees: origin/master (edefb75) egress_zone_id(24) = 0 PR head c9b0206 resolves `lan` <-- fail-OPEN here egress_zone_id(24) = 0 At the previous head the collision row is marked a projection (measured: RethProjection = true), its empty vote is withheld, and the ledger resolves the zone the operator wrote on the OTHER name for that device — a fail-OPEN the PR's own doc admitted in passing. `egressRethMemberOf` requires the PARENT to be a `reth*`, so neither name is the other's member port and the ownership is contested. The net effect RESTORES master and retires a delta an earlier round of this PR introduced. It is called out as its own change, not as a side effect of the refactor, because an operator holding such a config sees the difference. THE SHIPPED CLUSTER CONFIG IS NOW A FIXTURE Rule 3 makes docs/ha-cluster-userspace.conf a live dependency of a rule this round introduces: without it, ifindex 25 (reth0's untagged base) fails closed against both master and the previous head. TestShippedClusterConfigResolvesBothRethIfindexes_6722 parses that file through the real parser, the real `${node}` group expansion and the real CompileConfig, and asserts ifindex 24 = lan / 25 = wan — so an edit that moves a zone binding off reth0.50/reth0.80, or adds an untagged unit to reth0, fails here instead of on a cluster. VALIDATION Full Go suite for ./pkg/... + ./cmd/... and the full userspace-dp cargo suite, both green; the protocol wire golden regenerated via XPF_PROTOCOL_WIRE_REGEN=1; make audit-check refreshed and pkg/refactoraudit green. A 15-cell mutation matrix reverts each production hunk individually — 11 Go, 4 Rust — and every cell reds on a named assertion; two cells that first came back green were fixtures that could not distinguish the branch they named, and both were rewritten rather than accepted. The retained AF_XDP shim object is UNCHANGED (no diff under userspace-xdp/ or any .o), so this round adds no shim-ABI risk. Not smoked on the cluster: every measurement above is a Go/Rust unit measurement, not a traffic test, and the forwarding path moves enough here that a real DUT run is owed before merge. A green smoke would be evidence about the reference topology and the forwarding path; it is NOT evidence about B1/C1/C2/C3 — the tests are, because none of those four shapes appears in the shipped cluster config. Two unrelated flakes seen once each under full-suite parallelism and not reproducible: pkg/ddns TestRFC2136UpsertNeverSendsDeleteRRset (3/3 green on re-run) and slowpath::tests::enqueue_refuses_frame_above_live_mtu ("slow-path worker is not running", 3/3 green on re-run). Neither file is touched by this PR.
f03a73c to
b556066
Compare
Smoke at
|
| master | pre-round-10 head | mixed pairing | |
|---|---|---|---|
| HA ifindex 24 | lan |
0 |
0 |
| HA ifindex 25 | wan |
wan |
nothing |
Ifindex 25 is the counterexample. Master forwards it. The buggy head forwards it. The mixed
window resolves nothing — strictly worse than either endpoint, which is a new outage caused
by the upgrade rather than a not-yet-fixed bug.
The round's own diagnosis of its error is the reusable part: it had measured the shape the
issue is about, where 0 genuinely is the pre-existing bug, and generalised to the whole
snapshot. The compatibility arm is narrower than master everywhere, so the failing case was
never going to be the motivating shape.
The bump is 4→5 both sides, gated on equality with a cell driving a helper at version+1,
and deliberately conditional on having observed a helper version — an unconditional gate would
abort every commit made while the helper is down, a brick rather than a fence.
Still owed and in flight: migrating the 54 fixtures that model the now-unreachable compatibility
arm, and measuring rather than reading the "nothing half-applied before the gate fires" claim.
Both move the head again, so this smoke will be re-run once more when the push is final.
Withdrawing the retransmit flagI flagged 197 retransmits on the 12-stream reverse here, then 342 after the protocol bump, and said two runs were not a trend. There is now a third data point from the same cluster and the same cell on an unrelated PR (#6815): 85. Three values spanning 4× across independent change sets is ambient variance on a shared box under concurrent load, not a signal about this PR. Withdrawing the flag rather than leaving it attached indefinitely. The single-stream cells remain the ones worth watching, and they are zero retransmits in every family across all three runs. |
The protocol bump needs its compatibility story measured rather than reasoned, and measured on the shapes the clusters actually run — not the shape the change was designed around. #6722 bumped 4 -> 5 earlier the same day on a matrix that measured one ifindex and generalised; the counterexample was an ifindex master forwarded, the buggy head forwarded, and the mixed pairing dropped, strictly worse than both endpoints. The motivating shape here is the least likely to expose a compatibility defect, because it is the one the secure-tunnel gate was built for. So the first thing measured is what a mixed pairing does to a cluster with NO secure tunnel. THE REFERENCE CLUSTER ARMS NOTHING. docs/ha-cluster-userspace.conf parses and strict-compiles to 16 interface rows and ZERO SecureTunnel rows, so on the loss userspace cluster the v6 gate is inert and the only mechanism in a mixed pairing is the helper's version-equality check. The mixed-version behaviour there is therefore identical in kind to every previous bump. BOTH OPERAND ORDERS REFUSE, against the real dispatcher. The existing test drove CONFIG_SNAPSHOT_PROTOCOL_VERSION - 1 (a v5 control plane meeting this v6 helper) through handle_stream over a socketpair. The other direction — a v6 control plane meeting a v5 HELPER — cannot be run without a v5 binary, and what it depends on is that `snapshot.version != CONST` refuses whichever side is ahead. That was read from the symmetry of one line; a new cell drives `+ 1` and measures it. Weakening the check to `<` makes the new cell red. THE COMMIT CONSEQUENCE, both directions, with the REAL error. The existing #5679 proof injects a generic apply failure, which leaves the step from "the helper refused on version" to "the commit fails via the deferred path" as a read of the classification. The new daemon test injects the actual string the helper emits, wrapped the way process_control.go and publishSnapshotFailClosedLocked wrap it, in both orders: the commit FAILS, the apply was attempted once, the error is not abort-class, and the peer config-sync is not skipped. The answer is a clean refusal in both directions — no wrong answers. But the two halves of "aborts the commit with the helper still forwarding" belong to different paths, and the asymmetry is deliberate: v6 CP -> v5 helper, no secure tunnel : commit FAILS (deferred), helper refused the snapshot and stays ARMED on previous-good, forwarding v5 CP -> v6 helper, no secure tunnel : same v6 CP -> v5 helper, WITH secure tunnel : commit ABORTS, helper DISARMED A helper that cannot PARSE the snapshot keeps forwarding what it already enforces, because disarming it would convert a handshake disagreement into a dataplane outage. A helper that would MISENFORCE it — a pre-v6 reader that ignores secure_tunnel and plans an AF_XDP binding for the xfrmi — is disarmed, because forwarding under a rule it reads wrongly is worse than not forwarding. Both classifications are asserted so neither can drift into the other, and both planes are asserted at 6 in the same test, since a bump that moved only one side would make every pairing a mismatch including matched deployments.
Round 10 of #6722 arrived with FOUR more runtime spellings of one defect, from two independent reviewer legs — the sixth through ninth across the PR's life, four of them in a single round. Each earlier spelling had been closed by adding a case to a predicate. This round replaces the predicate instead, because the count is the finding. WHY THE ENUMERATION KEPT FAILING The Rust agreement ledger asked "do the rows sharing this ifindex agree about its zone?" and grew an exemption list for the rows whose agreement or dissent turned out to be an artefact. It cannot answer that soundly: a row's Zone is the OUTCOME of two Go-side derivations whose inputs the rows no longer carry. buildInterfaceZoneMap fans one authored reference up to a base and down onto units. Measured on the real builder: for `ge-0/0/1` with unit 0 in `lan` and unit 1 in `dmz`, the BASE row carries "dmz" — a zone nothing on that netdev was ever put in, picked because "dmz" sorts before "lan". snapshotLinuxName then collapses several configured identities onto one netdev. By the time a row exists, "the operator zoned this identity" and "another identity was zoned and this row inherited the words" look identical. Every spelling was an attempt to reconstruct that provenance downstream, and provenance is not recoverable from the outcome. THE REPLACEMENT authoredZoneRefs (zones.go) records the operator's literal `security-zone <z> interfaces <ref>` bindings before any derivation. stampEgressZones (interfaces.go) resolves them through the same aliasing the builder performs and decides, per ifindex, the zone that ifindex EGRESSES into. Three rules, in order: 1. CONTESTED OWNERSHIP -> no zone. Two egress identities on one netdev with no valid reth membership between them. 2. AUTHORED -> that zone. Exactly one distinct literal binding wins; two or more is a real conflict about a real device. 3. TRUNK CARRIER -> the units' unanimous zone, for an ifindex with no authored binding and no unit row on it. This is what keeps the reference cluster's `reth0` base zoned `wan`, matching origin/master. The answer ships as InterfaceSnapshot.EgressZone. rethProjectionMembers and the reth_projection wire field are deleted: there is no per-row classification predicate on the dataplane side any more, so there is nothing left for a new config shape to disagree with. What the helper still does is CORROBORATE — it honours the answer only where a row on that ifindex literally names that zone, preserving the #2391/#2409/#2706 property that a drifted or hostile snapshot cannot conjure a zone no row named. MEASURED, through the real CompileConfig + buildInterfaceSnapshots shape c9b0206 now master B1 ge-0/0/1 units 0/1 lan/dmz, ifindex 10 0 lan lan B1 control: rename dmz->aaa (sorts first) green lan lan B1 control: single unit 0 lan lan lan C1 authored ge-0/0/1.100 aliases reth1.100 0 lan lan C2 WireGuard wg0 as a reth member lan OPEN none none C3 reth-as-member + #5832 collision lan OPEN none none reference HA cluster ifindex 24 / 25 lan / wan same same WHAT IS NOT MADE UNREPRESENTABLE, stated plainly "A reth member is a bare L2 port — no logical units, no tunnel, not itself a reth" is a model rule imported from Junos, not something derivable from the config. It stays a definition, but it now lives in ONE place, stated positively, and is read by both validateRethMemberStrict (hard reject at commit) and egressMemberIsBarePort (the runtime half that holds the line on the tolerant load / peer-sync path, where the rejection is a warning per #1960). C2's missing clause was a hole in that definition — a WireGuard interface configures no logical unit, so the existing unit clause could not see it — not a tenth reconstruction. THE VACUOUS BINDER egress_zone_id's Some(0) short-circuit went vacuous at ad4f0c1, when populate_egress began sourcing EgressInterface::zone_id from the same ledger the fallback read: both arms then returned the same number for every state, and the claimed binder's mutation (filter zero before or_else) still returned 0. The resolver is now a single map read — exactly equivalent for every state, with no branch left to mutate — and the doc says so instead of calling the short-circuit load-bearing. WIRE ENCODING EgressZone is emitted unconditionally and decoded as Option<String>, because ABSENT (a Go binary predating the field) and EMPTY (a decision that the ifindex identifies no zone) must be distinguishable. Absent falls back to the pre-#6722 row-unanimity rule, which has NO exemption list — so every shared-netdev shape this issue was holed by resolves to nothing under it rather than to a wrong zone, and it is strictly narrower than origin/master. CLAIM CORRECTIONS FOLDED The round-9 commit-check remedy pointed the operator at the wrong interface (the %q had moved from `name` to `parent`, so "remove the redundant-parent line from it" named a physical port that carries no such line), and its first consequence was asserted across a branch where it is unreachable. Both are fixed and each consequence now names its sub-branch. Three near-verbatim restatements of the corrected predicate — in compiler_opts.go, compiler_uniformgates_routing_rib_rpm.go and the architecture doc — are rewritten rather than patched, because the mechanism they describe is gone. A trailing comment block in userspace-dp/src/afxdp/forwarding/tests.rs announced "the four tests below" binding a three-conjunct projection gate; there were no tests below it at all. THE WIRE CONTRACT CHANGED. ConfigSnapshotProtocolVersion 4 -> 5. OPERATORS: xpfd and the userspace dataplane helper must be upgraded TOGETHER. `make cluster-deploy` / `make test-deploy` already push and restart both, so the supported path needs no new step. A PARTIAL upgrade — one half moved, the other still on a v4 build — is now refused loudly instead of silently mis-forwarding: the helper rejects the snapshot (its apply_snapshot and bump_fib_generation gates are exact-equality) and the commit ABORTS with ErrEgressZoneProtocolIncompatible naming the observed version and the remedy. That abort is FAIL-CLOSED, not keep-forwarding, and an earlier revision of this message got it backwards. ErrEgressZoneProtocolIncompatible joins requiredProtocolGateSentinels, so the control plane refuses BEFORE it publishes, disarmSnapshotProtocolFailClosedLocked DISARMS the helper, and transit falls to the kernel path (#2138). A bump with no gate would have been the keep-forwarding shape; the gate deliberately trades that availability for a loud, legible refusal. Measured against a recording helper in egress_zone_failclosed_6722_test.go rather than argued off the call graph: the sentinel is returned, a set_forwarding_state{Armed:false} really reaches the helper, and NO apply_snapshot is sent, so nothing is half-applied. A matched-version control proves the gate does not fence the ordinary path. Both properties are mutation-bound: relocating the gate to after apply_snapshot reds the ordering assertion (requests seen: [apply_snapshot status set_forwarding_state{armed:false}]), and dropping the disarm while keeping the gate reds the disarm assertion (requests seen: [status]) — different assertions, so neither cell's RED can be the other's. THE COMPATIBILITY ARM IS DELETED and the 32 fixture literals stamped. The bump makes that arm production-unreachable — every non-test caller sits behind the exact-equality gate — so keeping it would have left ~40 fixtures exercising a snapshot the wire cannot carry. `egress_zone` is a plain String again, the stamping goes through one deliberately-weaker test helper (`test_fixtures::v5`, which fills only EMPTY rows on ifindexes with no explicit stamp), and the one test that covered only the deleted arm is RETIRED with its reason recorded rather than rewritten into something that resembles coverage. The risk in that migration is a fixture that starts passing for a NEW reason, so it was measured on sandboxed copies rather than argued. Mutating the consumer to source the egress row's zone_id from the row's own zone name — origin/master's behaviour — reds exactly ONE test tree-wide, and the same mutation on the PRE-migration tree reds the SAME one. The migration removed no binding. The three migrated tests picked as likely discriminators all stayed green, and the reason is structural: for an ifindex with a single configured identity the ledger's answer and the row's own zone are the same value, so no mutation swapping one for the other is observable. What that did expose is a real gap: the single surviving discriminator asserts a 0 SENTINEL, so the positive direction of B1 — the ledger's non-zero answer beating a dissenting row — had no cell at all. egress_row_zone_is_order_invariant_not_last_write_6722 is that cell: the reference LAN ifindex resolved in emission order and reversed, where reversing puts the unzoned member row last so last-write-wins reads 0 and the ledger reads `lan`. Green on the tree, RED under the mutation with `left: 0, right: 1`. WHY A BUMP, when this repo does not bump for additive fields. This is a field DELETION (`reth_projection`) paired with an addition (`egress_zone`), and a deletion cannot ride an unchanged version: two binaries built either side of it both advertise the same number and read the same bytes differently, with nothing on the wire to tell them apart. The version that would have collided is 4 — the one master ships — so the collision is with binaries that are deployed. And the mixed pairing is not merely "not yet fixed". MEASURED, feeding the v4 Go builder's rows to the v5 helper on docs/ha-cluster-userspace.conf (node 0): ifindex 24 egress zone 0 (origin/master and the matched v5 pair: lan) ifindex 25 egress zone 0 (origin/master and the matched v5 pair: wan) Ifindex 25 settles it: the mixed pairing loses a zone even the PRE-#6722 helper resolved, so it is strictly worse than either endpoint rather than an intermediate state. Under `default-policy deny-all` that is a silent transit outage carrying a version number both sides agree on. An earlier revision of this work claimed both directions "degrade to the same fail-closed answer the pairing gave before this PR"; that claim was too strong and this measurement is what refuted it. The other direction was measured on the real binary too: the v5 Go builder's rows fed to the helper at c9b0206 deserialize (there is no `deny_unknown_fields` anywhere in userspace-dp/src/protocol, at that commit or this one), read `reth_projection = false` from serde's default, and answer `ledger[24] = None, to_zone = 0, action = Deny`. THE GATE IS KEYED ON EQUALITY, NOT `>=` ensureEgressZoneProtocolLocked (manager_compile.go) refuses to commit against a running helper whose advertised ConfigSnapshotProtocolVersion is not exactly this binary's. `>=` would pass a helper NEWER than xpfd, whose own exact-equality gate would then refuse our snapshot anyway — and a `> N` spelling stays green at precisely the value that collides, which is the shape this gate exists to catch. A dedicated test cell drives a helper at ProtocolVersion + 1 and reds under `>=`. Two properties the gate deliberately does NOT have. It takes no config, because every snapshot carries EgressZone and there is no shape to test. And it is conditional on having actually OBSERVED a helper version: `lastStatus` is zero before the first handshake, so firing on "version unknown" would abort every commit made while the helper is down or starting — a brick, not a fence (#1960). When no version can be learned it returns nil and the pre-existing behaviour stands. A DELIBERATE BEHAVIOUR CHANGE BEYOND THE FOUR FINDINGS The #5832 canonical-collision-without-a-reth shape — two names that merely canonicalize onto one device, rejected at commit but ADMITTED on the tolerant load / peer-sync path — measured on all three trees: origin/master (edefb75) egress_zone_id(24) = 0 PR head c9b0206 resolves `lan` <-- fail-OPEN here egress_zone_id(24) = 0 At the previous head the collision row is marked a projection (measured: RethProjection = true), its empty vote is withheld, and the ledger resolves the zone the operator wrote on the OTHER name for that device — a fail-OPEN the PR's own doc admitted in passing. `egressRethMemberOf` requires the PARENT to be a `reth*`, so neither name is the other's member port and the ownership is contested. The net effect RESTORES master and retires a delta an earlier round of this PR introduced. It is called out as its own change, not as a side effect of the refactor, because an operator holding such a config sees the difference. THE SHIPPED CLUSTER CONFIG IS NOW A FIXTURE Rule 3 makes docs/ha-cluster-userspace.conf a live dependency of a rule this round introduces: without it, ifindex 25 (reth0's untagged base) fails closed against both master and the previous head. TestShippedClusterConfigResolvesBothRethIfindexes_6722 parses that file through the real parser, the real `${node}` group expansion and the real CompileConfig, and asserts ifindex 24 = lan / 25 = wan — so an edit that moves a zone binding off reth0.50/reth0.80, or adds an untagged unit to reth0, fails here instead of on a cluster. VALIDATION Full Go suite for ./pkg/... + ./cmd/... and the full userspace-dp cargo suite, both green; the protocol wire golden regenerated via XPF_PROTOCOL_WIRE_REGEN=1; make audit-check refreshed and pkg/refactoraudit green. A 15-cell mutation matrix reverts each production hunk individually — 11 Go, 4 Rust — and every cell reds on a named assertion; two cells that first came back green were fixtures that could not distinguish the branch they named, and both were rewritten rather than accepted. The retained AF_XDP shim object is UNCHANGED (no diff under userspace-xdp/ or any .o), so this round adds no shim-ABI risk. Not smoked on the cluster: every measurement above is a Go/Rust unit measurement, not a traffic test, and the forwarding path moves enough here that a real DUT run is owed before merge. A green smoke would be evidence about the reference topology and the forwarding path; it is NOT evidence about B1/C1/C2/C3 — the tests are, because none of those four shapes appears in the shipped cluster config. Two unrelated flakes seen once each under full-suite parallelism and not reproducible: pkg/ddns TestRFC2136UpsertNeverSendsDeleteRRset (3/3 green on re-run) and slowpath::tests::enqueue_refuses_frame_above_live_mtu ("slow-path worker is not running", 3/3 green on re-run). Neither file is touched by this PR.
b556066 to
f7c8ce7
Compare
Cluster smoke at
|
| path | helper | commit |
|---|---|---|
| plain version mismatch | refuses the snapshot, stays armed on previous-good, keeps forwarding | fails via the deferred path |
| capability gate arms | deliberately disarmed | aborts early |
Corrected in the commit message, _Log.md and docs/userspace-dataplane-architecture.md.
2. The fixture-migration proof I specified could not be produced — and that is the
finding, not a shortfall. I asked for 2-3 of the 54 migrated fixtures shown still RED
under a consumer mutation. Measured: they stay green. Not because the stamp weakened
them, but because for a single-identity ifindex the ledger's answer and the row's own zone
are the same value, so no mutation swapping one for the other is observable there. Those
fixtures bind that a zone reaches the policy decision; they never bound which mechanism
supplied it, before the migration or after.
The property was then proven a better way — the same mutation against the pre-migration
tree, failure sets compared:
| tree | tests the consumer mutation reds |
|---|---|
| pre-migration | unzoned_iface_tunnel_unit_..._via_egress_row_6722 |
| post-migration | unzoned_iface_tunnel_unit_..._via_egress_row_6722 |
Identical. The migration removed no binding. A post-migration count alone would have been
a number with nothing to compare it to.
And the comparison exposed a gap neither of us was looking for: the only discriminator
asserts a 0 sentinel, so B1's positive direction — the ledger's non-zero answer
beating a dissenting row — had no cell at all.
egress_row_zone_is_order_invariant_not_last_write_6722 closes it, RED under the mutation
at left: 0, right: 1, constructed by reversing emission order so last-write-wins reads 0
where the ledger reads lan.
Mutual distinguishability on the fail-closed cells
| mutation | requests the helper saw | reds on |
|---|---|---|
gate relocated AFTER apply_snapshot |
[apply_snapshot status set_forwarding_state{armed:false}] |
ordering |
| disarm dropped, gate kept | [status] |
disarm |
Two one-line edits, two different outcomes, two different assertions — neither cell's RED
can be the other's. That rules out a stale artifact rather than merely making it unlikely.
Hostile Claude at
|
1 participant
Closes #6713
Round 3 — the round-2 fix was inert; it has been removed
Rounds 1 and 2 argued about whether the fallback could hand an "unzoned"
st0.0its zoned sibling's zone. Round 2 added an
ifindex_own_zone_idmap to preventit. That map never had an effect at runtime, because it could not: it was built
from
InterfaceSnapshot.zone, and the Go builder has already propagated.buildInterfaceZoneMap(pkg/dataplane/userspace/zones.go) writesout[base]for a unit-suffixed zone reference, and
snapshotLinuxNamecollapses a non-VLANunit 0 onto the base netdev — so zoning
st0.1putsvpnbon the very ifindexunit 0 forwards out of, on the BASE row, before Rust ever sees it.
Measured on the fixture's own config with the real builders
(
buildInterfaceZoneMap+buildInterfaceSnapshots,buildLinkSnapshotstubbed— it is a package var for exactly this):
The round-2 Rust fixture gave that base row no zone — a snapshot the builder
never emits. And the general case follows: a Rust child→parent propagation can
only add an entry for
parent_ifindex(U), which IS the base row's ifindex, whoseown
Zoneis non-empty in both zone-ref spellings. Soifindex_own_zone_idandifindex_to_zone_idwere the same map on every producible snapshot.Round 3 accepts the propagated behaviour and documents it (see "What a shared
ifindex resolves to" below) rather than carrying an own-vs-inherited flag across
the Go→Rust boundary. Deleted: the map, its insert, and the two #6722 guards that
asserted the impossible shape. Every claim the round-2 comment, architecture-doc
section and
_Log.mdentry made about "own vs propagated" is corrected.Round 3 also fixed a real coverage hole round 2 introduced: it called
egress_zone_id'sSome(0)short-circuit "redundant rather than load-bearing".It is load-bearing, and the guard that should have caught its removal had been
modelling an impossible shape too (an unzoned physical parent carrying a zoned
VLAN unit — the builder emits that parent zoned). Re-pointed at the producible
shape, it reds on the lone mutation again.
The defect
An IPsec secure tunnel (
st0, an xfrmi) isARPHRD_NONE, soforwarding_build::populate_egressnever builds anEgressInterfacefor it —its
src_macgate is unsatisfiable for such a device. The to-zone of aforwarding decision was read from
state.egressalone, so a correctly-zonedtunnel resolved to zone id 0, the reserved "unknown zone" sentinel that
evaluate_policy_result_l3_awaredeliberately refuses to match ANY exact,wildcard or
junos-globalrule against. Every LAN→tunnel packet was adjudicatedas
(lan, 0), no operator-authored permit could apply, and the drop wasattributed to the implicit default policy — pointing every diagnostic an
operator would reach for at the wrong place.
The tunnel is correctly zoned in
ifindex_to_zone_id, which the INGRESShalf of the same zone pair already reads. The egress half simply did not consult
it.
Direction taken, and why the other was rejected
The issue named two candidates. This takes (2), fix the read —
ForwardingState::egress_zone_idfalls back toifindex_to_zone_idwhen theinterface has no
egressrow — and makes that helper the single egress-zoneresolver, so policy adjudication, the #3651 per-zone traffic counter, and the
filter-log egress-zone field cannot disagree.
(1), admit the interface to
state.egress, was rejected on correctness, notjust on blast radius. An
EgressInterfacecarriessrc_mac+bind_ifindex:it asserts an Ethernet frame can be built for the interface and handed to an
AF_XDP bind target. That is false for a link-layer-less xfrmi, and
session_glue::populate_egress_resolutionacts on it — it would setresolution.src_mac = Some([0; 6])andtx_ifindex = <xfrmi>where today itcorrectly leaves
src_mac = Noneand the packet reaches the kernel through theslow-path reinject. It would also change what ~30 other
state.egressconsumerssee (MSS clamp, interface-SNAT source selection, ICMP/PTB reply generation,
zone_to_rgs, WireGuard, fabric, HA owner-RG), none of which this defectrequires.
Scope of the fallback
It fires only when
egresshas no row at all. A row that exists carryingzone_id == 0stays 0, so for every ifindex that HAS an egress row the resolvedto-zone is bit-identical to before.
That
Some(0)short-circuit is load-bearing, not defensive.populate_egressis last-write-wins across snapshot rows, and a non-VLAN unit 0 collapses onto its
base netdev — so a zoned trunk with a declared-but-unzoned unit 0 (
ge-0/0/9zoned
lan,ge-0/0/9.0in no zone, both MAC-ful, both ifindex 90) ends up withegress[90].zone_id == 0whileifindex_to_zone_id[90] == lan. Removing theshort-circuit changes the adjudicated to-zone of every such interface;
unzoned_interface_with_egress_row_stays_zone_zero_6713builds exactly thatshape and reds on the lone mutation (M-A below).
What a shared ifindex resolves to (#6722)
Several logical units can share one ifindex, and
ifindex_to_zone_idis aper-NETDEV map. A MAC-less unit that shares a base ifindex with a zoned sibling
adjudicates under that zone. Zone only
st0.1and the Go builder still emitsthe
st0BASE row carryingvpnb(buildInterfaceZoneMapwritesout[base]for a unit-suffixed zone reference), and
st0.0shares the base's ifindex — sotransit out
st0.0is(lan, vpnb), and afrom-zone lan to-zone vpnb permitapplies to it.
That is stated as a behaviour rather than defended as a guarantee, and it is
defensible because it is the value the ingress half has always used for that
same ifindex: scoping only the egress half to a narrower map would make one
ifindex answer
vpnbinbound and the 0 sentinel outbound, and 0 matches no ruleat all — #6713 again for that config. Junos zones logical UNITS, so
st0.0andst0.1sharing a zone is a genuine parity gap; it needs per-unit identity end toend (the unit-0 ifindex collapse included) and is filed separately rather than
papered over at this one read.
New Go guard
pkg/dataplane/userspace/zone_propagation_6722_test.gopins the twocross-boundary facts the userspace-dp fixtures encode — a unit-suffixed zone
reference zones the base row, and a non-VLAN unit 0 shares the base ifindex — so
a Rust fixture cannot drift back to a snapshot the Go builder cannot emit.
Per-zone traffic counters change attribution without a code change
The three #3651
record_zone_trafficcall sites(
flow_cache_hit.rs,poll_descriptor/mod.rs,disposition.rs) arepre-existing
egress_zone_idcallers that this PR changes the behaviour ofwithout touching a line: a MAC-less tunnel's bytes are now attributed to its zone
instead of zone 0. Reverting any of the three reds nothing in the suite (they are
counters, not forwarding). Noted so a
show security zone-traffic-style delta onupgrade is not mistaken for a regression.
Measured — #6713 stays fixed at round 3
Re-run at the round-3 head through the real chain: snapshot rows measured from
buildInterfaceZoneMap+buildInterfaceSnapshots(both zone-ref spellingsemit identical rows:
st0andst0.0zonedvpn, one ifindex 42, MAC-less) →real
build_forwarding_state→ real FIB → real policy evaluator.2 spellings × 3 next-hops × 2 destinations × 3 policy shapes = 36 cells:
Every permitted cell resolves
from=1 to=7(lan→vpn) withpolicy_id=0, anoperator rule — not
DEFAULT_POLICY_SENTINEL_ID. All 24 control cells(permit scoped to a different pair; operator deny) still deny. 0 of 12
dropped.
The round-1 measurement below is retained for the master-vs-head delta.
Measured (round 1)
Real chain: real Go snapshot (
buildSnapshotWithSchedulerStateAndNATCounters,only
buildLinkSnapshotstubbed to a faithful MAC-less xfrmi) → realbuild_forwarding_state→ real FIB → real policy evaluator. Config: LANge-0/0/0.0(zonetrust) →st0.0(10.5.5.1/30, zonevpn),security ipsec vpn v1 bind-interface <spelling>, static route192.168.99.0/24 next-hop <nh>, default policy deny.Bare
bind-interface st0, the spelling broken on master, 3 next-hops × 2destinations × 2 permit styles = 12 cells:
10.5.5.2(trust,0)Deny pid=4294967295, DROPPED(trust,vpn)Permit pid=0, DELIVERED10.5.5.2(trust,0)Deny, DROPPED(trust,vpn)Permit, DELIVERED10.5.5.2(trust,0)Deny, DROPPED(trust,vpn)Permit, DELIVERED10.5.5.2(trust,0)Deny, DROPPED(trust,vpn)Permit, DELIVEREDst0.0NoRoute→ reinject, delivered unpolicedst0.0(trust,0)Deny, DROPPED(trust,vpn)Permit, DELIVEREDst0.0NoRoute, delivered unpolicedst0.0(trust,0)Deny, DROPPED(trust,vpn)Permit, DELIVEREDst0NoRoute, delivered unpolicedst0(trust,0)Deny, DROPPED(trust,vpn)Permit, DELIVEREDst0NoRoute, delivered unpolicedst0(trust,0)Deny, DROPPED(trust,vpn)Permit, DELIVEREDMaster 8/12 dropped → 0/12 dropped. Exactly the 8 the issue reported.
The dotted
bind-interface st0.0spelling was measured under two kernel models,because on master today it does not reach policy at all:
config.XFRMIfNameAndID(bind-interface)=st0.0, whilesnapshotLinuxNameasks forst0): the tunnel resolves toifindex 0, the FIB returns
NoRoute, and all 12 cells are unpoliced kernelreinject at BOTH revisions. That is userspace-dp: IPsec-passthrough plaintext routes via Linux xfrm with no xpf zone-policy consumer #5619, not this issue, and this PR does
not change it.
post-userspace: resolve secure-tunnel units to the netdev that exists (#5619 PR1) #6691 world in which the two spellings converge): master drops the same
8 of 12, this PR permits all 12. userspace: resolve secure-tunnel units to the netdev that exists (#5619 PR1) #6691's convergence argument becomes true.
state.egress.contains_key(&42)remains false in all 72 measured cells atboth revisions — nothing on the TX path moved.
Fail-closed, proved explicitly
A third policy shape was measured alongside the two permit styles: the only
permit in the config scoped to a DIFFERENT zone pair (
vpn → lan). Under it thetunnel is still denied with
policy_id=4294967295at both master and thisPR, now for the real pair
(trust, vpn)instead of the zone-0 accident. Thenopermitcolumn is 8/12 dropped at master and 8/12 dropped here — unchanged.Tests, each proven to fire
Round-3 battery. Each mutation applied ALONE against a sha256-verified pristine
baseline, with
cargo build --release --bins --testsasserted rc 0 beforethe test run so a build break cannot be misread as a red. Rust rows run
-- --test-threads=1.Some(0)(delete the short-circuit)unzoned_interface_with_egress_row_stays_zone_zero_6713filter_log_egress_zone_idbody →egress-onlyfilter_log_egress_zone_id_reports_a_macless_tunnels_zone_6713forward_request.rs's own call →egress-onlybuild_live_forward_request_logs_a_macless_egress_zone_6713out[base] = zoneNameinbuildInterfaceZoneMapTestHostInboundVlanUnit0KeepsBaseAddress_5699macless_unit_on_a_shared_ifindex_resolves_one_zone_both_directions_6722+ the same two hand-built fixturesM-A is the round-2 regression closed. That guard had stopped firing on the
lone
Some(0)mutation because it modelled a snapshot the builder never emits;re-pointed at the producible zoned-trunk/unzoned-unit-0 shape it reds again on
the lone mutation.
M-B's 7th row is a negative control, not padding. Six tests reach the
resolver through the real builder; M-B reds five of them. The sixth,
unzoned_interface_with_egress_row_stays_zone_zero_6713, correctly staysgreen — it asserts that an ifindex whose
egressrow carrieszone_id == 0stays 0, and deleting the fallback cannot disturb that. A guard that red here
would be asserting the wrong thing. Read the row as 5-of-6 plus a control that
held, not as "7".
M-C and M-D do not red each other's test, so the two call sites remain
independently bound —
forward_request.rsmakes its ownegress_zone_idcalland does not route through
filter_log_egress_zone_id.M-E and M-G together are the round — read them as a pair
Either one alone gives the wrong impression. M-E restores round 2's code
verbatim and reds only the two tests that hand-build a
ForwardingStateandtherefore no longer populate the map that code reads — a fixture artifact. All
six tests that go through the real
build_forwarding_statefrom aConfigSnapshotstayed green: round 2's fix changed nothing on anyproducible snapshot. On its own that reads as "round 2 was pointless, so revert
it."
M-G is why the replacement is a guard and not just a revert. Simulating the
option-(a) direction — an own-zone map that genuinely excludes a base row's
inherited zone — reds the coherence test while the other five real-builder tests
stay green. That test is the only thing in the suite that would catch a future
re-scoping of the egress half, which is precisely the change rounds 1 and 2 kept
reaching for. Deleting the inert map without adding it would leave the next
author free to reintroduce the same mistake against a green suite.
M-F, stated without overclaiming: the pre-existing #5699 test also depends
on the
out[base]write and reds for its own unrelated reason, so the new Goguards are not the only thing holding that Go behaviour. What they add is
pinning the two specific facts the Rust reasoning rests on — the base row
arrives zoned, and unit 0 shares the base ifindex — which #5699 does not assert.
Which tests are fixture artifacts, and why
Two tests hand-build a
ForwardingStaterather than going throughbuild_forwarding_state—build_live_forward_request_logs_a_macless_egress_zone_6713and
filter_log_egress_zone_id_reports_a_macless_tunnels_zone_6713. Under anymutation that changes which map the fallback reads (M-E, M-G), they red
because their hand-built state does not populate the newly-read map — not
because behaviour on a real snapshot changed. Recorded explicitly so this is not
re-litigated: the load-bearing signal in those rows is the six tests that DO go
through the real builder, and under M-E all six stayed green.
Suites
timeout 3600 cargo test --release --bins --tests -- --test-threads=1— rc 0(the timeout wrapper is deliberate: a wedged run surfaces as rc 124 rather than
silence, per userspace-dp: full-suite runs intermittently HANG or fail across MULTIPLE concurrency tests (wg-engine install/reconcile, CoS-lease seqlock) — reproduced on master #6657). 4241 passed, 0 failed, 2 ignored, plus 60 / 8 / 22 / 31 /
1 / 2 in the other binaries. The eight userspace-dp: a MAC-less xfrmi is dropped by populate_egress, so LAN->tunnel traffic evaluates zone pair (lan, 0) and an operator's explicit permit is DENIED — live on master for bind-interface st0 #6713/userspace-dp: resolve the to-zone of a MAC-less egress interface (IPsec xfrmi) #6722 tests were confirmed as
RUN (
test <name> ... ok), not merely absent from a green summary.Neither userspace-dp: full-suite runs intermittently HANG or fail across MULTIPLE concurrency tests (wg-engine install/reconcile, CoS-lease seqlock) — reproduced on master #6657 nor Flaky: current_generation_install_and_delete_still_apply_on_poisoned_shared_mutex asserts on process-global counters, fails intermittently under the full cargo suite #6732 fired.
go build ./...rc 0,go vet ./...rc 0,go test ./...rc 0.afxdp::ha::tests::current_generation_install_and_delete_still_apply_on_poisoned_shared_mutexfails under a plain parallel
cargo test— verified pre-existing: itfails 3/3 runs at
origin/master(ad9591177) in a detached worktree. Itasserts equality against the process-global
SESSION_INSTALL_STALE_IGNORED/SESSION_DELETE_STALE_IGNOREDstatics while sibling tests bump themconcurrently.
make test-rust's--test-threads=1serializes it away.Owed
A cluster smoke is owed — this is a dataplane forwarding change. No
cluster/incus run was performed here; the frame-level reinject-vs-recycle step
is established by reading the two
poll_descriptorarms(
mod.rs:4241/:4290deny-and-recycle; permit falls through to the sharedepilogue's
is_slow_path_eligiblereinject at:5137) rather than by runningthe poll loop.
Correction to a landed commit message (
9c6cddc70)9c6cddc70("userspace-dp: require a parent ROW before exempting a RETHmember") is on this branch and its body says two universals — the SCOPE
comment's "the other ways two rows share an ifindex are all genuine
independent observers and still vote", and the architecture doc's "the
exemption reaches nothing else" — were "restored as guarantees rather than
qualified into accuracy."
That was false when written. The v3 self-parent hole
(
set interfaces st0 gigether-options redundant-parent st0) made a member ofthe unit-0-collapse class exemptable, which is precisely one of the "other
ways" the first universal claims to cover:
st0.0matched its own co-residentst0BASE row, exempted itself, and the ledger resolvedvpnbfor an ifindexwhose own netdev row is unzoned. Measured end to end at
dcd031d58:ledger[42]=Some(32521),egress_zone_id=32521,action=Permit.Both universals hold at the current head, for a different reason than that
commit gives. The paragraph that stood here named
rethProjectionNetdevsandits three-part netdev-SET rule; that spelling is retired and the symbol no
longer exists. The current predicate is
rethProjectionMembers(
pkg/dataplane/userspace/interfaces.go): a row is marked only when itdeclares a
redundant-parent, that parent is a different CONFIGUREDinterface, and
snapshotLinuxNameresolves the parent's base row onto thesame netdev as this row's — the aliasing function asked directly, rather than
a reconstruction of its answer. The unit-0 collapse, interface-level tunnels
and a recycled ifindex satisfy none of that, so they all still vote.
Recorded here rather than fixed in place: a landed commit message cannot be
edited without a rebase, and this branch is not being rebased.
Round 7 — the case split is FOUR-way, and two rows were reachable
Rounds 4-6 reduced
rethProjectionMembers' comparison to two branches ("X isthe member
RethToPhysicalpicked" and "a slash-vs-dash canonicalizationcollision") and described that reduction as exhaustive. It is not.
snapshotLinuxNameisLinuxIfName(ResolveReth(x))for areth*-prefixedname and
LinuxIfName(x)otherwise, and it is applied to the PARENT and tothe CANDIDATE, so each side takes either arm independently. Writing
LforLinuxIfName,RforResolveReth:L(R(P)) = L(X)L(P) = L(X)L(R(P)) = L(R(X))L(P) = L(R(X))Measured at
195fcad51driving the realCompileConfigand the realbuildInterfaceSnapshots, withorigin/master(edefb7570) run as thecontrol on the same configs. Observed values, not expectations:
reth1 gigether-options redundant-parent reth0reth0reth1"reth1""reth1"ge-0/0/1 redundant-parent reth1+reth1 redundant-parent ge-0/0/1reth1/ge-0/0/1ge-0/0/1/reth1"ge-0-0-1"both"ge-0-0-1"bothreth1 redundant-parent ge-0/0/1(no cycle)ge-0/0/1reth1"ge-0-0-1""reth1"Row 3 makes
RethToPhysical[reth0] = reth1, soreth1— the L3 owner — ismarked a PROJECTION of
reth0, andreth0's own rows land on the netdev namereth1that no NIC ever carries. Row 4's two-cycle marks every row on theone ifindex, i.e. declares that the ifindex has no independent observer at all;
the zone survives only because the zoned row's mark is inert under the Rust
gate's
zone.is_empty(). In both, an ifindex that answered the0sentinel onmaster resolves a zone instead. Master accepts the same three configs and marks
nothing (no
reth_projectionfield), so rows 3 and 4 are a delta this PRintroduces.
(Corrected in round 8 — this paragraph originally said master held the ifindex
"AMBIGUOUS". Right number, wrong mechanism: master has no agreement ledger.
populate_egressinserts oneegressentry per snapshot row keyed by ifindex,so the LAST row wins, and
egress_zone_idreads that map and answers 0 when thelast row on the ifindex is unzoned. The ledger this PR adds is what makes 0 the
principled answer to DISAGREEMENT rather than an artifact of row order.)
The third row above marks nothing but is not clean either:
ResolveKernelIfName(pkg/config/types.go) readsRethToPhysicalUNGATEDfor a dotted ref, so
ge-0/0/1.0DISPLAYS asreth1while the dataplane bindsge-0-0-1. Measured on both this branch and master — a pre-existing resolversplit, not a regression, closed here as a side effect.
The fix is the commit gate, not a fourth conjunct
validateRethMemberStrictgains a clause rejecting anyreth*interface thatdeclares a
gigether-options redundant-parent: a reth OWNS the L3 identity ofa redundant pair and is never a member port of another interface. It sits after
the self-parent clause (so that message is unchanged) and tests
strings.HasPrefix(name, "reth")— the identical testsnapshotLinuxNameusesto decide whether to resolve, so the two cannot drift.
Rows 3 and 4 are then empty as a property of the code, not a failed search:
rethProjectionMembersonly ever considers a candidate that declares aredundant-parent, so once noreth*name may declare one, the candidate sideis unconditionally
LinuxIfName(name)and neither row is representable. Onlyafter that is the two-branch reading of the predicate true.
Deliberately NOT done: the stronger clause "a
redundant-parentmust NAME areth*interface" would also close row 2, but row 2 is already closed by #5832and cell K exists to guard that cross-gate dependency — a second gate rejecting
K's config would leave K green with #5832 relaxed, retiring the only fixture
that binds it.
Cells and mutation proof
L(TestRethNamingARedundantParentIsRejected_6722) — the three shapes aboveare commit rejections, the tolerant path still ADMITS each with a warning
(#1960 no-brick), and a control asserts the ordinary bondless-RETH membership
on the SAME two interface names still compiles and is still marked.
M(TestRethNamingARedundantParentMarksTheRethOnTheLenientPath_6722) —records what the tolerant path does with rows 3 and 4 and states the bound.
(The bound as first stated here — "the silenced row carries no logical units
and no zone" — is false on both halves and was corrected in round 8; see below.)
Prose corrected everywhere the superseded framing appeared: the
rethProjectionMembersdoc comment (now carrying the four-row table), thevalidateRethMemberStrictdoc comment (four shapes, not three), the Rustledger comment in
forwarding_build/interfaces.rs, the test-file header, anddocs/userspace-dataplane-architecture.md(its "exactly two branches"paragraph replaced by the measured table).
Gates
go build ./...,go vet ./...,go test ./...— clean (62 packages ok,rc 0).
cargo test --release --bins --testsinuserspace-dp— 4419 passed,0 failed.
make audit-check— up to date. Mergedorigin/master(neverrebased);
_Log.mdwas the only conflict, union-resolved with both sidesverified as pure insertions, predicted 1589 headings / 3005 entries and got
exactly that, and every line of both parents re-verified as an in-order
subsequence of the result.
One transient
pkg/apifailure(
TestRetiredLegNeverGainsARotatedCredential_5561, arriving from the #6645merge) appeared in a single full-tree run and did not reproduce in 8
subsequent runs, isolated or whole-package; this branch touches no file under
pkg/api/. Flagged as a master-side flake candidate, not a regression here.Round 8 — four claim/test items folded (
2baa6095b)Hostile review at
e17de05f1: MERGE-NEEDS-MINOR, zero blocking, and thefour-case answer itself verified with no fifth case. Four items folded. No
runtime behaviour changes on any accepted config — the single production edit is
a commit-check error MESSAGE.
F1 — the stated tolerant-path bound was false on BOTH halves. Three sites:
the
rethProjectionMembersdoc comment, cell M's doc block, and the round-7_Log.mdentry, all saying the marked reth "carries no units (the gate's unitclause covers it) and no zone".
The parenthetical is the load-bearing part and it is wrong: on the TOLERANT path
the unit clause is downgraded to a warning exactly like the reth clause, so it
covers nothing there. Measured with the real
CompileConfigLenientand the realbuildInterfaceSnapshots:reth1 redundant-parent reth0+reth1 unit 0 family inet address 10.0.61.1/24{reth1}reth1(marked, zone"") andreth1.0security-zone dmz interfaces reth1{reth1}reth1marked AND zoneddmz{ge-0/0/1, reth1}ge-0/0/1marked AND zonedlanSo "no units" and "no zone" are both false, and cell M contradicted itself two
lines apart — it asserted the marked row carries no zone and then explained that
"a zoned reth still votes".
The bound is not a property of what the marked row carries. It is two structural
facts, both bound by assertions:
reth_projection && zone.is_empty(), so a marked row that names a zone stillvotes. Withholding can never discard a zone the operator wrote — it can only
let the ifindex resolve a zone another row on it named, or leave it with no
contributing row and answer the
0sentinel. Bound Rust-side with disjointreds (forcing the flag false reds only the unzoned cell; dropping the
zone.is_empty()conjunct reds only the zoned cell).buildInterfaceSnapshotsstampsRethProjection: falseon every unit row unconditionally, so the mark cannever silence an independently addressed L3 interface. (The second half of
this as first written — "so its unzoned units still hold the shared ifindex
ambiguous" — is false for a VLAN unit and was corrected in round 9; see
below.)
F2 — cell M's "no units" loop was non-distinguishing. It looped over the
snapshot asserting no
reth1.*row exists — a property of its own fixture,which declares no units, so no production edit could red it — under a comment
claiming the opposite ("The bound, asserted rather than asserted-about") and
justified by F1's false parenthetical. Replaced with a sub-case whose marked
reth does carry a unit, asserting the base row is marked and the unit row is
not. Measured: stamping the unit row from the projection map instead of the
constant
falsenow reds cell M (and cell F); the old loop stayed green underthat same mutation.
F3 — the validator message asserted a consequence one of its own shapes does
not have. It said unconditionally that the builder "then marks %q as a
PROJECTION of %q and withholds its egress-zone vote". False for
reth1 gigether-options redundant-parent ge-0/0/1— cell L's ownreth-names-a-physicalsub-case — which marks nothing (S(reth1)="reth1"vs
S(ge-0/0/1)="ge-0-0-1"), as that cell's comment already said. An operatorhitting the non-cycling shape was told a consequence their config does not have.
The message now splits the three cases: a reth parent lands the parent's rows on
a name no NIC carries and marks the reth; the two-name cycle marks BOTH rows on
the shared device; the non-cycling case marks nothing but splits the resolvers.
F5 (nit) — master's mechanism. Corrected in the round-7 section above, in
cell L's block and in the architecture doc. Master has no agreement ledger; it
reaches
0by row-sourced last-write-wins. The Rust provenance comment on thisbranch already stated this correctly, so it was an internal inconsistency
between the commit/docs and the code comment.
Round-7 commit message correction.
3f99ba49econceded "four pre-existingfailures in
pkg/dataplane/userspace" as the unixsun_path108-byte limit.Under a short TMPDIR there are no failures at all — the package is
okandthe full
go test ./...exits 0. They were an artifact of the longGOTMPDIRthat run used, not a property of the tree, and the message should not have
conceded them. Recorded in
_Log.mdrather than fixed in place, since a landedcommit message cannot be edited without a rebase and this branch is not being
rebased.
Gates at
2baa6095b.go build ./...,go vet ./...,go test ./...—clean, 62 packages ok, rc 0, short TMPDIR.
cargo test --release --bins --testsin
userspace-dp— 4419 passed, 0 failed. gofmt clean on the three Go files.Mutation: the unit-row stamp reds the new cell-M sub-case; neutering the reth
clause still reds exactly L1/L2/L3, so the F3 message rewrite kept the rejection
fragment those cells match on.
Round 9 — two claim defects, three more found by sweeping (
c9b020695)Hostile review at
2baa6095b: two measured claim defects, non-blocking, andthe F2 replacement +
_Log.mdhandling independently confirmed clean. No runtimebehaviour change, no
.rsfile touched; the one production edit is acommit-check error MESSAGE.
Item 1 — bound (2)'s second sentence was false; the replacement is STRONGER
Fact (1) — a withheld vote is always an EMPTY one — is sound and unchanged.
Fact (2)'s inference was not. "A grandfathered reth carrying units keeps voting
through them, so its unzoned units still hold the shared ifindex ambiguous" holds
only for a non-VLAN unit 0, which collapses onto the base netdev.
snapshotLinuxNamesends a VLAN unit toL(R(name)).<vlan>— a differentnetdev. Measured, real lenient compiler + real builder:
reth1 unit 100 vlan-id 100[reth0 "lan", reth1 "" MARKED][reth1.100 ""]lanreth1 unit 0[reth0 "lan", reth1 "" MARKED, reth1.0 ""]The replacement bound is stronger, not weaker: a row-3 mark is RUNTIME-INERT.
A row-3 mark needs
S(parent) == S(name)with bothreth*.S(name)is themarked reth's own name unless something declares it as a redundant parent — and
if something does,
R(parent)is a member ofparentwhileR(name)is amember of
name, two different interfaces, so the equality would need acanonicalization collision, which is #5832's shape rather than this one. So the
marked netdev is the literal string
rethN; a reth is not a kernel device,buildLinkSnapshotanswers ifindex 0, and bothpopulate_interfaces(:55) andpopulate_egress(:492) skipifindex <= 0. The row never reaches the ledger.Row 4 is not inert — its netdev is a real physical member's — and is bounded
by fact (1).
Item 2 — the reth-parent branch, and the CYCLE branch nobody asked about
The reth-parent branch asserted unconditionally that the parent's rows land on
rethNand that the builder marks the reth. Measured false when the parentalready has a real physical member:
Sweep, not reported. The predicate behind that finding is "an unconditional
sentence inside a multi-branch message", so all three branches were checked
rather than the one named. The CYCLE branch is conditional too — a two-name cycle
whose reth has a lower-named third member:
(The first attempt at this used
ge-0/0/3, which loses the lexicographic tie andreproduces the bare-cycle result — the sweep only bites with a name that wins.)
The message now asserts only what is unconditional — the line enters the reth
into
RethToPhysical's scoring against the parent's real physical ports — andstates each consequence as a possibility, with an inline note recording why so a
later edit does not re-introduce an outcome claim.
Second sweep, on item 1's predicate, found the same over-general inference at
three more sites, all now carrying the unit-0 qualifier: cell M's doc block, the
tail of
validateRethMemberStrict's own doc comment, and fact 5 of the test-fileheader together with the matching paragraph in
docs/userspace-dataplane-architecture.md. Cell F's sentence is left as written —its config is a non-VLAN unit 0, so it is true as scoped.
Binding
New cell-M sub-case
reth-carrying-a-vlan-unit: base row marked, unit rowexempt, and the unit row on a different ifindex with LinuxName
reth1.100.Gates at
c9b020695go build ./...,go vet ./...,go test ./...— clean, 62 packages ok, rc 0,short TMPDIR.
cargo test --release --bins --tests— 4419 passed, 0 failed(unchanged, and no
.rsfile was touched this round). gofmt clean on the threeGo files.
🤖 Generated with Claude Code
https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi