Alexays / Waybar

Highly customizable Wayland bar for Sway and Wlroots based compositors. :v: :tada:
MIT License
6.39k stars 696 forks source link

Support interface (white)list in Network module #784

Open vmsh0 opened 4 years ago

vmsh0 commented 4 years ago

I would like to have support for an interface whitelist (which would also work as a hierarchy for preferred interfaces) for the interface option in the Network module.

My use case is that if I switch network connections while I have an active VPN interface (e.g. tun0 from OpenVPN) waybar switches to it and keeps showing it until the corresponding interface gets destroyed or goes down, hiding my "main" interfaces (Wi-Fi and Ethernet). The interface option is not currently useful to avoid this, as it would allow me to chose either the Wi-Fi or the Ethernet interface.

A workaround would be to rename the desided interfaces to have the same prefix, and then use the existing wildcard functionality. However, it's probably not a very good idea to rename a network interface to make it play better with a desktop bar. :)

Would a pull request with this functionality be welcome?

Alexays commented 4 years ago

Hi! For sure go ahead, look at the wildcard functionality, should be the same logic 😄

vmsh0 commented 4 years ago

Proposed solution: reuse the interface option. When it is not present or it is a string, the behaviour is exactly the same as before (we don't want to break existing configs). When it is an array, the following additional logic applies:

This behaviour was chosen to be as consistent as possible with what happens now: when interface matches an interface which is down, that is still the preferred interface. If the wildcard would match multiple interfaces, the first match from getifaddrs() is taken. And when no interface matches, there is no preferred interface. Same as above! :)

It satisfies my use case and hopefully that of other people.

Does that sound ok? May I proceed?

Alexays commented 4 years ago

LGTM, you've got my go!

vmsh0 commented 4 years ago

I'm almost done.

There's just one little detail: in network.cpp evsock is added to various groups to receive notifications. However, nl_recvmsgs_default() is apparently never called.

Which should mean that rtnetlink notifications are not working!

I kinda suspect it was done like this because evsock is used synchronously in getExternalInterface, and having it also for callbacks meant that getExternalInterface could not read the response to its queries because they were consumed by the callback.

I think the proper fix would be to give getExternalInterface its own socket to do its thing. Does that sound right?

Addendum: I think the uninitialized thread_ symbol in network.hpp was maybe meant for calling nl_recvmsgs_default(). :)

Alexays commented 4 years ago

@vmsh0 oh yeah, that was something in my todo list, but I forgot 😢, LGTM, Go on it :)

vmsh0 commented 4 years ago

Ok, it should be fixed, and the new feature seems to work well!

I'm having second thoughts on the fall-back thing. It doesn't really make much sense to fall back to the first matched interface even if it's not running. What would you show it for? And worse, it could confuse the user into thinking that it's actually up. I'm gonna get that out of the way before opening the PR - unless you think it should stay.

A better feature would be a configuration switch which would allow the user to choose how to prioritize interfaces - do they want them prioritized by the order of appearence in the configuration, or do they want to prioritize the whitelisted interface which appears in the system defaut route?

Would you be interested in the feature? Can I go ahead and do that as well?

Alexays commented 4 years ago

This behavior should remain the same by default, in case another interface is added later like a vpn, but on the other hand you can make a priority, but only if whitelist is used.

vmsh0 commented 4 years ago

Sorry - I don't think I explained myself very well. Let me try again :)

Existing configs will still behave exactly the same as before. This is imperative! The only thing that will affect existing configs is that the rtnetlink bug is now fixed, so users might see a different update behaviour (using the logic that already existed in network.cpp).

What I'm doubtful about is what the exact behaviour should be when more than one interface is specified in the config file. What is clear is that a list of interfaces should at a minimum always work as a whitelist. I.e. when we have a list of interfaces, no interface that's not in the list can ever be selected by the module. Following that, there needs to be a strategy to select an interface among those specified in the list.

My first idea was to select the first interface that is up (specifically, I'm using the RUNNING flag), and when no interface is up, to just fall back the first (existing) interface in the list, even if it's down. This implied an ordering over the interfaces - specifically, the first interface specified in the list that is up is shown.

However, upon further reflection and tests, I realized that selecting an interface which is not up could easily give the user the illusion that it is up. (E.g. some network drivers and/or userspace tooling don't remove addresses from interfaces when they are brought down). For this reason, I don't really want to push this functionality. It really feels like bad UX.

A simple alternative is just to select the first interface specified in the list that is up. When all interfaces are down, the current if will just be -1, as with the single-interface case. (format-disconnected will be shown on the bar.)

However this is a bit too simple to be useful to most users. So what I wanted to do is to use an enchanced priority algorithm which would prioritize interfaces by the metric of the default routes in which they appear in the routing table (and using the order in the config as a fallback). This would work similarly to the existing auto-selection algorithm (i.e. attempt to show the interface the packets are actually egressing from), but with the whitelist and the fallback to the configuration order. And I was considering the idea of having this enhanced algorithm selectable, i.e. having a config option to enable/disable it.

Alexays commented 4 years ago

Oh yeah, great idea, I agree with you, it's better to use only the whitelisted interfaces, so as not to put the user in doubt and the format-disconnected will be used. If you we're able to implement an algorithm to auto-select the interface by the metric of the default rotues, in terms of UX, it just on the top! Feel free to refactor some of the Network part to suit your need 😄

vmsh0 commented 4 years ago

Hi! I see you deployed a fix for the update issues. I'll rebase on that. I still have plans to work on the interface list thing & the refactor, I'm just a bit busy with uni right now & it will take some time.

Cheers! :)

xeruf commented 4 days ago

how is the state here?

vmsh0 commented 10 hours ago

I still use Waybar & still have the issue. I have a partial implementation on my computer, but never gotten around to finishing it. I make no promises, but I will bump it up on my list of priorities :)