Set includeallnetworks to true in starttunneloperation and ios 945

[buggmagnet]

This PR does the following things:

• Turns TunnelMonitor into an actor
• Adds massive amounts of logs that should give us more information about device connectivity in dire situations
• Adds tons of tests to TunnelMonitorState in order to simplify it in the future
• Also fixes bugs in TunnelMonitorState
• Changes the PacketTunnelPathObserver to use NWPathMonitor instead of the deprecated defaultPath from NEProvider
• Changes the path monitored by TunnelManager.swift to prohibit interfaces of type .other
• Adds lots of CustomDebugStringDescription convenience methods for improved debugging

How to test this

• Play with airplane mode, both when the tunnel is up and down
• Try to switch from wifi to cellular connections back and forth
• Try plugging an ethernet adapter on your device ? (I haven't tried that but it would be interesting to know if it breaks things)
• Try this on an unstable connection

---

This change is 

[linear]

IOS-945 Set `includeAllNetworks` to true in `StartTunnelOperation` and rework connectivity monitor

[pinkisemils]

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

---

ios/PacketTunnelCore/TunnelMonitor/TunnelMonitor.swift line 115 at r1 (raw file):

            guard isReachable else { return }

            startMonitoring()

Does this imply that we start sending pings when connectivity is restored? Shouldn't we wait until the tunnel is up again?

[mojganii]

Reviewable status: 0 of 39 files reviewed, 6 unresolved discussions (waiting on @buggmagnet)

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelPathObserver.swift line 29 at r1 (raw file):

    }

    init(packetTunnelProvider: NEPacketTunnelProvider, eventQueue: DispatchQueue) {

We are passing packetTunnelProvider as DI but it never used. Since we are relying on NWPathMonitor instead of tunnelwg

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 70 at r1 (raw file):

        let pinger = TunnelPinger(pingProvider: adapter.icmpPingProvider, replyQueue: internalQueue)

        let tunnelMonitor = TunnelMonitorActor(

do we need to mention the type of object in its name?

---

ios/MullvadVPN/TunnelManager/StartTunnelOperation.swift line 134 at r1 (raw file):

        protocolConfig.excludeLocalNetworks = true
        if #available(iOS 17.4, *) {
            protocolConfig.excludeDeviceCommunication = true

the default value for this property is true, then if we don't want to have that except DEBUG so we need to turn it off in the others.

---

ios/MullvadSettings/TunnelSettingsV7.swift line 12 at r1 (raw file):

import MullvadTypes

public struct TunnelSettingsV7: Codable, Equatable, TunnelSettings, Sendable {

there i the same settings on @SteffenErn's PR. did you agree how to ship the common part between you?

---

ios/PacketTunnelCore/TunnelMonitor/TunnelDeviceInfoProtocol.swift line 20 at r1 (raw file):

    /// Returns tunnel statistics.
//    func getStats() throws -> WgStats

it should be removed since you changed the form to modern one.

[buggmagnet]

Reviewable status: 0 of 39 files reviewed, 6 unresolved discussions (waiting on @mojganii, @pinkisemils, and @SteffenErn)

---

ios/MullvadSettings/TunnelSettingsV7.swift line 12 at r1 (raw file):

Previously, mojganii wrote…

there i the same settings on @SteffenErn's PR. did you agree how to ship the common part between you?

Yes, I plan to rebase on main after merging his PR, so this will go away.

---

ios/MullvadVPN/TunnelManager/StartTunnelOperation.swift line 134 at r1 (raw file):

Previously, mojganii wrote…

the default value for this property is true, then if we don't want to have that except DEBUG so we need to turn it off in the others.

Steffen's PR will change a lot of this, I will probably remove all my changes when rebasing on top of his work.

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelPathObserver.swift line 29 at r1 (raw file):

Previously, mojganii wrote…

We are passing packetTunnelProvider as DI but it never used. Since we are relying on NWPathMonitor instead of tunnelwg

You're right, we can remove this now, nice catch !

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelProvider.swift line 70 at r1 (raw file):

Previously, mojganii wrote…

do we need to mention the type of object in its name?

We do not, I originally had it alongside the TunnelMonitor to avoid namespace clashing, now that it replaced it, we can reuse the original name.

---

ios/PacketTunnelCore/TunnelMonitor/TunnelDeviceInfoProtocol.swift line 20 at r1 (raw file):

Previously, mojganii wrote…

it should be removed since you changed the form to modern one.

Done.

---

ios/PacketTunnelCore/TunnelMonitor/TunnelMonitor.swift line 115 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Does this imply that we start sending pings when connectivity is restored? Shouldn't we wait until the tunnel is up again?

This was discussed offline, the PacketTunnelActor depends on this class to consider itself Connected (otherwise it will stay stuck in the Connecting state.

We discussed that we could improve upon this design later down the line IIRC (please correct me if I'm wrong, it's been a while since we had the discussion and I don't remember anymore what was said).

[mojganii]

Reviewable status: 0 of 39 files reviewed, 7 unresolved discussions (waiting on @pinkisemils and @SteffenErn)

---

ios/PacketTunnelCore/Actor/PacketTunnelActor+ConnectionMonitoring.swift line 30 at r1 (raw file):

     - Important: this method will suspend and must only be invoked as a part of channel consumer to guarantee transactional execution.
     */
    func handleMonitorEvent(_ event: TunnelMonitorEvent) async {

handleMonitorEvent, onEstablishConnection and onHandleConnectionRecovery haven't been used.

[buggmagnet]

Reviewable status: 0 of 39 files reviewed, 7 unresolved discussions (waiting on @acb-mv, @mojganii, @pinkisemils, and @SteffenErn)

---

ios/PacketTunnelCore/Actor/PacketTunnelActor+ConnectionMonitoring.swift line 30 at r1 (raw file):

Previously, mojganii wrote…

handleMonitorEvent, onEstablishConnection and onHandleConnectionRecovery haven't been used.

It looks like this was never removed when we introduced the subreducer work from @acb-mv
Good catch, I will remove that !

[pinkisemils]

Reviewed 4 of 39 files at r1, 1 of 5 files at r2.
Reviewable status: 5 of 39 files reviewed, 8 unresolved discussions (waiting on @acb-mv, @mojganii, and @SteffenErn)

---

ios/PacketTunnel/WireGuardAdapter/WgAdapter.swift line 123 at r4 (raw file):

        /// Whichever finishes first cancels the other one
        let configurationTask = Task {
            let configuration = await getConfiguration()

This should be reworked to not need cancellation. If a tunnel is up, fetching stats oes not involve I/O, so there is no need to use timeouts here.

[pinkisemils]

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

---

ios/PacketTunnelCore/TunnelMonitor/TunnelMonitor.swift line 115 at r1 (raw file):

Previously, buggmagnet wrote…

This was discussed offline, the PacketTunnelActor depends on this class to consider itself Connected (otherwise it will stay stuck in the Connecting state.

We discussed that we could improve upon this design later down the line IIRC (please correct me if I'm wrong, it's been a while since we had the discussion and I don't remember anymore what was said).

Will the packet tunnel actor not receive an update too, to call adapter.start?

---

ios/PacketTunnelCore/TunnelMonitor/TunnelMonitorState.swift line 215 at r4 (raw file):

    }

    mutating func reset(resetRetryAttempts: Bool) {

Instead of having a boolean arrange for different behavior, why not have a func reset() and a func resetRetryAttempts() ?

---

ios/PacketTunnelCore/TunnelMonitor/TunnelDeviceInfoProtocol.swift line 17 at r4 (raw file):

    /// Returns tunnel statistics.
    func getStats() async throws -> WgStats

I am not sure I agree with making getStats asynchronous. There is no I/O Involved in fetching them. Instead, the upstream should be changed.

[buggmagnet]

Reviewable status: 5 of 39 files reviewed, 10 unresolved discussions (waiting on @acb-mv, @mojganii, @pinkisemils, and @SteffenErn)

---

ios/PacketTunnel/WireGuardAdapter/WgAdapter.swift line 123 at r4 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

This should be reworked to not need cancellation. If a tunnel is up, fetching stats oes not involve I/O, so there is no need to use timeouts here.

I agree, but we'd need to modify WireGuardKit for that.
The intent here was to do exactly what the code it replaced did to avoid new behavior introduced in what was largely untested code

[pinkisemils]

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

---

ios/PacketTunnel/WireGuardAdapter/WgAdapter.swift line 123 at r4 (raw file):

Previously, buggmagnet wrote…

I agree, but we'd need to modify WireGuardKit for that.
The intent here was to do exactly what the code it replaced did to avoid new behavior introduced in what was largely untested code

Yes, but you can just block here.

[acb-mv]

Reviewed 28 of 39 files at r1, 3 of 5 files at r2, 1 of 1 files at r3, 1 of 2 files at r4, all commit messages.
Reviewable status: 38 of 39 files reviewed, 11 unresolved discussions (waiting on @buggmagnet, @mojganii, and @SteffenErn)

---

ios/PacketTunnelCore/Pinger/TunnelPinger.swift line 42 at r4 (raw file):

                do {
                    let seq = try pingProvider.receiveICMP()
                    logger.debug("received seq \(seq)")

Did you mean to leave this in?

[buggmagnet]

Reviewable status: 38 of 39 files reviewed, 11 unresolved discussions (waiting on @acb-mv, @mojganii, @pinkisemils, and @SteffenErn)

---

ios/PacketTunnel/WireGuardAdapter/WgAdapter.swift line 123 at r4 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Yes, but you can just block here.

Blocking would violate the swift async runtime that specifically says you should never prevent the runtime from moving forward (i.e. block the current thread).
Instead we should modify WireGuardKit to make this call non asynchronous.

---

ios/PacketTunnelCore/Pinger/TunnelPinger.swift line 42 at r4 (raw file):

Previously, acb-mv wrote…

Did you mean to leave this in?

We don't have to, although it would help us figure out better connectivity when users report problems.
We can lower this to a .trace level to only have it when needed I guess ?

---

ios/PacketTunnelCore/TunnelMonitor/TunnelDeviceInfoProtocol.swift line 17 at r4 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

I am not sure I agree with making getStats asynchronous. There is no I/O Involved in fetching them. Instead, the upstream should be changed.

It's up to us to decide when to do this, but yes, getStats should not be asynchronous indeed.

---

ios/PacketTunnelCore/TunnelMonitor/TunnelMonitor.swift line 115 at r1 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Will the packet tunnel actor not receive an update too, to call adapter.start?

If we lose connectivity, the packet tunnel will enter the blocked state, and try to reconnect periodically
as established in subreducerForTunnelMonitorEvent

---

ios/PacketTunnelCore/TunnelMonitor/TunnelMonitorState.swift line 215 at r4 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Instead of having a boolean arrange for different behavior, why not have a func reset() and a func resetRetryAttempts() ?

Sure we can have that.

[mojganii]

Just the commit history should be updated after @SteffenErn's PR for TuunelSettings

Reviewable status: 36 of 39 files reviewed, 7 unresolved discussions (waiting on @acb-mv, @pinkisemils, and @SteffenErn)

[pinkisemils]

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

---

ios/PacketTunnel/WireGuardAdapter/WgAdapter.swift line 123 at r4 (raw file):

Previously, buggmagnet wrote…

Blocking would violate the swift async runtime that specifically says you should never prevent the runtime from moving forward (i.e. block the current thread).
Instead we should modify WireGuardKit to make this call non asynchronous.

It is perfectly legal to block on a mutex provided that the mutex is never held across suspension points and that the mutex is not held by any other thread doing a blocking call.

Does the tunnel monitor depend on the packet tunnel actor not blocking right now?
What operations does the adapter do that will block?

I do not think it is reasonable to add a timeout to a non-blocking call. Why add all this complexity when it is completely unrelated to the original task at hand?

[rablador]

Reviewed 30 of 39 files at r1, 3 of 5 files at r2, 2 of 2 files at r4, 2 of 2 files at r5, 5 of 5 files at r6, all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on @acb-mv, @mojganii, @pinkisemils, and @SteffenErn)

---

ios/PacketTunnelCore/TunnelMonitor/TunnelMonitor.swift line 37 at r6 (raw file):

        self.tunnelDeviceInfo = tunnelDeviceInfo

        var innerContinuation: AsyncStream<TunnelMonitorEvent>.Continuation?

Why create a local var as intermediare for self.eventHandler?

---

ios/PacketTunnelCore/TunnelMonitor/TunnelMonitor.swift line 156 at r6 (raw file):

            }

            let now = Date()

Nit: var assignment not necessary

---

ios/PacketTunnel/PacketTunnelProvider/PacketTunnelPathObserver.swift line 37 at r6 (raw file):

        guard started == false else { return }
        defer { started = true }
        pathMonitor.pathUpdateHandler = { updatedPath in

Instead of having state in started can't we we check if pathUpdateHandler is nil or not?