Resolve location selection against remote locations #764

graeme · 2024-04-05T11:54:05Z

Please review the release process for BrowserServicesKit here.

Required:

Task/Issue URL: https://app.asana.com/0/1203137811378537/1206773442486707/f
iOS PR: duckduckgo/iOS#2688
macOS PR: duckduckgo/macos-browser#2805
What kind of version bump will this require?: Major

Description:

The VPN geoswitching logic currently doesn’t handle the case that a location becomes unavailable. We should fall back to the country, if a city isn’t available, or nearest if the country isn’t available.

Steps to test this PR:
See platform PRs

OS Testing:

Internal references:

Software Engineering Expectations
Technical Design Template

diegoreymendez

I think direction is ok for helping us prevent issues, and I think this direction is great... but this is not recovery. Are we planning to follow up with proper recovery from failed registration / connection attempts?

PS: Just being extra cautious here since I'm the one asking for recovery - if we do add recovery, let's do a follow up PR to avoid this becoming huge / neverending etc. I'm happy to keep things separated.

diegoreymendez · 2024-04-10T06:31:12Z

Sources/NetworkProtection/PacketTunnelProvider.swift

@@ -1041,9 +993,6 @@ open class PacketTunnelProvider: NEPacketTunnelProvider {
            }

            settings.selectedServer = .endpoint(serverName)
-            if case .connected = connectionStatus {
-                try? await updateTunnelConfiguration(environment: settings.selectedEnvironment, serverSelectionMethod: .preferredServer(serverName: serverName), reassert: true)


This change is actually changing behaviour. Why is it no longer necessary to update the tunnel configuration when the server selection changes?

diegoreymendez · 2024-04-10T06:42:40Z

Sources/NetworkProtection/Models/VPNServerSelectionSanitizer.swift

+    func sanitizedServerSelectionMethod() async -> NetworkProtectionServerSelectionMethod
+}
+
+public final class VPNServerSelectionSanitizer: VPNServerSelectionSanitizing {


A few comments here:

Let's add unit tests to cover this class in its entirety.

The name of this class sounds off to me. This does more than just sanitize the selection - it effectively manages the entire selection logic (locations vs servers in settings). Additionally when referencing this in the init parameter below (for PacketTunnelProvider) the instance is named serverSelection, which makes me think you agree. Why not name this something more generic like VPNServerSelector or VPNServerSelectionResolver.

To further expand on point 2, I really think that to the PacketTunnelProvider whether this does sanitization or not is an implementation detail - what matters is that this class provides a server / location to connect to.

Yeah, the naming here gave me a lot of trouble. So just decided to open it up to get opinions. Thanks for the input.

diegoreymendez · 2024-04-10T06:45:45Z

Sources/NetworkProtection/PacketTunnelProvider.swift

-        case .setSelectedServer(let selectedServer):
-            let serverSelectionMethod: NetworkProtectionServerSelectionMethod
-
-            switch selectedServer {
-            case .automatic:
-                serverSelectionMethod = .automatic
-            case .endpoint(let serverName):
-                serverSelectionMethod = .preferredServer(serverName: serverName)
-            }
-
-            Task { @MainActor in
-                if case .connected = connectionStatus {
-                    try? await updateTunnelConfiguration(environment: settings.selectedEnvironment, serverSelectionMethod: serverSelectionMethod, reassert: true)
-                }
-                completionHandler?(nil)
-            }
-        case .setSelectedLocation(let selectedLocation):
-            let serverSelectionMethod: NetworkProtectionServerSelectionMethod
-
-            switch selectedLocation {
-            case .nearest:
-                serverSelectionMethod = .automatic
-            case .location(let location):
-                serverSelectionMethod = .preferredLocation(location)
-            }


Can you explain why this is all going away? Admittedly I'm not looking to deeply into this and opting to ask as I want to avoid making (mistaken) assumptions.

Yes sure. It’s essentially extracting this logic into the… class that has yet to be named ;). Let’s go with ServerSelector. I’ve been trying to avoid adding too much more logic to the PacketTunnelProvider, but this selection logic seemed closely related to the responsibilities of the ServerSelector. The extraction should allow it to all be unit tested too which I’ll do before moving this PR out of draft.

graeme · 2024-04-10T08:42:26Z

Sources/NetworkProtection/PacketTunnelProvider.swift

-                serverSelectionMethod = .preferredLocation(location)
-            }
-
+        case .setSelectedServer, .setSelectedLocation:


Note that this is now handling both server selection and location selection cases.

graeme · 2024-04-10T08:58:43Z

@diegoreymendez I agree about this not fully handling recovery. This however, does add some improvement to the robustness of the location / server selection logic and I’m trying to go for incremental improvements. However, recovering from a failed register request feels a bit more generic and not just relating to locations…

…again though, however, I’m also not 100% sure if this is the right approach or whether the location fallback logic should not be pre-emptive as it currently is, but a reaction of a railed register request.

…ion-selection

* main: Replace unowned reference with weak one (#832) Removes VPN waitlist and beta code (#801) Add `survey` action and Privacy Pro attributes (#826) Autofill engagement KPIs for pixel reporting (#830) Bump C-S-S to 5.17.0 (#828) ensure bookmarks can be shown in top hits (#818) Subscription refactoring (#815)

quanganhdo

Good to see unit tests to cover different scenarios. Left some suggestions to improve code readability.

quanganhdo · 2024-06-07T15:39:45Z

Sources/NetworkProtection/Models/VPNServerSelectionResolver.swift

+        self.vpnSettings = vpnSettings
+    }
+
+    public func resolvedServerSelectionMethod() async -> NetworkProtectionServerSelectionMethod {


Suggestion: Include the server selection fallback logic in a comment to make it more readable.

The VPN geoswitching logic currently doesn’t handle the case that a location becomes unavailable. We should fall back to the country, if a city isn’t available, or nearest if the country isn’t available.

quanganhdo · 2024-06-07T16:03:46Z

Sources/NetworkProtection/Models/VPNServerSelectionResolver.swift

+            break
+        case .endpoint(let string):
+            // Selecting a specific server will override locations setting
+            // Only available in debug


Suggestion: Enforce this logic in the code (e.g. guarding .setSelectedServer(.endpoint(String)) inside ALPHA || DEBUG)

github-actions · 2024-06-15T00:17:08Z

This PR has been inactive for more than 7 days and will be automatically closed 7 days from now.

github-actions · 2024-06-23T00:18:49Z

This PR has been closed after 14 days of inactivity. Feel free to reopen it if you plan to continue working on it or have further discussions.

graeme added 5 commits April 3, 2024 16:21

Add function to ignore locations cache

59359ee

Create NetworkProtectionLocationSelectionUseCase.swift

371d6c3

Update with server + location choice from settings

799caaf

Replace PacketTunnelProvider logic

a820f94

Tidy up location sanitization function a bit

95eeb55

diegoreymendez reviewed Apr 10, 2024

View reviewed changes

graeme commented Apr 10, 2024

View reviewed changes

graeme added 9 commits April 25, 2024 17:30

Merge remote-tracking branch 'origin/main' into graeme/sanitize-locat…

0c93643

…ion-selection

Readd selected server setting

e941390

Update naming to use resolve instead of sanitize

a3693bb

Create MockNetworkProtectionLocationListRepository.swift

8b0b0ed

Make debugEvents non-optional

d402aca

Add some tests

dbc8db6

Create the resolver internally

04d390c

Swiftlint

30e5e62

Merge remote-tracking branch 'origin/main' into graeme/sanitize-locat…

63daeee

…ion-selection

graeme mentioned this pull request May 22, 2024

Resolve location selection against remote locations duckduckgo/macos-browser#2805

Closed

graeme changed the title ~~Sanitize location selection against remote locations~~ Resolve location selection against remote locations May 22, 2024

SwiftLint issues

d2200c9

graeme marked this pull request as ready for review May 24, 2024 12:53

SwiftLint again

cdb946c

graeme requested a review from samsymons May 24, 2024 12:56

graeme mentioned this pull request May 24, 2024

Sanitize location selection against remote locations duckduckgo/iOS#2688

Closed

13 tasks

github-actions bot added the stale label Jun 1, 2024

duckduckgo deleted a comment from github-actions bot Jun 1, 2024

github-actions bot removed the stale label Jun 2, 2024

quanganhdo reviewed Jun 7, 2024

View reviewed changes

github-actions bot added the stale label Jun 15, 2024

github-actions bot closed this Jun 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve location selection against remote locations #764

Resolve location selection against remote locations #764

graeme commented Apr 5, 2024 •

edited

Loading

diegoreymendez left a comment •

edited

Loading

diegoreymendez Apr 10, 2024

diegoreymendez Apr 10, 2024

graeme Apr 10, 2024

diegoreymendez Apr 10, 2024

graeme Apr 10, 2024

graeme Apr 10, 2024

graeme commented Apr 10, 2024

quanganhdo left a comment

quanganhdo Jun 7, 2024

quanganhdo Jun 7, 2024

github-actions bot commented Jun 15, 2024

github-actions bot commented Jun 23, 2024

Resolve location selection against remote locations #764

Resolve location selection against remote locations #764

Conversation

graeme commented Apr 5, 2024 • edited Loading

Internal references:

diegoreymendez left a comment • edited Loading

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

Choose a reason for hiding this comment

graeme commented Apr 10, 2024

quanganhdo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jun 15, 2024

github-actions bot commented Jun 23, 2024

graeme commented Apr 5, 2024 •

edited

Loading

diegoreymendez left a comment •

edited

Loading