Skip to content

Commit

Permalink
Replace footgunny From implementations for RelayQuery
Browse files Browse the repository at this point in the history
  • Loading branch information
@dlon
dlon committed Aug 29, 2024
1 parent 5ff1c6e commit 012ed67
Show file tree
Hide file tree
Showing 6 changed files with 179 additions and 195 deletions.
10 changes: 5 additions & 5 deletions mullvad-relay-selector/src/relay_selector/mod.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -369,14 +369,14 @@ impl<'a> TryFrom<NormalSelectorConfig<'a>> for RelayQuery {
bridge_settings: match bridge_state {
BridgeState::On => match bridge_settings.bridge_type {
mullvad_types::relay_constraints::BridgeType::Normal => {
Constraint::Only(BridgeQuery::Normal(bridge_settings.normal.clone()))
BridgeQuery::Normal(bridge_settings.normal.clone())
}
mullvad_types::relay_constraints::BridgeType::Custom => {
Constraint::Only(BridgeQuery::Custom(bridge_settings.custom.clone()))
BridgeQuery::Custom(bridge_settings.custom.clone())
}
},
BridgeState::Auto => Constraint::Only(BridgeQuery::Auto),
BridgeState::Off => Constraint::Only(BridgeQuery::Off),
BridgeState::Auto => BridgeQuery::Auto,
BridgeState::Off => BridgeQuery::Off,
},
}
}
Expand Down Expand Up @@ -930,7 +930,7 @@ impl RelaySelector {
if !BridgeQuery::should_use_bridge(&query.openvpn_constraints().bridge_settings) {
Ok(None)
} else {
let bridge_query = &query.openvpn_constraints().bridge_settings.clone().unwrap();
let bridge_query = &query.openvpn_constraints().bridge_settings;
let custom_lists = &custom_lists;
match protocol {
TransportProtocol::Udp => {
Expand Down
182 changes: 116 additions & 66 deletions mullvad-relay-selector/src/relay_selector/query.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -32,9 +32,10 @@ use crate::{AdditionalWireguardConstraints, Error};
use mullvad_types::{
constraints::Constraint,
relay_constraints::{
BridgeConstraints, LocationConstraint, ObfuscationSettings, OpenVpnConstraints, Ownership,
Providers, RelayConstraints, RelaySettings, SelectedObfuscation, ShadowsocksSettings,
TransportPort, Udp2TcpObfuscationSettings, WireguardConstraints,
BridgeConstraints, BridgeSettings, BridgeState, BridgeType, LocationConstraint,
ObfuscationSettings, OpenVpnConstraints, Ownership, Providers, RelayConstraints,
SelectedObfuscation, ShadowsocksSettings, TransportPort, Udp2TcpObfuscationSettings,
WireguardConstraints,
},
wireguard::QuantumResistantState,
Intersection,
Expand Down Expand Up @@ -179,6 +180,42 @@ impl RelayQuery {

Ok(())
}

/// The mapping from [`RelayQuery`] to all underlying settings types.
///
/// Useful in contexts where you cannot use the query directly but
/// still want use of the builder for convenience. For example in
/// end to end tests where you must use the management interface
/// to apply settings to the daemon.
pub fn into_settings(
self,
) -> (
RelayConstraints,
BridgeState,
BridgeSettings,
ObfuscationSettings,
) {
let (bridge_state, bridge_settings) = self
.openvpn_constraints
.bridge_settings
.clone()
.into_settings();
let obfuscation = self
.wireguard_constraints
.obfuscation
.clone()
.into_settings();
let constraints = RelayConstraints {
location: self.location,
providers: self.providers,
ownership: self.ownership,
tunnel_protocol: self.tunnel_protocol,
wireguard_constraints: self.wireguard_constraints.into_constraints(),
openvpn_constraints: self.openvpn_constraints.into_constraints(),
};

(constraints, bridge_state, bridge_settings, obfuscation)
}
}

impl Default for RelayQuery {
Expand Down Expand Up @@ -207,26 +244,6 @@ impl Default for RelayQuery {
}
}

impl From<RelayQuery> for RelayConstraints {
/// The mapping from [`RelayQuery`] to [`RelayConstraints`].
fn from(value: RelayQuery) -> Self {
RelayConstraints {
location: value.location,
providers: value.providers,
ownership: value.ownership,
tunnel_protocol: value.tunnel_protocol,
wireguard_constraints: WireguardConstraints::from(value.wireguard_constraints),
openvpn_constraints: OpenVpnConstraints::from(value.openvpn_constraints),
}
}
}

impl From<RelayQuery> for RelaySettings {
fn from(value: RelayQuery) -> Self {
RelayConstraints::from(value).into()
}
}

/// A query for a relay with Wireguard-specific properties, such as `multihop` and [wireguard
/// obfuscation][`SelectedObfuscation`].
///
Expand Down Expand Up @@ -256,6 +273,31 @@ pub enum ObfuscationQuery {
Shadowsocks(ShadowsocksSettings),
}

impl ObfuscationQuery {
fn into_settings(self) -> ObfuscationSettings {
match self {
ObfuscationQuery::Auto => ObfuscationSettings {
selected_obfuscation: SelectedObfuscation::Auto,
..Default::default()
},
ObfuscationQuery::Off => ObfuscationSettings {
selected_obfuscation: SelectedObfuscation::Off,
..Default::default()
},
ObfuscationQuery::Udp2tcp(settings) => ObfuscationSettings {
selected_obfuscation: SelectedObfuscation::Udp2Tcp,
udp2tcp: settings,
..Default::default()
},
ObfuscationQuery::Shadowsocks(settings) => ObfuscationSettings {
selected_obfuscation: SelectedObfuscation::Shadowsocks,
shadowsocks: settings,
..Default::default()
},
}
}
}

impl From<ObfuscationSettings> for ObfuscationQuery {
/// A query for obfuscation settings.
///
Expand Down Expand Up @@ -307,6 +349,16 @@ impl WireguardRelayQuery {
quantum_resistant: QuantumResistantState::Auto,
}
}

/// The mapping from [`WireguardRelayQuery`] to [`WireguardConstraints`].
fn into_constraints(self) -> WireguardConstraints {
WireguardConstraints {
port: self.port,
ip_version: self.ip_version,
entry_location: self.entry_location,
use_multihop: self.use_multihop.unwrap_or(false),
}
}
}

impl Default for WireguardRelayQuery {
Expand All @@ -315,18 +367,6 @@ impl Default for WireguardRelayQuery {
}
}

impl From<WireguardRelayQuery> for WireguardConstraints {
/// The mapping from [`WireguardRelayQuery`] to [`WireguardConstraints`].
fn from(value: WireguardRelayQuery) -> Self {
WireguardConstraints {
port: value.port,
ip_version: value.ip_version,
entry_location: value.entry_location,
use_multihop: value.use_multihop.unwrap_or(false),
}
}
}

impl From<WireguardRelayQuery> for AdditionalWireguardConstraints {
/// The mapping from [`WireguardRelayQuery`] to [`AdditionalWireguardConstraints`].
fn from(value: WireguardRelayQuery) -> Self {
Expand All @@ -348,16 +388,21 @@ impl From<WireguardRelayQuery> for AdditionalWireguardConstraints {
#[derive(Debug, Clone, Eq, PartialEq, Intersection)]
pub struct OpenVpnRelayQuery {
pub port: Constraint<TransportPort>,
pub bridge_settings: Constraint<BridgeQuery>,
pub bridge_settings: BridgeQuery,
}

impl OpenVpnRelayQuery {
pub const fn new() -> OpenVpnRelayQuery {
OpenVpnRelayQuery {
port: Constraint::Any,
bridge_settings: Constraint::Any,
bridge_settings: BridgeQuery::Auto,
}
}

/// The mapping from [`OpenVpnRelayQuery`] to [`OpenVpnConstraints`].
fn into_constraints(self) -> OpenVpnConstraints {
OpenVpnConstraints { port: self.port }
}
}

impl Default for OpenVpnRelayQuery {
Expand All @@ -371,14 +416,15 @@ impl Default for OpenVpnRelayQuery {
///
/// [`BridgeState`]: mullvad_types::relay_constraints::BridgeState
/// [`BridgeSettings`]: mullvad_types::relay_constraints::BridgeSettings
#[derive(Debug, Clone, Eq, PartialEq)]
#[derive(Debug, Default, Clone, Eq, PartialEq)]
pub enum BridgeQuery {
/// Bridges should not be used.
Off,
/// Don't care, let the relay selector choose!
///
/// If this variant is intersected with another [`BridgeQuery`] `bq`,
/// `bq` is always preferred.
#[default]
Auto,
/// Bridges should be used.
Normal(BridgeConstraints),
Expand All @@ -387,22 +433,38 @@ pub enum BridgeQuery {
}

impl BridgeQuery {
/// If `bridge_constraints` is `Any`, bridges should not be used due to
/// latency concerns.
///
/// If `bridge_constraints` is `Only(settings)`, then `settings` will be
/// used to decide if bridges should be used. See [`BridgeQuery`] for more
/// details, but the algorithm beaks down to this:
/// `settings` will be used to decide if bridges should be used. See [`BridgeQuery`]
/// for more details, but the algorithm beaks down to this:
///
/// * `BridgeQuery::Off`: bridges will not be used
/// * otherwise: bridges should be used
pub const fn should_use_bridge(bridge_constraints: &Constraint<BridgeQuery>) -> bool {
match bridge_constraints {
Constraint::Only(settings) => match settings {
BridgeQuery::Normal(_) | BridgeQuery::Custom(_) => true,
BridgeQuery::Off | BridgeQuery::Auto => false,
},
Constraint::Any => false,
pub const fn should_use_bridge(settings: &BridgeQuery) -> bool {
match settings {
BridgeQuery::Normal(_) | BridgeQuery::Custom(_) => true,
BridgeQuery::Off | BridgeQuery::Auto => false,
}
}

fn into_settings(self) -> (BridgeState, BridgeSettings) {
match self {
BridgeQuery::Off => (BridgeState::Off, Default::default()),
BridgeQuery::Auto => (BridgeState::Auto, Default::default()),
BridgeQuery::Normal(constraints) => (
BridgeState::On,
BridgeSettings {
bridge_type: BridgeType::Normal,
normal: constraints,
custom: None,
},
),
BridgeQuery::Custom(custom) => (
BridgeState::On,
BridgeSettings {
bridge_type: BridgeType::Normal,
normal: Default::default(),
custom,
},
),
}
}
}
Expand All @@ -425,13 +487,6 @@ impl Intersection for BridgeQuery {
}
}

impl From<OpenVpnRelayQuery> for OpenVpnConstraints {
/// The mapping from [`OpenVpnRelayQuery`] to [`OpenVpnConstraints`].
fn from(value: OpenVpnRelayQuery) -> Self {
OpenVpnConstraints { port: value.port }
}
}

#[allow(unused)]
pub mod builder {
//! Strongly typed Builder pattern for of relay constraints though the use of the Typestate
Expand Down Expand Up @@ -498,10 +553,6 @@ pub mod builder {
debug_assert!(self.query.validate().is_ok());
self.query
}

pub fn into_constraint(self) -> RelayConstraints {
RelayConstraints::from(self.build())
}
}

impl RelayQueryBuilder<Any> {
Expand Down Expand Up @@ -802,8 +853,7 @@ pub mod builder {
bridge_settings: bridge_settings.clone(),
};

self.query.openvpn_constraints.bridge_settings =
Constraint::Only(BridgeQuery::Normal(bridge_settings));
self.query.openvpn_constraints.bridge_settings = BridgeQuery::Normal(bridge_settings);

let builder = RelayQueryBuilder {
query: self.query,
Expand All @@ -820,14 +870,14 @@ pub mod builder {
self.protocol.bridge_settings.location =
Constraint::Only(LocationConstraint::from(location));
self.query.openvpn_constraints.bridge_settings =
Constraint::Only(BridgeQuery::Normal(self.protocol.bridge_settings.clone()));
BridgeQuery::Normal(self.protocol.bridge_settings.clone());
self
}
/// Constrain the [`Providers`] of the selected bridge.
pub fn bridge_providers(mut self, providers: Providers) -> Self {
self.protocol.bridge_settings.providers = Constraint::Only(providers);
self.query.openvpn_constraints.bridge_settings =
Constraint::Only(BridgeQuery::Normal(self.protocol.bridge_settings.clone()));
BridgeQuery::Normal(self.protocol.bridge_settings.clone());
self
}
/// Constrain the [`Ownership`] of the selected bridge.
Expand Down
7 changes: 4 additions & 3 deletions mullvad-relay-selector/tests/relay_selector.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1292,7 +1292,7 @@ fn openvpn_handle_bridge_settings() {
// should fail.
query
.set_openvpn_constraints(OpenVpnRelayQuery {
bridge_settings: Constraint::Only(BridgeQuery::Normal(BridgeConstraints::default())),
bridge_settings: BridgeQuery::Normal(BridgeConstraints::default()),
..query.openvpn_constraints().clone()
})
.unwrap();
Expand Down Expand Up @@ -1524,9 +1524,10 @@ fn valid_user_setting_should_yield_relay() {
// Make a valid user relay constraint
let location = GeographicLocationConstraint::hostname("se", "got", "se9-wireguard");
let user_query = RelayQueryBuilder::new().location(location.clone()).build();
let user_constraints = RelayQueryBuilder::new()
let (user_constraints, ..) = RelayQueryBuilder::new()
.location(location.clone())
.into_constraint();
.build()
.into_settings();

let config = SelectorConfig {
relay_settings: user_constraints.into(),
Expand Down
9 changes: 9 additions & 0 deletions mullvad-types/src/constraints/constraint.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,15 @@ impl<T: fmt::Display> fmt::Display for Constraint<T> {
}
}

impl<T: Default> Constraint<T> {
pub fn unwrap_or_default(self) -> T {
match self {
Constraint::Any => Default::default(),
Constraint::Only(value) => value,
}
}
}

impl<T> Constraint<T> {
pub fn unwrap(self) -> T {
match self {
Expand Down
Loading

0 comments on commit 012ed67

Please sign in to comment.