From e704203927fcd40ee2b4687a134d513c3909f3bf Mon Sep 17 00:00:00 2001 From: dmitri-netbird Date: Thu, 10 Sep 2026 20:05:42 +0200 Subject: [PATCH 01/15] [management] do not hard-code tmp dir path in ws_conn_adapter_test (#7503) * do not hard-code tmp dir path Signed-off-by: Dmitri Dolguikh * use os.TempDir to get tmp dir Signed-off-by: Dmitri Dolguikh --------- Signed-off-by: Dmitri Dolguikh --- util/wsproxy/server/ws_conn_adapter_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/util/wsproxy/server/ws_conn_adapter_test.go b/util/wsproxy/server/ws_conn_adapter_test.go index 5369b2362..d46e4830b 100644 --- a/util/wsproxy/server/ws_conn_adapter_test.go +++ b/util/wsproxy/server/ws_conn_adapter_test.go @@ -34,7 +34,7 @@ func TestAdapterHandlingConnectionClosures(t *testing.T) { for _, c := range cases { t.Run(c.description, func(t *testing.T) { - serversock := filepath.Join("/tmp", "http-server-"+strconv.FormatInt(rand.Int64(), 10)+".sock") + serversock := filepath.Join(os.TempDir(), "http-server-"+strconv.FormatInt(rand.Int64(), 10)+".sock") t.Cleanup(func() { os.Remove(serversock) }) l, err := net.Listen("unix", serversock) @@ -111,7 +111,7 @@ func TestAdapterHandlingConnectionClosures(t *testing.T) { func TestAdapterHandlingHttpConnection_NoHeadersSent(t *testing.T) { t.Skip("currently disabled as it requires idle timeout to be set") - serversock := filepath.Join("/tmp", "http-server-"+strconv.FormatInt(rand.Int64(), 10)+".sock") + serversock := filepath.Join(os.TempDir(), "http-server-"+strconv.FormatInt(rand.Int64(), 10)+".sock") defer os.Remove(serversock) l, err := net.Listen("unix", serversock) From 2f48dbea6ae4d07411e37283008d63534803e37a Mon Sep 17 00:00:00 2001 From: Nicolas Frati Date: Thu, 10 Sep 2026 21:41:59 +0200 Subject: [PATCH 02/15] [client] Add a release-wired rootless UBI image variant (#7469) * [client] Add a release-wired rootless UBI image variant * [client] Add ARM64 to the rootless UBI image * [client] Express license output validation as a guard --- .goreleaser.yaml | 37 ++++++++++++++++ client/Dockerfile-rootless.ubi | 45 ++++++++++++++++++++ client/collect-licenses.sh | 77 ++++++++++++++++++++++++++++++++++ 3 files changed, 159 insertions(+) create mode 100644 client/Dockerfile-rootless.ubi create mode 100644 client/collect-licenses.sh diff --git a/.goreleaser.yaml b/.goreleaser.yaml index c5d260376..778ccb892 100644 --- a/.goreleaser.yaml +++ b/.goreleaser.yaml @@ -289,6 +289,43 @@ dockers_v2: "org.opencontainers.image.revision": "{{.FullCommit}}" "org.opencontainers.image.source": "{{.GitURL}}" "maintainer": "dev@netbird.io" + - id: netbird-rootless-ubi + disable: "{{ .Env.SKIP_DOCKER_PUSH }}" + ids: + - netbird + images: + - netbirdio/netbird + - ghcr.io/netbirdio/netbird + tags: + - "{{ .Version }}-rootless-ubi" + - "{{ if eq .Env.SKIP_PUBLISH \"false\" }}rootless-ubi-latest{{ end }}" + dockerfile: client/Dockerfile-rootless.ubi + extra_files: + - client/netbird-entrypoint.sh + platforms: + - linux/amd64 + - linux/arm64 + build_args: + VERSION: "{{ .Version }}" + RELEASE: "{{ .Timestamp }}" + hooks: + pre: + - cmd: 'sh client/collect-licenses.sh "{{ .ContextDir }}/licenses" amd64 arm64' + env: + - GOOS=linux + - CGO_ENABLED=0 + labels: + "org.opencontainers.image.created": "{{.Date}}" + "org.opencontainers.image.version": "{{.Version}}" + "org.opencontainers.image.revision": "{{.FullCommit}}" + "org.opencontainers.image.source": "{{.GitURL}}" + annotations: + "org.opencontainers.image.created": "{{.Date}}" + "org.opencontainers.image.title": "{{.ProjectName}}" + "org.opencontainers.image.version": "{{.Version}}" + "org.opencontainers.image.revision": "{{.FullCommit}}" + "org.opencontainers.image.source": "{{.GitURL}}" + "maintainer": "dev@netbird.io" - id: relay disable: "{{ .Env.SKIP_DOCKER_PUSH }}" ids: diff --git a/client/Dockerfile-rootless.ubi b/client/Dockerfile-rootless.ubi new file mode 100644 index 000000000..4701728c1 --- /dev/null +++ b/client/Dockerfile-rootless.ubi @@ -0,0 +1,45 @@ +FROM registry.access.redhat.com/ubi9/ubi-minimal@sha256:7fbeae18dc9476399f565e68255f602a3374ea8614ba3d14843565131a13ff93 + +ARG TARGETPLATFORM +ARG NETBIRD_BINARY=$TARGETPLATFORM/netbird +ARG VERSION=dev +ARG RELEASE=1 + +LABEL name="netbird-rootless" \ + maintainer="NetBird " \ + vendor="NetBird GmbH" \ + version="${VERSION}" \ + release="${RELEASE}" \ + summary="NetBird Rootless Client" \ + description="NetBird connects devices through an encrypted overlay using userspace networking without a TUN device or network administration capabilities." + +RUN microdnf install -y bash ca-certificates && microdnf clean all + +COPY --chmod=0555 client/netbird-entrypoint.sh /usr/local/bin/netbird-entrypoint.sh +COPY --chmod=0555 ${NETBIRD_BINARY} /usr/local/bin/netbird +COPY licenses/ /licenses/ +# Only application storage is group-writable for arbitrary non-root UIDs. +# Runtime-created credentials keep the client's restrictive file modes. +RUN mkdir -p /var/lib/netbird && \ + chown 1000:0 /var/lib/netbird && \ + chmod 0770 /var/lib/netbird && \ + chmod -R a+rX /licenses + +WORKDIR /var/lib/netbird +USER 1000:0 + +ENV \ + HOME="/var/lib/netbird" \ + NETBIRD_BIN="/usr/local/bin/netbird" \ + NB_USE_NETSTACK_MODE="true" \ + NB_ENABLE_NETSTACK_LOCAL_FORWARDING="true" \ + NB_CONFIG="/var/lib/netbird/config.json" \ + NB_STATE_DIR="/var/lib/netbird" \ + NB_DAEMON_ADDR="unix:///var/lib/netbird/netbird.sock" \ + NB_LOG_FILE="console,/var/lib/netbird/client.log" \ + NB_DISABLE_DNS="true" \ + NB_ENABLE_CAPTURE="false" \ + NB_ENTRYPOINT_SERVICE_TIMEOUT="30" + +STOPSIGNAL SIGTERM +ENTRYPOINT ["/usr/local/bin/netbird-entrypoint.sh"] diff --git a/client/collect-licenses.sh b/client/collect-licenses.sh new file mode 100644 index 000000000..7dfabada9 --- /dev/null +++ b/client/collect-licenses.sh @@ -0,0 +1,77 @@ +#!/bin/sh +set -eu + +if [ "$#" -lt 2 ]; then + printf '%s\n' "usage: $0 OUTPUT_DIRECTORY GOARCH..." >&2 + exit 2 +fi + +repo_root=$(CDPATH= cd -- "$(dirname "$0")/.." && pwd) +output_name=$(basename "$1") +if [ -z "$output_name" ] || [ "$output_name" = "." ] || + [ "$output_name" = ".." ] || [ "$output_name" = "/" ]; then + printf '%s\n' "OUTPUT_DIRECTORY must name a directory" >&2 + exit 2 +fi +output_parent=$(CDPATH= cd -- "$(dirname "$1")" && pwd) +output="$output_parent/$output_name" +shift +modules=$(mktemp "${TMPDIR:-/tmp}/netbird-client-licenses.modules.XXXXXX") +sorted_modules=$(mktemp "${TMPDIR:-/tmp}/netbird-client-licenses.sorted.XXXXXX") +trap 'rm -f "$modules" "$sorted_modules"' EXIT HUP INT TERM + +if [ -e "$output" ] || [ -L "$output" ]; then + printf 'output directory already exists: %s\n' "$output" >&2 + exit 1 +fi +mkdir "$output" +mkdir "$output/third_party" + +cp "$repo_root/LICENSE" "$output/BSD-3-Clause.txt" + +cd "$repo_root" +for arch in "$@"; do + GOOS=${GOOS:-linux} GOARCH="$arch" CGO_ENABLED=${CGO_ENABLED:-0} \ + go list -deps -f '{{with .Module}}{{if .Replace}}{{.Replace.Path}}{{"\t"}}{{.Replace.Version}}{{"\t"}}{{.Replace.Dir}}{{else}}{{.Path}}{{"\t"}}{{.Version}}{{"\t"}}{{.Dir}}{{end}}{{end}}' -tags load_wgnt_from_rsrc ./client >>"$modules" +done +LC_ALL=C sort -u "$modules" >"$sorted_modules" + +goroot=$(go env GOROOT) +for term in LICENSE PATENTS; do + if [ ! -f "$goroot/$term" ]; then + printf 'missing Go standard-library term: %s\n' "$goroot/$term" >&2 + exit 1 + fi + cp "$goroot/$term" "$output/Go-$term" +done + +while IFS=' ' read -r module version module_dir; do + [ -n "$module" ] || continue + [ "$module" = "github.com/netbirdio/netbird" ] && continue + + if [ -z "$version" ] || [ ! -d "$module_dir" ]; then + printf 'cannot collect terms for module %s at version %s\n' "$module" "$version" >&2 + exit 1 + fi + + destination="$output/third_party/$module/$version" + mkdir -p "$destination" + printf 'module: %s\nversion: %s\n' "$module" "$version" >"$destination/MODULE" + + found=false + for term in \ + "$module_dir"/LICENSE* "$module_dir"/License* "$module_dir"/license* \ + "$module_dir"/LICENCE* "$module_dir"/Licence* "$module_dir"/licence* \ + "$module_dir"/COPYING* "$module_dir"/Copying* "$module_dir"/copying* \ + "$module_dir"/NOTICE* "$module_dir"/Notice* "$module_dir"/notice* \ + "$module_dir"/PATENTS* "$module_dir"/Patents* "$module_dir"/patents*; do + [ -f "$term" ] || continue + cp "$term" "$destination/" + found=true + done + + if [ "$found" = false ]; then + printf 'no root license terms found for module %s at %s\n' "$module" "$module_dir" >&2 + exit 1 + fi +done <"$sorted_modules" From a419e770d9750caf4ea7c525056c9d2a60bd78b6 Mon Sep 17 00:00:00 2001 From: Riccardo Manfrin <3090891+riccardomanfrin@users.noreply.github.com> Date: Fri, 11 Sep 2026 09:38:22 +0200 Subject: [PATCH 03/15] [client, proxy] Make the buffer-pool retune reachable while a device is stalled (#7452) * [client] Track the WireGuard device on the engine as a lock-free handle Add an atomic handle on the wg device next to wgInterface, stored once the interface is up and cleared when it is closed. Nothing reads it yet, so this is a pure addition with no behavior change; it exists so the next commit can reach the device without taking syncMsgMux. * [client] Retune the WireGuard buffer pool without the engine lock SetPerformance took syncMsgMux before reaching the device. That lock is held by handleSync while it adds and removes peers, and peer removal is exactly what blocks when a device's buffer pool is exhausted: Peer.Stop waits on a keepalive timer callback that is itself parked in WaitPool.Get. Raising the cap is the way out of that state, so the call must not queue behind the lock the stall is holding. Read the device through the atomic handle instead. Device.SetPreallocatedBuffersPerPool takes the pool's own lock and broadcasts, so the waiters wake up. * [proxy] Extract the buffer-cap apply loop out of the perf handler Pure move: the loop over the registered clients becomes applyBufferCap, with the same sequential behavior and the same return values. Split out so the next commit can change how it iterates without the diff also carrying the move. * [proxy] Bound the perf endpoint so one wedged client cannot hold it The apply loop was sequential and unbounded. embed.Client.SetPerformance goes through the client lock, which Start holds for the whole of a startup, so a single account that is busy or wedged delayed the new buffer cap for every other account on the node -- on the endpoint whose whole purpose is to un-wedge a node. Apply to all clients concurrently and give the whole call a 5s budget. Accounts that do not answer in time are reported in "failed" instead of blocking the response. * [client] Drop the device handle before closing the interface close() cleared the atomic handle only after wgInterface.Close() returned, so a concurrent SetPerformance could still load it, retune a device that is being torn down, and report the change as applied for an engine that has stopped. Clear it first, so the window closes before the teardown begins. Reported by cubic on PR #7452. * [proxy] Put the per-client retune behind a field Pure refactor: applyBufferCap calls h.setPerformance instead of the client method directly, and NewHandler wires it to setClientPerformance. Same call, same behavior; the seam is what lets the next two commits be tested without a live embedded client. * [proxy] Do not report a finished retune as timed out When the deadline fires, select chooses at random among the ready cases, so a result already sitting in the buffered channel could be skipped and its account reported as timed out even though the cap had been applied. Drain what is buffered before declaring the rest pending. Reported by cubic on PR #7452. * [proxy] Keep one retune per account in flight The 5s budget bounds how long the endpoint waits, not the work: SetPerformance goes through the embedded client's lock, and on a wedged account Stop holds that lock forever, so every retry left one more goroutine parked there. Route each account through a single worker. A request that finds one already running takes its result if it has landed, and otherwise reports the account under "in_flight" instead of starting a second attempt. One stuck account now costs one goroutine, no matter how often the endpoint is called. Reported by CodeRabbit and cubic on PR #7452. * [proxy] Make the retune budget a var Pure refactor: perfApplyTimeout becomes a var so a test can shorten it instead of waiting five seconds. Same value, same behavior in production. * [proxy] Extract the buffered-result drain Pure refactor: the loop that empties the results channel when the deadline fires becomes collectBuffered. Same behavior; split out so it can be tested on its own, which the inline version could not be without racing the deadline. * [proxy] Cover the retune single-flight and the deadline drain TestApplyBufferCapSingleFlightPerAccount fails without the worker registry: five calls against a client stuck in its own lock start five blocked workers instead of one. TestCollectBufferedCountsResultsReadyAtTheDeadline pins the drain helper's contract - buffered results counted, errors recorded, only unanswered accounts left pending. It drives collectBuffered directly: through applyBufferCap the two select cases race by construction, so an end-to-end version of it would pass on the unfixed code about half the time. * [proxy] Keep the worker alongside each pending account Pure refactor: the pending set becomes a map to the account's worker instead of an empty struct. Same membership and same behavior; the next commit needs the worker to resolve an account whose result has not reached the channel yet. * [proxy] Publish a retune result before releasing its slot The worker sent its result last, after taking perfMu to remove itself from the registry. That lock is taken once per account by every caller walking the fleet, so a worker that finished on time could queue behind an apply over thousands of accounts and land after the deadline. Send first, deregister after. Reported by cubic on PR #7452. * [proxy] Read the worker, not the clock, for a finished retune Publishing earlier only narrows the window: a client that answers just before the deadline can still be reported as timed out. At the deadline the workers themselves are authoritative - a closed done channel means the retune finished and w.err carries its outcome, ordered by the close. Consult them instead of declaring every pending account timed out, and keep the timeout label for the ones actually still running. Reported by cubic on PR #7452. * [proxy] Cover the finished-worker resolution at the deadline Fails on the previous behavior with "applied = 0, want 1": every pending account was labelled a timeout, including the one whose retune had already completed. --- client/internal/engine.go | 28 +++-- proxy/internal/debug/handler.go | 189 ++++++++++++++++++++++++++++-- proxy/internal/debug/perf_test.go | 158 +++++++++++++++++++++++++ 3 files changed, 355 insertions(+), 20 deletions(-) create mode 100644 proxy/internal/debug/perf_test.go diff --git a/client/internal/engine.go b/client/internal/engine.go index f8b65f7d8..d517d1d68 100644 --- a/client/internal/engine.go +++ b/client/internal/engine.go @@ -14,12 +14,14 @@ import ( "sort" "strings" "sync" + "sync/atomic" "time" "github.com/hashicorp/go-multierror" "github.com/pion/ice/v4" "github.com/pion/stun/v3" log "github.com/sirupsen/logrus" + wgdevice "golang.zx2c4.com/wireguard/device" "golang.zx2c4.com/wireguard/tun/netstack" "golang.zx2c4.com/wireguard/wgctrl/wgtypes" @@ -236,6 +238,12 @@ type Engine struct { wgInterface WGIface + // wgDevice is a lock-free handle on the WireGuard device behind + // wgInterface. Reaching the device through wgInterface requires + // syncMsgMux, which handleSync holds while it adds and removes peers; + // SetPerformance must stay reachable exactly when that work is stuck. + wgDevice atomic.Pointer[wgdevice.Device] + udpMux *udpmux.UniversalUDPMuxDefault // networkSerial is the latest CurrentSerial (state ID) of the network sent by the Management service @@ -651,6 +659,7 @@ func (e *Engine) Start(netbirdConfig *mgmProto.NetbirdConfig, mgmtURL *url.URL) log.Errorf("failed to pull up wgInterface [%s]: %s", e.wgInterface.Name(), err.Error()) return fmt.Errorf("up wg interface: %w", err) } + e.wgDevice.Store(e.wgInterface.GetWGDevice()) // Set up notrack rules immediately after proxy is listening to prevent // conntrack entries from being created before the rules are in place @@ -2144,6 +2153,10 @@ func (e *Engine) close() { log.Debugf("removing Netbird interface %s", e.config.WgIfaceName) if e.wgInterface != nil { + // Drop the handle before the close starts: a retune that loads it + // afterwards would touch a device on its way out and report success + // for an engine that is already gone. + e.wgDevice.Store(nil) if err := e.wgInterface.Close(); err != nil { log.Errorf("failed closing Netbird interface %s %v", e.config.WgIfaceName, err) } @@ -2303,15 +2316,16 @@ type Performance struct { } // SetPerformance applies the given tuning to this engine's live Device. +// +// It deliberately does not take syncMsgMux. Raising the buffer pool cap is the +// recovery path for a device whose pool is exhausted, and an exhausted pool +// blocks peer removal inside handleSync, which holds syncMsgMux for as long as +// it stays blocked. Taking the lock here would make the retune unreachable in +// the one situation that needs it. func (e *Engine) SetPerformance(t Performance) error { - e.syncMsgMux.Lock() - defer e.syncMsgMux.Unlock() - if e.wgInterface == nil { - return fmt.Errorf("wg interface not initialized") - } - dev := e.wgInterface.GetWGDevice() + dev := e.wgDevice.Load() if dev == nil { - return fmt.Errorf("wg device not initialized") + return errors.New("wg device not initialized") } if t.PreallocatedBuffersPerPool != nil { dev.SetPreallocatedBuffersPerPool(*t.PreallocatedBuffersPerPool) diff --git a/proxy/internal/debug/handler.go b/proxy/internal/debug/handler.go index 6300228d7..960c3e089 100644 --- a/proxy/internal/debug/handler.go +++ b/proxy/internal/debug/handler.go @@ -105,6 +105,20 @@ type Handler struct { startTime time.Time templates *template.Template templateMu sync.RWMutex + + // setPerformance applies a buffer cap to one client. Held as a field so + // tests can drive applyBufferCap without a live embedded client. + setPerformance func(*nbembed.Client, uint32) error + + perfMu sync.Mutex + perfInflight map[types.AccountID]*perfWorker +} + +// perfWorker is the single in-flight retune for one account. err is valid once +// done is closed. +type perfWorker struct { + done chan struct{} + err error } // NewHandler creates a new debug handler. @@ -113,10 +127,11 @@ func NewHandler(provider clientProvider, healthChecker healthChecker, logger *lo logger = log.StandardLogger() } h := &Handler{ - provider: provider, - health: healthChecker, - logger: logger, - startTime: time.Now(), + provider: provider, + health: healthChecker, + logger: logger, + startTime: time.Now(), + setPerformance: setClientPerformance, } if err := h.loadTemplates(); err != nil { logger.Errorf("failed to load embedded templates: %v", err) @@ -716,15 +731,7 @@ func (h *Handler) handlePerf(w http.ResponseWriter, r *http.Request) { } capN := uint32(n) - applied := 0 - failed := map[string]string{} - for accountID, client := range h.provider.ListClientsForStartup() { - if err := client.SetPerformance(nbembed.Performance{PreallocatedBuffersPerPool: &capN}); err != nil { - failed[string(accountID)] = err.Error() - continue - } - applied++ - } + applied, failed, inFlight := h.applyBufferCap(capN) resp := map[string]any{ "success": true, @@ -734,9 +741,165 @@ func (h *Handler) handlePerf(w http.ResponseWriter, r *http.Request) { if len(failed) > 0 { resp["failed"] = failed } + if len(inFlight) > 0 { + resp["in_flight"] = inFlight + } h.writeJSON(w, resp) } +// perfApplyTimeout bounds the whole apply, however many clients are registered. +// A var, not a const, so tests can shorten the wait. +var perfApplyTimeout = 5 * time.Second + +type perfResult struct { + accountID types.AccountID + err error +} + +// setClientPerformance is the production implementation behind Handler.setPerformance. +func setClientPerformance(client *nbembed.Client, capN uint32) error { + return client.SetPerformance(nbembed.Performance{PreallocatedBuffersPerPool: &capN}) +} + +// collectBuffered takes every result already sitting in the channel, removing +// those accounts from pending, and returns how many of them succeeded. It is +// called when the deadline fires: select picks at random among ready cases, so +// a result that landed in time would otherwise be reported as a timeout. +func collectBuffered(results <-chan perfResult, pending map[types.AccountID]*perfWorker, failed map[string]string) int { + applied := 0 + for { + select { + case res := <-results: + delete(pending, res.accountID) + if res.err != nil { + failed[string(res.accountID)] = res.err.Error() + continue + } + applied++ + default: + return applied + } + } +} + +// resolvePending closes out the accounts still pending when the deadline fires. +// A worker whose done channel is closed has finished, whatever the results +// channel has managed to deliver, so its own error is the truth; the rest are +// genuinely still running and are reported as timed out. Returns how many of +// them had in fact succeeded. +func resolvePending(pending map[types.AccountID]*perfWorker, failed map[string]string) int { + applied := 0 + for accountID, w := range pending { + select { + case <-w.done: + if w.err != nil { + failed[string(accountID)] = w.err.Error() + continue + } + applied++ + default: + failed[string(accountID)] = fmt.Sprintf("timed out after %s waiting for the client", perfApplyTimeout) + } + } + return applied +} + +// startPerfWorker returns the in-flight retune for the account, starting one if +// there is none. The bool reports whether this call started it. +// +// At most one retune runs per account at a time. A client wedged inside its own +// lock never returns, so without this a caller could add one permanently blocked +// goroutine per request just by retrying the endpoint. +func (h *Handler) startPerfWorker(accountID types.AccountID, client *nbembed.Client, capN uint32, results chan<- perfResult) (*perfWorker, bool) { + h.perfMu.Lock() + defer h.perfMu.Unlock() + + if w, ok := h.perfInflight[accountID]; ok { + return w, false + } + + w := &perfWorker{done: make(chan struct{})} + if h.perfInflight == nil { + h.perfInflight = make(map[types.AccountID]*perfWorker) + } + h.perfInflight[accountID] = w + + go func() { + err := h.setPerformance(client, capN) + w.err = err + close(w.done) + + // Publish before touching the registry: perfMu is taken once per + // account by every caller walking the fleet, so a finishing worker + // can queue behind a long apply and miss its own deadline. + results <- perfResult{accountID: accountID, err: err} + + h.perfMu.Lock() + delete(h.perfInflight, accountID) + h.perfMu.Unlock() + }() + + return w, true +} + +// applyBufferCap sets the WireGuard buffer pool cap on every registered client +// and reports how many took it, a per-account error for those that did not, and +// the accounts whose earlier retune has not come back yet. +// +// Clients are handled concurrently and the wait is bounded: SetPerformance goes +// through the embedded client's lock, which Start and Stop hold for as long as +// they take - and on a wedged client Stop never returns. This endpoint is the +// recovery path for exactly that fleet, so one stuck account must neither delay +// the others nor accumulate goroutines across retries. +func (h *Handler) applyBufferCap(capN uint32) (int, map[string]string, []string) { + clients := h.provider.ListClientsForStartup() + results := make(chan perfResult, len(clients)) + + applied := 0 + failed := map[string]string{} + var inFlight []string + pending := make(map[types.AccountID]*perfWorker, len(clients)) + + for accountID, client := range clients { + w, started := h.startPerfWorker(accountID, client, capN, results) + if started { + pending[accountID] = w + continue + } + // Another request owns this account's retune. Take its result if it + // has already landed, otherwise report it as still running instead of + // waiting on it again. + select { + case <-w.done: + if w.err != nil { + failed[string(accountID)] = w.err.Error() + continue + } + applied++ + default: + inFlight = append(inFlight, string(accountID)) + } + } + + deadline := time.After(perfApplyTimeout) + for range len(pending) { + select { + case res := <-results: + delete(pending, res.accountID) + if res.err != nil { + failed[string(res.accountID)] = res.err.Error() + continue + } + applied++ + case <-deadline: + applied += collectBuffered(results, pending, failed) + applied += resolvePending(pending, failed) + return applied, failed, inFlight + } + } + return applied, failed, inFlight +} + // handleRuntime returns cheap runtime and process stats. Safe to hit on a // running proxy; does not read pprof profiles. func (h *Handler) handleRuntime(w http.ResponseWriter, _ *http.Request) { diff --git a/proxy/internal/debug/perf_test.go b/proxy/internal/debug/perf_test.go new file mode 100644 index 000000000..abcfccb50 --- /dev/null +++ b/proxy/internal/debug/perf_test.go @@ -0,0 +1,158 @@ +package debug + +import ( + "context" + "errors" + "sync/atomic" + "testing" + "time" + + nbembed "github.com/netbirdio/netbird/client/embed" + "github.com/netbirdio/netbird/proxy/internal/health" + "github.com/netbirdio/netbird/proxy/internal/roundtrip" + "github.com/netbirdio/netbird/proxy/internal/types" +) + +// perfProvider serves a fixed set of accounts. The clients are nil: the tests +// drive Handler.setPerformance, which never dereferences them. +type perfProvider struct { + accounts []types.AccountID +} + +func (p *perfProvider) GetClient(types.AccountID) (*nbembed.Client, bool) { return nil, false } + +func (p *perfProvider) ListClientsForDebug() map[types.AccountID]roundtrip.ClientDebugInfo { + return nil +} + +func (p *perfProvider) ListClientsForStartup() map[types.AccountID]*nbembed.Client { + out := make(map[types.AccountID]*nbembed.Client, len(p.accounts)) + for _, id := range p.accounts { + out[id] = nil + } + return out +} + +type stubHealth struct{} + +func (stubHealth) ReadinessProbe() bool { return true } +func (stubHealth) StartupProbe(context.Context) bool { return true } +func (stubHealth) CheckClientsConnected(context.Context) (bool, map[types.AccountID]health.ClientHealth) { + return true, nil +} + +func shortenPerfTimeout(t *testing.T, d time.Duration) { + t.Helper() + prev := perfApplyTimeout + perfApplyTimeout = d + t.Cleanup(func() { perfApplyTimeout = prev }) +} + +// TestCollectBufferedCountsResultsReadyAtTheDeadline covers the select-ordering +// trap: when the deadline fires, results already buffered must be counted, not +// reported as timeouts. Driving collectBuffered directly keeps it deterministic +// - through applyBufferCap the two select cases race by construction. +func TestCollectBufferedCountsResultsReadyAtTheDeadline(t *testing.T) { + results := make(chan perfResult, 3) + results <- perfResult{accountID: "ok"} + results <- perfResult{accountID: "broken", err: errors.New("boom")} + + pending := map[types.AccountID]*perfWorker{ + "ok": {done: make(chan struct{})}, + "broken": {done: make(chan struct{})}, + "wedged": {done: make(chan struct{})}, + } + failed := map[string]string{} + + applied := collectBuffered(results, pending, failed) + + if applied != 1 { + t.Fatalf("applied = %d, want 1", applied) + } + if failed["broken"] != "boom" { + t.Fatalf("failed = %v, want the error recorded for \"broken\"", failed) + } + if _, ok := pending["wedged"]; !ok || len(pending) != 1 { + t.Fatalf("pending = %v, want only the account that never answered", pending) + } +} + +// TestApplyBufferCapSingleFlightPerAccount covers the goroutine accumulation +// reported on PR #7452: repeated calls against a client stuck in its own lock +// must not start a second attempt for the same account. +func TestApplyBufferCapSingleFlightPerAccount(t *testing.T) { + shortenPerfTimeout(t, 50*time.Millisecond) + + release := make(chan struct{}) + t.Cleanup(func() { close(release) }) + + var calls atomic.Int32 + h := &Handler{ + provider: &perfProvider{accounts: []types.AccountID{"wedged"}}, + health: stubHealth{}, + setPerformance: func(_ *nbembed.Client, _ uint32) error { + calls.Add(1) + <-release + return nil + }, + } + + for i := range 5 { + applied, failed, inFlight := h.applyBufferCap(4096) + if applied != 0 { + t.Fatalf("call %d: applied = %d, want 0", i, applied) + } + if i == 0 { + if len(failed) != 1 { + t.Fatalf("first call: failed = %v, want the account reported as timed out", failed) + } + continue + } + if len(inFlight) != 1 { + t.Fatalf("call %d: inFlight = %v, want the account reported as still running", i, inFlight) + } + if len(failed) != 0 { + t.Fatalf("call %d: failed = %v, want empty while the retune is in flight", i, failed) + } + } + + if got := calls.Load(); got != 1 { + t.Fatalf("setPerformance called %d times, want 1: each retry started another blocked worker", got) + } +} + +// TestResolvePendingTrustsFinishedWorkers covers the reporting race cubic +// flagged on PR #7452: a retune that finished just before the deadline must be +// reported by its outcome, not as a timeout, whatever the results channel has +// delivered so far. +func TestResolvePendingTrustsFinishedWorkers(t *testing.T) { + ok := &perfWorker{done: make(chan struct{})} + close(ok.done) + + broken := &perfWorker{done: make(chan struct{}), err: errors.New("boom")} + close(broken.done) + + stillRunning := &perfWorker{done: make(chan struct{})} + + pending := map[types.AccountID]*perfWorker{ + "ok": ok, + "broken": broken, + "running": stillRunning, + } + failed := map[string]string{} + + applied := resolvePending(pending, failed) + + if applied != 1 { + t.Fatalf("applied = %d, want 1", applied) + } + if failed["broken"] != "boom" { + t.Fatalf("failed[broken] = %q, want the worker's own error", failed["broken"]) + } + if _, ok := failed["ok"]; ok { + t.Fatalf("failed = %v, want no entry for the account that succeeded", failed) + } + if got := failed["running"]; got == "" || got == "boom" { + t.Fatalf("failed[running] = %q, want the timeout message", got) + } +} From add8a75981b84375c3cfca5cb23f33f41d77e1b5 Mon Sep 17 00:00:00 2001 From: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Date: Fri, 11 Sep 2026 13:49:20 +0200 Subject: [PATCH 04/15] [management] validate peer existence when adding to group (#7486) --- management/server/group.go | 30 +++++++-- management/server/group_test.go | 81 ++++++++++++++++++++++- management/server/store/sql_store.go | 4 +- management/server/store/sql_store_test.go | 14 ++++ 4 files changed, 121 insertions(+), 8 deletions(-) diff --git a/management/server/group.go b/management/server/group.go index 33870f25e..ca20a6b08 100644 --- a/management/server/group.go +++ b/management/server/group.go @@ -101,10 +101,8 @@ func (am *DefaultAccountManager) CreateGroup(ctx context.Context, accountID, use return status.Errorf(status.Internal, "failed to create group: %v", err) } - for _, peerID := range newGroup.Peers { - if err := transaction.AddPeerToGroup(ctx, accountID, peerID, newGroup.ID); err != nil { - return status.Errorf(status.Internal, "failed to add peer %s to group %s: %v", peerID, newGroup.ID, err) - } + if err = syncGroupMembership(ctx, transaction, accountID, newGroup.ID, newGroup.Peers, nil); err != nil { + return err } snap, err = affectedpeers.Load(ctx, transaction, accountID, change) @@ -200,6 +198,9 @@ func (am *DefaultAccountManager) UpdateGroup(ctx context.Context, accountID, use // syncGroupMembership applies the peer membership delta for a group within a transaction. func syncGroupMembership(ctx context.Context, transaction store.Store, accountID, groupID string, peersToAdd, peersToRemove []string) error { + if err := validateGroupPeers(ctx, transaction, accountID, peersToAdd); err != nil { + return err + } for _, peerID := range peersToAdd { if err := transaction.AddPeerToGroup(ctx, accountID, peerID, groupID); err != nil { return status.Errorf(status.Internal, "failed to add peer %s to group %s: %v", peerID, groupID, err) @@ -213,6 +214,25 @@ func syncGroupMembership(ctx context.Context, transaction store.Store, accountID return nil } +func validateGroupPeers(ctx context.Context, transaction store.Store, accountID string, peerIDs []string) error { + if len(peerIDs) == 0 { + return nil + } + + peers, err := transaction.GetPeersByIDs(ctx, store.LockingStrengthNone, accountID, peerIDs) + if err != nil { + return err + } + + for _, peerID := range peerIDs { + if _, ok := peers[peerID]; !ok { + return status.Errorf(status.InvalidArgument, "peer with ID %s not found", peerID) + } + } + + return nil +} + // CreateGroups adds new groups to the account. // Note: This function does not acquire the global lock. // It is the caller's responsibility to ensure proper locking is in place before invoking this method. @@ -540,7 +560,7 @@ func (am *DefaultAccountManager) GroupAddPeer(ctx context.Context, accountID, gr change := affectedpeers.Change{OutputPeerIDs: []string{peerID}, LinkGroups: []string{groupID}} err := am.Store.ExecuteInTransaction(ctx, func(transaction store.Store) error { - if err := transaction.AddPeerToGroup(ctx, accountID, peerID, groupID); err != nil { + if err := syncGroupMembership(ctx, transaction, accountID, groupID, []string{peerID}, nil); err != nil { return err } diff --git a/management/server/group_test.go b/management/server/group_test.go index f5aeceea8..da056c8a9 100644 --- a/management/server/group_test.go +++ b/management/server/group_test.go @@ -11,10 +11,10 @@ import ( "testing" "time" - "go.uber.org/mock/gomock" "github.com/google/uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + "go.uber.org/mock/gomock" "golang.org/x/exp/maps" nbdns "github.com/netbirdio/netbird/dns" @@ -1236,3 +1236,82 @@ func Test_IncrementNetworkSerial(t *testing.T) { assert.Equal(t, totalPeers, int(account.Network.Serial), "Expected %d serial increases in account %s, got %d", totalPeers, accountID, account.Network.Serial) } + +func TestDefaultAccountManager_GroupPeersMustBelongToAccount(t *testing.T) { + manager, _, account, peer1, _, _ := setupNetworkMapTest(t) + + otherAccount, err := createAccount(manager, "other_account", "other_user", "") + require.NoError(t, err) + + foreignPeer := &peer2.Peer{ + ID: "foreign-peer", + AccountID: otherAccount.Id, + Key: "foreign-key", + DNSLabel: "foreign-peer", + IP: uint32ToIP(1), + } + require.NoError(t, manager.Store.AddPeerToAccount(context.Background(), foreignPeer)) + + assertRejected := func(t *testing.T, err error) { + t.Helper() + require.Error(t, err) + s, ok := status.FromError(err) + require.True(t, ok, "expected status error, got %v", err) + assert.Equal(t, status.InvalidArgument, s.Type(), "peer outside the account should be rejected as invalid argument") + } + + t.Run("create rejects foreign peer", func(t *testing.T) { + err := manager.CreateGroup(context.Background(), account.Id, userID, &types.Group{ + Name: "foreign", + Issued: types.GroupIssuedAPI, + Peers: []string{peer1.ID, foreignPeer.ID}, + }) + assertRejected(t, err) + + _, err = manager.Store.GetGroupByName(context.Background(), store.LockingStrengthNone, account.Id, "foreign") + assert.Error(t, err, "rejected create must not persist the group") + }) + + t.Run("update rejects foreign and unknown peers", func(t *testing.T) { + group := &types.Group{ID: "own", Name: "own", Issued: types.GroupIssuedAPI, Peers: []string{peer1.ID}} + require.NoError(t, manager.CreateGroup(context.Background(), account.Id, userID, group)) + + group.Peers = []string{peer1.ID, foreignPeer.ID} + assertRejected(t, manager.UpdateGroup(context.Background(), account.Id, userID, group)) + + group.Peers = []string{peer1.ID, "does-not-exist"} + assertRejected(t, manager.UpdateGroup(context.Background(), account.Id, userID, group)) + + stored, err := manager.Store.GetGroupByID(context.Background(), store.LockingStrengthNone, account.Id, group.ID) + require.NoError(t, err) + assert.Equal(t, []string{peer1.ID}, stored.Peers, "rejected updates must not change membership") + }) + + t.Run("update tolerates and drops pre-existing dangling members", func(t *testing.T) { + group := &types.Group{ID: "polluted", Name: "polluted", Issued: types.GroupIssuedAPI, Peers: []string{peer1.ID}} + require.NoError(t, manager.CreateGroup(context.Background(), account.Id, userID, group)) + require.NoError(t, manager.Store.AddPeerToGroup(context.Background(), account.Id, foreignPeer.ID, group.ID)) + + group.Peers = []string{peer1.ID, foreignPeer.ID} + assert.NoError(t, manager.UpdateGroup(context.Background(), account.Id, userID, group), "keeping an existing member must not be rejected") + + group.Peers = []string{peer1.ID} + require.NoError(t, manager.UpdateGroup(context.Background(), account.Id, userID, group)) + + stored, err := manager.Store.GetGroupByID(context.Background(), store.LockingStrengthNone, account.Id, group.ID) + require.NoError(t, err) + assert.Equal(t, []string{peer1.ID}, stored.Peers, "dangling member should be removed once omitted") + }) + + t.Run("direct add rejects foreign and unknown peers", func(t *testing.T) { + group := &types.Group{ID: "direct", Name: "direct", Issued: types.GroupIssuedAPI, Peers: []string{peer1.ID}} + require.NoError(t, manager.CreateGroup(context.Background(), account.Id, userID, group)) + + assertRejected(t, manager.GroupAddPeer(context.Background(), account.Id, group.ID, foreignPeer.ID)) + assertRejected(t, manager.GroupAddPeer(context.Background(), account.Id, group.ID, "does-not-exist")) + + stored, err := manager.Store.GetGroupByID(context.Background(), store.LockingStrengthNone, account.Id, group.ID) + require.NoError(t, err) + assert.Equal(t, []string{peer1.ID}, stored.Peers, "rejected direct adds must not change membership") + }) +} diff --git a/management/server/store/sql_store.go b/management/server/store/sql_store.go index ef353ea83..33c723a8a 100644 --- a/management/server/store/sql_store.go +++ b/management/server/store/sql_store.go @@ -3473,7 +3473,7 @@ func (s *SqlStore) GetPeerGroups(ctx context.Context, lockStrength LockingStreng var groups []*types.Group query := tx. Joins("JOIN group_peers ON group_peers.group_id = groups.id"). - Where("group_peers.peer_id = ?", peerId). + Where("groups.account_id = ? AND group_peers.peer_id = ?", accountId, peerId). Preload(clause.Associations). Find(&groups) @@ -5053,7 +5053,7 @@ func (s *SqlStore) GetPeersByGroupIDs(ctx context.Context, accountID string, gro Select("DISTINCT peer_id"). Where("account_id = ? AND group_id IN ?", accountID, groupIDs) - result := s.db.Where("id IN (?)", peerIDsSubquery).Find(&peers) + result := s.db.Where("account_id = ? AND id IN (?)", accountID, peerIDsSubquery).Find(&peers) if result.Error != nil { log.WithContext(ctx).Errorf("failed to get peers by group IDs: %s", result.Error) return nil, status.Errorf(status.Internal, "failed to get peers by group IDs") diff --git a/management/server/store/sql_store_test.go b/management/server/store/sql_store_test.go index 4b7bcf068..fbcff5257 100644 --- a/management/server/store/sql_store_test.go +++ b/management/server/store/sql_store_test.go @@ -2844,6 +2844,14 @@ func TestSqlStore_GetPeerGroups(t *testing.T) { groups, err = store.GetPeerGroups(context.Background(), LockingStrengthNone, accountID, peerID) require.NoError(t, err) assert.Len(t, groups, 2) + + foreignPeerID := "foreign-peer" + err = store.AddPeerToGroup(context.Background(), accountID, foreignPeerID, "cfefqs706sqkneg59g4h") + require.NoError(t, err) + + groups, err = store.GetPeerGroups(context.Background(), LockingStrengthNone, "other-account", foreignPeerID) + require.NoError(t, err) + assert.Empty(t, groups, "groups of another account must not be returned") } func TestSqlStore_GetAccountPeers(t *testing.T) { @@ -4039,9 +4047,15 @@ func TestSqlStore_GetPeersByGroupIDs(t *testing.T) { } require.NoError(t, store.CreateGroups(ctx, accountID, groups)) + otherAccount := newAccountWithId(ctx, "other-account", "other-user", "") + require.NoError(t, store.SaveAccount(ctx, otherAccount)) + foreignPeer := &nbpeer.Peer{ID: "foreign-peer", AccountID: otherAccount.Id} + require.NoError(t, store.AddPeerToAccount(ctx, foreignPeer)) + require.NoError(t, store.AddPeerToGroup(ctx, accountID, peer1, group1ID)) require.NoError(t, store.AddPeerToGroup(ctx, accountID, peer2, group1ID)) require.NoError(t, store.AddPeerToGroup(ctx, accountID, peer1, group2ID)) + require.NoError(t, store.AddPeerToGroup(ctx, accountID, foreignPeer.ID, group1ID)) peers, err := store.GetPeersByGroupIDs(ctx, accountID, tt.groupIDs) require.NoError(t, err) From 1047df5fa26dd690ef812bd35a454fd6cd0d68c2 Mon Sep 17 00:00:00 2001 From: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Date: Fri, 11 Sep 2026 13:50:11 +0200 Subject: [PATCH 05/15] [management] pass tls config for combined server (#7499) --- combined/cmd/root.go | 11 +++--- management/internals/server/server.go | 50 ++++++++++++++++++--------- 2 files changed, 39 insertions(+), 22 deletions(-) diff --git a/combined/cmd/root.go b/combined/cmd/root.go index 3e583ef20..917312e57 100644 --- a/combined/cmd/root.go +++ b/combined/cmd/root.go @@ -205,7 +205,7 @@ func createAllServers(ctx context.Context, cfg *CombinedConfig) (*serverInstance metricsServer: metricsServer, } - _, tlsSupport, err := handleTLSConfig(cfg) + tlsConfig, tlsSupport, err := handleTLSConfig(cfg) if err != nil { return nil, fmt.Errorf("failed to setup TLS config: %w", err) } @@ -214,7 +214,7 @@ func createAllServers(ctx context.Context, cfg *CombinedConfig) (*serverInstance return nil, err } - if err := servers.createManagementServer(ctx, cfg); err != nil { + if err := servers.createManagementServer(ctx, cfg, tlsConfig); err != nil { return nil, err } @@ -264,7 +264,7 @@ func (s *serverInstances) createRelayServer(cfg *CombinedConfig, tlsSupport bool return nil } -func (s *serverInstances) createManagementServer(ctx context.Context, cfg *CombinedConfig) error { +func (s *serverInstances) createManagementServer(ctx context.Context, cfg *CombinedConfig, tlsConfig *tls.Config) error { if !cfg.Management.Enabled { return nil } @@ -297,7 +297,7 @@ func (s *serverInstances) createManagementServer(ctx context.Context, cfg *Combi LogConfigInfo(mgmtConfig) - s.mgmtSrv, err = createManagementServer(cfg, mgmtConfig) + s.mgmtSrv, err = createManagementServer(cfg, mgmtConfig, tlsConfig) if err != nil { cleanupSTUNListeners(s.stunListeners) return fmt.Errorf("failed to create management server: %w", err) @@ -513,7 +513,7 @@ func handleTLSConfig(cfg *CombinedConfig) (*tls.Config, bool, error) { return nil, false, nil } -func createManagementServer(cfg *CombinedConfig, mgmtConfig *nbconfig.Config) (mgmtServer.Server, error) { +func createManagementServer(cfg *CombinedConfig, mgmtConfig *nbconfig.Config, tlsConfig *tls.Config) (mgmtServer.Server, error) { mgmt := cfg.Management // Extract port from listen address @@ -542,6 +542,7 @@ func createManagementServer(cfg *CombinedConfig, mgmtConfig *nbconfig.Config) (m AutoResolveDomains: true, MgmtPort: mgmtPort, MgmtMetricsPort: cfg.Server.MetricsPort, + TLSConfig: tlsConfig, DisableMetrics: mgmt.DisableAnonymousMetrics, DisableGeoliteUpdate: mgmt.DisableGeoliteUpdate, // Always enable user deletion from IDP in combined server (embedded IdP is always enabled) diff --git a/management/internals/server/server.go b/management/internals/server/server.go index 22a61bada..9709d1099 100644 --- a/management/internals/server/server.go +++ b/management/internals/server/server.go @@ -74,6 +74,7 @@ type BaseServer struct { grpcExtensions []GRPCExtension listener net.Listener + tlsConfig *tls.Config certManager *autocert.Manager update *version.Update @@ -94,6 +95,7 @@ type Config struct { DisableGeoliteUpdate bool UserDeleteFromIDPEnabled bool AutoResolveDomains bool + TLSConfig *tls.Config } // NewServer initializes and configures a new Server instance @@ -110,6 +112,7 @@ func NewServer(cfg *Config) *BaseServer { disableLegacyManagementPort: cfg.DisableLegacyManagementPort, mgmtMetricsPort: cfg.MgmtMetricsPort, autoResolveDomains: cfg.AutoResolveDomains, + tlsConfig: cfg.TLSConfig, } s.container[ContainerKeyBaseServer] = s @@ -139,21 +142,9 @@ func (s *BaseServer) Start(ctx context.Context) error { } s.EphemeralManager().LoadInitialPeers(srvCtx) - var tlsConfig *tls.Config - tlsEnabled := false - if s.Config.HttpConfig.LetsEncryptDomain != "" { - s.certManager, err = encryption.CreateCertManager(s.Config.Datadir, s.Config.HttpConfig.LetsEncryptDomain) - if err != nil { - return fmt.Errorf("failed creating LetsEncrypt cert manager: %v", err) - } - tlsEnabled = true - } else if s.Config.HttpConfig.CertFile != "" && s.Config.HttpConfig.CertKey != "" { - tlsConfig, err = loadTLSConfig(s.Config.HttpConfig.CertFile, s.Config.HttpConfig.CertKey) - if err != nil { - log.WithContext(srvCtx).Errorf("cannot load TLS credentials: %v", err) - return err - } - tlsEnabled = true + tlsEnabled, err := s.setupTLS(srvCtx) + if err != nil { + return err } installationID, err := getInstallationID(srvCtx, s.Store()) @@ -215,8 +206,8 @@ func (s *BaseServer) Start(ctx context.Context) error { log.WithContext(ctx).Infof("running HTTP server (LetsEncrypt challenge handler): %s", cml.Addr().String()) s.serveHTTP(ctx, cml, s.certManager.HTTPHandler(nil)) } - case tlsConfig != nil: - s.listener, err = tls.Listen("tcp", fmt.Sprintf(":%d", s.mgmtPort), tlsConfig) + case s.tlsConfig != nil: + s.listener, err = tls.Listen("tcp", fmt.Sprintf(":%d", s.mgmtPort), s.tlsConfig) if err != nil { return fmt.Errorf("failed creating TLS listener on port %d: %v", s.mgmtPort, err) } @@ -240,6 +231,31 @@ func (s *BaseServer) Start(ctx context.Context) error { return nil } +// setupTLS resolves the listener's TLS source: an injected config wins over the HttpConfig certificate settings +func (s *BaseServer) setupTLS(ctx context.Context) (bool, error) { + switch { + case s.tlsConfig != nil: + return true, nil + case s.Config.HttpConfig.LetsEncryptDomain != "": + certManager, err := encryption.CreateCertManager(s.Config.Datadir, s.Config.HttpConfig.LetsEncryptDomain) + if err != nil { + return false, fmt.Errorf("failed creating LetsEncrypt cert manager: %v", err) + } + s.certManager = certManager + return true, nil + case s.Config.HttpConfig.CertFile != "" && s.Config.HttpConfig.CertKey != "": + tlsConfig, err := loadTLSConfig(s.Config.HttpConfig.CertFile, s.Config.HttpConfig.CertKey) + if err != nil { + log.WithContext(ctx).Errorf("cannot load TLS credentials: %v", err) + return false, err + } + s.tlsConfig = tlsConfig + return true, nil + default: + return false, nil + } +} + // Stop attempts a graceful shutdown, waiting up to 5 seconds for active connections to finish func (s *BaseServer) Stop() error { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) From ad3f570e324c1098d4f035466e77730e7df1b214 Mon Sep 17 00:00:00 2001 From: Pascal Fischer <32096965+pascal-fischer@users.noreply.github.com> Date: Fri, 11 Sep 2026 13:51:03 +0200 Subject: [PATCH 06/15] [management] validate the domain for the flock in proxy (#7501) --- proxy/internal/acme/locker.go | 16 ++++++++++++---- proxy/internal/acme/locker_test.go | 30 ++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 4 deletions(-) diff --git a/proxy/internal/acme/locker.go b/proxy/internal/acme/locker.go index 2f0f18885..f42324736 100644 --- a/proxy/internal/acme/locker.go +++ b/proxy/internal/acme/locker.go @@ -2,12 +2,14 @@ package acme import ( "context" + "fmt" "path/filepath" log "github.com/sirupsen/logrus" "github.com/netbirdio/netbird/proxy/internal/flock" "github.com/netbirdio/netbird/proxy/internal/k8s" + "github.com/netbirdio/netbird/shared/management/domain" ) // certLocker provides distributed mutual exclusion for certificate operations. @@ -74,9 +76,15 @@ func newFlockLocker(certDir string, logger *log.Logger) *flockLocker { return &flockLocker{certDir: certDir, logger: logger} } -// Lock acquires an advisory file lock for the given domain. -func (l *flockLocker) Lock(ctx context.Context, domain string) (func(), error) { - lockPath := filepath.Join(l.certDir, domain+".lock") +// Lock acquires an advisory file lock for the given domain. The domain must +// be a valid hostname so the lock file always resolves to a direct child of +// certDir; anything else is rejected before touching the filesystem. +func (l *flockLocker) Lock(ctx context.Context, name string) (func(), error) { + if !domain.IsValidDomainNoWildcard(name) { + return nil, fmt.Errorf("invalid domain %q for lock file", name) + } + + lockPath := filepath.Join(l.certDir, name+".lock") lockFile, err := flock.Lock(ctx, lockPath) if err != nil { return nil, err @@ -89,7 +97,7 @@ func (l *flockLocker) Lock(ctx context.Context, domain string) (func(), error) { return func() { if err := flock.Unlock(lockFile); err != nil { - l.logger.Debugf("release cert lock for domain %q: %v", domain, err) + l.logger.Debugf("release cert lock for domain %q: %v", name, err) } }, nil } diff --git a/proxy/internal/acme/locker_test.go b/proxy/internal/acme/locker_test.go index 39245df0c..f131f64f3 100644 --- a/proxy/internal/acme/locker_test.go +++ b/proxy/internal/acme/locker_test.go @@ -63,3 +63,33 @@ func TestNewCertLockerK8sFallsBackToFlock(t *testing.T) { _, ok := locker.(*flockLocker) assert.True(t, ok, "k8s-lease without SA should fall back to flockLocker") } + +func TestFlockLockerRejectsUnsafeDomain(t *testing.T) { + root := t.TempDir() + certDir := filepath.Join(root, "certs") + require.NoError(t, os.Mkdir(certDir, 0o700)) + locker := newFlockLocker(certDir, nil) + + for _, d := range []string{ + "", + ".", + "..", + "../escape", + "../../etc/cron.d/attacker", + "sub/dir.example.com", + `back\slash.example.com`, + "*.example.com", + } { + unlock, err := locker.Lock(context.Background(), d) + assert.Error(t, err, "domain %q", d) + assert.Nil(t, unlock, "domain %q", d) + } + + assert.NoFileExists(t, filepath.Join(root, "escape.lock")) + certEntries, err := os.ReadDir(certDir) + require.NoError(t, err) + assert.Empty(t, certEntries) + rootEntries, err := os.ReadDir(root) + require.NoError(t, err) + assert.Len(t, rootEntries, 1) +} From b57f0e56085bb57d05611b2c378913a2b9098c43 Mon Sep 17 00:00:00 2001 From: Nicolas Frati Date: Fri, 11 Sep 2026 14:20:58 +0200 Subject: [PATCH 07/15] [infrastructure] Preserve snapshot image variant tags (#7511) --- .github/workflows/release.yml | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index c1bbe9c44..9d3fe3641 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -287,10 +287,15 @@ jobs: image_refs=() tag_and_push() { - local src="$1" img_name tag dst + local src="$1" img_name tag dst variant="" img_name="${src%%:*}" + # Client variants share a repository, so keep their tag suffixes. + case "$src" in + *-rootless-ubi-amd64) variant="-rootless-ubi" ;; + *-rootless-amd64) variant="-rootless" ;; + esac for tag in $(resolve_tags); do - dst="${img_name}:${tag}" + dst="${img_name}:${tag}${variant}" echo "Tagging ${src} -> ${dst}" docker tag "$src" "$dst" docker push "$dst" From 58114f98fb253ea3e4abf6eefe07d3d57b43e5d5 Mon Sep 17 00:00:00 2001 From: Bethuel Mmbaga Date: Fri, 11 Sep 2026 15:41:52 +0300 Subject: [PATCH 08/15] [management] Only trust forwarded-IP headers from configured trusted peers (#7454) --- .../getting-started-enterprise.sh | 5 +- infrastructure_files/getting-started.sh | 29 +++ infrastructure_files/management.json.tmpl | 4 +- management/internals/server/boot.go | 56 ++++-- management/internals/server/realip_test.go | 171 ++++++++++++++++++ 5 files changed, 240 insertions(+), 25 deletions(-) create mode 100644 management/internals/server/realip_test.go diff --git a/infrastructure_files/getting-started-enterprise.sh b/infrastructure_files/getting-started-enterprise.sh index 3f7cf6357..701598a60 100755 --- a/infrastructure_files/getting-started-enterprise.sh +++ b/infrastructure_files/getting-started-enterprise.sh @@ -808,8 +808,9 @@ server: # Trust X-Forwarded-* only from the Traefik container's static address. Both # keys must stay in step with the ipv4_address pinned in docker-compose.yml: - # trustedPeers decides whether forwarded headers are read at all, and leaving - # it unset falls back to 0.0.0.0/0. + # trustedPeers decides whether forwarded headers are read at all. Leaving it + # unset trusts nothing and records Traefik's own address as every peer's + # connection IP. reverseProxy: trustedPeers: - "${TRAEFIK_IP}/32" diff --git a/infrastructure_files/getting-started.sh b/infrastructure_files/getting-started.sh index 5efc0181e..afbc5c282 100755 --- a/infrastructure_files/getting-started.sh +++ b/infrastructure_files/getting-started.sh @@ -153,6 +153,7 @@ check_domain_resolves() { # NETBIRD_TRAEFIK_CERTRESOLVER external-Traefik cert resolver (type 1) # NETBIRD_BIND_LOCALHOST_ONLY true/false (default true, types 2-5) # NETBIRD_EXTERNAL_PROXY_NETWORK docker network to join (types 2-4) +# NETBIRD_TRUSTED_PEERS reverse proxy address management sees (default: built-in Traefik's IP, empty for types 1-5) # NETBIRD_NON_INTERACTIVE true forces unattended mode even with a TTY # tty_available succeeds only when we may prompt: never when the operator has @@ -459,6 +460,8 @@ initialize_default_values() { MANAGEMENT_HOST_PORT="8081" # Combined server port (management + signal + relay) BIND_LOCALHOST_ONLY="true" EXTERNAL_PROXY_NETWORK="" + TRUSTED_PEERS="" # Address the reverse proxy connects to management from + # Traefik static IP within the internal bridge network TRAEFIK_IP="172.30.0.10" @@ -519,6 +522,7 @@ apply_agent_network_preset() { REVERSE_PROXY_TYPE="0" ENABLE_PROXY="true" ENABLE_CROWDSEC="false" + TRUSTED_PEERS="${NETBIRD_TRUSTED_PEERS:-$TRAEFIK_IP/32}" TRAEFIK_ACME_EMAIL=$(resolve NETBIRD_LETSENCRYPT_EMAIL required read_traefik_acme_email) @@ -573,6 +577,21 @@ configure_reverse_proxy() { 4) EXTERNAL_PROXY_NETWORK=$(resolve NETBIRD_EXTERNAL_PROXY_NETWORK "" read_proxy_docker_network "Caddy") ;; *) ;; # No network prompt for other options esac + + # Only the bundled Traefik has an address we know at render time. External proxies + # must supply the address their proxy reaches management from. + if [[ "$REVERSE_PROXY_TYPE" == "0" ]]; then + TRUSTED_PEERS="${NETBIRD_TRUSTED_PEERS:-$TRAEFIK_IP/32}" + else + TRUSTED_PEERS="${NETBIRD_TRUSTED_PEERS:-}" + if [[ -z "$TRUSTED_PEERS" ]]; then + echo "" > /dev/stderr + echo "Note: reverseProxy.trustedPeers is unset, so NetBird will use the address your" > /dev/stderr + echo "proxy connects from as each peer's connection IP. To record real client IPs," > /dev/stderr + echo "set NETBIRD_TRUSTED_PEERS to your proxy's address (e.g. 172.20.0.5/32) and re-run." > /dev/stderr + echo "" > /dev/stderr + fi + fi return 0 } @@ -1033,6 +1052,7 @@ server: reverseProxy: trustedHTTPProxies: - "$TRAEFIK_IP/32" +$(render_trusted_peers) store: engine: "sqlite" @@ -1041,6 +1061,12 @@ EOF return 0 } +render_trusted_peers() { + if [[ -n "$TRUSTED_PEERS" ]]; then + printf ' trustedPeers:\n - "%s"' "$TRUSTED_PEERS" + fi +} + render_dashboard_env() { cat < 0 && trustedProxiesCount > 0 { - log.WithContext(context.Background()).Warn("TrustedHTTPProxies and TrustedHTTPProxiesCount both are configured. " + - "This is not recommended way to extract X-Forwarded-For. Consider using one of these options.") - } - realipOpts := []realip.Option{ - realip.WithTrustedPeers(trustedPeers), - realip.WithTrustedProxies(trustedHTTPProxies), - realip.WithTrustedProxiesCount(trustedProxiesCount), - realip.WithHeaders([]string{realip.XForwardedFor, realip.XRealIp}), - } + realipOpts := realIPOptions(s.Config.ReverseProxy) proxyUnary, proxyStream, proxyAuthClose := nbgrpc.NewProxyAuthInterceptors(s.Store()) s.proxyAuthClose = proxyAuthClose gRPCOpts := []grpc.ServerOption{ @@ -333,7 +318,7 @@ func (s *BaseServer) AccessLogsManager() accesslogs.Manager { }) } -func loadTLSConfig(certFile string, certKey string) (*tls.Config, error) { +func loadTLSConfig(certFile, certKey string) (*tls.Config, error) { // Load server's certificate and private key serverCert, err := tls.LoadX509KeyPair(certFile, certKey) if err != nil { @@ -380,3 +365,34 @@ func streamInterceptor( wrapped.WrappedContext = context.WithValue(ctx, nbContext.RequestIDKey, reqID) return handler(srv, wrapped) } + +// realIPOptions builds the real-IP middleware options from the reverse proxy config. +// +// TrustedPeers controls which transport peers are allowed to supply forwarded-IP +// headers. If empty, forwarded headers are ignored and the transport peer address +// is used directly. Operators terminating connections at a reverse proxy should +// configure TrustedPeers with that proxy's address or network. +// +// Only X-Forwarded-For is trusted. X-Real-IP contains a single client-supplied +// address with no proxy chain to validate, and none of the reverse proxies we ship +// use it on the gRPC path. +func realIPOptions(cfg nbconfig.ReverseProxy) []realip.Option { + if idx := slices.IndexFunc(cfg.TrustedPeers, func(p netip.Prefix) bool { return p.Bits() == 0 }); idx >= 0 { + log.WithContext(context.Background()).Warnf("TrustedPeers contains the default route %s, which trusts "+ + "X-Forwarded-For from every client and allows connection IP spoofing. Set TrustedPeers to the address "+ + "of your reverse proxy, or leave it empty to use the connection's source address.", cfg.TrustedPeers[idx]) + } + if cfg.TrustedHTTPProxiesCount > 0 { + log.WithContext(context.Background()).Warn( + "TrustedHTTPProxiesCount skips X-Forwarded-For entries by position before TrustedHTTPProxies filters by address. " + + "An incorrect count may skip the real client IP and produce an incorrect source address.", + ) + } + + return []realip.Option{ + realip.WithTrustedPeers(cfg.TrustedPeers), + realip.WithTrustedProxies(cfg.TrustedHTTPProxies), + realip.WithTrustedProxiesCount(cfg.TrustedHTTPProxiesCount), + realip.WithHeaders([]string{realip.XForwardedFor}), + } +} diff --git a/management/internals/server/realip_test.go b/management/internals/server/realip_test.go new file mode 100644 index 000000000..89ac02730 --- /dev/null +++ b/management/internals/server/realip_test.go @@ -0,0 +1,171 @@ +package server + +import ( + "context" + "io" + "net" + "net/netip" + "testing" + "time" + + "github.com/grpc-ecosystem/go-grpc-middleware/v2/interceptors/realip" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/grpc" + "google.golang.org/grpc/credentials/insecure" + "google.golang.org/grpc/metadata" + "google.golang.org/protobuf/types/known/emptypb" + + nbconfig "github.com/netbirdio/netbird/management/internals/server/config" +) + +const ( + realIPProbeMethod = "/netbird.test.RealIPProbe/Probe" + realIPProbeStreamMethod = "/netbird.test.RealIPProbe/ProbeStream" +) + +// realIPProbe records the real IP the middleware derived for each call. +type realIPProbe struct { + got chan string +} + +func (p *realIPProbe) record(ctx context.Context) { + addr, _ := realip.FromContext(ctx) + p.got <- addr.String() +} + +func (p *realIPProbe) wait(t *testing.T) string { + t.Helper() + + select { + case got := <-p.got: + return got + case <-time.After(5 * time.Second): + t.Fatal("timed out waiting for probe") + return "" + } +} + +func startProbeServer(t *testing.T, cfg nbconfig.ReverseProxy) (*grpc.ClientConn, *realIPProbe) { + t.Helper() + + listener, err := net.Listen("tcp", "127.0.0.1:0") + require.NoError(t, err) + + probe := &realIPProbe{got: make(chan string, 1)} + opts := realIPOptions(cfg) + srv := grpc.NewServer( + grpc.ChainUnaryInterceptor(realip.UnaryServerInterceptorOpts(opts...)), + grpc.ChainStreamInterceptor(realip.StreamServerInterceptorOpts(opts...)), + ) + srv.RegisterService(&grpc.ServiceDesc{ + ServiceName: "netbird.test.RealIPProbe", + HandlerType: (*any)(nil), + Methods: []grpc.MethodDesc{{ + MethodName: "Probe", + Handler: func(_ any, ctx context.Context, dec func(any) error, interceptor grpc.UnaryServerInterceptor) (any, error) { + req := new(emptypb.Empty) + if err := dec(req); err != nil { + return nil, err + } + handler := func(ctx context.Context, _ any) (any, error) { + probe.record(ctx) + return &emptypb.Empty{}, nil + } + if interceptor == nil { + return handler(ctx, req) + } + return interceptor(ctx, req, &grpc.UnaryServerInfo{FullMethod: realIPProbeMethod}, handler) + }, + }}, + Streams: []grpc.StreamDesc{{ + StreamName: "ProbeStream", + ServerStreams: true, + Handler: func(_ any, stream grpc.ServerStream) error { + probe.record(stream.Context()) + return nil + }, + }}, + }, probe) + + go func() { _ = srv.Serve(listener) }() + t.Cleanup(srv.Stop) + + conn, err := grpc.NewClient(listener.Addr().String(), grpc.WithTransportCredentials(insecure.NewCredentials())) + require.NoError(t, err) + t.Cleanup(func() { _ = conn.Close() }) + + return conn, probe +} + +func callUnary(t *testing.T, conn *grpc.ClientConn, probe *realIPProbe, kv ...string) string { + t.Helper() + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + ctx = metadata.AppendToOutgoingContext(ctx, kv...) + require.NoError(t, conn.Invoke(ctx, realIPProbeMethod, &emptypb.Empty{}, &emptypb.Empty{})) + + return probe.wait(t) +} + +func callStream(t *testing.T, conn *grpc.ClientConn, probe *realIPProbe, kv ...string) string { + t.Helper() + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + ctx = metadata.AppendToOutgoingContext(ctx, kv...) + desc := &grpc.StreamDesc{StreamName: "ProbeStream", ServerStreams: true} + stream, err := conn.NewStream(ctx, desc, realIPProbeStreamMethod) + require.NoError(t, err) + require.NoError(t, stream.CloseSend()) + require.ErrorIs(t, stream.RecvMsg(&emptypb.Empty{}), io.EOF) + + return probe.wait(t) +} + +func assertRealIP(t *testing.T, cfg nbconfig.ReverseProxy, want string, kv ...string) { + t.Helper() + + conn, probe := startProbeServer(t, cfg) + t.Run("unary", func(t *testing.T) { + assert.Equal(t, want, callUnary(t, conn, probe, kv...)) + }) + t.Run("stream", func(t *testing.T) { + assert.Equal(t, want, callStream(t, conn, probe, kv...)) + }) +} + +func TestRealIPDefaultIgnoresClientForwardedHeaders(t *testing.T) { + assertRealIP(t, nbconfig.ReverseProxy{}, "127.0.0.1", + realip.XForwardedFor, "203.0.113.44", + realip.XRealIp, "203.0.113.44", + ) +} + +func TestRealIPUntrustedPeerIgnoresForwardedHeaders(t *testing.T) { + cfg := nbconfig.ReverseProxy{TrustedPeers: []netip.Prefix{netip.MustParsePrefix("10.9.8.7/32")}} + + assertRealIP(t, cfg, "127.0.0.1", + realip.XForwardedFor, "203.0.113.44", + realip.XRealIp, "203.0.113.44", + ) +} + +func TestRealIPTrustedPeerHonoursForwardedHeaders(t *testing.T) { + cfg := nbconfig.ReverseProxy{TrustedPeers: []netip.Prefix{netip.MustParsePrefix("127.0.0.1/32")}} + + assertRealIP(t, cfg, "203.0.113.44", + realip.XForwardedFor, "203.0.113.44", + realip.XRealIp, "203.0.113.44", + ) +} + +func TestRealIPIgnoresXRealIPWhenProxyCountIsSet(t *testing.T) { + cfg := nbconfig.ReverseProxy{ + TrustedPeers: []netip.Prefix{netip.MustParsePrefix("127.0.0.1/32")}, + TrustedHTTPProxiesCount: 1, + } + + assertRealIP(t, cfg, "127.0.0.1", realip.XRealIp, "203.0.113.44") +} From 794956a7a313c8919fd92123fb06ff30b7b0032a Mon Sep 17 00:00:00 2001 From: Zoltan Papp Date: Fri, 11 Sep 2026 16:21:10 +0200 Subject: [PATCH 09/15] [client] Fix relay instance address race (#7498) Read the relay instance URL and IP atomically to prevent reconnects from mixing values from different connections. Extend existing connection and offer/answer logs with relay URLs and IPs to help trace mismatched advertisements. --- client/internal/peer/handshaker.go | 8 +- shared/relay/client/client.go | 41 ++++---- shared/relay/client/client_serverip_test.go | 45 ++++----- shared/relay/client/manager.go | 6 +- shared/relay/client/manager_address_test.go | 103 ++++++++++++++++++++ shared/relay/client/picker.go | 7 +- 6 files changed, 157 insertions(+), 53 deletions(-) create mode 100644 shared/relay/client/manager_address_test.go diff --git a/client/internal/peer/handshaker.go b/client/internal/peer/handshaker.go index 6ecb2a947..654e32158 100644 --- a/client/internal/peer/handshaker.go +++ b/client/internal/peer/handshaker.go @@ -116,7 +116,7 @@ func (h *Handshaker) Listen(ctx context.Context) { for { select { case remoteOfferAnswer := <-h.remoteOffersCh: - h.log.Infof("received offer, running version %s, remote WireGuard listen port %d, session id: %s, remote ICE supported: %t", remoteOfferAnswer.Version, remoteOfferAnswer.WgListenPort, remoteOfferAnswer.SessionIDString(), remoteOfferAnswer.hasICECredentials()) + h.log.Infof("received offer, running version %s, remote WireGuard listen port %d, session id: %s, remote ICE supported: %t, relay server: %s, relay IP: %s", remoteOfferAnswer.Version, remoteOfferAnswer.WgListenPort, remoteOfferAnswer.SessionIDString(), remoteOfferAnswer.hasICECredentials(), remoteOfferAnswer.RelaySrvAddress, remoteOfferAnswer.RelaySrvIP) // Record signaling received for reconnection attempts if h.metricsStages != nil { @@ -138,7 +138,7 @@ func (h *Handshaker) Listen(ctx context.Context) { continue } case remoteOfferAnswer := <-h.remoteAnswerCh: - h.log.Infof("received answer, running version %s, remote WireGuard listen port %d, session id: %s, remote ICE supported: %t", remoteOfferAnswer.Version, remoteOfferAnswer.WgListenPort, remoteOfferAnswer.SessionIDString(), remoteOfferAnswer.hasICECredentials()) + h.log.Infof("received answer, running version %s, remote WireGuard listen port %d, session id: %s, remote ICE supported: %t, relay server: %s, relay IP: %s", remoteOfferAnswer.Version, remoteOfferAnswer.WgListenPort, remoteOfferAnswer.SessionIDString(), remoteOfferAnswer.hasICECredentials(), remoteOfferAnswer.RelaySrvAddress, remoteOfferAnswer.RelaySrvIP) // Record signaling received for reconnection attempts if h.metricsStages != nil { @@ -209,14 +209,14 @@ func (h *Handshaker) sendOffer() error { } offer := h.buildOfferAnswer() - h.log.Debugf("sending offer with serial: %s", offer.SessionIDString()) + h.log.Debugf("sending offer with serial: %s, relay server: %s, relay IP: %s", offer.SessionIDString(), offer.RelaySrvAddress, offer.RelaySrvIP) return h.signaler.SignalOffer(offer, h.config.Key) } func (h *Handshaker) sendAnswer() error { answer := h.buildOfferAnswer() - h.log.Debugf("sending answer with serial: %s", answer.SessionIDString()) + h.log.Debugf("sending answer with serial: %s, relay server: %s, relay IP: %s", answer.SessionIDString(), answer.RelaySrvAddress, answer.RelaySrvIP) return h.signaler.SignalAnswer(answer, h.config.Key) } diff --git a/shared/relay/client/client.go b/shared/relay/client/client.go index 38c9c7375..7171b40ad 100644 --- a/shared/relay/client/client.go +++ b/shared/relay/client/client.go @@ -279,7 +279,7 @@ func (c *Client) Connect(ctx context.Context) error { c.stateSubscription = NewPeersStateSubscription(c.log, c.relayConn, c.closeConnsByPeerID) c.log = c.log.WithField("relay", instanceURL.String()) - c.log.Infof("relay connection established") + c.log.Infof("relay connection established, server IP: %s", connectedIP(c.relayConn)) c.serviceIsRunning = true @@ -364,23 +364,6 @@ func (c *Client) ServerInstanceURL() (string, error) { return c.instanceURL.String(), nil } -// ConnectedIP returns the IP address of the live relay-server connection, -// extracted from the underlying socket's RemoteAddr. Zero value if not -// connected or if the address is not an IP literal. -func (c *Client) ConnectedIP() netip.Addr { - c.mu.Lock() - conn := c.relayConn - c.mu.Unlock() - if conn == nil { - return netip.Addr{} - } - addr := conn.RemoteAddr() - if addr == nil { - return netip.Addr{} - } - return extractIPLiteral(addr.String()) -} - // SetOnDisconnectListener sets a function that will be called when the connection to the relay server is closed. func (c *Client) SetOnDisconnectListener(fn func(string)) { c.listenerMutex.Lock() @@ -777,6 +760,17 @@ func (c *Client) listenForStopEvents(ctx context.Context, hc *healthcheck.Receiv } } +func (c *Client) serverInstanceAddress() (string, netip.Addr, error) { + c.mu.Lock() + defer c.mu.Unlock() + + addr, err := c.ServerInstanceURL() + if err != nil { + return "", netip.Addr{}, err + } + return addr, connectedIP(c.relayConn), nil +} + func (c *Client) closeAllConns() { for _, container := range c.conns { container.close() @@ -923,6 +917,17 @@ func (c *Client) handlePeersWentOfflineMsg(buf []byte) { c.stateSubscription.OnPeersWentOffline(peersID) } +func connectedIP(conn net.Conn) netip.Addr { + if conn == nil { + return netip.Addr{} + } + addr := conn.RemoteAddr() + if addr == nil { + return netip.Addr{} + } + return extractIPLiteral(addr.String()) +} + // extractIPLiteral returns the IP from address forms produced by the relay // dialers (URL or host:port). Zero value if the host is not an IP. func extractIPLiteral(s string) netip.Addr { diff --git a/shared/relay/client/client_serverip_test.go b/shared/relay/client/client_serverip_test.go index 7e699e37d..a52d434f7 100644 --- a/shared/relay/client/client_serverip_test.go +++ b/shared/relay/client/client_serverip_test.go @@ -8,6 +8,8 @@ import ( "testing" "time" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" "go.opentelemetry.io/otel" "github.com/netbirdio/netbird/client/iface" @@ -68,18 +70,17 @@ func TestClient_ServerIPRecoversFromUnresolvableFQDN(t *testing.T) { if !c.Ready() { t.Fatalf("client not ready after connect") } - if got := c.ConnectedIP(); got.String() != "127.0.0.1" { - t.Fatalf("ConnectedIP = %q, want 127.0.0.1", got) - } + url, ip, err := c.serverInstanceAddress() + require.NoError(t, err) + assert.Equal(t, srvCfg.ExposedAddress, url, "relay URL must come from the handshake") + assert.Equal(t, netip.MustParseAddr("127.0.0.1"), ip, "relay IP must come from the connection") }) } -// TestClient_ConnectedIPAfterFQDNDial verifies ConnectedIP returns the -// resolved IP after a successful FQDN-based dial. The underlying socket's -// RemoteAddr must be exposed through the dialer wrappers; if it returns -// the dial-time URL instead, ConnectedIP returns empty and the dial -// IP we advertise to peers is empty too. -func TestClient_ConnectedIPAfterFQDNDial(t *testing.T) { +// TestClient_ServerInstanceAddressAfterFQDNDial verifies the relay address +// includes the resolved IP after an FQDN dial. The dialer wrappers must expose +// the socket's RemoteAddr; returning the dial-time URL would lose the IP. +func TestClient_ServerInstanceAddressAfterFQDNDial(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() @@ -111,10 +112,10 @@ func TestClient_ConnectedIPAfterFQDNDial(t *testing.T) { } t.Cleanup(func() { _ = c.Close() }) - got := c.ConnectedIP().String() - if got != "127.0.0.1" && got != "::1" { - t.Fatalf("ConnectedIP after FQDN dial = %q, want 127.0.0.1 or ::1", got) - } + url, ip, err := c.serverInstanceAddress() + require.NoError(t, err) + assert.Equal(t, srvCfg.ExposedAddress, url, "relay URL must come from the handshake") + assert.Contains(t, []string{"127.0.0.1", "::1"}, ip.String(), "relay IP must resolve to localhost") } func TestSubstituteHost(t *testing.T) { @@ -214,15 +215,12 @@ func TestSubstituteHost(t *testing.T) { } } -func TestClient_ConnectedIPEmptyWhenNotConnected(t *testing.T) { - c := NewClient("rel://example.invalid:80", hmacTokenStore, "x", iface.DefaultMTU) - if got := c.ConnectedIP(); got.IsValid() { - t.Fatalf("ConnectedIP on disconnected client = %q, want zero", got) - } +func TestConnectedIPNilConnection(t *testing.T) { + assert.False(t, connectedIP(nil).IsValid(), "missing connection must not provide an IP") } // staticAddr is a net.Addr that returns a fixed string. Used to verify -// ConnectedIP parses RemoteAddr correctly. +// connectedIP parses RemoteAddr correctly. type staticAddr struct{ s string } func (a staticAddr) Network() string { return "tcp" } @@ -235,7 +233,7 @@ type stubConn struct { func (s stubConn) RemoteAddr() net.Addr { return s.remote } -func TestClient_ConnectedIPParsesRemoteAddr(t *testing.T) { +func TestConnectedIPParsesRemoteAddr(t *testing.T) { tests := []struct { name string s string @@ -252,15 +250,12 @@ func TestClient_ConnectedIPParsesRemoteAddr(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - c := &Client{relayConn: stubConn{remote: staticAddr{s: tt.s}}} - got := c.ConnectedIP() + got := connectedIP(stubConn{remote: staticAddr{s: tt.s}}) var gotStr string if got.IsValid() { gotStr = got.String() } - if gotStr != tt.want { - t.Errorf("ConnectedIP(%q) = %q, want %q", tt.s, gotStr, tt.want) - } + assert.Equal(t, tt.want, gotStr, "IP extracted from RemoteAddr %q", tt.s) }) } } diff --git a/shared/relay/client/manager.go b/shared/relay/client/manager.go index 50fcc0b8f..367c6dfc5 100644 --- a/shared/relay/client/manager.go +++ b/shared/relay/client/manager.go @@ -256,11 +256,7 @@ func (m *Manager) RelayInstanceAddress() (string, netip.Addr, error) { if m.relayClient == nil { return "", netip.Addr{}, ErrRelayClientNotConnected } - addr, err := m.relayClient.ServerInstanceURL() - if err != nil { - return "", netip.Addr{}, err - } - return addr, m.relayClient.ConnectedIP(), nil + return m.relayClient.serverInstanceAddress() } // ServerURLs returns the addresses of the relay servers. diff --git a/shared/relay/client/manager_address_test.go b/shared/relay/client/manager_address_test.go new file mode 100644 index 000000000..4f669e60d --- /dev/null +++ b/shared/relay/client/manager_address_test.go @@ -0,0 +1,103 @@ +package client + +import ( + "net/netip" + "runtime" + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestManager_RelayInstanceAddressAcrossReconnect(t *testing.T) { + relays := []struct { + url *RelayAddr + conn stubConn + ip netip.Addr + }{ + { + url: &RelayAddr{addr: "rels://relay-a.example:443"}, + conn: stubConn{remote: staticAddr{s: "192.0.2.1:443"}}, + ip: netip.MustParseAddr("192.0.2.1"), + }, + { + url: &RelayAddr{addr: "rels://relay-b.example:443"}, + conn: stubConn{remote: staticAddr{s: "192.0.2.2:443"}}, + ip: netip.MustParseAddr("192.0.2.2"), + }, + } + c := &Client{ + instanceURL: relays[0].url, + relayConn: relays[0].conn, + serviceIsRunning: true, + } + m := &Manager{relayClient: c} + started := make(chan struct{}) + stop := make(chan struct{}) + done := make(chan struct{}) + t.Cleanup(func() { + close(stop) + <-done + }) + go func() { + defer close(done) + for i := 0; ; i++ { + select { + case <-stop: + return + default: + } + // Publish successive connection states using the lifecycle locks. + // Yield before publication so a getter using only muInstanceURL + // can read the old URL while waiting for the new connection's IP. + c.mu.Lock() + runtime.Gosched() + relay := relays[i%len(relays)] + c.muInstanceURL.Lock() + c.instanceURL = relay.url + c.muInstanceURL.Unlock() + c.relayConn = relay.conn + c.mu.Unlock() + if i == 0 { + close(started) + } + } + }() + <-started + + for range 1000 { + url, ip, err := m.RelayInstanceAddress() + require.NoError(t, err) + wantIP := relays[0].ip + if url == relays[1].url.String() { + wantIP = relays[1].ip + } + if !assert.Equal(t, wantIP, ip, "advertised IP must belong to relay %s", url) { + return + } + } +} + +func TestManager_RelayInstanceAddressDisconnected(t *testing.T) { + for _, tt := range []struct { + name string + client *Client + }{ + {name: "no client"}, + {name: "not connected", client: &Client{}}, + { + name: "closed connection", + client: &Client{ + relayConn: stubConn{remote: staticAddr{s: "192.0.2.1:443"}}, + }, + }, + } { + t.Run(tt.name, func(t *testing.T) { + m := &Manager{relayClient: tt.client} + url, ip, err := m.RelayInstanceAddress() + assert.Error(t, err) + assert.Empty(t, url, "disconnected relay must not advertise a URL") + assert.False(t, ip.IsValid(), "disconnected relay must not advertise a stale IP") + }) + } +} diff --git a/shared/relay/client/picker.go b/shared/relay/client/picker.go index 17b1390b1..fc1d8c1cb 100644 --- a/shared/relay/client/picker.go +++ b/shared/relay/client/picker.go @@ -63,7 +63,12 @@ func (sp *ServerPicker) PickServer(parentCtx context.Context) (*Client, error) { if !ok { return nil, <-errChan } - log.Infof("chosen home Relay server: %s", cr.Url) + instanceURL, serverIP, err := cr.RelayClient.serverInstanceAddress() + if err != nil { + log.Infof("chosen home Relay server: %s, instance address unavailable: %v", cr.Url, err) + return cr.RelayClient, nil + } + log.Infof("chosen home Relay server: %s, instance URL: %s, server IP: %s", cr.Url, instanceURL, serverIP) return cr.RelayClient, nil case <-ctx.Done(): return nil, fmt.Errorf("connect to relay server: %w", ctx.Err()) From f422c4165437679a1470893e7917a8ef597a2277 Mon Sep 17 00:00:00 2001 From: dmitri-netbird Date: Fri, 11 Sep 2026 17:07:26 +0200 Subject: [PATCH 10/15] [management] extract peer update logic and wrap it in tests (#7338) * extract peer update loop into a dedicated struct and wrap it in tests Signed-off-by: Dmitri Dolguikh * make linter happy Signed-off-by: Dmitri Dolguikh --------- Signed-off-by: Dmitri Dolguikh --- encryption/message.go | 10 ++ .../shared/grpc/peer_update_handler.go | 135 +++++++++++++++ .../shared/grpc/peer_update_handler_test.go | 155 ++++++++++++++++++ management/internals/shared/grpc/server.go | 88 +--------- .../internals/shared/grpc/sync_sender_mock.go | 70 ++++++++ management/internals/shared/grpc/token_mgr.go | 2 + .../internals/shared/grpc/token_mgr_mock.go | 111 +++++++++++++ .../internals/shared/grpc/update_debouncer.go | 8 + .../shared/grpc/update_debouncer_mock.go | 96 +++++++++++ 9 files changed, 589 insertions(+), 86 deletions(-) create mode 100644 management/internals/shared/grpc/peer_update_handler.go create mode 100644 management/internals/shared/grpc/peer_update_handler_test.go create mode 100644 management/internals/shared/grpc/sync_sender_mock.go create mode 100644 management/internals/shared/grpc/token_mgr_mock.go create mode 100644 management/internals/shared/grpc/update_debouncer_mock.go diff --git a/encryption/message.go b/encryption/message.go index 6e4cd7391..2bf2c59dc 100644 --- a/encryption/message.go +++ b/encryption/message.go @@ -6,6 +6,16 @@ import ( "golang.zx2c4.com/wireguard/wgctrl/wgtypes" ) +type Encrypter interface { + EncryptMessage(remotePubKey wgtypes.Key, ourPrivateKey wgtypes.Key, message pb.Message) ([]byte, error) +} + +type DefaultEncrypter struct{} + +func (e DefaultEncrypter) EncryptMessage(remotePubKey wgtypes.Key, ourPrivateKey wgtypes.Key, message pb.Message) ([]byte, error) { + return EncryptMessage(remotePubKey, ourPrivateKey, message) +} + // EncryptMessage encrypts a body of the given protobuf Message func EncryptMessage(remotePubKey wgtypes.Key, ourPrivateKey wgtypes.Key, message pb.Message) ([]byte, error) { byteResp, err := pb.Marshal(message) diff --git a/management/internals/shared/grpc/peer_update_handler.go b/management/internals/shared/grpc/peer_update_handler.go new file mode 100644 index 000000000..d2f403841 --- /dev/null +++ b/management/internals/shared/grpc/peer_update_handler.go @@ -0,0 +1,135 @@ +package grpc + +import ( + "context" + "time" + + "github.com/netbirdio/netbird/encryption" + "github.com/netbirdio/netbird/management/internals/controllers/network_map" + "github.com/netbirdio/netbird/management/server/telemetry" + "github.com/netbirdio/netbird/shared/management/proto" + log "github.com/sirupsen/logrus" + "golang.zx2c4.com/wireguard/wgctrl/wgtypes" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" +) + +func PeerUpdateHandlerFactory( + peerKey wgtypes.Key, + updates chan *network_map.UpdateMessage, + secretsManager SecretsManager, + srv proto.ManagementService_SyncServer, + cleanupfunc func()) *PeerUpdateHandler { + return &PeerUpdateHandler{ + peerKey: peerKey, + updates: updates, + secretsManager: secretsManager, + srv: srv, + encrypter: encryption.DefaultEncrypter{}, + debouncer: NewUpdateDebouncer(1000 * time.Millisecond), + cleanupFunc: cleanupfunc, + } +} + +// PeerUpdateHandler sends updates to the connected peer until the updates channel is closed. +// It implements a backpressure mechanism that sends the first update immediately, +// then debounces subsequent rapid updates, ensuring only the latest update is sent +// after a quiet period. +type PeerUpdateHandler struct { + peerKey wgtypes.Key + updates chan *network_map.UpdateMessage + appMetrics telemetry.AppMetrics + secretsManager SecretsManager + srv syncSender + encrypter encryption.Encrypter + debouncer Debouncer + cleanupFunc func() +} + +func (pu *PeerUpdateHandler) WithMetrics(appMetrics telemetry.AppMetrics) *PeerUpdateHandler { + pu.appMetrics = appMetrics + return pu +} + +//go:generate go tool mockgen -source=./peer_update_handler.go -destination=./sync_sender_mock.go -package=grpc +type syncSender interface { + Send(*proto.EncryptedMessage) error + Context() context.Context +} + +func (pu *PeerUpdateHandler) HandleUpdates(ctx context.Context) error { + log.WithContext(ctx).Tracef("starting to handle updates for peer %s", pu.peerKey.String()) + + defer pu.debouncer.Stop() + + for { + select { + // condition when there are some updates + // todo set the updates channel size to 1 + case update, open := <-pu.updates: + if pu.appMetrics != nil { + pu.appMetrics.GRPCMetrics().UpdateChannelQueueLength(len(pu.updates) + 1) + } + + if !open { + log.WithContext(ctx).Debugf("updates channel for peer %s was closed", pu.peerKey.String()) + pu.cleanupFunc() + return nil + } + + log.WithContext(ctx).Tracef("received an update for peer %s", pu.peerKey.String()) + if pu.debouncer.ProcessUpdate(update) { + // Send immediately (first update or after quiet period) + if err := pu.SendUpdate(ctx, update); err != nil { + log.WithContext(ctx).Debugf("error while sending an update to peer %s: %v", pu.peerKey.String(), err) + return err + } + } + + // Timer expired - quiet period reached, send pending updates if any + case <-pu.debouncer.TimerChannel(): + pendingUpdates := pu.debouncer.GetPendingUpdates() + if len(pendingUpdates) == 0 { + continue + } + log.WithContext(ctx).Debugf("sending %d debounced update(s) for peer %s", len(pendingUpdates), pu.peerKey.String()) + for _, pendingUpdate := range pendingUpdates { + if err := pu.SendUpdate(ctx, pendingUpdate); err != nil { + log.WithContext(ctx).Debugf("error while sending an update to peer %s: %v", pu.peerKey.String(), err) + return err + } + } + + // condition when client <-> server connection has been terminated + case <-pu.srv.Context().Done(): + // happens when connection drops, e.g. client disconnects + log.WithContext(ctx).Debugf("stream of peer %s has been closed", pu.peerKey.String()) + pu.cleanupFunc() + return pu.srv.Context().Err() + } + } +} + +func (pu *PeerUpdateHandler) SendUpdate(ctx context.Context, update *network_map.UpdateMessage) error { + key, err := pu.secretsManager.GetWGKey() + if err != nil { + pu.cleanupFunc() + return status.Errorf(codes.Internal, "failed processing update message") + } + + encryptedResp, err := pu.encrypter.EncryptMessage(pu.peerKey, key, update.Update) + if err != nil { + pu.cleanupFunc() + return status.Errorf(codes.Internal, "failed processing update message") + } + err = pu.srv.Send(&proto.EncryptedMessage{ + WgPubKey: key.PublicKey().String(), + Body: encryptedResp, + }) + if err != nil { + pu.cleanupFunc() + return status.Errorf(codes.Internal, "failed sending update message") + } + log.WithContext(ctx).Tracef("sent an update to peer %s", pu.peerKey.String()) + return nil +} diff --git a/management/internals/shared/grpc/peer_update_handler_test.go b/management/internals/shared/grpc/peer_update_handler_test.go new file mode 100644 index 000000000..02de49c47 --- /dev/null +++ b/management/internals/shared/grpc/peer_update_handler_test.go @@ -0,0 +1,155 @@ +package grpc + +import ( + "context" + "fmt" + "sync" + "testing" + "time" + + pb "github.com/golang/protobuf/proto" //nolint + "github.com/netbirdio/netbird/management/internals/controllers/network_map" + "github.com/netbirdio/netbird/shared/management/proto" + "github.com/stretchr/testify/assert" + "go.uber.org/mock/gomock" + "golang.zx2c4.com/wireguard/wgctrl/wgtypes" +) + +func TestSendPeerUpdates_FirstUpdate(t *testing.T) { + ctrl := gomock.NewController(t) + secretsManager := NewMockSecretsManager(ctrl) + updateDebouncer := NewMockDebouncer(ctrl) + syncSender := NewMocksyncSender(ctrl) + + pu := PeerUpdateHandler{ + peerKey: mustGenerateKey(t), + updates: make(chan *network_map.UpdateMessage), + secretsManager: secretsManager, + encrypter: testEncrypter{}, + debouncer: updateDebouncer, + srv: syncSender, + cleanupFunc: func() {}, + } + + msg := network_map.UpdateMessage{ + Update: &proto.SyncResponse{Version: 1}, + } + + timeCh := make(chan time.Time) + srvCtx := context.TODO() + srvKey := mustGenerateKey(t) + // mock a first update, should send it right away + updateDebouncer.EXPECT().ProcessUpdate(gomock.Eq(&msg)).Return(true) + updateDebouncer.EXPECT().TimerChannel().AnyTimes().Return(timeCh) + syncSender.EXPECT().Context().AnyTimes().Return(srvCtx) + secretsManager.EXPECT().GetWGKey().Return(srvKey, nil) + syncSender.EXPECT().Send(pbMatcher{x: &proto.EncryptedMessage{WgPubKey: srvKey.PublicKey().String(), Body: mustMarshal(t, &msg)}}) + updateDebouncer.EXPECT().Stop() + + var wg sync.WaitGroup + wg.Go(func() { pu.HandleUpdates(context.TODO()) }) //nolint:errcheck + pu.updates <- &msg + close(pu.updates) + wg.Wait() +} + +func TestSendPeerUpdates_TimerUpdate(t *testing.T) { + ctrl := gomock.NewController(t) + secretsManager := NewMockSecretsManager(ctrl) + updateDebouncer := NewMockDebouncer(ctrl) + syncSender := NewMocksyncSender(ctrl) + + pu := PeerUpdateHandler{ + peerKey: mustGenerateKey(t), + updates: make(chan *network_map.UpdateMessage), + secretsManager: secretsManager, + encrypter: testEncrypter{}, + debouncer: updateDebouncer, + srv: syncSender, + cleanupFunc: func() {}, + } + + msg := network_map.UpdateMessage{ + Update: &proto.SyncResponse{Version: 1}, + } + + timeCh := make(chan time.Time) + srvCtx := context.TODO() + srvKey := mustGenerateKey(t) + updateDebouncer.EXPECT().GetPendingUpdates().Return([]*network_map.UpdateMessage{&msg}) + updateDebouncer.EXPECT().TimerChannel().AnyTimes().Return(timeCh) + syncSender.EXPECT().Context().AnyTimes().Return(srvCtx) + secretsManager.EXPECT().GetWGKey().Return(srvKey, nil) + syncSender.EXPECT().Send(pbMatcher{x: &proto.EncryptedMessage{WgPubKey: srvKey.PublicKey().String(), Body: mustMarshal(t, &msg)}}) + updateDebouncer.EXPECT().Stop() + + var wg sync.WaitGroup + wg.Go(func() { pu.HandleUpdates(context.TODO()) }) //nolint:errcheck + timeCh <- time.Now() + close(pu.updates) + wg.Wait() +} + +func TestSendPeerUpdates_ServerContextDone(t *testing.T) { + ctrl := gomock.NewController(t) + secretsManager := NewMockSecretsManager(ctrl) + updateDebouncer := NewMockDebouncer(ctrl) + syncSender := NewMocksyncSender(ctrl) + + pu := PeerUpdateHandler{ + peerKey: mustGenerateKey(t), + updates: make(chan *network_map.UpdateMessage), + secretsManager: secretsManager, + encrypter: testEncrypter{}, + debouncer: updateDebouncer, + srv: syncSender, + cleanupFunc: func() {}, + } + + timeCh := make(chan time.Time) + srvCtx, cancel := context.WithCancel(context.TODO()) + updateDebouncer.EXPECT().TimerChannel().AnyTimes().Return(timeCh) + syncSender.EXPECT().Context().AnyTimes().Return(srvCtx) + updateDebouncer.EXPECT().Stop() + + var wg sync.WaitGroup + wg.Go(func() { pu.HandleUpdates(context.TODO()) }) //nolint:errcheck + cancel() + wg.Wait() +} + +func mustGenerateKey(t *testing.T) wgtypes.Key { + t.Helper() + k, err := wgtypes.GenerateKey() + assert.NoError(t, err) + return k +} + +func mustMarshal(t *testing.T, msg *network_map.UpdateMessage) []byte { + t.Helper() + r, err := pb.Marshal(msg.Update) + assert.NoError(t, err) + return r +} + +type testEncrypter struct{} + +func (testEncrypter) EncryptMessage(remotePubKey wgtypes.Key, ourPrivateKey wgtypes.Key, message pb.Message) ([]byte, error) { + return pb.Marshal(message) +} + +type pbMatcher struct { + x pb.Message +} + +func (pbm pbMatcher) Matches(x any) bool { + msg, ok := x.(pb.Message) + if !ok { + return false + } + return pb.Equal(pbm.x, msg) +} + +func (pbm pbMatcher) String() string { + return fmt.Sprintf("is equal to %s (%T)", pbm.x, pbm.x) +} diff --git a/management/internals/shared/grpc/server.go b/management/internals/shared/grpc/server.go index a9cc0ad36..c178b6fa1 100644 --- a/management/internals/shared/grpc/server.go +++ b/management/internals/shared/grpc/server.go @@ -337,7 +337,8 @@ func (s *Server) Sync(req *proto.EncryptedMessage, srv proto.ManagementService_S s.syncSem.Add(-1) - return s.handleUpdates(ctx, accountID, peerKey, peer, updates, srv, syncStart) + return PeerUpdateHandlerFactory(peerKey, updates, s.secretsManager, srv, func() { s.cancelPeerRoutines(ctx, accountID, peer, syncStart) }). + WithMetrics(s.appMetrics).HandleUpdates(ctx) } func (s *Server) handleHandshake(ctx context.Context, srv proto.ManagementService_JobServer) (wgtypes.Key, error) { @@ -404,91 +405,6 @@ func (s *Server) sendJobsLoop(ctx context.Context, accountID string, peerKey wgt } } -// handleUpdates sends updates to the connected peer until the updates channel is closed. -// It implements a backpressure mechanism that sends the first update immediately, -// then debounces subsequent rapid updates, ensuring only the latest update is sent -// after a quiet period. -func (s *Server) handleUpdates(ctx context.Context, accountID string, peerKey wgtypes.Key, peer *nbpeer.Peer, updates chan *network_map.UpdateMessage, srv proto.ManagementService_SyncServer, streamStartTime time.Time) error { - log.WithContext(ctx).Tracef("starting to handle updates for peer %s", peerKey.String()) - - // Create a debouncer for this peer connection - debouncer := NewUpdateDebouncer(1000 * time.Millisecond) - defer debouncer.Stop() - - for { - select { - // condition when there are some updates - // todo set the updates channel size to 1 - case update, open := <-updates: - if s.appMetrics != nil { - s.appMetrics.GRPCMetrics().UpdateChannelQueueLength(len(updates) + 1) - } - - if !open { - log.WithContext(ctx).Debugf("updates channel for peer %s was closed", peerKey.String()) - s.cancelPeerRoutines(ctx, accountID, peer, streamStartTime) - return nil - } - - log.WithContext(ctx).Tracef("received an update for peer %s", peerKey.String()) - if debouncer.ProcessUpdate(update) { - // Send immediately (first update or after quiet period) - if err := s.sendUpdate(ctx, accountID, peerKey, peer, update, srv, streamStartTime); err != nil { - log.WithContext(ctx).Debugf("error while sending an update to peer %s: %v", peerKey.String(), err) - return err - } - } - - // Timer expired - quiet period reached, send pending updates if any - case <-debouncer.TimerChannel(): - pendingUpdates := debouncer.GetPendingUpdates() - if len(pendingUpdates) == 0 { - continue - } - log.WithContext(ctx).Debugf("sending %d debounced update(s) for peer %s", len(pendingUpdates), peerKey.String()) - for _, pendingUpdate := range pendingUpdates { - if err := s.sendUpdate(ctx, accountID, peerKey, peer, pendingUpdate, srv, streamStartTime); err != nil { - log.WithContext(ctx).Debugf("error while sending an update to peer %s: %v", peerKey.String(), err) - return err - } - } - - // condition when client <-> server connection has been terminated - case <-srv.Context().Done(): - // happens when connection drops, e.g. client disconnects - log.WithContext(ctx).Debugf("stream of peer %s has been closed", peerKey.String()) - s.cancelPeerRoutines(ctx, accountID, peer, streamStartTime) - return srv.Context().Err() - } - } -} - -// sendUpdate encrypts the update message using the peer key and the server's wireguard key, -// then sends the encrypted message to the connected peer via the sync server. -func (s *Server) sendUpdate(ctx context.Context, accountID string, peerKey wgtypes.Key, peer *nbpeer.Peer, update *network_map.UpdateMessage, srv proto.ManagementService_SyncServer, streamStartTime time.Time) error { - key, err := s.secretsManager.GetWGKey() - if err != nil { - s.cancelPeerRoutines(ctx, accountID, peer, streamStartTime) - return status.Errorf(codes.Internal, "failed processing update message") - } - - encryptedResp, err := encryption.EncryptMessage(peerKey, key, update.Update) - if err != nil { - s.cancelPeerRoutines(ctx, accountID, peer, streamStartTime) - return status.Errorf(codes.Internal, "failed processing update message") - } - err = srv.Send(&proto.EncryptedMessage{ - WgPubKey: key.PublicKey().String(), - Body: encryptedResp, - }) - if err != nil { - s.cancelPeerRoutines(ctx, accountID, peer, streamStartTime) - return status.Errorf(codes.Internal, "failed sending update message") - } - log.WithContext(ctx).Tracef("sent an update to peer %s", peerKey.String()) - return nil -} - // sendJob encrypts the update message using the peer key and the server's wireguard key, // then sends the encrypted message to the connected peer via the sync server. func (s *Server) sendJob(ctx context.Context, peerKey wgtypes.Key, job *job.Event, srv proto.ManagementService_JobServer) error { diff --git a/management/internals/shared/grpc/sync_sender_mock.go b/management/internals/shared/grpc/sync_sender_mock.go new file mode 100644 index 000000000..3d1696f59 --- /dev/null +++ b/management/internals/shared/grpc/sync_sender_mock.go @@ -0,0 +1,70 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: ./peer_update_handler.go +// +// Generated by this command: +// +// mockgen -source=./peer_update_handler.go -destination=./sync_sender_mock.go -package=grpc +// + +// Package grpc is a generated GoMock package. +package grpc + +import ( + context "context" + reflect "reflect" + + proto "github.com/netbirdio/netbird/shared/management/proto" + gomock "go.uber.org/mock/gomock" +) + +// MocksyncSender is a mock of syncSender interface. +type MocksyncSender struct { + ctrl *gomock.Controller + recorder *MocksyncSenderMockRecorder + isgomock struct{} +} + +// MocksyncSenderMockRecorder is the mock recorder for MocksyncSender. +type MocksyncSenderMockRecorder struct { + mock *MocksyncSender +} + +// NewMocksyncSender creates a new mock instance. +func NewMocksyncSender(ctrl *gomock.Controller) *MocksyncSender { + mock := &MocksyncSender{ctrl: ctrl} + mock.recorder = &MocksyncSenderMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MocksyncSender) EXPECT() *MocksyncSenderMockRecorder { + return m.recorder +} + +// Context mocks base method. +func (m *MocksyncSender) Context() context.Context { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Context") + ret0, _ := ret[0].(context.Context) + return ret0 +} + +// Context indicates an expected call of Context. +func (mr *MocksyncSenderMockRecorder) Context() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Context", reflect.TypeOf((*MocksyncSender)(nil).Context)) +} + +// Send mocks base method. +func (m *MocksyncSender) Send(arg0 *proto.EncryptedMessage) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Send", arg0) + ret0, _ := ret[0].(error) + return ret0 +} + +// Send indicates an expected call of Send. +func (mr *MocksyncSenderMockRecorder) Send(arg0 any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Send", reflect.TypeOf((*MocksyncSender)(nil).Send), arg0) +} diff --git a/management/internals/shared/grpc/token_mgr.go b/management/internals/shared/grpc/token_mgr.go index fb2d83a9a..4dab5007f 100644 --- a/management/internals/shared/grpc/token_mgr.go +++ b/management/internals/shared/grpc/token_mgr.go @@ -25,6 +25,8 @@ import ( const defaultDuration = 12 * time.Hour // SecretsManager used to manage TURN and relay secrets +// +//go:generate go tool mockgen -source=./token_mgr.go -destination=./token_mgr_mock.go -package=grpc type SecretsManager interface { GenerateTurnToken() (*Token, error) GenerateRelayToken() (*Token, error) diff --git a/management/internals/shared/grpc/token_mgr_mock.go b/management/internals/shared/grpc/token_mgr_mock.go new file mode 100644 index 000000000..e7dde4e3a --- /dev/null +++ b/management/internals/shared/grpc/token_mgr_mock.go @@ -0,0 +1,111 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: ./token_mgr.go +// +// Generated by this command: +// +// mockgen -source=./token_mgr.go -destination=./token_mgr_mock.go -package=grpc +// + +// Package grpc is a generated GoMock package. +package grpc + +import ( + context "context" + reflect "reflect" + + gomock "go.uber.org/mock/gomock" + wgtypes "golang.zx2c4.com/wireguard/wgctrl/wgtypes" +) + +// MockSecretsManager is a mock of SecretsManager interface. +type MockSecretsManager struct { + ctrl *gomock.Controller + recorder *MockSecretsManagerMockRecorder + isgomock struct{} +} + +// MockSecretsManagerMockRecorder is the mock recorder for MockSecretsManager. +type MockSecretsManagerMockRecorder struct { + mock *MockSecretsManager +} + +// NewMockSecretsManager creates a new mock instance. +func NewMockSecretsManager(ctrl *gomock.Controller) *MockSecretsManager { + mock := &MockSecretsManager{ctrl: ctrl} + mock.recorder = &MockSecretsManagerMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockSecretsManager) EXPECT() *MockSecretsManagerMockRecorder { + return m.recorder +} + +// CancelRefresh mocks base method. +func (m *MockSecretsManager) CancelRefresh(peerKey string) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "CancelRefresh", peerKey) +} + +// CancelRefresh indicates an expected call of CancelRefresh. +func (mr *MockSecretsManagerMockRecorder) CancelRefresh(peerKey any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "CancelRefresh", reflect.TypeOf((*MockSecretsManager)(nil).CancelRefresh), peerKey) +} + +// GenerateRelayToken mocks base method. +func (m *MockSecretsManager) GenerateRelayToken() (*Token, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GenerateRelayToken") + ret0, _ := ret[0].(*Token) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GenerateRelayToken indicates an expected call of GenerateRelayToken. +func (mr *MockSecretsManagerMockRecorder) GenerateRelayToken() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GenerateRelayToken", reflect.TypeOf((*MockSecretsManager)(nil).GenerateRelayToken)) +} + +// GenerateTurnToken mocks base method. +func (m *MockSecretsManager) GenerateTurnToken() (*Token, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GenerateTurnToken") + ret0, _ := ret[0].(*Token) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GenerateTurnToken indicates an expected call of GenerateTurnToken. +func (mr *MockSecretsManagerMockRecorder) GenerateTurnToken() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GenerateTurnToken", reflect.TypeOf((*MockSecretsManager)(nil).GenerateTurnToken)) +} + +// GetWGKey mocks base method. +func (m *MockSecretsManager) GetWGKey() (wgtypes.Key, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetWGKey") + ret0, _ := ret[0].(wgtypes.Key) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetWGKey indicates an expected call of GetWGKey. +func (mr *MockSecretsManagerMockRecorder) GetWGKey() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetWGKey", reflect.TypeOf((*MockSecretsManager)(nil).GetWGKey)) +} + +// SetupRefresh mocks base method. +func (m *MockSecretsManager) SetupRefresh(ctx context.Context, accountID, peerKey string) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "SetupRefresh", ctx, accountID, peerKey) +} + +// SetupRefresh indicates an expected call of SetupRefresh. +func (mr *MockSecretsManagerMockRecorder) SetupRefresh(ctx, accountID, peerKey any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetupRefresh", reflect.TypeOf((*MockSecretsManager)(nil).SetupRefresh), ctx, accountID, peerKey) +} diff --git a/management/internals/shared/grpc/update_debouncer.go b/management/internals/shared/grpc/update_debouncer.go index 8af9c2656..9483007c6 100644 --- a/management/internals/shared/grpc/update_debouncer.go +++ b/management/internals/shared/grpc/update_debouncer.go @@ -6,6 +6,14 @@ import ( "github.com/netbirdio/netbird/management/internals/controllers/network_map" ) +//go:generate go tool mockgen -source=./update_debouncer.go -destination=./update_debouncer_mock.go -package=grpc +type Debouncer interface { + Stop() + TimerChannel() <-chan time.Time + ProcessUpdate(update *network_map.UpdateMessage) bool + GetPendingUpdates() []*network_map.UpdateMessage +} + // UpdateDebouncer implements a backpressure mechanism that: // - Sends the first update immediately // - Coalesces rapid subsequent network map updates (only latest matters) diff --git a/management/internals/shared/grpc/update_debouncer_mock.go b/management/internals/shared/grpc/update_debouncer_mock.go new file mode 100644 index 000000000..4dff632b7 --- /dev/null +++ b/management/internals/shared/grpc/update_debouncer_mock.go @@ -0,0 +1,96 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: ./update_debouncer.go +// +// Generated by this command: +// +// mockgen -source=./update_debouncer.go -destination=./update_debouncer_mock.go -package=grpc +// + +// Package grpc is a generated GoMock package. +package grpc + +import ( + reflect "reflect" + time "time" + + network_map "github.com/netbirdio/netbird/management/internals/controllers/network_map" + gomock "go.uber.org/mock/gomock" +) + +// MockDebouncer is a mock of Debouncer interface. +type MockDebouncer struct { + ctrl *gomock.Controller + recorder *MockDebouncerMockRecorder + isgomock struct{} +} + +// MockDebouncerMockRecorder is the mock recorder for MockDebouncer. +type MockDebouncerMockRecorder struct { + mock *MockDebouncer +} + +// NewMockDebouncer creates a new mock instance. +func NewMockDebouncer(ctrl *gomock.Controller) *MockDebouncer { + mock := &MockDebouncer{ctrl: ctrl} + mock.recorder = &MockDebouncerMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockDebouncer) EXPECT() *MockDebouncerMockRecorder { + return m.recorder +} + +// GetPendingUpdates mocks base method. +func (m *MockDebouncer) GetPendingUpdates() []*network_map.UpdateMessage { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetPendingUpdates") + ret0, _ := ret[0].([]*network_map.UpdateMessage) + return ret0 +} + +// GetPendingUpdates indicates an expected call of GetPendingUpdates. +func (mr *MockDebouncerMockRecorder) GetPendingUpdates() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetPendingUpdates", reflect.TypeOf((*MockDebouncer)(nil).GetPendingUpdates)) +} + +// ProcessUpdate mocks base method. +func (m *MockDebouncer) ProcessUpdate(update *network_map.UpdateMessage) bool { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ProcessUpdate", update) + ret0, _ := ret[0].(bool) + return ret0 +} + +// ProcessUpdate indicates an expected call of ProcessUpdate. +func (mr *MockDebouncerMockRecorder) ProcessUpdate(update any) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ProcessUpdate", reflect.TypeOf((*MockDebouncer)(nil).ProcessUpdate), update) +} + +// Stop mocks base method. +func (m *MockDebouncer) Stop() { + m.ctrl.T.Helper() + m.ctrl.Call(m, "Stop") +} + +// Stop indicates an expected call of Stop. +func (mr *MockDebouncerMockRecorder) Stop() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Stop", reflect.TypeOf((*MockDebouncer)(nil).Stop)) +} + +// TimerChannel mocks base method. +func (m *MockDebouncer) TimerChannel() <-chan time.Time { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "TimerChannel") + ret0, _ := ret[0].(<-chan time.Time) + return ret0 +} + +// TimerChannel indicates an expected call of TimerChannel. +func (mr *MockDebouncerMockRecorder) TimerChannel() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "TimerChannel", reflect.TypeOf((*MockDebouncer)(nil).TimerChannel)) +} From ec0c36b0e7b34fd17948ff3fcd7c77d50104750e Mon Sep 17 00:00:00 2001 From: Brandon Hopkins <76761586+TechHutTV@users.noreply.github.com> Date: Fri, 11 Sep 2026 08:25:10 -0700 Subject: [PATCH 11/15] [client] Add light mode with system, light, and dark theme options (#7344) * desktop UI light mode * Theme review fixes plus macOS window outline fix * Windows runtime chrome re-theming plus apply serialization * Windows chrome threading and theme event ordering fixes * Darken toggle and setting sidebar text * resolve theme appearance, apply on UI thread * read theme once per window * Re-assert Windows dark opt-in after SetTheme * split app-wide GTK theming from per-window chrome * Update Wails dependency and checksums * KDE tray icon panel fix * Five review fixes: theme ordering, cgo dedup, KDE panel resolution * Path guard hardening, toggle contrast, windows comment * non-vacuous escape tests * Default view edits * Polish settings nav, controls, borders, and disc * Profiles settings boarder, modals, and buttons * Additional edits based on feedback * Switch colors away from slight blue hue * Update missing lang * Fix vertical tab active view --- client/ui/frontend/index.html | 18 + client/ui/frontend/src/app.tsx | 57 +-- .../src/assets/logos/netbird-full-light.svg | 19 + client/ui/frontend/src/components/Badge.tsx | 12 +- .../src/components/CopyToClipboard.tsx | 2 +- .../frontend/src/components/DropdownMenu.tsx | 2 +- .../src/components/LanguagePicker.tsx | 6 +- .../ui/frontend/src/components/SquareIcon.tsx | 4 +- .../frontend/src/components/ThemePicker.tsx | 109 ++++++ client/ui/frontend/src/components/Tooltip.tsx | 4 +- .../frontend/src/components/VerticalTabs.tsx | 14 +- .../src/components/buttons/Button.tsx | 49 +-- .../src/components/buttons/IconButton.tsx | 2 +- .../frontend/src/components/dialog/Dialog.tsx | 4 +- .../frontend/src/components/inputs/Input.tsx | 18 +- .../src/components/inputs/SearchInput.tsx | 2 +- .../components/switches/FancyToggleSwitch.tsx | 6 +- .../src/components/switches/SwitchItem.tsx | 6 +- .../components/switches/SwitchItemGroup.tsx | 2 +- .../src/components/switches/ToggleSwitch.tsx | 6 +- .../src/components/typography/HelpText.tsx | 2 +- .../ui/frontend/src/contexts/ThemeContext.tsx | 129 +++++++ client/ui/frontend/src/globals.css | 65 +++- .../ui/frontend/src/layouts/AppRightPanel.tsx | 2 +- client/ui/frontend/src/lib/formatters.ts | 4 +- .../main/MainConnectionStatusSwitch.tsx | 13 +- .../src/modules/main/MainExitNodeSwitcher.tsx | 6 +- .../frontend/src/modules/main/MainHeader.tsx | 4 +- .../src/modules/main/advanced/Navigation.tsx | 2 +- .../main/advanced/networks/NetworkFilters.tsx | 2 +- .../main/advanced/networks/Networks.tsx | 9 +- .../main/advanced/peers/PeerDetailPanel.tsx | 12 +- .../main/advanced/peers/PeerFilters.tsx | 2 +- .../src/modules/main/advanced/peers/Peers.tsx | 6 +- .../src/modules/profiles/ProfileDropdown.tsx | 10 +- .../src/modules/profiles/ProfilesTab.tsx | 8 +- .../src/modules/settings/SettingsAbout.tsx | 16 +- .../src/modules/settings/SettingsGeneral.tsx | 2 + .../src/modules/settings/SettingsSection.tsx | 4 +- .../settings/SettingsTroubleshooting.tsx | 13 +- client/ui/frontend/tailwind.config.ts | 47 +-- client/ui/i18n/locales/de/common.json | 15 + client/ui/i18n/locales/en/common.json | 20 + client/ui/i18n/locales/es/common.json | 15 + client/ui/i18n/locales/fr/common.json | 15 + client/ui/i18n/locales/hu/common.json | 15 + client/ui/i18n/locales/it/common.json | 15 + client/ui/i18n/locales/ja/common.json | 15 + client/ui/i18n/locales/pt/common.json | 15 + client/ui/i18n/locales/ru/common.json | 15 + client/ui/i18n/locales/uk/common.json | 19 +- client/ui/i18n/locales/zh-CN/common.json | 15 + client/ui/main.go | 11 +- client/ui/preferences/store.go | 53 ++- client/ui/services/appappearance_linux.go | 99 +++++ client/ui/services/appappearance_linux_gtk.go | 63 ++++ client/ui/services/appappearance_other.go | 7 + client/ui/services/preferences.go | 4 + client/ui/services/theme.go | 194 ++++++++++ client/ui/services/windowappearance_darwin.go | 53 +++ client/ui/services/windowappearance_other.go | 16 + .../ui/services/windowappearance_windows.go | 54 +++ client/ui/services/windowmanager.go | 123 +++++- client/ui/tray_theme_linux.go | 147 ++++++-- client/ui/tray_theme_linux_test.go | 357 ++++++++++++++++-- client/ui/tray_theme_watcher_linux.go | 51 +-- go.mod | 2 +- go.sum | 4 +- 68 files changed, 1853 insertions(+), 259 deletions(-) create mode 100644 client/ui/frontend/src/assets/logos/netbird-full-light.svg create mode 100644 client/ui/frontend/src/components/ThemePicker.tsx create mode 100644 client/ui/frontend/src/contexts/ThemeContext.tsx create mode 100644 client/ui/services/appappearance_linux.go create mode 100644 client/ui/services/appappearance_linux_gtk.go create mode 100644 client/ui/services/appappearance_other.go create mode 100644 client/ui/services/theme.go create mode 100644 client/ui/services/windowappearance_darwin.go create mode 100644 client/ui/services/windowappearance_other.go create mode 100644 client/ui/services/windowappearance_windows.go diff --git a/client/ui/frontend/index.html b/client/ui/frontend/index.html index e62139956..f4c8b3d68 100644 --- a/client/ui/frontend/index.html +++ b/client/ui/frontend/index.html @@ -6,7 +6,25 @@ NetBird +
diff --git a/client/ui/frontend/src/app.tsx b/client/ui/frontend/src/app.tsx index 7f1359510..6accda36f 100644 --- a/client/ui/frontend/src/app.tsx +++ b/client/ui/frontend/src/app.tsx @@ -13,6 +13,7 @@ import { SkeletonTheme } from "react-loading-skeleton"; import "react-loading-skeleton/dist/skeleton.css"; import { welcome } from "@/lib/welcome"; import LoginWaitingForBrowserDialog from "@/modules/login/LoginWaitingForBrowserDialog.tsx"; +import { ThemeProvider } from "@/contexts/ThemeContext.tsx"; import { initI18n } from "@/lib/i18n"; import { initPlatform } from "@/lib/platform"; import { initLogForwarding } from "@/lib/logs"; @@ -35,30 +36,38 @@ Promise.all([ ]).finally(() => { ReactDOM.createRoot(document.getElementById("root")!).render( - - - - - } - /> - } /> - } - /> - } /> - } /> - - }> - } /> - } /> - } /> - - - - + + + + + + } + /> + } + /> + } + /> + } /> + } /> + + }> + } /> + } /> + } /> + + + + + , ); }); diff --git a/client/ui/frontend/src/assets/logos/netbird-full-light.svg b/client/ui/frontend/src/assets/logos/netbird-full-light.svg new file mode 100644 index 000000000..3457b50c6 --- /dev/null +++ b/client/ui/frontend/src/assets/logos/netbird-full-light.svg @@ -0,0 +1,19 @@ + + + + + + + + + + + + + + + + + + + diff --git a/client/ui/frontend/src/components/Badge.tsx b/client/ui/frontend/src/components/Badge.tsx index c5e2b5f22..a6f2e886a 100644 --- a/client/ui/frontend/src/components/Badge.tsx +++ b/client/ui/frontend/src/components/Badge.tsx @@ -11,12 +11,14 @@ type Props = HTMLAttributes & { }; const VARIANT_CLASSES: Record = { - info: "bg-sky-900 border border-sky-700 text-sky-200", + info: "bg-sky-100 border border-sky-300 text-sky-800 dark:bg-sky-900 dark:border-sky-700 dark:text-sky-200", neutral: "bg-nb-gray-900 border border-nb-gray-850 text-nb-gray-200", - brand: "bg-netbird/15 border border-netbird/30 text-netbird", - success: "bg-green-900 border border-green-700 text-green-200", - warning: "bg-yellow-900 border border-yellow-700 text-yellow-200", - danger: "bg-red-900 border border-red-700 text-red-200", + brand: "bg-netbird/15 border border-netbird/30 text-netbird-700 dark:text-netbird", + success: + "bg-green-100 border border-green-300 text-green-800 dark:bg-green-900 dark:border-green-700 dark:text-green-200", + warning: + "bg-yellow-100 border border-yellow-300 text-yellow-800 dark:bg-yellow-900 dark:border-yellow-700 dark:text-yellow-200", + danger: "bg-red-100 border border-red-300 text-red-800 dark:bg-red-900 dark:border-red-700 dark:text-red-200", }; export const Badge = forwardRef(function Badge( diff --git a/client/ui/frontend/src/components/CopyToClipboard.tsx b/client/ui/frontend/src/components/CopyToClipboard.tsx index 3cf681a1c..4af4ecc8f 100644 --- a/client/ui/frontend/src/components/CopyToClipboard.tsx +++ b/client/ui/frontend/src/components/CopyToClipboard.tsx @@ -81,7 +81,7 @@ export const CopyToClipboard = ({ aria-live={"polite"} className={cn( "group/copy wails-no-draggable pointer-events-auto inline-flex cursor-default items-center gap-2 rounded-sm text-left outline-none", - "focus-visible:ring-2 focus-visible:ring-white/60 focus-visible:ring-offset-2 focus-visible:ring-offset-nb-gray-940", + "focus-visible:ring-2 focus-visible:ring-nb-gray-50/60 focus-visible:ring-offset-2 focus-visible:ring-offset-nb-gray-940", className, )} > diff --git a/client/ui/frontend/src/components/DropdownMenu.tsx b/client/ui/frontend/src/components/DropdownMenu.tsx index d43c37e1b..8cedcea03 100644 --- a/client/ui/frontend/src/components/DropdownMenu.tsx +++ b/client/ui/frontend/src/components/DropdownMenu.tsx @@ -16,7 +16,7 @@ const menuItemVariants = cva("", { variant: { default: "text-nb-gray-200 hover:bg-nb-gray-900 hover:text-nb-gray-50 focus-visible:bg-nb-gray-900 focus-visible:text-nb-gray-50 data-[state=open]:bg-nb-gray-900 data-[state=open]:text-nb-gray-50", - danger: "text-red-500 hover:bg-red-900/20 hover:text-red-500 focus-visible:bg-red-900/20 focus-visible:text-red-500", + danger: "text-red-500 hover:bg-red-500/10 hover:text-red-500 focus-visible:bg-red-500/10 focus-visible:text-red-500 dark:hover:bg-red-900/20 dark:focus-visible:bg-red-900/20", }, }, defaultVariants: { variant: "default" }, diff --git a/client/ui/frontend/src/components/LanguagePicker.tsx b/client/ui/frontend/src/components/LanguagePicker.tsx index 7a30f8b33..35ef7d5b5 100644 --- a/client/ui/frontend/src/components/LanguagePicker.tsx +++ b/client/ui/frontend/src/components/LanguagePicker.tsx @@ -97,9 +97,9 @@ export function LanguagePicker() { "rounded-md border bg-white dark:bg-nb-gray-900", "border-neutral-200 dark:border-nb-gray-700", "cursor-default text-xs font-semibold text-nb-gray-100 outline-none", - "hover:border-nb-gray-600 data-[state=open]:border-nb-gray-600", + "hover:border-nb-gray-700 data-[state=open]:border-nb-gray-700 dark:hover:border-nb-gray-600 dark:data-[state=open]:border-nb-gray-600", isFocusVisible && - "focus-visible:ring-2 focus-visible:ring-white/60 focus-visible:ring-offset-2 focus-visible:ring-offset-nb-gray-940", + "focus-visible:ring-2 focus-visible:ring-nb-gray-50/60 focus-visible:ring-offset-2 focus-visible:ring-offset-nb-gray-940", "disabled:opacity-50", )} > @@ -157,7 +157,7 @@ export function LanguagePicker() { placeholder={t("settings.general.language.search")} aria-label={t("settings.general.language.search")} className={cn( - "w-full bg-transparent text-xs text-nb-gray-100 placeholder:text-nb-gray-300", + "w-full bg-transparent text-xs text-nb-gray-100 placeholder:text-nb-gray-600 dark:placeholder:text-nb-gray-300", "border-none outline-none", )} /> diff --git a/client/ui/frontend/src/components/SquareIcon.tsx b/client/ui/frontend/src/components/SquareIcon.tsx index e904d2de5..aaf3b1100 100644 --- a/client/ui/frontend/src/components/SquareIcon.tsx +++ b/client/ui/frontend/src/components/SquareIcon.tsx @@ -5,7 +5,7 @@ import { cn } from "@/lib/cn"; export type SquareIconVariant = "default" | "info" | "warning" | "danger"; const variantClass: Record = { - default: "text-white", + default: "text-nb-gray-50", info: "text-sky-400", warning: "text-netbird", danger: "text-red-500", @@ -27,7 +27,7 @@ export const SquareIcon = ({
o.value === theme) ?? OPTIONS[0]; + const CurrentIcon = current.icon; + + const select = async (value: string) => { + if (busy || value === theme) return; + setBusy(true); + try { + await setTheme(value as ThemePreference); + } catch (e) { + await errorDialog({ + Title: t("settings.error.saveTitle"), + Message: formatErrorMessage(e), + }); + } finally { + setBusy(false); + } + }; + + return ( +
+
+ + {t("settings.general.theme.help")} +
+
+ + + + + + void select(v)}> + {OPTIONS.map(({ value, icon: Icon, labelKey }) => ( + + + {t(labelKey)} + + ))} + + + +
+
+ ); +} diff --git a/client/ui/frontend/src/components/Tooltip.tsx b/client/ui/frontend/src/components/Tooltip.tsx index 2c77ba139..d7a85277a 100644 --- a/client/ui/frontend/src/components/Tooltip.tsx +++ b/client/ui/frontend/src/components/Tooltip.tsx @@ -81,12 +81,12 @@ export const Tooltip = ({ onPointerLeave={interactive ? scheduleClose : undefined} onPointerDownOutside={interactive ? undefined : (e) => e.preventDefault()} className={cn( - "z-50 select-none text-xs text-nb-gray-100 shadow-lg", + "z-50 select-none text-xs text-nb-gray-100 shadow-sm dark:shadow-lg", "data-[state=delayed-open]:animate-in data-[state=closed]:animate-out", "data-[state=closed]:fade-out-0 data-[state=delayed-open]:fade-in-0", !interactive && "pointer-events-none", contentClassName ?? - "rounded-md border border-nb-gray-850 bg-nb-gray-900 px-2 py-1", + "rounded-md border border-nb-gray-800 bg-white px-2 py-1 dark:border-nb-gray-850 dark:bg-nb-gray-900", )} > {content} diff --git a/client/ui/frontend/src/components/VerticalTabs.tsx b/client/ui/frontend/src/components/VerticalTabs.tsx index 1aedf82a6..306850ee2 100644 --- a/client/ui/frontend/src/components/VerticalTabs.tsx +++ b/client/ui/frontend/src/components/VerticalTabs.tsx @@ -46,12 +46,12 @@ const Trigger = forwardRef(function VerticalTab (function VerticalTab aria-hidden={"true"} className={cn( "ml-2 shrink-0 transition-colors duration-150", - "text-nb-gray-400 group-data-[state=active]:text-nb-gray-100", + "text-nb-gray-350 dark:text-nb-gray-400", + "group-data-[state=active]:text-nb-gray-100", )} /> {title} diff --git a/client/ui/frontend/src/components/buttons/Button.tsx b/client/ui/frontend/src/components/buttons/Button.tsx index 6b151c17b..931988ead 100644 --- a/client/ui/frontend/src/components/buttons/Button.tsx +++ b/client/ui/frontend/src/components/buttons/Button.tsx @@ -24,71 +24,74 @@ const buttonVariants = cva( variants: { variant: { default: [ - "border-gray-200 bg-white text-gray-900 hover:bg-gray-100 hover:text-black focus:ring-zinc-200/50", - "dark:border-gray-700/30 dark:bg-nb-gray dark:text-gray-400 dark:hover:bg-zinc-800/50 dark:hover:text-white dark:focus:ring-zinc-800/50", + "border-neutral-200 bg-white text-neutral-900 hover:bg-neutral-100 hover:text-black focus:ring-neutral-200/50", + "dark:border-gray-700/30 dark:bg-nb-gray dark:text-gray-400 dark:hover:bg-zinc-800/50 dark:hover:text-nb-gray-50 dark:focus:ring-zinc-800/50", ], primary: [ - "dark:text-gray-100 dark:ring-offset-neutral-950/50 dark:focus:ring-netbird-600/50 enabled:dark:bg-netbird enabled:dark:hover:bg-netbird-500/80 enabled:dark:hover:text-white disabled:dark:bg-nb-gray-900", - "enabled:bg-netbird enabled:text-white enabled:hover:bg-netbird-500 enabled:focus:ring-netbird-400/50", + "dark:text-gray-100 dark:ring-offset-neutral-950/50 dark:focus:ring-netbird-600/50 enabled:dark:bg-netbird enabled:dark:hover:bg-netbird-500/80 enabled:dark:hover:text-nb-gray-50 disabled:dark:bg-nb-gray-900", + "enabled:bg-netbird enabled:text-white enabled:hover:bg-netbird-500 enabled:focus:ring-netbird-400/50 disabled:bg-nb-gray-700", ], secondary: [ - "border-gray-200 bg-white text-gray-900 hover:bg-gray-100 hover:text-black focus:ring-zinc-200/50", - "dark:ring-offset-neutral-950/50 dark:focus:ring-neutral-500/20", - "dark:border-gray-700/40 dark:bg-nb-gray-920 dark:text-gray-400 dark:hover:bg-nb-gray-910 dark:hover:text-white", + "border-neutral-200 bg-white text-neutral-900 hover:border-nb-gray-700 hover:bg-nb-gray-950 hover:text-black focus:ring-nb-gray-500/50 focus:ring-offset-0", + "dark:ring-offset-neutral-950/50 dark:focus:ring-neutral-500/20 dark:focus:ring-offset-1", + "dark:border-gray-700/40 dark:bg-nb-gray-920 dark:text-gray-400 dark:hover:border-gray-700/40 dark:hover:bg-nb-gray-910 dark:hover:text-nb-gray-50", ], secondaryLighter: [ - "border-gray-200 bg-white text-gray-900 hover:bg-gray-100 hover:text-black focus:ring-zinc-200/50", + "border-neutral-200 bg-white text-neutral-900 hover:bg-neutral-100 hover:text-black focus:ring-neutral-200/50", "dark:ring-offset-neutral-950/50 dark:focus:ring-neutral-500/20", - "dark:border-gray-700/70 dark:bg-nb-gray-900/70 dark:text-gray-400 dark:hover:bg-nb-gray-800/60 dark:hover:text-white", + "dark:border-gray-700/70 dark:bg-nb-gray-900/70 dark:text-gray-400 dark:hover:bg-nb-gray-800/60 dark:hover:text-nb-gray-50", ], subtle: [ - "border-nb-gray-200 bg-nb-gray-50 text-nb-gray-900 hover:bg-nb-gray-100 focus:ring-nb-gray-200/60", + "border-neutral-200 bg-neutral-50 text-neutral-900 hover:bg-neutral-100 focus:ring-neutral-200/60", "dark:ring-offset-neutral-950/50 dark:focus:ring-nb-gray-200/40", "dark:border-nb-gray-200 dark:bg-nb-gray-50 dark:text-nb-gray-900 dark:hover:bg-nb-gray-100 dark:hover:text-nb-gray-950", ], input: [ - "border-neutral-200 bg-white text-gray-900 hover:bg-gray-100 hover:text-black focus:ring-zinc-200/50", + "border-neutral-200 bg-white text-neutral-900 hover:bg-neutral-100 hover:text-black focus:ring-neutral-200/50", "dark:ring-offset-neutral-950/50 dark:focus:ring-neutral-500/20", "dark:border-nb-gray-700 dark:bg-nb-gray-900 dark:text-gray-400 dark:hover:bg-nb-gray-900/80", ], dropdown: [ - "border-neutral-200 bg-white text-gray-900 hover:bg-gray-100 hover:text-black focus:ring-zinc-200/50", + "border-neutral-200 bg-white text-neutral-900 hover:bg-neutral-100 hover:text-black focus:ring-neutral-200/50", "dark:ring-offset-neutral-950/50 dark:focus:ring-neutral-500/20", "dark:border-nb-gray-900 dark:bg-nb-gray-900/40 dark:text-gray-400 dark:hover:bg-nb-gray-900/50", ], dotted: [ - "border-dashed border-gray-200 bg-white text-gray-900 hover:bg-gray-100 hover:text-black focus:ring-zinc-200/50", + "border-dashed border-neutral-200 bg-white text-neutral-900 hover:bg-neutral-100 hover:text-black focus:ring-neutral-200/50", "dark:ring-offset-neutral-950/50 dark:focus:ring-neutral-500/20", - "dark:border-gray-500/40 dark:bg-nb-gray-900/30 dark:text-gray-400 dark:hover:bg-nb-gray-900/50 dark:hover:text-white", + "dark:border-gray-500/40 dark:bg-nb-gray-900/30 dark:text-gray-400 dark:hover:bg-nb-gray-900/50 dark:hover:text-nb-gray-50", ], tertiary: [ - "border-gray-200 bg-white text-gray-900 hover:bg-gray-100 hover:text-black focus:ring-zinc-200/50", + "border-neutral-200 bg-white text-neutral-900 hover:bg-neutral-100 hover:text-black focus:ring-neutral-200/50", "dark:border-gray-700/40 dark:bg-white dark:text-gray-800 dark:hover:bg-neutral-200 dark:focus:ring-zinc-800/50 disabled:dark:bg-nb-gray-920 disabled:dark:text-nb-gray-300", ], white: [ - "border-white bg-white text-gray-800 outline-none hover:bg-neutral-200 focus:ring-white/50 disabled:dark:bg-nb-gray-920 disabled:dark:text-nb-gray-300", + "border-white bg-white text-neutral-800 outline-none hover:bg-neutral-200 focus:ring-white/50 dark:text-gray-800 disabled:dark:bg-nb-gray-920 disabled:dark:text-nb-gray-300", "disabled:dark:border-nb-gray-900 disabled:dark:bg-nb-gray-900 disabled:dark:text-nb-gray-300", ], outline: [ - "border-gray-200 bg-white text-gray-900 hover:bg-gray-100 hover:text-black focus:ring-zinc-200/50", + "border-neutral-200 bg-white text-neutral-900 hover:bg-neutral-100 hover:text-black focus:ring-neutral-200/50", "dark:border-netbird dark:bg-transparent dark:text-netbird dark:hover:bg-nb-gray-900/30 dark:focus:ring-zinc-800/50", ], "danger-outline": [ + "bg-transparent text-red-600 enabled:hover:bg-red-50 enabled:focus:ring-red-200/50", "dark:bg-transparent dark:text-red-500 enabled:dark:hover:border-red-800/50 enabled:hover:dark:bg-red-950/50 enabled:dark:focus:bg-red-950/40 enabled:dark:focus:ring-red-800/20", ], "danger-text": [ - "rounded-sm !px-0 !py-0 !shadow-none focus:ring-red-500/30 dark:border-transparent dark:bg-transparent dark:text-red-500 dark:ring-offset-neutral-950/50 dark:hover:text-red-600", + "rounded-sm border-transparent bg-transparent !px-0 !py-0 text-red-600 !shadow-none hover:text-red-700 focus:ring-red-500/30", + "dark:border-transparent dark:bg-transparent dark:text-red-500 dark:ring-offset-neutral-950/50 dark:hover:text-red-600", ], "default-outline": [ - "dark:ring-offset-nb-gray-950/50 dark:focus:ring-nb-gray-500/20", - "dark:border-transparent dark:bg-transparent dark:text-nb-gray-400 dark:hover:border-nb-gray-800/50 dark:hover:bg-nb-gray-900/30 dark:hover:text-white", - "data-[state=open]:dark:border-nb-gray-800/50 data-[state=open]:dark:bg-nb-gray-900/30 data-[state=open]:dark:text-white", + "ring-offset-nb-gray-950/50 focus:ring-nb-gray-500/20", + "border-transparent bg-transparent text-nb-gray-400 hover:border-nb-gray-800/50 hover:bg-nb-gray-900/30 hover:text-nb-gray-50", + "data-[state=open]:border-nb-gray-800/50 data-[state=open]:bg-nb-gray-900/30 data-[state=open]:text-nb-gray-50", ], ghost: [ - "dark:ring-offset-nb-gray-950/50 dark:focus:ring-nb-gray-500/20", - "dark:border-transparent dark:bg-transparent dark:text-nb-gray-400 dark:hover:bg-nb-gray-900/30 dark:hover:text-white", + "ring-offset-nb-gray-950/50 focus:ring-nb-gray-500/20", + "border-transparent bg-transparent text-nb-gray-400 hover:bg-nb-gray-900/30 hover:text-nb-gray-50", ], danger: [ + "bg-red-600 text-red-50 hover:bg-red-700 focus:bg-red-700 focus:ring-red-700/20", "dark:bg-red-600 dark:text-red-100 dark:hover:border-red-800/50 hover:dark:bg-red-700 dark:focus:bg-red-700 dark:focus:ring-red-700/20", ], }, diff --git a/client/ui/frontend/src/components/buttons/IconButton.tsx b/client/ui/frontend/src/components/buttons/IconButton.tsx index 3d36bc111..8d688cb0b 100644 --- a/client/ui/frontend/src/components/buttons/IconButton.tsx +++ b/client/ui/frontend/src/components/buttons/IconButton.tsx @@ -24,7 +24,7 @@ export const IconButton = forwardRef(function IconButt "flex h-10 w-10 cursor-default items-center justify-center rounded-lg outline-none", "text-nb-gray-400 hover:bg-nb-gray-900 hover:text-nb-gray-300", isFocusVisible && - "focus-visible:ring-2 focus-visible:ring-white/60 focus-visible:ring-offset-2 focus-visible:ring-offset-nb-gray-940", + "focus-visible:ring-2 focus-visible:ring-nb-gray-50/60 focus-visible:ring-offset-2 focus-visible:ring-offset-nb-gray-940", "wails-no-draggable transition-colors duration-150", className, )} diff --git a/client/ui/frontend/src/components/dialog/Dialog.tsx b/client/ui/frontend/src/components/dialog/Dialog.tsx index fa8007d9f..c43c04b0b 100644 --- a/client/ui/frontend/src/components/dialog/Dialog.tsx +++ b/client/ui/frontend/src/components/dialog/Dialog.tsx @@ -23,7 +23,7 @@ const Overlay = forwardRef, OverlayPr ref={ref} className={cn( "fixed inset-0 z-50 grid items-center justify-items-center overflow-y-auto px-10 py-16", - "bg-black/60", + "bg-black/25 dark:bg-black/60", "data-[state=open]:animate-in data-[state=open]:fade-in-0", exitAnimation && "data-[state=closed]:animate-out data-[state=closed]:fade-out-0", @@ -67,7 +67,7 @@ export const Content = forwardRef, Co className={cn( "relative z-[52] mx-auto w-full outline-none ring-0", "focus:outline-none focus:ring-0 focus-visible:outline-none focus-visible:ring-0", - "rounded-lg border border-nb-gray-900 bg-nb-gray py-7 shadow-2xl", + "rounded-lg border border-nb-gray-800 bg-nb-gray-940 py-7 shadow-2xl dark:border-nb-gray-900 dark:bg-nb-gray", "data-[state=open]:animate-in data-[state=open]:fade-in-0", "data-[state=open]:zoom-in-95 data-[state=open]:slide-in-from-left-1", exitAnimation && diff --git a/client/ui/frontend/src/components/inputs/Input.tsx b/client/ui/frontend/src/components/inputs/Input.tsx index 2dad80d7a..eada79a1f 100644 --- a/client/ui/frontend/src/components/inputs/Input.tsx +++ b/client/ui/frontend/src/components/inputs/Input.tsx @@ -32,19 +32,19 @@ const inputVariants = cva("", { variants: { variant: { default: [ - "border-neutral-200 placeholder:text-neutral-500 dark:border-nb-gray-700 dark:bg-nb-gray-900 dark:placeholder:text-neutral-400/70", + "border-neutral-200 placeholder:text-nb-gray-600 dark:border-nb-gray-700 dark:bg-nb-gray-900 dark:placeholder:text-neutral-400/70", "ring-offset-neutral-200/20 focus-visible:ring-neutral-300/10 dark:ring-offset-neutral-950/50 dark:focus-visible:ring-neutral-500/20", ], darker: [ - "border-neutral-300 placeholder:text-neutral-500 dark:border-nb-gray-800 dark:bg-nb-gray-920 dark:placeholder:text-neutral-400/70", + "border-neutral-300 placeholder:text-nb-gray-600 dark:border-nb-gray-800 dark:bg-nb-gray-920 dark:placeholder:text-neutral-400/70", "ring-offset-neutral-200/20 focus-visible:ring-neutral-300/10 dark:ring-offset-neutral-950/50 dark:focus-visible:ring-neutral-500/20", ], error: [ - "border-neutral-200 text-red-500 placeholder:text-neutral-500 dark:border-red-500 dark:bg-nb-gray-900 dark:placeholder:text-neutral-400/70", + "border-neutral-200 text-red-500 placeholder:text-nb-gray-600 dark:border-red-500 dark:bg-nb-gray-900 dark:placeholder:text-neutral-400/70", "ring-offset-red-500/10 focus-visible:ring-red-500/10 dark:ring-offset-red-500/10 dark:focus-visible:ring-red-500/10", ], warning: [ - "border-neutral-200 text-orange-400 placeholder:text-neutral-500 dark:border-orange-400 dark:bg-nb-gray-900 dark:placeholder:text-neutral-400/70", + "border-neutral-200 text-orange-400 placeholder:text-nb-gray-600 dark:border-orange-400 dark:bg-nb-gray-900 dark:placeholder:text-neutral-400/70", "ring-offset-orange-400/10 focus-visible:ring-orange-400/10 dark:ring-offset-orange-400/10 dark:focus-visible:ring-orange-400/10", ], }, @@ -158,7 +158,7 @@ function NumberStepper({ className={cn( "flex h-[40px] shrink-0 flex-col overflow-hidden", "rounded-r-md border border-l-0", - "border-neutral-200 dark:border-nb-gray-700 dark:bg-nb-gray-900", + "border-neutral-200 bg-white dark:border-nb-gray-700 dark:bg-nb-gray-900", error && "dark:border-red-500", disabled && "pointer-events-none opacity-40", )} @@ -274,7 +274,9 @@ export const Input = forwardRef(function Input(