Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor mullvad-relay-selector #5935

Conversation

MarkusPettersson98
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 commented Mar 11, 2024

Buckle up!

This PR builds on the foundations laid out in #5835 by implementing a system for choosing appropriate relay(s) built on 'queries'. The intent is to make working with the relay selector as declarative as possible while simplifying and modularizing its internals.

Elevator pitch

Tell the computer what you want, not what to do - ElKowar

A query is a set of constraints which dictates which relay(s) that can be chosen by the relay selector.

The user's relay constraints (settings) can be expressed as a query. The semantics of merging two queries in a way that always respects user settings is defined by the new Intersection trait. It should be easy to correlate the code found in the new mullvad-relay-selector crate with the description found in the relay selector document on how it should behave.

Code changes

Retrofitting this new system for driving the relay selector through queries was not possible without rewriting most of mullvad-relay-selector. Here is a short breakdown of the new crate structure

  • src/lib.rs: The public API of the mullvad-relay-selector crate.
  • src/relay-selector/mod.rs: The new module where RelaySelector is defined. This is still the largest module in the crate (except for tests).
  • src/relay-selector/query.rs: Definition of a query on different types of relays. This module is integral to the new API of mullvad-relay-selector
  • src/relay-selector/matcher.rs: Logic for filtering out candidate relays based on a query.
  • src/relay-selector/detailer.rs: Logic for deriving connection details for the selected relay(s). This could arguably live outside of the mullvad-relay-selector crate. For now, I placed it here since this logic was in the old mullvad-relay-selector.
  • tests/: Integration tests for the new relay selector. These tests only use the public APIs of RelaySelector and make sure that the actual output matches the expected output in different scenarios. This is the spiritual successor to the tests found in the old mullvad-relay-selector crate.
  • The rest of the modules found in the new mullvad-relay-selector crate is either not that interesting or self-explanatory.

This change is Reviewable

Copy link

linear bot commented Mar 11, 2024

@MarkusPettersson98 MarkusPettersson98 force-pushed the optimize-order-of-connection-parameters-when-trying-to-des-543 branch from ad22251 to b02fb81 Compare March 11, 2024 09:32
mullvad-types/Cargo.toml Outdated Show resolved Hide resolved
@MarkusPettersson98 MarkusPettersson98 force-pushed the optimize-order-of-connection-parameters-when-trying-to-des-543 branch 5 times, most recently from c377ef6 to 467c015 Compare March 11, 2024 15:14
@MarkusPettersson98 MarkusPettersson98 force-pushed the optimize-order-of-connection-parameters-when-trying-to-des-543 branch 2 times, most recently from 691c3dd to 01fcea7 Compare March 12, 2024 14:42
@MarkusPettersson98 MarkusPettersson98 force-pushed the optimize-order-of-connection-parameters-when-trying-to-des-543 branch from ebc30e8 to 2fd5e56 Compare March 13, 2024 09:45
@MarkusPettersson98 MarkusPettersson98 force-pushed the optimize-order-of-connection-parameters-when-trying-to-des-543 branch from fe80557 to d5f4bca Compare March 13, 2024 14:17
@MarkusPettersson98 MarkusPettersson98 force-pushed the optimize-order-of-connection-parameters-when-trying-to-des-543 branch 7 times, most recently from 85f3939 to f8ec68b Compare March 20, 2024 16:02
@dlon dlon requested a review from Serock3 March 20, 2024 21:35
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 8 of 8 files at r36, 6 of 8 files at r37, 3 of 3 files at r38, all commit messages.
Reviewable status: all files reviewed, 29 unresolved discussions (waiting on @MarkusPettersson98 and @Serock3)


mullvad-relay-selector/src/relay_selector/mod.rs line 119 at r36 (raw file):

/// The second one is a custom config, where [`SelectorConfig::relay_settings`] is [`RelaySettings::Custom`].
/// For this variant, the endpoint where the client should connect to is already specified inside of the variant,
/// so in practice the relay selector becomes superflous. Also, there exists no mapping to [`RelayQueryBuilder`].

Nit: superfluous


mullvad-types/src/relay_constraints.rs line 396 at r38 (raw file):

#[derive(Debug, Clone, Eq, PartialEq, Deserialize, Serialize)]
pub struct Providers {
    pub providers: HashSet<Provider>,

Can we make it not public? I think losing the encapsulation means you technically lose the guarantee of it being non-empty.


talpid-types/src/net/mod.rs line 563 at r38 (raw file):

    pub fn has_ipv6(&self) -> bool {
        match self {
            Connectivity::Status { ipv6, .. } => *ipv6,

Nit: matches!(self, Connectivity::Status { ipv6: true, .. })

Copy link
Contributor Author

@MarkusPettersson98 MarkusPettersson98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 27 unresolved discussions (waiting on @dlon and @Serock3)


mullvad-types/src/relay_constraints.rs line 396 at r38 (raw file):

Previously, dlon (David Lönnhager) wrote…

Can we make it not public? I think losing the encapsulation means you technically lose the guarantee of it being non-empty.

Yes, you are right. Getting a read-only reference to the underlying set of providers is enough for my use case anyway, so I'll go ahead and add a method for that 😊

@MarkusPettersson98 MarkusPettersson98 force-pushed the optimize-order-of-connection-parameters-when-trying-to-des-543 branch from 3655554 to e52697b Compare March 21, 2024 09:44
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r39, all commit messages.
Reviewable status: all files reviewed, 26 unresolved discussions (waiting on @Serock3)

@MarkusPettersson98 MarkusPettersson98 force-pushed the optimize-order-of-connection-parameters-when-trying-to-des-543 branch 3 times, most recently from f1ac4c6 to 7db41f3 Compare March 21, 2024 15:49
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r40, 1 of 1 files at r41, all commit messages.
Reviewable status: all files reviewed, 26 unresolved discussions (waiting on @Serock3)

@MarkusPettersson98 MarkusPettersson98 force-pushed the optimize-order-of-connection-parameters-when-trying-to-des-543 branch from 7db41f3 to 3207b4d Compare March 25, 2024 12:29
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r42, all commit messages.
Reviewable status: all files reviewed, 25 unresolved discussions (waiting on @Serock3)

@MarkusPettersson98 MarkusPettersson98 force-pushed the optimize-order-of-connection-parameters-when-trying-to-des-543 branch 2 times, most recently from 427eb6f to da09c4d Compare March 27, 2024 08:55
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r43, all commit messages.
Reviewable status: all files reviewed, 25 unresolved discussions (waiting on @Serock3)

@MarkusPettersson98 MarkusPettersson98 force-pushed the optimize-order-of-connection-parameters-when-trying-to-des-543 branch 2 times, most recently from 55c1e40 to 8fb08bb Compare March 27, 2024 09:41
Copy link
Member

@raksooo raksooo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r44, 1 of 1 files at r45.
Reviewable status: all files reviewed, 25 unresolved discussions (waiting on @Serock3)

@MarkusPettersson98 MarkusPettersson98 force-pushed the optimize-order-of-connection-parameters-when-trying-to-des-543 branch from 8fb08bb to fbbbb0c Compare March 27, 2024 10:08
Copy link
Member

@dlon dlon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r46, all commit messages.
Reviewable status: all files reviewed, 25 unresolved discussions (waiting on @Serock3)

Implement a system built on 'queries' for selecting appropriate relays.
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.

Split `mullvad-relay-selector` 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.
@MarkusPettersson98 MarkusPettersson98 force-pushed the optimize-order-of-connection-parameters-when-trying-to-des-543 branch from fbbbb0c to 707ecf4 Compare March 27, 2024 10:44
@MarkusPettersson98 MarkusPettersson98 merged commit d867bd8 into main Mar 27, 2024
47 of 50 checks passed
@MarkusPettersson98 MarkusPettersson98 deleted the optimize-order-of-connection-parameters-when-trying-to-des-543 branch March 27, 2024 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants