Feature/embedded stun #5062
Feature/embedded stun #5062
Conversation
📝 WalkthroughWalkthroughAdds an embedded STUN server with multi-UDP listeners, integrates its lifecycle into relay startup/shutdown with new CLI/config flags, updates pion-related dependencies, and adds extensive STUN tests including concurrency, multi-port, and shutdown scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant UDP as UDP Socket
participant STUN as STUN Server
Client->>UDP: send BindingRequest (UDP packet)
UDP->>STUN: readLoop receives packet
STUN->>STUN: decode & validate STUN message
STUN->>STUN: build BindingSuccess (XOR-MAPPED-ADDRESS + Fingerprint)
STUN->>UDP: send response
UDP->>Client: BindingSuccess (XOR-MAPPED-ADDRESS)
sequenceDiagram
participant Relay
participant Listeners as UDP Listeners
participant STUN as STUN Server
Relay->>Listeners: create UDP listeners for configured ports
Relay->>STUN: NewServer(conns, logLevel)
Relay->>STUN: Listen() (goroutine)
STUN->>STUN: spawn readLoop per connection
Relay->>Relay: initiate shutdown sequence
Relay->>STUN: Shutdown()
STUN->>STUN: close conns, wait for wg
STUN-->>Relay: return ErrServerClosed / nil
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 7
🤖 Fix all issues with AI agents
In @stun/server_test.go:
- Around line 193-331: The TestServer_ConcurrentClients test is too heavy for
default CI; modify the TestServer_ConcurrentClients function to skip when
testing.Short() is true (and optionally unless a specific env var like
STUN_CONCURRENT_TEST=1 is set): at the start of TestServer_ConcurrentClients
check if testing.Short() { t.Skip("skipping heavy concurrent test in short
mode") } (or check os.Getenv("STUN_CONCURRENT_TEST") to require an opt-in), so
the existing logic (numClients, requestsPerClient, server setup, goroutines,
asserts) runs only when not short/when the env var is set.
- Around line 391-435: The benchmark currently calls b.Fatal from worker
goroutines inside BenchmarkSTUNServer's b.RunParallel which is unsafe; instead
add sync/atomic (e.g., an int32 errFlag and a variable to hold the first error),
have each worker store the first error atomically and stop further work (break
out of the pb.Next loop) when errFlag is set, avoid calling b.Fatal inside the
goroutine, and after b.RunParallel completes check the stored error and call
b.Fatal once if non-nil; reference the BenchmarkSTUNServer function, the
b.RunParallel call, the pb loop, and the clientConn read/write/error handling to
locate where to apply the change.
In @stun/server.go:
- Around line 59-71: The Listen method on Server currently treats an empty or
nil s.conns as a graceful shutdown by immediately returning ErrServerClosed;
validate s.conns at the start of Server.Listen and return a distinct hard error
when no listeners are configured. In practice, check if len(s.conns) == 0 (or
s.conns == nil) inside Listen and return a new or existing configuration error
(not ErrServerClosed) with a clear message, e.g., "no listeners configured",
before creating the context, setting s.cancel, or starting goroutines; update
any callers/tests that expect ErrServerClosed accordingly.
- Around line 74-95: The readLoop currently continues on every ReadFromUDP error
which causes a busy-loop when the UDP conn is closed externally; update the
error handling in readLoop (the conn.ReadFromUDP error branch) to detect if the
error is a closed connection (use errors.Is(err, net.ErrClosed) or unwrap to
check for net.ErrClosed) and return/stop the loop instead of logging+continuing;
keep the existing ctx.Done check for graceful shutdown and ensure the errors
package is imported if not already.
🧹 Nitpick comments (1)
stun/server.go (1)
33-55: Logger setup is solid; consider whetherServer.logLevelis needed.
Server.logLevelis stored but not used elsewhere; if it’s not intended for future use, it can be dropped to reduce state.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
go.modrelay/cmd/root.gostun/server.gostun/server_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-08T16:23:14.146Z
Learnt from: lixmal
Repo: netbirdio/netbird PR: 4792
File: client/iface/bind/ice_bind.go:34-44
Timestamp: 2026-01-08T16:23:14.146Z
Learning: In client/iface/bind/ice_bind.go, the fallback receiver path in CreateReceiverFn (lines 34-44) is intentionally a minimal stub to maintain interface compatibility with the wireguard-go fork. It does not include STUN filtering, activity recording, or UDP mux initialization because IPv6 is not yet supported in the udpmux, and this path primarily handles non-IPv4 PacketConn cases that aren't fully supported yet.
Applied to files:
stun/server.go
🧬 Code graph analysis (3)
relay/cmd/root.go (1)
stun/server.go (3)
Server(21-28)NewServer(33-55)ErrServerClosed(17-17)
stun/server_test.go (1)
stun/server.go (3)
Server(21-28)NewServer(33-55)ErrServerClosed(17-17)
stun/server.go (1)
formatter/set.go (1)
SetTextFormatter(13-17)
🔇 Additional comments (5)
relay/cmd/root.go (2)
9-27: Imports wiring looks fine.
240-244: STUN shutdown wiring looks good.stun/server.go (2)
150-176: Shutdown approach is good (cancel + close + wait with timeout).
98-147: Go version and pion/stun v3 APIs are compatible.
- The project uses Go 1.24.10 (go.mod), which fully supports
min()introduced in Go 1.21.- All pion/stun v3 APIs used here (
Build(),NewTransactionIDSetter(),XORMappedAddress,Fingerprint) are confirmed present and actively used throughout the codebase in the same patterns.go.mod (1)
79-80: All pion stack versions exist and are compatible. No breaking API changes detected in code or release notes, and no security advisories found for these versions. The version discrepancy in the dependency graph (pion/ice/v4 requires v3.0.0 while the codebase uses v3.1.0) is correctly resolved by Go to the higher version. No action needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
relay/cmd/root.go (2)
48-51: Consider adding validation for STUN configuration.When
EnableSTUNis true, the code will fail at runtime ifSTUNPortsis empty (the loop at lines 203-214 won't execute, andstunListenerswill be empty). Consider adding validation in theConfig.Validate()method to catch this configuration error early.📋 Suggested validation addition
In the
Validate()method (around line 54-62), add:if c.EnableSTUN && len(c.STUNPorts) == 0 { return fmt.Errorf("stun-ports must not be empty when enable-stun is true") }
199-227: LGTM - STUN server startup is properly integrated.The startup logic correctly:
- Creates UDP listeners with appropriate error handling and cleanup
- Instantiates the STUN server with configured log level
- Launches the server in a background goroutine with proper WaitGroup tracking
- Filters expected
ErrServerClosedduring shutdownNote that if the STUN server encounters an unexpected error at runtime (line 224), it's logged but the relay service continues running. This appears intentional and consistent with other auxiliary services, but ensure monitoring is in place to detect STUN failures.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
relay/cmd/root.gostun/server.gostun/server_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- stun/server_test.go
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: lixmal
Repo: netbirdio/netbird PR: 4792
File: client/iface/bind/ice_bind.go:34-44
Timestamp: 2026-01-08T16:23:14.146Z
Learning: In client/iface/bind/ice_bind.go, the fallback receiver path in CreateReceiverFn (lines 34-44) is intentionally a minimal stub to maintain interface compatibility with the wireguard-go fork. It does not include STUN filtering, activity recording, or UDP mux initialization because IPv6 is not yet supported in the udpmux, and this path primarily handles non-IPv4 PacketConn cases that aren't fully supported yet.
📚 Learning: 2026-01-08T16:23:14.146Z
Learnt from: lixmal
Repo: netbirdio/netbird PR: 4792
File: client/iface/bind/ice_bind.go:34-44
Timestamp: 2026-01-08T16:23:14.146Z
Learning: In client/iface/bind/ice_bind.go, the fallback receiver path in CreateReceiverFn (lines 34-44) is intentionally a minimal stub to maintain interface compatibility with the wireguard-go fork. It does not include STUN filtering, activity recording, or UDP mux initialization because IPv6 is not yet supported in the udpmux, and this path primarily handles non-IPv4 PacketConn cases that aren't fully supported yet.
Applied to files:
relay/cmd/root.gostun/server.go
🧬 Code graph analysis (1)
stun/server.go (2)
client/firewall/uspfilter/log/log.go (1)
Level(22-22)formatter/set.go (1)
SetTextFormatter(13-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Darwin
- GitHub Check: Build Cache
- GitHub Check: Linux
- GitHub Check: release
- GitHub Check: Windows
- GitHub Check: release_ui
- GitHub Check: release_ui_darwin
- GitHub Check: iOS / Build
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Check External GPL/AGPL Licenses
🔇 Additional comments (5)
stun/server.go (3)
59-72: LGTM - Listen implementation is correct for single-call usage.The method properly initializes the context, starts read loops for all connections, and blocks until shutdown. Note that Listen is designed to be called once per Server instance (it blocks until shutdown).
154-175: LGTM - Graceful shutdown implementation is correct.The shutdown logic properly:
- Cancels the context to signal readLoops
- Waits for goroutines with timeout
- Returns appropriate errors
The pattern of closing connections before calling
Shutdown(as done inroot.golines 242-246) correctly unblocks the read loops.
75-98: The shutdown sequence design is intentional—listeners are closed first to unblock readLoops before context cancellation. This pattern is explicitly documented in bothrelay/cmd/root.go(line 241 comment) and throughoutstun/server_test.go. The review's suggestion to "reverse the shutdown order" contradicts the deliberate design. Any error logs during the brief window between listener closure andShutdown()invocation are expected and part of graceful termination. No change needed here.The concern about synchronous packet handling is overstated—
handlePacketis fast (validate, parse, respond) with no blocking I/O on the critical path. UDP's connectionless nature means one packet response doesn't block reading subsequent packets. This approach is appropriate for STUN's use case.Likely an incorrect or invalid review comment.
relay/cmd/root.go (2)
100-102: LGTM - STUN CLI flags are well-configured.The flags use appropriate defaults:
- Standard STUN port 3478
- Reasonable log level "info"
IntSliceVarproperly supports multiple port specifications
240-250: LGTM - STUN shutdown is properly integrated.The shutdown sequence correctly:
- Closes listeners to unblock read operations
- Calls
Shutdownto cancel context and wait for goroutines- Aggregates errors using multierror pattern
The shutdown integrates well with the graceful shutdown of other services.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
relay/cmd/root.go (1)
235-246: STUN server error handling may silently swallow unexpected errors.When
stunServer.Listen()returns an error other thanErrServerClosed, it's only logged but doesn't trigger application shutdown. This differs from other servers that calllog.Fatalf. Consider whether unexpected STUN failures should be treated more critically.♻️ Optional: Match error handling pattern with other servers
if stunServer != nil { wg.Add(1) go func() { defer wg.Done() if err := stunServer.Listen(); err != nil { if errors.Is(err, stun.ErrServerClosed) { return } - log.Errorf("STUN server error: %v", err) + log.Fatalf("STUN server error: %v", err) } }() }stun/server.go (1)
57-72: Race condition:s.cancelis written without synchronization.If
Listen()is called concurrently from multiple goroutines, or ifShutdown()is called beforeListen()completes the assignment on line 61, there's a potential race. While the current usage inroot.goappears safe (single call), this could be a latent issue.♻️ Optional: Add synchronization or document single-call constraint
Either document that
Listen()must be called exactly once, or protect thecancelfield:type Server struct { conns []*net.UDPConn logger *log.Entry logLevel log.Level wg sync.WaitGroup + mu sync.Mutex cancel context.CancelFunc }And in
Listen():func (s *Server) Listen() error { + s.mu.Lock() var ctx context.Context ctx, s.cancel = context.WithCancel(context.Background()) + s.mu.Unlock()
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
relay/cmd/root.gostun/server.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-08T16:23:14.146Z
Learnt from: lixmal
Repo: netbirdio/netbird PR: 4792
File: client/iface/bind/ice_bind.go:34-44
Timestamp: 2026-01-08T16:23:14.146Z
Learning: In client/iface/bind/ice_bind.go, the fallback receiver path in CreateReceiverFn (lines 34-44) is intentionally a minimal stub to maintain interface compatibility with the wireguard-go fork. It does not include STUN filtering, activity recording, or UDP mux initialization because IPv6 is not yet supported in the udpmux, and this path primarily handles non-IPv4 PacketConn cases that aren't fully supported yet.
Applied to files:
stun/server.gorelay/cmd/root.go
🧬 Code graph analysis (2)
stun/server.go (2)
client/firewall/uspfilter/log/log.go (1)
Level(22-22)formatter/set.go (1)
SetTextFormatter(13-17)
relay/cmd/root.go (1)
stun/server.go (3)
NewServer(33-55)Server(21-28)ErrServerClosed(17-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Android / Build
- GitHub Check: iOS / Build
- GitHub Check: Client / Unit
- GitHub Check: release_ui
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: Windows
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: release
- GitHub Check: JS / Lint
- GitHub Check: Build Cache
- GitHub Check: release_ui_darwin
- GitHub Check: Linux
- GitHub Check: Client / Unit
- GitHub Check: Check External GPL/AGPL Licenses
🔇 Additional comments (12)
relay/cmd/root.go (6)
48-51: LGTM - New STUN configuration fields.The new STUN configuration fields are well-structured and appropriately placed within the Config struct.
100-102: LGTM - CLI flags for STUN configuration.The CLI flags are well-designed with sensible defaults (port 3478 is the standard STUN port per RFC 5389).
150-165: LGTM - Fail-fast listener creation with proper cleanup.The early listener creation with cleanup of already-opened listeners on failure is a solid pattern for resource management.
179-184: LGTM - Consistent STUN listener cleanup on failure paths.The cleanup of STUN listeners on relay server and healthcheck server creation failures prevents resource leaks.
Also applies to: 193-196
201-204: Consider validatingEnableSTUNflag consistency.The server is created if
len(stunListeners) > 0, but this check is redundant sincestunListenersis only populated whenEnableSTUNis true. However, the current approach is defensive and works correctly.
259-269: Shutdown sequence closes listeners before calling Shutdown - verify this is intentional.The code closes STUN listeners first (lines 261-265), then calls
stunServer.Shutdown(). This is the documented pattern per the Shutdown method comment instun/server.go, but ensure theShutdownmethod handles the case wherecancelmay be nil ifListen()was never called.Verified by examining
stun/server.go:162-164- the nil check ons.cancelhandles this case correctly.stun/server.go (6)
1-14: LGTM - Package structure and imports.Clean package structure with appropriate imports for STUN handling using pion/stun/v3.
16-28: LGTM - Server struct design.The struct appropriately encapsulates the server state with a WaitGroup for goroutine synchronization and a cancel function for graceful shutdown.
33-55: LGTM - Constructor with independent logging.Good design decision to create a separate logger instance, allowing the STUN server's log level to be configured independently from the main application.
74-103: LGTM - readLoop implementation with proper shutdown handling.The read loop correctly:
- Checks context cancellation after blocking reads
- Handles
net.ErrClosedfor external connection closure- Uses a reasonable buffer size (1500 bytes for standard MTU)
The inline packet handling is appropriate for STUN's fast, small responses.
105-155: LGTM - handlePacket correctly implements STUN Binding Request handling.The implementation properly:
- Validates STUN message format
- Decodes and checks message type
- Builds response with XORMappedAddress and Fingerprint
- Logs at appropriate levels
157-180: Shutdown is safe but could log if called before Listen.If
Shutdown()is called beforeListen(), the WaitGroup has nothing to wait for, so it completes immediately. This is correct behavior. The nil check oncancel(line 162) prevents a panic.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
stun/server_test.go (2)
74-77: Unused error channel.
serverErrChis created but never consumed. Consider either removing it or checking the error after shutdown to catch unexpected failures.Suggested simplification
- serverErrCh := make(chan error, 1) go func() { - serverErrCh <- server.Listen() + _ = server.Listen() }()
462-463: Silently ignored errors in hot path.Errors from
stun.BuildandclientConn.Writeare discarded here, unlike the error handling elsewhere in this benchmark. Whilestun.Buildwith these arguments is unlikely to fail, ignoring write errors could mask socket issues.Consistent error handling
- msg, _ := stun.Build(stun.TransactionID, stun.BindingRequest) - _, _ = clientConn.Write(msg.Raw) + msg, err := stun.Build(stun.TransactionID, stun.BindingRequest) + if err != nil { + setErr(err) + return + } + if _, err := clientConn.Write(msg.Raw); err != nil { + setErr(err) + return + }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
stun/server.gostun/server_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- stun/server.go
🧰 Additional context used
🧬 Code graph analysis (1)
stun/server_test.go (1)
stun/server.go (3)
Server(24-31)NewServer(36-58)ErrServerClosed(17-17)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (15)
- GitHub Check: Client / Unit
- GitHub Check: Darwin
- GitHub Check: iOS / Build
- GitHub Check: Windows
- GitHub Check: Build Cache
- GitHub Check: Linux
- GitHub Check: Android / Build
- GitHub Check: Client / Unit
- GitHub Check: release_ui
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: Check External GPL/AGPL Licenses
🔇 Additional comments (8)
stun/server_test.go (8)
1-18: LGTM!Imports are appropriate for the test suite. Using
math/randfor test jitter is fine since Go 1.20+ auto-seeds the global random source.
20-28: LGTM!Clean test helper that creates the server on an ephemeral port. The design correctly leaves lifecycle management to the caller, which is necessary given the specific shutdown ordering required (close listener before shutdown).
30-67: LGTM!Excellent approach to server readiness detection. Polling with actual STUN requests is more robust than fixed sleeps, especially on CI with variable performance characteristics.
127-155: LGTM!Good test for verifying the server ignores non-STUN traffic. The 200ms timeout is reasonable for detecting no-response scenarios.
157-184: LGTM!Well-structured shutdown test. The pattern of using a done channel with a fallback timeout ensures the test won't hang indefinitely while still detecting shutdown failures.
186-225: LGTM!Good sequential stress test that verifies the server handles multiple clients correctly. Each client is properly closed after use.
356-363: Minor:deferplacement after conditional block.The
shutdownCanceldefer on line 361 is placed after some logic, which is fine but slightly unusual. More importantly, ifserveris nil (remote server case), this cleanup block is skipped entirely, which is correct behavior.
366-423: LGTM!Excellent test coverage for the multi-listener functionality. Testing both ports ensures the server correctly handles requests on all configured listeners.
- --enable-stun requires non-empty --stun-ports - Ports must be in range 1-65535 - No duplicate ports allowed
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @relay/cmd/root.go:
- Around line 276-284: The function createHealthCheck currently declares an
unused parameter err that is immediately shadowed by the local err returned from
healthcheck.NewServer; remove the unused err parameter from createHealthCheck's
signature and update all call sites (e.g., the call near line 194) to stop
passing an error argument, keeping the body logic (calling
healthcheck.NewServer, cleanupSTUNListeners on error, logging via log.Debugf,
and returning the formatted error) unchanged; ensure references to
cleanupSTUNListeners and healthcheck.NewServer remain intact.
- Around line 286-294: The createRelayServer function currently accepts an
unused/shadowed err parameter; remove that parameter and change its signature to
createRelayServer(cfg server.Config, stunListeners []*net.UDPConn)
(*server.Server, error), keep the local srv, err := server.NewServer(cfg) logic
and cleanupSTUNListeners/stunListeners handling as-is, and update its call
site(s) (e.g., the caller referenced near line 185) to pass only cfg and
stunListeners (not an error) so there is no parameter shadowing or unused
argument.
In @stun/server.go:
- Around line 161-162: The comment above Shutdown() is incorrect about who
closes UDP connections; update the doc comment for the Server/Shutdown behavior
to state that Shutdown() closes the UDP connections created/passed to NewServer
(i.e., the server takes responsibility for closing them), and remove or rephrase
the sentence "The caller is responsible for closing the UDP connections passed
to NewServer." to accurately reflect that Shutdown() performs the close.
🧹 Nitpick comments (1)
stun/server.go (1)
27-34: Unusedcancelfield in Server struct.The
cancel context.CancelFuncfield is declared but never initialized or used. The server relies on closing connections to stop read loops instead. Either remove the unused field or implement context-based cancellation for cleaner shutdown semantics.♻️ Proposed fix: Remove unused field
type Server struct { conns []*net.UDPConn logger *log.Entry logLevel log.Level - wg sync.WaitGroup - cancel context.CancelFunc + wg sync.WaitGroup }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
relay/cmd/root.gostun/server.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-08T16:23:14.146Z
Learnt from: lixmal
Repo: netbirdio/netbird PR: 4792
File: client/iface/bind/ice_bind.go:34-44
Timestamp: 2026-01-08T16:23:14.146Z
Learning: In client/iface/bind/ice_bind.go, the fallback receiver path in CreateReceiverFn (lines 34-44) is intentionally a minimal stub to maintain interface compatibility with the wireguard-go fork. It does not include STUN filtering, activity recording, or UDP mux initialization because IPv6 is not yet supported in the udpmux, and this path primarily handles non-IPv4 PacketConn cases that aren't fully supported yet.
Applied to files:
relay/cmd/root.gostun/server.go
🧬 Code graph analysis (1)
stun/server.go (2)
formatter/set.go (1)
SetTextFormatter(13-17)client/errors/errors.go (1)
FormatErrorOrNil(25-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client / Unit (386)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Darwin
- GitHub Check: Client / Unit
- GitHub Check: JS / Lint
- GitHub Check: iOS / Build
- GitHub Check: release_ui_darwin
- GitHub Check: release
- GitHub Check: Client / Unit
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: Windows
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: Linux
- GitHub Check: Check External GPL/AGPL Licenses
🔇 Additional comments (10)
stun/server.go (4)
70-84: LGTM - Listen implementation is correct.The method properly validates connections, starts read loops with WaitGroup tracking, and blocks until shutdown. The design assumes single invocation which aligns with the lifecycle pattern in
relay/cmd/root.go.
86-107: LGTM - readLoop is well-implemented.Good handling of
net.ErrClosedfor graceful shutdown, appropriate buffer sizing, and the synchronous packet handling is justified for STUN's small/fast responses.
136-159: LGTM - STUN response handling is correct.The response properly echoes the transaction ID, includes the XOR-mapped address, and adds a fingerprint for integrity. The error handling with appropriate log levels is well done.
117-117: No action required. The project specifies Go 1.24.10 ingo.mod, which fully supports themin()builtin (introduced in Go 1.21).Likely an incorrect or invalid review comment.
relay/cmd/root.go (6)
48-51: LGTM - Config fields for STUN are well-structured.The configuration follows the existing pattern and provides appropriate controls for the embedded STUN server.
61-78: LGTM - Comprehensive STUN configuration validation.The validation properly enforces required ports when enabled, validates the port range, and prevents duplicates.
302-317: LGTM - STUN listener creation with proper cleanup.The function correctly handles partial failure by cleaning up already-opened listeners. Binding to all interfaces (empty IP in UDPAddr) is appropriate for a STUN server.
233-244: LGTM - STUN server startup with graceful error handling.The conditional startup and graceful handling of
ErrServerClosedis correct. Usinglog.Errorfinstead oflog.Fatalfis reasonable for an optional embedded service.
257-261: LGTM - STUN shutdown properly integrated.The shutdown follows the established pattern, aggregating errors with other shutdown errors.
119-121: LGTM - CLI flags follow conventions.The default port 3478 is the standard STUN port per RFC 5389. The IntSlice type appropriately supports multiple ports.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @relay/cmd/root.go:
- Around line 284-302: The helper functions createHealthCheck and
createRelayServer currently declare an unused err parameter that gets shadowed
by local err values; remove the err error parameter from both function
signatures and adjust all call sites to stop passing an error argument (e.g.,
calls that currently pass err should simply call createHealthCheck(hCfg,
stunListeners) and createRelayServer(cfg, stunListeners)); ensure imports and
any references match the new signatures and run tests/build to verify no
remaining references.
🧹 Nitpick comments (1)
relay/cmd/root.go (1)
245-256: Consider error handling consistency for STUN server startup.The STUN server uses
log.Errorfon failure while other servers (metrics, relay, healthcheck) uselog.Fatalf. If STUN is enabled and fails to start, the service continues running in a degraded state without the STUN functionality.This may be intentional (STUN being optional), but consider whether a startup failure should be fatal when
--enable-stunwas explicitly set.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
relay/cmd/root.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-08T16:23:14.146Z
Learnt from: lixmal
Repo: netbirdio/netbird PR: 4792
File: client/iface/bind/ice_bind.go:34-44
Timestamp: 2026-01-08T16:23:14.146Z
Learning: In client/iface/bind/ice_bind.go, the fallback receiver path in CreateReceiverFn (lines 34-44) is intentionally a minimal stub to maintain interface compatibility with the wireguard-go fork. It does not include STUN filtering, activity recording, or UDP mux initialization because IPv6 is not yet supported in the udpmux, and this path primarily handles non-IPv4 PacketConn cases that aren't fully supported yet.
Applied to files:
relay/cmd/root.go
🧬 Code graph analysis (1)
relay/cmd/root.go (1)
stun/server.go (3)
Server(27-34)NewServer(39-65)ErrServerClosed(20-20)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (23)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Signal / Unit (386)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Signal / Unit (amd64)
- GitHub Check: Client / Unit (386)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Relay / Unit (386)
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: Windows
- GitHub Check: Darwin
- GitHub Check: Linux
- GitHub Check: release_ui_darwin
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: release
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: iOS / Build
- GitHub Check: Check External GPL/AGPL Licenses
- GitHub Check: JS / Lint
🔇 Additional comments (6)
relay/cmd/root.go (6)
48-78: LGTM!The STUN configuration fields and validation logic are well-implemented. The validation correctly enforces that ports are required when STUN is enabled, validates port ranges (1-65535), and detects duplicate ports.
119-121: LGTM!The CLI flags follow the existing patterns in the file, and the default STUN port 3478 aligns with RFC 5389.
310-325: LGTM!The STUN listener creation correctly implements fail-fast behavior with proper cleanup of already-opened listeners on failure.
259-282: LGTM!The shutdown logic correctly aggregates errors using
multierrorand follows a sensible order: healthcheck first (stop advertising health), then services, and metrics last (keep observability running longest).
304-308: LGTM!Ignoring close errors is acceptable for cleanup code, as this is best-effort cleanup during error paths.
199-214: LGTM!The STUN server is correctly created only when listeners exist, and the integration with the startup/shutdown lifecycle is well-structured.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @stun/server_test.go:
- Around line 179-216: TestServer_MultipleRequests leaks UDP connections when
net.DialUDP succeeds but subsequent require.NoError triggers a fail; after each
successful DialUDP in the loop (in TestServer_MultipleRequests) ensure
clientConn is always closed by wrapping the per-iteration logic in an anonymous
function and deferring clientConn.Close() immediately after DialUDP (or register
t.Cleanup(func(){ clientConn.Close() }) right after DialUDP) so the connection
is closed on all error/exit paths.
🧹 Nitpick comments (4)
stun/server_test.go (1)
218-253: Consider seedingrandfor reproducibility in concurrent client tests.Using
rand.Int63nwithout seeding relies on the global random source, which may have been seeded elsewhere. For deterministic test runs (especially debugging flakes), consider using a localrand.Randwith a logged seed.stun/server.go (1)
26-32: StoredlogLevelfield is unused.The
logLevelfield is set inNewServerbut never read. Consider removing it if not needed for future use.Suggested fix
type Server struct { conns []*net.UDPConn logger *log.Entry - logLevel log.Level wg sync.WaitGroup }And in
NewServer:return &Server{ conns: conns, logger: logger, - logLevel: level, }relay/cmd/root.go (2)
169-173: STUN listener creation failure doesn't clean up the metrics server.If
createSTUNListeners()fails, themetricsServercreated earlier is not cleaned up. While this may be acceptable since the process exits, for consistency with the fail-fast pattern, consider cleaning up or documenting this behavior.
284-302: STUN listener cleanup is coupled to unrelated failures.Both
createHealthCheckandcreateRelayServercallcleanupSTUNListenerson their own failures. This works but is fragile—if more resources are added, each creation function needs to know about all prior resources.Consider a single cleanup path using
deferor an explicit cleanup function at the call site inexecute().Alternative pattern using deferred cleanup
stunListeners, err := createSTUNListeners() if err != nil { return err } // Cleanup if we fail after this point cleanupOnError := true defer func() { if cleanupOnError { cleanupSTUNListeners(stunListeners) } }() // ... create other resources ... // Success - don't cleanup cleanupOnError = false
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
relay/cmd/root.gostun/server.gostun/server_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-08T16:23:14.146Z
Learnt from: lixmal
Repo: netbirdio/netbird PR: 4792
File: client/iface/bind/ice_bind.go:34-44
Timestamp: 2026-01-08T16:23:14.146Z
Learning: In client/iface/bind/ice_bind.go, the fallback receiver path in CreateReceiverFn (lines 34-44) is intentionally a minimal stub to maintain interface compatibility with the wireguard-go fork. It does not include STUN filtering, activity recording, or UDP mux initialization because IPv6 is not yet supported in the udpmux, and this path primarily handles non-IPv4 PacketConn cases that aren't fully supported yet.
Applied to files:
stun/server.gorelay/cmd/root.go
🧬 Code graph analysis (1)
stun/server.go (2)
formatter/set.go (1)
SetTextFormatter(13-17)client/errors/errors.go (1)
FormatErrorOrNil(25-30)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (21)
- GitHub Check: Management / Unit (amd64, mysql)
- GitHub Check: Client (Docker) / Unit
- GitHub Check: Client / Unit (amd64)
- GitHub Check: Relay / Unit (amd64, -race)
- GitHub Check: Relay / Unit (386)
- GitHub Check: Management / Unit (amd64, sqlite)
- GitHub Check: Management / Unit (amd64, postgres)
- GitHub Check: Client / Unit (386)
- GitHub Check: release
- GitHub Check: release_ui_darwin
- GitHub Check: FreeBSD Port / Build & Test
- GitHub Check: release_ui
- GitHub Check: Client / Unit
- GitHub Check: Client / Unit
- GitHub Check: Android / Build
- GitHub Check: JS / Lint
- GitHub Check: Darwin
- GitHub Check: Windows
- GitHub Check: Client / Unit
- GitHub Check: Linux
- GitHub Check: Check External GPL/AGPL Licenses
🔇 Additional comments (13)
stun/server_test.go (4)
19-27: LGTM! Clean helper function design.The helper properly uses
testing.TBto support both tests and benchmarks, creates an ephemeral UDP listener, and returns all necessary resources for the caller to manage lifecycle.
29-66: Well-designed readiness polling to avoid flaky tests.Using active polling with STUN requests is more reliable than
time.Sleep. The short retry interval (10ms) and configurable timeout make this robust for CI environments.
355-410: LGTM! Multi-port test validates core functionality.The test correctly verifies that a single server instance can serve multiple UDP listeners and that XOR-MAPPED-ADDRESS reflects the correct client port.
412-477: Benchmark structure is sound with proper atomic error handling.The use of
atomic.Pointerfor first-error signaling prevents callingb.Fatalfrom worker goroutines (which would cause a panic), and the early exit on error avoids wasted work.stun/server.go (4)
61-78:Listen()always returnsErrServerClosed, even on normal exit.This is intentional based on the design—the only way to exit
Listen()is viaShutdown(). The tests confirm this expectation. The naming is clear.
80-101: Read loop handles connection closure correctly.The loop properly checks for
net.ErrClosedto distinguish graceful shutdown from actual errors, avoiding spurious warnings during normal shutdown.
103-153: LGTM! Packet handling is clean and safe.The handler correctly validates STUN messages, ignores non-binding requests with debug logs, and includes proper error logging for failures. The inline handling (no goroutine spawn) is appropriate for the lightweight STUN response workflow.
155-170: Potential double-close ifShutdown()is called when listeners are already closed externally.The tests explicitly close listeners before calling
Shutdown(). WhileShutdown()handlesnet.ErrClosedgracefully (line 162), this pattern of external closure before shutdown is unconventional. Consider documenting this expectation or havingShutdown()be the sole owner of closing connections.The current test pattern (close listener, then shutdown) works, but if callers don't follow this pattern,
Shutdown()will attempt to close already-open connections—which is fine. However, if this is the intended API, the doc comment should clarify whether callers should close listeners before or after callingShutdown().relay/cmd/root.go (5)
48-52: LGTM! Clean addition of STUN configuration fields.The new fields are logically grouped and documented with a comment.
61-78: Thorough STUN port validation.Good coverage: requires ports when enabled, validates range, detects duplicates. This prevents runtime failures during listener creation.
245-256: STUN server startup logs error but doesn't trigger shutdown.Unlike other servers that use
log.Fatalf, the STUN server only logs an error on failure. This inconsistency means the relay continues running even if STUN fails to start properly (beyond the expectedErrServerClosed). Consider whether STUN failure should be fatal.Is this intentional? If STUN is an optional enhancement, non-fatal logging makes sense. If STUN availability is critical when
--enable-stunis set,log.Fatalfwould be more consistent with other servers.
259-282: LGTM! Shutdown aggregates errors properly.Using
multierror.Appendto collect all shutdown errors before returning allows all servers to attempt graceful shutdown even if one fails.
310-325: LGTM! STUN listener creation with proper cleanup on partial failure.If any listener fails to bind, previously opened listeners are properly closed before returning the error.
| func TestServer_MultipleRequests(t *testing.T) { | ||
| server, listener, serverAddr := createTestServer(t) | ||
|
|
||
| go func() { | ||
| _ = server.Listen() | ||
| }() | ||
|
|
||
| waitForServerReady(t, serverAddr, 2*time.Second) | ||
|
|
||
| // Create multiple clients and send requests | ||
| for i := 0; i < 5; i++ { | ||
| clientConn, err := net.DialUDP("udp", nil, serverAddr) | ||
| require.NoError(t, err) | ||
|
|
||
| msg, err := stun.Build(stun.TransactionID, stun.BindingRequest) | ||
| require.NoError(t, err) | ||
|
|
||
| _, err = clientConn.Write(msg.Raw) | ||
| require.NoError(t, err) | ||
|
|
||
| buf := make([]byte, 1500) | ||
| _ = clientConn.SetReadDeadline(time.Now().Add(2 * time.Second)) | ||
| n, err := clientConn.Read(buf) | ||
| require.NoError(t, err) | ||
|
|
||
| response := &stun.Message{Raw: buf[:n]} | ||
| err = response.Decode() | ||
| require.NoError(t, err) | ||
|
|
||
| assert.Equal(t, stun.BindingSuccess, response.Type) | ||
|
|
||
| clientConn.Close() | ||
| } | ||
|
|
||
| // Close listener first to unblock readLoop, then shutdown | ||
| _ = listener.Close() | ||
| _ = server.Shutdown() | ||
| } |
There was a problem hiding this comment.
Resource leak: client connections not closed on error.
If require.NoError fails after net.DialUDP succeeds, the connection leaks. Consider using defer or restructuring.
Suggested fix
for i := 0; i < 5; i++ {
clientConn, err := net.DialUDP("udp", nil, serverAddr)
require.NoError(t, err)
+ defer clientConn.Close()
msg, err := stun.Build(stun.TransactionID, stun.BindingRequest)
require.NoError(t, err)
_, err = clientConn.Write(msg.Raw)
require.NoError(t, err)
buf := make([]byte, 1500)
_ = clientConn.SetReadDeadline(time.Now().Add(2 * time.Second))
n, err := clientConn.Read(buf)
require.NoError(t, err)
response := &stun.Message{Raw: buf[:n]}
err = response.Decode()
require.NoError(t, err)
assert.Equal(t, stun.BindingSuccess, response.Type)
-
- clientConn.Close()
}📝 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.
| func TestServer_MultipleRequests(t *testing.T) { | |
| server, listener, serverAddr := createTestServer(t) | |
| go func() { | |
| _ = server.Listen() | |
| }() | |
| waitForServerReady(t, serverAddr, 2*time.Second) | |
| // Create multiple clients and send requests | |
| for i := 0; i < 5; i++ { | |
| clientConn, err := net.DialUDP("udp", nil, serverAddr) | |
| require.NoError(t, err) | |
| msg, err := stun.Build(stun.TransactionID, stun.BindingRequest) | |
| require.NoError(t, err) | |
| _, err = clientConn.Write(msg.Raw) | |
| require.NoError(t, err) | |
| buf := make([]byte, 1500) | |
| _ = clientConn.SetReadDeadline(time.Now().Add(2 * time.Second)) | |
| n, err := clientConn.Read(buf) | |
| require.NoError(t, err) | |
| response := &stun.Message{Raw: buf[:n]} | |
| err = response.Decode() | |
| require.NoError(t, err) | |
| assert.Equal(t, stun.BindingSuccess, response.Type) | |
| clientConn.Close() | |
| } | |
| // Close listener first to unblock readLoop, then shutdown | |
| _ = listener.Close() | |
| _ = server.Shutdown() | |
| } | |
| func TestServer_MultipleRequests(t *testing.T) { | |
| server, listener, serverAddr := createTestServer(t) | |
| go func() { | |
| _ = server.Listen() | |
| }() | |
| waitForServerReady(t, serverAddr, 2*time.Second) | |
| // Create multiple clients and send requests | |
| for i := 0; i < 5; i++ { | |
| clientConn, err := net.DialUDP("udp", nil, serverAddr) | |
| require.NoError(t, err) | |
| defer clientConn.Close() | |
| msg, err := stun.Build(stun.TransactionID, stun.BindingRequest) | |
| require.NoError(t, err) | |
| _, err = clientConn.Write(msg.Raw) | |
| require.NoError(t, err) | |
| buf := make([]byte, 1500) | |
| _ = clientConn.SetReadDeadline(time.Now().Add(2 * time.Second)) | |
| n, err := clientConn.Read(buf) | |
| require.NoError(t, err) | |
| response := &stun.Message{Raw: buf[:n]} | |
| err = response.Decode() | |
| require.NoError(t, err) | |
| assert.Equal(t, stun.BindingSuccess, response.Type) | |
| } | |
| // Close listener first to unblock readLoop, then shutdown | |
| _ = listener.Close() | |
| _ = server.Shutdown() | |
| } |
🤖 Prompt for AI Agents
In @stun/server_test.go around lines 179 - 216, TestServer_MultipleRequests
leaks UDP connections when net.DialUDP succeeds but subsequent require.NoError
triggers a fail; after each successful DialUDP in the loop (in
TestServer_MultipleRequests) ensure clientConn is always closed by wrapping the
per-iteration logic in an anonymous function and deferring clientConn.Close()
immediately after DialUDP (or register t.Cleanup(func(){ clientConn.Close() })
right after DialUDP) so the connection is closed on all error/exit paths.
| } | ||
|
|
||
| srv, err := server.NewServer(cfg) | ||
| srv, err := createRelayServer(cfg, stunListeners) |
There was a problem hiding this comment.
Do not pass stunListeners. In case of error clean up listeners in this level.
| } | ||
| httpHealthcheck, err := createHealthCheck(hCfg, stunListeners) | ||
| if err != nil { | ||
| return err |
Describe your changes
Issue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
Chores
Tests
✏️ Tip: You can customize this high-level summary in your review settings.