Sanitize location selection against remote locations #764
Conversation
There was a problem hiding this comment.
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.
| @@ -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) | |||
There was a problem hiding this comment.
This change is actually changing behaviour. Why is it no longer necessary to update the tunnel configuration when the server selection changes?
| func sanitizedServerSelectionMethod() async -> NetworkProtectionServerSelectionMethod | ||
| } | ||
|
|
||
| public final class VPNServerSelectionSanitizer: VPNServerSelectionSanitizing { |
There was a problem hiding this comment.
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 namedserverSelection, which makes me think you agree. Why not name this something more generic likeVPNServerSelectororVPNServerSelectionResolver. - To further expand on point 2, I really think that to the
PacketTunnelProviderwhether this does sanitization or not is an implementation detail - what matters is that this class provides a server / location to connect to.
There was a problem hiding this comment.
Yeah, the naming here gave me a lot of trouble. So just decided to open it up to get opinions. Thanks for the input.
| 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) | ||
| } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| serverSelectionMethod = .preferredLocation(location) | ||
| } | ||
|
|
||
| case .setSelectedServer, .setSelectedLocation: |
There was a problem hiding this comment.
Note that this is now handling both server selection and location selection cases.
|
@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 …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. |
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:
What kind of version bump will this require?: Major
Optional:
Tech Design URL:
CC:
Description:
Steps to test this PR:
1.
1.
OS Testing:
Internal references:
Software Engineering Expectations
Technical Design Template