Migrate settings to Version3 and introduce incremental migration scheme.

[buggmagnet]

This PR introduces a version 3 of the settings that includes UDP over TCP obfuscation settings.
This PR also introduces back the migration scheme.
I took a couple of decisions in this PR that we already discussed beforehand, but for the sake of documentation

• We do not handle a special migration from V1 to V2 anymore, because there's a very low chance we still have users running on settings V1.
• V1 to V2 upgrade is just taking the compatible settings forward (such as DNS options and Relay Filter)

Open question

How do we want to handle version downgrade ? That's mostly a question for ourselves since users cannot downgrade the app.
I decided to go for nothing to migrate, but we can also trigger a reset of the settings. I think it wouldn't be convenient for us though.

This PR is mostly done, but there is some refactoring I want to do in MigrationManager to avoid reading the settings twice for example.

---

This change is 

[linear]

IOS-356 Create WireGuard Obfuscation settings

[buggmagnet]

Reviewable status: 0 of 15 files reviewed, 1 unresolved discussion

---

ios/MullvadSettings/TunnelSettings.swift line 16 at r2 (raw file):

/// Protocol all TunnelSettings must adhere to, for upgrade purposes.
public protocol TunnelSettings: Codable {

This is intentionally close to the protocol defined in Migration.swift
At the moment, we don't need a migration type that involves doing API calls.
If we ever do, we can make TunnelSettings adhere to that protocol, or create migration classes that implement the protocol with minimal amount of changes.

[rablador]

Reviewed 14 of 14 files at r1, 4 of 4 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadSettings/MigrationManager.swift line 89 at r1 (raw file):

        }

        var versionTypeCopy = savedSchema

Why all this variable juggling?

---

ios/MullvadSettings/SettingsManager.swift line 36 at r1 (raw file):

    #else
    public static let store: SettingsStore = KeychainSettingsStore(

Can't we just change this to a var and call it a day?

---

ios/MullvadSettings/TunnelSettings.swift line 16 at r2 (raw file):

Previously, buggmagnet wrote…

This is intentionally close to the protocol defined in Migration.swift
At the moment, we don't need a migration type that involves doing API calls.
If we ever do, we can make TunnelSettings adhere to that protocol, or create migration classes that implement the protocol with minimal amount of changes.

Not following this. And even if the reasoning is sound, no implementation so far uses any of the supplied arguments.

---

ios/MullvadVPNTests/MigrationManagerTests.swift line 48 at r1 (raw file):

        try SettingsManager.writeSettings(settings)

        let nothingToMigrationExpectation = expectation(description: "No migration")

Nit: "migrate"

---

ios/MullvadVPNTests/MigrationManagerTests.swift line 127 at r1 (raw file):

    }

    private func migrateToLatest(_ settings: any TunnelSettings, version: SchemaVersion) throws {

Nit-ish: version can probably be derived from TunnelSettings instead. That way there can be no discrepancies in the supplied args.

[buggmagnet]

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @rablador)

---

ios/MullvadSettings/MigrationManager.swift line 89 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Why all this variable juggling?

I'm not sure what to answer. Those variables are reused in the repeat loop to incrementally upgrade the settings.
How would you write this ?

---

ios/MullvadSettings/SettingsManager.swift line 36 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Can't we just change this to a var and call it a day?

Ideally we shouldn't have to do this, but since SettingsManager is architectured to be globally available it's complicated to use it, or use any components that depend on it in tests.
I didn't want to refactor SettingsManager so this is the 2nd best approach I could think of to avoid changing it, but still make it safely usable in tests.

---

ios/MullvadSettings/TunnelSettings.swift line 16 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

Not following this. And even if the reasoning is sound, no implementation so far uses any of the supplied arguments.

You are correct. I wanted to get the team's feeling on that, happy to remove all the currently unused arguments.

---

ios/MullvadVPNTests/MigrationManagerTests.swift line 48 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: "migrate"

🫠 I made that mistake everywhere

---

ios/MullvadVPNTests/MigrationManagerTests.swift line 127 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit-ish: version can probably be derived from TunnelSettings instead. That way there can be no discrepancies in the supplied args.

Not sure what you mean by that. TunnelSettings do not provide a version attached to it.
Do you mean that we should include the version in the TunnelSettings protocol ?

[rablador]

Reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @buggmagnet)

---

ios/MullvadSettings/MigrationManager.swift line 89 at r1 (raw file):

Previously, buggmagnet wrote…

I'm not sure what to answer. Those variables are reused in the repeat loop to incrementally upgrade the settings.
How would you write this ?

guard var savedSchema and var savedSettings would eliminate the need for both versionTypeCopy and latestSettings.

---

ios/MullvadSettings/SettingsManager.swift line 36 at r1 (raw file):

Previously, buggmagnet wrote…

Ideally we shouldn't have to do this, but since SettingsManager is architectured to be globally available it's complicated to use it, or use any components that depend on it in tests.
I didn't want to refactor SettingsManager so this is the 2nd best approach I could think of to avoid changing it, but still make it safely usable in tests.

Well, I tried my own suggestion and it seems ApplicationSecurityGroupIdentifier is nil at the time of unit test setup, so that clearly didn't work...

---

ios/MullvadSettings/TunnelSettings.swift line 16 at r2 (raw file):

Previously, buggmagnet wrote…

You are correct. I wanted to get the team's feeling on that, happy to remove all the currently unused arguments.

I'll let the others chime in, but I don't think we should keep them.

---

ios/MullvadVPNTests/MigrationManagerTests.swift line 127 at r1 (raw file):

Previously, buggmagnet wrote…

Not sure what you mean by that. TunnelSettings do not provide a version attached to it.
Do you mean that we should include the version in the TunnelSettings protocol ?

We could do something like the snippet below to determine version instead. It's not super elegant, but that way we don't need to specify version twice att call, ie migrateToLatest(TunnelSettingsV1), reducing potential discrepancies and bugs.

Code snippet:

switch settings {
    case is TunnelSettingsV1:
[...]

[pronebird]

Reviewed 2 of 14 files at r1.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadSettings/MigrationManager.swift line 74 at r4 (raw file):

        // Special case downgrade attempts as nothing to do
        guard settingsVersion < SchemaVersion.current.rawValue else {

!= is the correct operator here. App downgrades should reset all settings, you have a condition below that checks for unknown version.

---

ios/MullvadSettings/TunnelSettings.swift line 16 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

I'll let the others chime in, but I don't think we should keep them.

If the method is not async nor has a completion handler then passing any REST facilities is pointless imo, because they are asynchronous and especially. Just remove the unused parameters as @rablador said.

---

ios/MullvadSettings/TunnelSettings.swift line 32 at r4 (raw file):

    case v2 = 2

    /// V2 format with wireGuard obfuscation options, stored as `TunnelSettingsV3`.

Ultra nit: wireguard is typically written with a capital W, i.e WireGuard

---

ios/MullvadSettings/TunnelSettings.swift line 51 at r4 (raw file):

    }

    var nextVersionType: any TunnelSettings.Type {

This is the same as nextVersion.settingsType

[buggmagnet]

Reviewable status: 10 of 16 files reviewed, 6 unresolved discussions (waiting on @pronebird and @rablador)

---

ios/MullvadSettings/MigrationManager.swift line 89 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

guard var savedSchema and var savedSettings would eliminate the need for both versionTypeCopy and latestSettings.

Oh good point ! That's still readable with less variables.

---

ios/MullvadSettings/MigrationManager.swift line 74 at r4 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

!= is the correct operator here. App downgrades should reset all settings, you have a condition below that checks for unknown version.

Ok, I wasn't 100% if this was the desired behaviour, but thinking about it longer, it's easier to wipe settings on downgrade than not doing anything, and potentially having inconsistent and weird results.

---

ios/MullvadSettings/SettingsManager.swift line 36 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Well, I tried my own suggestion and it seems ApplicationSecurityGroupIdentifier is nil at the time of unit test setup, so that clearly didn't work...

Yes, we could set an identifier in the bundle for UnitTests but ehh.

---

ios/MullvadSettings/TunnelSettings.swift line 16 at r2 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

If the method is not async nor has a completion handler then passing any REST facilities is pointless imo, because they are asynchronous and especially. Just remove the unused parameters as @rablador said.

Done

---

ios/MullvadSettings/TunnelSettings.swift line 32 at r4 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

Ultra nit: wireguard is typically written with a capital W, i.e WireGuard

Genuine mistake, thanks for catching it !

---

ios/MullvadSettings/TunnelSettings.swift line 51 at r4 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

This is the same as nextVersion.settingsType

Actually I ended up not using that, and will remove it

[buggmagnet]

Reviewable status: 10 of 16 files reviewed, 6 unresolved discussions (waiting on @pronebird and @rablador)

---

ios/MullvadVPNTests/MigrationManagerTests.swift line 127 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

We could do something like the snippet below to determine version instead. It's not super elegant, but that way we don't need to specify version twice att call, ie migrateToLatest(TunnelSettingsV1), reducing potential discrepancies and bugs.

I see what you mean, but I think it would be awkward if we had more than 3 versions (which we probably will have in the future) we would have to keep updating this function with more switch cases and wouldn't scale well IMHO

[pronebird]

Reviewed 2 of 14 files at r1.
Reviewable status: 10 of 16 files reviewed, 6 unresolved discussions (waiting on @rablador)

---

ios/MullvadSettings/WireGuardObfuscationSettings.swift line 11 at r4 (raw file):

import Foundation

public enum WireGuardObfuscationState: Codable {

FYI

It's designed differently on desktop, selected obfuscation method can be off, auto, udp2tcp.

Technically more obfuscation protocols will be added in the future so the list can be extended.

Port setting you have basically apply to udp2tcp - that's just a particular kind of obfuscator.

Maybe it's simpler this way for now and you can always do migration. I don't know. But I would be remiss if I didn't mention that.

[buggmagnet]

Reviewable status: 10 of 16 files reviewed, 5 unresolved discussions (waiting on @pronebird and @rablador)

---

ios/MullvadSettings/WireGuardObfuscationSettings.swift line 11 at r4 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

FYI

It's designed differently on desktop, selected obfuscation method can be off, auto, udp2tcp.

Technically more obfuscation protocols will be added in the future so the list can be extended.

Port setting you have basically apply to udp2tcp - that's just a particular kind of obfuscator.

Maybe it's simpler this way for now and you can always do migration. I don't know. But I would be remiss if I didn't mention that.

Fair enough, thanks for letting me know.
As you pointed out, I went for simplicity this time. Let's see what the future holds out, but I'm not too worried in how complex it will be to update to.

[rablador]

Reviewed 6 of 6 files at r5, 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @pronebird)