Add ability to override in-IPs to the CLI

[dlon]

This adds a new subcommand mullvad relay override that can be used to override values in the relay list. The fields that can be overridden (as of this PR) are ipv4_addr_in and ipv6_addr_in.

Fix DES-421.

---

This change is 

[linear]

DES-421 Add daemon support for overriding IPs

This will depend a bit on the design, but will probably be something along the lines of the daemon receiving a list of hostname-ip pairs for which it should override the relaylist ip with.

[MarkusPettersson98]

Reviewed 12 of 14 files at r1, 2 of 2 files at r2, 12 of 12 files at r4, all commit messages.
Reviewable status: all files reviewed, 9 unresolved discussions (waiting on @dlon)

---

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

                        &hostname,
                        |relay_override| {
                            let _ = relay_override.ipv4_addr_in.take();

Since we're just discarding the return value of ipv4_addr_in.take(), you might as well just assign aNone directly

Self::update_override(
    &hostname,
    |relay_override| relay_override.ipv4_addr_in = None,
    false,
)
.await?

Code quote:

let _ = relay_override.ipv4_addr_in.take();

---

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

                        &hostname,
                        |relay_override| {
                            let _ = relay_override.ipv6_addr_in.take();

dito

Code quote:

let _ = relay_override.ipv6_addr_in.take();

---

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

}

async fn get_filtered_relays_with_client(

May I suggest adding a small doc-comment explaining get_filtered_relays(_with_client) while you are here poking? 👀

Code quote:

pub async fn get_filtered_relays() -> Result<Vec<RelayListCountry>> {
    let mut rpc = MullvadProxyClient::new().await?;
    get_filtered_relays_with_client(&mut rpc).await
}

async fn get_filtered_relays_with_client(

---

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

pub async fn handle() -> Result<()> {
    if !receive_confirmation("Are you sure you want to disconnect, log out, delete all settings, logs and cache files for the Mullvad VPN system service? [y/N]", false).await {

I suggest moving the [y/N] part of the prompt to receive_confirmation, since the confirmation-format is defined in that function 😊

Code quote:

[y/N]

---

mullvad-relay-selector/src/lib.rs line 120 at r2 (raw file):

                let city_code = city.code.clone();
                let latitude = city.latitude;
                let longitude = city.longitude;

Most of these let-bindings could be inlined without hurting readability

fn relays_with_location(relay_list: &RelayList) -> Vec<Relay> {
    let mut relays = vec![];
    for country in &relay_list.countries {
        for city in &country.cities {
            for relay in &city.relays {
                let mut relay_with_location: Relay = relay.clone();
                relay_with_location.location = Some(Location {
                    country: country.name.clone(),
                    country_code: country.code.clone(),
                    city: city.name.clone(),
                    city_code: city.code.clone(),
                    latitude: city.latitude,
                    longitude: city.longitude,
                });
                relays.push(relay_with_location);
            }
        }
    }
    relays
}

Code quote:

            let country_name = country.name.clone();
            let country_code = country.code.clone();
            for city in &country.cities {
                let city_name = city.name.clone();
                let city_code = city.code.clone();
                let latitude = city.latitude;
                let longitude = city.longitude;

---

mullvad-relay-selector/src/lib.rs line 172 at r2 (raw file):

    fn reset_overrides(&mut self) {
        self.relays = Self::relays_with_location(&self.locations);
    }

You would have to dig into the documentation of RelayList to know that setting self.relays = Self::relays_with_location(&self.locations) will cause self.relays to contain relays as returned by the Mullvad API. I wish that intention was more clear, either here or in relays_with_location 😊

Code quote:

    fn reset_overrides(&mut self) {
        self.relays = Self::relays_with_location(&self.locations);
    }

---

mullvad-relay-selector/src/lib.rs line 175 at r2 (raw file):

    fn append_overrides(&mut self, overrides: Vec<RelayOverride>) {
        self.overrides.clear();

Why is self.overrides cleared here? Wouldn't it be safe to just HashMap::insert the new overrides and apply them over the applicable relays?

Code quote:

self.overrides.clear();

---

mullvad-relay-selector/src/lib.rs line 198 at r2 (raw file):

                    );
                    relay.ipv6_addr_in = Some(ipv6_addr_in.clone());
                }

Would it make sense to implement this override logic on the RelayOverride struct? 😊

impl RelayOverride {
    /// Apply a [`RelayOverride`] to a [`Relay`], replacing
    /// [`Relay::ipv4_addr_in`] and [`Relay::ipv6_addr_in`]
    /// for the matching relay.
    pub fn apply(&self, relay: &mut Relay) {
        ..
    }
}

Code quote:

                if let Some(ipv4_addr_in) = overrides.ipv4_addr_in {
                    log::debug!(
                        "Overriding ipv4_addr_in for {}: {ipv4_addr_in}",
                        relay.hostname
                    );
                    relay.ipv4_addr_in = ipv4_addr_in.clone();
                }
                if let Some(ipv6_addr_in) = overrides.ipv6_addr_in {
                    log::debug!(
                        "Overriding ipv6_addr_in for {}: {ipv6_addr_in}",
                        relay.hostname
                    );
                    relay.ipv6_addr_in = Some(ipv6_addr_in.clone());
                }

---

mullvad-types/src/settings/mod.rs line 200 at r2 (raw file):

            self.relay_overrides.push(relay_override);
        }
    }

This logic could probably be simplified using enumerate

pub fn set_relay_override(&mut self, relay_override: RelayOverride) {
    let existing_override = self
        .relay_overrides
        .iter_mut()
        .enumerate()
        .find(|(_, elem)| elem.hostname == relay_override.hostname);

    match existing_override {
        None => self.relay_overrides.push(relay_override),
        Some((index, elem)) => {
            if relay_override.is_empty() {
                self.relay_overrides.swap_remove(index);
            } else {
                // Update existing options
                *elem = relay_override;
            }
        }
    }
}

Code quote:

    pub fn set_relay_override(&mut self, relay_override: RelayOverride) {
        if relay_override.is_empty() {
            if let Some(index) = self
                .relay_overrides
                .iter()
                .position(|elem| elem.hostname == relay_override.hostname)
            {
                self.relay_overrides.swap_remove(index);
            }
            return;
        }
        if let Some(elem) = self
            .relay_overrides
            .iter_mut()
            .find(|elem| elem.hostname == relay_override.hostname)
        {
            // Update existing options
            *elem = relay_override;
        } else {
            self.relay_overrides.push(relay_override);
        }
    }

[dlon]

Reviewable status: 8 of 15 files reviewed, 9 unresolved discussions (waiting on @MarkusPettersson98)

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Since we're just discarding the return value of ipv4_addr_in.take(), you might as well just assign aNone directly

Self::update_override(
    &hostname,
    |relay_override| relay_override.ipv4_addr_in = None,
    false,
)
.await?

Done.

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

dito

Done.

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

May I suggest adding a small doc-comment explaining get_filtered_relays(_with_client) while you are here poking? 👀

Done.

---

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

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

I suggest moving the [y/N] part of the prompt to receive_confirmation, since the confirmation-format is defined in that function 😊

Done.

---

mullvad-relay-selector/src/lib.rs line 120 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Most of these let-bindings could be inlined without hurting readability

fn relays_with_location(relay_list: &RelayList) -> Vec<Relay> {
    let mut relays = vec![];
    for country in &relay_list.countries {
        for city in &country.cities {
            for relay in &city.relays {
                let mut relay_with_location: Relay = relay.clone();
                relay_with_location.location = Some(Location {
                    country: country.name.clone(),
                    country_code: country.code.clone(),
                    city: city.name.clone(),
                    city_code: city.code.clone(),
                    latitude: city.latitude,
                    longitude: city.longitude,
                });
                relays.push(relay_with_location);
            }
        }
    }
    relays
}

Done.

---

mullvad-types/src/settings/mod.rs line 200 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

This logic could probably be simplified using enumerate

pub fn set_relay_override(&mut self, relay_override: RelayOverride) {
    let existing_override = self
        .relay_overrides
        .iter_mut()
        .enumerate()
        .find(|(_, elem)| elem.hostname == relay_override.hostname);

    match existing_override {
        None => self.relay_overrides.push(relay_override),
        Some((index, elem)) => {
            if relay_override.is_empty() {
                self.relay_overrides.swap_remove(index);
            } else {
                // Update existing options
                *elem = relay_override;
            }
        }
    }
}

Great idea. Done.

[dlon]

Reviewable status: 8 of 15 files reviewed, 9 unresolved discussions (waiting on @MarkusPettersson98)

---

mullvad-relay-selector/src/lib.rs line 172 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

You would have to dig into the documentation of RelayList to know that setting self.relays = Self::relays_with_location(&self.locations) will cause self.relays to contain relays as returned by the Mullvad API. I wish that intention was more clear, either here or in relays_with_location 😊

This should be more obvious now.

---

mullvad-relay-selector/src/lib.rs line 175 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Why is self.overrides cleared here? Wouldn't it be safe to just HashMap::insert the new overrides and apply them over the applicable relays?

No longer relevant.

[dlon]

Reviewable status: 7 of 16 files reviewed, 9 unresolved discussions (waiting on @MarkusPettersson98)

---

mullvad-relay-selector/src/lib.rs line 198 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Would it make sense to implement this override logic on the RelayOverride struct? 😊

impl RelayOverride {
    /// Apply a [`RelayOverride`] to a [`Relay`], replacing
    /// [`Relay::ipv4_addr_in`] and [`Relay::ipv6_addr_in`]
    /// for the matching relay.
    pub fn apply(&self, relay: &mut Relay) {
        ..
    }
}

Done.

[MarkusPettersson98]

Reviewed 6 of 7 files at r5, 1 of 1 files at r6, 3 of 3 files at r7, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved