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

[dlon]

Reviewed 1 of 1 files at r6.
Reviewable status: 9 of 11 files reviewed, 6 unresolved discussions (waiting on @buggmagnet, @pinkisemils, and @Pururun)

---

mullvad-api/src/api.rs line 264 at r7 (raw file):

    /// `index` is used to keep track of the [`AccessMethodSetting`] to use.
    index: usize,
    provider: P,

If you want, you can get rid of the &self parameter. By replacing P with std::marker::PhantomData<P>, and simply calling P::allowed_clients(...)

[MarkusPettersson98]

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

---

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

Previously, buggmagnet wrote…

Done

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

?

[dlon]

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

---

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

Previously, dlon (David Lönnhager) wrote…

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.

(Suggestion, likely out of scope.)

Would it be too ugly to inject everything via a single trait to get rid of this, the relay selector, and allowed clients?

#[async_trait]
pub trait AccessModeAsdfghjklsomethingProvider: Clone {
    async fn allowed_clients() -> ...;

    async fn get_shadowsocks_bridge(&self) -> Option<Shadowsocks>;

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

mullvad-daemon could then just implement on_event as let _ = internal_tx.send(InternalDaemonEvent::AccessMethodEvent { ... });

[buggmagnet]

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

---

mullvad-api/src/api.rs line 264 at r7 (raw file):

Previously, dlon (David Lönnhager) wrote…

If you want, you can get rid of the &self parameter. By replacing P with std::marker::PhantomData<P>, and simply calling P::allowed_clients(...)

Done

Mostly thanks to @MarkusPettersson98's patch 🍻

[buggmagnet]

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

---

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

Previously, dlon (David Lönnhager) wrote…

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.

I went for access_mode since we already have several access_method.rs files

[buggmagnet]

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

---

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

Previously, dlon (David Lönnhager) wrote…

(Suggestion, likely out of scope.)

Would it be too ugly to inject everything via a single trait to get rid of this, the relay selector, and allowed clients?

#[async_trait]
pub trait AccessModeAsdfghjklsomethingProvider: Clone {
    async fn allowed_clients() -> ...;

    async fn get_shadowsocks_bridge(&self) -> Option<Shadowsocks>;

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

mullvad-daemon could then just implement on_event as let _ = internal_tx.send(InternalDaemonEvent::AccessMethodEvent { ... });

It's funny you mentioned that, while I was working on aea3c0792
@MarkusPettersson98 suggested that we could also remove the dependency on encrypted DNS Proxy by suggesting something similar to your suggestion.

I'll circle back to you soon after discussing this with @pinkisemils, but that makes sense to me too, and we could pair up on this I think.

[dlon]

Reviewed 1 of 4 files at r8, 1 of 4 files at r10.
Reviewable status: 7 of 13 files reviewed, 6 unresolved discussions (waiting on @buggmagnet, @MarkusPettersson98, @pinkisemils, and @Pururun)

---

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

Previously, buggmagnet wrote…

It's funny you mentioned that, while I was working on aea3c0792
@MarkusPettersson98 suggested that we could also remove the dependency on encrypted DNS Proxy by suggesting something similar to your suggestion.

I'll circle back to you soon after discussing this with @pinkisemils, but that makes sense to me too, and we could pair up on this I think.

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

#[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.

---

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

pub trait ShadowsocksBridgeProvider: Send + Sync {
    fn get_bridge_forced(&self) -> Option<Shadowsocks>;

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

---

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

    /// Returns a non-custom bridge based on the relay and bridge constraints, ignoring the bridge
    /// state.
    fn get_bridge_forced(&self) -> Option<Shadowsocks> {

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.

[pinkisemils]

Reviewed 1 of 1 files at r6, 2 of 4 files at r8, 1 of 4 files at r9.
Reviewable status: 9 of 13 files reviewed, 7 unresolved discussions (waiting on @buggmagnet, @dlon, @MarkusPettersson98, and @Pururun)

---

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

            .iter()
            .enumerate()
            .find(|(_, access_method)| access_method.get_id() == id)

What even is this?

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

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

[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