session: give sessions a true ingress-interface identity - #6928
Conversation
A session carried only its ingress ZONE, so `show security flow session interface <name>` and the matching `clear` could only ask whether `<name>` was bound to that zone. A session on interface X therefore matched a filter for EVERY sibling interface Y of the same zone -- a wrong-answer-shaped result on `show`, and on `clear` one that destroys state the operator did not target. #4792 widened the CLI's zone map from the first bound interface to all of them, which is as precise as a zone-derived answer can be. This adds the datum that makes it exact. `SessionMetadata` gains `ingress_ifindex` + `ingress_vlan_id`, stamped ONCE at install in `poll_descriptor` (both the transit forward entry and the host-inbound LocalDelivery entry) from the frame's `UserspaceDpMeta` -- the binding the packet actually arrived on, plus its 802.1Q tag -- and never re-derived from the zone afterwards, since re-deriving is the approximation being removed. `publish_conntrack` mirrors both into the conntrack value the Go control plane reads. The VLAN half is load-bearing, not decoration: two units of one trunk NIC (reth0.50 and reth0.80 on the loss cluster's WAN NIC) share a physical ifindex, so without it they alias onto the parent and the cross-interface match returns for exactly the topology this project tests on. The pair is deliberately the same {parent ifindex, VLAN} identity the Go side already resolves the EGRESS interface name by, so one map serves both directions. `0` means "no ingress identity carried" and is never a valid ifindex. Three populations legitimately carry it: the reverse companion (its true ingress is the forward flow's egress, unresolved at install), a peer-synced session, and any session installed by a pre-#4983 helper. The peer case is a design decision, not an omission -- an ifindex is NODE-LOCAL, so node 0's ge-0-0-1 and node 1's ge-7-0-1 are different numbers for the same logical RETH member, and shipping the peer's value would render a confidently WRONG interface name locally, strictly worse than approximating. The identity is therefore not carried on the cluster wire, and no serde/JSON control-protocol field is added (protocol_wire_v1.json is unchanged). ABI: the two fields join the shared C conntrack struct -- not the sync-only trailing fields -- growing `session_value` 136 -> 144 and `session_value_v6` 184 -> 192. The u32 lands on the existing 8-byte boundary and the u16 inside the tail pad it forces, so the pair costs 8 bytes, not 16. All three mirrors move in lockstep (C header, Rust BpfSessionValueV4/V6, Go bpfSessionValue{,V6} whose tail pad is DECLARED for the #6082 zero-copy marshal path) and both size asserts are bumped. As with the #5460 flags widen, `sessions`/`sessions_v6` are pinned maps, so a rolling deploy cannot cross this: the #5307 pre-flight refuses while the old daemon is still forwarding and the remediation is a full dataplane reload with brief downtime. Validated: `cargo test --release` green (4242 + 124 across the suites). `build_conntrack_value_stamps_ingress_identity_{v4,v6}_4983` go RED on an assertion (`left: 0, right: 24`) with the publish hunk reverted to `ingress_ifindex: 0`, and the over-reach guard `ingress_identity_does_not_occupy_the_fib_egress_slots_4983` -- which pins that the ingress identity does NOT leak into the FIB egress slots the CLI resolves the egress name from -- stays GREEN under that revert. Advances #4983. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
`show security flow session interface <name>` and the matching `clear`
read the session's ingress identity now that the dataplane records it,
instead of asking whether `<name>` is bound to the session's ingress
ZONE. A session on interface X no longer matches a filter for a sibling
interface Y of the same zone -- on `clear`, the case where the
approximation destroyed state the operator did not target.
`resolveIngressIfaces` is the ingress twin of the existing
`resolveEgressIfaces` and resolves {IngressIfindex, IngressVlanID}
through the SAME {parent ifindex, VLAN} name map the egress side already
uses, so one map defines one interface identity for the whole filter.
The map field is renamed `egressIfacesMap` -> `ifaceNamesByKey` to say
so; it is read by both directions now, and the rename is confined to
pkg/cli. `cli_clear.go` needs no edit -- it shares `matchesV4`/`V6` and
already calls `populateIfaceMaps` (#1827 PR-3).
The fallback is deliberate and documented at the call site, not an
accident of a zero value. An ingress ifindex of 0 means "no identity
carried" and is never a valid ifindex; the reverse companion, every
peer-synced session (an ifindex is node-local, so it is not carried
across the cluster wire), and any session installed by a pre-#4983
helper mid rolling upgrade all carry it. Those answer from the zone
approximation exactly as before -- 0 must never read as "matches
nothing", which would silently hide them from `show` and `clear`, and
never as "matches everything". A NON-ZERO ifindex the running config
cannot name (an interface deleted since install, a tunnel/fabric ingress
with no config unit) falls back the same way: an unnameable identity is
not evidence the session is uninteresting.
Tests. The binding fixture uses a FOUR-member trust zone across three
physical NICs -- a single-interface zone makes the assertion true for
free, and with two a "fix" that merely swapped X for the zone's other
interface would still pass. Two of the members are VLAN units of one
trunk NIC, mirroring the loss cluster's reth0.50/reth0.80, which pins
the VLAN half of the identity. Every fixture interface carries `unit 0`
because that is how a real Junos config is written and because a
unit-less interface produces NO entry in the name map -- without it the
assertions would pass with the fix reverted.
`TestIngressIdentityAbsentFallsBackToZone4983` holds a session WITH an
identity and one WITHOUT in the SAME test and asserts they are treated
DIFFERENTLY; a test where all or none carry the field cannot see the
fallback branch at all.
Validated: with both `resolveIngressIfaces` call sites reverted to
`f.zoneIfaces[val.IngressZone]`, `go test ./pkg/cli -run 4983` exits 1
with every failure an assertion message (`go vet ./pkg/cli` still exits
0 -- not a build break), and green on restore.
`TestIngressIdentityDoesNotDisturbEgressMatching4983` -- the FIB egress
result, the #4792 egress zone fallback, and a non-interface filter --
stays GREEN under that revert, as does the unresolvable-ifindex case.
`go test ./pkg/cli ./pkg/dataplane ./pkg/dataplane/userspace ./pkg/daemon
./pkg/cluster ./pkg/api ./pkg/grpcapi -count=1 -race` green;
`go test ./pkg/refactoraudit/` green.
Advances #4983.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
The session shape and the conntrack ABI both changed, so the module docs that describe them move in the same work. `userspace-dp/src/session/README.md` gains a section next to the sibling per-session stamping sections (#2785 log flags, #3056 policy id): where the identity is recorded, that it is stamped once and never re-derived from the zone, how the CLI resolves it through the same {parent ifindex, VLAN} map the egress side uses, and the full contract for `0` -- the three populations that legitimately carry it and why the fallback is to the zone approximation rather than to "matches nothing". It also records the ABI growth (136 -> 144 / 184 -> 192) and the pinned-map consequence: as with the #5460 flags widen, a rolling deploy cannot cross it and the remediation is a full dataplane reload. `docs/session-sync-architecture.md` records the opposite-on-both-counts case next to the sync-only trailing fields it sits beside: the ingress identity IS part of the on-map conntrack ABI, and it is deliberately NOT synced. An ifindex is node-local -- node 0's ge-0-0-1 and node 1's ge-7-0-1 are different numbers for the same logical RETH member -- so carrying the originating node's value would make the importing node name the WRONG interface. A peer-synced session therefore lands in the same "no identity carried" branch as a pre-#4983 one, and the CLI answers it from the zone exactly as it did before. Advances #4983. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
|
HELD as draft pending a design decision — do not merge. Two options were raised after this PR was pushed, and both check out against the code: A no-ABI side table avoids the forced dataplane reload entirely. The ABI pre-flight only refuses when a live pin already exists: The stored identity can be cluster-stable rather than node-local. That second point matters more than the downtime: as scoped here, every peer-synced session lands in the absent-field branch, so after a failover the new master degrades to exactly the pre-#4983 zone approximation — the diagnostic stops working in the case where diagnostics matter most. The Step-0 absence proof and the fail-on-revert probes in this PR stand either way; the ABI mechanism is what is under review. |
Resolve the sole conflict, _Log.md, by union: every entry from both sides is retained and none is rewritten. The file is no longer append-ordered, so line counts and prefix checks say nothing useful about the result; the resolution was verified structurally instead, by confirming that each pre-merge side diffs into the merged file with add-hunks only and no changed-or-deleted hunk on either side. Every other path merged without conflict. Because a clean textual auto-merge can still break compilation when a signature moves on one side, that was confirmed by building rather than by inspection: go build ./... clean on the merged tree, and cargo check --all-targets clean for the Rust crate. Advances #6928.
The #4983 feature is a three-link chain — the poll body PRODUCES the session's ingress identity, publish_conntrack MIRRORS it onto the map, and the CLI filter CONSUMES it — and only the last two were pinned. The Rust tests hand-set a SessionMetadata and the Go tests hand-build a dataplane.SessionValue, so both sides of the middle were bound and the producer was not: replacing the four `meta.ingress_*` lines in poll_binding_process_descriptor with `0` left the whole Rust suite green. A later refactor of that 5000-line function could drop them and every session would silently carry ingress_ifindex 0, returning `show/clear security flow session interface X` to exact pre-#4983 behaviour with CI green and no counter or log to notice. tests_session_ingress_identity.rs drives the real poll_binding_process_descriptor with a permitted LAN -> WAN SYN ingressing on a VLAN unit of a TRUNK NIC (parent ifindex 11, VID 50) whose sibling unit on the same parent is the egress and whose parent resolves to a different zone. A separate test asserts the reverse companion installed by the same call still carries 0/0, so the two arms are distinguished rather than both satisfied by one constant. build_missing_neighbor_session_metadata also hardcoded 0/0 — for a FORWARD session, with `meta` in the caller's scope 32 lines above. That seed is installed whenever a flow's first packet races an unresolved ARP/NDP, is published to the conntrack map at install, and is never re-installed (retry_pending_neigh replays the buffered frame and takes no &mut SessionTable), so such a flow kept the zone approximation for its whole life. It now takes the ingress binding from the frame. The detailed show output derived its "In: ... If:" column from the ingress zone's first interface, so once the filter became exact the two disagreed: an interface-filtered show selected the right sessions and printed the wrong interface for each. A sessionIngressIf closure — same shape as the existing sessionEgressIf — resolves it from the recorded identity. Claims are scoped and corrected. The consumer change is in the in-daemon CLI only; pkg/grpcapi (which the remote cli binary uses for show AND clear) and pkg/api keep the zone approximation in its pre-#4792 first-interface-only form, and this branch's diff against both is empty. The shipped population list named the reverse companion — whose fallback is inert, since every CLI show/clear call site skips IsReverse != 0 before filtering — and omitted the seed path and the host-outbound GRE path in tunnel.rs. Two ABI notes still said "size-asserted at 136"; the assertions are 144/192. Validation. cargo test --release: 4257 passed / 0 failed / 2 ignored in the main binary, every other target ok. go build ./... exit 0; go test on pkg/cli, pkg/dataplane, pkg/grpcapi, pkg/api all ok. Mutation matrix, each applied by edit and restored to a clean git diff: - both poll-body stamps -> 0, measured against the FULL Rust suite: 4256 passed / 1 FAILED, the single failure being the new transit test ("... left: 0, right: 11"). Nothing else among 4257 tests sees it. - transit ingress_vlan_id -> 0 alone: same test RED on the VID assertion ("left: 0, right: 50") — the two halves bind independently. - seed stamp -> 0 with both poll-body stamps intact: the seed test RED ("left: 0, right: 11"), transit test GREEN. - reverse companion -> meta.ingress_*: the over-reach guard RED ("left: 11, right: 0") — proven to fire, not merely to stay green. - display call sites -> zoneIfaces[val.IngressZone]: the column test RED on both the v4 and v6 arms; the egress-column guard stayed GREEN. Every RED is an assertion failure, not a build break. Advances #4983.
Parent mutation matrix at
|
| cell | site reverted to 0 | result |
|---|---|---|
| control | none | rc=0, unit binary 4257 passed / 0 failed |
| A1 | LocalMiss stamp, poll_descriptor/mod.rs ~1927-1928 |
rc=0, 4257 passed — UNBOUND |
| A2 | ForwardFlow stamp, poll_descriptor/mod.rs ~2422-2423 |
rc=101 RED |
| B | MissingNeighborSeed call-site args, ~4812-4813 | rc=101 RED |
A2's sole failure was
poll_descriptor_transit_install_stamps_ingress_binding_4983.
A2 and B going RED is the internal control that makes A1 interpretable.
The same mutate → rebuild → test loop reddens two other sites, so A1's green
is a genuine "nothing binds this", not a stale-test-binary artifact.
Root cause
userspace-dp/src/afxdp/tests_session_ingress_identity.rs contains exactly
three tests — transit (ForwardFlow), reverse-companion (deliberately 0), and
the missing-neighbor seed — and zero occurrences of localmiss,
local_miss, helper_local, or install_helper. No test drives the
host-inbound LocalMiss install path
(install_helper_local_session_on_miss → publish_bpf_conntrack_entry) at
all, so its stamp could be 0 indefinitely without any test noticing — in a
PR whose claim is that every forward session carries a true ingress identity.
Owed before merge
A test that drives the LocalMiss install end to end and asserts the stamped
ingress_ifindex/ingress_vlan_id, with RED-on-revert observed. The fixture
must use distinct non-zero values (not 0, not 1, and not the transit
test's values) — equal or default values make the assertion vacuous, since it
must distinguish "stamped correctly" from "stamped from the wrong source".
Note on a mis-aimed first attempt
My first matrix targeted neighbor_dispatch.rs:449
(ingress_ifindex: meta.ingress_ifindex as i32) for the seed cell. That is
the LearnedNeighborKey used for dynamic-neighbor learning, not the
session-metadata stamp, so that cell would have proved nothing either way. The
seed path stamps via build_missing_neighbor_session_metadata
(neighbor_dispatch.rs:606-625), whose struct init uses field shorthand — the
real values are passed from the poll_descriptor call site, which is what
cell B mutates.
The reverse companion (~2749-2758) is deliberately 0 and documented, so it
is not a cell.
A per-site mutation matrix at the previous head found the LocalMiss (host-inbound) install's ingress stamp UNBOUND: reverting `ingress_ifindex` / `ingress_vlan_id` to 0 in the `local_metadata` literal left the whole Rust suite green at 4257/4257. The same mutate/rebuild/test loop applied to the ForwardFlow install and to the missing-neighbor seed call site both went RED, so the green was a real "nothing binds it" rather than a stale-binary artifact. The cause is a coverage hole, not a production defect: the stamp is present and correct at the site, but `tests_session_ingress_identity.rs` had zero occurrences of `local_miss` / `install_helper` — no test drove the host-inbound install path at all. Host-bound flows (management SSH, BGP, syslog-TCP, RPM/feed fetches) are exactly the population an operator filters by interface when the firewall itself is the endpoint, and on a multi-interface zone the pre-#4983 fallback answers with every sibling. Add `poll_descriptor_local_miss_install_stamps_ingress_binding_4983`, which drives one host-bound TCP SYN through the real `poll_binding_process_descriptor` body. The destination is the ingress unit's OWN address, so the session-miss resolution is `LocalDelivery` and the poll takes `install_helper_local_session_on_miss` -> `publish_bpf_conntrack_entry`; the test then asserts the installed `SessionOrigin::LocalMiss` metadata carries the frame's binding. The fixture adds a THIRD LAN unit, `reth2.70` on its own trunk: {parent ifindex 7, VLAN 80}, logical ifindex 17. Every number is disjoint from every other value the stamp could have been copied from — 7 is neither 0 nor 1, so no default satisfies it; 7 != 80, so a transposition of the two halves is RED; 7 != 17, so stamping the RESOLVED LOGICAL unit instead of the received binding is RED; 7 is neither zone id; and {7,80} is neither the transit fixture's {11,50} nor its egress sibling's {11,80}, so no single constant satisfies this test and the transit test at once. VLAN 80 is deliberately reused from reth0.80 on a DIFFERENT parent, because the ingress map is keyed by the {parent, VLAN} pair: that separates "recorded the pair this frame arrived on" from "found whichever unit carries VLAN 80". Fixture liveness is asserted before the metadata (`dbg.local == 1`, exactly one installed session, the binding resolves to the lan logical unit), so a fixture that stopped admitting the flow, or that started taking a transit arm, is RED rather than vacuously green. Validation, one production site mutated per cell, full `cargo test --release` each time. Cell A (both fields to 0 at the LocalMiss literal) rc=101 with the new test as the ONLY failure — "the host-local session must record the ifindex of the binding its first packet arrived on ... left: 0 right: 7" — while the transit, reverse-companion and neighbor-seed tests stay green. Cell B (restore the ifindex, zero only `ingress_vlan_id`) rc=101 with "... must record the ingress 802.1Q VID ... left: 0 right: 80", proving the VLAN half binds independently instead of hiding behind the first assertion. Restored: `cargo test --release` rc=0, 4382 passed / 0 failed (4258 in the main binary, +1 over the baseline); `go test ./pkg/cli/... ./pkg/dataplane/...` rc=0. Also re-derive the site enumeration rather than trusting it. The 11 `SessionMetadata` literals in `userspace-dp/src` include two under `#[cfg(test)]`; the nine live ones are the four poll populations plus the synthesized reverse companion, the peer-synced wire ingest, the host-outbound GRE path, the flow-cache seed, and `promote`, which mutates an existing metadata and preserves the pair. Cross-checked against every session install site and every `publish_bpf_conntrack_entry` call site, plus a grep for direct writes to the two fields (there are none outside the literals). NAT64 and fabric-redirect ride the ordinary ForwardFlow install and there are no ALG child-session installs, so no fifth site exists. The documented zero-populations are unchanged, hence no edit to the field contracts in `session/entry.rs` or `pkg/dataplane/types.go`; `session/README.md` records that all three stamping sites now carry a per-site fail-on-revert test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
Master advanced while the LocalMiss coverage fold was in review. Only _Log.md conflicted; resolved as a union keeping both sides' entries in full (85 lines from the PR branch, 87 from master), which is correct for an append-only activity log recording unrelated work. No source file conflicted, so the fold's hunks carry across unaltered.
A hostile review claimed the transit stamp could be rewritten as
`if owner_rg_id > 0 { meta.ingress_ifindex } else { 0 }` with the whole
suite still green. Verified rather than assumed: that mutation leaves
all four ingress-identity tests GREEN. The escape is not academic — it
is exactly the shape a future HA-scoping change would take, and it would
silently drop the identity on every STANDALONE firewall, the majority
deployment, with CI reporting nothing.
The cause is that the existing fixture is a chassis-cluster topology, so
its installed session always carries a non-zero `owner_rg_id`; an
`owner_rg_id`-conditional stamp is indistinguishable from an
unconditional one there. Add a second arm that replays the SAME driven
flow on a standalone topology and asserts the identity is still stamped.
Both halves of "not clustered" are required, and the driver now records
why. `enforce_ha_resolution_snapshot` (forwarding/ha.rs) turns a
resolution whose `owner_rg_id <= 0` into `HAInactive` when `ha_state` is
NON-empty, because that combination means a cluster node whose snapshot
predates the RETH RG propagation fix. Clearing only the redundancy
groups produced an arm that installed ZERO sessions and went red on the
liveness assertion — a fixture that never drove the path at all, which
would have looked like a passing mutation check for entirely the wrong
reason. The arm therefore also asserts `owner_rg_id == 0` as an explicit
precondition, so it cannot quietly reacquire an RG and degrade into a
duplicate of its sibling.
Validation: with the `owner_rg_id` gate applied, the new arm is RED on
the IDENTITY assertion — "a standalone firewall's transit session must
record its ingress binding too ... left: 0 right: 11" — while the
chassis-cluster sibling, the LocalMiss test, the seed test and the
reverse over-reach guard all stay GREEN. Restored: `cargo test --release`
rc=0 with 4259 passing in the main binary; `go test ./pkg/cli/...
./pkg/dataplane/...` rc=0.
Also correct two documentation defects found in the same review.
Ten ABI size figures in `pkg/dataplane/bpf_session_value.go` still read
136/184 — including "mirrors C struct session_value exactly (136 bytes)"
and "fails if the size drifts from 136". The real sizes are 144/192; the
PR had updated `types.go` and the parity test but missed the file that
documents the ABI contract. Corrected, with the earlier values kept as
explicit history and the #6082 `binary.Size` measurement re-scoped to
the struct's size at that time.
`session/README.md` listed "a session installed by a pre-#4983 helper,
mid rolling upgrade" as a legitimate zero-carrying population while the
ABI note directly below it said a rolling deploy cannot cross this
change. The note is right, and the mechanism was verified at both sites:
`sessions`/`sessions_v6` are in `userspaceABICheckedPinnedMaps` (which
unions `userspaceShimSharedMapSpecs`) and `validateUserspaceShimLivePins`
hard-refuses a `ValueSize` mismatch against the live pin, so a new daemon
never reads an old helper's rows and the remediation starts from an empty
map. The population is unreachable, not merely rare; the entry is
replaced with that reasoning in `session/README.md`, `session/entry.rs`
and both copies in `pkg/dataplane/types.go`.
Finally, record the coverage boundary in the test module itself: these
tests bind what the poll body STAMPS onto the installed session. They
cannot assert that a TRANSIT session's identity reaches the
operator-visible conntrack map, because the transit install never calls
`publish_bpf_conntrack_entry` — it writes shim steering keys via
`publish_live_session_entry` plus the shared maps. Only the LocalMiss and
missing-neighbor-seed installs publish there. That gap predates this work
and is tracked separately; stating it here keeps the tests from being
read as proof of something they do not check.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
The #4983 docs said the stamped ingress identity is mirrored into the conntrack value "where the Go control plane reads them". That is true of the metadata and false of what reaches `show security flow session`, and the difference is the dominant population. `show`/`clear security flow session` enumerate the BPF conntrack map (`Manager.IterateSessions` over `m.maps["sessions"]`). The helper writes that map from exactly three sites, all in `afxdp/poll_descriptor`: the host-inbound LocalMiss install, the missing-neighbor-seed install, and the reverse-companion repair — and the third carries `IsReverse != 0`, which every show/clear call site skips before filtering. The ordinary TRANSIT forward install is not among them. It calls `publish_live_session_entry`, which writes the shim's steering table: a 36-byte key whose value is a single action byte, a different map from the 144-byte conntrack value. So a transit session has no conntrack row at all — not a zeroed identity, no row — and an interface filter cannot select it either way. That gap predates this work. It dates to `fab9230c5`, the commit that first added the conntrack mirror and wired only these three sites, and it is strictly larger than #4983: it is tracked as #6965, which needs research before implementation because publishing at the transit install adds a BPF syscall to the new-session cold path (the path #5287 exists because a full-table conntrack pass stalled the low-latency core) and forces three open decisions — whether the reverse companion gets a row, what happens at conntrack-map capacity, and whether SessionCount / Prometheus / GC semantics shift once the mirror is near-complete. Nothing here changes behaviour; it stops the documentation from promising more than the code delivers. `session/README.md` gains a "Which sessions this is OPERATOR-VISIBLE for" paragraph, and the same scope note lands on both `IngressIfindex` field docs in `pkg/dataplane/types.go` and on the field doc in `session/entry.rs`. The transit test's doc comment now records why it has no assertion for the mirror, since that absence otherwise reads as an oversight: the test stays green if you "omit the full conntrack publication" because there is no transit publication to omit. An assertion that the stamp reaches the operator-visible map would fail today and would be testing #6965's fix, not this one. The test binds the stamp; the mirror is #6965's to bind. The module header states the boundary once and points at #6965. Docs and comments only — no production or test logic changed. Gates: `cargo test --release` rc=0 with 4261 passing; `go build` and `go vet ./pkg/dataplane/...` rc=0; `go test -count=1 ./pkg/cli/... ./pkg/dataplane/...` rc=0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
# Conflicts: # _Log.md
Re-verification of the Codex leg at the current head — 3 of 8 blockers are already CLOSEDRecording this on the PR rather than leaving it in a review conversation, because the The Codex leg pinned CLOSED — the mutation no longer holds#3 — "the real-poll transit binder covers only an HA-owned path." Codex's mutation was That is #4 — "LocalDelivery is a separate, unguarded forward producer." Codex's mutation was #1 — "the ordinary transit producer never reaches the Go-visible conntrack map." STILL OPEN — five
A fixture-realism defect found against this PR's own fixture
|
Two of the five surviving Codex blockers on #4983. #2 — the doc claimed "an interface-filtered show/clear is exact on the console". It is not, for clear. `clearFilteredSessions` propagates to the HA peer unconditionally (pkg/cli/cli_clear.go:252, no interface-filter guard), the filter rides gRPC, and the peer resolves the interface from the ingress ZONE using `zone.Interfaces[0]` for every session in that zone (server_sessions.go:508-515, matched at :578-583 and :623-628 where `inIf` depends only on the zone, never on the session). On zone `[reth0.50, reth0.80]`, clearing `.50` therefore DELETES peer flows actually received on `.80` — a wrong-session deletion, not a display defect. The runtime hole is pre-existing and this PR does not widen it, which was worth establishing by measurement rather than argument: the PR's diff against master is empty for both pkg/grpcapi and pkg/api, and empty for pkg/cli/cli_clear.go. What it adds is `resolveIngressIfaces` (absent on master), which makes the local half exact and narrows the defect. What it also added was a claim overstating that as end-to-end exactness. The claim is now corrected at both sites: show is exact for rows this node owns, clear is not exact at all because it leaves this node. Filed as #6975 with the trace; the previous "tracked separately" named no issue. #6 — the ABI guards assert sizeof and binary.Size, so they cannot see a field REORDER. Swapping the Go tail to `IngressVlanID; pad; IngressIfindex` keeps 144/192 and keeps binary.Size equal to it, while C and Rust still write the ifindex at 136/184; a record carrying `{ifindex:11, vlan:50}` then decodes as `{ifindex:50, vlan:11}`. Both are plausible values, so the CLI filters confidently on the wrong interface rather than falling back to the zone approximation the way a zero would make it. Added TestBPFSessionValueIngressIdentityOffsets pinning all four offsets against bpf/headers/xpf_conntrack.h. Validation for #6 uses Codex's own mutation and carries its own negative control: under the reorder the two size guards stay GREEN (`ok`) — which is the point — while the new guard reds on all four offsets, `offsetof(bpfSessionValue.IngressIfindex) = 140, want 136` and siblings. go build 0, go vet 0 on pkg/dataplane and pkg/cli, package tests ok. Advances #4983.
Progress at
|
A separate Codex review leg pinned a head two folds old. Every blocking finding was re-verified at the current head before acting, and three of the eight were already closed by later rounds — re-running Codex's own mutations was the only way to establish that, since commit titles cannot. The fixture finding is against my own work and it is right. The ingress-identity snapshot gave reth0.50 redundancy group 2 while its SIBLING unit on the same parent, reth0.80, is group 1. That topology cannot be produced: the snapshot builder resolves one redundancy group per BASE interface and stamps that single value onto every unit of it, so two units of one parent always agree. The old justification cited reth1.0 as "the fixture's other LAN interface", but reth1.0 is a different base and does not constrain this unit at all. Changed to group 1, which is producible and leaves behaviour identical because ownership derives from the egress resolution, not from the ingress unit's group. The second added unit keeps group 2 and now records why that one IS producible — it is the only unit on its parent, so there is no sibling to conflict with. A fixture change owes a fresh mutation check, so the standalone guard was re-proven red against the corrected fixture rather than assumed. The claim corrections are done as one pass, because this class clusters and that has now held on two consecutive PRs. The README said the filter and the displayed interface name cannot disagree. True only for a stamped identity: on the fallback the filter considers every interface in the zone while the display uses the first, so a zero-identity row in a two-interface zone is selected by one name and prints the other. The reverse companion's zero was justified as the forward egress being unresolved at install. The installed decision already carries it. The real reason is that the forward egress predicts where the reply will arrive rather than observing where it did, and routing may be asymmetric. "VID 0 means untagged" is not synonymous — a priority-tagged frame carries VID 0 with the tag present, and the frame inspector's property tests cover exactly that shape. Reworded to "no VLAN id recorded". "The two policy-admitted install sites" overstates one of them: the host-inbound install also runs for a junos-host NoMatch flow, admitted by the zone's host-inbound set with no policy matching at all. Finally, types.go still described the on-map layout as 128 bytes and the over-size as exactly eight; it is 144 for v4 after this issue's growth, and the excess is whatever the sync-only trailing fields sum to. The same file called REST show/clear approximate, when the REST handler rejects a filtered clear with HTTP 400 rather than degrading it — there is no approximate REST clear to be wrong about. Swept for other instances of the stale size: this was the only survivor. Gates: cargo test --release rc=0 with 4261 passing; go test over pkg/cli and pkg/dataplane rc=0; rustfmt and gofmt clean on every touched file. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
Close the last three blocking findings on #4983's review: the two Go conversion directions and the CLI display fallback were deletable with the suite green, and this PR's own egress-FIB subtest could not fire. No production code changes — all three were binding and claim gaps. Each was reproduced with the reviewer's own mutation before anything was written. Conversion directions. Deleting all four IngressIfindex/IngressVlanID assignment pairs from toBPF/sessionValue (v4 and v6) left `go test ./pkg/dataplane/` ok, because the round-trip fixture left both identity fields at zero and 0 -> absent -> 0 compares equal. The fixture now carries non-zero, mutually distinct values, and two new tests assert on the intermediate value instead of the composition: TestSessionValueToBPFCarriesIngressIdentity and TestBPFSessionValueLiftsIngressIdentity. The latter builds a bpfSessionValue DIRECTLY rather than via toBPF, because the real case is a record the Rust helper wrote that Go's writer never touched. Both are needed beyond the fixture change. orig.toBPF().sessionValue() is a symmetric composition, so it detects any single-direction loss but is structurally blind to the same mistake made in both directions — a transposition composes to the identity while every on-map record is written and read at the wrong offset. Egress-FIB control. Confirmed at subtest scope and refuted at package scope, which changes the disposition. With resolveEgressIfaces' precise arm deleted, TestIngressIdentityDoesNotDisturbEgressMatching4983 and all three subtests still pass: the fixture's FibIfindex=12 resolved to ge-0/0/1, the untrust zone's only member, so the precise answer and the zone fallback were the same one-element list. The package does not stay green, though — the pre-existing TestMatchesV4_InterfaceFilter/matches_egress_via_FIB_lookup fails, because its FIB name is outside the egress zone. The arm's admitting half was always bound; its NARROWING half was not, and the subtest presented as a control it could not be. The untrust zone now binds two interfaces so the zone fallback is a strict superset of any FIB-precise answer, and TestEgressFIBIdentityDoesNotMatchEgressZoneSibling4983 asserts the sibling is excluded on both address families, with a fixture control: with FibIfindex=0 the same session must still reach the sibling through the fallback, or the exclusion would be explained by unreachability rather than by narrowing. The old subtest keeps its over-reach charter and now states it, naming both real bindings. That shape was checked for producibility before it was relied on: populateIfaceMaps appends every zone.Interfaces entry and #4792 exists because a zone binds more than one interface. Each member is a separate NIC with its own unit 0 and ifindex, so no redundancy-group property is involved and there is no base-interface constraint to violate. Display fallback. Wider than filed: replacing sessionIngressIf's zone fallback with `return ""` left the entire pkg/cli package green, not just the two added display tests. That fallback is the compat path for every session that legitimately carries no ingress identity — the reverse companion, a peer-synced session whose originating ifindex is node-local, and a session installed by a pre-#4983 helper mid rolling upgrade — so blanking it empties the In: If: column for every HA-synced flow. The new test covers the three ways the resolver can be asked: identity absent and identity present-but-unnameable both fall back to the zone's interface, and a zone binding no interfaces falls back to the zone name. The fixture builder is parameterised so one config drives the precise arm and every fallback arm, and the empty zone goes through the real parser and commit with an assertion that it survived with zero interfaces, so the last-resort arm is reachable by construction rather than by assumption. The README described only the precise arm of the column, which reads as though a blank column were acceptable for the populations that carry no identity. It now states the fallback symmetry the new tests enforce. Validation. Every mutation cell ran go vet first at rc 0, so each RED is an assertion failure rather than a build break. Conversion matrix (4 tests x 5 cells), with the transposition as the negative control: cell round trip offsets toBPF lift baseline PASS PASS PASS PASS delete both directions FAIL PASS FAIL FAIL transpose both directions PASS PASS FAIL FAIL delete v4 toBPF only FAIL PASS FAIL PASS delete v4 sessionValue only FAIL PASS PASS FAIL The transpose row is the point: the round trip cannot see it while both direction tests fail with "v4 toBPF().IngressIfindex = 50, want 11". Rows 4 and 5 show each direction test binds its own direction. The offsets guard passes in every row — it pins layout, not assignment — so the three tests are orthogonal rather than overlapping. Egress: deleting the precise arm fails the new sibling test on v4 and v6 while the over-reach subtest stays green on all three subtests. Display: with the whole fallback returning "", all three new subtests fail and both existing display tests pass. With only the final `return zoneName` replaced, only the zone-with-no-interfaces subtest fails — each arm is separately bound, not covered as a block. go build ./... rc 0; go vet ./... rc 0; go test ./pkg/dataplane/ ./pkg/cli/ -count=2 both ok; go test ./pkg/refactoraudit/ ok.
Bring the branch up to master (a3f333f) after the round-3 fold, as the last step of the round rather than the first — a rebase mid-round would have replayed each commit onto a moving base for no benefit. Only _Log.md conflicted, in four hunks; every other file merged automatically, including the userspace-dp sources both sides touched. The textually clean merge did not compile. Master's 3775af5 adds a new file that constructs SessionMetadata with a struct literal, and this PR adds two fields to that struct — an added field in one file and an added file in another, so no textual conflict exists and git is silent. The incompatibility is in the type system and only a build can see it, which is why the merge result was built rather than assumed: error[E0063]: missing fields `ingress_ifindex` and `ingress_vlan_id` in initializer of `entry::SessionMetadata` --> src/afxdp/session_glue/newflow_contention_tests.rs:68:5 Fixed in this merge commit rather than a follow-up so the merge commit itself builds and the branch stays bisectable. The value is 0/0, which is correct rather than a placeholder: the fixture builds a SyncedSessionEntry, and a peer-synced session carries no ingress identity by design — an ifindex is node-local, so the originating node's number names a different NIC on the importing node, and it is deliberately kept off the cluster wire. The comment records that, so the next reader does not "correct" it to a non-zero value. _Log.md was union-resolved and verified structurally rather than by keyword: the merged file is a superset of BOTH parents by line multiset (0 lines of 190ea01 and 0 lines of a3f333f missing). A keyword probe was tried first and matched nothing, which proves nothing about the resolve — a probe that does not fire is not evidence. An anchored sweep for git's exact marker syntax over the whole tree returns nothing. Gates on the merge result: go build ./... rc 0; go vet ./... rc 0; go test ./pkg/dataplane/ ./pkg/cli/ both ok; go test ./pkg/refactoraudit/ ok; cargo test --release in userspace-dp exit 0, 4279 passed / 0 failed plus 60, 8, 22, 31, 1 and 2 passed with 0 failed. userspace-xdp/ is untouched by this PR, so the shim .o and its manifest are unchanged and no make generate is owed.
Another lane pushed "session: drop an unproducible fixture dimension and correct six claims" onto the branch while this round was in flight. My base 2795885 is still an ancestor — no history was rewritten — so this integrates by merge rather than by force-push. Only _Log.md conflicted, in one hunk, union-resolved and verified structurally: the merged file is a superset of BOTH parents by line multiset (0 lines missing from either). The README auto-merged and carries both sides' new paragraphs. The other side's code changes are comment-only in pkg/dataplane/types.go and userspace-dp/src/session/entry.rs, plus the removal of the unproducible RG fixture dimension. That was still built rather than assumed, because the previous merge in this round was textually clean and did not compile. Nothing broke: the two rounds are disjoint — that one corrects claims and drops a fixture dimension, this one adds bindings. go build ./... rc 0; go vet ./... rc 0; go test ./pkg/dataplane/ ./pkg/cli/ both ok; go test ./pkg/refactoraudit/ ok; cargo test --release in userspace-dp exit 0, 4279 passed / 0 failed plus 60, 8, 22, 31, 1 and 2 passed with 0 failed.
Parent mutation proof at
|
| cell | mutation | result | tests that turned RED |
|---|---|---|---|
| A | none (control) | 5 passed, 0 failed | — |
| B | sever the transit forward stamp | 3 passed, 2 failed | poll_descriptor_transit_install_stamps_ingress_binding_4983, poll_descriptor_transit_install_stamps_ingress_binding_without_an_rg_4983 |
| C | sever the host-inbound stamp | 4 passed, 1 failed | poll_descriptor_local_miss_install_stamps_ingress_binding_4983 |
| D | sever the neighbor-seed stamp | 4 passed, 1 failed | poll_descriptor_missing_neighbor_seed_stamps_ingress_binding_4983 |
What makes this a proof rather than a count: each cell reddens exactly the test whose name describes the path it severed, and nothing else. No cell reddens the whole suite — an over-strong mutation that kills everything would prove nothing about which site is bound. The three production stamp sites are therefore independently bound, and the reverse-companion test stays green in every cell, which is correct: that path deliberately carries no identity.
Every mutation anchor was asserted unique before applying (anchor occurrences: 1); a non-unique anchor would have marked the cell UNKNOWN rather than green. The tree was verified clean before the run and after each restore.
A harness correction worth recording
An earlier run of this matrix reported cells B and C as BUILD-ERROR (not a RED). That was wrong, and the bug was in my classifier, not the PR: it keyed on ^error(\[|:), which matches cargo's test-failure summary error: test failed, to rerun pass '--bin xpf-userspace-dp'. Both cells had in fact gone properly RED. The classifier now distinguishes a genuine compile failure (error[E0nnn], could not compile) — which makes a cell UNKNOWN — from a test failure, which makes it RED. Folding UNKNOWN in with either GREEN or RED is how a mutation matrix comes to lie in whichever direction the reader is predisposed to.
Gate state
Independent hostile review in flight at this head. All eight blocking findings from the earlier Codex leg are closed. mergeable = MERGEABLE.
Independent AGY leg at
|
| leg | verdict |
|---|---|
| Parent mutation proof | PASSED — 4-cell discriminating matrix (see previous comment) |
| AGY | MERGE-READY (this comment) |
| Hostile Claude | in flight |
| Codex | queued behind another PR's leg |
Independent hostile leg at
|
The Go mirror pins where the #4983 ingress-identity pair sits; the Rust side that WRITES those bytes pinned only how big the struct is. Swapping the declaration order of ingress_ifindex and ingress_vlan_id in both BpfSessionValueV4 and BpfSessionValueV6 moves the u16 to 136/184 and the u32 to 140/188 while size_of stays 144/192, so bpf_conntrack_struct_sizes_match_c reports ok and the whole crate suite passes. Measured, not reasoned about. Nothing else covered it. The build_conntrack_value_stamps_ingress_ identity_* tests compare struct fields, so they are transposition-blind by construction, and the C header has no compiled consumer on this side. The cost of the gap is not a decode error, which is what makes it worth a guard: the helper writes the VLAN id where Go reads the ifindex, so a session on {parent ifindex 24, VLAN 80} is lifted as IngressIfindex=80, IngressVlanID=24. The name lookup misses and the row degrades to the zone approximation — and on a box that really does have an ifindex 80, the CLI names the wrong NIC. Both values are plausible, so nothing surfaces. Fixed with four const _: [(); N] = [(); offset_of!(..)] assertions beside the structs, the idiom UserspaceDpMeta already uses, so a transposition is a build failure rather than a test failure. Three sides now agree on one set of numbers: the C header writes them, Rust asserts them, Go asserts them. Claim corrections, each verified before being written rather than taken from the review: The reverse companion's rationale was retracted in one file and left standing in seven others, including the production install site and the shipped C ABI header. Confirmed the correction itself first: ForwardingResolution::egress_ifindex really is populated by the FIB, local-delivery and fabric resolvers, and the reverse install sits in the same scope as the forward decision — so "the forward egress is not resolved at install time" is false. The true reason is that the forward egress predicts where the reply will ARRIVE rather than recording where it did, and routing may be asymmetric, so there is nothing observed to stamp. One phrasing now used at all eight sites. "The stamp is on every forward session the helper installs" was false at three sites, and the same comment listed both counter-examples twenty lines below itself. Confirmed both carry is_reverse=false with ingress_ifindex=0: the host-outbound GRE path, and the HA peer import via is_reverse: req.is_reverse. Reworded to "every forward session installed from a RECEIVED FRAME", which makes them instances of the rule rather than exceptions — neither has an observed local ingress to copy. The zero-population enumeration omitted fabric ingress, the one path where the zone and the ifindex name DIFFERENT interfaces: the frame carries a zone-encoded override taking precedence over the ifindex->zone map, so the zone is the originating chassis's while the stamp is the local fabric NIC. Verified inert — the fabric member is declared only under fabric-options member-interfaces with no unit, and the name map keys on {parent ifindex, unit VLAN}, so it has no entry and the lookup falls back to the zone. Documented with what changes if that member ever gains a unit, and that the path is unreachable from the shipped topology rather than merely uncovered, which is why no test or smoke sees it. Smaller corrections, each measured: the shim steering key is 40 bytes, not 36 (compiled the repr(C) shape standalone: size=40 align=2). The "4-byte" and "2-byte" tail-pad figures describe the same layout from either end — the append grew the struct 136->140->144, and the u16 lands inside that pad at 140 leaving 2 unused — now stated once so a reader diffing the header against bpf_map_tests.rs does not read one as a bug. The peer-clear call is cli_clear.go:251, cited as :252 twice. And the ABI flag-day remediation is `xpfd cleanup` or a reboot, not "a full dataplane reload": a bpffs pin outlives the process, dataplane.Cleanup() is reachable only from the cleanup subcommand, and a restart leaves the old-size pin for the pre-flight to refuse again. session_sync.rs, the only zero site without a rationale, now has one. Validation. B1 is a two-cell proof and its RED is a build failure at the assertion, not a test failure. GREEN: assertions in place, layout untouched, cargo build --release exit 0 with zero errors — which is also what shows the asserted numbers are the real ones. RED: same tree, the two declarations swapped in both structs, exit 101, and the only errors are the four assertions: expected an array with a size of 136, found one with a size of 140 expected an array with a size of 140, found one with a size of 136 expected an array with a size of 184, found one with a size of 188 expected an array with a size of 188, found one with a size of 184 error: could not compile `xpf-userspace-dp` due to 4 previous errors One correction to the finding as filed: the transposed tree did not come back fully green here. slowpath::tests::enqueue_refuses_frame_above_live_ mtu failed once with "slow-path worker is not running". That is a worker-startup flake under parallel load, not a layout consequence — on the same transposed tree it passes 6/6 run alone, each run verified to have actually executed rather than filtered out. Recorded because "the whole suite stayed green" is the evidence the finding rests on. go build ./... rc 0; go vet ./... rc 0; go test ./pkg/dataplane/ ./pkg/cli/ both ok; go test ./pkg/refactoraudit/ ok; cargo test --release exit 0 with 4279 passed / 0 failed plus 60, 8, 22, 31, 1 and 2 passed. userspace-xdp/ is untouched, so the shim .o and manifest are unchanged and no make generate is owed.
Correction to my own B1 write-upI stated that under the transposed-struct probe "the entire Rust suite stays green (4280 + 60 + 8 + 22 + 31 + 1 + 2 passed, 0 failed)". The measured figure on re-run is 4278 passed / 1 failed: The finding is unaffected — the point of B1 was that no test observes the transposition, and a flake in an unrelated slow-path worker is not an observation of it. But the claim as I wrote it was stronger than the measurement, and "the whole suite stayed green" was the sentence B1's severity rested on, so it gets corrected rather than left. Worth recording how it was caught, because the first attempt to verify the flake was itself void: the probe printed B1 itself is closed at |
On a live-pin ABI refusal we printed "Do a FULL dataplane reload (stop xpfd so the old pin is released, then start it to load the new shim)". That instruction does not work. Releasing a pin means unlinking it: Cleanup() in loader.go does os.RemoveAll(bpfPinPath), and it is reachable only from the `xpfd cleanup` subcommand. A bpffs pin outlives the process that created it, so an operator who follows our message stops xpfd, starts it, and hits the identical refusal — mid-upgrade, with the dataplane down. Round 4 corrected this claim in four comments. This is the instance an operator actually reads, so leaving it while fixing the prose around it would have left the authoritative-looking text wrong. pkg/dataplane/ README.md carried the same instruction and is the document the message cites, so it is corrected too, with the reason recorded so it does not get re-simplified back to "a reload". The message now names the mechanism and says plainly that a restart is not sufficient. It is kept short deliberately: it is read mid-incident. Both assertions guarding it were defending the text rather than the behaviour, which is why the wrong instruction survived. The positive test required the phrases "FULL dataplane reload", "stale pin" and "released" — all three satisfied by the broken instruction. A phrase-presence assertion cannot distinguish a correct instruction from an incorrect one. It now asserts the two properties that make the message useful: that it names `xpfd cleanup`, and that it states a restart does not release the pin. The reasoning is in the test so the next person does not restore the old phrase to satisfy a string match. The negative case asserted the SSOT-drift path does not contain "FULL dataplane reload". That phrase no longer exists anywhere, so the guard had silently become a no-op that could never fire again — rewording one message retired the guard on the other. It now pins the CONSTANT, which survives any rewording of either, plus a second assertion that the drift path must not tell the operator to unpin: that is destructive, and wrong for a drift `make generate-userspace-xdp` fixes in place. Validation: two cells, each an assertion RED with go vet rc 0 first, and the partition is exclusive — each cell reds exactly one test while the other holds as its control. cell SSOT-drift live-pin baseline PASS PASS restore the OLD wording verbatim PASS FAIL mis-wire stale-pin into the SSOT-drift site FAIL PASS The live-pin failure names what is missing — "remediation must name `xpfd cleanup` — the only path that unpins bpffs state and therefore the only thing that clears this refusal" — on both subtests. One cell was discarded rather than scored: the first mis-wire swapped all ten `userspaceShimGenerateRemediation` use sites and produced go vet rc 1, a build break rather than an assertion RED. Re-run against exactly the ValueSize drift arm, which is the edge of the claim, it reds cleanly. go build ./... rc 0; go vet ./... rc 0; go test ./pkg/dataplane/ ./pkg/cli/ both ok; go test ./pkg/refactoraudit/ ok. No Rust source and no shim artefact touched.
Hostile re-gate at
|
| cell | mutation | result |
|---|---|---|
| A | none (control) | GREEN |
| B | v4 136 → 137 |
COMPILE-ERROR E0308 |
| C | v6 188 → 187 |
COMPILE-ERROR E0308 |
The v6 pair is bound as tightly as the v4 pair — the off-by-one hiding place at 184/188 is closed.
2. The rewritten assertions can all fire — 4-cell matrix, no vacuity
The old negative (must NOT contain "FULL dataplane reload") was genuinely vacuous once the phrase existed nowhere. The replacement compares against userspaceShimStalePinRemediation — the shipped constant, so rewording either message moves both sides together. That is the right fix, and it is not the only thing holding:
| cell | mutation | result |
|---|---|---|
| A | none | GREEN |
| B | strip xpfd cleanup from the remediation |
RED — TestLivePinABIMismatchUsesStalePinRemediation |
| C | reword both restart-warning phrases | RED — same test |
| D | make the SSOT-drift message contain the stale-pin constant | RED — TestSSOTDriftKeepsGenerateRemediation |
No new assertion references a literal owned by a different artifact in a way that could silently retire it. The one residual coupling is benign and worth naming: the drift-side negative on "xpfd cleanup" is scoped to that literal, so renaming the subcommand would narrow it — but the same rename fails cell B loudly first, so it cannot go quiet unobserved.
3. The corrected operator message is true
Cleanup()(loader.go:1245) doesos.RemoveAll(bpfPinPath)— confirmed.- Its only non-test caller is
cmd/xpfd/main.go:213, inside thecleanupsubcommand branch — "reachable only fromxpfd cleanup" is exact. - A bpffs pin outliving the process is a kernel property; the old "stop xpfd so the old pin is released" was wrong, and wrong in the worst place — mid-upgrade with the dataplane down.
NON-BLOCKING 1 — the remediation names a destructive command without its blast radius
xpfd cleanup does not only unpin the one stale map. main.go:205-208 records that it "GCs all pinned dataplane state and clears the FRR managed routes", and this PR's own new test comment calls it "destroys all pinned dataplane state, which is the wrong and destructive answer". The operator-facing string says only "Run xpfd cleanup (or reboot) to unpin". The PR knows the blast radius well enough to guard the other message against recommending it; the text an operator actually follows should say what else it takes.
NON-BLOCKING 2 — "three sides, one set of numbers" overstates the machine-checking to three
bpf_map/mod.rs says the offsets "are the ones C writes (bpf/headers/xpf_conntrack.h) ... three sides, one set of numbers." Two of those three are machine-checked (the Rust offset_of! consts, the Go mirror). The C side is pinned by a comment only — and r4's own transposition argument applies to it unchanged: swapping the two C fields keeps sizeof at 144/192, so the size test cannot see it either.
Severity is low because r4 is right that the C struct has no compiled consumer (I checked: no .c includes it; the references are Rust comments and sibling headers). But that also makes "the ones C writes" stale framing post-#1476 — nothing C-side writes anything now; the header is a layout SSOT that Go and Rust mirror. Three lines apart the same comment block says both "has no compiled consumer" and "the ones C writes".
4/5. Disturbance and claim audit
The r4 doc edits retract a claim rather than add one — the earlier "the forward flow's egress interface is not resolved at install time" is marked FALSE and replaced with the real reason (it is resolved, it is simply a prediction rather than an observation). That is the correct direction for this PR's recurring failure mode. The deliberate zeros and their rationales are extended, not weakened, and the fabric-ingress case is newly enumerated with its inertness argued from buildSessionEgressIfaces keying. I did not re-derive the ten construction sites or the Go filter fallback — the 6eca116db gate covered them and neither commit touches those call sites.
Neither non-blocking item changes runtime behaviour or leaves a guard unable to fire.
MERGE-NEEDS-MINOR
Codex re-verify at
|
Round 5 replaced "a reload releases the pin" — which nothing does — with
the opposite categorical claim: that restarting never releases it and only
`xpfd cleanup` does. That is also false.
Manager.Teardown is m.Close() followed by return Cleanup(), and
daemon_run_shutdown.go calls d.dp.Teardown() on every non-hitless shutdown
("HA shutdown: tearing down BPF state"); only the hitless arm takes
d.dp.Close() and preserves the pins deliberately. So dataplane.Cleanup()
has two production callers rather than one, and whether a restart clears a
stale pin depends on how xpfd last stopped.
The consequence is operator-facing and not academic. The round-5 message
sent the operator to `xpfd cleanup`, which additionally removes every other
pinned dataplane map and clears the FRR managed routes, in a mode where a
plain restart would have sufficed — mid-upgrade, with the dataplane down.
It pointed at a destructive action and understated what it destroys.
The message now states the mode dependency and leads with the targeted
recovery — unlink the one named pin, per
docs/operations/userspace-shim-pin-recovery.md, which already documented
exactly that and was never referenced — before naming `xpfd cleanup` and
what else it takes. types.go:138 and :386 claimed Cleanup() is reachable
only from the subcommand; both are corrected, as is the same paragraph in
pkg/dataplane/README.md. The runbook gains the cleanup-scope note and the
mode dependency.
Why nothing caught it: the guard asserted substrings. A phrase-presence
assertion is satisfied identically by a correct and an incorrect
instruction — it defends the text, not the behaviour. The round-4 wrong
message passed that way, and round 5 rewrote the block to require
"restarting xpfd does NOT release", locking in the new wrong claim.
The false claim is a claim about the call graph, so the binder is a
call-graph check rather than a proxy for one. cleanup_reachability_6928_test.go
parses the non-test tree and pins Cleanup()'s production callers to exactly
{cmd/xpfd/main.go:main, loader.go:Teardown}. It first asserts the walk
found any caller at all, since a broken walk would pass vacuously — the
same failure mode being closed one level up — and separately asserts at
least two callers exist, so a collapse back to the single CLI caller reds
and forces the wording to be revisited. A companion pins that both shutdown
arms still exist, so Teardown cannot quietly become dead code while the
mode-dependent wording survives. The substring block is replaced by a
negative on the two claims the code disproves.
Both guards verified red-first. Severing Teardown's call to Cleanup() reds
the caller-set test against its two-entry expectation; restoring the
disproven phrase reds TestLivePinABIMismatchUsesStalePinRemediation at
"remediation restored the categorical claim". Production was restored and
verified clean after each. The two SSOT-drift negatives were checked for
the vacuity trap: one keys on the constant and survives any rewording, the
other on the action name, which the new message still contains.
Validation: go build ./... rc=0; go test ./pkg/dataplane/... ./pkg/daemon/...
./pkg/cli/... rc=0 across all seven packages; gofmt clean on every touched
file. pkg/dataplane/constants.go and bpf_session_value_test.go are
gofmt-dirty at this head already and are deliberately left alone.
Advances #4983.
Blocker folded at
|
| mutation | result |
|---|---|
sever Teardown → Cleanup |
RED — got: cmd/xpfd/main.go:main against the 2-entry want |
restore "restarting xpfd does NOT release" into the message |
RED — remediation restored the categorical claim |
Production restored and verified clean after each. Two preconditions: the walk must find some caller (a broken walk would otherwise pass vacuously — the same failure mode one level up), and at least two callers must exist, so a future collapse back to the single CLI caller reds and forces the wording to be revisited rather than silently becoming wrong in the other direction a third time.
The vacuity trap was checked, not assumed
Both SSOT-drift negatives were examined. :173 keys on the constant, so it survives any rewording — already correct. :178 keys on "xpfd cleanup", which is an action name rather than borrowed prose, and the new message still contains it, so it can still fire. Neither went vacuous.
Gates
go build ./... rc=0 · go test ./pkg/dataplane/... ./pkg/daemon/... ./pkg/cli/... rc=0, all 7 packages ok · gofmt -l clean on all four touched files.
pkg/dataplane/constants.go and bpf_session_value_test.go are gofmt-dirty at the PR head already and confirmed unmodified in this diff — flagged rather than reformatted, since fixing them would widen the diff into unrelated files.
Four claim corrections deliberately NOT folded
session/README.md:1011/:1021, types.go:150 + its v6 copy, three "pre-#4983 helper" sites, three "VLAN 0 = untagged" sites. I scoped these as "fold if cheap" and they are not — four separate claims across ~8 sites, each needing its own verification against code. Leaving them listed beats half-verifying four claims and shipping a fifth wrong sentence in a PR whose entire subject is wrong sentences. They get their own pass.
Head is a76d4143b, MERGEABLE/CLEAN. The hostile and Codex legs are owed at this head; the prior verdicts predate it.
The ingress VLAN field is documented at six sites as "0 = untagged". It stores a bare VID, so 0 covers both an untagged frame and an 802.1p priority-tagged one — a real 802.1Q tag carrying VID 0 with PCP/DEI set. The session row cannot tell those apart, and a consumer following the comment would conclude such a session arrived untagged. The case is neither hypothetical nor unhandled elsewhere: userspace-dp/src/afxdp/README.md documents that the TX side emits a tag on tag PRESENCE via TxVlanTag (#2149) rather than on vlan_id > 0, precisely so a priority-tagged VLAN-0 frame keeps its priority instead of collapsing to untagged. Only the session row loses the distinction, which is what the comments needed to say. Corrected at all six sites: pkg/dataplane/types.go and pkg/dataplane/bpf_session_value.go (v4 and v6 each), bpf/headers/xpf_conntrack.h, and userspace-dp/src/afxdp/bpf_map/mod.rs (v4 and v6). Each now states that 0 means the VID was zero, that untagged and priority-tagged are both included, and that the TX side does draw the distinction. Doc comments only; no behaviour change. The Rust edit sits immediately above the compile-time offset_of! ABI assertions, so cargo check --all-targets was run rather than assumed. Validation: go build ./... rc=0; go test ./pkg/dataplane/ rc=0; cargo check --all-targets rc=0, zero errors; gofmt clean on both Go files; a residual sweep for "0 = untagged" across the affected trees returns nothing. Advances #4983.
Claim pass: 1 of 4 at
|
| what I relayed | what is actually true |
|---|---|
types.go:169 is a VLAN claim |
not a VLAN claim at all — it is the REST-clear-rejection note |
counterexample at userspace-dp/src/inspect.rs:334 |
that file does not exist. It is userspace-dp/src/afxdp/frame/inspect.rs, and the counterexample is not there either |
| "three sites" | six — types.go ×2, bpf_session_value.go ×2, xpf_conntrack.h ×1, plus afxdp/bpf_map/mod.rs ×2 which my list omitted entirely |
The wrong-file cite is the instructive one: src/inspect.rs versus src/afxdp/frame/inspect.rs reads correct, so nobody checks it. Same family as the two files named authz.go that have now cost this campaign time twice.
The practical damage: I scoped the task as "verify four supplied traces" when it was really "perform four independent derivations" — about double. Scoping a derivation as a verification is precisely what makes a lane run out mid-way, which is why the stop at 1 of 4 was the right call and not a shortfall.
The item that landed — the claim is false, not merely imprecise
IngressVlanID stores a bare VID, so 0 covers an untagged frame and an 802.1p priority-tagged one: a real 802.1Q tag carrying VID 0 with PCP/DEI set.
The counterexample is not a hypothetical, and it is in this repo's own TX path. userspace-dp/src/afxdp/README.md:603-607: the TX side deliberately emits a tag on presence via TxVlanTag (#2149) rather than on vlan_id > 0, specifically so a priority-tagged VLAN-0 frame does not collapse to untagged. So the case is real, known, and already handled elsewhere in the same crate — only the session row loses the distinction, and a consumer following "0 = untagged" concludes such a session arrived untagged.
All six sites corrected to say 0 means the VID was zero, that both cases are included, and that the TX side does draw the distinction. Residual sweep for 0 = untagged across those trees returns nothing.
Doc comments only — but the Rust edit sits immediately above the compile-time offset_of! ABI assertions, so cargo check --all-targets was run rather than assuming a comment is safe there: rc=0. go build ./... rc=0, go test ./pkg/dataplane/ rc=0, gofmt clean.
Method for the next pass, which is the durable part
- Treat the cites as pointers to a topic, not to a line. Resolve every one against the actual head before acting on it.
- Never trust a relayed count. "Three sites" was a grep, not a survey. Sweep tree-wide first — here 3 became 6, and the two additions were in the Rust mirror, exactly where a Go-side sweep stops.
- Budget one full derivation per item.
The stopping note is in _Log.md beside the work, so the next pass inherits the correction rather than my estimate.
Remaining and untouched: session/README.md:1011 versus :1021; types.go:150 "locally-owned rows are exact"; the three "pre-#4983 helper" sites. All three cite lists should be assumed wrong until resolved.
Head f2f75d19d, MERGEABLE/CLEAN.
The comment claimed an interface-filtered show is exact for rows this node
owns. It is exact only when the row's {ifindex, vlan} still names a
currently-configured unit.
resolveIngressIfaces returns a single name only on a hit in
ifaceNamesByKey, otherwise it falls back to every interface bound to the
ingress zone. That map is rebuilt per query from the current config and the
current kernel ifindex, while the row's ifindex was recorded at install, so
three local shapes are not exact:
A non-zero ifindex the config cannot name — a unit deleted since install, a
tunnel or fabric ingress with no config unit — misses and falls back to the
zone. The resolver's own doc comment already said so.
A recycled ifindex can hit a key the kernel has since reassigned to a
different interface. That is worse than approximate: it renders one
confident wrong name rather than a zone list.
The map keys a unit under vlan-id, else unit number, while the row carries
the VID observed on the wire. Those agree when the unit number equals the
vlan id, or when both are zero. A unit whose number is populated and whose
traffic is untagged keys zero on the wire and number in the map, and
misses. sessionDisplayVLANID's own doc notes the config may populate only
one of the two.
Worth recording what is NOT wrong, since it was the first hypothesis:
units do not collapse onto {ifindex, 0}. That same number-fallback keeps
them distinct, and only a genuine key collision reaches the
first-writer-wins insert. Stating it here so the refuted shape is not
re-derived later.
Both sites narrowed, v4 and v6. Comments only. The recycled-ifindex shape
is a real wrinkle in an operator-facing filter rather than a documentation
defect, but fixing it changes behaviour and belongs in its own change.
Validation: go build ./... rc=0; go test ./pkg/dataplane/ ./pkg/cli/ rc=0;
gofmt clean. Advances #4983.
Exactness claim narrowed at
|
Both were doc/comment claims about #4983's ingress identity. Each was re-derived from the head rather than verified against its cite, and each turned out to be wrong in a different way than reported. ITEM 1 — session/README.md contradicted itself about whether the session FILTER and the "In: ... If:" DISPLAY agree on their fallback. One passage said they diverge by construction; the next said the divergence "covers the FALLBACK arms too ... the same degradation". The first is right, and the types settle it without reading the prose: the filter's zoneIfaces is map[uint16][]string built with append(..., zone.Interfaces...) and its fallback returns EVERY bound interface, while the display's is map[uint16]string built with zone.Interfaces[0] and returns the FIRST, else the zone name. cli_show_flow.go states the split itself where it builds them — the display maps "stay display-only" precisely because an interface-filtered show must see every interface of a zone (#4792). The false paragraph is replaced with the derivation, plus two consequences it hid: a zone binding NO interface gives the filter an empty slice, so no interface filter selects that row at all while the column prints the zone name; and the display chain is pinned by a test while nothing pins agreement, because there is none. The correct passage is kept verbatim. ITEM 2 — three sites nominally still described a "session installed by a pre-#4983 helper" as a population that legitimately carries ingress identity 0. There is no such population: sessions/sessions_v6 are both in userspaceShimSharedMapSpecs, userspaceABICheckedPinnedMaps unions that into the ABI-checked set, userspaceMapABIDiff compares ValueSize, and validateUserspaceShimLivePins hard-refuses a diff — so a new daemon either reads a freshly created empty map or does not load at all. Two corrections beyond the brief. First, the reference account in session/README.md is right in substance but named the remediation a "full dataplane reload", which loader_userspace_shim.go records as already corrected once: a reload never releases a pin. entry.rs carried the opposite superseded form ("NOT a restart"), which the same comment records as equally false, since a non-hitless HA shutdown calls Manager.Teardown and removes the pin path. pkg/dataplane/types.go already had the correct mode-dependent account, so that is the reconciliation target. Second, the sweep found five sites asserting the stale population rather than three, and both extras were the v4/v6 twins in the Rust mirror (afxdp/bpf_map/mod.rs) — where a Go-side sweep structurally cannot reach. Four test comments restated it as well. All nine now carry the same verified account, and the replacement third population is the real one: the host-outbound GRE path, which reads self-originated traffic off the TUN device and has no ingress binding to record. Every surviving pre-#4983 mention is now a negation or a corrective note, except two that describe pre-#4983 BEHAVIOUR rather than a population and are deliberately untouched. Doc and comment only; no behavioural change was needed or made. Validation: go build ./... and go vet on pkg/cli + pkg/dataplane clean; gofmt clean on every touched Go file; go test ./pkg/cli/... ./pkg/dataplane/... passes (5 packages); cargo test --release passes (7 suites, 4406 tests) — run because Rust doc comments and a shared C header comment changed. Advances #4983.
…ions # Conflicts: # _Log.md
Both remaining claim items delivered at
|
Codex re-verify at
|
Round 6 of the #4983 review is claim correction only: every changed line in every file here is a comment, and no test assertion moves. The recurring failure is one this PR has now hit three times — a passage is corrected, and an adjacent passage keeps restating the superseded form. types.go narrowed the exactness claim on locally-owned rows and then reasserted it 27 lines later ("exact where this node is the authority"). Its own recycled-ifindex bullet is the counterexample: an interface removed, its kernel index reused by a configured sibling, and both the filter and the If: column confidently report the sibling for a row this node owns outright. Local authority buys exactness only for the nameable subset; that is what it now says. A reported third unresolved shape turned out to be the third bullet already documented, reached by a wrong mechanism. Verified firsthand that a non-VLAN unit's row carries the PARENT ifindex, not the child: meta.ingress_ifindex is the physical AF_XDP bind, and forwarding_build/interfaces.rs keys ingress_logical_ifindex as (bind_ifindex, vlan) -> logical, so the child index is what the map resolves TO. Only the VLAN half diverges. The bullet now names the concrete config and says both sides key the parent, so the next reader does not go hunting for an ifindex that is not in the row. The interface-filter description was false in both the code comment and its test preamble: matchesV4/V6 OR the ingress and egress arms, so a session that arrived on A and egresses B matches a filter for B. The OR is deliberate, so the sentence is fixed rather than the behaviour. Eight sites, not the seven reported, still carried superseded accounts, and the sweep had to cross Go, Rust and C to see them. session/README.md still said cleanup or reboot is required and a restart never suffices, 35 lines after the corrected mode-dependent account in the same file. The C header still equated VID 0 with untagged while its v6 twin and both language mirrors had been corrected. Two sites called the frame-driven producers "policy-admitted" when a host-bound flow can be admitted by the zone host-inbound set alone. The three-writer claim about the conntrack map omitted the Go-side peer-sync writer. The selectability claim contradicted the zero-interface-zone case stated 20 lines below it. And four sites still labelled the targeted single-pin instruction "full-reload" remediation, which names an action nothing in the product performs. Two of the artefacts corrected are this PR's own. The cleanup-caller test claimed a zero-caller walk would otherwise pass vacuously; proven false by disabling that arm and forcing the walk empty, whereupon the exact comparison still reds with an empty got against a two-entry want. The arm is kept as a diagnostic and the claim rewritten. Two _Log.md entries overstated the substring companion the same way. The honest limit is now stated rather than implied: the caller-set check pins who calls Cleanup(), not which shutdown mode reaches it and not that anything reaches it. A call-graph instrument cannot express reachability, and the substring companion proves the call expressions are written in the shutdown file, not which branch holds them. Validated: gofmt clean on every file touched, go build ./... and go vet on both packages exit 0, go test -count=1 ./pkg/dataplane/... ./pkg/cli/... all ok, cargo check --all-targets exit 0, and the five tests_session_ingress_identity binding tests pass. Advances #4983.
Fold r6 at
|
Codex re-verify at
|
…gress-iface # Conflicts: # _Log.md
This PR exists because sentences in the ingress-identity area asserted more than the code delivered. Round 6 was a claim-correction round that added zero executable assertions, and it left three prose claims that are materially wrong — one of them contradicted by the same round's own derivation. Each was falsified by constructing the case first. "A zone binding NO interface means no interface filter selects that row at all" is refuted with no edit required. matchesV4/matchesV6 reject only when BOTH arms miss, so an empty ingress slice removes one route in, not the row: the session is still selected by the interface it egresses on. TestInterfaceFilterReachesRowViaEgressArm6928 builds exactly the described row — ingress zone binding nothing, ingress identity 0, nameable egress — and shows the filter selecting it, with a negative control that an unrelated interface selects nothing. The README and the resolveIngressIfaces comment now name the real unreachability condition: both arms empty. "The filter and the displayed interface name cannot disagree" for a non-zero nameable identity is false with no fallback in the derivation. A session arriving on lo.50 and egressing lo.80, both stamped and both nameable, is selected by `interface lo.80` through the egress arm and prints If: lo.50. That is the account cli_show_flow.go:307-315 already derives at the print site; the README sentence contradicted it. The narrower property that does hold — an ingress-arm match names the interface typed, because arm and column read one stamped pair through one map — is now pinned by both sub-tests of TestInterfaceFilterEgressArmMakesColumnNameAnotherInterface6928. "The caller test stops the categorical wording coming back green" is false by substitution: rewriting the remediation to "A plain restart ALWAYS releases this pin, on every shutdown path" — categorically false, and false in the opposite direction from the two banned literals — left the whole pkg/dataplane suite green. A guard over two strings constrains vocabulary, not the claim, and no test can decide whether an English sentence describes a code fact. A third banned literal would be the same mistake a third time, so the escape is stated rather than narrowed, and the two facts underneath are bound instead. Mode placement is now bound behaviourally. Both escapes the prior round admitted were reproduced rather than argued: inverting `if hitless`, and replacing the call with a commented-out `// d.dp.Teardown()` that still builds, each left the scoped suite green. d.dp is a RuntimeDataPlane interface, so no production seam was needed — the new pkg/daemon/shutdown_dataplane_mode_6928_test.go drives the real runShutdownSequence against a substituted dataplane and asserts both arms. Both mutations are RED against it, so the substring companion was deleted rather than re-labelled: it proved a strict subset. The "exact direct-caller set" was wrong in both directions, measured: an aliased `import dp` plus dp.Cleanup() added a real production caller the walk silently missed, and an unrelated bare Cleanup() elsewhere was falsely counted. The walk now resolves each file's import binding for the dataplane path — alias, dot-import, blank import, plain — and counts a bare Cleanup() only inside pkg/dataplane or a dot-importing file. Re-measured: the aliased caller is reported, the unrelated one ignored, and a dot-import case is caught too. Local shadowing and indirect calls remain out of reach without go/types and are stated as such. Validation: full `go test ./...` rc 0 (62 ok, 0 FAIL) and full `cargo test --release` rc 0 (4282 passed). An earlier cargo run hit a thread-starvation flake in afxdp::wg (passes 5/5 in isolation; this PR touches no file under afxdp/wg). gofmt clean on all five touched Go files; go build and go vet rc 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
Round 7 at
|
| guard | revert | observed |
|---|---|---|
| egress-arm reachability | drop && !f.ifaceMatchesAny(outIfs) (v4+v6) |
RED — "selected 0 of the 2 sessions" |
| column-name disagreement (both subtests) | restore pre-#4983 zone-derived If: |
RED — prints ge-0/0/8.0, want lo.50 |
| shutdown mode | if hitless -> if !hitless |
RED both arms |
| shutdown mode | // d.dp.Teardown() |
RED (builds clean) |
| import-aware walk | add aliased dp.Cleanup() |
RED, correctly naming the caller |
Full go test ./... rc=0 (62 ok), cargo test --release rc=0 (4282). The
install_session_serializes_with_reconcile_removal flake appeared once on a loaded
box, passes 5/5 in isolation, and this PR touches zero files under afxdp/wg/ —
that is #6989.
The merge finding, which corrects MY guidance
It rejected git merge-file --union, because the union FUSED a shared boundary
line — collapsing one blank line plus one - **Timestamp**: 2026-08-12, which
would have stripped master's "#5561 round 21b" entry of its timestamp bullet.
And it would have passed a zero-deletions check: 80281 lines against an
additive 80283. Two lines short, no deletions reported.
That is precisely the fusion hazard I have been warning about, caught in the wild —
and it means the tool I recommended to two other lanes can produce it. It resolved
by deterministic replay of both diffs onto the merge base instead, and predicted
the totals before resolving: 80283 lines (77129+1191+1963), 1594 ##
headings (1573+7+14), 487 headings preceded by a non-blank line (476+4+7), zero
deletions both directions, each parent differing from the result by exactly the
other's insertions.
The prediction-before-resolving discipline is what caught it. A zero-deletions
check alone would not have.
1 participant
Closes #4983.
The defect
A session carried only its ingress zone, so
show security flow session interface <name>and the matchingclearcould only ask "is<name>bound to the session's ingress zone?". A session on interface X therefore matched a filter for every sibling interface Y of that zone — a wrong-answer-shapedshow, and aclearthat destroys state the operator did not target. #4792 widened the CLI's zone map from the first bound interface to all of them, which is as precise as a zone-derived answer can be. This adds the real datum.Verified still present on
origin/masterd77583f before starting:SessionMetadata(userspace-dp/src/session/entry.rs:24) carried zone but no ifindex; Cstruct session_valuehad onlyfib_ifindex(the egress, andpublish_conntrackwrote it 0);pkg/cli/session_filter.go:266,311still readf.zoneIfaces[val.IngressZone].The mechanism
SessionMetadatagainsingress_ifindex+ingress_vlan_id, stamped once at install inpoll_descriptor(transit forward + host-inbound LocalDelivery) from the frame'sUserspaceDpMeta— the binding the packet actually arrived on, plus its 802.1Q tag — and never re-derived from the zone.publish_conntrackmirrors both into the conntrack value;pkg/cli's newresolveIngressIfacesresolves the pair through the same{parent ifindex, VLAN}map the egress side already uses, so one map defines one interface identity for both directions.The VLAN half is load-bearing, not decoration:
reth0.50andreth0.80share the loss cluster's physical WAN NIC, so without it they alias onto the parent and the cross-interface match returns for exactly the topology this project tests on.cli_clear.goneeded no edit — it sharesmatchesV4/V6and already callspopulateIfaceMaps.Absent-field behaviour (deliberate, documented at the call site)
0means "no ingress identity carried" and is never a valid ifindex. Three populations carry it, and all three fall back to the zone approximation — never "matches nothing" (which would silently hide them fromshow/clear), never "matches everything":ge-0-0-1and node 1'sge-7-0-1are different numbers for the same logical RETH member, so shipping the peer's value would render a confidently wrong interface name locally, strictly worse than approximating. The identity is therefore not carried on the cluster wire, and no serde/JSON control-protocol field is added —protocol_wire_v1.jsonis unchanged (confirmed:git statusclean underuserspace-dp/tests/fixtures/), so there is no virtio_net AF_XDP zero-copy delivers 0 packets to the XSK — no forwarding on plain virtio (copy-mode fallback?) #1961-class one-sided-wire hazard to grep for;A non-zero ifindex the running config cannot name (interface deleted since install, tunnel/fabric ingress with no config unit) falls back the same way: an unnameable identity is not evidence the session is uninteresting.
ABI change — read this before merging
The two fields join the shared C conntrack struct, not the sync-only trailing fields:
session_value136 → 144,session_value_v6184 → 192. The u32 lands on the existing 8-byte boundary and the u16 inside the tail pad it forces, so the pair costs 8 bytes, not 16. All three mirrors move in lockstep (C header, RustBpfSessionValueV4/V6, GobpfSessionValue{,V6}whose tail pad is declared for the #6082 zero-copy marshal path) and both size asserts are bumped.sessions/sessions_v6are pinned maps. Exactly as for the #5460 flags widen (34427efe1, which bumped this same ABI 128 → 136 and shipped), a rolling deploy cannot cross this:validateUserspaceShimLivePins(#5307) refuses while the old daemon is still forwarding, and the documented remediation is a full dataplane reload with brief downtime. That is the operator-visible cost of the fix; it is precedented, but it is a real cost and the reviewer should weigh it.Fail-on-revert
resolveIngressIfacescall sites →f.zoneIfaces[val.IngressZone]go test ./pkg/cli -run 4983exit 1, every failure an assertion message (go vet ./pkg/cliexit 0 — not a build break). Restored: exit 0.publish_conntrack→ingress_ifindex: 0cargo test --release 4983exit 101,assertion 'left == right' failed ... left: 0, right: 24. Restored: 3 passed.Over-reach guards, GREEN under both reverts:
TestIngressIdentityDoesNotDisturbEgressMatching4983(FIB egress result, the #4792 egress zone fallback, a non-interface filter),TestIngressIdentityUnresolvableIfindexFallsBackToZone4983, andingress_identity_does_not_occupy_the_fib_egress_slots_4983— which pins that the ingress identity does not leak into the FIB egress slots the CLI resolves the egress name from.Fixture realism: the binding zone has four members across three physical NICs — a single-interface zone makes the assertion true for free, and with two a "fix" that merely swapped X for the zone's other interface would still pass. Every fixture interface carries
unit 0, because that is how a real Junos config is written and because a unit-less interface produces no entry in the name map at all; the first draft omitted it and the assertions passed with the fix reverted.TestIngressIdentityAbsentFallsBackToZone4983holds a session with an identity and one without in the same test and asserts they are treated differently.Tests run
go build ./...— exit 0;go vet ./pkg/cli ./pkg/dataplane ./pkg/dataplane/userspace— exit 0;gofmt -lon every touched.go— clean.go test ./pkg/cli ./pkg/dataplane ./pkg/dataplane/userspace ./pkg/daemon ./pkg/cluster ./pkg/api ./pkg/grpcapi -count=1 -race— green.cargo test --release(full userspace-dp suite) — exit 0, 4242 + 124 passed.go test ./pkg/refactoraudit/ -count=1— exit 0.Docs
userspace-dp/src/session/README.md(new section beside the sibling #2785/#3056 stamping sections: where it is recorded, the0contract, the ABI + pinned-map consequence) anddocs/session-sync-architecture.md(why it is on the conntrack ABI yet deliberately not synced), plus_Log.md.Reviewer: attack these first
poll_descriptorstamp is the one link NOT bound by a fail-on-revert test. Changingingress_ifindex: meta.ingress_ifindexto0at the install site leaves the whole suite green. Every existing poll-level test intests_embedded_poll_filter.rseither pre-installs the session or asserts none is minted, so binding it needs a new harness that drives a transit flow throughpoll_binding_process_descriptorto the install point (full FIB + neighbor + permit fixture). I did not build it; the two ends of the chain (publish→ABI, ABI→CLI) are bound.meta.ingress_ifindexis the PHYSICAL bind ifindex, not the logical unit ifindex. I chose it overprerouting_ingress_scope'singress_logicalbecause a bondless-RETH VLAN unit can carry a synthetic logical ifindex (syntheticLogicalIfindex,pkg/dataplane/userspace/interfaces.go) that the CLI cannot reproduce;{physical, vlan}is resolvable by the map that already exists. Check I have that trade right.Adjacent, found, NOT fixed (deliberately out of scope)
The display surfaces still derive the ingress interface from the zone map and were left alone to keep this diff to the issue's stated scope (
session_filter.go,cli_clear.go, the dataplane):pkg/cli/cli_show_flow.go(theIn: ... If:column),pkg/api/sessions.go,pkg/grpcapi/server_sessions.go. Result: an interface-filteredshownow selects exactly, but theIf:column it prints for the selected session may still name the zone's representative interface. Worth a follow-up issue.🤖 Generated with Claude Code
https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi