[PR #6047] Phase 1: connection-mode enum + backwards-compat (issue #5989) #27416

Open
opened 2026-08-05 07:08:41 -04:00 by saavagebueno · 0 comments
Owner

Original Pull Request: https://github.com/netbirdio/netbird/pull/6047

State: closed
Merged: No


Summary

This PR is Phase 1 of three that together implement issues #5989 and #5990 (the connection-mode consolidation RFC and its server-side per-peer/per-group companion).

In Scope:

  • New ConnectionMode proto enum with four reserved values (relay-forced, p2p, p2p-lazy, p2p-dynamic); three are functional in this PR. p2p-dynamic is reserved on the wire and in the DB but the daemon currently treats it like p2p (pass-through). The decoupled worker_relay/worker_ice lifecycle that makes p2p-dynamic distinct comes in Phase 2.
  • Two new account-level settings on the management server: relay_timeout_seconds, p2p_timeout_seconds (both nullable, NULL = built-in default).
  • Replaces the asymmetric Lazy/ForceRelay precedence in client/internal/conn_mgr.go with a single resolver: client env > client config > server-pushed.
  • Full backwards-compatibility:
    • Old clients still see only the legacy lazy_connection_enabled boolean (mapped from the resolved mode by toPeerConfig).
    • Old servers send connection_mode = UNSPECIFIED (proto default 0) and the new client falls back to the legacy boolean -- verified via tests.
    • NB_FORCE_RELAY, NB_ENABLE_EXPERIMENTAL_LAZY_CONN, --enable-lazy-connection, and NB_LAZY_CONN_INACTIVITY_THRESHOLD continue to work and emit one-shot deprecation warnings.

The companion Dashboard PR is at netbirdio/dashboard#627 (will be added once it exists).

Implementation map

Layer Files
proto shared/management/proto/management.proto, client/proto/daemon.proto
Mode type shared/connectionmode/ (Mode, ParseString, FromProto, ToProto, ResolveLegacyLazyBool, ToLazyConnectionEnabled)
client env / CLI client/internal/peer/env.go (ResolveModeFromEnv), client/cmd/{root,up}.go (--connection-mode, --relay-timeout, --p2p-timeout)
client engine / conn-mgr client/internal/{engine,conn_mgr,connect}.go (resolveConnectionMode + UpdatedRemotePeerConfig + EngineConfig fields)
client per-peer client/internal/peer/conn.go (Mode-driven skip-ICE branch)
mgmt server management/server/types/settings.go (new nullable columns), management/server/http/handlers/accounts/accounts_handler.go (PUT validation + response), management/server/account.go (audit emission), management/server/activity/codes.go (3 new event codes), management/internals/shared/grpc/conversion.go (toPeerConfig writes both old and new wire fields)
OpenAPI shared/management/http/api/openapi.yml + regenerated types.gen.go
tests shared/connectionmode/mode_test.go, client/internal/peer/env_test.go, client/internal/conn_mgr_test.go, management/internals/shared/grpc/conversion_test.go

Hardware-tested

Verified on a real production NetBird instance with 32 connected peers across 12 OpenWrt-router versions (22.03 to 25.12), Windows 10/11, Debian, Android 12/14, and iOS 26.3.1:

  • Pre-deploy baseline: 32 connected.
  • After deploying the Phase-1 management image: 32 connected, no disconnect, all old clients continued to function unchanged (they read only the legacy lazy_connection_enabled boolean which toPeerConfig keeps writing via the back-compat mapping).
  • Switched the account-wide connection_mode between p2p and p2p-lazy via the API: 32 connected throughout, no disconnect.
  • 8-minute monitoring window after deploy: zero peer-count drift.

The daemon-side change is exercised end-to-end by switching the account-wide setting and observing the daemon pick up the new mode on the next NetworkMap update (handled by UpdatedRemotePeerConfig).

Known limitations (called out in spec section 8)

  1. relay-forced cannot be pushed to old clients because the legacy lazy_connection_enabled boolean cannot express it. The wire field falls back to false and old clients run in normal p2p. Workaround: upgrade the client, or set local NB_FORCE_RELAY=true.
  2. p2p-dynamic is not visible in the Dashboard dropdown in Phase 1 even though the API accepts it; setting it via API gives p2p behaviour until the Phase-2 daemon implementation lands.
  3. Audit events are account-scoped only; per-peer / per-group events follow in Phase 3 (#5990).

Phase 2 / 3 follow-ups

  • Phase 2 (separate PR): p2p-dynamic daemon implementation -- decouple worker_relay / worker_ice OnNewOffer registration, two-tier inactivity manager, plus the DeactivatePeer no-op fix in conn_mgr.go that #5989 calls out for the lazy/eager mismatch.
  • Phase 3 (separate PR, addresses #5990): per-peer / per-group resolution for connection_mode, p2p_timeout, relay_timeout with most-restrictive group-conflict resolution and per-scope audit events.

Test plan

  • All new + existing unit tests in client/internal/... and management/... green
  • TestAccount_GetPeerNetworkMap (existing) remains green -- old peers without connection_mode resolve via ResolveLegacyLazyBool exactly as before
  • TestToPeerConfig_ConnectionModeResolution (new, 9 sub-cases) covers the resolution matrix
  • TestResolveConnectionMode (new, 10 sub-cases) covers the env > config > server precedence chain
  • TestResolveModeFromEnv (new, 9 sub-cases) covers the env-var matrix including the legacy-conflict case
  • Hardware-verified against 32 mixed-version peers in production (no disconnects)

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Configurable peer connection modes (relay-forced, p2p, p2p-lazy, p2p-dynamic) via CLI flags and account settings.
    • Relay and P2P timeout overrides to tune idle behavior.
    • Activity/audit events emitted when connection-mode or timeout settings change.
  • Tests

    • Added unit tests covering mode/timeout resolution, env-var parsing, serialization round-trips, and API validation.
**Original Pull Request:** https://github.com/netbirdio/netbird/pull/6047 **State:** closed **Merged:** No --- ## Summary This PR is **Phase 1 of three** that together implement issues #5989 and #5990 (the connection-mode consolidation RFC and its server-side per-peer/per-group companion). **In Scope:** - New `ConnectionMode` proto enum with four reserved values (`relay-forced`, `p2p`, `p2p-lazy`, `p2p-dynamic`); three are functional in this PR. `p2p-dynamic` is reserved on the wire and in the DB but the daemon currently treats it like `p2p` (pass-through). The decoupled `worker_relay`/`worker_ice` lifecycle that makes `p2p-dynamic` distinct comes in Phase 2. - Two new account-level settings on the management server: `relay_timeout_seconds`, `p2p_timeout_seconds` (both nullable, NULL = built-in default). - Replaces the asymmetric Lazy/ForceRelay precedence in `client/internal/conn_mgr.go` with a single resolver: client env > client config > server-pushed. - Full backwards-compatibility: - Old clients still see only the legacy `lazy_connection_enabled` boolean (mapped from the resolved mode by `toPeerConfig`). - Old servers send `connection_mode = UNSPECIFIED` (proto default 0) and the new client falls back to the legacy boolean -- verified via tests. - `NB_FORCE_RELAY`, `NB_ENABLE_EXPERIMENTAL_LAZY_CONN`, `--enable-lazy-connection`, and `NB_LAZY_CONN_INACTIVITY_THRESHOLD` continue to work and emit one-shot deprecation warnings. The companion Dashboard PR is at netbirdio/dashboard#627 (will be added once it exists). ## Implementation map | Layer | Files | |---|---| | proto | `shared/management/proto/management.proto`, `client/proto/daemon.proto` | | Mode type | `shared/connectionmode/` (Mode, ParseString, FromProto, ToProto, ResolveLegacyLazyBool, ToLazyConnectionEnabled) | | client env / CLI | `client/internal/peer/env.go` (ResolveModeFromEnv), `client/cmd/{root,up}.go` (`--connection-mode`, `--relay-timeout`, `--p2p-timeout`) | | client engine / conn-mgr | `client/internal/{engine,conn_mgr,connect}.go` (resolveConnectionMode + UpdatedRemotePeerConfig + EngineConfig fields) | | client per-peer | `client/internal/peer/conn.go` (Mode-driven skip-ICE branch) | | mgmt server | `management/server/types/settings.go` (new nullable columns), `management/server/http/handlers/accounts/accounts_handler.go` (PUT validation + response), `management/server/account.go` (audit emission), `management/server/activity/codes.go` (3 new event codes), `management/internals/shared/grpc/conversion.go` (`toPeerConfig` writes both old and new wire fields) | | OpenAPI | `shared/management/http/api/openapi.yml` + regenerated `types.gen.go` | | tests | `shared/connectionmode/mode_test.go`, `client/internal/peer/env_test.go`, `client/internal/conn_mgr_test.go`, `management/internals/shared/grpc/conversion_test.go` | ## Hardware-tested Verified on a real production NetBird instance with **32 connected peers across 12 OpenWrt-router versions (22.03 to 25.12), Windows 10/11, Debian, Android 12/14, and iOS 26.3.1**: - Pre-deploy baseline: 32 connected. - After deploying the Phase-1 management image: 32 connected, no disconnect, **all old clients continued to function unchanged** (they read only the legacy `lazy_connection_enabled` boolean which `toPeerConfig` keeps writing via the back-compat mapping). - Switched the account-wide `connection_mode` between `p2p` and `p2p-lazy` via the API: 32 connected throughout, no disconnect. - 8-minute monitoring window after deploy: zero peer-count drift. The daemon-side change is exercised end-to-end by switching the account-wide setting and observing the daemon pick up the new mode on the next NetworkMap update (handled by `UpdatedRemotePeerConfig`). ## Known limitations (called out in spec section 8) 1. **`relay-forced` cannot be pushed to old clients** because the legacy `lazy_connection_enabled` boolean cannot express it. The wire field falls back to `false` and old clients run in normal `p2p`. Workaround: upgrade the client, or set local `NB_FORCE_RELAY=true`. 2. **`p2p-dynamic` is not visible in the Dashboard dropdown in Phase 1** even though the API accepts it; setting it via API gives `p2p` behaviour until the Phase-2 daemon implementation lands. 3. **Audit events are account-scoped only**; per-peer / per-group events follow in Phase 3 (#5990). ## Phase 2 / 3 follow-ups - **Phase 2** (separate PR): `p2p-dynamic` daemon implementation -- decouple `worker_relay` / `worker_ice` `OnNewOffer` registration, two-tier inactivity manager, plus the `DeactivatePeer` no-op fix in `conn_mgr.go` that #5989 calls out for the lazy/eager mismatch. - **Phase 3** (separate PR, addresses #5990): per-peer / per-group resolution for `connection_mode`, `p2p_timeout`, `relay_timeout` with most-restrictive group-conflict resolution and per-scope audit events. ## Test plan - [x] All new + existing unit tests in `client/internal/...` and `management/...` green - [x] `TestAccount_GetPeerNetworkMap` (existing) remains green -- old peers without `connection_mode` resolve via `ResolveLegacyLazyBool` exactly as before - [x] `TestToPeerConfig_ConnectionModeResolution` (new, 9 sub-cases) covers the resolution matrix - [x] `TestResolveConnectionMode` (new, 10 sub-cases) covers the env > config > server precedence chain - [x] `TestResolveModeFromEnv` (new, 9 sub-cases) covers the env-var matrix including the legacy-conflict case - [x] Hardware-verified against 32 mixed-version peers in production (no disconnects) 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Configurable peer connection modes (relay-forced, p2p, p2p-lazy, p2p-dynamic) via CLI flags and account settings. * Relay and P2P timeout overrides to tune idle behavior. * Activity/audit events emitted when connection-mode or timeout settings change. * **Tests** * Added unit tests covering mode/timeout resolution, env-var parsing, serialization round-trips, and API validation. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
saavagebueno added the pull-request label 2026-08-05 07:08:41 -04:00
Sign in to join this conversation.
No Label pull-request
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: DYNR/netbird#27416