Open
Conversation
In netstack (proxy) mode, the process lacks permission to create /var/run/wireguard, making the UAPI listener unnecessary and causing a misleading error log. Introduce NewUSPConfigurerNoUAPI and use it for the netstack device to avoid attempting to open the UAPI socket entirely. Also consolidate UAPI error logging to a single call site.
Contributor
📝 WalkthroughWalkthroughThe changes remove logrus logging from UAPI error handling in the configurer, introduce a new constructor variant that skips UAPI initialization, and update device creation to use this variant. Resource cleanup is improved with explicit socket closure on failure. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
|
shuuri-labs
approved these changes
Feb 19, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
client/iface/device/device_netstack.go (1)
83-89: Pre-existing:t.deviceis not closed onConfigureInterfacefailure.After
tunIface.Close()is called,t.device(which wraps the now-closed tun) is left running. wireguard-go's internal goroutines will attempt further reads from the closed tun and will log errors or panic.t.device.Close()should be called here, beforetunIface.Close()(so the device can drain cleanly), or at minimum immediately after.♻️ Proposed fix
err = t.configurer.ConfigureInterface(t.key, t.port) if err != nil { + t.device.Close() if cErr := tunIface.Close(); cErr != nil { log.Debugf("failed to close tun device: %v", cErr) } return nil, fmt.Errorf("error configuring interface: %s", err) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/iface/device/device_netstack.go` around lines 83 - 89, When ConfigureInterface fails (error returned from t.configurer.ConfigureInterface), ensure the wireguard device is cleanly shut down by calling t.device.Close() before (or at minimum immediately after) closing the underlying tun with tunIface.Close(); update the error-handling block that currently calls tunIface.Close() to first call t.device.Close() (handle and log any error from t.device.Close()), then call tunIface.Close() and return the formatted error from ConfigureInterface so the device's goroutines can drain and won't continue reading a closed tun.client/iface/configurer/usp.go (1)
334-348: Optional: guard Linux socket cleanup behind the UAPI-started condition.When the
NoUAPIconstructor is used on Linux,Close()still issues anos.Statcall for the socket path that was never created. Theos.Statguard keeps this safe, but it performs a syscall unnecessarily and slightly obscures intent. Consider gating the block on whether UAPI was ever started.♻️ Proposed change
func (t *WGUSPConfigurer) Close() { if t.uapiListener != nil { err := t.uapiListener.Close() if err != nil { log.Errorf("failed to close uapi listener: %v", err) } + + if runtime.GOOS == "linux" { + sockPath := "/var/run/wireguard/" + t.deviceName + ".sock" + if _, statErr := os.Stat(sockPath); statErr == nil { + _ = os.Remove(sockPath) + } + } } - - if runtime.GOOS == "linux" { - sockPath := "/var/run/wireguard/" + t.deviceName + ".sock" - if _, statErr := os.Stat(sockPath); statErr == nil { - _ = os.Remove(sockPath) - } - } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/iface/configurer/usp.go` around lines 334 - 348, The Linux socket unlink in WGUSPConfigurer.Close currently runs unconditionally; change it to run only when UAPI was actually started by guarding the socket cleanup on the same condition that indicates UAPI was created (e.g., check t.uapiListener != nil or an explicit flag you add), so the os.Stat/remove syscall is skipped when NoUAPI constructor was used; update the Close method to perform the socket cleanup (using t.deviceName) only when t.uapiListener (or your uapi-started flag) is non-nil/true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@client/iface/configurer/usp.go`:
- Around line 334-348: The Linux socket unlink in WGUSPConfigurer.Close
currently runs unconditionally; change it to run only when UAPI was actually
started by guarding the socket cleanup on the same condition that indicates UAPI
was created (e.g., check t.uapiListener != nil or an explicit flag you add), so
the os.Stat/remove syscall is skipped when NoUAPI constructor was used; update
the Close method to perform the socket cleanup (using t.deviceName) only when
t.uapiListener (or your uapi-started flag) is non-nil/true.
In `@client/iface/device/device_netstack.go`:
- Around line 83-89: When ConfigureInterface fails (error returned from
t.configurer.ConfigureInterface), ensure the wireguard device is cleanly shut
down by calling t.device.Close() before (or at minimum immediately after)
closing the underlying tun with tunIface.Close(); update the error-handling
block that currently calls tunIface.Close() to first call t.device.Close()
(handle and log any error from t.device.Close()), then call tunIface.Close() and
return the formatted error from ConfigureInterface so the device's goroutines
can drain and won't continue reading a closed tun.
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
2 participants
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.
Describe your changes
In netstack (proxy) mode, the process lacks permission to create
/var/run/wireguard, making the UAPI listener unnecessary and causing
a misleading error log. Introduce NewUSPConfigurerNoUAPI and use it
for the netstack device to avoid attempting to open the UAPI socket
entirely. Also consolidate UAPI error logging to a single call site.
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