-
Notifications
You must be signed in to change notification settings - Fork 269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(apple): Handle network changes reliably on macOS and iOS #4133
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎ 1 Ignored Deployment
|
Terraform Cloud Plan Output
|
Performance Test ResultsTCP
UDP
|
42e3189
to
a7bff54
Compare
10860a5
to
6527c5b
Compare
@conectado @ReactorScram @thomaseizinger Just want to leave this here as an example of how noisy the path update handler can be. This was all printed from a single state change from
In the above log, |
c559e97
to
2c0ae74
Compare
2d8fabb
to
543ccba
Compare
// sentinel, which isn't helpful to us. | ||
private func resetToSystemDNSGettingBindResolvers() -> [String] { | ||
var resolvers: [String] = [] | ||
let semaphore = DispatchSemaphore(value: 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each path update callback spawns a thread (workQueue.async
), so we're ok to block here with a semaphore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I noticed this sometimes blocked the main UI thread on iOS, so I enclosed this inside a Task
in the calling function.
Thanks, that is really helpful! Should connlib wait 500ms after receiving an update before it acts on it? I.e. a new update coming in would reset the timer to 0. That appears it would end up only apply a single one of these then. |
Yeah I think 500ms is a reasonable debounce timeout to start with. |
52feab4
to
67c828f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I don't fully understand all the swift stuff but I have one question regarding on_tunnel_ready
.
@jamilbk The PR description might need some adjusting now that more features / fixes have been packed into it. If possible, it would also be nice to perhaps split a few of these refactorings off into their separate PR. From what I've followed it, the bulk of changes that "fix" things seem to be in |
@thomaseizinger Yeah I can stop adding to this. Until today It's really a pita. |
Don't worry if it is too difficult :) Perhaps now that we have a working version, splitting it apart isn't too much work? |
Unfortunately the history is quite clobbered in this PR. There were a lot of style changes as well, and multiple changes to the same pieces of code for different issues :-/. I can rebase and at least the last few commits could be gone through on their own. I'll do that. |
Going forward, I don't expect any more near-full-rewrites, but I wasn't confident that I could compartmentalize small changes to go into main. It kinda turned into a ball of yarn... |
Last thought: we at least need unit tests in the clients. I reckon with a combo of:
We could get pretty far without having to build a massive e2e brittle test harness. |
1aec868
to
5895f8a
Compare
@conectado @ReactorScram Ready for final review. Apologies for the size... this one was quite a doozie. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I left a lot of questions relating to trying to learn Swift.
This was hard to review. I see a lot of stuff changed names, some diffs look like just formatting changes too. So I may have overlooked semantic changes, in addition to the expected amount of not understanding it all yet.
I'd rather have formatting and renaming separated from semantic changes in general.
The control flow also seems confusing to me, but maybe that's something we could clean up after GA. Maybe the flow in the Tauri Client only makes sense to me because I wrote it.
// Constants | ||
private weak var packetTunnelProvider: NEPacketTunnelProvider? | ||
private(set) var logger: AppLogger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are not constant in the sense of Rust's const
, but they're objects that won't be replaced during the lifetime of this NetworkSettings
object?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, that's probably not a good comment. I'll fix it.
fn reconnect(&mut self); | ||
#[swift_bridge(swift_name = "setDns")] | ||
fn set_dns(&mut self, dns_servers: String); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll leave some redundant questions just to make sure I understand how the Apple Client fits together.
So this part is exposing the Rust functions to Swift, we're declaring these two new functions. This is still on the Rust side, so I assume it would error if the signature is wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah! The macros in here are used to generate the bindings.
} | ||
} | ||
} | ||
public var appStore: AppStore? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"store" here means some persistent variable storage, it's not related to the Apple App Store package manager, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was confused by that too at first. That's correct.
It could probably use a rename.
.filter { $0.isInitialized } | ||
.sink { [weak self] tunnelAuthStatus in | ||
tunnelStore.$status | ||
.sink { [weak self] status in | ||
guard let self = self else { return } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is equivalent to the let-else
we use in Rust? The else
is required to diverge, and it doesn't do anything special, it's just a more idiomatic way to do an early return?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's correct. Guards are pretty heavily used in Swift and they turn Optional(obj)
into just obj
so you don't need the optional modifier in the remainder of the block body.
|
||
// Tell the system the tunnel is up, moving the tunnelManager status to | ||
// `connected`. | ||
completionHandler(nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Completion handlers are async callbacks, like the Promises in JS?
workQueue.async { [weak self] in | ||
guard let self = self else { return } | ||
|
||
if referenceVersionString == self.displayableResources.versionString { | ||
if hash == Data(SHA256.hash(data: Data((resourceListJSON ?? "").utf8))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why wouldn't string equality work here?
} | ||
|
||
if shouldFetchSystemResolvers(path: path) { | ||
// Spawn a new thread to avoid blocking the UI on iOS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No chance that thread / task can leak?
private let target: Target | ||
private let folderURL: URL | ||
private let logger: Logger | ||
|
||
// All log writes happen in the workQueue | ||
private let workQueue: DispatchQueue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this do backpressure if the queue gets too long?
// Constants | ||
private weak var packetTunnelProvider: NEPacketTunnelProvider? | ||
private(set) var logger: AppLogger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct, that's probably not a good comment. I'll fix it.
public var tunnelAddressIPv4: String? | ||
public var tunnelAddressIPv6: String? | ||
public var dnsAddresses: [String] = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, Apple's API expects them as Strings, so there's no point going further than that. They're sent from connlib and assumed to be valid.
// Remove the passwordReference from our configuration so that it's not used again | ||
// if the app is re-launched. There's no good way to send data like this from the | ||
// Network Extension to the GUI, so save it to a file for the GUI to read later. | ||
try String(reason.rawValue).write(to: SharedAccess.providerStopReasonURL, atomically: true, encoding: .utf8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Not really, the sequence of events is:
- Tunnel process writes reason to file
- Tunnel process exits
- GUI receives
NEVPNStatusDidChange
- GUI reads the file and acts accordingly, deleting it
- Hm, it could, but this file is solely for (at this point) showing a UI notification. It's a boolean flag that tells it "You were unexpectedly signed out, sign in again". If the GUI crashes before it's able to "consume" the notification, it'll just show it again on next launch, no biggie (other than an annoyance to the user).
- Yes
swift/apple/FirezoneKit/Sources/FirezoneKit/Views/MenuBar.swift
Outdated
Show resolved
Hide resolved
} | ||
} | ||
} | ||
public var appStore: AppStore? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was confused by that too at first. That's correct.
It could probably use a rename.
// if that doesn't exist. The Firezone ID is a UUIDv4 that is used to dedup this device | ||
// for upsert and identification in the admin portal. | ||
public static func getOrCreateFirezoneId(logger: AppLogger) -> String { | ||
let fileURL = SharedAccess.baseFolderURL.appendingPathComponent("firezone-id") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These files aren't really accessible outside Firezone. I guess you could append extensions but extensions aren't really common for random app-specific files on Unix I think.
let newUUIDString = UUID().uuidString | ||
|
||
do { | ||
try newUUIDString.write(to: fileURL, atomically: true, encoding: .utf8) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah there are some nice things actually in the Apple Garden™
|
||
class SystemConfigurationResolvers { | ||
private let logger: AppLogger | ||
private var dynamicStore: SCDynamicStore? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually the system's configuration and is not app specific. That's why this API is not available for iOS. I don't believe it's writable due to App sandboxing.
private var dynamicStore: SCDynamicStore? | ||
|
||
// Arbitrary name for the connection to the store | ||
private let storeName = "dev.firezone.firezone.dns" as CFString |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, I think we might be able to write to the SC store, but I'm not sure, since our tunnel process is supposed to be sandboxed (may need an entitlement for that).
https://developer.apple.com/documentation/systemconfiguration/scpreferencessetspecific
…removed (#4324) - Handles an edge case where we would crash if the tunnel configuration was removed in the middle of saving Settings. - Handles an edge case where our tunnel configuration may have been changed by the system -- if it was disabled, we disconnect.
When binding to published variables from other objects, we must use the parameter received in the callback to act upon, and not re-read the stored parameter on the associated object again. The stored parameter can be stale, which leads to UI bugs where we transitioned state but the menubar icon or menu did not get updated. <img width="274" alt="Screenshot 2024-03-25 at 11 45 35 PM" src="https://github.com/firezone/firezone/assets/167144/17a26b52-0599-4523-a7d1-d1bb2e617c44"> This fixes a long-standing bug that has occurred off and on since the early versions.
Co-authored-by: Reactor Scram <ReactorScram@users.noreply.github.com> Signed-off-by: Jamil <jamilbk@users.noreply.github.com>
Signed-off-by: Jamil <jamilbk@users.noreply.github.com>
696f3ea
to
13b3765
Compare
Tried to organize this PR into commits so that it's a bit easier to review.
.stoppingTunnel
,.stoppedTunnelTemporarily
, and.stoppingTunnelTemporarily
states have been removed.self.
prefix from local vars when it's not necessary to use it, to be more consistent.onTunnelReady
andgetSystemDefaultResolvers
has been removed, andonUpdateRoutes
wired up, along with cleanup necessary to support that.reconnect
andset_dns
stubs in the FFI and fixing the log filter so that we can log them (see chore(apple): Remove connlib_client_apple log filters because they have no effect #4182 )SystemConfiguration
to read DNS servers.path.gateways
has changed, and avoid setting new DNS if it hasn't.Refs #4028
Fixes #4297
Fixes #3565
Fixes #3429
Fixes #4175
Fixes #4176
Fixes #4309