mirror of
https://github.com/netbirdio/netbird.git
synced 2026-10-06 13:39:07 +02:00
09b19595f13dc5008aaf0394cc81effc3cb3ea9c
10
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Riccardo Manfrin
|
9f8ddc7131 |
[client] Discover interfaces lazily in stdnet instead of at construction (#7346)
* [client] Discover interfaces lazily in stdnet instead of at construction
stdnet.NewNet and NewNetWithDiscover ended with
return n, n.UpdateInterfaces()
handing back a non-nil *Net together with the discovery error. Three of the
five call sites (Engine.newWgIface, ice.NewAgent, SingleSocketUDPMux) logged
the error and kept using the instance, which is only safe as long as the
instance still works after a failed discovery.
That stopped being true when Interfaces() gained a lazily refreshed cache:
updateInterfaces sets lastUpdate only on success, so after a failed
construction the 30s cache guard never holds and Interfaces() returns an
error rather than the empty list it used to return. Feeding such an instance
to pion is worse than passing nothing at all - ice.NewAgent falls back to its
own stdnet when Net is nil, and the interface blacklist is applied separately
through AgentConfig.InterfaceFilter, so the fallback loses nothing. Instead,
a transient discovery failure (the Android bridge at boot, or an interface
disappearing between net.Interfaces() and Interface.Addrs()) turned into a
hard "error getting local interfaces" from ice.NewAgent, and aborted the STUN
and TURN probes, which never even need the interface list.
Since the accessors already refresh a stale cache on demand, the eager
discovery in the constructors is redundant: drop it, make both constructors
infallible, and let the discovery error surface at the call that actually
needs the interfaces. UpdateInterfaces had no callers left and is not part of
transport.Net, so it is removed along with it.
InterfaceByIndex and InterfaceByName read the cached slice directly and never
refreshed it, so they would have kept reporting ErrInterfaceNotFound forever
on an instance whose first discovery failed. They now go through the same
refresh path as Interfaces().
* [client] Warm the stdnet interface cache at construction
Moving discovery to first use regressed the privileged suites on the three
platforms that always build an ICE bind: Darwin, FreeBSD and Windows time out
in TestWGIface_UpdateAddr, TestRecreation, TestEngine_SSH and
TestEngine_MultiplePeers, while Linux stays green because a host with the
WireGuard kernel module takes the kernel-device branch and never drives the
mux that asks for interfaces.
interfaceFilter probes with wgctrl every interface the disallow list does not
already exclude. Discovering at construction ran that probe before the caller
had an overlay interface of its own; discovering at first use runs it after,
so on a userspace WireGuard platform the probe reaches the UAPI socket of the
same process. The tests reach it because they construct with a nil disallow
list, where the client passes DefaultInterfaceBlacklist and its own interface
is excluded by prefix.
Restore the original timing with an explicit warm-up. The constructors stay
infallible and the error is still reported by the accessor that needs the
interfaces, so the contract this branch is about is unchanged.
* Revert "[client] Warm the stdnet interface cache at construction"
This reverts commit
|
||
|
Zoltan Papp
|
5ceca6e500 |
[client] Report both peers' state when the connect test times out (#7944)
Test_ConnectPeers fails every few weeks on the Linux runner with a bare "waiting for peer handshake timeout after 30s". The failing logs show both kernel devices up and both peers configured within a second, then nothing for 30 s, which is six retries of the 5 s handshake retransmit and so a condition that lasted the whole window rather than a race. The failure cannot be reproduced locally and the log cannot tell whether initiations were sent, whether they arrived, or whether only one direction worked. On timeout the test now prints each device's view of its peer, the endpoint, the byte counters and the last handshake, so the next failure says which of those it is. The comment also states that the peers are kernel devices on the runner and that the first initiation of each side is always lost to the other side not knowing the peer yet. |
||
|
Riccardo Manfrin
|
1e5b0a5c89 |
[client] Make Test_ConnectPeers deterministic under Docker/eBPF kernel / Darwin CI (#6884)
## Describe your changes Make `Test_ConnectPeers` deterministic. Two issues, both surfaced once the privileged suite moved into a `--privileged` Docker container (#6425): 1. The peers used `getLocalIP()` as their WireGuard endpoint, i.e. the host's routable NIC IP (the docker bridge IP `172.17.0.2` in CI). That address might not hairpin reliably inside the container, so the handshake intermittently timed out (flaky). Use loopback (`127.1.0.x`) instead — always self-reachable. 2. On a Linux runner with the WG kernel module the iface uses the eBPF proxy factory. Its manager is a singleton with one shared XDP program + settings map, so bringing up the two ifaces makes the second factory overwrite the first's `wg_port`/`proxy_port` and the handshake is dropped. The test is incompatible with the eBPF factory, so disable it via `NB_DISABLE_EBPF_WG_PROXY` (peers then handshake directly over loopback). Running the suite across all three modes (eBPF / UDP proxy / ICE bind) would need a larger refactor. Also fixes a typo in the `ErrSharedSockStopped` message (`socked` → `socket`). ## Issue ticket number and link No public issue — CI flakiness follow-up to #6871 on `Test_ConnectPeers` (https://github.com/netbirdio/netbird/blob/main/client/iface/iface_test.go). ## Stack <!-- branch-stack --> ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [ ] Created tests that fail without the change (if possible) > By submitting this pull request, you confirm that you have read and agree to the terms of the [Contributor License Agreement](https://github.com/netbirdio/netbird/blob/main/CONTRIBUTOR_LICENSE_AGREEMENT.md). ## Documentation Select exactly one: - [ ] I added/updated documentation for this change - [x] Documentation is **not needed** for this change (explain why) Test-only change (plus a log-string typo). No public API, CLI, config, or behavior change. ### Docs PR URL (required if "docs added" is checked) Paste the PR link from https://github.com/netbirdio/docs here: N/A <!-- codesmith:footer --> --- <a href="https://app.blacksmith.sh/netbirdio/codesmith/netbird/pr/6884"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v2.svg"><img alt="View with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"></picture></a> <a href="https://backend.blacksmith.sh/track/enable-autofix?expires=1787485967&installation_model_id=427504&pr_number=6884&repository=netbirdio%2Fnetbird&return_to=https%3A%2F%2Fgithub.com%2Fnetbirdio%2Fnetbird%2Fpull%2F6884&signature=09fb996715d039bd02775a140e8cc03deca5cb42540790b82a744527264a30ad"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img alt="Autofix with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"></picture></a> <sup>Need help on this PR? Tag <code>@codesmith-bot</code> with what you need. Autofix is disabled.</sup> <!-- codesmith:autofix:disabled --> <!-- /codesmith:footer --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Corrected the “shared socket stopped” error message for clearer output. * **Tests** * Improved peer connection test reliability in privileged CI by disabling the eBPF WireGuard proxy and using fixed loopback UDP endpoints for deterministic setup. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Riccardo Manfrin
|
3358138ccc |
[client] Fix flaky Test_ConnectPeers busy-loop handshake wait (#6871)
## Describe your changes `Test_ConnectPeers` in `client/iface` is flaky in CI, timing out with `waiting for peer handshake timeout after 30s`. The wait loop polled `getPeer()` in a tight busy-loop with no sleep (a `select` with a `default` branch), pegging a CPU core. The peers run userspace WireGuard (stdnet transport), so the spin starved the wireguard-go goroutines that actually process the handshake, making the 30s wait flaky under CI load. Poll on a 500ms ticker instead so the CPU is yielded between checks, and check the handshake state before waiting. Same logic, no busy-spin. ## Issue ticket number and link N/A — CI flakiness fix. ### Checklist - [x] Is it a bug fix - [ ] Is a typo/documentation fix - [ ] Is a feature enhancement - [ ] It is a refactor - [ ] Created tests that fail without the change (if possible) - [x] This change does **not** modify the public API, gRPC protocols, functionality behavior, CLI / service flags, or introduce a new feature ## Documentation - [x] Documentation is **not needed** for this change (test-only fix) <!-- codesmith:footer --> --- <a href="https://app.blacksmith.sh/netbirdio/codesmith/netbird/pr/6871"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-light-v2.svg"><img alt="View with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/view-with-codesmith-dark-v2.svg"></picture></a> <a href="https://backend.blacksmith.sh/track/enable-autofix?expires=1787405146&installation_model_id=427504&pr_number=6871&repository=netbirdio%2Fnetbird&return_to=https%3A%2F%2Fgithub.com%2Fnetbirdio%2Fnetbird%2Fpull%2F6871&signature=a68daf1747583280b5d1d083c6b45ade4c0d441a38fd25ab9a2ca01f87511ad9"><picture><source media="(prefers-color-scheme: dark)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"><source media="(prefers-color-scheme: light)" srcset="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-light.svg"><img alt="Autofix with [code]smith" src="https://pr-comments-assets.blacksmith.sh/codesmith/autofix-with-codesmith-dark.svg"></picture></a> <sup>Need help on this PR? Tag <code>@codesmith-bot</code> with what you need. Autofix is disabled.</sup> <!-- codesmith:autofix:disabled --> <!-- /codesmith:footer --> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Improved peer connection test waiting behavior to avoid busy spinning. * Added a timeout and periodic checks, with clearer failure handling when a handshake does not complete. <!-- end of auto-generated comment: release notes by coderabbit.ai --> |
||
|
Zoltan Pappandcoderabbitai[bot]
|
2d7b309004 |
[client] Categorize privileged tests behind a build tag and run them in Docker (#6425)
* [client] categorize root/system-mutating tests behind a privileged build tag Tests that need root or mutate host state (nftables/iptables/DNS, TUN/WireGuard interfaces, routes, eBPF, SSH/service install) are now gated behind a //go:build privileged tag. The default `go test ./client/...` runs as a non-root user with no sudo and leaves host networking untouched; mixed files were split so pure-logic tests stay in the default suite. A self-hosting ory/dockertest/v4 harness (client/testutil/privileged) runs the privileged suite inside a --privileged --cap-add=NET_ADMIN container via `make test-privileged`; a DOCKER_CI=true guard skips the spawn when already inside the container. Added `make test-unit` for the host-safe run. * [client] add PRIV_RUN/PRIV_PKGS filters to the privileged test harness The dockertest harness now reads two optional env vars when building the in-container `go test` command: PRIV_RUN adds a -run test-name filter and PRIV_PKGS overrides the package list. Both empty reproduce the full privileged suite, so CI and `make test-privileged` behave as before. Lets a developer run a single privileged test in the container, e.g.: PRIV_RUN=TestNftablesManager PRIV_PKGS=./client/firewall/nftables/... make test-privileged * [client] fix unused-helper lint after the privileged test split Splitting privileged tests into *_privileged_test.go left their shared helpers in the untagged files, so in the default (no-tag) build they had no callers and golangci-lint flagged them as unused. Moved the privileged-only helpers into the privileged files next to their callers (generateDummyHandler; createEngine/startSignal/startManagement/getConnectedPeers/ getPeers + kaep/kasp; (*mockDaemon).setJWTToken). Annotated the shared routing-test fixtures that must stay untagged for cross-platform compilation with //nolint:unused (systemops_bsd expected* vars, ensureIPv6DefaultRoute on bsd/windows, loopbackIfaceWindows), matching the existing linux variant. * [client] fix privileged test CI failures and run the harness on macOS The host-safe unit run dropped sudo but two privileged test groups were never tagged, and the Docker privileged job silently never ran the suite: - Gate the ssh/server PrivilegeDropper command-construction tests behind the privileged tag (they require root to target a different UID); split them into executor_unix_privileged_test.go. - Tag sharedsock raw-socket tests privileged (need CAP_NET_RAW). - Fix the Docker job command: nested single quotes around the build tags closed the sh -c wrapper early, dropping the go list package set and the privileged tag, so go test ran on the empty repo root. Use double quotes. Make the self-hosting harness usable from a dev Mac: - Build it on darwin as well as linux; it only drives Docker. - Resolve the active docker context endpoint into DOCKER_HOST when the default /var/run/docker.sock is absent (Docker Desktop, Colima, OrbStack). - Rename the misspelled containerGoModache constant to containerGoModCache. * Update client/internal/engine_privileged_test.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update client/internal/routemanager/systemops/systemops_linux_test.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update client/internal/routemanager/systemops/systemops_windows_test.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * Update client/server/server_privileged_test.go Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> * [ci] Run privileged-tagged tests on darwin, windows and freebsd The privileged build tag split moved root/system-mutating tests behind //go:build privileged, but only the linux docker job was given the tag. The native darwin (sudo), windows (PsExec64 -s) and freebsd VM runners already have the required privileges, so add the privileged tag there too to keep CI running the same set of tests as before the split. * [ci] Exclude dockertest harness from the darwin privileged run The privileged tag now compiles client/testutil/privileged on darwin, whose TestRunPrivilegedSuiteInDocker spawns a container the macOS runner has no Docker for. Exclude the harness package from the darwin list, matching the linux job, so the privileged tests run in place without a container spawn. --------- Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com> |
||
|
Viktor Liu
|
205ebcfda2 | [management, client] Add IPv6 overlay support (#5631) | ||
|
Viktor Liu
|
9cc9462cd5 | [client] Use stdnet with a context to avoid DNS deadlocks (#4781) | ||
|
Zoltan PappandHakan Sariman
|
0819df916e |
[client] Replace string to netip.Prefix (#3362)
Replace string to netip.Prefix --------- Co-authored-by: Hakan Sariman <hknsrmn46@gmail.com> |
||
|
Zoltan Papp
|
30ebcf38c7 |
[client] Eliminate UDP proxy in user-space mode (#2712)
In the case of user space WireGuard mode, use in-memory proxy between the TURN/Relay connection and the WireGuard Bind. We keep the UDP proxy and eBPF proxy for kernel mode. The key change is the new wgproxy/bind and the iface/bind/ice_bind changes. Everything else is just to fulfill the dependencies. |
||
|
Zoltan Papp
|
fd67892cb4 |
[client] Refactor/iface pkg (#2646)
Refactor the flat code structure |
