Implement "smart routing" daita setting

[hulthe]

---

This change is 

[linear]

DES-1068 Implement "Use anywhere" DAITA setting.

[hulthe]

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

---

mullvad-cli/src/cmds/tunnel.rs line 201 at r2 (raw file):

            rpc.set_daita_settings(DaitaSettings {
                enabled: *daita,
                use_anywhere: true, /* TODO */

What do we think is the best way of fixing this?

Maybe adding a use-anywhere mode of some kind to mullvad tunnel set wireguard --daita?
Or adding RPC calls that specifically set daita.enabled and daita.use_anywhere?
Or something else?

[MarkusPettersson98]

Reviewed 8 of 11 files at r1, 17 of 17 files at r2.
Reviewable status: 24 of 26 files reviewed, 4 unresolved discussions (waiting on @hulthe)

---

gui/src/renderer/components/WireguardSettings.tsx line 616 at r2 (raw file):

        <Cell.Container>
          <AriaLabel>
            <Cell.InputLabel>The "Just make it work" Button</Cell.InputLabel>

👌

Code quote:

 <Cell.InputLabel>The "Just make it work" Button</Cell.InputLabel>

---

mullvad-relay-selector/src/relay_selector/mod.rs line 126 at r2 (raw file):

    pub daita: bool,

    pub daita_use_anywhere: bool,

Nit Please add a small comment similar to the one for the daita property 😊

Code quote:

    pub daita_use_anywhere: bool,

---

mullvad-relay-selector/src/relay_selector/mod.rs line 753 at r2 (raw file):

    /// * `Ok(WireguardInner::Multihop)` otherwise
    fn get_wireguard_auto_multihop_config(
        // TODO: document or change assumption that `query` is constrained to daita only

Do we even need to make this assumption in this function? It seems general enough that you can simply remove this TODO note IMHO. Maybe we should abstract this function over the entry relay predicate (i.e. the closure passed to take_while below), but even that seems overkill for now.

Code quote:

       // TODO: document or change assumption that `query` is constrained to daita only

---

mullvad-relay-selector/src/relay_selector/query.rs line 244 at r2 (raw file):

//        }
//    }
//}

Please remove 😄

Code quote:

//impl From<WireguardRelayQuery> for AdditionalWireguardConstraints {
//    /// The mapping from [`WireguardRelayQuery`] to [`AdditionalWireguardConstraints`].
//    fn from(value: WireguardRelayQuery) -> Self {
//        AdditionalWireguardConstraints {
//            daita: value
//                .daita
//                .unwrap_or(AdditionalWireguardConstraints::default().daita),
//        }
//    }
//}

[MarkusPettersson98]

Reviewable status: 24 of 26 files reviewed, 5 unresolved discussions (waiting on @hulthe)

---

mullvad-cli/src/cmds/tunnel.rs line 201 at r2 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

What do we think is the best way of fixing this?

Maybe adding a use-anywhere mode of some kind to mullvad tunnel set wireguard --daita?
Or adding RPC calls that specifically set daita.enabled and daita.use_anywhere?
Or something else?

Either add RPC calls for setting daita.enabled and daita.use_anywhere separately, or have the existing set_daita_settings RPC accept a type which can represent updating these values together or separately. Something like

enum DaitaSettingsUpdate {
    SetEnabled(bool),
    SetUseAnywhere(bool),
    SetSettings(DaitaSettings),
}

The latter seems more sound to me, but it is a bit more noisy. Also, it breaks the current protobuf interface, forcing all clients to be updated. But maybe that's fine 🤷

---

mullvad-cli/src/cmds/tunnel.rs line 198 at r3 (raw file):

        #[cfg(daita)]
        if let Some(daita) = daita {

Nit: Since all desktop platforms ship DAITA, we should be able to remove the #[cfg(daita)] gates 🎉

Code quote:

        #[cfg(daita)]
        if let Some(daita) = daita {

[hulthe]

Reviewable status: 21 of 26 files reviewed, 4 unresolved discussions (waiting on @MarkusPettersson98)

---

mullvad-relay-selector/src/relay_selector/mod.rs line 126 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Nit Please add a small comment similar to the one for the daita property 😊

Done

---

mullvad-relay-selector/src/relay_selector/mod.rs line 753 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Do we even need to make this assumption in this function? It seems general enough that you can simply remove this TODO note IMHO. Maybe we should abstract this function over the entry relay predicate (i.e. the closure passed to take_while below), but even that seems overkill for now.

I had another look, and I agree. I've removed the comment.

---

mullvad-relay-selector/src/relay_selector/query.rs line 244 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Please remove 😄

Done.

[MarkusPettersson98]

Reviewed 1 of 1 files at r3, 3 of 3 files at r4, all commit messages.
Reviewable status: 25 of 26 files reviewed, 2 unresolved discussions (waiting on @hulthe)

[MarkusPettersson98]

Reviewed 1 of 11 files at r1, 15 of 15 files at r5, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @hulthe)

---

mullvad-api/src/relay_list.rs line 277 at r5 (raw file):

fn inclusive_range_from_pair<T>(pair: (T, T)) -> RangeInclusive<T> {
    RangeInclusive::new(pair.0, pair.1)
}

Nit: Could be make const 😊

Code quote:

fn inclusive_range_from_pair<T>(pair: (T, T)) -> RangeInclusive<T> {
    RangeInclusive::new(pair.0, pair.1)
}

[MarkusPettersson98]

Reviewed 11 of 11 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @hulthe)

---

mullvad-cli/src/cmds/tunnel.rs line 48 at r6 (raw file):

        #[cfg(daita)]
        #[arg(long)]
        daita_use_anywhere: Option<BooleanOption>,

We can safely ditch the #[cfg(daita)] gates, right? Since we ship DAITA on all platforms with the CLI now 😊

Code quote:

        /// Configure whether to enable DAITA
        #[cfg(daita)]
        #[arg(long)]
        daita: Option<BooleanOption>,
        /// Configure whether to enable DAITA "use anywhere"
        #[cfg(daita)]
        #[arg(long)]
        daita_use_anywhere: Option<BooleanOption>,

[hulthe]

Reviewable status: 28 of 32 files reviewed, 3 unresolved discussions (waiting on @MarkusPettersson98)

---

mullvad-api/src/relay_list.rs line 277 at r5 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Nit: Could be make const 😊

how did you even find this when reviewing haha

---

mullvad-cli/src/cmds/tunnel.rs line 201 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Either add RPC calls for setting daita.enabled and daita.use_anywhere separately, or have the existing set_daita_settings RPC accept a type which can represent updating these values together or separately. Something like

enum DaitaSettingsUpdate {
    SetEnabled(bool),
    SetUseAnywhere(bool),
    SetSettings(DaitaSettings),
}

The latter seems more sound to me, but it is a bit more noisy. Also, it breaks the current protobuf interface, forcing all clients to be updated. But maybe that's fine 🤷

I opted for multiple RPC-calls instead. Didn't want to risk over-engineering anything (and also, adding new proto types is annoying heh). lmk if you disagree with this

---

mullvad-cli/src/cmds/tunnel.rs line 198 at r3 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Nit: Since all desktop platforms ship DAITA, we should be able to remove the #[cfg(daita)] gates 🎉

👍 👍

---

mullvad-cli/src/cmds/tunnel.rs line 48 at r6 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

We can safely ditch the #[cfg(daita)] gates, right? Since we ship DAITA on all platforms with the CLI now 😊

👍

[MarkusPettersson98]

Reviewed 4 of 5 files at r7, all commit messages.
Reviewable status: 31 of 32 files reviewed, 3 unresolved discussions (waiting on @hulthe)

---

mullvad-api/src/relay_list.rs line 277 at r5 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

how did you even find this when reviewing haha

🫣

---

mullvad-cli/src/cmds/tunnel.rs line 201 at r2 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

I opted for multiple RPC-calls instead. Didn't want to risk over-engineering anything (and also, adding new proto types is annoying heh). lmk if you disagree with this

I think that's absolutely fine!

---

mullvad-cli/src/cmds/tunnel.rs line 198 at r3 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

👍 👍

🎉

---

mullvad-cli/src/cmds/tunnel.rs line 48 at r6 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

👍

🎉

---

mullvad-daemon/src/lib.rs line 3011 at r7 (raw file):

                    } else {
                        false
                    };

This feels so suboptimal 😂 I get that there is nothing to do about it without a major refactoring,

Code quote:

                #[cfg(daita)]
                let daita_use_anywhere =
                    if let RelaySettings::Normal(constraints) = settings.relay_settings {
                        // Detect whether we're using "use_anywhere" by checking if multihop is
                        // in use but not explicitly enabled.
                        daita && multihop && !constraints.wireguard_constraints.use_multihop
                    } else {
                        false
                    };

[raksooo]

Reviewed 1 of 11 files at r6, 1 of 7 files at r10, 1 of 13 files at r14, 1 of 1 files at r15, 11 of 29 files at r18, 2 of 7 files at r19.
Reviewable status: 36 of 41 files reviewed, 4 unresolved discussions (waiting on @hulthe and @MarkusPettersson98)

[hulthe]

Reviewable status: 36 of 41 files reviewed, 3 unresolved discussions (waiting on @MarkusPettersson98)

---

test/test-manager/src/tests/daita.rs line 33 at r18 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Nit You could simplify this a bit by using the relays method on RelayList.

-        .countries
-        .iter()
-        .flat_map(|countries| &countries.cities)
-        .flat_map(|cities| &cities.relays)
+        .relays()

Done

---

test/test-manager/src/tests/daita.rs line 87 at r18 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Should we also assert that we run into the expected error cause?

ensure!(matches!(state.cause(), ErrorStateCause::StartTunnelError));

what do you mean? TunnelParameterError is the expected state.cause

[raksooo]

Reviewed 1 of 7 files at r19, 1 of 7 files at r20.
Reviewable status: 33 of 42 files reviewed, 3 unresolved discussions (waiting on @hulthe and @MarkusPettersson98)

[MarkusPettersson98]

Reviewed 3 of 7 files at r19, 5 of 7 files at r20, all commit messages.
Reviewable status: 41 of 42 files reviewed, 1 unresolved discussion (waiting on @hulthe)

---

test/test-manager/src/tests/daita.rs line 87 at r18 (raw file):

Previously, hulthe (Joakim Hulthe) wrote…

what do you mean? TunnelParameterError is the expected state.cause

Nvm, I was stupid

[MarkusPettersson98]

Reviewable status: 41 of 42 files reviewed, all discussions resolved