feat(client/linux): revamp the Linux VPN routing logic #2291
Conversation
jyyi1
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
| @@ -20,7 +20,7 @@ | |||
| "deb": { | |||
| "depends": [ | |||
| "gconf2", "gconf-service", "libnotify4", "libappindicator1", "libxtst6", "libnss3", | |||
There was a problem hiding this comment.
"gconf2", "gconf-service", "libappindicator1" do not exist on Ubuntu 24's apt repositories any more. After removal these dependencies, the app still runs. Maybe we can remove these? (even though electron declares they are required)
There was a problem hiding this comment.
Maybe they are required on newer versions?
I'd say that if it works, let's remove, especially if it blocks support on Ubuntu 24
There was a problem hiding this comment.
Are they needed for earlier Ubuntu versions?
There was a problem hiding this comment.
From my experiment, nope. App installs and runs.
There was a problem hiding this comment.
Gemini told me:
gconf2 and gconf-service: These were core parts of GNOME 2. GNOME 3, with its dconf configuration system, became the default in Ubuntu 11.10 (Oneiric Ocelot) released in October 2011. While gconf2 stuck around for backward compatibility, this marked the start of its decline.
libappindicator1: This was tied to the older system tray design. Ubuntu's switch to the Ayatana indicator system began around Ubuntu 10.04 LTS (Lucid Lynx) in April 2010 and continued to evolve in subsequent releases.
Key takeaway: While these packages might still be available in later Ubuntu versions, their core functionalities were effectively superseded around Ubuntu 11.10. You'll mainly encounter them if you're dealing with older applications designed for GNOME 2 or early implementations of the system tray.
| canHandleUDP = true | ||
| } | ||
|
|
||
| if d.pkt == nil { |
There was a problem hiding this comment.
The truncate approach requires an additional exchange, though it's all local, so it shouldn't matter much
client/go/outline/device_linux.go
Outdated
| }) | ||
| } | ||
|
|
||
| tcp := defaultBaseTCPDialer() |
There was a problem hiding this comment.
This is modifying the base dialer. Imutability > Mutability. They should remain unmodified after creation, this kind of side-effect is hard to track.
I think it will be cleaner if you have a newClientWithFwmark(transport, fwmark), and create the base dialers there.
There was a problem hiding this comment.
This also avoids tying the interface with LinuxOpts. I'd say delete DeviceOpts for now.
There was a problem hiding this comment.
Removed defaultBaseTCPDialer and defaultBaseUDPDialer.
| return nil, errIllegalConfig("must provide a valid TUN interface IP(v4)") | ||
| } | ||
| for _, dns := range conf.DNSServers { | ||
| dnsIP := net.ParseIP(dns).To4() |
There was a problem hiding this comment.
Why convert to IPv4? I'd keep them as they are.
There was a problem hiding this comment.
See the comment below.
| gonm "github.com/Wifx/gonetworkmanager/v2" | ||
| ) | ||
|
|
||
| type linuxVPNConn struct { |
There was a problem hiding this comment.
This type is doing a lot. See the other comments. We need to clarify side-effects and some method are better off being standalone.
There was a problem hiding this comment.
It's also not super clear what this type represents, perhaps some comments will help.
There was a problem hiding this comment.
Added some comments. This is the Linux implementation of VPNConnection. We used Linux-only APIs (which is not compilable on when targeting Windows).
But I may move some common logic (for example, the WaitGroups, and the traffic copying goroutines) to the vpn.go file, because these can be shared between Windows and Linux, but still keep the Linux specific calls in this vpn_linux.go file. Though this move might be done in the next PR (when introducing the callback, because the callback of status change logic is shared).
|
@fortuna I cleaned up some the code in the |
client/go/outline/dialer.go
Outdated
| ) | ||
|
|
||
| // newTCPDialer creates a default base TCP dialer for [Client]. | ||
| func newTCPDialer() net.Dialer { |
There was a problem hiding this comment.
Let's just inline these two functions in the caller.
client/go/outline/vpn/device.go
Outdated
|
|
||
| // RemoteDevice is an IPDevice that connects to a remote Outline server. | ||
| type RemoteDevice struct { | ||
| network.IPDevice |
There was a problem hiding this comment.
I wonder if we should just use io.ReadWriterCloser for now and rethink if we need the IPDevice abstraction.
This PR simplifies the Linux VPN routing architecture by:
This approach minimizes changes to the user's network configuration and improves overall stability.
Next steps: See #2291 (comment)
Tested on Ubuntu 22 and Ubuntu 24
Known Issue: The server connection status is not refreshed when network changes.
TODO: #2312