Avoid using an unavailable ip version to connect to a relay

[Pururun]

This PR tries to do two things:

1. Throw an error if the user device IP setting does not align with the available ip version on the network
2. Prevent the app from trying to connect using an unavailable ip version.

---

This change is 

[linear]

DES-1139 [Android] App will always try to connect tunnel over IPv4 before trying IPv6

[dlon]

Reviewed 6 of 6 files at r1, 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions

---

talpid-types/src/net/mod.rs line 607 at r1 (raw file):

    /// If IPv4 status is unknown, `true` is returned.
    pub fn has_ipv4(&self) -> bool {
        matches!(self, Connectivity::Status { ipv4: false, .. }).not()

This may be more readable: matches!(self, Connectivity::Status { ipv4: true, .. } | Connectivity::PresumeOnline)

---

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

            .filter(|_query| runtime_params.compatible(&user_query))
            .filter_map(|query| query.clone().intersection(user_query.clone()))
            .map(|query| force_valid_ip_version(&query, runtime_params.clone()))

I suspect that this belongs in detailer, where we currently have resolve_ip_version

[Serock3]

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

---

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

        }
        true
    }

Bikeshedding, but the fn could be simplified to

        match query.wireguard_constraints().ip_version {
            Constraint::Any => self.ipv4 || self.ipv6,
            Constraint::Only(talpid_types::net::IpVersion::V4) => self.ipv4,
            Constraint::Only(talpid_types::net::IpVersion::V6) => self.ipv6,
        }

Code quote:

        if !self.ipv4 {
            let must_use_ipv4 = matches!(
                query.wireguard_constraints().ip_version,
                Constraint::Only(talpid_types::net::IpVersion::V4)
            );
            if must_use_ipv4 {
                log::trace!(
                    "{query:?} is incompatible with {self:?} due to IPv4 not being available"
                );
                return false;
            }
        }
        if !self.ipv6 {
            let must_use_ipv6 = matches!(
                query.wireguard_constraints().ip_version,
                Constraint::Only(talpid_types::net::IpVersion::V6)
            );
            if must_use_ipv6 {
                log::trace!(
                    "{query:?} is incompatible with {self:?} due to IPv6 not being available"
                );
                return false;
            }
        }
        true
    }

[Rawa]

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

---

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

#[derive(Clone, Debug)]
pub struct RuntimeParameters {
    /// Whether IPv6 is available

Typo: Should be IPv4 instead of IPv6

[MarkusPettersson98]

Reviewed all commit messages.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Pururun)

---

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

}

fn force_valid_ip_version(query: &RelayQuery, runtime_params: RuntimeParameters) -> RelayQuery {

⛏️ We don't need to take ownership of runtime_params since we're only ever reading from it. I suggest changing the signature of force_valid_ip_version to instead take a reference to RuntimeParameters.

fn force_valid_ip_version(query: &RelayQuery, runtime_params: &RuntimeParameters) -> RelayQuery

Code quote:

fn force_valid_ip_version(query: &RelayQuery, runtime_params: RuntimeParameters) -> RelayQuery

---

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

        _error => query.clone(),
    }
}

Please, add documentation for this function explaining its intent and why we need it 😊

Code quote:

fn force_valid_ip_version(query: &RelayQuery, runtime_params: RuntimeParameters) -> RelayQuery {
    let mut wireguard_constraints = query.wireguard_constraints().clone();
    if wireguard_constraints.ip_version.is_any() {
        if runtime_params.ipv4 && !runtime_params.ipv6 {
            wireguard_constraints.ip_version = Constraint::Only(talpid_types::net::IpVersion::V4)
        }
        if runtime_params.ipv6 && !runtime_params.ipv4 {
            wireguard_constraints.ip_version = Constraint::Only(talpid_types::net::IpVersion::V6)
        }
    }
    let mut ret = query.clone();
    match ret.set_wireguard_constraints(wireguard_constraints) {
        Ok(()) => ret,
        _error => query.clone(),
    }
}

[Pururun]

Reviewable status: 3 of 8 files reviewed, 4 unresolved discussions (waiting on @dlon, @Rawa, and @Serock3)

---

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

Previously, Rawa (David Göransson) wrote…

Typo: Should be IPv4 instead of IPv6

Done

---

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

Previously, Serock3 (Sebastian Holmin) wrote…

Bikeshedding, but the fn could be simplified to

        match query.wireguard_constraints().ip_version {
            Constraint::Any => self.ipv4 || self.ipv6,
            Constraint::Only(talpid_types::net::IpVersion::V4) => self.ipv4,
            Constraint::Only(talpid_types::net::IpVersion::V6) => self.ipv6,
        }

Done

---

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

Previously, dlon (David Lönnhager) wrote…

I suspect that this belongs in detailer, where we currently have resolve_ip_version

Moved it there, but that required adding runtime parameters as a argument in a lot of places.

---

talpid-types/src/net/mod.rs line 607 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

This may be more readable: matches!(self, Connectivity::Status { ipv4: true, .. } | Connectivity::PresumeOnline)

Done

[dlon]

Reviewed 3 of 5 files at r3, all commit messages.
Reviewable status: 6 of 8 files reviewed, 4 unresolved discussions (waiting on @Pururun, @Rawa, and @Serock3)

---

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

Previously, Pururun (Jonatan Rhodin) wrote…

Moved it there, but that required adding runtime parameters as a argument in a lot of places.

Gah, I might have been wrong in suggesting this. I do think that it's weird for any to default to ipv4 if it's not available