Skip to content

Commit

Permalink
Remove matches_with_opts
Browse files Browse the repository at this point in the history
  • Loading branch information
@MarkusPettersson98
MarkusPettersson98 committed Mar 18, 2024
1 parent c7b048d commit 69bc7f3
Show file tree
Hide file tree
Showing 4 changed files with 84 additions and 120 deletions.
1 change: 0 additions & 1 deletion mullvad-daemon/src/tunnel.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
42 changes: 21 additions & 21 deletions mullvad-relay-selector/src/relay_selector/matcher.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,13 +86,26 @@ impl<T: EndpointMatcher> RelayMatcher<T> {
// 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()
}
}
}
}
}
Expand Down Expand Up @@ -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<ResolvedLocationConstraint>, 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<ResolvedLocationConstraint>,
relay: &Relay,
) -> bool {
filter.matches_with_opts(relay, false)
filter.matches(relay)
}

/// Returns whether `relay` satisfy the ownership constraint posed by `filter`.
Expand Down
154 changes: 56 additions & 98 deletions mullvad-types/src/relay_constraints.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,32 +86,55 @@ pub enum LocationConstraint {
}

#[derive(Debug, Clone)]
pub enum ResolvedLocationConstraint {
Location(GeographicLocationConstraint),
Locations(Vec<GeographicLocationConstraint>),
pub struct ResolvedLocationConstraint(Vec<GeographicLocationConstraint>);

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<GeographicLocationConstraint>;

fn into_iter(self) -> Self::IntoIter {
self.0.into_iter()
}
}

impl FromIterator<GeographicLocationConstraint> for ResolvedLocationConstraint {
fn from_iter<T: IntoIterator<Item = GeographicLocationConstraint>>(iter: T) -> Self {
Self(Vec::from_iter(iter))
}
}

impl ResolvedLocationConstraint {
pub fn from_constraint(
location: Constraint<LocationConstraint>,
location_constraint: Constraint<LocationConstraint>,
custom_lists: &CustomListsSettings,
) -> Constraint<ResolvedLocationConstraint> {
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())
}),
}
}
Expand All @@ -127,26 +150,12 @@ impl Set<Constraint<ResolvedLocationConstraint>> for Constraint<ResolvedLocation
fn is_subset(&self, other: &Self) -> 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;
Expand All @@ -159,26 +168,9 @@ impl Set<Constraint<ResolvedLocationConstraint>> for Constraint<ResolvedLocation
}
}

impl Constraint<ResolvedLocationConstraint> {
/// # 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<Relay> for ResolvedLocationConstraint {
fn matches(&self, relay: &Relay) -> bool {
self.into_iter().any(|location| location.matches(relay))
}
}

Expand Down Expand Up @@ -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<Relay> 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
Expand All @@ -391,34 +377,6 @@ impl GeographicLocationConstraint {
}
}

impl Constraint<Vec<GeographicLocationConstraint>> {
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<GeographicLocationConstraint> {
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<Relay> for GeographicLocationConstraint {
fn matches(&self, relay: &Relay) -> bool {
self.matches_with_opts(relay, false)
}
}

impl Set<GeographicLocationConstraint> for GeographicLocationConstraint {
/// Returns whether `self` is equal to or a subset of `other`.
fn is_subset(&self, other: &Self) -> bool {
Expand Down
7 changes: 7 additions & 0 deletions mullvad-types/src/relay_list.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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<H: std::hash::Hasher>(&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")]
Expand Down

0 comments on commit 69bc7f3

Please sign in to comment.