Upgrade settings to associate with multi-hop

[mojganii]

This PR introduces the ability to store entry and exit locations in RelayConstraints to implemnt multi-hop feature. Additionally, users can enable or disable this feature through the settings, which will also upgrade the tunnel settings accordingly.
Note that, for backward compatibility, the entry location functionality is not yet implemented. However, in an upcoming PR, we will refactor the related code sections to incorporate entry locations, either.

---

This change is 

[linear]

IOS-689 Upgrade settings schema to associate with multi-hop

[mojganii]

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

---

ios/MullvadREST/Relay/RelaySelectorResult.swift line 12 at r1 (raw file):

import MullvadTypes

public typealias RelaySelectorResult = RelaySelectorMatch

when tweaking RelaySelector to work with entry and exit is done then this type would be something like below :

Code snippet:

struct RelaySelectorResult {
    let entryRelay : RelaySelectorMatch?
    let exitRelay : RelaySelectorMatch
}

[mojganii]

Reviewable status: 0 of 20 files reviewed, 2 unresolved discussions

---

ios/MullvadREST/Relay/RelaySelector.swift line 45 at r1 (raw file):

            let mappedBridges = mapRelays(relays: relaysResponse.bridge.relays, locations: relaysResponse.locations)
            let filteredRelays = applyConstraint(
                constraints.entryLocations ?? constraints.exitLocations,

"When selecting a bridge for API communication, the relay selector should honor if multi hop is enabled or not - if it is, the entry constraints should be used to pick a bridge for API access, otherwise the exit constraints should be used to pick a bridge for API access."

https://linear.app/mullvad/issue/IOS-598/allow-relay-selector-to-select-an-entry-peer

[rablador]

Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @mojganii)

---

ios/MullvadREST/Relay/RelaySelector.swift line 114 at r1 (raw file):

        /**
         Filters relay list using given constraints and selects random relay.

Move this comment to the public evaluate function above.

---

ios/MullvadREST/Relay/RelaySelector.swift line 282 at r1 (raw file):

    /// Produce a list of `RelayWithLocation` items satisfying the given constraints
    private static func applyConstraint<T: AnyRelay>(

applyConstraints seems more appropriate.

---

ios/MullvadREST/Relay/RelaySelector.swift line 338 at r1 (raw file):

    /// Produce a port that is either user provided or randomly selected, satisfying the given constraints.
    private static func applyConstraint(

I think we should call this method applyPortConstraint or something similar to distinguish it from the similarly named function above.

---

ios/MullvadVPNTests/MullvadREST/Relay/RelaySelectorTests.swift line 62 at r1 (raw file):

    }

    func testMultipleLocationsConstraint() throws {

Why was this test removed? We should still test constraints consisting of multiple relays (ie custom lists).

[rablador]

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

---

ios/MullvadTypes/RelayConstraints.swift line 41 at r1 (raw file):

    public init(
        entryLocations: RelayConstraint<UserSelectedRelays>? = nil,

"Sweden" should be default for entry as well.

[rablador]

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

---

ios/MullvadTypes/RelayConstraints.swift line 41 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

"Sweden" should be default for entry as well.

I don't think entry should be optional. Instead, depending on if multihop is enabled or not we can let RelaySelector return a RelaySelectorResult where entry is nil (like your proposal where RelaySelectorResult contains entry and exit variables of type RelaySelectorMatch?), which is then passed to wireguard config later down the line.

Other reviewers, please chime in. :)

[buggmagnet]

Reviewed 20 of 20 files at r1, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @mojganii and @rablador)

a discussion (no related file):
Please add tests in MigrationManagerTests to migrate for the new version added, but also for the missing migrations (v3 and v4)

---

---

ios/MullvadREST/Relay/RelaySelector.swift line 90 at r1 (raw file):

    }

    public enum WireGuard {

What was the reason to add this and the Shadowsocks namespaces ?

---

ios/MullvadREST/Relay/RelaySelector.swift line 338 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

I think we should call this method applyPortConstraint or something similar to distinguish it from the similarly named function above.

Totally agree, good catch !

---

ios/MullvadTypes/RelayConstraints.swift line 23 at r1 (raw file):

}

public struct RelayConstraints: Codable, Equatable {

I think we should leave the CustomDebugStringConvertible conformance, it makes debugging a much nicer experience.

---

ios/MullvadTypes/RelayConstraints.swift line 41 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

I don't think entry should be optional. Instead, depending on if multihop is enabled or not we can let RelaySelector return a RelaySelectorResult where entry is nil (like your proposal where RelaySelectorResult contains entry and exit variables of type RelaySelectorMatch?), which is then passed to wireguard config later down the line.

Other reviewers, please chime in. :)

Agreed, especially if we have a default entry, it doesn't make sense to make the entry point optional.

---

ios/MullvadVPNTests/MullvadREST/Relay/RelaySelectorTests.swift line 62 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Why was this test removed? We should still test constraints consisting of multiple relays (ie custom lists).

+1

[mojganii]

Reviewable status: 11 of 20 files reviewed, 8 unresolved discussions (waiting on @buggmagnet and @rablador)

a discussion (no related file):

Previously, buggmagnet wrote…

Please add tests in MigrationManagerTests to migrate for the new version added, but also for the missing migrations (v3 and v4)

Done.

---

---

ios/MullvadREST/Relay/RelaySelector.swift line 90 at r1 (raw file):

Previously, buggmagnet wrote…

What was the reason to add this and the Shadowsocks namespaces ?

functions tracking were getting hard, I tried to separate them into different namespaces.

---

ios/MullvadREST/Relay/RelaySelector.swift line 114 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Move this comment to the public evaluate function above.

Done.

---

ios/MullvadREST/Relay/RelaySelector.swift line 282 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

applyConstraints seems more appropriate.

Done.

---

ios/MullvadREST/Relay/RelaySelector.swift line 338 at r1 (raw file):

Previously, buggmagnet wrote…

Totally agree, good catch !

fair enough!

---

ios/MullvadTypes/RelayConstraints.swift line 23 at r1 (raw file):

Previously, buggmagnet wrote…

I think we should leave the CustomDebugStringConvertible conformance, it makes debugging a much nicer experience.

having this protocol wouldn't give a trunstable log when there is no acknowledge here if multi-hop is enabled or not

---

ios/MullvadTypes/RelayConstraints.swift line 41 at r1 (raw file):

Previously, buggmagnet wrote…

Agreed, especially if we have a default entry, it doesn't make sense to make the entry point optional.

Making this element required led to having different functions for selecting relays depending on whether multi-hop is enabled or not in the upper layers. The relaySelector should not and does not have any knowledge of whether multi-hop is active or not.

---

ios/MullvadVPNTests/MullvadREST/Relay/RelaySelectorTests.swift line 62 at r1 (raw file):

Previously, buggmagnet wrote…

+1

sorry, It was unintentional.

[rablador]

Reviewed 9 of 9 files at r2, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @buggmagnet and @mojganii)

---

ios/MullvadREST/Relay/RelaySelector.swift line 408 at r2 (raw file):

}

// swiftlint:disable:this file_length

I think we should try to break out a few methods rather than allowing the file to grow. Could we perhaps break out the namespaced things? (shadowsocks, wireguard)

[mojganii]

Reviewable status: 18 of 27 files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadREST/Transport/Shadowsocks/ShadowsocksLoader.swift line 49 at r3 (raw file):

    /// Returns the last used shadowsocks configuration, otherwise a new randomized configuration.
    public func load() throws -> ShadowsocksConfiguration {
        do {

@buggmagnet I don't know what it's the best place to inform shadow socks to choose entryLocations or exitLocations when settings gets updated. shadowSocksConfiguration should update itself upon the multi hop state changes.

[mojganii]

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

---

ios/MullvadREST/Relay/RelaySelector.swift line 408 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

I think we should try to break out a few methods rather than allowing the file to grow. Could we perhaps break out the namespaced things? (shadowsocks, wireguard)

Done.

[buggmagnet]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @acb-mv and @mojganii)

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 267 at r9 (raw file):

Previously, buggmagnet wrote…

The ConnectionData structure should really use the MultihopState enum instead of being a boolean.
Also it seems like this should logically be grouped with the new constraints propagation.

We discussed offline that the ConnectionData would not keep track of the multi hop settings.
Because the packet tunnel actor already uses the relay selector via the RelaySelectorWrapper

[buggmagnet]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @acb-mv, @mojganii, and @rablador)

---

ios/PacketTunnelCore/Actor/ObservedState.swift line 37 at r8 (raw file):

Previously, rablador (Jon Petersson) wrote…

Why? Didn't we agree that packet tunnel would know from the relay constraints what to do?

We discussed offline, we will remove isMultihop from the ObservedState

[rablador]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @acb-mv, @buggmagnet, and @mojganii)

[mojganii]

Reviewable status: 24 of 46 files reviewed, 7 unresolved discussions (waiting on @acb-mv, @buggmagnet, and @rablador)

---

ios/MullvadREST/Transport/Shadowsocks/ShadowsocksLoader.swift line 35 at r9 (raw file):

Previously, buggmagnet wrote…

We really should have tests for this class.
Here we recreate the configuration before we get the new constraints, which should be done the other way,
get the new constraints, then create a new configuration.

Done.

---

ios/MullvadTypes/RelayConstraints.swift line 41 at r1 (raw file):

Previously, buggmagnet wrote…

This was discussed offline.

Done

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 169 at r9 (raw file):

Previously, mojganii wrote…

I can't follow your suggestion. could you please elaborate?

having a object which manages both changes?

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 267 at r9 (raw file):

Previously, buggmagnet wrote…

We discussed offline that the ConnectionData would not keep track of the multi hop settings.
Because the packet tunnel actor already uses the relay selector via the RelaySelectorWrapper

Done.

---

ios/PacketTunnelCore/Actor/ObservedState.swift line 37 at r8 (raw file):

Previously, buggmagnet wrote…

We discussed offline, we will remove isMultihop from the ObservedState

Done

[rablador]

Reviewed 17 of 22 files at r10, 4 of 4 files at r11, 3 of 3 files at r12, all commit messages.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @acb-mv, @buggmagnet, and @mojganii)

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 81 at r11 (raw file):

            relayCache: ipOverrideWrapper,
            multihopState: .off,
            multihopPropagator: multihopUpdater

Nit: In ShadowsocksRelaySelector this is called multihopUpdater. I think we should settle on one definition.

[buggmagnet]

Reviewed 3 of 18 files at r7, 11 of 22 files at r10, 3 of 4 files at r11, 1 of 3 files at r12, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @acb-mv and @mojganii)

---

ios/MullvadREST/Transport/Shadowsocks/ShadowsocksLoader.swift line 36 at r12 (raw file):

        constraintsUpdater.onNewConstraints = { [weak self] newConstraints in
            self?.relayConstraints = newConstraints
            try? self?.cache.clear()

why not call try? self?.clear() directly ?

---

ios/MullvadSettings/MultihopSettings.swift line 43 at r12 (raw file):

public class MultihopUpdater {
    private let mutex = NSLock()
    private var observerList: [MultihopObserver] = []

We already have a class called ObserverList that does this functionality, we should reuse it instead of re-creating the same concept.

---

ios/MullvadTypes/FileCache.swift line 38 at r12 (raw file):

        let fileCoordinator = NSFileCoordinator(filePresenter: nil)
        try fileCoordinator.coordinate(writingItemAt: fileURL, options: [.forReplacing]) { fileURL in
            try Data().write(to: fileURL)

This really should be doing try FileManager.default.removeItem(at: fileURL) instead

---

ios/MullvadVPNTests/MullvadREST/Shadowsocks/ShadowsocksLoaderTests.swift line 64 at r12 (raw file):

    }

    func testReloadConfigOnConstraintUpdate() throws {

This test name is misleading. What this test is really doing is checking that changing constraints clears the cache.
Changing relay constraints doesn't reload the cached configuration anymore.

We should rename this testConstraintsUpdateClearsCache IMHO

[rablador]

Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @acb-mv, @buggmagnet, and @mojganii)

---

ios/MullvadREST/Transport/Shadowsocks/ShadowsocksLoader.swift line 36 at r12 (raw file):

Previously, buggmagnet wrote…

why not call try? self?.clear() directly ?

My thought too, but I figured that maybe clear might be updated at some point and then it would do other undesired things.

---

ios/MullvadTypes/FileCache.swift line 38 at r12 (raw file):

Previously, buggmagnet wrote…

This really should be doing try FileManager.default.removeItem(at: fileURL) instead

+1

[mojganii]

Reviewable status: 41 of 46 files reviewed, 10 unresolved discussions (waiting on @acb-mv, @buggmagnet, and @rablador)

---

ios/MullvadREST/Transport/Shadowsocks/ShadowsocksLoader.swift line 36 at r12 (raw file):

Previously, buggmagnet wrote…

why not call try? self?.clear() directly ?

we have the same functionality which is exposed for the external calling and this block and that function should have done the same behaviour.

---

ios/MullvadSettings/MultihopSettings.swift line 43 at r12 (raw file):

Previously, buggmagnet wrote…

We already have a class called ObserverList that does this functionality, we should reuse it instead of re-creating the same concept.

the responsibility of the class is listening to the changes and propagate them to listeners but observer list is an array of observers which keeps their reference, In my opinion they have single/separate responsibility. If I couldn't follow your suggestion then elaborate more, please.

---

ios/MullvadTypes/FileCache.swift line 38 at r12 (raw file):

Previously, buggmagnet wrote…

This really should be doing try FileManager.default.removeItem(at: fileURL) instead

It can be but the deleting has more cost.I was waiting to know other's opinion to choose between clear content or delete file

---

ios/MullvadVPNTests/MullvadREST/Shadowsocks/ShadowsocksLoaderTests.swift line 64 at r12 (raw file):

Previously, buggmagnet wrote…

This test name is misleading. What this test is really doing is checking that changing constraints clears the cache.
Changing relay constraints doesn't reload the cached configuration anymore.

We should rename this testConstraintsUpdateClearsCache IMHO

Done.

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 81 at r11 (raw file):

Previously, rablador (Jon Petersson) wrote…

Nit: In ShadowsocksRelaySelector this is called multihopUpdater. I think we should settle on one definition.

Right, It's slipped to be renamed.

[rablador]

Reviewed 5 of 5 files at r13, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @acb-mv and @buggmagnet)

[rablador]

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @acb-mv and @buggmagnet)

---

ios/MullvadSettings/MultihopSettings.swift line 43 at r12 (raw file):

Previously, mojganii wrote…

the responsibility of the class is listening to the changes and propagate them to listeners but observer list is an array of observers which keeps their reference, In my opinion they have single/separate responsibility. If I couldn't follow your suggestion then elaborate more, please.

I didn't know or forgot that we have a class for this! The concept does look very similar to this implementation. If it's not too much work to use the existing one I think it might be a good idea to reuse it.

[buggmagnet]

Reviewed 3 of 22 files at r10, 1 of 5 files at r13, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @acb-mv, @mojganii, and @rablador)

---

ios/MullvadREST/Transport/Shadowsocks/ShadowsocksLoader.swift line 36 at r12 (raw file):

Previously, mojganii wrote…

we have the same functionality which is exposed for the external calling and this block and that function should have done the same behaviour.

I'm sorry I don't undertstand what you mean.
We shouldn't have more than 1 way to clear the cache, whether it's internal or external.
So there's no reason to not do try? self?.clear() instead here.

---

ios/MullvadTypes/FileCache.swift line 38 at r12 (raw file):

deleting has more cost
That's not correct. Without going too much into the details, "deleting a file" basically tells the system that the contents of that file can be overwritten later, and that the file can be marked as cleared, whereas "writing empty content" into a file needs the system to open the file, write the content, truncate it" etc...

In general, we shouln't make such assumptions without measuring first.
That being said,

We can't just clear the file like that, we still need to use the FileCoordinator to guarantee safe cross process access.

i.e what needs to be done here is :

    public func clear() throws {
        let fileCoordinator = NSFileCoordinator(filePresenter: nil)
        try fileCoordinator.coordinate(writingItemAt: fileURL, options: [.forReplacing]) { fileURL in
            try FileManager.default.removeItem(at: fileURL)
        }
    }

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 176 at r13 (raw file):

        let multihopState = try? settingsReader.read().multihopState

        multihopStateListener.onNewMultihop?(multihopState ?? .off)

Why are we doing this here ? That doesn't really make sense.
The listener updates should only happen when the user selects new relay constraints, if we're doing this so that ShadowsocksRelaySelector has the correct value, then we should dependency inject it when we create it instead of propagating fake updates.

[mojganii]

Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @acb-mv and @buggmagnet)

---

ios/MullvadREST/Transport/Shadowsocks/ShadowsocksLoader.swift line 36 at r12 (raw file):

Previously, buggmagnet wrote…

I'm sorry I don't undertstand what you mean.
We shouldn't have more than 1 way to clear the cache, whether it's internal or external.
So there's no reason to not do try? self?.clear() instead here.

I made a mistake. yes, It should call try? self?.clear(). I mixed up

---

ios/MullvadTypes/FileCache.swift line 38 at r12 (raw file):

Previously, buggmagnet wrote…

deleting has more cost
That's not correct. Without going too much into the details, "deleting a file" basically tells the system that the contents of that file can be overwritten later, and that the file can be marked as cleared, whereas "writing empty content" into a file needs the system to open the file, write the content, truncate it" etc...

In general, we shouln't make such assumptions without measuring first.
That being said,

We can't just clear the file like that, we still need to use the FileCoordinator to guarantee safe cross process access.

i.e what needs to be done here is :

    public func clear() throws {
        let fileCoordinator = NSFileCoordinator(filePresenter: nil)
        try fileCoordinator.coordinate(writingItemAt: fileURL, options: [.forReplacing]) { fileURL in
            try FileManager.default.removeItem(at: fileURL)
        }
    }

shouldn't the writing option be forDeleting?

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 176 at r13 (raw file):

Previously, buggmagnet wrote…

Why are we doing this here ? That doesn't really make sense.
The listener updates should only happen when the user selects new relay constraints, if we're doing this so that ShadowsocksRelaySelector has the correct value, then we should dependency inject it when we create it instead of propagating fake updates.

I did this before through DI, but I'll do that instead of faking update

[buggmagnet]

Reviewed 1 of 3 files at r12.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @acb-mv, @mojganii, and @rablador)

---

ios/MullvadTypes/FileCache.swift line 38 at r12 (raw file):

Previously, mojganii wrote…

shouldn't the writing option be forDeleting?

Yes it should, good catch !

[rablador]

Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @acb-mv and @mojganii)

[mojganii]

Reviewable status: 36 of 52 files reviewed, 9 unresolved discussions (waiting on @acb-mv, @buggmagnet, and @rablador)

---

ios/MullvadSettings/MultihopSettings.swift line 43 at r12 (raw file):

Previously, rablador (Jon Petersson) wrote…

I didn't know or forgot that we have a class for this! The concept does look very similar to this implementation. If it's not too much work to use the existing one I think it might be a good idea to reuse it.

done!
I've forgotten which we had this.

---

ios/MullvadTypes/FileCache.swift line 38 at r12 (raw file):

Previously, buggmagnet wrote…

Yes it should, good catch !

Done.

---

ios/MullvadTypes/ObserverList.swift line 65 at r14 (raw file):

    }

    public func notify(_ body: (T) -> Void) {

forEach was not well-known, I renamed that to notify for the clarification.

[buggmagnet]

Reviewed 1 of 5 files at r13, 15 of 16 files at r14, 1 of 1 files at r15, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @acb-mv)

[buggmagnet]

Reviewed 5 of 5 files at r16, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @acb-mv)

[github-actions]

🚨 End to end tests failed. Please check the failed workflow run.