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

Remove duct from talpid-tunnel #7410

Merged
merged 2 commits into from
Jan 3, 2025
Merged

Remove duct from talpid-tunnel #7410

merged 2 commits into from
Jan 3, 2025

Conversation

MarkusPettersson98
Copy link
Contributor

@MarkusPettersson98 MarkusPettersson98 commented Jan 2, 2025

This PR removes duct from the talpid-tunnel crate. While doing so, I refactored some code that is responsible for configuring tun devices to use tun 0.7, up from the previous dependency on tun 0.5. This allowed me to remove spawning an ip subprocess on Linux and instead use syscalls (under the hood).


This change is Reviewable

@MarkusPettersson98 MarkusPettersson98 force-pushed the remove-duct branch 7 times, most recently from 074b592 to b64f26f Compare January 2, 2025 13:11
@MarkusPettersson98 MarkusPettersson98 force-pushed the remove-duct branch 3 times, most recently from 137c891 to ff82420 Compare January 2, 2025 13:43
@MarkusPettersson98 MarkusPettersson98 marked this pull request as ready for review January 2, 2025 13:48
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 3 of 4 files at r1, all commit messages.
Reviewable status: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @MarkusPettersson98)


talpid-tunnel/Cargo.toml line 30 at r1 (raw file):

[target.'cfg(any(target_os = "linux", target_os = "macos"))'.dependencies]
tun = "0.7"

Should nix be moved here too?


Cargo.lock line 4779 at r1 (raw file):

[[package]]
name = "thiserror"

Would it break anything to bump thiserror elsewhere?


talpid-tunnel/src/tun_provider/unix.rs line 94 at r1 (raw file):

impl UnixTun {
    /// Retrieve the tunnel interface name.
    pub fn interface_name(&self) -> Result<String, Error> {

mod.rs implies that Android and Linux/macOS implement a shared interface for Tun.

// macos and linux
type Tun = imp::UnixTun;
type TunProvider = imp::UnixTunProvider;

// android
type Tun = imp::VpnServiceTun;
type TunProvider = imp::AndroidTunProvider;

But there's nothing that enforces this. 😅

Do you think we could improve this?

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: 3 of 4 files reviewed, 3 unresolved discussions (waiting on @dlon)


Cargo.lock line 4779 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Would it break anything to bump thiserror elsewhere?

Do you mean in a separate PR? We should probably look at moving to thiserror 2 ourselves, but we will inevitably have both version 1 & 2 in our tree for the foreseeable future:/


talpid-tunnel/Cargo.toml line 30 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Should nix be moved here too?

Sure! I kind of didn't want to do it in this PR since it could easily be removed anyway: https://github.com/mullvad/mullvadvpn-app/pull/7411/files, but now I did it anyway:)


talpid-tunnel/src/tun_provider/unix.rs line 94 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

mod.rs implies that Android and Linux/macOS implement a shared interface for Tun.

// macos and linux
type Tun = imp::UnixTun;
type TunProvider = imp::UnixTunProvider;

// android
type Tun = imp::VpnServiceTun;
type TunProvider = imp::AndroidTunProvider;

But there's nothing that enforces this. 😅

Do you think we could improve this?

Sure, that sounds like a trait to me:)

dlon
dlon previously approved these changes Jan 3, 2025
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 4 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @MarkusPettersson98)


Cargo.lock line 4779 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Do you mean in a separate PR? We should probably look at moving to thiserror 2 ourselves, but we will inevitably have both version 1 & 2 in our tree for the foreseeable future:/

Since we can't get rid of version 1 anyway, nevermind. :(


talpid-tunnel/src/tun_provider/unix.rs line 94 at r1 (raw file):

Previously, MarkusPettersson98 (Markus Pettersson) wrote…

Sure, that sounds like a trait to me:)

At minimum, I think we should maintain the illusion (return a result on Android too). Don't feel obligated to refactor this more unless you want to.

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


Cargo.lock line 4779 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

Since we can't get rid of version 1 anyway, nevermind. :(

://


talpid-tunnel/src/tun_provider/unix.rs line 94 at r1 (raw file):

Previously, dlon (David Lönnhager) wrote…

At minimum, I think we should maintain the illusion (return a result on Android too). Don't feel obligated to refactor this more unless you want to.

Y, I'll mirror the APIs and doing the proper thingy will be left as an exercise for future me:)

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

@MarkusPettersson98 MarkusPettersson98 merged commit cbec676 into main Jan 3, 2025
56 checks passed
@MarkusPettersson98 MarkusPettersson98 deleted the remove-duct branch January 3, 2025 09:52
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.

2 participants