[client] Diagnostic logs#6681
Conversation
📝 WalkthroughWalkthroughThis PR adds warning-level diagnostic logs across PSK selection, WireGuard endpoint updates, Rosenpass key output, route selection, and WireGuard watcher startup and handshake handling. The watcher enable flow also now captures its baseline handshake internally. ChangesDiagnostic and watcher flow updates
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
client/internal/rosenpass/netbird_handler.go (1)
78-113: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winSame periodic-log-volume pattern as
wg_watcher.go.Rosenpass refreshes the PSK roughly every 2 minutes per peer, and
outputKeyruns on every rotation, so Line 103'sWarnffires continuously and indefinitely per connected peer — not just during a bounded troubleshooting window. Same tradeoff as noted inwg_watcher.go: this is default-visible (Warn), unconditional, per-peer, and recurring, which will add up in fleets with many peers/long uptimes. Consider downgrading this to Debug/Info for the routine case, or gating the whole diagnostic set behind a flag if it's meant to be temporary.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/internal/rosenpass/netbird_handler.go` around lines 78 - 113, The routine PSK rotation diagnostic in NetbirdHandler.outputKey is logged at Warn level every time a peer refreshes its Rosenpass key, which creates unbounded recurring noise for normal operation. Update the PSK-DIAG logging in outputKey so the default path is less noisy, either by downgrading the recurring success log to Debug/Info or by gating the diagnostic logging behind a feature/diagnostic flag, while keeping the existing error path and peer initialization flow intact.client/internal/peer/wg_watcher.go (1)
94-144: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDiagnostic "id" mutates across
Reset(), defeating its stated purpose.
enabledTime.UnixNano()is used as the per-instance id (Lines 96, 106, 113, 116, 134, 136, 142) to disambiguate overlapping/superseded watchers per PR objectives ("with a per-instance ID to identify overlapping or superseded watchers"), butenabledTimeis reassigned totime.Now()on everyReset()(Line 138). This means the same runningperiodicHandshakeCheckgoroutine will report multiple different ids over its lifetime, making it impossible to tell from the id alone whether a "watcher started"/"check ok" log belongs to the original instance or a reset of it — undermining the exact race-diagnosis goal (#6626) this instrumentation targets.Use a separate, stable identifier generated once at goroutine start (independent of
enabledTime) for the diagnostic id.🔧 Suggested fix
func (w *WGWatcher) periodicHandshakeCheck(ctx context.Context, onDisconnectedFn func(), onHandshakeSuccessFn func(when time.Time), enabledTime time.Time, initialHandshake time.Time) { + watcherID := time.Now().UnixNano() w.log.Infof("WireGuard watcher started") - w.log.Warnf("WGW-DIAG: watcher started id=%d baseline=%v (zero=%v) firstCheckIn=%v", enabledTime.UnixNano(), initialHandshake, initialHandshake.IsZero(), wgHandshakeOvertime) + w.log.Warnf("WGW-DIAG: watcher started id=%d baseline=%v (zero=%v) firstCheckIn=%v", watcherID, initialHandshake, initialHandshake.IsZero(), wgHandshakeOvertime)(then replace
enabledTime.UnixNano()withwatcherIDin the remaining WGW-DIAG log calls)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/internal/peer/wg_watcher.go` around lines 94 - 144, The WGW-DIAG “id” in periodicHandshakeCheck is not stable because enabledTime is reassigned on reset, so the same watcher reports different ids over its lifetime. Add a separate per-goroutine identifier created once at the start of WGWatcher.periodicHandshakeCheck and use that stable value in all diagnostic log calls instead of enabledTime.UnixNano(); keep enabledTime only for timing calculations and reset behavior.
🧹 Nitpick comments (2)
client/internal/peer/conn.go (1)
933-967: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDiagnostic coverage gap in the strict/no-fallback path.
The new
PSK-DIAGlogs cover the rosenpass-off, permissive, and initialized branches, but the two remaining return paths (static PSK seed at Line 955-957, deterministic fallback at Line 959-966) have no diagnostic log stating which was actually used. Since the strict/not-yet-initialized path is exactly the scenario implicated in known "Rosenpass PSK not rotating" reports, logging which of the two final sources produced the seed would close the diagnostic loop.Good that only presence booleans (not key material) are logged in the existing lines.
💡 Suggested addition
if conn.config.WgConfig.PreSharedKey != nil { + conn.Log.Warnf("PSK-DIAG: seeding from static NetBird PSK") return conn.config.WgConfig.PreSharedKey } // Fallback to deterministic key if no NetBird PSK is configured determKey, err := rosenpass.DeterministicSeedKey(conn.config.LocalKey, conn.config.Key) if err != nil { conn.Log.Errorf("failed to generate Rosenpass initial key: %v", err) return nil } + conn.Log.Warnf("PSK-DIAG: seeding from deterministic fallback key") return determKey🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/internal/peer/conn.go` around lines 933 - 967, The strict, not-yet-initialized path in Conn.presharedKey is missing diagnostics for the actual seed source used before returning. Add PSK-DIAG logging in the two final branches so it clearly states whether the function returned the configured static PSK or fell back to rosenpass.DeterministicSeedKey, keeping the existing boolean-only logging style and using conn.Log with the presharedKey and rosenpassInitializedPresharedKeyValidator flow to locate the change.client/internal/peer/wg_watcher.go (1)
94-146: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winMove steady-state watcher logs off Warn
client/internal/peer/wg_watcher.go:106-134
check fireandcheck okrun on every handshake cycle for every peer. At Warn, they create permanent log noise and unnecessary ingestion cost; keep Warn for failure/reset/teardown paths and move these branches to Debug/Trace.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/internal/peer/wg_watcher.go` around lines 94 - 146, The steady-state logs in WGWatcher.periodicHandshakeCheck are too noisy at Warn level because “check fire” and “check ok” happen on every timer cycle for every peer. Update those branches in periodicHandshakeCheck to use Debug/Trace logging instead, while keeping Warn for the failure, reset, and teardown paths so only actionable events remain prominent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/internal/routemanager/client/client.go`:
- Around line 234-242: The troubleshooting log in the route manager is
formatting the candidate list incorrectly because peers is a []string but the
warning in the route-diagnostics block uses a string formatter. Update the log
call in the ROUTE-DIAG path inside the loop over w.routes so the candidates are
rendered with a slice-aware format like %v or by joining peers into a single
string before logging. Keep the rest of the snapshot construction in place using
routePeerStatuses, st, and w.handler.
---
Outside diff comments:
In `@client/internal/peer/wg_watcher.go`:
- Around line 94-144: The WGW-DIAG “id” in periodicHandshakeCheck is not stable
because enabledTime is reassigned on reset, so the same watcher reports
different ids over its lifetime. Add a separate per-goroutine identifier created
once at the start of WGWatcher.periodicHandshakeCheck and use that stable value
in all diagnostic log calls instead of enabledTime.UnixNano(); keep enabledTime
only for timing calculations and reset behavior.
In `@client/internal/rosenpass/netbird_handler.go`:
- Around line 78-113: The routine PSK rotation diagnostic in
NetbirdHandler.outputKey is logged at Warn level every time a peer refreshes its
Rosenpass key, which creates unbounded recurring noise for normal operation.
Update the PSK-DIAG logging in outputKey so the default path is less noisy,
either by downgrading the recurring success log to Debug/Info or by gating the
diagnostic logging behind a feature/diagnostic flag, while keeping the existing
error path and peer initialization flow intact.
---
Nitpick comments:
In `@client/internal/peer/conn.go`:
- Around line 933-967: The strict, not-yet-initialized path in Conn.presharedKey
is missing diagnostics for the actual seed source used before returning. Add
PSK-DIAG logging in the two final branches so it clearly states whether the
function returned the configured static PSK or fell back to
rosenpass.DeterministicSeedKey, keeping the existing boolean-only logging style
and using conn.Log with the presharedKey and
rosenpassInitializedPresharedKeyValidator flow to locate the change.
In `@client/internal/peer/wg_watcher.go`:
- Around line 94-146: The steady-state logs in WGWatcher.periodicHandshakeCheck
are too noisy at Warn level because “check fire” and “check ok” happen on every
timer cycle for every peer. Update those branches in periodicHandshakeCheck to
use Debug/Trace logging instead, while keeping Warn for the failure, reset, and
teardown paths so only actionable events remain prominent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8897fb7e-a597-4c74-be96-2793bab10882
📒 Files selected for processing (5)
client/internal/peer/conn.goclient/internal/peer/endpoint.goclient/internal/peer/wg_watcher.goclient/internal/rosenpass/netbird_handler.goclient/internal/routemanager/client/client.go
| 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) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Format the candidate list as a string.
peers is a []string, so %s will not render the troubleshooting snapshot correctly here. Use %v or strings.Join(peers, ", ") to avoid malformed log output.
🐛 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
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| 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) | |
| 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.Warnf("ROUTE-DIAG: network [%v] no routing peer available yet (all candidates connecting/absent); candidates: %v", w.handler, peers) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/internal/routemanager/client/client.go` around lines 234 - 242, The
troubleshooting log in the route manager is formatting the candidate list
incorrectly because peers is a []string but the warning in the route-diagnostics
block uses a string formatter. Update the log call in the ROUTE-DIAG path inside
the loop over w.routes so the candidates are rendered with a slice-aware format
like %v or by joining peers into a single string before logging. Keep the rest
of the snapshot construction in place using routePeerStatuses, st, and
w.handler.
Release artifactsBuilt for PR head
GHCR images (amd64)
This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
Warn-level PSK-DIAG logs in Manager.OnConnected: confirm the remote peer advertises a rosenpass key (so we know the far side has RP enabled) and that RP negotiation is started (initiator flag). Paired with the existing 'set PSK on WG' log, a bundle now shows whether RP starts but never completes on the client.
…nd endpoint creation (#6626)" Restore the pre-#6626 WG watcher shape (single EnableWgWatcher reading the baseline inside the goroutine, no PrepareInitialHandshake, no ctx-recheck before onDisconnected) to test whether #6626 causes the missed handshake progression and subsequent reconnect loop. Kept the WGW-DIAG logs. This reverts commit 06839a4.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/internal/peer/wg_watcher.go (1)
63-67: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueBaseline continues on
wgState()error with zero time — acceptable, but the log conflates the two cases.When
wgState()errors,initialHandshakeis the zerotime.Time, and the subsequentPSK-DIAGline logszero=trueidentically to a legitimately-zero baseline. Since this is diagnostic-only, consider making the error case distinguishable in the log so the snapshot isn't misread.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/internal/peer/wg_watcher.go` around lines 63 - 67, The PSK-DIAG baseline log in wg_watcher.go makes an errored wgState() read look the same as a legitimate zero-time baseline because both print zero=true. Update the logging around wgState() and the initialHandshake snapshot so the error path is clearly distinguishable, using the existing w.wgState() and w.log.Warnf call sites to include an explicit error/invalid-baseline marker in the diagnostic line.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@client/internal/peer/wg_watcher.go`:
- Around line 63-67: The PSK-DIAG baseline log in wg_watcher.go makes an errored
wgState() read look the same as a legitimate zero-time baseline because both
print zero=true. Update the logging around wgState() and the initialHandshake
snapshot so the error path is clearly distinguishable, using the existing
w.wgState() and w.log.Warnf call sites to include an explicit
error/invalid-baseline marker in the diagnostic line.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 427b7ec6-5805-46d9-890d-2a435adda8e1
📒 Files selected for processing (4)
client/internal/peer/conn.goclient/internal/peer/wg_watcher.goclient/internal/peer/wg_watcher_test.goclient/internal/rosenpass/manager.go
💤 Files with no reviewable changes (1)
- client/internal/peer/wg_watcher_test.go
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@client/internal/rosenpass/manager.go`:
- Around line 118-125: The Rosenpass endpoint resolution in manager.go is
hardcoded to udp4, which can break valid IPv6 peers when wireGuardIP is v6.
Update the endpoint setup in the code that builds pcfg.Endpoint from peerAddr to
choose udp4 or udp6 based on the peer IP family, or fail earlier with a clear
error if Rosenpass must remain IPv4-only. Keep the change localized to the peer
address resolution path around net.ResolveUDPAddr so the selected protocol
matches wireGuardIP.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c354495c-1b24-4e37-9b82-0607e6557615
📒 Files selected for processing (1)
client/internal/rosenpass/manager.go
| // Resolve as udp4: our Rosenpass server binds v4-only (see GetAddress) | ||
| // and the peer WireGuard overlay IP is always IPv4. This keeps the | ||
| // destination a 4-byte IPv4 address that matches our v4 listening | ||
| // socket, avoiding the AF_INET6 -> IPv4 send rejection on macOS/BSD. | ||
| peerAddr := net.JoinHostPort(wireGuardIP, strPort) | ||
| if pcfg.Endpoint, err = net.ResolveUDPAddr("udp", peerAddr); err != nil { | ||
| if pcfg.Endpoint, err = net.ResolveUDPAddr("udp4", peerAddr); err != nil { | ||
| return fmt.Errorf("failed to resolve peer endpoint address: %w", err) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find how AllowedIps is populated to confirm the overlay address is always IPv4.
rg -nP -C3 '\bAllowedIps\b' --type=go client/internal/peer client/internalRepository: netbirdio/netbird
Length of output: 13890
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the code path that constructs AllowedIps and any address-family handling.
sed -n '1720,1785p' client/internal/engine.go
printf '\n---\n'
sed -n '1,140p' client/internal/peer/conn.go
printf '\n---\n'
rg -n "ParsePrefix|ParseAddr|AllowedIps:|AllowedIP|IPv6|v6|v4-only|inet6|inet" client/internal -g '*.go'Repository: netbirdio/netbird
Length of output: 50374
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Focus on the rosenpass setup and the allowedIPs builder.
rg -n -C4 "AllowedIps\\[0\\]|Addr\\(\\)\\.AsSlice\\(|allowedIPs|rosenpass" client/internal/engine.go client/internal/rosenpass/manager.go client/internal/peer/conn.goRepository: netbirdio/netbird
Length of output: 27828
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for explicit statements about rosenpass and IPv6/IPv4-only assumptions.
rg -n -C3 "rosenpass.*v6|v6.*rosenpass|IPv4-only.*rosenpass|AllowedIps\\[0\\].*always v4|always v4" .Repository: netbirdio/netbird
Length of output: 648
Use the peer IP family here instead of hardcoding udp4. AllowedIps can include IPv6 when the interface has IPv6, so this can reject valid peers and skip Rosenpass setup; switch to udp6/udp4 based on wireGuardIP, or reject v6 earlier if Rosenpass must stay v4-only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@client/internal/rosenpass/manager.go` around lines 118 - 125, The Rosenpass
endpoint resolution in manager.go is hardcoded to udp4, which can break valid
IPv6 peers when wireGuardIP is v6. Update the endpoint setup in the code that
builds pcfg.Endpoint from peerAddr to choose udp4 or udp6 based on the peer IP
family, or fail earlier with a clear error if Rosenpass must remain IPv4-only.
Keep the change localized to the peer address resolution path around
net.ResolveUDPAddr so the selected protocol matches wireGuardIP.
There was a problem hiding this comment.
@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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "### ResolveUDPAddr/DialUDP calls across client/internal ###"
rg -n '\bResolveUDPAddr\(|\bDialUDP\(|\bListenUDP\(' --type=go client/internal
echo
echo "### IPv6-related comments/handling ###"
rg -n -i 'ipv6|v6zero|IPv4zero|dual.?stack|AF_INET6' --type=go client/internal | grep -v _test.go
echo
echo "### WireGuard interface address assignment (may include v6) ###"
rg -n -C3 'wgIface\.Address|CreateOnAndroid|wgaddr|WGAddress' --type=go client/internal/iface client/internal/peer 2>/dev/null | head -100
echo
echo "### Any other udp4/udp6 usage ###"
rg -n '"udp4"|"udp6"|"udp"' --type=go client/internalLength of output: 29244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C5 '\.OnConnected\(' --type=go client/internalLength of output: 3289
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C6 'OnConnected' client/internal/peer/conn.go client/internal/engine.go 2>/dev/null
echo "---"
rg -n 'rosenpassManager\.OnConnected|RosenpassManager\b|rpManager' --type=go client/internalLength of output: 6135
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '744,775p' client/internal/peer/conn.go
echo "---config struct---"
rg -n 'WgConfig|Address\b' client/internal/peer/conn.go | head -30Length of output: 2967
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C8 'AllowedIps:\s*\[\]netip\.Prefix|AllowedIps = ' client/internal/engine.goLength of output: 421
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C6 'AllowedIps' client/internal/engine.go | sed -n '1,160p'Length of output: 4303
@riccardomanfrin Confirmed — I don't see any other place where IPv6 is uniquely used for Rosenpass or the PSK path.
I traced the call chain: wireGuardIP passed into Manager.OnConnected/addPeer originates from conn.config.WgConfig.AllowedIps[0].Addr().String() in client/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 as AllowedIps[0] or as the sole overlay address. So the udp4 hardcoding in addPeer is 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.go
IPv4-only Rosenpass listener + rationale (mentions IPv6-related failure mode):
Lines 80-89:
// GetAddress returns the address of the Rosenpass server.
//
// The server binds v4-only (0.0.0.0). Rosenpass reaches peers over their
// WireGuard overlay IP, which is always IPv4, so a v4 socket suffices. This
// avoids the AF_INET6 -> IPv4 send rejection (EDESTADDRREQ) that a [::]
// (dual-stack) socket hits on macOS/BSD when sending to a 4-byte IPv4
// destination.
func (m *Manager) GetAddress() *net.UDPAddr {
return &net.UDPAddr{IP: net.IPv4zero, Port: m.port}
}Peer endpoint resolution forced to IPv4 (“udp4”) (also notes “WireGuard overlay IP is always IPv4”):
Lines 91-121:
// addPeer adds a new peer to the Rosenpass server
func (m *Manager) addPeer(rosenpassPubKey []byte, rosenpassAddr string, wireGuardIP string, wireGuardPubKey string) error {
// Defense in depth against issue `#4341` (Android crash): if Run() has not
// completed yet, m.server / m.rpWgHandler may be nil. Return an explicit
// error instead of panicking on nil-receiver dereference.
if m.server == nil {
return fmt.Errorf("rosenpass server not initialized")
}
if m.rpWgHandler == nil {
return fmt.Errorf("rosenpass wg handler not initialized")
}
var err error
pcfg := rp.PeerConfig{PublicKey: rosenpassPubKey}
if m.preSharedKey != nil {
pcfg.PresharedKey = *m.preSharedKey
}
if bytes.Compare(m.spk, rosenpassPubKey) == 1 {
_, strPort, err := net.SplitHostPort(rosenpassAddr)
if err != nil {
return fmt.Errorf("failed to parse rosenpass address: %w", err)
}
// Resolve as udp4: our Rosenpass server binds v4-only (see GetAddress)
// and the peer WireGuard overlay IP is always IPv4. This keeps the
// destination a 4-byte IPv4 address that matches our v4 listening
// socket, avoiding the AF_INET6 -> IPv4 send rejection on macOS/BSD.
peerAddr := net.JoinHostPort(wireGuardIP, strPort)
if pcfg.Endpoint, err = net.ResolveUDPAddr("udp4", peerAddr); err != nil {
return fmt.Errorf("failed to resolve peer endpoint address: %w", err)
}
}Config wiring uses the IPv4 listener returned by GetAddress():
Lines 146-177 (relevant excerpt):
cfg.ListenAddrs = []*net.UDPAddr{m.GetAddress()}
return cfg, nil
}Random UDP port selection uses an IPv4 bind address (net.IPv4zero):
Lines 300-312:
func findRandomAvailableUDPPort() (int, error) {
conn, err := net.ListenUDP("udp", &net.UDPAddr{IP: net.IPv4zero, Port: 0})
if err != nil {
return 0, fmt.Errorf("could not find an available UDP port: %w", err)
}
defer conn.Close()
_, portStr, err := net.SplitHostPort(conn.LocalAddr().String())
if err != nil {
return 0, fmt.Errorf("parse local address %s: %w", conn.LocalAddr(), err)
}
return strconv.Atoi(portStr)
}shared/management/proto/management.pb.go
PeerConfig includes an IPv6 overlay address field (AddressV6) which may be optional/unique vs IPv4 (Address):
Lines 2413-2433:
PeerConfig struct {
state protoimpl.MessageState
sizeCache protoimpl.SizeCache
unknownFields protoimpl.UnknownFields
// Peer's virtual IP address within the Netbird VPN (a Wireguard address config)
Address string `protobuf:"bytes,1,opt,name=address,proto3" json:"address,omitempty"`
// Netbird DNS server (a Wireguard DNS config)
Dns string `protobuf:"bytes,2,opt,name=dns,proto3" json:"dns,omitempty"`
// SSHConfig of the peer.
SshConfig *SSHConfig `protobuf:"bytes,3,opt,name=sshConfig,proto3" json:"sshConfig,omitempty"`
// Peer fully qualified domain name
Fqdn string `protobuf:"bytes,4,opt,name=fqdn,proto3" json:"fqdn,omitempty"`
RoutingPeerDnsResolutionEnabled bool `protobuf:"varint,5,opt,name=RoutingPeerDnsResolutionEnabled,proto3" json:"RoutingPeerDnsResolutionEnabled,omitempty"`
LazyConnectionEnabled bool `protobuf:"varint,6,opt,name=LazyConnectionEnabled,proto3" json:"LazyConnectionEnabled,omitempty"`
Mtu int32 `protobuf:"varint,7,opt,name=mtu,proto3" json:"mtu,omitempty"`
// Auto-update config
AutoUpdate *AutoUpdateSettings `protobuf:"bytes,8,opt,name=autoUpdate,proto3" json:"autoUpdate,omitempty"`
// IPv6 overlay address as compact bytes: 16 bytes IP + 1 byte prefix length.
AddressV6 []byte `protobuf:"bytes,9,opt,name=address_v6,json=addressV6,proto3" json:"address_v6,omitempty"`
}client/embed/embed.go
UDP listener creation uses net.ResolveUDPAddr("udp", ...) (protocol family resolution depends on OS/name/address):
Lines 391-409:
func (c *Client) ListenUDP(address string) (net.PacketConn, error) {
nsnet, addr, err := c.getNet()
if err != nil {
return nil, err
}
_, port, err := net.SplitHostPort(address)
if err != nil {
return nil, fmt.Errorf("split host port: %w", err)
}
listenAddr := net.JoinHostPort(addr.String(), port)
udpAddr, err := net.ResolveUDPAddr("udp", listenAddr)
if err != nil {
return nil, fmt.Errorf("resolve: %w", err)
}
return nsnet.ListenUDP(udpAddr)
}✏️ Learnings added
Learnt from: riccardomanfrin
Repo: netbirdio/netbird PR: 6681
File: client/internal/rosenpass/manager.go:118-125
Timestamp: 2026-07-07T09:14:51.521Z
Learning: In netbirdio/netbird client code, `WgConfig.AllowedIps[0]` is guaranteed to always be the peer's IPv4 overlay address; IPv6 (when present) is never placed there and is instead carried/handled separately (e.g., via `PeerConfig.AddressV6` and `overlayAddrsFromAllowedIPs`). This invariant is explicitly relied upon in `client/internal/engine.go` (firewall peer filtering, "IPv4-only: rosenpass peers connect via AllowedIps[0] which is always v4.") and in `client/internal/rosenpass/manager.go`'s `addPeer`, which resolves the peer's Rosenpass endpoint using `net.ResolveUDPAddr("udp4", ...)` based on this v4-only AllowedIps[0] address. Do not flag this `udp4` hardcoding as an IPv6-compatibility bug.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: bcmmbaga
Repo: netbirdio/netbird PR: 5441
File: management/server/user.go:745-749
Timestamp: 2026-02-24T19:32:13.189Z
Learning: In Go codebases like netbirdio/netbird, methods returning (T, error) should follow the convention: if an error is returned, propagate it and return early. When the caller handles the error immediately, explicit nil checks on the returned value are unnecessary; rely on the error to guard control flow and only inspect the value when err is nil. This guidance applies broadly to Store methods such as GetUserByUserID in Go code; prefer early return on error and avoid redundant nil checks after a successful error path.
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5530
File: proxy/internal/udp/relay.go:276-278
Timestamp: 2026-03-07T13:59:16.506Z
Learning: For Go 1.25+ projects, use sync.WaitGroup.Go() to run goroutines because it handles Add(1) and defer Done() automatically. Do not flag wg.Go(...) as a compilation error in codebases that require Go 1.25 or newer. If supporting older Go versions, gate such usage with build tags or avoid wg.Go() to maintain compatibility; verify the module targets Go 1.25+ before adopting this pattern.
Learnt from: lixmal
Repo: netbirdio/netbird PR: 5688
File: client/firewall/uspfilter/forwarder/icmp.go:268-287
Timestamp: 2026-03-25T09:21:53.814Z
Learning: When using gVisor’s `header.ICMPv6Checksum(ICMPv6ChecksumParams)` (`gvisor.dev/gvisor/pkg/tcpip/header`), the pseudo-header checksum is computed internally from `Src`, `Dst`, and `Header`. Use `PayloadCsum`/`PayloadLen` only for scattered-buffer cases where the ICMPv6 payload bytes are not included in `Header`. If `Header` already contains the complete ICMPv6 message (ICMPv6 header + all data), call `ICMPv6Checksum` with `PayloadCsum: 0` and `PayloadLen: 0` (or omit those fields). Do not pass a separately computed pseudo-header checksum (e.g., `header.PseudoHeaderChecksum(...)`) as `PayloadCsum` in that case, because it will be double-counted and produce an incorrect checksum.
Learnt from: riccardomanfrin
Repo: netbirdio/netbird PR: 6263
File: management/server/types/account.go:1808-1809
Timestamp: 2026-05-26T14:28:54.207Z
Learning: In netbirdio/netbird, development build identifiers must follow the exact convention of starting with the prefix "development" (e.g., "development", "development-<sha>", "development-<sha>-dirty"). Do not treat other strings like "0.50.0-dev" or "dev" as development builds. During code review, use the codebase’s existing check `version.IsDevelopmentVersion()` (which implements `strings.HasPrefix(v, "development")`) to detect development builds, and do not suggest adding legacy compatibility for "-dev" suffix formats.
1 participant
Describe your changes
Diagnostic logging only. Purpose: produce a snapshot
Three grep-able Warn-level log groups (visible at default log level, no trace needed):
ROUTE-DIAG(routemanager/client/client.go) — when a network has no usablerouting peer, logs each candidate peer's connection state (connecting/connected/
idle, relayed, latency), and the moment a routing peer becomes usable. Maps the
DNS-outage-at-connect convergence window (Martin's variant).
WGW-DIAG(peer/wg_watcher.go) — watcher baseline handshake, each check(ok / timeout), and the teardown decision (ctx live → TEARDOWN vs cancelled →
stand down), with a per-instance id to spot overlapping/superseded watchers.
Probes the [client] Fix race between WG watcher initial handshake read and endpoint creation #6626 watcher-rework behaviour (Erik's stuck-loop variant).
PSK-DIAG(peer/conn.go,peer/endpoint.go,rosenpass/netbird_handler.go)— Rosenpass PSK lifecycle: when RP sets the PSK, when RemoveEndpointAddress
drops it, what
presharedKey()returns, and whether WG is configured with aPSK present/absent after reconnect.
Stack
Checklist
Documentation
Select exactly one:
Throwaway diagnostic logging to produce a debug build for customer
troubleshooting. Not intended to merge; no public API / CLI / config change.
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
N/A
Summary by CodeRabbit