macOS: VPN Metadata Improvements #2704

diegoreymendez · 2024-04-25T15:01:13Z

Task/Issue URL: https://app.asana.com/0/0/1207169059489741/f

iOS: duckduckgo/iOS#2791
BSK: duckduckgo/BrowserServicesKit#799

Description

Add last IPC, controller, tunnel errors to VPN metadata
Updates VPN feedback metadata to include ipsec tunnel count.
Add lastExtensionVersionRun to VPN metadata in the App Store build

Testing

Test 1: IPC Failure in metadata

NOTE: in this test there a UI issue where the toggle is not becoming disabled until you hide and show the VPN popover, but is unrelaed to my changes

Comment this line
Start the app
Remove the login item using launchctl remove <bundle_id>
Try to start the VPN, it should fail
Print the metadata to console and ensure you see the IPC error in it (Error domain=NSCocoaErrorDomain code=4099)

Test 2: Controller Failure in metadata

NOTE: in this test there a UI issue where the toggle is not becoming disabled until you hide and show the VPN popover, but is unrelaed to my changes

Make sure the previously commented line is back to being uncommented
Throw any error here (like StartError.noAuthToken)
Start the app
Try to start the VPN, it should fail
Print the metadata to console and ensure you see the IPC error in it (Error domain=DuckDuckGoVPN.NetworkProtectionTunnelController.StartError code=0)

Test 3: Tunnel Failure in metadata

NOTE: in this test there a UI issue where the toggle is not becoming disabled until you hide and show the VPN popover, but is unrelaed to my changes

Roll back the error we were force throwing in the previous test
Add the following method to MacPacketTunnelProvider.swift

override func startTunnel(options: [String : NSObject]?, completionHandler: @escaping (Error?) -> Void) {
super.startTunnel(options: options, completionHandler: completionHandler)
```
 Task {
     try await Task.sleep(interval: .seconds(5))
     cancelTunnelWithError(NSError(domain: "test", code: 5))
 }
```
}
Start the app
Try to start the VPN, it should fail
Wait at least 5 seconds
Print the metadata to console and ensure you see the IPC error in it (Error domain=test code=5)

Internal references:

Pull Request Review Checklist
Software Engineering Expectations
Technical Design Template
Pull Request Documentation

diegoreymendez · 2024-04-25T15:13:10Z

DuckDuckGo/NetworkProtection/AppAndExtensionAndAgentTargets/VPNOperationErrorRecorder.swift

+import NetworkProtectionIPC
+
+@objc
+final class ErrorInformation: NSObject, Codable {


Privacy friendly error information for reports.

diegoreymendez · 2024-04-25T15:13:19Z

DuckDuckGo/NetworkProtection/AppAndExtensionAndAgentTargets/VPNOperationErrorRecorder.swift

+///
+/// To be used in combination with ``VPNOperationErrorRecorder``
+///
+final class VPNOperationErrorHistory {


To query error history.

diegoreymendez · 2024-04-25T15:13:29Z

DuckDuckGo/NetworkProtection/AppAndExtensionAndAgentTargets/VPNOperationErrorRecorder.swift

+///
+/// To be used in combination with ``VPNOperationErrorHistory``
+///
+final class VPNOperationErrorRecorder {


To store new error history.

diegoreymendez · 2024-04-25T15:15:06Z

DuckDuckGo/VPNFeedbackForm/VPNMetadataCollector.swift

@@ -222,7 +228,7 @@ final class DefaultVPNMetadataCollector: VPNMetadataCollector {
    func collectVPNState() async -> VPNMetadata.VPNState {
        let onboardingState: String

-        switch UserDefaults.netP.networkProtectionOnboardingStatus {


Just making the defaults injectable as we'll eventually want to test this class.

diegoreymendez · 2024-04-25T15:16:35Z

DuckDuckGoVPN/TunnelControllerIPCService.swift

@@ -127,6 +127,19 @@ extension TunnelControllerIPCService: IPCServerInterface {
        completion(nil)
    }

+    func fetchLastError(completion: @escaping (Error?) -> Void) {
+        Task {
+            guard #available(macOS 13.0, *),


This is necessary because fetchLastDisconnectError is not available before macOS 13.

I'm ok with not returning anything if this is not available, since I'm not sure we can properly return something that doesn't also introduce potential noise.

We can do follow-up work on this if necessary.

diegoreymendez · 2024-04-25T15:18:34Z

DuckDuckGo/NetworkProtection/AppAndExtensionAndAgentTargets/VPNOperationErrorRecorder.swift

+    /// The earliest error is the one that best represents the latest failure
+    ///
+    var lastStartError: ErrorInformation? {
+        lastIPCStartError ?? lastControllerStartError


By doing this, we try to make the metadata easier to parse. If there's an IPC error we should report that because it means we never got to the controller (and error information there may be from a previous attempt).

But if there was no IPC error, we should report the controller start error instead.

graeme

Looks good. Thanks for the detailed test steps

Task/Issue URL: https://app.asana.com/0/0/1206897550778458/f iOS PR: duckduckgo/iOS#2791 macOS PR: duckduckgo/macos-browser#2704 What kind of version bump will this require?: Minor ## Description Adds some improved tracking of tunnel interfaces to include `ipsec` tunnels.

Task/Issue URL: https://app.asana.com/0/414235014887631/1207169059489741/f macOS: duckduckgo/macos-browser#2704 BSK: duckduckgo/BrowserServicesKit#799 ## Description Updates VPN feedback metadata to include `ipsec` tunnel count.

…utdown # By Dax the Duck (9) and others # Via GitHub (6) and others * main: (47 commits) Update BSK to 141.1.1 (#2713) macOS: VPN Metadata Improvements (#2704) duck page suggestions (#2666) macOS: Add pixels to track VPN wake and stop attempts (#2694) Trusted url indicator (#2665) remove Tab.TabContent.url (#2647) Bump version to 1.85.0 (176) Update release automation to support Privacy Pro section in release notes (#2710) Fix for VPN stop issues. (#2689) Hard-codes the VPN waitlist flags to ON (#2709) Add subscription status to the macOS metadata (#2680) Bump version to 1.85.0 (175) Call finish in the correct place (#2702) Fix layout issue on DBP (#2700) Update autoconsent to v10.6.1 (#2618) Make Clear All History shortcut available without entering Main Menu (#2682) Fix DataBrokerProtectionProcessor.swift lint Fix SwiftLint Add parameter allowed encoding to error descriptions (#2691) Remove debug flags ... # Conflicts: # DuckDuckGo.xcodeproj/project.pbxproj # DuckDuckGo.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved

diegoreymendez added 2 commits April 25, 2024 16:08

Several VPN metadata improvements

f2dc2aa

Removed a change made by mistake

3466bba

diegoreymendez self-assigned this Apr 25, 2024

diegoreymendez mentioned this pull request Apr 25, 2024

VPN Metadata Improvements duckduckgo/BrowserServicesKit#799

Merged

github-actions bot added the bot: not in app board Added by automation for pull requests with tasks not added to macOS App Board Asana project label Apr 25, 2024

diegoreymendez mentioned this pull request Apr 25, 2024

iOS: VPN Metadata Improvements duckduckgo/iOS#2791

Merged

Updates BSK

04d5c82

diegoreymendez commented Apr 25, 2024

View reviewed changes

Rolls back an unnecessary change

5579331

diegoreymendez commented Apr 25, 2024

View reviewed changes

diegoreymendez changed the title ~~macOS | iOS: VPN Metadata Improvements~~ macOS: VPN Metadata Improvements Apr 25, 2024

diegoreymendez marked this pull request as ready for review April 25, 2024 15:21

diegoreymendez removed the bot: not in app board Added by automation for pull requests with tasks not added to macOS App Board Asana project label Apr 25, 2024

diegoreymendez requested a review from graeme April 25, 2024 15:57

graeme approved these changes Apr 26, 2024

View reviewed changes

diegoreymendez added 2 commits April 26, 2024 18:06

Merges the latest from main and updates BSK

6e33e54

Fixes some failing unit test

2fd14f4

diegoreymendez merged commit 429d897 into main Apr 26, 2024
19 checks passed

diegoreymendez deleted the diego/vpn-metadata-changes branch April 26, 2024 16:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

macOS: VPN Metadata Improvements #2704

macOS: VPN Metadata Improvements #2704

diegoreymendez commented Apr 25, 2024 •

edited

Loading

diegoreymendez Apr 25, 2024

diegoreymendez Apr 25, 2024

diegoreymendez Apr 25, 2024

diegoreymendez Apr 25, 2024

diegoreymendez Apr 25, 2024 •

edited

Loading

diegoreymendez Apr 25, 2024

graeme left a comment

macOS: VPN Metadata Improvements #2704

macOS: VPN Metadata Improvements #2704

Conversation

diegoreymendez commented Apr 25, 2024 • edited Loading

Description

Testing

Test 1: IPC Failure in metadata

Test 2: Controller Failure in metadata

Test 3: Tunnel Failure in metadata

Internal references:

diegoreymendez Apr 25, 2024

Choose a reason for hiding this comment

diegoreymendez Apr 25, 2024

Choose a reason for hiding this comment

diegoreymendez Apr 25, 2024

Choose a reason for hiding this comment

diegoreymendez Apr 25, 2024

Choose a reason for hiding this comment

diegoreymendez Apr 25, 2024 • edited Loading

Choose a reason for hiding this comment

diegoreymendez Apr 25, 2024

Choose a reason for hiding this comment

graeme left a comment

Choose a reason for hiding this comment

diegoreymendez commented Apr 25, 2024 •

edited

Loading

diegoreymendez Apr 25, 2024 •

edited

Loading