Put app in blocked state on tunnel start if relay constraints cannot be satisfied

[rablador]

On tunnel start, if no valid relay can be selected app-side we let the packet tunnel take care of it instead. This will put the app in blocked state.

This PR takes care of issue the following issue as well:
https://linear.app/mullvad/issue/IOS-351/removing-relay-filters-does-not-exit-blocked-state

---

This change is 

[linear]

IOS-351 Removing Relay filters does not exit blocked state

See the attached slack discussion for the repro steps

[pronebird]

Reviewed 1 of 3 files at r1.
Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @rablador)

---

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

            self.dispatchQueue.async {
                do {
                    try self.startTunnel(tunnel: try result.get())

Nit: AFAIK one try in front is enough as it implies try result.get() too.

---

ios/PacketTunnelCore/IPC/PacketTunnelOptions.swift line 43 at r1 (raw file):

    }

    public mutating func setSelectedRelay(_ value: SelectedRelay?) throws {

getSelectedRelay() will probably break because it decodes SelectedRelay.self. Instead you could omit encoding nil values altogether. Do not pass nil to indicate absence of value, instead avoid passing nil altogether.

[pronebird]

Reviewed 1 of 3 files at r1.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @rablador)

---

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

            dispatchQueue: internalQueue,
            interactor: TunnelInteractorProxy(self),
            selectedRelay: try? selectRelay(),

Prefer to keep the actual work inside of operation body (main) and not outside to avoid working with stale data. For example relay constraints could be changed before StartTunnelOperation begins execution.

[pronebird]

Reviewed 1 of 3 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @rablador)

[rablador]

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pronebird)

---

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

Previously, pronebird (Andrej Mihajlov) wrote…

Prefer to keep the actual work inside of operation body (main) and not outside to avoid working with stale data. For example relay constraints could be changed before StartTunnelOperation begins execution.

Sounds right, on it.

---

ios/PacketTunnelCore/IPC/PacketTunnelOptions.swift line 43 at r1 (raw file):

Previously, pronebird (Andrej Mihajlov) wrote…

getSelectedRelay() will probably break because it decodes SelectedRelay.self. Instead you could omit encoding nil values altogether. Do not pass nil to indicate absence of value, instead avoid passing nil altogether.

Ah, yes, my bad. However, we do want to remove the value if there's no selected relay, right?

[rablador]

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @pronebird)

---

ios/PacketTunnelCore/IPC/PacketTunnelOptions.swift line 43 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

Ah, yes, my bad. However, we do want to remove the value if there's no selected relay, right?

My bad again. I just realised we creatre new PacketTunnelOptions before this call, so simply setting nothing is the way to go.

[buggmagnet]

Reviewed all commit messages.
Reviewable status: 0 of 3 files reviewed, 3 unresolved discussions (waiting on @pronebird and @rablador)

---

ios/MullvadVPN/TunnelManager/StartTunnelOperation.swift line 76 at r2 (raw file):

    private func startTunnel(tunnel: Tunnel) throws {
        let selectedRelay = try? interactor.selectRelay()

Was there any reason to not use a more straightforward approach ?

    private func startTunnel(tunnel: Tunnel) throws {
        let selectedRelay = try interactor.selectRelay()
        var tunnelOptions = PacketTunnelOptions()

        try tunnelOptions.setSelectedRelay(selectedRelay)
        interactor.setTunnel(tunnel, shouldRefreshTunnelState: false)

        interactor.updateTunnelStatus { tunnelStatus in
            tunnelStatus = TunnelStatus()
            tunnelStatus.packetTunnelStatus.tunnelRelay = selectedRelay.packetTunnelRelay
            tunnelStatus.state = .connecting(selectedRelay.packetTunnelRelay)
        }

        try tunnel.start(options: tunnelOptions.rawOptions())
    }

[rablador]

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @pronebird)

---

ios/MullvadVPN/TunnelManager/StartTunnelOperation.swift line 76 at r2 (raw file):

Previously, buggmagnet wrote…

Was there any reason to not use a more straightforward approach ?

    private func startTunnel(tunnel: Tunnel) throws {
        let selectedRelay = try interactor.selectRelay()
        var tunnelOptions = PacketTunnelOptions()

        try tunnelOptions.setSelectedRelay(selectedRelay)
        interactor.setTunnel(tunnel, shouldRefreshTunnelState: false)

        interactor.updateTunnelStatus { tunnelStatus in
            tunnelStatus = TunnelStatus()
            tunnelStatus.packetTunnelStatus.tunnelRelay = selectedRelay.packetTunnelRelay
            tunnelStatus.state = .connecting(selectedRelay.packetTunnelRelay)
        }

        try tunnel.start(options: tunnelOptions.rawOptions())
    }

As far as I understand, if interactor.selectRelay() throws we won't set up a tunnel and start it, which means we cannot enter a blocked state.

[buggmagnet]

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @pronebird)

---

ios/MullvadVPN/TunnelManager/StartTunnelOperation.swift line 76 at r2 (raw file):

Previously, rablador (Jon Petersson) wrote…

As far as I understand, if interactor.selectRelay() throws we won't set up a tunnel and start it, which means we cannot enter a blocked state.

Yeah I quickly realized what I wrote wouldn't work, so I deleted the comment... Too late !

[rablador]

Reviewable status: 0 of 3 files reviewed, 2 unresolved discussions (waiting on @pronebird)

---

ios/MullvadVPN/TunnelManager/StartTunnelOperation.swift line 76 at r2 (raw file):

Previously, buggmagnet wrote…

Yeah I quickly realized what I wrote wouldn't work, so I deleted the comment... Too late !

It can never be unseen!

[buggmagnet]

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pronebird)

[pronebird]

Reviewed 1 of 3 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved

---

ios/PacketTunnelCore/IPC/PacketTunnelOptions.swift line 43 at r1 (raw file):

Previously, rablador (Jon Petersson) wrote…

My bad again. I just realised we creatre new PacketTunnelOptions before this call, so simply setting nothing is the way to go.

Correct.

[pronebird]

Reviewed 2 of 3 files at r2.
Reviewable status: complete! all files reviewed, all discussions resolved