Broadcast API access method changes #5605

MarkusPettersson98 · 2023-12-19T15:53:36Z

This PR changes how the AccessModeSelector interacts with the daemon. A new internal daemon (AccessMethodEvent) event has been added in order for the AccessModeSelector to properly be able to delegate the job of applying the correct firewall rules for any given access method. This allowed us to get rid of ApiEndpointUpdateCallback from the RequestService.

Fixes DES-521

This change is

linear · 2023-12-19T15:53:38Z

DES-521 Announce access method rotation to clients

Serock3

This is probably outside the scope of this PR, but the CLI for api-access could be improved. One issue I found is the error message when you try to remove the direct or mullvad bridge methods.

mullvad-api/src/rest.rs

Serock3

This is probably outside the scope of this PR, but the CLI for api-access could be improved. One issue I found is the error message when you try to remove the direct or mullvad bridge methods.

Also the list command should probably give information about which API access method is currently active.

MarkusPettersson98

I agree, the UX here could be improved. Let's create a separate issue for that 😊

Reviewable status: 0 of 16 files reviewed, 1 unresolved discussion (waiting on @Serock3)

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

Previously, Serock3 (Sebastian Holmin) wrote…

Why is the log level a warning? Is retrieving the next API connection mode not hte intended behaviour?

It is, the correct log level should be info. Well spotted!

Serock3 · 2023-12-21T14:21:32Z

mullvad-daemon/src/api.rs

+#[derive(Clone)]
+pub struct ResolvedConnectionMode {
+    /// The connection strategy to be used by the `mullvad-api` crate when
+    /// initialzing API requests.


Suggested change

/// initialzing API requests.

/// initializing API requests.

Serock3 · 2023-12-21T14:25:23Z

mullvad-daemon/src/api.rs

+/// * When a [`mullvad_api::rest::RequestService`] requests a new
+/// [`ApiConnectionMode`] from the running [`AccessModeSelector`].
+///
+/// * When testing some [`AccessMethodSetting`] to see if can be used to


Suggested change

/// * When testing some [`AccessMethodSetting`] to see if can be used to

/// * When testing some [`AccessMethodSetting`] to see if it can be used to

MarkusPettersson98

Reviewable status: 0 of 16 files reviewed, 3 unresolved discussions (waiting on @Serock3)

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

Previously, Serock3 (Sebastian Holmin) wrote…

/// * When testing some [`AccessMethodSetting`] to see if it can be used to

done

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

Previously, Serock3 (Sebastian Holmin) wrote…

    /// initializing API requests.

done

MarkusPettersson98 · 2023-12-22T12:47:47Z

I rebased this PR on top of #5617, which should be prioritized.

Serock3

Great work! Although I cannot say I fully understand all the complexities of this change, I will start by given some minor comments with thoughts and suggestions that I picked up along the way of reviewing.

You can ignore most of the suggested changes if you want, they were mostly a way for me to figure out what the code was doing by toying around with it.

mullvad-daemon/src/access_method.rs

Serock3 · 2024-01-03T10:06:08Z

mullvad-daemon/src/lib.rs

+            log::debug!(
+                "API access method {method} {result}",
+                method = test_subject.setting.name,
+                result = if result.as_ref().is_ok_and(|is_true| *is_true) {
+                    "could successfully connect to the Mullvad API".to_string()
+                } else {
+                    "could not connect to the Mullvad API".to_string()
+                }
+            );
+


Suggested change

log::debug!(

"API access method {method} {result}",

method = test_subject.setting.name,

result = if result.as_ref().is_ok_and(|is_true| *is_true) {

"could successfully connect to the Mullvad API".to_string()

} else {

"could not connect to the Mullvad API".to_string()

}

);

log::debug!(

"API access method {method} {result}",

method = test_subject.setting.name,

result = match result {

Ok(true) => "could successfully connect to the Mullvad API",

_ => "could not connect to the Mullvad API",

}

);

Serock3 · 2024-01-03T10:21:08Z

mullvad-daemon/src/lib.rs

+        });
+    }
+
+    async fn test_api_access_method_inner(


What do you think of making this an inner function of on_test_api_access_method?

Serock3 · 2024-01-03T10:36:32Z

mullvad-daemon/src/api.rs

+/// A [`NewAccessMethodEvent`] is emitted when the active access method changes.
+/// Which access method that should be active at any given time is decided by
+/// the [`AccessModeSelector`] spawned when the daemon starts.
+///
+/// This event may eventually lead to a
+/// [`mullvad_management_interface::client::DaemonEvent::NewAccessMethod`] to be
+/// broadcasted to clients. The event is emitted in two scenarios:
+///
+/// * When a [`mullvad_api::rest::RequestService`] requests a new
+/// [`ApiConnectionMode`] from the running [`AccessModeSelector`].
+///
+/// * When testing some [`AccessMethodSetting`] to see if it can be used to
+/// successfully reach the Mullvad API. This will temporarily switch the
+/// currently active access method, but since this is just a test it should not
+/// produce any unwanted noise for clients.
+pub struct NewAccessMethodEvent {
+    /// The new active [`AccessMethodSetting`].


Nice to see some quality documentation! Just two thoughts:

Perhaps a link to crate::InternalDaemonEvent::AccessMethodEvent instead of NewAccessMethodEvent is more informative?

When testing an access method, does it emit a DaemonEvent::NewAccessMethod or not? Maybe the phrasing could be a bit clearer?

Serock3 · 2024-01-03T10:48:49Z

mullvad-daemon/src/api.rs

+    /// It is up to the daemon to actually allow traffic to/from
+    /// `api_endpoint` by updating the firewall. This `Sender` allows the
+    /// daemon to communicate when that action is done.
+    pub update_finished_tx: Option<oneshot::Sender<()>>,


Maybe this could have a clearer name? What do you think about endpoint_active_tx?

Serock3 · 2024-01-03T13:04:40Z

mullvad-daemon/src/api.rs

        let (cmd_tx, cmd_rx) = mpsc::unbounded();

-        let connection_modes = match ConnectionModesIterator::new(connection_modes) {
+        let mut connection_modes = match ConnectionModesIterator::new(connection_modes) {
            Ok(provider) => provider,
            Err(Error::NoAccessMethods) | Err(_) => {


Is this not just the same as any error variant? Could it be replaced with .unwrap_or_else?

let mut connection_modes = ConnectionModesIterator::new(connection_modes).unwrap_or_else(|_| { // No settings seem to have been found. Default to using the the // direct access method. let default = mullvad_types::access_method::Settings::direct(); ConnectionModesIterator::new(vec![default]).expect( "Failed to create the data structure responsible for managing access methods", ) });

Serock3 · 2024-01-03T13:09:33Z

mullvad-daemon/src/api.rs

            Message::Next(_) => f.write_str("Next"),
            Message::Update(_, _) => f.write_str("Update"),
+            Message::Resolve(_, _) => f.write_str("Resolve"),
        }


My rustfmt automatically makes this change whenever I format

Suggested change

Message::Next(_) => f.write_str("Next"),

Message::Update(_, _) => f.write_str("Update"),

Message::Resolve(_, _) => f.write_str("Resolve"),

}

Message::Set(..) => f.write_str("Set"),

Message::Next(_) => f.write_str("Next"),

Message::Update(..) => f.write_str("Update"),

Message::Resolve(..) => f.write_str("Resolve"),

Serock3 · 2024-01-03T13:23:30Z

mullvad-daemon/src/api.rs

+        let next = {
+            let resolved = self.resolve(access_method).await;
+            // Note: If the daemon is busy waiting for a call to this function
+            // to complete while we wait for the daemon to fully handle this
+            // `NewAccessMethodEvent`, then we find ourselves in a deadlock.
+            // This can happen during daemon startup when spawning a new
+            // `MullvadRestHandle`, which will call and await `next` on a Stream
+            // created from this `AccessModeSelector` instance. As such, the
+            // completion channel is discarded in this instance.
+            let _completion =
+                NewAccessMethodEvent::new(resolved.setting.clone(), resolved.endpoint.clone())
+                    .send(&self.access_method_event_sender);
+            self.current = resolved.clone();
+            resolved
+        };
+
        // Save the new connection mode to cache!
        {
            let cache_dir = self.cache_dir.clone();
-            let next = next.clone();
+            let new_connection_mode = next.connection_mode.clone();
            tokio::spawn(async move {
-                if next.save(&cache_dir).await.is_err() {
+                if new_connection_mode.save(&cache_dir).await.is_err() {
                    log::warn!(
                        "Failed to save {connection_mode} to cache",
-                        connection_mode = next
+                        connection_mode = new_connection_mode
                    )
                }
            });
        }
-        self.reply(tx, next)
-    }
-
-    fn next_connection_mode(&mut self) -> ApiConnectionMode {
-        let access_method = self
-            .connection_modes
-            .next()
-            .map(|access_method_setting| access_method_setting.access_method)
-            .unwrap_or(AccessMethod::from(BuiltInAccessMethod::Direct));
-
-        let connection_mode = self.from(access_method);
-        log::info!("New API connection mode selected: {connection_mode}");
-        connection_mode
+        Ok(next.connection_mode)


Small nit, remove unneeded block

let resolved = self.resolve(access_method).await; // Note: If the daemon is busy waiting for a call to this function // to complete while we wait for the daemon to fully handle this // `NewAccessMethodEvent`, then we find ourselves in a deadlock. // This can happen during daemon startup when spawning a new // `MullvadRestHandle`, which will call and await `next` on a Stream // created from this `AccessModeSelector` instance. As such, the // completion channel is discarded in this instance. let _completion = NewAccessMethodEvent::new(resolved.setting.clone(), resolved.endpoint.clone()) .send(&self.access_method_event_sender); // Save the new connection mode to cache! { let cache_dir = self.cache_dir.clone(); let new_connection_mode = resolved.connection_mode.clone(); tokio::spawn(async move { if new_connection_mode.save(&cache_dir).await.is_err() { log::warn!( "Failed to save {connection_mode} to cache", connection_mode = new_connection_mode ) } }); } self.current = resolved; Ok(self.current.connection_mode.clone())

mullvad-api/src/rest.rs

mullvad-daemon/src/access_method.rs

MarkusPettersson98

Reviewable status: 0 of 17 files reviewed, 16 unresolved discussions (waiting on @Serock3)

mullvad-api/src/rest.rs line 122 at r12 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

The mullvad_api::ApiEndpointUpdateCallback trait doesn't seem to be used at all after this change. Perhaps it can be removed competely?

Nice catch! It is definitely not needed anymore, so I will go ahead and remove it immediately 😊

mullvad-daemon/src/access_method.rs line 0 at r12 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

(Reviewable was unable to map this GitHub inline comment thread to the right spot — sorry!)

I suspect that the removal of this function belongs to the previous commit.

You would be correct 😊

mullvad-daemon/src/access_method.rs line 83 at r12 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Is it possible fors only one endpoint to be available? What Happends then? Does the force_api_endpoint_rotation fn return an error or does the currently active on get removed anyway?

Yes, there might be only one available API endpoint if only one access method is enabled. Calling force_api_endpoint_rotation in this state will perform a 'rotation' of the active API endpoint, but the active API endpoint will never actually change. Also, there can only every be one valid endpoint at any given time, due to how we apply firewall rules. 😊

Technically force_api_endpoint_rotation returns an error if the RequestService service goes down for some reason, but it won't return an error in case there is no next API endpoint. 😊

mullvad-daemon/src/api.rs line 50 at r12 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Nice to see some quality documentation! Just two thoughts:

Perhaps a link to crate::InternalDaemonEvent::AccessMethodEvent instead of NewAccessMethodEvent is more informative?

When testing an access method, does it emit a DaemonEvent::NewAccessMethod or not? Maybe the phrasing could be a bit clearer?

Tried to clarify the doc comment by linking to crate::InternalDaemonEvent::AccessMethodEvent and re-phrasing the bit about testing.

mullvad-daemon/src/api.rs line 62 at r12 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Maybe this could have a clearer name? What do you think about endpoint_active_tx?

Sure, the update prefix is a bit generic in this case. 😊

mullvad-daemon/src/api.rs line 106 at r12 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

This functions return type is a bit confusing. Returning a oneshot::Receiver make it behave almost as if it was async, but with eager execution and no requirement of being awaited. Maybe it could be refactored to just be a normal async function?

The oneshot::Receiver does not always have to be awaited, which is why I chose to return it instead. But to avoid future confussion I will turn this function async, and callers who don't care to await the result can simply move the call inside a new async task with tokio::spawn 😊

mullvad-daemon/src/api.rs line 147 at r12 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

My rustfmt automatically makes this change whenever I format

            Message::Set(..) => f.write_str("Set"),
            Message::Next(_) => f.write_str("Next"),
            Message::Update(..) => f.write_str("Update"),
            Message::Resolve(..) => f.write_str("Resolve"),

How strange that mine didn't .. Updated the formatting manually now, nice catch! 😊

mullvad-daemon/src/api.rs line 274 at r12 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

let mut connection_modes =
    ConnectionModesIterator::new(connection_modes).unwrap_or_else(|_| {
        // No settings seem to have been found. Default to using the the
        // direct access method.
        let default = mullvad_types::access_method::Settings::direct();
        ConnectionModesIterator::new(vec![default]).expect(
            "Failed to create the data structure responsible for managing access methods",
        )
    });

Yes, it is and it can be replaced with that expression! Nice one 😊

mullvad-daemon/src/api.rs line 391 at r12 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Small nit, remove unneeded block

        let resolved = self.resolve(access_method).await;
        // Note: If the daemon is busy waiting for a call to this function
        // to complete while we wait for the daemon to fully handle this
        // `NewAccessMethodEvent`, then we find ourselves in a deadlock.
        // This can happen during daemon startup when spawning a new
        // `MullvadRestHandle`, which will call and await `next` on a Stream
        // created from this `AccessModeSelector` instance. As such, the
        // completion channel is discarded in this instance.
        let _completion =
            NewAccessMethodEvent::new(resolved.setting.clone(), resolved.endpoint.clone())
                .send(&self.access_method_event_sender);

        // Save the new connection mode to cache!
        {
            let cache_dir = self.cache_dir.clone();
            let new_connection_mode = resolved.connection_mode.clone();
            tokio::spawn(async move {
                if new_connection_mode.save(&cache_dir).await.is_err() {
                    log::warn!(
                        "Failed to save {connection_mode} to cache",
                        connection_mode = new_connection_mode
                    )
                }
            });
        }
        self.current = resolved;
        Ok(self.current.connection_mode.clone())

done

mullvad-daemon/src/api.rs line 438 at r12 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

The link to ApiConnectionModeProvider doesn't work. Has the type been removed?

The comment linked to the wrong datatype, it should be RelaySelector. Fixed!

mullvad-daemon/src/api.rs line 472 at r12 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

Super small suggestion: maybe replace nested match with this?

        AccessMethod::Custom(CustomAccessMethod::Shadowsocks(shadowsocks)) => {
            ApiConnectionMode::Proxied(ProxyConfig::Shadowsocks(shadowsocks))
        }
        AccessMethod::Custom(CustomAccessMethod::Socks5(socks)) => {
            ApiConnectionMode::Proxied(ProxyConfig::Socks(socks))
        }

Nice suggestion! I managed to flatten the entire function even more by implementing From<proxy::CustomProxy> for ProxyConfig 😊

mullvad-daemon/src/lib.rs line 2487 at r12 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

            log::debug!(
                "API access method {method} {result}",
                method = test_subject.setting.name,
                result = match result {  
                    Ok(true) => "could successfully connect to the Mullvad API",  
                    _ => "could not connect to the Mullvad API",  
                }  
            );

Done.

mullvad-daemon/src/lib.rs line 2492 at r12 (raw file):

Previously, Serock3 (Sebastian Holmin) wrote…

What do you think of making this an inner function of on_test_api_access_method?

I think that it makes on_test_api_access_method really big. 😄

Add a new `InternalDaemonEvent` for announcing when the current API access method changes.

Previously, the `mullvad-api` would tell the `mullvad-daemon` that it wanted a new API endpoint by calling a certain callback (`ApiEndpointUpdateCallback`), which would asynchronously resolve a new API endpoint and tell the daemon to punch an appropriate hole in the firewall for that particular endpoint before the `mullvad-api` crate would consume it. The logic of the callback can be moved inside `AccessModeSelector`, which simplifies the contract between `mullvad-daemon` and `mullvad-api` somewhat.

Until now, `AccessModeSelector` has not been able to resolve API endpoints on it's own. This has happened at some later stage, for example in the `mullvad-api` crate. However, for testing the `Direct` access method, it is very useful to be able to resolve the actual endpoint without involving the daemon's "API runtime".

This commit implements the daemon logic for handling a `NewAccessMethodEvent`. Such an event occur when `AccessModeSelector` announces that a new access method is active, and it will cause the daemon to except some API endpoint in the firewall. It may conditionally broadcast the new access method to all clients.

Since the `AccessModeSelector` knows how to resolve endpoints on it's own, we no longer have to re-use the existing `MullvadRestHandle` from the daemon. Instead, we may spawn a completely new `ApiProxy` and except the appropriate endpoint in the firewall without affecting the running `MullvadRestHandle`.

MarkusPettersson98 requested a review from Serock3 December 19, 2023 15:53

MarkusPettersson98 force-pushed the broadcast-api-access-method-changes branch 2 times, most recently from c4ff630 to f78b40e Compare December 20, 2023 12:16

MarkusPettersson98 marked this pull request as ready for review December 21, 2023 10:13

Serock3 reviewed Dec 21, 2023

View reviewed changes

mullvad-api/src/rest.rs Outdated Show resolved Hide resolved

Serock3 reviewed Dec 21, 2023

View reviewed changes

This comment was marked as outdated.

Sign in to view

MarkusPettersson98 commented Dec 21, 2023

View reviewed changes

Serock3 reviewed Dec 21, 2023

View reviewed changes

MarkusPettersson98 commented Dec 21, 2023

View reviewed changes

MarkusPettersson98 force-pushed the broadcast-api-access-method-changes branch 5 times, most recently from 6a0d05f to c791bc4 Compare December 22, 2023 12:45

MarkusPettersson98 force-pushed the broadcast-api-access-method-changes branch 3 times, most recently from 11c3032 to 16ecc52 Compare December 22, 2023 15:10

MarkusPettersson98 force-pushed the broadcast-api-access-method-changes branch 5 times, most recently from af930ed to 117c935 Compare January 3, 2024 09:15

Serock3 reviewed Jan 3, 2024

View reviewed changes

MarkusPettersson98 force-pushed the broadcast-api-access-method-changes branch from 117c935 to 3a56175 Compare January 3, 2024 17:12

MarkusPettersson98 commented Jan 4, 2024

View reviewed changes

MarkusPettersson98 force-pushed the broadcast-api-access-method-changes branch from 3a56175 to 4186b9e Compare January 4, 2024 08:03

Serock3 approved these changes Jan 4, 2024

View reviewed changes

MarkusPettersson98 force-pushed the broadcast-api-access-method-changes branch 2 times, most recently from 7692344 to 2f918b7 Compare January 5, 2024 09:15

MarkusPettersson98 added 6 commits January 8, 2024 09:03

Add new internal daemon event AccessMethodEvent

10c990e

Add a new `InternalDaemonEvent` for announcing when the current API access method changes.

Cleanup daemon code related to access methods

22fa185

MarkusPettersson98 force-pushed the broadcast-api-access-method-changes branch from 2f918b7 to 32ba86f Compare January 8, 2024 08:05

MarkusPettersson98 merged commit 9d98bd3 into main Jan 8, 2024
40 of 41 checks passed

MarkusPettersson98 deleted the broadcast-api-access-method-changes branch January 8, 2024 08:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Broadcast API access method changes #5605

Broadcast API access method changes #5605

MarkusPettersson98 commented Dec 19, 2023 •

edited by faern

Loading

linear bot commented Dec 19, 2023

Serock3 left a comment

Serock3 left a comment

This comment was marked as outdated.

This comment was marked as outdated.

MarkusPettersson98 left a comment

Serock3 Dec 21, 2023

Serock3 Dec 21, 2023

MarkusPettersson98 left a comment

MarkusPettersson98 commented Dec 22, 2023

Serock3 left a comment •

edited

Loading

Serock3 Jan 3, 2024

Serock3 Jan 3, 2024

Serock3 Jan 3, 2024

Serock3 Jan 3, 2024

Serock3 Jan 3, 2024

Serock3 Jan 3, 2024

Serock3 Jan 3, 2024

Serock3 Jan 3, 2024

MarkusPettersson98 left a comment

	/// * When testing some [`AccessMethodSetting`] to see if can be used to
	/// * When testing some [`AccessMethodSetting`] to see if it can be used to

Broadcast API access method changes #5605

Broadcast API access method changes #5605

Conversation

MarkusPettersson98 commented Dec 19, 2023 • edited by faern Loading

linear bot commented Dec 19, 2023

Serock3 left a comment

Choose a reason for hiding this comment

Serock3 left a comment

Choose a reason for hiding this comment

This comment was marked as outdated.

This comment was marked as outdated.

MarkusPettersson98 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkusPettersson98 left a comment

Choose a reason for hiding this comment

MarkusPettersson98 commented Dec 22, 2023

Serock3 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MarkusPettersson98 left a comment

Choose a reason for hiding this comment

MarkusPettersson98 commented Dec 19, 2023 •

edited by faern

Loading

Serock3 left a comment •

edited

Loading