From b88ac2f7cac3bf69e3e2b641a13d44610b136838 Mon Sep 17 00:00:00 2001 From: Markus Pettersson Date: Mon, 18 Mar 2024 14:09:38 +0100 Subject: [PATCH] Remove `matches_with_opts` --- mullvad-daemon/src/tunnel.rs | 1 - .../src/relay_selector/matcher.rs | 42 ++--- mullvad-types/src/relay_constraints.rs | 154 +++++++----------- mullvad-types/src/relay_list.rs | 7 + 4 files changed, 84 insertions(+), 120 deletions(-) diff --git a/mullvad-daemon/src/tunnel.rs b/mullvad-daemon/src/tunnel.rs index 819bf5bdaaa6..775e04d45c96 100644 --- a/mullvad-daemon/src/tunnel.rs +++ b/mullvad-daemon/src/tunnel.rs @@ -161,7 +161,6 @@ impl InnerParametersGenerator { bridge, } => { let bridge_relay = bridge.as_ref().and_then(|bridge| bridge.relay()); - // TODO(markus): Can this be done 'generically'? self.last_generated_relays = Some(LastSelectedRelays::OpenVpn { relay: exit.clone(), bridge: bridge_relay.cloned(), diff --git a/mullvad-relay-selector/src/relay_selector/matcher.rs b/mullvad-relay-selector/src/relay_selector/matcher.rs index 5e4351af5106..81e920afeba7 100644 --- a/mullvad-relay-selector/src/relay_selector/matcher.rs +++ b/mullvad-relay-selector/src/relay_selector/matcher.rs @@ -86,13 +86,26 @@ impl RelayMatcher { // set to true should always be prioritized over relays which has this flag set to false. // We should only consider relays with `include_in_country` set to false if there are no // other candidates left. - if !shortlist.clone().any(|relay| relay.include_in_country) { - shortlist.cloned().collect() - } else { - shortlist - .filter(|relay| filter_on_include_in_country(&self.locations, relay)) - .cloned() - .collect() + match &self.locations { + Constraint::Any => shortlist.cloned().collect(), + Constraint::Only(locations) => { + let mut included = std::collections::HashSet::new(); + let mut excluded = std::collections::HashSet::new(); + for relay in shortlist { + for location in locations { + if location.is_country() && relay.include_in_country { + included.insert(relay.clone()); + } else { + excluded.insert(relay.clone()); + }; + } + } + if included.is_empty() { + excluded.into_iter().collect() + } else { + included.into_iter().collect() + } + } } } } @@ -225,20 +238,7 @@ pub const fn filter_on_active(relay: &Relay) -> bool { /// Returns whether `relay` satisfy the location constraint posed by `filter`. pub fn filter_on_location(filter: &Constraint, relay: &Relay) -> bool { - filter.matches_with_opts(relay, true) -} - -/// Returns whether `relay` has the `include_in_country` flag set to true. -/// -/// # Note -/// This filter only applies if the underlying [location constraint][`ResolvedLocationConstraint`] -/// is based on country. I.e., this filter does not have any effect if the underlying constraint -/// is scoped to a specific hostname or city. -pub fn filter_on_include_in_country( - filter: &Constraint, - relay: &Relay, -) -> bool { - filter.matches_with_opts(relay, false) + filter.matches(relay) } /// Returns whether `relay` satisfy the ownership constraint posed by `filter`. diff --git a/mullvad-types/src/relay_constraints.rs b/mullvad-types/src/relay_constraints.rs index 742ad6e17b3c..0a074933db43 100644 --- a/mullvad-types/src/relay_constraints.rs +++ b/mullvad-types/src/relay_constraints.rs @@ -86,32 +86,55 @@ pub enum LocationConstraint { } #[derive(Debug, Clone)] -pub enum ResolvedLocationConstraint { - Location(GeographicLocationConstraint), - Locations(Vec), +pub struct ResolvedLocationConstraint(Vec); + +impl<'a> IntoIterator for &'a ResolvedLocationConstraint { + type Item = &'a GeographicLocationConstraint; + + type IntoIter = core::slice::Iter<'a, GeographicLocationConstraint>; + + fn into_iter(self) -> Self::IntoIter { + self.0.iter() + } +} + +impl IntoIterator for ResolvedLocationConstraint { + type Item = GeographicLocationConstraint; + + type IntoIter = std::vec::IntoIter; + + fn into_iter(self) -> Self::IntoIter { + self.0.into_iter() + } +} + +impl FromIterator for ResolvedLocationConstraint { + fn from_iter>(iter: T) -> Self { + Self(Vec::from_iter(iter)) + } } impl ResolvedLocationConstraint { pub fn from_constraint( - location: Constraint, + location_constraint: Constraint, custom_lists: &CustomListsSettings, ) -> Constraint { + location_constraint.map(|location| Self::from_location_constraint(location, custom_lists)) + } + + fn from_location_constraint( + location: LocationConstraint, + custom_lists: &CustomListsSettings, + ) -> ResolvedLocationConstraint { match location { - Constraint::Any => Constraint::Any, - Constraint::Only(LocationConstraint::Location(location)) => { - Constraint::Only(Self::Location(location)) - } - Constraint::Only(LocationConstraint::CustomList { list_id }) => custom_lists + LocationConstraint::Location(location) => Self::from_iter(std::iter::once(location)), + LocationConstraint::CustomList { list_id } => custom_lists .iter() .find(|list| list.id == list_id) - .map(|custom_list| { - Constraint::Only(Self::Locations( - custom_list.locations.iter().cloned().collect(), - )) - }) + .map(|custom_list| Self::from_iter(custom_list.locations.clone())) .unwrap_or_else(|| { log::warn!("Resolved non-existent custom list"); - Constraint::Only(ResolvedLocationConstraint::Locations(vec![])) + Self::from_iter(std::iter::empty()) }), } } @@ -127,26 +150,12 @@ impl Set> for Constraint bool { match self { Constraint::Any => other.is_any(), - Constraint::Only(ResolvedLocationConstraint::Location(location)) => match other { - Constraint::Any => true, - Constraint::Only(ResolvedLocationConstraint::Location(other_location)) => { - location.is_subset(other_location) - } - Constraint::Only(ResolvedLocationConstraint::Locations(other_locations)) => { - other_locations - .iter() - .any(|other_location| location.is_subset(other_location)) - } - }, - Constraint::Only(ResolvedLocationConstraint::Locations(locations)) => match other { + Constraint::Only(locations) => match other { Constraint::Any => true, - Constraint::Only(ResolvedLocationConstraint::Location(other_location)) => locations - .iter() - .all(|location| location.is_subset(other_location)), - Constraint::Only(ResolvedLocationConstraint::Locations(other_locations)) => { + Constraint::Only(other_locations) => { for location in locations { if !other_locations - .iter() + .into_iter() .any(|other_location| location.is_subset(other_location)) { return false; @@ -159,26 +168,9 @@ impl Set> for Constraint { - /// # Note - /// - /// The following statements are true iff `self` is based on [country][`GeographicLocationConstraint::Country`]. - /// - /// * If `ignore_include_in_country` is true, the outcome of `matches_with_opts` is only based on the geographical location of `relay`. - /// - /// * If `ignore_include_in_country` is false, any [relay][`Relay`] which has the - /// `include_in_country` property set to false will cause `matches_with_opts` to return false. - /// Otherwise, the outcome is based on the geographical location of `relay`. - pub fn matches_with_opts(&self, relay: &Relay, ignore_include_in_country: bool) -> bool { - match self { - Constraint::Any => true, - Constraint::Only(ResolvedLocationConstraint::Location(location)) => { - location.matches_with_opts(relay, ignore_include_in_country) - } - Constraint::Only(ResolvedLocationConstraint::Locations(locations)) => locations - .iter() - .any(|loc| loc.matches_with_opts(relay, ignore_include_in_country)), - } +impl Match for ResolvedLocationConstraint { + fn matches(&self, relay: &Relay) -> bool { + self.into_iter().any(|location| location.matches(relay)) } } @@ -356,25 +348,19 @@ impl GeographicLocationConstraint { GeographicLocationConstraint::Hostname(country.into(), city.into(), hostname.into()) } - /// # Note - /// - /// The following statements are true iff `self` is based on [country][`GeographicLocationConstraint::Country`]. - /// - /// * If `ignore_include_in_country` is true, the `include_in_country` property of `relay` is - /// disregarded. The outcome of `matches_with_opts` is only based on the geographical location of `relay`. - /// - /// * If `ignore_include_in_country` is false, any [relay][`Relay`] which has the - /// `include_in_country` property set to false will cause `matches_with_opts` to return false - /// regardless of the geographical location of `relay`. - pub fn matches_with_opts(&self, relay: &Relay, ignore_include_in_country: bool) -> bool { + // TODO(markus): Document + pub fn is_country(&self) -> bool { + matches!(self, GeographicLocationConstraint::Country(_)) + } +} + +impl Match for GeographicLocationConstraint { + fn matches(&self, relay: &Relay) -> bool { match self { - GeographicLocationConstraint::Country(ref country) => { - relay - .location - .as_ref() - .map_or(false, |loc| loc.country_code == *country) - && (ignore_include_in_country || relay.include_in_country) - } + GeographicLocationConstraint::Country(ref country) => relay + .location + .as_ref() + .map_or(false, |loc| loc.country_code == *country), GeographicLocationConstraint::City(ref country, ref city) => { relay.location.as_ref().map_or(false, |loc| { loc.country_code == *country && loc.city_code == *city @@ -391,34 +377,6 @@ impl GeographicLocationConstraint { } } -impl Constraint> { - pub fn matches_with_opts(&self, relay: &Relay, ignore_include_in_country: bool) -> bool { - match self { - Constraint::Only(constraint) => constraint - .iter() - .any(|loc| loc.matches_with_opts(relay, ignore_include_in_country)), - Constraint::Any => true, - } - } -} - -impl Constraint { - pub fn matches_with_opts(&self, relay: &Relay, ignore_include_in_country: bool) -> bool { - match self { - Constraint::Only(constraint) => { - constraint.matches_with_opts(relay, ignore_include_in_country) - } - Constraint::Any => true, - } - } -} - -impl Match for GeographicLocationConstraint { - fn matches(&self, relay: &Relay) -> bool { - self.matches_with_opts(relay, false) - } -} - impl Set for GeographicLocationConstraint { /// Returns whether `self` is equal to or a subset of `other`. fn is_subset(&self, other: &Self) -> bool { diff --git a/mullvad-types/src/relay_list.rs b/mullvad-types/src/relay_list.rs index 7d7545e97f63..1f6f32774a77 100644 --- a/mullvad-types/src/relay_list.rs +++ b/mullvad-types/src/relay_list.rs @@ -116,6 +116,13 @@ impl PartialEq for Relay { // See uniqueness argument in the implementation of [`PartialEq`] for [`Relay`]. impl Eq for Relay {} +// See uniqueness argument in the implementation of [`PartialEq`] for [`Relay`]. +impl std::hash::Hash for Relay { + fn hash(&self, state: &mut H) { + self.hostname.hash(state) + } +} + /// Specifies the type of a relay or relay-specific endpoint data. #[derive(Debug, Clone, PartialEq, Eq, Deserialize, Serialize)] #[serde(rename_all = "snake_case")]