Skip to content

Commit

Permalink
Refactor mullvad-relay-selector
Browse files Browse the repository at this point in the history
Implement a system for choosing appropriate relay(s) built on 'queries'.
A query is a set of constraints which dictates which relay(s) that *can*
be chosen by the relay selector.

The user's settings can naturally be expressed as a query. The semantics
of merging two queries in a way that always prefer user settings is
defined by the new `Intersection` trait.

Decrust `mullvad-relay-selector` by splitting it up into several modules

- `query.rs`: Definition of a query on different types of relays. This
module is integral to the new API of `mullvad-relay-selector`
- `matcher.rs`: Logic for filtering out candidate relays based on a query.
- `detailer.rs`: Logic for deriving connection details for the selected relay.
- `tests/`: Integration tests for the new relay selector. These tests only use
the public APIs of `RelaySelector` and make sure that the output matches
the expected output in different scenarios.
  • Loading branch information
MarkusPettersson98 committed Mar 11, 2024
1 parent 9c035ee commit b02fb81
Show file tree
Hide file tree
Showing 46 changed files with 3,847 additions and 3,128 deletions.
18 changes: 15 additions & 3 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 2 additions & 0 deletions Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ chrono = { version = "0.4.26", default-features = false}
clap = { version = "4.4.18", features = ["cargo", "derive"] }
once_cell = "1.13"

# Test dependencies
proptest = "1.4"

[profile.release]
opt-level = 3
Expand Down
5 changes: 3 additions & 2 deletions mullvad-cli/src/cmds/bridge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,10 @@ use anyhow::{bail, Result};
use clap::Subcommand;
use mullvad_management_interface::MullvadProxyClient;
use mullvad_types::{
constraints::Constraint,
relay_constraints::{
BridgeConstraintsFormatter, BridgeState, BridgeType, Constraint, LocationConstraint,
Ownership, Provider, Providers,
BridgeConstraintsFormatter, BridgeState, BridgeType, LocationConstraint, Ownership,
Provider, Providers,
},
relay_list::RelayEndpointData,
};
Expand Down
3 changes: 1 addition & 2 deletions mullvad-cli/src/cmds/custom_list.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,7 @@ use anyhow::{anyhow, bail, Result};
use clap::Subcommand;
use mullvad_management_interface::MullvadProxyClient;
use mullvad_types::{
relay_constraints::{Constraint, GeographicLocationConstraint},
relay_list::RelayList,
constraints::Constraint, relay_constraints::GeographicLocationConstraint, relay_list::RelayList,
};

#[derive(Subcommand, Debug)]
Expand Down
5 changes: 4 additions & 1 deletion mullvad-cli/src/cmds/debug.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
use anyhow::Result;
use mullvad_management_interface::MullvadProxyClient;
use mullvad_types::relay_constraints::{Constraint, RelayConstraints, RelaySettings};
use mullvad_types::{
constraints::Constraint,
relay_constraints::{RelayConstraints, RelaySettings},
};

#[derive(clap::Subcommand, Debug)]
pub enum DebugCommands {
Expand Down
5 changes: 3 additions & 2 deletions mullvad-cli/src/cmds/obfuscation.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
use anyhow::Result;
use clap::Subcommand;
use mullvad_management_interface::MullvadProxyClient;
use mullvad_types::relay_constraints::{
Constraint, ObfuscationSettings, SelectedObfuscation, Udp2TcpObfuscationSettings,
use mullvad_types::{
constraints::Constraint,
relay_constraints::{ObfuscationSettings, SelectedObfuscation, Udp2TcpObfuscationSettings},
};

#[derive(Subcommand, Debug)]
Expand Down
9 changes: 5 additions & 4 deletions mullvad-cli/src/cmds/relay.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@ use clap::Subcommand;
use itertools::Itertools;
use mullvad_management_interface::MullvadProxyClient;
use mullvad_types::{
constraints::{Constraint, Match},
location::{CountryCode, Location},
relay_constraints::{
Constraint, GeographicLocationConstraint, LocationConstraint, LocationConstraintFormatter,
Match, OpenVpnConstraints, Ownership, Provider, Providers, RelayConstraints, RelayOverride,
GeographicLocationConstraint, LocationConstraint, LocationConstraintFormatter,
OpenVpnConstraints, Ownership, Provider, Providers, RelayConstraints, RelayOverride,
RelaySettings, TransportPort, WireguardConstraints,
},
relay_list::{RelayEndpointData, RelayListCountry},
Expand Down Expand Up @@ -318,7 +319,7 @@ impl Relay {

print_option!(
"Multihop state",
if constraints.wireguard_constraints.use_multihop {
if constraints.wireguard_constraints.multihop() {
"enabled"
} else {
"disabled"
Expand Down Expand Up @@ -679,7 +680,7 @@ impl Relay {
wireguard_constraints.ip_version = ipv;
}
if let Some(use_multihop) = use_multihop {
wireguard_constraints.use_multihop = *use_multihop;
wireguard_constraints.use_multihop(*use_multihop);
}
match entry_location {
Some(EntryArgs::Location(location_args)) => {
Expand Down
3 changes: 2 additions & 1 deletion mullvad-cli/src/cmds/relay_constraints.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
use clap::Args;
use mullvad_types::{
constraints::Constraint,
location::{CityCode, CountryCode, Hostname},
relay_constraints::{Constraint, GeographicLocationConstraint, LocationConstraint},
relay_constraints::{GeographicLocationConstraint, LocationConstraint},
};

#[derive(Args, Debug, Clone)]
Expand Down
2 changes: 1 addition & 1 deletion mullvad-cli/src/cmds/tunnel.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use anyhow::Result;
use clap::Subcommand;
use mullvad_management_interface::MullvadProxyClient;
use mullvad_types::{
relay_constraints::Constraint,
constraints::Constraint,
wireguard::{QuantumResistantState, RotationInterval, DEFAULT_ROTATION_INTERVAL},
};

Expand Down
7 changes: 3 additions & 4 deletions mullvad-daemon/src/custom_list.rs
Original file line number Diff line number Diff line change
@@ -1,9 +1,8 @@
use crate::{new_selector_config, Daemon, Error, EventListener};
use mullvad_types::{
constraints::Constraint,
custom_list::{CustomList, Id},
relay_constraints::{
BridgeState, Constraint, LocationConstraint, RelaySettings, ResolvedBridgeSettings,
},
relay_constraints::{BridgeState, LocationConstraint, RelaySettings, ResolvedBridgeSettings},
};
use talpid_types::net::TunnelType;

Expand Down Expand Up @@ -133,7 +132,7 @@ where
{
match endpoint.tunnel_type {
TunnelType::Wireguard => {
if relay_settings.wireguard_constraints.use_multihop {
if relay_settings.wireguard_constraints.multihop() {
if let Constraint::Only(LocationConstraint::CustomList { list_id }) =
&relay_settings.wireguard_constraints.entry_location
{
Expand Down
44 changes: 22 additions & 22 deletions mullvad-daemon/src/device/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1348,12 +1348,10 @@ impl TunnelStateChangeHandler {
#[cfg(test)]
mod test {
use super::{Error, TunnelStateChangeHandler, WG_DEVICE_CHECK_THRESHOLD};
use mullvad_relay_selector::RelaySelector;
use std::sync::{
atomic::{AtomicBool, Ordering},
Arc,
};
use talpid_types::net::TunnelType;

const TIMEOUT_ERROR: Error = Error::OtherRestError(mullvad_api::rest::Error::TimeoutError);

Expand Down Expand Up @@ -1437,24 +1435,26 @@ mod test {
);
}

/// Test whether the relay selector selects wireguard often enough, given no special
/// constraints, to verify that the device is valid
#[test]
fn test_validates_by_default() {
for attempt in 0.. {
let should_validate =
TunnelStateChangeHandler::should_check_validity_on_attempt(attempt);
let (_, _, tunnel_type) =
RelaySelector::preferred_tunnel_constraints(attempt.try_into().unwrap());
assert_eq!(
tunnel_type,
TunnelType::Wireguard,
"failed on attempt {attempt}"
);
if should_validate {
// Now that we've triggered a device check, we can give up
break;
}
}
}
// TODO(markus): `preferred_tunnel_constraints` is slated for removal - consider writing a new test which
// does not depend on relay selector internals.
// /// Test whether the relay selector selects wireguard often enough, given no special
// /// constraints, to verify that the device is valid
// #[test]
// fn test_validates_by_default() {
// for attempt in 0.. {
// let should_validate =
// TunnelStateChangeHandler::should_check_validity_on_attempt(attempt);
// let (_, _, tunnel_type) =
// RelaySelector::preferred_tunnel_constraints(attempt.try_into().unwrap());
// assert_eq!(
// tunnel_type,
// TunnelType::Wireguard,
// "failed on attempt {attempt}"
// );
// if should_validate {
// // Now that we've triggered a device check, we can give up
// break;
// }
// }
// }
}
2 changes: 1 addition & 1 deletion mullvad-daemon/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1105,7 +1105,7 @@ where
// Note that `Constraint::Any` corresponds to just IPv4
matches!(
relay_constraints.wireguard_constraints.ip_version,
mullvad_types::relay_constraints::Constraint::Only(IpVersion::V6)
mullvad_types::constraints::Constraint::Only(IpVersion::V6)
)
} else {
false
Expand Down
2 changes: 1 addition & 1 deletion mullvad-daemon/src/migrations/v1.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::Result;
use mullvad_types::{relay_constraints::Constraint, settings::SettingsVersion};
use mullvad_types::{constraints::Constraint, settings::SettingsVersion};
use serde::{Deserialize, Serialize};

// ======================================================
Expand Down
2 changes: 1 addition & 1 deletion mullvad-daemon/src/migrations/v4.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::{Error, Result};
use mullvad_types::{relay_constraints::Constraint, settings::SettingsVersion};
use mullvad_types::{constraints::Constraint, settings::SettingsVersion};
use serde::{Deserialize, Serialize};

// ======================================================
Expand Down
2 changes: 1 addition & 1 deletion mullvad-daemon/src/migrations/v5.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::{Error, Result};
use mullvad_types::{relay_constraints::Constraint, settings::SettingsVersion};
use mullvad_types::{constraints::Constraint, settings::SettingsVersion};
use serde::{Deserialize, Serialize};

// ======================================================
Expand Down
2 changes: 1 addition & 1 deletion mullvad-daemon/src/migrations/v6.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
use super::{Error, Result};
use mullvad_types::{relay_constraints::Constraint, settings::SettingsVersion};
use mullvad_types::{constraints::Constraint, settings::SettingsVersion};
use serde::{Deserialize, Serialize};

// ======================================================
Expand Down
13 changes: 5 additions & 8 deletions mullvad-daemon/src/settings/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -376,16 +376,13 @@ impl<'a> Display for SettingsSummary<'a> {
write!(f, ", wg ip version: {ip_version}")?;
}

let multihop = matches!(
relay_settings,
let multihop = match relay_settings {
RelaySettings::Normal(RelayConstraints {
wireguard_constraints: WireguardConstraints {
use_multihop: true,
..
},
wireguard_constraints,
..
})
);
}) => wireguard_constraints.multihop(),
_ => false,
};

write!(
f,
Expand Down
Loading

0 comments on commit b02fb81

Please sign in to comment.