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

Implement daemon support for split tunneling for Android #6182

Conversation

MarkusPettersson98
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 commented Apr 25, 2024

This PR implements daemon support for split tunneling on Android.

The list of excluded apps is now handled by the daemon. The list of excluded apps is provided to the Android client via TunConfig, which has been updated slightly.


This change is Reviewable

Copy link

linear bot commented Apr 25, 2024

@MarkusPettersson98 MarkusPettersson98 force-pushed the implement-daemon-support-for-split-tunneling-for-android-des-746 branch 14 times, most recently from c524629 to ab17b2a Compare April 25, 2024 12:34
@MarkusPettersson98 MarkusPettersson98 marked this pull request as ready for review April 25, 2024 12:43
Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 5 files at r1, 18 of 18 files at r2, 18 of 18 files at r4, 1 of 1 files at r5, 2 of 2 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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.

:lgtm:

Reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)


mullvad-types/src/settings/mod.rs line 128 at r7 (raw file):

    /// # Note
    /// This function is fallible due to the Window's dito being fallible, and it is convenient to have the same API across all platforms.
    #[cfg(target_os = "android")]

Nit: Should we use two impl blocks since there's no overlap?

@MarkusPettersson98 MarkusPettersson98 force-pushed the implement-daemon-support-for-split-tunneling-for-android-des-746 branch from ab17b2a to 594fcfe Compare April 25, 2024 13:54
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: 21 of 22 files reviewed, 1 unresolved discussion (waiting on @dlon and @Pururun)


mullvad-types/src/settings/mod.rs line 128 at r7 (raw file):

Previously, dlon (David Lönnhager) wrote…

Nit: Should we use two impl blocks since there's no overlap?

Yes, that's a lot nicer! Thanks 😄

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 1 of 1 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Unify split tunnel code for Android and Windows
@MarkusPettersson98 MarkusPettersson98 force-pushed the implement-daemon-support-for-split-tunneling-for-android-des-746 branch from 29334f4 to 80b81de Compare April 29, 2024 09:02
Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 7 of 7 files at r9, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

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 7 of 7 files at r9, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @MarkusPettersson98)


mullvad-daemon/src/migrations/mod.rs line 205 at r9 (raw file):

        settings,
        #[cfg(target_os = "android")]
        directories.map(|directories| v9::Directories {

Feels a bit excessive to define v9::Directories instead of just passing in Option<Directories> (or Option<&Path>).


mullvad-daemon/src/migrations/v9.rs line 34 at r9 (raw file):

// ======================================================

/// This is an open migration

This isn't the case at the moment (but maybe it should be).


mullvad-daemon/src/migrations/v9.rs line 67 at r9 (raw file):

    }

    json_blob["settings_version"] = serde_json::json!(SettingsVersion::V10);

I suspect that we do not need to bump the version yet. Could we not just migrate the old settings whenever the new settings don't exist (and the old files do) and leave this as an "open" migration?


mullvad-daemon/src/migrations/v9.rs line 97 at r9 (raw file):

/// Read the existing split-tunneling settings which the Android client has kept track off and
/// write them to the settings object.
#[cfg(target_os = "android")]

Would it be cleaner to move these into mod android { ... }?


mullvad-daemon/src/migrations/v9.rs line 129 at r9 (raw file):

    ensure_split_tunnel_subkey(settings);
    // Store the read split tunnel state in the daemon's settings
    settings["split_tunnel"] = json!({ "enable_exclusions": enabled, "apps": apps });

You can probably get rid of ensure_split_tunnel_subkey by replacing this with something like

settings.insert("split_tunnel".to_string(), serde_json::Value::Object(json!( ... )));

or

settings.insert("split_tunnel".to_string(), json!( ... ).into());

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, 3 unresolved discussions (waiting on @dlon)


mullvad-daemon/src/migrations/mod.rs line 205 at r9 (raw file):

Previously, dlon (David Lönnhager) wrote…

Feels a bit excessive to define v9::Directories instead of just passing in Option<Directories> (or Option<&Path>).

We can't pass in Directories, as that would imply a cyclical dependency between v9.rs and mod.rs. Also, we should be able to modify Directories without potentially breaking the migration defined in v9.rs 😊

I don't have a strong opinion on whether to pass down settings_dir: Option<&Path> or directories: Option<Directories>, but the latter is a bit more explicit at the call site imho. The best thing would probably be to accept impl Into<V9::Directories>, but I didn't care enough to implement From<Directories> for <V9::Directories> 😊


mullvad-daemon/src/migrations/v9.rs line 34 at r9 (raw file):

Previously, dlon (David Lönnhager) wrote…

This isn't the case at the moment (but maybe it should be).

As stated in another comment: It is probably nice to keep this migration open for now - updated the code to allow us to support that properly 😊


mullvad-daemon/src/migrations/v9.rs line 67 at r9 (raw file):

Previously, dlon (David Lönnhager) wrote…

I suspect that we do not need to bump the version yet. Could we not just migrate the old settings whenever the new settings don't exist (and the old files do) and leave this as an "open" migration?

Sure, I'm open to keep this migration open 😉


mullvad-daemon/src/migrations/v9.rs line 97 at r9 (raw file):

Previously, dlon (David Lönnhager) wrote…

Would it be cleaner to move these into mod android { ... }?

Maybe, lot's of #[cfg(..)]action going on atm 😅

I went ahead and did it 😊


mullvad-daemon/src/migrations/v9.rs line 129 at r9 (raw file):

Previously, dlon (David Lönnhager) wrote…

You can probably get rid of ensure_split_tunnel_subkey by replacing this with something like

settings.insert("split_tunnel".to_string(), serde_json::Value::Object(json!( ... )));

or

settings.insert("split_tunnel".to_string(), json!( ... ).into());

Got rid of ensure_split_tunnel_key and simply used settings.insert 😊

@MarkusPettersson98 MarkusPettersson98 force-pushed the implement-daemon-support-for-split-tunneling-for-android-des-746 branch from 80b81de to 703cbc1 Compare April 29, 2024 11: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 1 of 1 files at r10, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved


mullvad-daemon/src/migrations/mod.rs line 205 at r9 (raw file):

we should be able to modify Directories without potentially breaking the migration defined in v9.rs 😊

Fair enough!

the latter is a bit more explicit at the call site imho

Is the parameter name not explicit enough, though? Anyway, this isn't important, so merge away.


mullvad-daemon/src/migrations/v9.rs line 67 at r9 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Sure, I'm open to keep this migration open 😉

Nice!

@MarkusPettersson98
Copy link
Contributor Author

Closing this PR - the has been cherry-picked into the base branch! :shipit:

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.

None yet

3 participants