Drop the dedicated server URL file - #202
Merged
Merged
Conversation
Replace the parallel Swift profile manager with a thin wrapper over the core's ID-based profilemanager.ServiceManager (via the new NetBirdSDK iOS binding), eliminating the duplicate implementation. Profiles are now ID-keyed (on-disk filename = id, display name lives in the config); the default profile stays "default" at the container root and never becomes a hex id, matching the Go/desktop semantics. - ProfileManager: thin wrapper over NetBirdSDKProfileManager on iOS; tvOS keeps its single-default, container-root behavior (no Go profile manager) - ProfileLayoutMigration: one-time, idempotent, file-coordinated migration from the legacy profiles/<name>/ directory layout to the Go layout; preserves auth tokens (copies unparseable configs verbatim), keeps the active selection and logged-out profiles, and only marks itself done on full success so a failure retries on the next launch - ProfileConnectionCache and all callers re-keyed by profile id - Settings/server changes already target the active profile through the profile manager (Preferences.configFile -> activeConfigPath) - Tests: NetBirdTests/ProfileMigrationTests covers the migration, reads it back through the Go lib, and round-trips a setting via NetBirdSDKPreferences (the test target now links NetBirdSDK) - Bump netbird-core submodule to include the iOS profile manager binding
Run the iOS test suite on PRs again to check whether the previously reported test crash still reproduces.
The non-null String path getters (getActiveConfigPath/getActiveStateFilePath/getConfigPath/getStateFilePath) are generated by gomobile as value-returning calls with an explicit NSErrorPointer parameter, not throwing methods. Wrap them via a goPath helper in ProfileManager (and matching helpers in the tests), fixing the iOS app build failure (missing argument for parameter).
The NetBirdTests target was iOS 14.0 while the NetBird app module requires iOS 15.0, so `@testable import NetBird` failed to build ("Compiling for iOS 14.0, but module 'NetBird' has a minimum deployment target of iOS 15.0"). Match the test target to the app so the test bundle compiles and the suite can run.
The dummy GoogleService-Info.plist used a GOOGLE_APP_ID of "dummy",
which fails Firebase's app-ID format validation. At launch the host app
calls FirebaseApp.configure(options:), and +[FIRApp addAppToAppDictionary:]
raised an uncaught NSException, aborting the host before the test runner
could establish a connection ("Test crashed with signal abrt before
establishing connection"). This blocked all hosted tests, not just the
new profile-migration ones.
Give the dummy app ID a well-formed value so configure() succeeds.
The host app calls FirebaseApp.configure() at launch. When launched purely to host unit tests, this aborted the process before the test runner could connect because FirebaseApp.configure() raises an uncaught ObjC exception on an invalid app ID (the CI dummy plist). Gate both the iOS and tvOS configure paths on the XCTestConfigurationFilePath environment variable so Firebase is never initialized in the test host, and fold the duplicated configure logic into a single helper.
The legacy netbird.cfg is written by the Go SDK, which serializes
Config.ManagementURL (a url.URL) as a nested {Scheme,Host,Path} object,
not a plain string. The settings round-trip test drives NetBirdSDKPreferences,
which fully unmarshals Config and rejected the string fixture with
"cannot unmarshal string into Go struct field Config.ManagementURL".
Normalize the fixture to the object form the SDK actually produces.
The test target used a recursive "$(PROJECT_DIR)/**" framework search path, scanning every subdirectory (DerivedData, netbird-core, package checkouts) and risking unexpected framework resolution. NetBirdSDK.xcframework lives at the project root, so point the search path at "$(PROJECT_DIR)" directly.
Conflict resolutions, all of the same shape: main added an ipv6 field and a logout-surviving server URL file while this branch re-keyed profiles from display name to ID. Both sides are kept. - netbird-core: our merge commit. Main's pointer (v0.77.1) is already an ancestor of it, so nothing from main is lost. - ProfileConnectionCache / MainViewModel: ID keying plus main's ipv6 handling. Carried main's server URL file over to the ID-based layout: it was keyed by display name and written into the per-profile directory, which this branch no longer has. It now sits next to the profile config (netbird_server_url in the container root for the default profile, profiles/<id>.server_url otherwise), and ProfileManager.managementURL(forID:) consults it between the config file and the connection cache. Its two callers (ServerViewModel, the extension's performLogin) pass the active profile ID.
The profile configs carry the WireGuard and SSH private keys, and the WireGuard key is the peer's identity: restoring a backup onto another device would produce two devices connecting with the same key. Mark the profiles/ directory and the container-root profile files (netbird.cfg, state.json, active_profile.json, netbird_server_url) as excluded from iCloud/iTunes backups. The exclusion is re-applied on every launch from the ProfileManager init: the Go core rewrites some of these files via temp-file+rename, which drops per-file attributes. The profiles/ directory is excluded as a whole (and created up front if missing), so its contents stay covered regardless of how they are rewritten. After a restore the user re-logs in, matching how a profile behaves when moved to a new machine on desktop.
The cache was backed by UserDefaults.standard, which is per-process: the network extension is the process that learns the peer's ip/fqdn and runs the login, so everything it wrote was invisible to the main app and vice versa. Back it with the App Group suite instead, so both processes see the same entries. Entries written by an earlier version are moved over on first use. A suite that already holds data is never overwritten, so a stale app-local copy cannot clobber what the extension has since written.
The file was a fourth copy of the management URL, written next to the profile config. It existed because Swift code used to rebuild configs and could drop the URL, leaving login to fall back to the default cloud server and write that back — so the URL had to be recoverable from somewhere else. Both reasons are gone. In the ID-based layout the config survives logout (only the keys are cleared), and the core's DirectUpdateOrCreateConfig merges into the existing config instead of rebuilding it, so no write can lose the URL. The connection cache covers the window before the config can be read back, and now that it lives in the App Group suite it does so across processes too — which was the file's last unique property. managementURL(forID:) is therefore config -> connection cache. saveServerURL(_:forID:) stays as the callers' entry point but records into the cache. Profiles migrated from the legacy layout keep their URL: ProfileLayoutMigration already seeds the cache from the legacy file. The file name stays in the backup exclusion list — earlier versions left files behind, and those should not reach a backup either.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe change moves management URL persistence into the per-profile connection cache. The cache adds App Group ChangesProfile connection cache migration
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This change removes a redundant server URL file while preserving URL lookup and migration behavior; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant NetworkExtensionAdapter
participant ProfileManager
participant ProfileConfig
participant ProfileConnectionCache
NetworkExtensionAdapter->>ProfileManager: Resolve managementURL(forID:)
ProfileManager->>ProfileConfig: Read management URL
ProfileConfig-->>ProfileManager: Return configured URL or no value
ProfileManager->>ProfileConnectionCache: Read cached management URL when needed
ProfileConnectionCache-->>ProfileManager: Return cached URL
ProfileManager-->>NetworkExtensionAdapter: Return management URL for login payload
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 40 functions across 5 files. (1 skipped: 1 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
login() read the management URL straight from the connection cache when building the "Login:<configPath>|<statePath>[|<managementURL>]" message, bypassing ProfileManager.managementURL(forID:), which resolves from the profile config first and falls back to the cache. With an empty cache but a live config the URL was omitted from the IPC message. The practical impact is small — when the config exists the extension reads the URL from it anyway — but this was the last call site reading the cache directly for the URL. Going through ProfileManager keeps both login paths consistent (performLogin already resolves this way), re-seeds the cache from the config as a side effect, and keeps the URL in the message even if a cache entry is ever lost.
The standard → App Group suite migration dropped the legacy copy whenever the suite already held data, and removed it even when the suite write never landed. Both lose data on the first launch after the upgrade: - On-demand VPN can start the extension before the app runs, seeding the suite with URL-only entries; the app's entire legacy cache (ip/fqdn and URLs for every profile) was then deleted without a merge. - With a missing App Group entitlement the suite write silently no-ops, and the standard-domain copy — the last remaining one — was deleted anyway. Merge instead: suite values win field by field (the suite is strictly newer than the legacy copy, which is frozen at upgrade time) and legacy values fill the fields the suite holds no value for. The standard copy is removed only after a verified read-back of the merged data — compared as decoded values, since JSON key order is not stable — so a write that did not land leaves the copy in place and the migration retries on the next init. The migration source is now injectable (defaulting to .standard), and ProfileConnectionCacheTests covers the merge semantics, the failed-write path, and idempotency. The migration still inherits the unsynchronized cross-process read-modify-write on the shared blob; that is addressed separately.
Every cache write was load → mutate → persist on a single JSON blob. With the cache in the App Group suite that is a cross-process read-modify-write with no coordination: the app writing ip/fqdn while the extension seeds a URL means one side's update is lost to the last writer. Split the storage so the schema matches the write patterns: one key per profile for the connection data (ip/fqdn/ipv6, always written together by the status updates) and one for the management URL (written by the login paths). Every write becomes a blind single-key set with no prior read, so concurrent writers cannot clobber each other's fields and no cross-process locking is needed — file coordination is unsafe from an extension, where the other process can be suspended while holding a claim. The blob is exploded into the per-profile keys on first use: existing keys are never overwritten (they are strictly newer than the blob), so concurrent migration from both processes stays benign, and the blob is removed only after every entry verifiably landed. The standard → suite merge keeps writing the blob and this second stage explodes it, so the full upgrade chain is standard blob → suite blob → per-profile keys. The public API is unchanged. clearConnectionData now removes the connection-data key instead of writing empty fields — same observable behavior, one blind delete. remove clears both keys; a process death between the two can orphan a URL key, which is harmless and cleaned up by the next remove for that ID.
# Conflicts: # NetBird.xcodeproj/project.pbxproj # NetBird/Source/App/ViewModels/ServerViewModel.swift # NetbirdKit/NetworkExtensionAdapter.swift # NetbirdKit/ProfileConnectionCache.swift # NetbirdKit/ProfileManager.swift # netbird-core
evgeniyChepelev
approved these changes
Aug 26, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
The management URL was kept in four places; this removes one of them.
The dedicated server URL file existed because Swift code used to rebuild
configs and could drop the URL, letting login fall back to the default cloud
server. That is no longer possible: in the ID-based layout the config survives
logout, and the core's
DirectUpdateOrCreateConfigmerges into the existingconfig instead of rebuilding it.
ProfileConnectionCachenow uses the App Group suite instead ofUserDefaults.standard, so the app and the extension share entries (theyeach kept a private copy before — a fix in its own right). Existing entries
are migrated on first use.
managementURL(forID:)is nowconfig -> connection cache.saveServerURL(_:forID:)stays as the callers' entry point, writing to thecache.
Profiles migrated from the legacy layout keep their URL:
ProfileLayoutMigrationalready seeds the cache from the legacy file. The file name stays in the backup
exclusion list, since older versions left files behind.
Based on
refactor/migrate-profiles-to-go(#147), notmain.Summary by CodeRabbit
Bug Fixes
Tests