List Wireguard connection as possible VPN#244
List Wireguard connection as possible VPN#244tintou merged 3 commits intoelementary:masterfrom milouse:feat-support-wireguard
Conversation
|
Rebased on last master and fixed all compilation warnings (not due to this MR). |
milouse
changed the title
|
Flagging it as draft as I just discover an incompatibility bug when listing both OpenVPN and Wireguard connection: When activating OpenVPN connection, the whole panel freeze and make the session crash. |
|
Crash fixed, but another problem arised: current widget only list 1 "vpn" active connection at a time, when we can actually have several in parallel. I’ll try to fix that later. |
milouse
changed the title
|
Fixed done, now the popover allow the connection of several VPN or wireguard connection in parallel. This MR is open again for review if one is interested? |
|
Hey @milouse apologies for not looking at this sooner! It looks like this branch was touching a number of things outside of just adding wireguard support which probably made it more difficult for someone to review. For future branches try to leave things like cleaning up unrelated deprecation warnings etc for another branch 😀 If you're still interested in working on this, and want to resolve conflicts, I'd love to review it! I've been working recently on revamping VPNs and cleaning up some of the mess that was here so it should be much more straightforward to add wireguard support. Thanks again! |
|
Hi, I can take time to look again on this. I’ll rebase this branch on your last developments to take them into account. I agree this branch touch a lot of different thing, but from my point of view, that was required refactoring to improve maintenability of the whole "vpn+wireguard" set of feature. Not sure an MR just refactoring things out of the blue would have make sense for reviewer :/ |
|
Refactoring branches are always appreciated! The more things can be broken up into smaller chunks the easier they are to review quickly. You can always leave a description that indicates why a certain refactor makes sense for a future branch etc. I think we've all had that experience of knowing some old code needs to change so we can eventually add a new feature :) |
|
In the next months, I’ll take time to have a fresh new looks on this, so you can expect new MR / iteration on the subject. Have a good week ! :) |
|
@danirabbit It was finally way much easier than I thought to rebase this MR on master. This is now done, thus feel free to review when you can. I already use it without any trouble. |
| * implementation for the signal handlers. */ | ||
| var _connection = (NM.VpnConnection) ac; | ||
| secure = secure || (_connection.get_vpn_state () == NM.VpnConnectionState.ACTIVATED); | ||
| _connection.vpn_state_changed.disconnect (update_vpn_connection); |
There was a problem hiding this comment.
Are these disconnects helping? I feel like I am missing something here.
There was a problem hiding this comment.
Good question… I’m not sure how signaling really works internally. Are callbacks stored in a simple list or a set (i.e. if we connnect again and again the same callback, do we get a full list of duplicated handlers, or does the "connect" method already take care to discard existing handlers when registering? I cannot found anything about this in the documentation, that’s why I did the disconnection myself to ensure we keep only one callback at a time.
Actually, when I did my tests, I discover that without disconnecting previous handlers, I ended with a lot of parallel callback calls after some time (easily testable with a simple log output), that’s why I added this disconnect call before to avoid duplicate. But maybe I missed something.
If one has a good documentation entry about maintaining signal handlers, I’d appreciate!
There was a problem hiding this comment.
I think @Marukesu has made good suggestions for documentation before on the Slack. Maybe he has insight with this?
|
(just rebased on master) |
|
I have tested this on Ubuntu 22.04 with elementary Desktop installed (and will on elementary too, and update here accordingly) procedure to install: git clone https://github.com/milouse/wingpanel-indicator-network
cd wingpanel-indicator-network
git switch feat-support-wireguard
meson build --prefix=/usr
cd build
ninja
sudo cp libnetwork.so /usr/lib/x86_64-linux-gnu/wingpanel/libnetwork.so
sudo chmod 644 /usr/lib/x86_64-linux-gnu/wingpanel/libnetwork.so
sudo chown root:root /usr/lib/x86_64-linux-gnu/wingpanel/libnetwork.so
killall io.elementary.wingpanel # to make the indicator reloadany wireguard connection you wish to manage this way should be set up using On Ubuntu with elementary Desktop: It works very well, Only there's a very generic looking network notification coming up when connecting or disconnecting for the wg interface. On elementary OS: This works even better. When disconnecting the annoying generic looking notification does not pop up (meaning that's probably an ubuntu thing I haven't gotten cleaned up on the other setup.) |
Error message is: io.elementary.w[33492]: DisplayWidget.vala:152: Unknown network state, cannot show the good icon: NETWORK_STATE_FAILED
zeebok
left a comment
zeebok
left a comment
There was a problem hiding this comment.
I think the code looks decent. And Rasmus seemed to do good testing and seems to think it is in good shape! Thanks for the work everybody!
|
Before I merge into main, @milouse are there any more updates to get merged into this or the other repo? |
|
Works for me! |
|
Sorry I was in holiday. Glad it’s finally merged :) Hope it will help lot of people :) Thank you all for reviewing/testing! |
5 participants
This MR comes as a complementary work of elementary/settings-network#338 to make wireguard connections first class citizens alongside more classical VPN.
Warning, in order to build this, you need an up-to-date version of libnm (myself I have 1.32.12 installed). The packaged libnm.vapi is outdated, that’s why I take the liberty to remove it from this repository. Even after removing it, the project build just fine.
This MR fixes #245