Implement new relay selector proto

[Serock3]

---

This change is 

[linear]

DES-2812 Implement new gRPC service for querying the relay selector

DES-2815 Document new gRPC API

[MarkusPettersson98]

@MarkusPettersson98 reviewed 1 file and all commit messages, and made 7 comments.
Reviewable status: 1 of 3 files reviewed, 7 unresolved discussions (waiting on @Serock3).

---

mullvad-management-interface/proto/relay_selector.proto line 1 at r2 (raw file):

// First draft of the new Relay Selector API.

⛏️ We should not reference the version nor that it's technically a draft in this top-level document.

"Relay Selector as a service" or something generic like that will do for now. We should flesh it out later:)

Code quote:

// First draft of the new Relay Selector API.

---

mullvad-management-interface/proto/relay_selector.proto line 8 at r2 (raw file):

service RelaySelectorService {
  // For a given relay constraint, return the list of matching relays and a list

"For a given set of relay constraints, .." ?

Code quote:

// For a given relay constraint

---

mullvad-management-interface/proto/relay_selector.proto line 28 at r2 (raw file):

// Constraints that apply when selecting any relay
message GeneralConstraints {
  mullvad_daemon.management_interface.LocationConstraint location = 1;

Is there a protobuf way of importing types from a different namespace? Or will we always have to prefix all management_interface types with mullvad_daemon.management_interface?:(

Code quote:

mullvad_daemon.management_interface.LocationConstraint location = 1;

---

mullvad-management-interface/proto/relay_selector.proto line 29 at r2 (raw file):

message GeneralConstraints {
  mullvad_daemon.management_interface.LocationConstraint location = 1;
  repeated string providers = 2;

⛏️ Should we type everything relay selector-related? If so, I propose the following change

Code quote (i):

 repeated string providers = 2;

Code snippet (ii):

repeated Provider providers = 2;

// A provider hosting relays on behalf of Mullvad.
message Provider {
  string name = 1;
}

---

mullvad-management-interface/proto/relay_selector.proto line 24 at r2 (raw file):

    MultiHopConstraints exit_constraints = 4;
  }
}

⛏️ Let's work out a doc-comment explaining the Selection message:)

Code quote:

message Selection {
  oneof selection_context {
    // Give me a relay list for regular single scenario
    EntryRelayConstraints single_relay_constraints = 1;
    // Give me a relay list for when DAITA direct only is disabled
    GeneralConstraints smart_hop_relay_constraints = 2;
    // Give me a list entry relays when multihop is enabled
    MultiHopConstraints entry_constraints = 3;
    // Give me a list exit relays when multihop is enabled
    MultiHopConstraints exit_constraints = 4;
  }
}

---

mullvad-management-interface/proto/relay_selector.proto line 10 at r2 (raw file):

  // For a given relay constraint, return the list of matching relays and a list
  // of non-matching relays along with the reasons they were filtered out.
  rpc GetMatchingRelays(Selection) returns (FilteredRelays) {}

⛏️ We can get rid of the trailing brackets

Code quote (i):

  rpc GetMatchingRelays(Selection) returns (FilteredRelays) {}

Code snippet (ii):

  rpc GetMatchingRelays(Selection) returns (FilteredRelays);

---

mullvad-management-interface/build.rs line 7 at r2 (raw file):

            &[
                "proto/management_interface.proto",
                "proto/relay_selector.proto",

If we are to version the relay selector API, do we need to embed the version in the file name? e.g. "relay_selector_v1.proto"?

Code quote:

"proto/relay_selector.proto",

[MarkusPettersson98]

@MarkusPettersson98 made 2 comments.
Reviewable status: 1 of 3 files reviewed, 9 unresolved discussions (waiting on @Serock3).

---

mullvad-management-interface/proto/relay_selector.proto line 41 at r2 (raw file):

// All constraints that apply when selecting a relay directly connected to the user
message EntryRelayConstraints {

"// All constraints that apply when selecting an entry relay. I.e. the first point of contact between a client and an exit."

Code quote:

// All constraints that apply when selecting a relay directly connected to the user
message EntryRelayConstraints {

---

mullvad-management-interface/proto/relay_selector.proto line 46 at r2 (raw file):

}

// Constraints for both hops in a multi-hop connection

multihop

Code quote:

 multi-hop

[MarkusPettersson98]

@MarkusPettersson98 made 1 comment.
Reviewable status: 1 of 3 files reviewed, 10 unresolved discussions (waiting on @Serock3).

---

mullvad-management-interface/proto/relay_selector.proto line 49 at r2 (raw file):

message MultiHopConstraints {
  EntryRelayConstraints entry = 1;
  GeneralConstraints exit = 3;

These integers are not consecutive

Code quote:

  EntryRelayConstraints entry = 1;
  GeneralConstraints exit = 3;

[MarkusPettersson98]

@MarkusPettersson98 made 1 comment.
Reviewable status: 1 of 3 files reviewed, 11 unresolved discussions (waiting on @Serock3).

---

mullvad-management-interface/proto/relay_selector.proto line 97 at r2 (raw file):

  bool anti_censorship = 7;
  // The relay *does*  support the requested anti-censorship method, but not with the requested port
  bool port = 8;

⛏️ I think this comment should read more like ip_version

"// The relay cannot be connected to via the requested port"

Code quote:

  // The relay *does*  support the requested anti-censorship method, but not with the requested port
  bool port = 8;

[Serock3]

@Serock3 made 11 comments and resolved 3 discussions.
Reviewable status: 1 of 3 files reviewed, 8 unresolved discussions (waiting on @MarkusPettersson98).

---

mullvad-management-interface/build.rs line 7 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

If we are to version the relay selector API, do we need to embed the version in the file name? e.g. "relay_selector_v1.proto"?

That's probably a good idea if we think that it's likely we'll need more versions. We could also let the first version skip _v1. Not sure what the right call is.

---

mullvad-management-interface/proto/relay_selector.proto line 1 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

⛏️ We should not reference the version nor that it's technically a draft in this top-level document.

"Relay Selector as a service" or something generic like that will do for now. We should flesh it out later:)

Good catch! I replace it was a short introduction doc.

---

mullvad-management-interface/proto/relay_selector.proto line 8 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

"For a given set of relay constraints, .." ?

Updated the docstring to be much more thorough.

---

mullvad-management-interface/proto/relay_selector.proto line 10 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

⛏️ We can get rid of the trailing brackets

What!? I thought the brackets were required. Be gone 🪓

---

mullvad-management-interface/proto/relay_selector.proto line 24 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

⛏️ Let's work out a doc-comment explaining the Selection message:)

Agree, these need to be updated.

---

mullvad-management-interface/proto/relay_selector.proto line 28 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Is there a protobuf way of importing types from a different namespace? Or will we always have to prefix all management_interface types with mullvad_daemon.management_interface?:(

I seems the mullvad_daemon. suffix can be scrapped! The rest has to be there, unfortunately.

---

mullvad-management-interface/proto/relay_selector.proto line 29 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

⛏️ Should we type everything relay selector-related? If so, I propose the following change

I have no strong opinions about this. I'm not sure if it make the API more future proof, but it might be nice for consistency anyway?

---

mullvad-management-interface/proto/relay_selector.proto line 41 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

"// All constraints that apply when selecting an entry relay. I.e. the first point of contact between a client and an exit."

I don't think we should mention the exit here, since it is also applies to singlehop.

---

mullvad-management-interface/proto/relay_selector.proto line 46 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

multihop

Fixed

---

mullvad-management-interface/proto/relay_selector.proto line 49 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

These integers are not consecutive

Fixed

---

mullvad-management-interface/proto/relay_selector.proto line 97 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

⛏️ I think this comment should read more like ip_version

"// The relay cannot be connected to via the requested port"

You might be right. I just wanted to make the behavior clear when e.g. selecting both a port and an obfuscation method. I suggest that this flag is only used when the anti_censorship flag is false.

[MarkusPettersson98]

@MarkusPettersson98 reviewed 1 file and all commit messages, made 6 comments, and resolved 3 discussions.
Reviewable status: 2 of 3 files reviewed, 7 unresolved discussions (waiting on @Serock3).

---

mullvad-management-interface/proto/relay_selector.proto line 10 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

What!? I thought the brackets were required. Be gone 🪓

🪓

---

mullvad-management-interface/proto/relay_selector.proto line 28 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

I seems the mullvad_daemon. suffix can be scrapped! The rest has to be there, unfortunately.

I see. It's something!

---

mullvad-management-interface/proto/relay_selector.proto line 41 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

I don't think we should mention the exit here, since it is also applies to singlehop.

Singlehop is just the case when the entry and exit are the same. IMO it still makes sense to talk about both entry and exit in the context of a singlehop connection

---

mullvad-management-interface/proto/relay_selector.proto line 97 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

You might be right. I just wanted to make the behavior clear when e.g. selecting both a port and an obfuscation method. I suggest that this flag is only used when the anti_censorship flag is false.

That is not quite true though, since not all ports between 0..=u16::MAX are valid ports for a vanilla WireGuard connection. In theory, the client could send in a query / Selection with an arbitrary port

---

mullvad-management-interface/proto/relay_selector.proto line 19 at r3 (raw file):

  // constraints one/a pair of these relays will be selected at random.
  // Also returns a list of non-matching relays along with the set of
  // constraints/conditions that made it unavailable.

made them unavailable*

Code quote:

  // Also returns a list of non-matching relays along with the set of
  // constraints/conditions that made it unavailable.

---

mullvad-management-interface/proto/relay_selector.proto line 77 at r3 (raw file):

  // This can be interpreted as a list of criteria necessary
  // to unblock the relay from being selectable.
  NonMatchingConstraints non_matching_constraints = 2;

What do you think of naming this field "why"?

The message name already conveys that the relay has been rejected/discarded, and non_matching_constraints is just a set of reasons for why this particular relay was discarded.

Looking ahead a bit, I think it would read nicely in the client code. Consider the following code

let selection = ..;
let relays = relay_selector.get_matching_relays(selection);

let relays = relay_selector.get_matching_relays(..);
// check if any relays where filtered out because of DAITA.
if relays.discarded.iter().any(|rejected| rejected.why.daita) {
  ..
}

Reading the above code, I could also see us naming it something like because. But you get the point, something short would be very fluent & cool!

Also, maybe rename discarded to discards to imply that it is a collection?

Code quote:

  // The list of constraints that did not match the relay.
  // This can be interpreted as a list of criteria necessary
  // to unblock the relay from being selectable.
  NonMatchingConstraints non_matching_constraints = 2;

[Serock3]

@Serock3 made 3 comments and resolved 2 discussions.
Reviewable status: 2 of 3 files reviewed, 5 unresolved discussions (waiting on @MarkusPettersson98).

---

mullvad-management-interface/proto/relay_selector.proto line 41 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Singlehop is just the case when the entry and exit are the same. IMO it still makes sense to talk about both entry and exit in the context of a singlehop connection

That's true, I added your version

---

mullvad-management-interface/proto/relay_selector.proto line 97 at r2 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

That is not quite true though, since not all ports between 0..=u16::MAX are valid ports for a vanilla WireGuard connection. In theory, the client could send in a query / Selection with an arbitrary port

That would require them to pick the "port" anti-censorship method, though, which every relay supports, so the anti_censorship flag would be flalse. Maybe I'm hard coding the way obfuscation methods and wg port bundled in the GUI too much, though. I'll go back to naming field nr 7 obfuscation and then use your phrasing for the port doc.

---

mullvad-management-interface/proto/relay_selector.proto line 77 at r3 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

What do you think of naming this field "why"?

The message name already conveys that the relay has been rejected/discarded, and non_matching_constraints is just a set of reasons for why this particular relay was discarded.

Looking ahead a bit, I think it would read nicely in the client code. Consider the following code

let selection = ..;
let relays = relay_selector.get_matching_relays(selection);

let relays = relay_selector.get_matching_relays(..);
// check if any relays where filtered out because of DAITA.
if relays.discarded.iter().any(|rejected| rejected.why.daita) {
  ..
}

Reading the above code, I could also see us naming it something like because. But you get the point, something short would be very fluent & cool!

Also, maybe rename discarded to discards to imply that it is a collection?

Hm, I like it. I'll go ahead and pick "why" for now. If we want to be a little more explicit, we could call it e.g. rejected_by. I like discards too, nice to have consistent grammar.

[MarkusPettersson98]

@MarkusPettersson98 reviewed 1 file and all commit messages, made 5 comments, and resolved 3 discussions.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @Serock3).

---

mullvad-management-interface/build.rs line 7 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

That's probably a good idea if we think that it's likely we'll need more versions. We could also let the first version skip _v1. Not sure what the right call is.

I realize that we would probably want to version the service in relay_selector.proto (i.e. service RelaySelectorService). So we can keep the current file names!

---

mullvad-management-interface/proto/relay_selector.proto line 29 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

I have no strong opinions about this. I'm not sure if it make the API more future proof, but it might be nice for consistency anyway?

Consistency is big. I also think it looks nicer if the docs can be maintained together with the message definition. But this is a very minor ⛏️, feel free to ignore.

---

mullvad-management-interface/proto/relay_selector.proto line 41 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

That's true, I added your version

:D

---

mullvad-management-interface/proto/relay_selector.proto line 97 at r2 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

That would require them to pick the "port" anti-censorship method, though, which every relay supports, so the anti_censorship flag would be flalse. Maybe I'm hard coding the way obfuscation methods and wg port bundled in the GUI too much, though. I'll go back to naming field nr 7 obfuscation and then use your phrasing for the port doc.

Yeah, I think the way anti-censorship works in the GUI spilled over a bit too much in the previous iteration. We have to consider that the client can make arbitrary queries, and not just valid queries:)

I think it makes sense to split anti-censorship into obfuscation and port.

---

mullvad-management-interface/proto/relay_selector.proto line 77 at r3 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Hm, I like it. I'll go ahead and pick "why" for now. If we want to be a little more explicit, we could call it e.g. rejected_by. I like discards too, nice to have consistent grammar.

✨

[MarkusPettersson98]

@MarkusPettersson98 made 1 comment.
Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @Serock3).

---

mullvad-management-interface/proto/relay_selector.proto line 72 at r4 (raw file):

}

message DiscardedRelay {

Code quote (i):

message DiscardedRelay {

Code snippet (ii):

// A relay which was discarded when evaluating some query, and why.
message DiscardedRelay {

[MarkusPettersson98]

@MarkusPettersson98 reviewed 1 file and all commit messages, and made 3 comments.
Reviewable status: 2 of 3 files reviewed, 6 unresolved discussions (waiting on @Serock3).

---

mullvad-management-interface/proto/relay_selector.proto line 20 at r5 (raw file):

  // Also returns a list of non-matching relays along with the set of
  // constraints/conditions that made them unavailable.
  rpc GetMatchingRelays(Query) returns (FilteredRelays);

⛏️ Can we just shorten FilteredRelays to Relays? Is that too minimalist?

Code quote:

 rpc GetMatchingRelays(Query) returns (FilteredRelays);

---

mullvad-management-interface/proto/relay_selector.proto line 20 at r5 (raw file):

  // Also returns a list of non-matching relays along with the set of
  // constraints/conditions that made them unavailable.
  rpc GetMatchingRelays(Query) returns (FilteredRelays);

⛏️ I suppose the function name GetMatchingRelays is a bit misleading - we do return discarded (non-matching) relays as well.

Code quote:

rpc GetMatchingRelays(Query) returns (FilteredRelays);

---

mullvad-management-interface/proto/relay_selector.proto line 29 at r5 (raw file):

// behavior.
message Query {
  oneof query_context {

⛏️ Is either context or case too minimalist? I do think query_ is implied from the message's name.

Code quote:

oneof query_context

[MarkusPettersson98]

@MarkusPettersson98 resolved 1 discussion.
Reviewable status: 2 of 3 files reviewed, 5 unresolved discussions (waiting on @Serock3).

[MarkusPettersson98]

@MarkusPettersson98 made 1 comment.
Reviewable status: 2 of 3 files reviewed, 6 unresolved discussions (waiting on @Serock3).

---

mullvad-management-interface/proto/relay_selector.proto line 94 at r5 (raw file):

// Set of constraints that prevents the relay from being selected
message NonMatchingConstraints {

Is incompatible a better term than non-matching? I.e. message IncompatibleConstraints(or perhapsincompatibilities` if one feel inclined)

Code quote:

message NonMatchingConstraints {

[MarkusPettersson98]

@MarkusPettersson98 reviewed 1 file.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @Serock3).

[Serock3]

@Serock3 resolved 6 discussions.
Reviewable status: 2 of 3 files reviewed, all discussions resolved (waiting on @MarkusPettersson98).

[MarkusPettersson98]

@MarkusPettersson98 reviewed 1 file and all commit messages, and made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved.

[MarkusPettersson98]

@MarkusPettersson98 made 1 comment.
Reviewable status: complete! all files reviewed, all discussions resolved.