VPN server failure detection recovery #786

graeme · 2024-04-17T21:14:59Z

Please review the release process for BrowserServicesKit here.

Required:

Task/Issue URL: https://app.asana.com/0/72649045549333/1206669340827392/f
iOS PR: duckduckgo/iOS#2779
macOS PR: https://github.com/duckduckgo/macos-browser/pull/2657
What kind of version bump will this require?: Major

Description:

Use the WireGuard handshakes to determine when a connection fails by checking that the last handshake time is less than 300 seconds (just over two handshakes).
When a failure is detected the client will call /register with server set to the name of the current (failed) server and mode set to failureRecovery. If the server is not in the "active" status on the controller a new registration will be returned otherwise the previous data will be returned (this data should be checked against the current tunnel data including allowedIPs).

Steps to test this PR:

Launch the VPN and connect
Block UDP traffic by following these instructions: https://app.asana.com/0/0/1207029295107667/f
Open the Console.app and filter by Category: Network Protection. The failure recovery logs are indicated with a 🟢 so you can also look for them.
After around 5 minutes you will see logs indicating that a register request has been made.
It will error with no recovery required as there is not a real problem.

OS Testing:

—

Internal references:

Software Engineering Expectations
Technical Design Template

Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift

graeme · 2024-04-18T09:20:38Z

Sources/NetworkProtection/Networking/NetworkProtectionClient.swift


-    init(environment: VPNSettings.SelectedEnvironment = .default, isSubscriptionEnabled: Bool) {
+    init(settings: VPNSettings, isSubscriptionEnabled: Bool) {


This change allowed us to instantiate the client so that we don’t need to re-init the NetworkProtectionDeviceManager every time it's used. Should make testing a bit easier further down the line + also means we can guarantee the client always gets the most up-to-date environment setting

Not sure I love this, though. I don't think we should support real-time updating of the environment while the VPN is connected.

If anything, we should discourage it.

If we want to change the environment the whole VPN should be stopped instead.

How necessary is this change? Can we roll it back?

Do we ever expect to support changing the selected environment while the connection is already established?

Note: this wasn't a problem before because it was possible to change environments, but we no longer can support this, and should in fact remove all the code that allows this to happen when the connection is up.

I also feel like replacing the environment param with settings should at least be considered outside the scope of this PR/branch. It already does a lot and refactoring this might lead to side effect.

I didn’t fully explain the above. The reason why it’s important to not recreate the device manager every time is that we could end up with two register requests happening at the same time which could create racey behaviour. The NetworkProtectionDeviceManager is an actor and it makes sense to just have one instance which will queue up these requests rather than creating new instances every time start, stop or failure recovery happens.

Rreplacing the environment param with settings was so that we could still support changing the environment without needing to restart the PacketTunnelProvider, but @diegoreymendez and I had a chat and we think it’s actually probable more sensible to require restarting as the token we’re using would become invalid.

I’m going to revert the injection of settings instead of environment, but I will still create a single instance of the NetworkProtectionDeviceManager. This will mean that we need to manually restart the tunnel for the env change to take effect.

Sources/NetworkProtection/PacketTunnelProvider.swift

diegoreymendez

Mostly looking good, added some comments - some of which need to be addressed.

I'll re-review and do some extended testing once it's ready again. Since this changes several key elements of the tunnel I want to be extra careful that everything is in place, so I'll be sinking some hours into this.

Sources/NetworkProtection/PacketTunnelProvider.swift

diegoreymendez · 2024-04-23T12:53:11Z

Sources/NetworkProtection/PacketTunnelProvider.swift

@@ -312,7 +312,17 @@ open class PacketTunnelProvider: NEPacketTunnelProvider {
        }
    }()

-    public lazy var tunnelFailureMonitor = NetworkProtectionTunnelFailureMonitor(tunnelProvider: self)
+    lazy var deviceManager: NetworkProtectionDeviceManagement = NetworkProtectionDeviceManager(


Nitpicky, but can we make sure this is private to prevent any form of misuse of this?

Sources/NetworkProtection/PacketTunnelProvider.swift

Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift

Sources/NetworkProtection/PacketTunnelProvider.swift

Sources/NetworkProtection/Recovery/Reasserting.swift

Sources/NetworkProtection/PacketTunnelProvider.swift

diegoreymendez · 2024-04-23T13:30:05Z

Sources/NetworkProtection/PacketTunnelProvider.swift

+    private func handleFailureRecovery(tunnelConfig: TunnelConfiguration, server: NetworkProtectionServer) {
+        self.lastSelectedServer = server
+        Task {
+            try await self.updateAdapterConfig(tunnelConfiguration: tunnelConfig, reassert: true)


What happens here if the adapter fails to update and throws an error?

Should we add handling? Should we fire pixels?

graeme · 2024-04-25T11:14:20Z

Sources/NetworkProtection/Recovery/FailureRecoveryHandler.swift

+            } catch {
+                switch error {
+                case FailureRecoveryError.noRecoveryNecessary:
+                    await reassertingControl?.stopReasserting()


This fixes the bug I mentioned yesterday.

For this type of thing it's really important to ensure calls are always balanced. If a method starts, it also should stop on every exit. The default error case doesn't stop reasserting again.

The reason why this is extremely important is because otherwise you can find issues like this one, where reasserting starts but never again stops.

I see two possible solutions to this:

Add this right after starting the reasserting:

defer { await reassertingControl?.stopReasserting() }

Since we're recovering through all the duration of the checks, why not start reasserting right before calling incrementalPeriodicChecks and stop reasserting right after that call? I guess this could be an issue if something else also stops reassertions, though... so option 1 may be better.

graeme · 2024-04-25T11:16:32Z

Sources/NetworkProtection/PacketTunnelProvider.swift

                                                                            includedRoutes: includedRoutes ?? [],
                                                                            excludedRoutes: settings.excludedRanges,
                                                                            regenerateKey: regenerateKey)
+        } catch {
+            providerEvents.fire(.tunnelUpdateAttempt(.failure(error)))
+            throw error


This means we’ll bubble up rekey failures as mentioned in https://app.asana.com/0/1203137811378537/1207149691501092/f

diegoreymendez

I did some testing. Works well so far but I'll be looking for edge cases.

Adding some comments to request changes.

The most important one is the one about stopping the reasserting.

Sources/NetworkProtection/PacketTunnelProvider.swift

diegoreymendez · 2024-04-25T10:14:59Z

Sources/NetworkProtection/PacketTunnelProvider.swift

+            }
+
+            Task {
+                guard let server = await self.lastSelectedServer else {


Can we obtain the server before spawning the task, so we avoid spawning the task if there's no server?

lastSelectedServer is a @MainActor var so this needs to happen in an async context.

Sources/NetworkProtection/Recovery/Reasserting.swift

Sources/NetworkProtection/Logging/Logging.swift

diegoreymendez · 2024-04-25T10:21:49Z

Sources/NetworkProtection/Networking/NetworkProtectionClient.swift


-    init(environment: VPNSettings.SelectedEnvironment = .default, isSubscriptionEnabled: Bool) {
+    init(settings: VPNSettings, isSubscriptionEnabled: Bool) {


Not sure I love this, though. I don't think we should support real-time updating of the environment while the VPN is connected.

If anything, we should discourage it.

If we want to change the environment the whole VPN should be stopped instead.

How necessary is this change? Can we roll it back?

Do we ever expect to support changing the selected environment while the connection is already established?

Note: this wasn't a problem before because it was possible to change environments, but we no longer can support this, and should in fact remove all the code that allows this to happen when the connection is up.

diegoreymendez · 2024-04-25T19:01:34Z

Sources/NetworkProtection/Recovery/FailureRecoveryHandler.swift

+            } catch {
+                switch error {
+                case FailureRecoveryError.noRecoveryNecessary:
+                    await reassertingControl?.stopReasserting()


For this type of thing it's really important to ensure calls are always balanced. If a method starts, it also should stop on every exit. The default error case doesn't stop reasserting again.

The reason why this is extremely important is because otherwise you can find issues like this one, where reasserting starts but never again stops.

I see two possible solutions to this:

Add this right after starting the reasserting:

defer { await reassertingControl?.stopReasserting() }

Since we're recovering through all the duration of the checks, why not start reasserting right before calling incrementalPeriodicChecks and stop reasserting right after that call? I guess this could be an issue if something else also stops reassertions, though... so option 1 may be better.

diegoreymendez · 2024-04-25T19:04:57Z

Sources/NetworkProtection/Recovery/FailureRecoveryHandler.swift

+                }
+                count += 1
+                currentDelay = min((currentDelay * config.factor), config.maxDelay)
+            } while count < config.times


What's the expected behavior if this is exceeded?

My understanding from what I'm seeing is that:

We try recovery 5 times (Each time we duplicate the previous delay, but never wait more than 5 minutes)

Once the attempts all fail, we bail out, and the failure monitor will keep checking periodically, but we won't do any more active recovery.

Is this correct?

Yeah, this is correct. I was initially throwing an error that gets bubbled up to the PacketTunnelProvider. But I wasn’t reacting to it in any way so decided it wasn’t necessary. I guess we could send another pixel in that case, but I’d rather not just now as it wasn’t in the original scope and will require another privacy triage etc.

diegoreymendez · 2024-04-25T19:05:49Z

Sources/NetworkProtection/Recovery/FailureRecoveryHandler.swift

+                    return
+                } catch {
+                    os_log("🟢 Failure recovery failed. Retrying...", log: .networkProtectionServerFailureRecoveryLog, type: .info)
+                    try? await Task.sleep(interval: currentDelay)


This should not be ignored as its throw implies the task has been cancelled.

diegoreymendez · 2024-04-25T19:07:04Z

Sources/NetworkProtection/Recovery/FailureRecoveryHandler.swift

+            var currentDelay = config.initialDelay
+            var count = 0
+            repeat {
+                do {


Since task cancellation is cooperative, right before the do I'd suggest doing:

try Task.checkCancellation()

Sources/NetworkProtectionTestUtils/MockNetworkProtectionDeviceManagement.swift

diegoreymendez · 2024-04-25T19:37:21Z

Sources/NetworkProtection/NetworkProtectionDeviceManager.swift

@@ -59,12 +63,12 @@ public actor NetworkProtectionDeviceManager: NetworkProtectionDeviceManagement {

    private let isSubscriptionEnabled: Bool

-    public init(environment: VPNSettings.SelectedEnvironment,
+    public init(settings: VPNSettings,


As mentioned above this should be rolled back - we can't support updating the environment without restarting the connection. This change can give the reader the false impression that's possible.

quanganhdo · 2024-04-26T03:00:03Z

Sources/NetworkProtection/Networking/NetworkProtectionClient.swift


-    init(environment: VPNSettings.SelectedEnvironment = .default, isSubscriptionEnabled: Bool) {
+    init(settings: VPNSettings, isSubscriptionEnabled: Bool) {


I also feel like replacing the environment param with settings should at least be considered outside the scope of this PR/branch. It already does a lot and refactoring this might lead to side effect.

Sources/NetworkProtection/NetworkProtectionDeviceManager.swift

Sources/NetworkProtection/PacketTunnelProvider.swift

quanganhdo · 2024-04-26T03:09:35Z

Sources/NetworkProtection/Recovery/FailureRecoveryHandler.swift

+                eventHandler(.completed(.unhealthy))
+                await reassertingControl?.stopReasserting()


In this snippet, the event is emitted before the stopReasserting() is invoked. In the catch block, it happens after the stopReasserting() call. The order of operations should be consistent, if only for readability.

Fair enough, can change that.

Co-authored-by: Anh Do <18567+quanganhdo@users.noreply.github.com>

Tests/NetworkProtectionTests/NetworkProtectionClientTests.swift

…lure-detection-recovery

* main: Add workflow to create Asana PR subtask when GitHub reviewer is assigned (#805) VPN server failure detection recovery (#786)

graeme added 11 commits April 8, 2024 16:35

Small tidy up to remove dependency cycle

bf94293

Inject settings rather than environment to client

369c273

Add failure recovery register request

c1ff07d

Add log category for failure recovery

5b802d8

Label the config+server tuple values

a21a3dd

Store server not just info

bbaaa81

Abstract reasserting from PacketTunnelProvider

5f51518

Create failurerecoveryHandler + empty tests

94e094a

Pull updating adapter config into helper function

beedbde

Pull deviceManager into a lazy var

5159e38

Add failure monitoring and recovery to PTP

4a52c88

graeme commented Apr 18, 2024

View reviewed changes

Sources/NetworkProtection/Diagnostics/NetworkProtectionTunnelFailureMonitor.swift Show resolved Hide resolved

graeme commented Apr 18, 2024

View reviewed changes

Sources/NetworkProtection/PacketTunnelProvider.swift Show resolved Hide resolved

graeme commented Apr 18, 2024

View reviewed changes

Sources/NetworkProtection/PacketTunnelProvider.swift Show resolved Hide resolved

graeme commented Apr 18, 2024

View reviewed changes

Sources/NetworkProtection/PacketTunnelProvider.swift Outdated Show resolved Hide resolved

graeme commented Apr 18, 2024

View reviewed changes

Sources/NetworkProtection/PacketTunnelProvider.swift Outdated Show resolved Hide resolved

graeme added 5 commits April 18, 2024 14:47

Remove location from request

727c271

Don't regenerate the key on failure recovery

78d3286

Fix broken existing tests

95ba950

Write tests for failureRecoveryHandler

ec7a3a1

Add comment about todo pixel

18d6a59

graeme requested a review from diegoreymendez April 18, 2024 15:38

graeme added the DO NOT MERGE label Apr 18, 2024

graeme marked this pull request as ready for review April 18, 2024 15:38

Merge branch 'main' into graeme/vpn-server-failure-detection-recovery

d82cea6

diegoreymendez reviewed Apr 23, 2024

View reviewed changes

graeme added 3 commits April 23, 2024 16:33

Update function name to be more consistent

c729eb1

Make sure tunnelUpdateAttempt pixels are fired

5513d30

Fix swiftlint

72e07ab

graeme added 5 commits April 25, 2024 13:05

Tidy up error handling

e82dc3f

Fix reasserting problem

cb9505f

Swiftlint

4df8447

Another swiftlint fix

62e5ae1

Make devicemanager property private

a88f8ea

graeme commented Apr 25, 2024

View reviewed changes

Remove... x? (wtf)

b26a039

graeme commented Apr 25, 2024

View reviewed changes

graeme removed the DO NOT MERGE label Apr 25, 2024

graeme requested a review from quanganhdo April 25, 2024 15:25

This was referenced Apr 25, 2024

VPN server failure recovery retry logic #789

Closed

VPN server failure recovery pixels + reasserting fix #791

Closed

diegoreymendez requested changes Apr 25, 2024

View reviewed changes

diegoreymendez reviewed Apr 25, 2024

View reviewed changes

quanganhdo reviewed Apr 26, 2024

View reviewed changes

graeme and others added 7 commits April 26, 2024 11:05

Use convenience serverInfo property

a13e884

Co-authored-by: Anh Do <18567+quanganhdo@users.noreply.github.com>

Use the typealiased tuple

7c9ce07

Co-authored-by: Anh Do <18567+quanganhdo@users.noreply.github.com>

Revert to injecting environment not settings

5766031

Always stop reasserting regardless of result

7e80423

Fix retry logic

77b69d0

Better handle task cancellation

6d3410b

Improve readability of GenrateTunnelConfigurationResult

0406ce0

graeme requested a review from diegoreymendez April 29, 2024 09:40

diegoreymendez reviewed Apr 29, 2024

View reviewed changes

Tests/NetworkProtectionTests/NetworkProtectionClientTests.swift Show resolved Hide resolved

Make sure netP convertible errors are converted

acb5485

diegoreymendez approved these changes Apr 29, 2024

View reviewed changes

Merge remote-tracking branch 'origin/main' into graeme/vpn-server-fai…

e0a1456

…lure-detection-recovery

graeme merged commit 2681b52 into main Apr 30, 2024
7 checks passed

graeme deleted the graeme/vpn-server-failure-detection-recovery branch April 30, 2024 09:01

samsymons added a commit that referenced this pull request May 1, 2024

Merge branch 'main' into sam/remove-subscription-bsk-dependency

8a3f2d0

* main: Add workflow to create Asana PR subtask when GitHub reviewer is assigned (#805) VPN server failure detection recovery (#786)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VPN server failure detection recovery #786

VPN server failure detection recovery #786

graeme commented Apr 17, 2024 •

edited

graeme Apr 18, 2024

diegoreymendez Apr 25, 2024 •

edited

quanganhdo Apr 26, 2024

graeme Apr 26, 2024 •

edited

diegoreymendez left a comment

diegoreymendez Apr 23, 2024

diegoreymendez Apr 23, 2024

graeme Apr 23, 2024 •

edited

graeme Apr 25, 2024

diegoreymendez Apr 25, 2024

graeme Apr 25, 2024

diegoreymendez left a comment

diegoreymendez Apr 25, 2024

graeme Apr 26, 2024

diegoreymendez Apr 25, 2024 •

edited

diegoreymendez Apr 25, 2024

diegoreymendez Apr 25, 2024

graeme Apr 26, 2024

diegoreymendez Apr 25, 2024

diegoreymendez Apr 25, 2024

diegoreymendez Apr 25, 2024 •

edited

quanganhdo Apr 26, 2024

quanganhdo Apr 26, 2024

graeme Apr 26, 2024


		init(environment: VPNSettings.SelectedEnvironment = .default, isSubscriptionEnabled: Bool) {
		init(settings: VPNSettings, isSubscriptionEnabled: Bool) {

		eventHandler(.completed(.unhealthy))
		await reassertingControl?.stopReasserting()

VPN server failure detection recovery #786

VPN server failure detection recovery #786

Conversation

graeme commented Apr 17, 2024 • edited

Internal references:

Choose a reason for hiding this comment

diegoreymendez Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graeme Apr 26, 2024 • edited

Choose a reason for hiding this comment

diegoreymendez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graeme Apr 23, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegoreymendez left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegoreymendez Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

diegoreymendez Apr 25, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graeme commented Apr 17, 2024 •

edited

diegoreymendez Apr 25, 2024 •

edited

graeme Apr 26, 2024 •

edited

graeme Apr 23, 2024 •

edited

diegoreymendez Apr 25, 2024 •

edited

diegoreymendez Apr 25, 2024 •

edited