Make remote config accessible to background agents #3255
Conversation
| var coordinatorError: NSError? | ||
| var writeError: Error? | ||
|
|
||
| NSFileCoordinator().coordinate(writingItemAt: file, options: .forReplacing, error: &coordinatorError) { fileUrl in |
There was a problem hiding this comment.
Let's check that this persist function overall is never called on the main thread, since the coordinate function will block that thread until it can complete what it's trying to do.
There was a problem hiding this comment.
I tracked the persist calls all the way up to here. Basically the full fetch then store process is initiated by the AppConfigurationFetch within a Task. AFAIK that should put it off the main thread.
PacketTunnelProvider/NetworkProtection/NetworkProtectionPacketTunnelProvider.swift
Outdated
Show resolved
Hide resolved
PacketTunnelProvider/NetworkProtection/NetworkProtectionPacketTunnelProvider.swift
Outdated
Show resolved
Hide resolved
| if privacyConfigurationManager.privacyConfig.isSubfeatureEnabled(BackgroundAgentPixelTestSubfeature.pixelTest) { | ||
| DailyPixel.fire(pixel: .networkProtectionConfigurationPixelTest) | ||
| } |
There was a problem hiding this comment.
I think it's possible we'll still miss calls to this pixel - this publisher is only called when we receive updates, so if no Privacy Config update is published on a given day then we'll get no events coming through.
We could probably add the BackgroundAgentPixelTestSubfeature.pixelTest check and pixel call somewhere that we know will get called regularly, like handleConnectionStatusChange. That said, we only really care about getting these calls on days where we're changing the rollout param and at that point we will get this publisher called, so I'd be fine with it staying as-is.
There was a problem hiding this comment.
Struggling a bit to follow what pixelTest is.
| var embeddedConfigData: Data { | ||
| let configString = """ | ||
| { | ||
| "readme": "https://github.com/duckduckgo/privacy-configuration", | ||
| "version": 1693838894358, | ||
| "features": { | ||
| "networkProtection": { | ||
| "state": "enabled", | ||
| "exceptions": [], | ||
| "settings": {} | ||
| } | ||
| }, | ||
| "unprotectedTemporary": [] | ||
| } | ||
| """ | ||
| let data = configString.data(using: .utf8) | ||
| return data! | ||
| } |
There was a problem hiding this comment.
I think we should close this gap as quickly as possible; from my perspective we can live with this in the short term, at least as part of running the pixel rollout test, but I'd advocate for us getting real embedded data working ASAP. Would you be able to scope the changes required for this for both iOS and macOS?
There was a problem hiding this comment.
Follow up opened here: https://app.asana.com/0/1200890834746050/1208305934034181/f
There was a problem hiding this comment.
This class could use unit tests, though I acknowledge there's no good test suite set up for it right now. I'll defer to @diegoreymendez on whether he'd like to block on this, as primary of the VPN AOR.
There was a problem hiding this comment.
Agree that it'd be ideal to have tests here, but I'm not confident the effort to implement those tests is worth additional delay here. Especially because the effort involved isn't completely clear to me and I don't want to be blocking things here.
| private let defaults = UserDefaults(suiteName: "com.duckduckgo.blocker-list.etags") | ||
| private let defaults = UserDefaults(suiteName: "\(Global.groupIdPrefix).app-configuration") |
There was a problem hiding this comment.
Is this a risky transition / migration? We're moving from using an existing user defaults suite, to using a new one.
Is there anything stored here that will get reset that might cause trouble for users?
There was a problem hiding this comment.
I don't think there's a risk here. Configuration starts up and fetches all configs at app launch. For an app update this will look like a fresh install for the configs. It will see no etags and just fetch the current versions and store them. I haven't run in to any issues during testing which involved a "migration".
| @@ -22,16 +22,30 @@ import Configuration | |||
|
|
|||
| public class FileStore { | |||
|
|
|||
| private let groupIdentifier: String = ContentBlockerStoreConstants.groupName | |||
| private let groupIdentifier: String = ContentBlockerStoreConstants.configurationGroupName | |||
There was a problem hiding this comment.
Since this is used for the file paths: is there anything that will get reset here that might cause trouble for users?
There was a problem hiding this comment.
Same as the etags. The module will see no files and download fresh ones. In the case of content blocking/privacy config we have embedded fallbacks while we wait for downloads.
| @@ -519,6 +519,7 @@ import os.log | |||
| ContentBlocking.shared.contentBlockingManager.scheduleCompilation() | |||
| AppConfigurationFetch.shouldScheduleRulesCompilationOnAppLaunch = false | |||
| } | |||
| AppDependencyProvider.shared.configurationManager.loadPrivacyConfigFromDiskIfNeeded() | |||
There was a problem hiding this comment.
Should this happen before AppDependencyProvider.shared.configurationManager is used in line 295?
There was a problem hiding this comment.
It doesn't matter. We don't use the built in timer of the configuration manager because of background app refresh. And even if we did the only purpose of this method is to diff the stored etag vs the etag in the privacy config manager and load fresh from disk if needed.
There was a problem hiding this comment.
Agree that it'd be ideal to have tests here, but I'm not confident the effort to implement those tests is worth additional delay here. Especially because the effort involved isn't completely clear to me and I don't want to be blocking things here.
| if privacyConfigurationManager.privacyConfig.isSubfeatureEnabled(BackgroundAgentPixelTestSubfeature.pixelTest) { | ||
| DailyPixel.fire(pixel: .networkProtectionConfigurationPixelTest) | ||
| } |
There was a problem hiding this comment.
Struggling a bit to follow what pixelTest is.
<!-- Note: This checklist is a reminder of our shared engineering expectations. --> Please review the release process for BrowserServicesKit [here](https://app.asana.com/0/1200194497630846/1200837094583426). **Required**: Task/Issue URL: https://app.asana.com/0/1203581873609357/1207165680693234/f iOS PR: duckduckgo/iOS#3255 macOS PR: duckduckgo/macos-browser#3124 What kind of version bump will this require?: Major **Optional**: Tech Design URL: CC: **Description**: This PR modularizes the config to allow it to be used in background processes <!-- Tagging instructions If this PR isn't ready to be merged for whatever reason it should be marked with the `DO NOT MERGE` label (particularly if it's a draft) If it's pending Product Review/PFR, please add the `Pending Product Review` label. If at any point it isn't actively being worked on/ready for review/otherwise moving forward (besides the above PR/PFR exception) strongly consider closing it (or not opening it in the first place). If you decide not to close it, make sure it's labelled to make it clear the PRs state and comment with more information. --> **Steps to test this PR**: 1. Run the VPN and DBP background agents 2. Look for `Configuration` logs that indicate the config being downloaded or refreshed 3. If the config is setup correctly you can watch for the pixel test as well. 1. <!-- Before submitting a PR, please ensure you have tested the combinations you expect the reviewer to test, then delete configurations you *know* do not need explicit testing. Using a simulator where a physical device is unavailable is acceptable. --> **OS Testing**: * [ ] iOS 14 * [ ] iOS 15 * [ ] iOS 16 * [ ] macOS 10.15 * [ ] macOS 11 * [ ] macOS 12 --- ###### Internal references: [Software Engineering Expectations](https://app.asana.com/0/59792373528535/199064865822552) [Technical Design Template](https://app.asana.com/0/59792373528535/184709971311943)
Task/Issue URL: https://app.asana.com/0/1203581873609357/1207165680693234/f
Tech Design URL:
CC:
Description:
Steps to test this PR:
Definition of Done (Internal Only):
Copy Testing:
’rather than'Orientation Testing:
Device Testing:
OS Testing:
Theme Testing:
Internal references:
Software Engineering Expectations
Technical Design Template