Move api.rs from mullvad-daemon to mullvad-api

[buggmagnet]

This PR does the following things:

• Move most the api.rs file from the mullvad-daemon folder to the mullvad-api folder
• Keep the firewall related functionalities in the daemon crate by creating a new trait called AllowedClientsProvider
• Fix building the relay_list example when building for Android (mostly replacing it by an empty binary)
• Fix related compilation errors that occurred as a result of moving files around

The work is done as an initiative from the iOS team to reuse more rust code, and thus we identified that moving most of the api functionalities from the daemon crate to the api crate was the next step in that direction.

This work was co-authored by @pinkisemils, @dlon and @buggmagnet

---

This change is 

[Pururun]

Reviewed 10 of 10 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[dlon]

Reviewable status: 10 of 11 files reviewed, 1 unresolved discussion

---

mullvad-api/src/api.rs line 626 at r2 (raw file):

                AccessMethod::BuiltIn(BuiltInAccessMethod::Direct) => ApiConnectionMode::Direct,
                AccessMethod::BuiltIn(BuiltInAccessMethod::Bridge) => {
                    let Some(bridge) = relay_selector.get_bridge_forced() else {

Should this be provided by a trait rather than a RelaySelector?

[buggmagnet]

Reviewable status: 10 of 11 files reviewed, 1 unresolved discussion (waiting on @dlon)

---

mullvad-api/src/api.rs line 626 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should this be provided by a trait rather than a RelaySelector?

If we end up using this code path, we will likely do that yes

[MarkusPettersson98]

Reviewed 4 of 10 files at r1, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @dlon)

---

mullvad-daemon/src/api.rs line 37 at r3 (raw file):

#[derive(Clone, Copy)]
pub struct AllowedClientsSelector {}

⛏️ IMO, public structs should be declared before crate-visible functions (i.e. create_bypass_tx in this case) 😊

Code quote:

pub struct AllowedClientsSelector {}

---

mullvad-daemon/src/access_method.rs line 157 at r3 (raw file):

    pub(crate) async fn test_access_method(
        proxy: talpid_types::net::AllowedEndpoint,
        access_method_selector: mullvad_api::api::AccessModeSelectorHandle,

⛏️ The api module is already in scope here

api::AccessModeSelectorHandle

Code quote:

mullvad_api::api::AccessModeSelectorHandle

[buggmagnet]

Reviewable status: 7 of 11 files reviewed, 1 unresolved discussion (waiting on @dlon, @MarkusPettersson98, and @Pururun)

---

mullvad-daemon/src/api.rs line 37 at r3 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

⛏️ IMO, public structs should be declared before crate-visible functions (i.e. create_bypass_tx in this case) 😊

Done

[pinkisemils]

Reviewed 1 of 4 files at r4, all commit messages.
Reviewable status: 8 of 11 files reviewed, 1 unresolved discussion (waiting on @buggmagnet, @dlon, @MarkusPettersson98, and @Pururun)

---

mullvad-api/src/api.rs line 626 at r2 (raw file):

Previously, buggmagnet wrote…

If we end up using this code path, we will likely do that yes

I think we should do it in this PR - this would allow the mullvad-api crate to not depend on mullvad-relay-selector.

[dlon]

Reviewed 7 of 10 files at r1, 4 of 4 files at r4, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions

---

mullvad-api/src/api.rs line 448 at r4 (raw file):

        let connection_mode = resolved.connection_mode.clone();
        tokio::spawn(async move {
            if connection_mode.save(&cache_dir).await.is_err() {

This might be something that should not be handled here either. Will you want/be able to persist the connection mode on iOS?

---

mullvad-daemon/src/access_method.rs line 158 at r4 (raw file):

        access_method_selector: api::AccessModeSelectorHandle,
        daemon_event_sender: crate::DaemonEventSender<(
            mullvad_api::api::AccessMethodEvent,

You do not have to prefix api with mullvad_api:: here

---

mullvad-daemon/src/access_method.rs line 168 at r4 (raw file):

            .map(|connection_mode| connection_mode.endpoint)?;

        mullvad_api::api::AccessMethodEvent::Allow { endpoint: proxy }

You do not have to prefix api with mullvad_api:: here

---

mullvad-daemon/src/access_method.rs line 174 at r4 (raw file):

        let result = Self::perform_api_request(api_proxy).await;

        mullvad_api::api::AccessMethodEvent::Allow { endpoint: reset }

You do not have to prefix api with mullvad_api:: here

---

mullvad-api/src/bin/relay_list.rs line 5 at r4 (raw file):

//! relay list at the time of creating the installer.

#[cfg(not(target_os = "android"))]

These changes seem unnecessary/irrelevant here

[dlon]

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @buggmagnet)

---

mullvad-daemon/src/lib.rs line 584 at r4 (raw file):

        let (tx, mut rx) = mpsc::unbounded::<T>();
        let sender = self.sender.clone();
        tokio::runtime::Handle::current().spawn(async move {

You should be able to use tokio::spawn here

[pinkisemils]

Reviewed 3 of 10 files at r1, 1 of 4 files at r4.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @buggmagnet)

---

mullvad-api/src/proxy.rs line 57 at r4 (raw file):

pub trait AllowedClientsProvider: Send + Sync {
    fn allowed_clients(&self, connection_mode: &ApiConnectionMode) -> AllowedClients;

Is there a usecase where this trait must have some state associated with it? Would we need self on iOS?
Otherwise, these trait methods could be freestanding, i.e., not take &self.

---

mullvad-daemon/src/lib.rs line 711 at r4 (raw file):

        let allowed_clients_selector = AllowedClientsSelector {};
        let selector_box: Box<dyn AllowedClientsProvider> = Box::new(allowed_clients_selector);

It doesn't have to be called selector_box, it could just be selector.

[dlon]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @buggmagnet)

---

mullvad-daemon/src/lib.rs line 576 at r4 (raw file):

    InternalDaemonEvent: From<E>,
{
    pub fn to_unbounded_sender<T>(&self) -> mpsc::UnboundedSender<T>

I suspect that we could avoid this additional channel by passing in some Sender trait to mullvad-api instead of specifically an mpsc channel. We already have such a trait in talpid_core::mpsc.

Won't ask you to do that here, though, because it would likely be a bit painful to refactor.

---

mullvad-api/src/api.rs line 1 at r4 (raw file):

//! This module is responsible for enabling custom [`AccessMethodSetting`]s to

Should we rename this to access_method.rs, or maybe access_mode.rs? api is overly generic, especially now that it lives in mullvad-api.

[buggmagnet]

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

---

mullvad-api/src/proxy.rs line 57 at r4 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Is there a usecase where this trait must have some state associated with it? Would we need self on iOS?
Otherwise, these trait methods could be freestanding, i.e., not take &self.

It's here so that the trait is dyn compatible

Is there another way to just work with an interface where the implementation lies in a different crate ?

---

mullvad-api/src/bin/relay_list.rs line 5 at r4 (raw file):

Previously, dlon (David Lönnhager) wrote…

These changes seem unnecessary/irrelevant here

This was done so that mullvad-api can be built standalone when targeting the Android platform like so

cargo build -p mullvad-api --target aarch64-linux-android

I did this before I figured out how the Android team builds their part of the app, but it doesn't hurt to have this now.

---

mullvad-daemon/src/access_method.rs line 158 at r4 (raw file):

Previously, dlon (David Lönnhager) wrote…

You do not have to prefix api with mullvad_api:: here

Done

---

mullvad-daemon/src/access_method.rs line 168 at r4 (raw file):

Previously, dlon (David Lönnhager) wrote…

You do not have to prefix api with mullvad_api:: here

Done

---

mullvad-daemon/src/access_method.rs line 174 at r4 (raw file):

Previously, dlon (David Lönnhager) wrote…

You do not have to prefix api with mullvad_api:: here

Done

---

mullvad-daemon/src/lib.rs line 584 at r4 (raw file):

Previously, dlon (David Lönnhager) wrote…

You should be able to use tokio::spawn here

Done

---

mullvad-daemon/src/lib.rs line 711 at r4 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

It doesn't have to be called selector_box, it could just be selector.

Done

[buggmagnet]

Reviewable status: 9 of 11 files reviewed, 6 unresolved discussions (waiting on @dlon, @pinkisemils, and @Pururun)

---

mullvad-api/src/api.rs line 626 at r2 (raw file):

this would allow the mullvad-api crate to not depend on mullvad-relay-selector

Is that a long term goal ? Not that I'm against it, but since this work is already quite ad-hoc and making changes spanning multiple platforms, in my experience it's better to keep this from growing in scope, and address that in a future PR.

Otherwise, I'd rather pair up with someone if we want to do these changes in this PR

[pinkisemils]

Reviewed 1 of 10 files at r1.
Reviewable status: 9 of 11 files reviewed, 5 unresolved discussions (waiting on @buggmagnet, @dlon, and @Pururun)

---

mullvad-api/src/api.rs line 626 at r2 (raw file):

Previously, buggmagnet wrote…

this would allow the mullvad-api crate to not depend on mullvad-relay-selector

Is that a long term goal ? Not that I'm against it, but since this work is already quite ad-hoc and making changes spanning multiple platforms, in my experience it's better to keep this from growing in scope, and address that in a future PR.

Otherwise, I'd rather pair up with someone if we want to do these changes in this PR

That is a long term goal to not have everything depend on everything, and since we're factoring out this component out of the daemon, it stands to reason that some of it's dependencies should be injected rather than have them infect other crates. This is well within the scope of the changes here.

---

mullvad-api/src/proxy.rs line 57 at r4 (raw file):

Previously, buggmagnet wrote…

It's here so that the trait is dyn compatible

Is there another way to just work with an interface where the implementation lies in a different crate ?

You can make user of the trait be generic of the implementation.

[dlon]

Reviewed 2 of 2 files at r5, all commit messages.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @buggmagnet and @pinkisemils)

---

mullvad-api/src/api.rs line 626 at r2 (raw file):
I'm fine with this being done in a future PR in order to reduce the scope.

Is that a long term goal?

It's a (unstated) goal to keep unnecessary dependencies low, not introduce new ones unless necessary, and not have everything depend on everything else.

This could also be done by feature-gating api and making mullvad-relay-selector an optional dependency, I guess.

Since mullvad-relay-selector is small anyway, I personally don't think it's a big deal to depend on it here, but it would be good to avoid it. In future PRs, if not here.

---

mullvad-api/src/bin/relay_list.rs line 5 at r4 (raw file):

Previously, buggmagnet wrote…

This was done so that mullvad-api can be built standalone when targeting the Android platform like so

cargo build -p mullvad-api --target aarch64-linux-android

I did this before I figured out how the Android team builds their part of the app, but it doesn't hurt to have this now.

Thanks for explaining, sounds good!

I like to use this one trick to keep the number of cfg conditions low, but it's only a suggestion:

#[cfg(not(target_os = "android"))]
mod imp {
    // my imports for non-android

    pub async fn main() {
        // my real main func
    }
}

#[cfg(target_os = "android")]
mod imp {
    // no imports

    // empty main func
    pub async fn main() {}
}

#[tokio::main]
async fn main() {
    imp::main().await
}

[pinkisemils]

Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @buggmagnet)

---

mullvad-api/src/api.rs line 61 at r5 (raw file):

    /// duration of 1 API call). Since this is just an internal test which
    /// should be opaque to any client, it should not produce any unwanted noise
    /// and as such it is *not* broadcasted after the daemon is done processing

Not sure if the docs here should refer to a daemon anymore.

[buggmagnet]

Reviewable status: 10 of 11 files reviewed, 6 unresolved discussions (waiting on @dlon, @MarkusPettersson98, @pinkisemils, and @Pururun)

---

mullvad-api/src/bin/relay_list.rs line 5 at r4 (raw file):

Previously, dlon (David Lönnhager) wrote…

Thanks for explaining, sounds good!

I like to use this one trick to keep the number of cfg conditions low, but it's only a suggestion:

#[cfg(not(target_os = "android"))]
mod imp {
    // my imports for non-android

    pub async fn main() {
        // my real main func
    }
}

#[cfg(target_os = "android")]
mod imp {
    // no imports

    // empty main func
    pub async fn main() {}
}

#[tokio::main]
async fn main() {
    imp::main().await
}

Nice, thanks for the suggestion !

[buggmagnet]

Reviewable status: 8 of 11 files reviewed, 5 unresolved discussions (waiting on @dlon, @MarkusPettersson98, @pinkisemils, and @Pururun)

---

mullvad-api/src/proxy.rs line 57 at r4 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

You can make user of the trait be generic of the implementation.

Done

[buggmagnet]

Reviewable status: 7 of 13 files reviewed, 7 unresolved discussions (waiting on @dlon, @MarkusPettersson98, @pinkisemils, and @Pururun)

---

mullvad-api/src/proxy.rs line 57 at r4 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

If I read your question correctly @pinkisemils, do you suggest changing the AllowedClientsProvider trait to

pub trait AllowedClientsProvider: Send + Sync {
    fn allowed_clients(connection_mode: &ApiConnectionMode) -> AllowedClients;
}

?

Done

---

mullvad-api/src/access_mode.rs line 386 at r10 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

What even is this?

.enumerate()
.whatever(|(_, thing)| ...

throws away the index that enumerate adds. This can be simplified to find().

This was discussed offline, index is used just after.

---

mullvad-api/src/api.rs line 626 at r2 (raw file):

Previously, dlon (David Lönnhager) wrote…

I'm fine with this being done in a future PR in order to reduce the scope.

Is that a long term goal?

It's a (unstated) goal to keep unnecessary dependencies low, not introduce new ones unless necessary, and not have everything depend on everything else.

This could also be done by feature-gating api and making mullvad-relay-selector an optional dependency, I guess.

Since mullvad-relay-selector is small anyway, I personally don't think it's a big deal to depend on it here, but it would be good to avoid it. In future PRs, if not here.

This is not dependant on relay-selector anymore, we also removed the dependency to eDNS at the same time.

[buggmagnet]

Reviewable status: 7 of 13 files reviewed, 6 unresolved discussions (waiting on @dlon, @MarkusPettersson98, @pinkisemils, and @Pururun)

---

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

Previously, dlon (David Lönnhager) wrote…

Might be nicer to keep the old method in RelaySelector and call that from here, especially if the trait method is renamed to get_bridge.

Done

[dlon]

Reviewed 2 of 4 files at r9, 2 of 4 files at r11, 4 of 5 files at r12, 1 of 1 files at r13, all commit messages.
Reviewable status: 12 of 13 files reviewed, 6 unresolved discussions (waiting on @buggmagnet, @MarkusPettersson98, and @pinkisemils)

---

mullvad-api/src/proxy.rs line 57 at r13 (raw file):

pub trait AllowedClientsProvider: Send + Sync {
    fn allowed_clients(connection_mode: &ApiConnectionMode) -> AllowedClients;

Could potentially move this method to BridgeAndDNSProvider (or whatever that will be called).

---

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

/// [`WIREGUARD_RETRY_ORDER`] defines an ordered set of relay parameters which the relay selector
/// should should prioritize on successive connection attempts. Note that these will *never*

Nit: Could we not make unrelated formatting changes? ;_;

[buggmagnet]

Reviewable status: 12 of 13 files reviewed, 6 unresolved discussions (waiting on @dlon, @MarkusPettersson98, and @pinkisemils)

---

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

Previously, dlon (David Lönnhager) wrote…

Nit: Could we not make unrelated formatting changes? ;_;

Yes but god rustfmt.toml said

comment_width = 100

[buggmagnet]

Reviewable status: 12 of 13 files reviewed, 6 unresolved discussions (waiting on @dlon, @MarkusPettersson98, and @pinkisemils)

---

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

Previously, buggmagnet wrote…

Yes but god rustfmt.toml said

comment_width = 100

This was discussed offline, it turns out that this is ignored by the stable version of cargo fmt
and my IDE was configured to use the nightly version of cargo fmt
I will revert this change.

[buggmagnet]

Reviewable status: 8 of 13 files reviewed, 5 unresolved discussions (waiting on @dlon, @MarkusPettersson98, and @pinkisemils)

---

mullvad-api/src/proxy.rs line 57 at r13 (raw file):

Previously, dlon (David Lönnhager) wrote…

Could potentially move this method to BridgeAndDNSProvider (or whatever that will be called).

Done

[buggmagnet]

Reviewable status: 8 of 13 files reviewed, 4 unresolved discussions (waiting on @dlon, @MarkusPettersson98, and @pinkisemils)

---

mullvad-types/src/relay_list.rs line 256 at r10 (raw file):

Previously, dlon (David Lönnhager) wrote…

Maybe just get_bridge? forced only makes sense in a relay selector context (which should be hidden here).

Done

[buggmagnet]

Reviewable status: 8 of 13 files reviewed, 3 unresolved discussions (waiting on @dlon and @pinkisemils)

---

mullvad-daemon/src/lib.rs line 576 at r4 (raw file):

Previously, dlon (David Lönnhager) wrote…

You might get away with only providing a resolve method that turns some AccessMethod into an Option<ApiConnectionMode>

#[async_trait]
pub trait AccessModeAsdfghjklsomethingProvider: Clone {
    async fn resolve(&mut self, method: AccessMethodSetting) -> Option<ApiConnectionMode>;

    async fn on_event(&self, event: AccessMethodEvent) -> Result<(), Error>;
}

This would allow you to get rid of AllowedClientsProvider, EncryptedDnsProxyState (it would be owned by the one implementing the trait), and ShadowsocksBridgeProvider. Unless there's something I haven't thought of.

Done
We discussed offline the part about the event sender, and decided that we would not do it.

[buggmagnet]

Reviewable status: 8 of 13 files reviewed, 2 unresolved discussions (waiting on @dlon and @pinkisemils)

---

mullvad-api/src/api.rs line 448 at r4 (raw file):

Previously, dlon (David Lönnhager) wrote…

This might be something that should not be handled here either. Will you want/be able to persist the connection mode on iOS?

Done

[buggmagnet]

Reviewable status: 8 of 13 files reviewed, 2 unresolved discussions (waiting on @dlon and @pinkisemils)

---

mullvad-api/src/api.rs line 61 at r5 (raw file):

Previously, pinkisemils (Emīls Piņķis) wrote…

Not sure if the docs here should refer to a daemon anymore.

This was discussed offline, the conclusion is that this comment serves as a justification to why we are sending the event (to avoid deadlocking the daemon) therefore we are keeping this comment here.

[dlon]

Reviewed 1 of 1 files at r14, 2 of 4 files at r16, 2 of 2 files at r17, 1 of 1 files at r18, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pinkisemils)

[dlon]

Reviewed 2 of 2 files at r19, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @pinkisemils)

[pinkisemils]

Reviewed 1 of 4 files at r9, 2 of 5 files at r12, 1 of 1 files at r13, 1 of 1 files at r14, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

---

mullvad-daemon/src/lib.rs line 576 at r4 (raw file):

Previously, buggmagnet wrote…

Done
We discussed offline the part about the event sender, and decided that we would not do it.

Should we really not

[pinkisemils]

Reviewed 1 of 5 files at r12, 2 of 4 files at r16, 2 of 2 files at r19.
Reviewable status: complete! all files reviewed, all discussions resolved

[MarkusPettersson98]

Reviewed 1 of 1 files at r6, 2 of 4 files at r9, 3 of 5 files at r12, 1 of 1 files at r13, 1 of 1 files at r14, 2 of 4 files at r16, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved

[Pururun]

Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved