Allow relay selector to filter daita enabled relays

[rablador]

When DAITA is enabled, the relay selector should only match DAITA enabled relays for the entry location when using multihop. If no DAITA relays can be matched, the packet tunnel should enter the blocked state - ideally with an error message displayed to the user that makes it obvious that no relays were matched due to none of the eligible ones allowing DAITA.

When using single-hop and DAITA is enabled only DAITA enabled relays should be considered first - the best suitable DAITA relay should be picked if at all possible. Otherwise, if the exit constraints do not match any DAITA enabled relay, the relay selector should use multihop to select a valid DAITA relay for the entry.

---

This change is 

[linear]

IOS-772 Allow relay selector to filter DAITA enabled relays

[acb-mv]

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

---

ios/MullvadREST/ApiHandlers/ServerRelaysResponse.swift line 37 at r1 (raw file):

        public let weight: UInt64
        public let includeInCountry: Bool
        public var daita: Bool?

Wouldn't a statement such as hasDAITA be a clearer name?

---

ios/MullvadREST/Relay/AnyRelay.swift line 20 at r1 (raw file):

    var active: Bool { get }
    var includeInCountry: Bool { get }
    var daita: Bool? { get }

Wouldn't a statement such as hasDAITA be a clearer name?

[rablador]

Reviewable status: 1 of 26 files reviewed, 2 unresolved discussions (waiting on @acb-mv)

---

ios/MullvadREST/ApiHandlers/ServerRelaysResponse.swift line 37 at r1 (raw file):

Previously, acb-mv wrote…

Wouldn't a statement such as hasDAITA be a clearer name?

I agree, but that's the name we get from the servers. We could have custom coding keys, but it feels like more work than it's worth.

[acb-mv]

Reviewable status: 1 of 26 files reviewed, all discussions resolved

[buggmagnet]

Reviewed 26 of 26 files at r1, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @rablador)

---

ios/MullvadREST/Relay/NoRelaysSatisfyingConstraintsError.swift line 15 at r1 (raw file):

    case invalidPort
    case multihopEntryEqualsExit
    case multihopOther

Can we find a better name for this ? Maybe multihopInvalidFlow ?

---

ios/MullvadREST/Relay/NoRelaysSatisfyingConstraintsError.swift line 25 at r1 (raw file):

    public var errorDescription: String? {
        "No relays satisfying constraints."

Since we log this, it would be interesting to improve this error message and see why we threw the error.

    public var errorDescription: String? {
        switch reason {
        case .filterConstraintNotMatching:
            "No relays satisfying constraints."
        case .invalidPort:
            "Invalid port selected by RelaySelector"
        case .multihopEntryEqualsExit:
            "Entry hop and Exit hop are the same"
        case .multihopOther:
            "Invalid multihop decision flow"
        case .noActiveRelaysFound:
            "No active relays found"
        case .noDaitaRelaysFound:
            "No Daita relays found"
        case .relayConstraintNotMatching:
            "Invalid constraint created to pick a relay"
        }
    }

---

ios/MullvadVPN/Notifications/Notification Providers/TunnelStatusNotificationProvider.swift line 242 at r1 (raw file):

            errorString = "No servers match your location filter. Try changing filter settings."
        case .multihopEntryEqualsExit:
            errorString = "The entry and exit server cannot be the same. Try changing one to a new server or location."

The entry and exit servers cannot be the same.

---

ios/MullvadVPN/SimulatorTunnelProvider/SimulatorTunnelProviderManager.swift line 175 at r1 (raw file):

    override func isEqual(_ object: Any?) -> Bool {
        guard let multihopOther = object as? Self else { return false }
        return self.identifier == multihopOther.identifier

Did you accidentally rename this ?

---

ios/MullvadVPNTests/MullvadREST/Relay/RelayPickingTests.swift line 76 at r1 (raw file):

            XCTAssertEqual(error?.reason, .multihopEntryEqualsExit)
        }
    }

Shouldn't we have a test for the noDaitaRelaysFound single picker case ?

[mojganii]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @buggmagnet and @rablador)

---

ios/MullvadREST/Relay/NoRelaysSatisfyingConstraintsError.swift line 14 at r1 (raw file):

    case filterConstraintNotMatching
    case invalidPort
    case multihopEntryEqualsExit

I suggest to eliminate multihop keyword, what do you think about relaysAreEqual.

---

ios/MullvadVPNTests/MullvadREST/Relay/RelayPickingTests.swift line 27 at r1 (raw file):

        let picker = SinglehopPicker(
            constraints: constraints,
            daitaSettings: DAITASettings(state: .off),

let's call them with the default value when the test isn't around DaitaSettings. if we set default value to on in the future, we will need to update these tests either.

Code snippet:

 daitaSettings: DAITASettings()

---

ios/MullvadVPNTests/MullvadREST/Relay/RelayPickingTests.swift line 76 at r1 (raw file):

Previously, buggmagnet wrote…

Shouldn't we have a test for the noDaitaRelaysFound single picker case ?

+1

[rablador]

Reviewable status: 20 of 26 files reviewed, 7 unresolved discussions (waiting on @buggmagnet and @mojganii)

---

ios/MullvadREST/Relay/NoRelaysSatisfyingConstraintsError.swift line 14 at r1 (raw file):

Previously, mojganii wrote…

I suggest to eliminate multihop keyword, what do you think about relaysAreEqual.

Opting to keep entry and exit in the wording to more clearly convey what's going on.

---

ios/MullvadREST/Relay/NoRelaysSatisfyingConstraintsError.swift line 15 at r1 (raw file):

Previously, buggmagnet wrote…

Can we find a better name for this ? Maybe multihopInvalidFlow ?

Done.

---

ios/MullvadREST/Relay/NoRelaysSatisfyingConstraintsError.swift line 25 at r1 (raw file):

Previously, buggmagnet wrote…

Since we log this, it would be interesting to improve this error message and see why we threw the error.

    public var errorDescription: String? {
        switch reason {
        case .filterConstraintNotMatching:
            "No relays satisfying constraints."
        case .invalidPort:
            "Invalid port selected by RelaySelector"
        case .multihopEntryEqualsExit:
            "Entry hop and Exit hop are the same"
        case .multihopOther:
            "Invalid multihop decision flow"
        case .noActiveRelaysFound:
            "No active relays found"
        case .noDaitaRelaysFound:
            "No Daita relays found"
        case .relayConstraintNotMatching:
            "Invalid constraint created to pick a relay"
        }
    }

Done.

---

ios/MullvadVPN/Notifications/Notification Providers/TunnelStatusNotificationProvider.swift line 242 at r1 (raw file):

Previously, buggmagnet wrote…

The entry and exit servers cannot be the same.

Done.

---

ios/MullvadVPNTests/MullvadREST/Relay/RelayPickingTests.swift line 27 at r1 (raw file):

Previously, mojganii wrote…

let's call them with the default value when the test isn't around DaitaSettings. if we set default value to on in the future, we will need to update these tests either.

With the current test setup (and sample relays) we will get a .noDaitaRelaysFound error instead. I specifically want DAITA to be off here so that we test only entry and exit error and don't involve DAITA at all, even if it's default in the app or not.

---

ios/MullvadVPNTests/MullvadREST/Relay/RelayPickingTests.swift line 76 at r1 (raw file):

Previously, mojganii wrote…

+1

RelayPicking protocol is concerned with multihop and makes sure the picking of one-one, one-many, many-many relationsships work correctly. A DAITA specific test already exists in RelaySelectorTests.testRelayWithDaita.

---

ios/MullvadVPN/SimulatorTunnelProvider/SimulatorTunnelProviderManager.swift line 175 at r1 (raw file):

Previously, buggmagnet wrote…

Did you accidentally rename this ?

Yes, fixed.

[buggmagnet]

Reviewed 6 of 6 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @mojganii)

[mojganii]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @buggmagnet)

[github-actions]

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