grpcapi+routing: #4650 GO residuals (A9-F3 gRPC egress parity + A10-F-1)#4672
Merged
Conversation
The gRPC sessionEntryV4/sessionEntryV6 entry builders inlined an
UNCONDITIONAL egressIfaces[{FibIfindex, FibVlanID}] lookup, while the
REST entry path (pkg/api/sessions.go) and the gRPC session-filter path
(server_sessions.go:480/525) both resolve the egress interface through
the FibIfindex!=0-guarded resolveSessionEgressIface helper. This made
gRPC internally inconsistent with its own filter path and drifted from
REST: a session with FibIfindex==0 plus a stale {ifindex:0, vlanID}
egressIfaces entry printed that bogus interface over gRPC while REST
printed the egress zone's interface.
Route both entry paths through the same guarded helper, preserving the
zoneNames display-name fallback (helper returns zoneIfaces[egressZone];
the entry falls back to zoneNames[egressZone] when empty) so the output
is bit-identical to the REST entry path for every session, and a
FibIfindex==0 session no longer consults a stale FIB entry.
Display-only correction (no forwarding effect) -> LOW.
Added session_egress_drift_4650_test.go: a REST-oracle parity test that
independently reproduces the guarded resolution and asserts gRPC V4+V6
match it for both the stale-{0,vlan} case (RED on revert: gRPC returned
the stale name) and an unchanged FibIfindex!=0 session.
go test ./pkg/grpcapi/... ./pkg/api/... green; go build; gofmt+vet clean.
Addresses #4650 (A9-F3).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
…F-1) fable-173 A10-F-1 flagged tunnelManager.Apply holding t.mu across the full netlink+exec reconcile as possible lock-scope hygiene. Assessed: the wide scope is load-bearing, not accidental. The reconcile interleaves shared-map reads/writes (ownedNames, appliedAddrs, appliedRI, wgConfigured, tunnels) with the netlink LinkDel/LinkAdd it drives — per-tunnel removal/adoption decisions are made FROM the maps and failure retries written BACK into them — and every applyKernelTunnelLocked/ applyAnchorLocked/applyWireguardTunLocked helper requires the lock. Two concurrent Apply/Clear calls would race BOTH the maps AND the kernel link state (double-delete, delete-during-recreate). There is no isolable critical section to narrow to; GetStatus is the counter-case that already narrows (snapshot names under the lock, probe netlink read-only unlocked). No behavior change. Added an inline lock-discipline comment at the Apply lock site so the wide scope is not re-flagged as hygiene, and logged the disposition in _Log.md (covers this and the A9-F3 gRPC egress fix). Addresses #4650 (A10-F-1; kept deliberate). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015oARShYtiJJ2H4UB4nXGqi
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses #4650 “GO residuals” by aligning gRPC session egress-interface display with the REST/oracle behavior (eliminating a drift when FibIfindex==0), and documenting why tunnelManager.Apply() intentionally holds t.mu across the full reconcile.
Changes:
- Route gRPC
sessionEntryV4/sessionEntryV6egress-interface resolution through the guardedresolveSessionEgressIfacehelper, matching the REST path’s resolution order. - Add a parity test that goes RED on revert for the stale
{ifindex:0,vlan}egressIfaces scenario (v4 + v6). - Add an inline comment documenting tunnel Apply lock discipline; update
_Log.mdaccordingly.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| pkg/grpcapi/server_sessions.go | Fix gRPC session entry egress-interface resolution to match guarded REST/filter behavior. |
| pkg/grpcapi/session_egress_drift_4650_test.go | Add parity test covering the drift case (v4/v6) and a normal FIB-resolved case. |
| pkg/routing/tunnel.go | Document deliberate wide t.mu lock scope for Apply reconcile (no behavior change). |
| _Log.md | Record the #4650 changes and rationale. |
Comment on lines
+10
to
+13
| // restEgressOracle reproduces, independently of the shared | ||
| // resolveSessionEgressIface helper, exactly how the REST path | ||
| // (pkg/api/sessions.go sessionEntryV4/V6) resolves a session's egress | ||
| // interface: the FibIfindex!=0-guarded egressIfaces lookup, then the |
Comment on lines
+5
to
+7
| egress-interface resolution through the same `FibIfindex!=0`-guarded | ||
| `resolveSessionEgressIface` helper that REST (pkg/api/sessions.go) and the | ||
| gRPC filter path (server_sessions.go:480/525) already use. The two entry |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Addresses #4650 (A9-F3 gRPC egress parity + A10-F-1; the Rust residuals A2-F1/A2-F2/A4-F5/A4-F8 -> separate cargo task).
A9-F3 (LOW) — session egress-iface display drift between gRPC and REST
The gRPC
sessionEntryV4/sessionEntryV6entry builders inlined anUNCONDITIONAL
egressIfaces[{FibIfindex, FibVlanID}]lookup, while theREST entry path (
pkg/api/sessions.go) and the gRPC session-filter path(
server_sessions.go:480/525) both resolve the egress interface throughthe
FibIfindex!=0-guardedresolveSessionEgressIfacehelper. A sessionwith
FibIfindex==0plus a stale{ifindex:0, vlanID}egressIfaces entrytherefore printed that bogus interface over gRPC while REST printed the
egress zone's interface — a drift between the two APIs, and gRPC was
internally inconsistent with its own filter path.
Fix: route both entry paths through the guarded helper, preserving the
zoneNamesdisplay-name fallback so the output is bit-identical to theREST entry path. Display-only correction, no forwarding effect -> LOW.
New
pkg/grpcapi/session_egress_drift_4650_test.go: a REST-oracle paritytest covering V4+V6 — the stale-
{0,vlan}case (RED on revert: gRPCreturned the stale name) and an unchanged
FibIfindex!=0session.A10-F-1 (LOW hygiene) — tunnel Apply lock scope: kept deliberate
Assessed
routing/tunnel.goApplyholdingt.muacross the fullnetlink+exec reconcile. It is load-bearing: the reconcile interleaves
shared-map reads/writes with the netlink LinkDel/LinkAdd it drives, every
apply*Lockedhelper requires the lock, and two concurrent Apply/Clearcalls would race both the maps and kernel link state (double-delete,
delete-during-recreate). No isolable critical section to narrow to;
GetStatus is the counter-case that already narrows. Added an inline
lock-discipline comment; no behavior change.
Validation
go test ./pkg/grpcapi/... ./pkg/api/... ./pkg/routing/...green;go build ./...; gofmt + vet clean. RED-on-revert verified for A9-F3 (both V4 and V6 printge-STALE-0on the pre-fix unconditional lookup).