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

[#3415][#3417]multi-homing: manage coma separated list of incoming interfaces #300

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

Strangelovian
Copy link

@Strangelovian Strangelovian commented Aug 19, 2020

I developed this change to address some multi-homing needs I have with deluge.
The GTK and Web UI will now allow a list of IP addresses or network interface names, instead of a single IP address.
This list, appended with ports, will be passed to libtorrent.

I tested on debian sid. With port 23456, The following input works as expected:
lo,10.3.14.15, 2000:abcd:1234:5678::15

Then deluge(d) binds only:

127.0.0.1:23456 
[::1]:23456
10.3.14.15:23456
[2000:abcd:1234:5678::15]:23456

One should note that lib torrent expects ipv6 addresses to be passed between square brackets.
But on the python side, brackets are not valid according to ipaddress:IPV6Address constructor.
Therefore I added some extra IPV6 logic to add the surrounding brackets, prior to passing the listen_interfaces to lib torrent.
From the Deluge UI, the end user should provide IPV6 addresses without square brackets.

1st commit: fix binding for multiple incoming/outgoing interfaces
2nd commit: fix geoip for ipv6 (don't make sense without the first commit since ipv6 won't work)

@Strangelovian Strangelovian changed the title manage coma separated list of incoming interfaces multi-homing: manage coma separated list of incoming interfaces Aug 19, 2020
@Strangelovian
Copy link
Author

The following test was already broken prior to this PR:
deluge/tests/test_component.py::ComponentTestClass::test_start_paused_error
in the job 1772.2 Unit tests - libtorrent 1.2.

When I run locally tox -e py3, all tests pass.

@Strangelovian
Copy link
Author

@Strangelovian Strangelovian changed the title multi-homing: manage coma separated list of incoming interfaces [#3415][#3417]multi-homing: manage coma separated list of incoming interfaces Aug 20, 2020
allow to configure up to 100 characters in gtk outgoing interfaces control instead of 15, which was too small for IP v6 addresses
@doadin
Copy link
Contributor

doadin commented Sep 5, 2020

It not very clean but since its at least related id like to mention doadin@633e312

It would be nice(since libtorrent supports it) to be able to use interface names instead of just ip's. Would also be nice to populate a dropdown menu in the gtkui(and webui?) of interfaces available. I think this can be done with netifaces.interfaces() then converting that list object to a menu. is ip should probabbly just be replaced with an all in one is interface?

@Strangelovian
Copy link
Author

With this PR you can configure ipv4, ipv6, linux interface names, and probably Windows GUID interface identifiers.
I’m not a UI specialist though, so I let the drop down implementation to others

@doadin
Copy link
Contributor

doadin commented Sep 6, 2020

sorry late night replies but a few things, yes i agree menu could definitely be a different PR.

Now could be just my 2 cents but I also just noticed you removed the is_ip, I think this is a step backwards instead of forward. While I think something like netifaces would provide better validation(and therefore a direction we should move toward) as it checks for real interfaces and not just if it looks good, I think having validation in general is good.

I think maybe something like this could be helpful don't know if its even proper python just throwing out an idea with something like my updated is_ip, then:

        lib_interfaces = []
        for interface in interfaces.split(','):
            interface = interface.strip()
            try:
              #first check if we have a valid argument interface will work with
              if is_ip(interface)
                # then check if we are using ipv6 and add brackets as needed
               if ipaddress.ip_address(interface).version == 6
                lib_interfaces.append(
                    '[' + interface + ']'
                )
              else
                # if we are not given a proper interface in any of the acceptable forms of an interface throw an exception
                except ValueError:
                    log.error('Invalid interface given')

I hope this comment/thought is clear.
for the gtkui part something like(again can be seperate):

def make_interface_menu(self):
        menu = gtk.Menu()
        interfacelist = netifaces.interfaces()
        for interface in interfacelist:
            item = gtk.MenuItem(interface)
            menu.append(item)
        menu.show()
        return menu 

@Strangelovian
Copy link
Author

I agree, the code can be rewritten to encapsulate the exception handling in is_ip, it would be cleaner.

Regarding validating the ip addresses and interface names, it would be better done via libtorrent feedback, which is not done at the moment. Currently, one has to use command line netstat to make sure libtorrent listens to the desired addresses.

So ideally for a clean code we would need:

  • expose the list of all addresses and interfaces in UI (Gtk + web)
  • allow user to select any combination of addresses and interface
  • validate the user input validity at the python level in the daemon
  • feedback from libtorrent: show user the list of really listened ip addresses

@doadin
Copy link
Contributor

doadin commented Sep 6, 2020

I agree there is only so much that can be done on our side validation wise. I think currently our position is just check if a ip provided is a valid format which doesn't include names/guid . Which this PR it takes it a step back and just relies on the user more. Which imo is not a great plan. but i think with a move to netifaces we still don't get feedback from libtorrent to know if the interface given is usable but we also eliminate, user error, eg. where a user enters 2607::01::1 where they meant 2607::01:1 and then spends a while trying to find the issue (more validation is helpful, only human, mistakes can be made). This is just one example but in any case again just my 2 cents but more validation vs less is the way forward.

for reference https://stackoverflow.com/questions/29913516/how-to-get-meaningful-network-interface-names-instead-of-guids-with-netifaces-un has examples to help with UI portion discussed here.

IMO Obviously first we would need to decide how to proceed with the main topic of this PR. but since the main point deals with validation instead of re-writing this section of code to add/remove validation 10 more times in potential subsequent PRs/commits. I think it would be helpful to decide on a single path forward and proceed that way.

I think end goal would be to have what qbittorrent has which is a list of interface names and then a sub list of ips associated with that interface. I think though one step at a time is fine and an interface list would do. I think particularly with the ui user needing to know the ip then manually enter it is not very user friendly. Its also a situation that all this in mind doesn't include ui support for multi-homing which will be a whole other situation.

I think

  1. Expand validation to support ipv6/long interface names
  2. Expand command line/ui elements to support long input to accommodate ipv6/interface names
  3. Change ui elements to be menus(with interface names) instead of ips. or add menu to select multi-homing/single then from there add a menu of interfaces for single or input box for multi.(similar happens with selecting socks5 vs socks 5/auth ? if i remember correctly)

@Strangelovian
Copy link
Author

Strangelovian commented Sep 6, 2020

Let's keep things simple. We won't get anywhere if we don't do incremental steps.
I'll replace this PR by two others such as:
PR1: the changes for ip / interface list
PR2: the geoipv6 stuff

I'll make PR1 compatible with your PR doadin@633e312, i.e. I'll restore the is_ip function.

@doadin
Copy link
Contributor

doadin commented Sep 6, 2020

While I think we are on the same page it would nice to get some feedback from @cas-- as hes the one with power around here :) . Id hate for all your work to be unaccepted based on suggestions I made.

@dtbartle
Copy link

dtbartle commented Dec 8, 2020

@Strangelovian @doadin any progress on this?

@Strangelovian
Copy link
Author

Sorry, no news from maintainers, this project is kind of abandonned

@ghost
Copy link

ghost commented Dec 20, 2020

I included these patches in my NixOS configuration and I can report dual-stack works again when the correct incoming addresses are specified. However GeoIP for IPv6 does not work: Every time I try to set the path to the GeoIP IPv6 database (in my thin client) and click 'Apply', it resets back to the default path.

@Strangelovian
Copy link
Author

I included these patches in my NixOS configuration and I can report dual-stack works again when the correct incoming addresses are specified. However GeoIP for IPv6 does not work: Every time I try to set the path to the GeoIP IPv6 database (in my thin client) and click 'Apply', it resets back to the default path.

Sorry, "impossible" to test for me with thin client mode. I tested two modes: standalone gtk+ deluge, and daemon deluge + web UI. Both were Okay. I guess it never works without testing first all possible configurations...

@ghost
Copy link

ghost commented Dec 21, 2020

I tested in standlone gtk+ mode now as well, the same thing happens there:

recording.mp4

@Strangelovian
Copy link
Author

I tested in standlone gtk+ mode now as well, the same thing happens there:
recording.mp4

Me too without this:
sudo apt install python3-geoip
The thing runs, but it's not displaying any country flags.
With it, flags are OK:
image

@JulianGro
Copy link

Sorry, no news from maintainers, this project is kind of abandonned

Just to comment on that: While work has been very slow, I had Cas troubleshoot an issue with me just three months ago. I assume the "illness" they talked about on their Patreon is still a thing.

@deluge-torrent deluge-torrent deleted a comment from Erarshad Dec 20, 2021
@doadin doadin mentioned this pull request Jan 15, 2022
@Tydus
Copy link
Contributor

Tydus commented Oct 29, 2022

Any update on this? I think a working IPv6 setup is important and urgent.
As a reference, some PT sites only work from IPv6 and I found that nearly everyone moved to qbt (except me using the patches above).
I've been a deluge fan for ~10 years (largely because of its distinctive thin-client design and plugin system.)
I think the blocking issues should be solved before we can "Make Deluge Great Again".

@cinderblock
Copy link

cinderblock commented Jul 8, 2023

In lieu of waiting for the merge of this awesome work, it is currently possible to configure Deluge to listen entirely on IPv6.
I haven't found documentation on this so I thought I'd add what I found here in case it helps someone else.


While the normal UI won't allow [::] to be set as the interface in the Preferences, you can directly set it in the core.conf file:

{
    // ...
    "listen_interface": "[::]",
    // ...
}

Unfortunately this does not apply to the UI server, it still listens on IPv4 only.

Looking into the sources, the Daemon does have a dedicated option for this interface.
Following the sources and some confusing variable name changes...

Description Daemon class argument name         cli argument         core.conf preferences.py "builder" preferences.py variable name
UI Interface interface --ui-interface N/A N/A N/A
BitTorrent Interface listen_interface --interface listen_interface entry_interface incoming_address
Outgoing BitTorrent Interface outgoing_interface --outgoing-interface outgoing_interface entry_outgoing_interface outgoing_address

Unfortunately, there doesn't seem to be a way to set Daemon's interface from core.conf. However, it is available as a cli argument --ui-interface (but it takes a slightly different format, no [] brackets).

So, setting listen_interface in core.conf and starting deluged with --ui-interface :: (or a full IPv6 address) gets you Deluge daemon running entirely on IPv6!


I'm having some subsequent issues getting deluge-web to work with an AAAA record for my IP that I can't yet explain, but I don't suspect it is related to this configuration. It works if I connect directly by IPv6 address.

@Theetjuh
Copy link

Would love a resurrection of this, can I just change the files for myself and get dualstack working?

@Theetjuh
Copy link

It did and it works beautifully!
Let’s hope this will get in the main branch soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants