mirror of
https://github.com/netbirdio/netbird.git
synced 2026-09-13 02:09:08 +02:00
fix/daemon-connection-lifecycle
5
Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
 Zoltán Papp
|
3615c01fe3 |
fix(client): keep the run's exit signal readable after supervisor Stop
Stop cleared the done channel, so a waiter that raced a teardown got nil from Done() and selected on a nil channel: an Up racing a Down hung in waitForUp for its full 50s timeout where it previously failed fast with "client gave up to connect". The same clearing made Alive() report a timed-out run dead while its goroutine was still tearing down, letting Up start an overlapping second run. Keep the channel in place instead: the exiting run closes it, so late waiters observe the real exit and Alive() stays true until the run is actually gone — the lifecycle clientGiveUpChan used to have. Stop still drops the current client and invalidates the generation. The stale-running-chan test begins a fresh run before waiting, as Up does, so the kept exit signal does not race the stale clientRunningChan. |
||
|
Zoltán Papp
|
5df35e3e27 |
fix(client): track which connection run is current in the daemon
Nothing recorded which run of the connection was current, so three defects followed from the same gap. A ConnectClient is single-use, and the daemon builds a fresh one per outer-retry turn (server.go connect). Each turn overwrote s.connectClient and nothing stopped the one it replaced — the outgoing run loop had returned, which is what brought control back to the retry, but that was assumed rather than enforced, and any teardown its error path left half-done got no second chance. cleanupConnection read s.connectClient, cancelled, then stopped that engine. Nothing established the client it read was still current by the time it stopped it, so a teardown could target a client a newer turn had already replaced and leave the live one running untracked. Down's wait on clientGiveUpChan and Up's refusal to start a second loop kept the window narrow, but by arrangement rather than by construction. Third, the engine was stopped twice concurrently: actCancel woke the run loop, which stops the engine on its way out, while cleanupConnection stopped the same engine directly. The TODO there said ConnectClient.Stop was the right call and that its unbounded wait was what ruled it out. RunSupervisor records the generation of the current run. Publish refuses a client from a superseded run and stops the client it displaces, so no ConnectClient is dropped without being stopped. Stop invalidates whatever run is in flight, stops the published client and waits for the run to exit. ConnectClient.StopWithContext bounds that wait, which removes the TODO's obstacle: cleanupConnection now hands the run loop sole ownership of engine shutdown and passes Down's 5s budget down. Stop() keeps its signature and its unbounded wait, so callers outside this change are untouched. embed.Client.Stop had built the same bound by hand with a goroutine and a select purely to watch its caller's context; it passes the context down instead. clientGiveUpChan and connectClient are gone — the supervisor answers both. The MDM restart path drops its hand-rolled 10s channel wait for the same Stop, which additionally stops the client the previous run left behind. Its deliberate choice to leave clientRunning set is unchanged. Down now waits inside cleanupConnection, under s.mutex, where it previously waited after releasing it. That is what pins the client being stopped to the one current when the call started; the cost is that Down can hold the mutex for up to its 5s budget. Found while fixing the iOS wifi-to-cellular black-hole (#7329), which was the same class of defect in the mobile SDKs. No bug report backs the daemon findings — they are read off the code, and the narrow windows above may be why they have not been observed. |
||
|
Riccardo Manfrin
|
2bcea9d582 |
[client] add MDM configuration profile support (Windows registry + macOS plist) (#6374)
* Initial scaffolding * Applies MDM override * Unit tests * Helpers business logic * Return error if trying to modify any config that is gated by MDM * Add ManagedFields to returned config over GetConfig * Adds initial 101 MDM policy business logic testing * gRPC MDM changes * MDM Name scoping for clarity * Implements windows loading of MDM policy * Adds missing WGPort config * Cleanup setupKey to align to linear * Align split tunnel code * Adds some log * Prefix every log with MDM * Adds debug config cobra command This can be useful for troubleshooting and checking config now that its resolution is not trivial defaults > config > env cars > CLI/UI > MDM * Adds MDM 1m diff checker & reloader * Adds also up/start after cancel * Publishes event for UI to sync upon MDM changes * Add events to resync UI to actual config This also provide fixup for UI no aligning to changed config when coming from cli up with config flags. * UI behavior conflicts relaxation UI sends full config snapshot with all values. It doesn't make sense to block it if the values are aligned with the values constrained by the MDM policy. It's just simplier to allow values that are compliant. (this goes for the CLI as well at this point) * Lock toggle Settngs * Advanced Settings locking * Fixup presharedkey * Apply MDM locks * Toggle gray in/out for Advanced Settings * Adds support for disabling of Profiles and UpdateSettings feature flags * Adds Gate Login as well when --disable-update-settings=true is given to service This commit tries to settle things with an old PR-4237 which had relaxed the case where the SetConfig returned an `Unavailable` code error. Under this circumnstance the PR allowed the upFunc to just emit a warning and progress further with the login gRPC. Since the login call is consuming the --management-url coming from the `up` command, it might be possible to abuse the "Unavailable" code to inject a management URL that is different from the configured one even though the --disable-update-settings is set to true (?) * Evaluate disable-update-settings errors only when there's an actual override * [UI] Fixup advanced Settings * [UI] Fixup for preshared key * [UI] Fixup for profile enable/disable toggle We need to align the initial state to evaluate the delta in case. The initial state has to be "true" since the profile starts visible. Then we receive MDM and transition the cache bool value to the actual MDM imposed state * Enforces disable networks * [UI] Aligns to "enable/disable once on change only" * Fixup: MDM wins. always * Removes --disable-advanced-settings It was a typo in our meetings. the actual thing is --disable-update-settings * [PROTO] Removes --disable-advanced-settings * [UI] Removes --disable-advanced-settings * Pins feat profile retrieval to notif event * [UI] Fix for "hide" not working when propagating to parent with children * Adds dep for reading plist files * Introduces support for darwing plist loading * Tests MDM config reload via ticker * [PROVISIONING] ADMX/ADML/PS/bash scripts/templates * CI fixes - Add docstrings to `mdm_integration` - refactor for cognitive complexity - mod tidy * Linting * Add docstrings to `mdm_integration` * nil,nil is no policy and no error. Allow it * nil,nil is no policy and no error. Allow it * exclude MDM profile adminstrated keys data from debug bundle * Fixes Rosenpass left disable after MDM unlock * Partial revert coderabbit added docstrings * Renaming fix * Avoid locking on clientRunning bool when the connection is aborted for whatever reason We want to just signal this through the giveUpChan, we will manage the signal from the waiter side and in case set it to false there. THis way we avoid locking, which should allow the MDM down+wait_for_term_chan_signal_+up procedure clientRunning is used to signal two different conditions here: 1. the initialization procedure is over (we have an engine) 2. the connection being up (or being attempted) Probably these two functionalities should not alias, and the failure of the second condition (because of any error) should just drive a reconnection (currently it's not happening, and we silently go idle). OR, mor probably, the two things are the SAME and there should not exist a case where we did the "Up" initialization and connection attempt but we are not still attempting it. * Moves test helper at te very bottom * Addresses github comments * No lock no copy * Prevents engine not stopping within 10 secs from being paired by another instance We instead juts SKIP updating the policy, so 1. the MDM ticker will kick in 1 minute time, 2. find the policy misaligned, 3. enter the onMDMPolicyChange, 4. find the s.clientRunning == true (because it is set to false only in server cleanupConnection, and not by s.actCancel()) 5. call s.actCancel() again if not nil 6. immediately return from <-s.clientGiveUpChan 7. finally call s.restartEngineForMDMLocked() * Since we ARE running there should be a config If the config was cancelled midflight, connect will abort later on * DisableAutoConnect should not stop a running connection. DisableAutoConnect should just avoid the connection attempts *when the service starts*. If we are started and we are up and running, DisableAutoConnect should not kick in. Another PR will follow about this topic * Removes unused vars * Moves callback into Run method arg * align comment to removal of DisableAutoConnect DisableAutoConnect should just avoid the connection attempts *when the service starts*. If we are started and we are up and running, DisableAutoConnect should not kick in * Removes unused managed_fields data. This was initially used to drive the UI but approach changed to reload config/features upon notifications which makes this data redundant. * Reorder stuff * Unexport unrequired vars/functions PoliciesEqual → policiesEqual AllKeys → allKeys * Adds list of MDM managed fields in the debug bundle |
||
|
Zoltan Papp
|
fe9b844511 |
[client] refactor auto update workflow (#5448)
Auto-update logic moved out of the UI into a dedicated updatemanager.Manager service that runs in the connection layer. The UI no longer polls or checks for updates independently. The update manager supports three modes driven by the management server's auto-update policy: No policy set by mgm: checks GitHub for the latest version and notifies the user (previous behavior, now centralized) mgm enforces update: the "About" menu triggers installation directly instead of just downloading the file — user still initiates the action mgm forces update: installation proceeds automatically without user interaction updateManager lifecycle is now owned by daemon, giving the daemon server direct control via a new TriggerUpdate RPC Introduces EngineServices struct to group external service dependencies passed to NewEngine, reducing its argument count from 11 to 4 |
||
|
Maycon Santos
|
15aa6bae1b |
[client] Fix exit node menu not refreshing on Windows (#5553)
* [client] Fix exit node menu not refreshing on Windows TrayOpenedCh is not implemented in the systray library on Windows, so exit nodes were never refreshed after the initial connect. Combined with the management sync not having populated routes yet when the Connected status fires, this caused the exit node menu to remain empty permanently after disconnect/reconnect cycles. Add a background poller on Windows that refreshes exit nodes while connected, with fast initial polling to catch routes from management sync followed by a steady 10s interval. On macOS/Linux, TrayOpenedCh continues to handle refreshes on each tray open. Also fix a data race on connectClient assignment in the server's connect() method and add nil checks in CleanState/DeleteState to prevent panics when connectClient is nil. * Remove unused exitNodeIDs * Remove unused exitNodeState struct |