Repository navigation
refactor: polyamide api and wireguard transports - #164
Conversation
starcatmeow
left a comment
starcatmeow
left a comment
There was a problem hiding this comment.
Found two regressions in the transport refactor; details are inline. These findings are based on static review.
starcatmeow
left a comment
starcatmeow
left a comment
There was a problem hiding this comment.
Two additional findings from the follow-up review. Both are based on static code inspection; no local tests or throughput benchmarks were run.
starcatmeow
left a comment
starcatmeow
left a comment
There was a problem hiding this comment.
Reviewed commit 305f4f4. All four previously reported issues are addressed: Linux TUN write-count handling, UAPI mutations bypassing adapter state, passive-client transport selection, and GRO buffer capacity.
No additional blocking issues found in the follow-up review. Review was based on static code inspection; tests were not run locally. Unit, integration, and CGO-disabled smoke tests passed in CI; the full e2e suite was still running at the time of review.
| func (n *Nylon) SyncApplicationState() error { | ||
| if n.Device == nil { | ||
| if len(n.Transports) == 0 { | ||
| return nil | ||
| } | ||
| if err := n.SyncWireGuard(); err != nil { | ||
| if err := n.SyncTransport(); err != nil { | ||
| return err | ||
| } | ||
| ComputeRoutes(n.RouterState, n) | ||
| return n.SyncSystemState() | ||
| } |
There was a problem hiding this comment.
[P1] one bad peer key stops OS route installs
SyncWireGuard accepted nodes sharing a key, zero keys, and peers sharing the local node's key, and only produced dead peers instead of throwing errors. SyncTransport now rejects them via the adapter's PreparePeer checks and returns an error, even though it still syncs every other node. This causes SyncSystemState to never run and breaks every router next to the bad node as OS routes and aliases are never installed or updated. This is a silent breaking change
| if w.listener != nil { | ||
| err = w.listener.Close() | ||
| } | ||
| w.workers.Wait() |
There was a problem hiding this comment.
[P2] Shutdown can hang forever while a nylon trace client is attached
close() now force-closes UAPI connections on shutdown (via the AfterFunc at line 62) and waits for their handlers here. However, handleTrace can't always return because ch is unbuffered, and go-broadcast delivers with a blocking ch <- m. When the connection closes under trace traffic, the broadcaster is stuck because the handler has stopped reading ch. The deferred Unregister(ch) (ipc_handler.go:519) then blocks on unreg, which the broadcaster never reads again. So w.workers.Wait() deadlocks.
As a result, cleanupTransport stops after pre_down: the TUN and transports are never closed, post_down never runs, and later SIGINTs are ignored, so the process needs SIGKILL. This semi-consistently hangs (7/10 runs when saturated) when run under --dbg-trace-tc.
encodeous
changed the title
extensive changes were made in preparation of separating wireguard from nylon/polyamide.
3 participants
No description provided.