-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
[client] Diagnostic logs #6681
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
[client] Diagnostic logs #6681
Changes from all commits
8012294
0860660
bf9de56
2f3bf5b
dd20a40
e24ae10
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -231,11 +231,15 @@ func (w *Watcher) getBestRouteFromStatuses(routePeerStatuses map[route.ID]router | |||||||||||||||||||||||||||||||||||||||
| switch { | ||||||||||||||||||||||||||||||||||||||||
| case chosen == "": | ||||||||||||||||||||||||||||||||||||||||
| var peers []string | ||||||||||||||||||||||||||||||||||||||||
| for _, r := range w.routes { | ||||||||||||||||||||||||||||||||||||||||
| peers = append(peers, r.Peer) | ||||||||||||||||||||||||||||||||||||||||
| for id, r := range w.routes { | ||||||||||||||||||||||||||||||||||||||||
| st := "unknown(no status)" | ||||||||||||||||||||||||||||||||||||||||
| if ps, ok := routePeerStatuses[id]; ok { | ||||||||||||||||||||||||||||||||||||||||
| st = fmt.Sprintf("%s relayed=%v lat=%v", ps.status, ps.relayed, ps.latency) | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| peers = append(peers, fmt.Sprintf("%s{%s}", r.Peer, st)) | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| log.Infof("network [%v] has not been assigned a routing peer as no peers from the list %s are currently available", w.handler, peers) | ||||||||||||||||||||||||||||||||||||||||
| log.Warnf("ROUTE-DIAG: network [%v] no routing peer available yet (all candidates connecting/absent); candidates: %s", w.handler, peers) | ||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+234
to
+242
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win Format the candidate list as a string.
🐛 Proposed fix- log.Warnf("ROUTE-DIAG: network [%v] no routing peer available yet (all candidates connecting/absent); candidates: %s", w.handler, peers)
+ log.Warnf("ROUTE-DIAG: network [%v] no routing peer available yet (all candidates connecting/absent); candidates: %v", w.handler, peers)📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||||||||||
| case chosen != currID: | ||||||||||||||||||||||||||||||||||||||||
| // we compare the current score + 10ms to the chosen score to avoid flapping between routes | ||||||||||||||||||||||||||||||||||||||||
| if currScore != 0 && currScore+0.01 > chosenScore { | ||||||||||||||||||||||||||||||||||||||||
|
|
@@ -247,7 +251,7 @@ func (w *Watcher) getBestRouteFromStatuses(routePeerStatuses map[route.ID]router | |||||||||||||||||||||||||||||||||||||||
| if rt := w.routes[chosen]; rt != nil { | ||||||||||||||||||||||||||||||||||||||||
| p = rt.Peer | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
| log.Infof("New chosen route is %s with peer %s with score %f for network [%v]", chosen, p, chosenScore, w.handler) | ||||||||||||||||||||||||||||||||||||||||
| log.Warnf("ROUTE-DIAG: new chosen route %s peer %s score %f for network [%v] (routing peer now usable)", chosen, p, chosenScore, w.handler) | ||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
| return chosen, chosenStatus | ||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
Repository: netbirdio/netbird
Length of output: 13890
🏁 Script executed:
Repository: netbirdio/netbird
Length of output: 50374
🏁 Script executed:
Repository: netbirdio/netbird
Length of output: 27828
🏁 Script executed:
Repository: netbirdio/netbird
Length of output: 648
Use the peer IP family here instead of hardcoding
udp4.AllowedIpscan include IPv6 when the interface has IPv6, so this can reject valid peers and skip Rosenpass setup; switch toudp6/udp4based onwireGuardIP, or reject v6 earlier if Rosenpass must stay v4-only.🤖 Prompt for AI Agents
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@coderabbitai ipv4 should always exist, while V6 should be only added as optional, so this should work. Do you see other possible places where V6 is uniquely used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
Length of output: 29244
🏁 Script executed:
Length of output: 3289
🏁 Script executed:
Length of output: 6135
🏁 Script executed:
Length of output: 2967
🏁 Script executed:
Length of output: 421
🏁 Script executed:
Length of output: 4303
@riccardomanfrinConfirmed — I don't see any other place where IPv6 is uniquely used for Rosenpass or the PSK path.I traced the call chain:
wireGuardIPpassed intoManager.OnConnected/addPeeroriginates fromconn.config.WgConfig.AllowedIps[0].Addr().String()inclient/internal/peer/conn.go(Line 753). This same invariant — "AllowedIps[0]is always v4" — is already explicitly relied upon elsewhere in the codebase, e.g.client/internal/engine.go(Line 699):// IPv4-only: rosenpass peers connect via AllowedIps[0] which is always v4.IPv6, when present, is always handled as a distinct, separate address (via
overlayAddrsFromAllowedIPs,PeerConfig.AddressV6), never asAllowedIps[0]or as the sole overlay address. So theudp4hardcoding inaddPeeris safe and consistent with the existing convention — no other Rosenpass/PSK code path depends on IPv6 uniquely. My original concern doesn't hold up against this invariant; withdrawing it.(๑•̀ㅂ•́)و good catch confirming the design invariant
🧬 Code Graph Analysis Results
client/internal/rosenpass/manager.goIPv4-only Rosenpass listener + rationale (mentions IPv6-related failure mode):
Lines 80-89:
Peer endpoint resolution forced to IPv4 (“udp4”) (also notes “WireGuard overlay IP is always IPv4”):
Lines 91-121:
Config wiring uses the IPv4 listener returned by
GetAddress():Lines 146-177 (relevant excerpt):
Random UDP port selection uses an IPv4 bind address (
net.IPv4zero):Lines 300-312:
shared/management/proto/management.pb.goPeerConfigincludes an IPv6 overlay address field (AddressV6) which may be optional/unique vs IPv4 (Address):Lines 2413-2433:
client/embed/embed.goUDP listener creation uses
net.ResolveUDPAddr("udp", ...)(protocol family resolution depends on OS/name/address):Lines 391-409:
✏️ Learnings added
🧠 Learnings used