Skip to content
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

Reduce VPN manager instances #3097

Merged
merged 12 commits into from
Jul 19, 2024
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions DuckDuckGo.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -10121,8 +10121,8 @@
isa = XCRemoteSwiftPackageReference;
repositoryURL = "https://github.com/DuckDuckGo/BrowserServicesKit";
requirement = {
kind = exactVersion;
version = 171.2.2;
branch = "sam/reduce-vpn-manager-instances";
kind = branch;
};
};
9F8FE9472BAE50E50071E372 /* XCRemoteSwiftPackageReference "lottie-spm" */ = {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@
"kind" : "remoteSourceControl",
"location" : "https://github.com/DuckDuckGo/BrowserServicesKit",
"state" : {
"revision" : "09844ec2d0c9d2312e02c90527e8df063db89318",
"version" : "171.2.1"
"branch" : "sam/reduce-vpn-manager-instances",
"revision" : "44965d389213c5e454950ee7284b6eb081cfecc9"
}
},
{
Expand Down
2 changes: 1 addition & 1 deletion DuckDuckGo/NetworkProtectionConvenienceInitialisers.swift
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import Subscription

private class DefaultTunnelSessionProvider: TunnelSessionProvider {
func activeSession() async -> NETunnelProviderSession? {
try? await ConnectionSessionUtilities.activeSession()
return await AppDependencyProvider.shared.networkProtectionTunnelController.activeSession()
}
}
samsymons marked this conversation as resolved.
Show resolved Hide resolved

Expand Down
8 changes: 4 additions & 4 deletions DuckDuckGo/NetworkProtectionDebugUtilities.swift
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ final class NetworkProtectionDebugUtilities {
// MARK: - Registation Key

func expireRegistrationKeyNow() async {
guard let activeSession = try? await ConnectionSessionUtilities.activeSession() else {
guard let activeSession = await AppDependencyProvider.shared.networkProtectionTunnelController.activeSession() else {
return
}

Expand All @@ -41,7 +41,7 @@ final class NetworkProtectionDebugUtilities {
// MARK: - Notifications

func sendTestNotificationRequest() async throws {
guard let activeSession = try? await ConnectionSessionUtilities.activeSession() else {
guard let activeSession = await AppDependencyProvider.shared.networkProtectionTunnelController.activeSession() else {
return
}

Expand All @@ -51,7 +51,7 @@ final class NetworkProtectionDebugUtilities {
// MARK: - Disable VPN

func disableConnectOnDemandAndShutDown() async {
guard let activeSession = try? await ConnectionSessionUtilities.activeSession() else {
guard let activeSession = await AppDependencyProvider.shared.networkProtectionTunnelController.activeSession() else {
return
}

Expand All @@ -61,7 +61,7 @@ final class NetworkProtectionDebugUtilities {
// MARK: - Failure Simulation

func triggerSimulation(_ option: NetworkProtectionSimulationOption) async {
guard let activeSession = try? await ConnectionSessionUtilities.activeSession() else {
guard let activeSession = await AppDependencyProvider.shared.networkProtectionTunnelController.activeSession() else {
return
}

Expand Down
6 changes: 3 additions & 3 deletions DuckDuckGo/NetworkProtectionStatusViewModel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ final class NetworkProtectionStatusViewModel: ObservableObject {
return formatter
}()

private let tunnelController: TunnelController
private let tunnelController: (TunnelController & TunnelSessionProvider)
private let statusObserver: ConnectionStatusObserver
private let serverInfoObserver: ConnectionServerInfoObserver
private let errorObserver: ConnectionErrorObserver
Expand Down Expand Up @@ -140,7 +140,7 @@ final class NetworkProtectionStatusViewModel: ObservableObject {

@Published public var animationsOn: Bool = false

public init(tunnelController: TunnelController,
public init(tunnelController: (TunnelController & TunnelSessionProvider),
settings: VPNSettings,
statusObserver: ConnectionStatusObserver,
serverInfoObserver: ConnectionServerInfoObserver = ConnectionServerInfoObserverThroughSession(),
Expand Down Expand Up @@ -346,7 +346,7 @@ final class NetworkProtectionStatusViewModel: ObservableObject {
}

private func refreshDataVolumeTotals() async {
guard let activeSession = try? await ConnectionSessionUtilities.activeSession() else {
guard let activeSession = await tunnelController.activeSession() else {
samsymons marked this conversation as resolved.
Show resolved Hide resolved
return
}

Expand Down
115 changes: 73 additions & 42 deletions DuckDuckGo/NetworkProtectionTunnelController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -33,16 +33,55 @@ enum VPNConfigurationRemovalReason: String {
case debugMenu
}

final class NetworkProtectionTunnelController: TunnelController {
final class NetworkProtectionTunnelController: TunnelController, TunnelSessionProvider {
static var shouldSimulateFailure: Bool = false

private var internalManager: NETunnelProviderManager?
private let debugFeatures = NetworkProtectionDebugFeatures()
private let tokenStore: NetworkProtectionKeychainTokenStore
private let errorStore = NetworkProtectionTunnelErrorStore()
private let notificationCenter: NotificationCenter = .default
private var previousStatus: NEVPNStatus = .invalid
private var cancellables = Set<AnyCancellable>()

// MARK: - Manager, Session, & Connection

/// The tunnel manager: will try to load if it its not loaded yet, but if one can't be loaded from preferences,
/// a new one will not be created. This is useful for querying the connection state and information without triggering
/// a VPN-access popup to the user.
///
@MainActor var tunnelManager: NETunnelProviderManager? {
get async {
if let internalManager {
return internalManager
}

let loadedManager = try? await NETunnelProviderManager.loadAllFromPreferences().first
internalManager = loadedManager
return loadedManager
}
}

public var connection: NEVPNConnection? {
get async {
await tunnelManager?.connection
}
}

public func activeSession() async -> NETunnelProviderSession? {
await session
}

public var session: NETunnelProviderSession? {
get async {
guard let manager = await tunnelManager, let session = manager.connection as? NETunnelProviderSession else {
return nil
}

return session
}
}

// MARK: - Starting & Stopping the VPN

enum StartError: LocalizedError, CustomNSError {
Expand Down Expand Up @@ -83,6 +122,7 @@ final class NetworkProtectionTunnelController: TunnelController {
init(accountManager: AccountManager, tokenStore: NetworkProtectionKeychainTokenStore) {
self.tokenStore = tokenStore
subscribeToStatusChanges()
subscribeToConfigurationChanges()
}

/// Starts the VPN connection used for Network Protection
Expand All @@ -106,7 +146,7 @@ final class NetworkProtectionTunnelController: TunnelController {
}

func stop() async {
guard let tunnelManager = await loadTunnelManager() else {
guard let tunnelManager = await self.tunnelManager else {
return
}

Expand Down Expand Up @@ -139,8 +179,7 @@ final class NetworkProtectionTunnelController: TunnelController {

var isInstalled: Bool {
get async {
let tunnelManager = await loadTunnelManager()
return tunnelManager != nil
return await self.tunnelManager != nil
}
}

Expand All @@ -150,7 +189,7 @@ final class NetworkProtectionTunnelController: TunnelController {
///
var isConnected: Bool {
get async {
guard let tunnelManager = await loadTunnelManager() else {
guard let tunnelManager = await self.tunnelManager else {
return false
}

Expand All @@ -174,7 +213,7 @@ final class NetworkProtectionTunnelController: TunnelController {

switch tunnelManager.connection.status {
case .invalid:
reloadTunnelManager()
clearInternalManager()
try await startWithError()
case .connected:
// Intentional no-op
Expand All @@ -184,10 +223,8 @@ final class NetworkProtectionTunnelController: TunnelController {
}
}

/// Reloads the tunnel manager from preferences.
///
private func reloadTunnelManager() {
internalTunnelManager = nil
private func clearInternalManager() {
internalManager = nil
}
samsymons marked this conversation as resolved.
Show resolved Hide resolved

private func start(_ tunnelManager: NETunnelProviderManager) throws {
Expand Down Expand Up @@ -224,35 +261,11 @@ final class NetworkProtectionTunnelController: TunnelController {
}
}

/// The actual storage for our tunnel manager.
///
private var internalTunnelManager: NETunnelProviderManager?

/// The tunnel manager: will try to load if it its not loaded yet, but if one can't be loaded from preferences,
/// a new one will not be created. This is useful for querying the connection state and information without triggering
/// a VPN-access popup to the user.
///
private var tunnelManager: NETunnelProviderManager? {
get async {
guard let tunnelManager = internalTunnelManager else {
let tunnelManager = await loadTunnelManager()
internalTunnelManager = tunnelManager
return tunnelManager
}

return tunnelManager
}
}

private func loadTunnelManager() async -> NETunnelProviderManager? {
try? await NETunnelProviderManager.loadAllFromPreferences().first
}

private func loadOrMakeTunnelManager() async throws -> NETunnelProviderManager {
guard let tunnelManager = await tunnelManager else {
let tunnelManager = NETunnelProviderManager()
try await setupAndSave(tunnelManager)
internalTunnelManager = tunnelManager
internalManager = tunnelManager
return tunnelManager
}

Expand All @@ -262,12 +275,7 @@ final class NetworkProtectionTunnelController: TunnelController {

private func setupAndSave(_ tunnelManager: NETunnelProviderManager) async throws {
setup(tunnelManager)
try await saveToPreferences(tunnelManager)
try await loadFromPreferences(tunnelManager)
try await saveToPreferences(tunnelManager)
}

private func saveToPreferences(_ tunnelManager: NETunnelProviderManager) async throws {
do {
try await tunnelManager.saveToPreferences()
} catch {
Expand All @@ -281,9 +289,7 @@ final class NetworkProtectionTunnelController: TunnelController {
}
throw StartError.saveToPreferencesFailed(error)
}
}

private func loadFromPreferences(_ tunnelManager: NETunnelProviderManager) async throws {
do {
try await tunnelManager.loadFromPreferences()
} catch {
Expand Down Expand Up @@ -311,6 +317,31 @@ final class NetworkProtectionTunnelController: TunnelController {
tunnelManager.onDemandRules = [NEOnDemandRuleConnect()]
}

// MARK: - Observing Configuration Changes

private func subscribeToConfigurationChanges() {
notificationCenter.publisher(for: .NEVPNConfigurationChange)
.receive(on: DispatchQueue.main)
.sink { _ in
Task { @MainActor in
guard let manager = await self.internalManager else {
return
}

do {
try await manager.loadFromPreferences()

if manager.connection.status == .invalid {
self.clearInternalManager()
}
Comment on lines +334 to +336
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the key to making sure that the shared manager is always valid. This publisher will clear it out, and the next time it's accessed then a new one will be created (assuming we have a configuration at all).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking good - exactly as in macOS so I'm hoping that means it won't cause trouble.

} catch {
self.clearInternalManager()
}
}
}
.store(in: &cancellables)
}

// MARK: - Observing Status Changes

private func subscribeToStatusChanges() {
Expand Down
Loading