[client] Make lazy connections opt-out via NB_LAZY_CONN#6617
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReplaces the lazy-connection boolean path with tri-state ChangesLazyConn Tri-State Override
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/internal/conn_mgr.go (1)
70-82: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear the lazy-connection status on every inactive path.
Start()andUpdatedRemoteFeatureFlag()now no-op in several “manager not running” cases (lazyForceNone, Rosenpass blocking startup, and remoteenabled=falsewhilelazyConnMgris nil) without callingUpdateLazyConnection(false). SinceConnMgrreceives a shared*peer.Status, a staletruecan survive reconfiguration and report lazy connections as enabled even though no manager is active.💡 Proposed fix
switch e.force { case lazyForceOff: log.Infof("lazy connection manager is disabled by %s", lazyconn.EnvLazyConn) e.statusRecorder.UpdateLazyConnection(false) return case lazyForceNone: log.Infof("lazy connection manager is managed by the management feature flag") + e.statusRecorder.UpdateLazyConnection(false) return } if e.rosenpassEnabled { log.Warnf("rosenpass connection manager is enabled, lazy connection manager will not be started") + e.statusRecorder.UpdateLazyConnection(false) return } @@ if enabled { @@ if e.rosenpassEnabled { log.Infof("rosenpass connection manager is enabled, lazy connection manager will not be started") + e.statusRecorder.UpdateLazyConnection(false) return nil } @@ } else { if e.lazyConnMgr == nil { + e.statusRecorder.UpdateLazyConnection(false) return nil }Also applies to: 93-119
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/internal/conn_mgr.go` around lines 70 - 82, Clear the lazy-connection status whenever the manager is not active. In ConnMgr.Start and UpdatedRemoteFeatureFlag, make sure every early return path that leaves the lazy connection manager stopped (lazyForceNone, rosenpassEnabled blocking startup, and remote enabled=false when lazyConnMgr is nil) calls statusRecorder.UpdateLazyConnection(false) before returning. Use the existing ConnMgr, Start, UpdatedRemoteFeatureFlag, and UpdateLazyConnection symbols to update all inactive branches consistently so a stale true cannot remain in shared peer.Status.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@client/internal/conn_mgr.go`:
- Around line 70-82: Clear the lazy-connection status whenever the manager is
not active. In ConnMgr.Start and UpdatedRemoteFeatureFlag, make sure every early
return path that leaves the lazy connection manager stopped (lazyForceNone,
rosenpassEnabled blocking startup, and remote enabled=false when lazyConnMgr is
nil) calls statusRecorder.UpdateLazyConnection(false) before returning. Use the
existing ConnMgr, Start, UpdatedRemoteFeatureFlag, and UpdateLazyConnection
symbols to update all inactive branches consistently so a stale true cannot
remain in shared peer.Status.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: fcaf8b21-6c2d-4a3f-ad3d-e60f80dcb89e
📒 Files selected for processing (8)
client/android/env_list.goclient/cmd/root.goclient/internal/conn_mgr.goclient/internal/conn_mgr_test.goclient/internal/lazyconn/env.goclient/internal/lazyconn/env_test.goclient/ios/NetBirdSDK/env_list.goclient/ui/const.go
fb71d70 to
4791267
Compare
Release artifactsBuilt for PR head
GHCR images (amd64)No GHCR images were pushed. This comment is updated by the Release workflow. Artifact links expire according to the workflow retention policy. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/internal/conn_mgr_test.go (1)
10-23: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd precedence coverage for local config fallback.
The table covers
NB_LAZY_CONN, but not the documented fallback to local config or env-over-local-config precedence. Add cases for env unset + local config on/off, plus env set overriding local config.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/internal/conn_mgr_test.go` around lines 10 - 23, Add test coverage for the `resolveLazyForce` precedence rules around local config fallback. Extend `TestResolveLazyForce` to include cases where `NB_LAZY_CONN` is unset and the local config forces on/off, as well as cases where `NB_LAZY_CONN` is set and should override the local config in both directions. Use the existing `resolveLazyForce` and `lazyForce` table pattern to verify the documented env-over-local-config behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@client/internal/conn_mgr_test.go`:
- Around line 10-23: Add test coverage for the `resolveLazyForce` precedence
rules around local config fallback. Extend `TestResolveLazyForce` to include
cases where `NB_LAZY_CONN` is unset and the local config forces on/off, as well
as cases where `NB_LAZY_CONN` is set and should override the local config in
both directions. Use the existing `resolveLazyForce` and `lazyForce` table
pattern to verify the documented env-over-local-config behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 027fb85a-19af-4542-895e-f46fa5c94b0b
📒 Files selected for processing (9)
client/android/env_list.goclient/cmd/root.goclient/cmd/up.goclient/internal/conn_mgr.goclient/internal/conn_mgr_test.goclient/internal/lazyconn/env.goclient/internal/lazyconn/env_test.goclient/ios/NetBirdSDK/env_list.goclient/ui/const.go
💤 Files with no reviewable changes (1)
- client/cmd/up.go
✅ Files skipped from review due to trivial changes (3)
- client/ui/const.go
- client/android/env_list.go
- client/cmd/root.go
🚧 Files skipped from review as they are similar to previous changes (3)
- client/internal/lazyconn/env.go
- client/ios/NetBirdSDK/env_list.go
- client/internal/lazyconn/env_test.go
4791267 to
f38893a
Compare
f38893a to
68690e9
Compare
68690e9 to
159e573
Compare
riccardomanfrin
left a comment
riccardomanfrin
left a comment
There was a problem hiding this comment.
I think you need to add the new key in canonical_loaders.go:13
Otherwise it will be discarded as unknown for darwin and windows
| } | ||
| } | ||
|
|
||
| if v, ok := policy.GetString(mdm.KeyLazyConnection); ok { |
There was a problem hiding this comment.
if we use GetBool we behave like the other bools (e.g. we allow also for things like "yes"/"no")
| if err != nil { | ||
| log.Warnf("failed to parse %s: %v", EnvEnableLazyConn, err) | ||
| return false | ||
| log.Warnf("failed to parse %s value %q: %v", EnvLazyConn, raw, err) |
There was a problem hiding this comment.
this was a previous mistake but here we might log that the value might come either from NB_ env OR from MDM... might allow faster recon of why a flag is there
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
client/mdm/policy.go (1)
66-75: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCase-sensitive value lookup could silently ignore MDM policy values.
GetBool's string branch looks upboolStringLiterals[t]using the raw value. A capitalized value (e.g."On","TRUE","Yes") — plausible from plist/registry-authored MDM configs — won't match, soGetBoolreturns(false, false)and callers treat the key as unset rather than applying the intended override. This is inconsistent with the doc comment's promise of tolerant parsing and with the case-insensitive key handling already used incanonicalKey(client/mdm/canonical_loaders.go).🐛 Proposed fix to normalize case before lookup
case string: - b, known := boolStringLiterals[t] + b, known := boolStringLiterals[strings.ToLower(strings.TrimSpace(t))] return b, knownAlso applies to: 160-180
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@client/mdm/policy.go` around lines 66 - 75, Normalize boolean string parsing in GetBool so capitalized MDM values are not treated as missing. Update the string branch to compare against boolStringLiterals using a lowercased value, and keep the tolerant parsing behavior aligned with the doc comment and canonicalKey’s case-insensitive handling. Use the GetBool method and boolStringLiterals map in client/mdm/policy.go as the anchor points for the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@client/mdm/policy.go`:
- Around line 66-75: Normalize boolean string parsing in GetBool so capitalized
MDM values are not treated as missing. Update the string branch to compare
against boolStringLiterals using a lowercased value, and keep the tolerant
parsing behavior aligned with the doc comment and canonicalKey’s
case-insensitive handling. Use the GetBool method and boolStringLiterals map in
client/mdm/policy.go as the anchor points for the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bbefb048-cf46-4ba4-a8c5-60b20d4d644a
📒 Files selected for processing (6)
client/internal/lazyconn/env.goclient/internal/profilemanager/config.goclient/internal/profilemanager/config_mdm_test.goclient/mdm/canonical_loaders.goclient/mdm/policy.goclient/mdm/policy_test.go
✅ Files skipped from review due to trivial changes (1)
- client/mdm/policy_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- client/internal/lazyconn/env.go
- client/internal/profilemanager/config.go
|
Lazy connections are now opt-out and controlled centrally by NB_LAZY_CONN, MDM policy (lazyConnection), and the management feature flag (#6617). The per-device UI toggle no longer fits this model: after #6617 the daemon stopped persisting the setting and dropped it from GetConfig, so the Wails toggle always read back OFF and its writes did not survive a restart. Remove the toggle and the orphaned runtime plumbing, matching what main did for the Fyne UI: - drop the toggle from SettingsNetwork and the lazy i18n keys - drop LazyConnectionEnabled from the UI settings service and SetConfig request - drop the runtime-apply block in server.SetConfig - delete Engine.SetLazyConnEnabled and ConnMgr.SetLocalLazyConn The proto fields and FullStatus status reporting are left intact.
2 participants
Describe your changes
Lazy connections are becoming the default (enabled by management for new accounts in #6571). This reworks the client-side control so lazy is opt-out, driven by management with explicit local overrides, and removes the old experimental opt-in.
Resolution order:
NB_LAZY_CONNenv var (if set) > MDM policy override > management feature flag > off.NB_LAZY_CONNenv var (on/off/unset); set, it overrides the management feature flag in both directions; unset defers to managementlazyConnectionkey,on/off) with the same semantics, below the env varNB_ENABLE_EXPERIMENTAL_LAZY_CONNvariable--enable-lazy-connectionCLI flag: inert and hidden (kept so existing invocations don't error)LazyConnectionEnabledsetting end to end (config, daemon apply, desktop UI checkbox, MDM bool, and login telemetry); lazy is no longer a persisted local toggleThe per-peer lazy flag previously reported to management via login metadata is no longer sent (it reflected the removed local setting). Runtime lazy status in
netbird statusis unaffected.Issue ticket number and link
Complements #6571.
Stack
Checklist
Documentation
Select exactly one:
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
netbirdio/docs#821
Summary by CodeRabbit
NB_LAZY_CONN(on/off/unset), with unset deferring to management using consistent precedence.lazyConnection, including normalization ofon/off/yes/novalues.--enable-lazy-connection; it’s no longer used byup.