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

Added support of ipv6 address (boolean "use_v6" parameter in TelegramClient constructor) #425

Merged
merged 4 commits into from Nov 16, 2017

Conversation

@heffcodex
Copy link
Contributor

@heffcodex heffcodex commented Nov 12, 2017

Hello! Well, I did that:)
With this mod the library can work with my PySocks fork to use IPv6 address.

@@ -69,6 +69,7 @@ class TelegramBareClient:

def __init__(self, session, api_id, api_hash,
connection_mode=ConnectionMode.TCP_FULL,
use_v6=False,
Copy link
Member

@Lonami Lonami Nov 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

v6 sounds like "version six". Version six of what? Maybe something more explicit like use_ipv6 would make more sense.

Copy link
Contributor Author

@heffcodex heffcodex Nov 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I also think that it will be better.

"""Loads a saved session_user_id.session or creates a new one.
If session_user_id=None, later .save()'s will have no effect.
"""
if session_user_id is None:
return Session(None)
return Session(None, use_v6)
Copy link
Member

@Lonami Lonami Nov 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not too sure about this, having to pass use_v6 around everywhere. I will change this to have None IP address, and if this is the case, get the right IP from somewhere else.

@@ -64,7 +64,10 @@ def __init__(self, session_user_id):
self._last_msg_id = 0 # Long

# These values will be saved
self.server_address = '91.108.56.165'
if use_v6:
self.server_address = '[2001:67c:4e8:f002::a]'
Copy link
Member

@Lonami Lonami Nov 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So maybe the problem was the address wasn't enclosed with square braces? Why are these needed?

Copy link
Contributor Author

@heffcodex heffcodex Nov 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AFAIK, ipv6 address must be enclosed in square brackets if it used with port or within URL. But I'm not exactly sure, that these brackets are needed also there.

@Lonami
Copy link
Member

@Lonami Lonami commented Nov 12, 2017

Thank you! However I've noticed you don't use the use_v6 information when a reconnection occurs, which is likely. See this method:

def _get_dc(self, dc_id, ipv6=False, cdn=False):

@heffcodex
Copy link
Contributor Author

@heffcodex heffcodex commented Nov 12, 2017

I missed this place in the code. Do you have any suggestions on how best to throw a flag from the class constructor into this function?

@Lonami
Copy link
Member

@Lonami Lonami commented Nov 12, 2017

I've made some changes to your code so it's a bit cleaner. Please review them and let me know if this is enough to help you do that final change I requested.

@heffcodex
Copy link
Contributor Author

@heffcodex heffcodex commented Nov 12, 2017

I see that ipv6 parameter is used only in the same function:

return self._get_dc(dc_id, ipv6=ipv6, cdn=cdn)

So, maybe the best way will be to store use_ipv6 in class field and check it within this function?

@Lonami
Copy link
Member

@Lonami Lonami commented Nov 12, 2017

Yeah, best option in my opinion is to have TelegramBareClient._use_ipv6 so we can use self._use_ipv6 instead passing ipv6= as a parameter. We also need to think about this case: what if an user is using IPv4 but decides to use IPv6 at some point? It can happen, overall if you're testing. Make the attribute private so we don't need to worry about the user changing it at any time, only on creation.

@heffcodex
Copy link
Contributor Author

@heffcodex heffcodex commented Nov 12, 2017

Anyway, the originally selected server IP is saved in the session, so I think we can make the field private and fill it only when creating an instance of the class (or restoring the session from the file).

Man197 added 2 commits Nov 12, 2017
Better flag argument name (use_v6 -> use_ipv6).
The flag is now stored into private classfield.
_get_dc ipv6 fix.
@@ -300,15 +313,15 @@ def _get_dc(self, dc_id, ipv6=False, cdn=False):

return next(
dc for dc in TelegramBareClient._config.dc_options
if dc.id == dc_id and bool(dc.ipv6) == ipv6 and bool(dc.cdn) == cdn
if dc.id == dc_id and bool(dc.ipv6) == self._use_ipv6 and bool(dc.cdn) == cdn
Copy link
Member

@Lonami Lonami Nov 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here again, I noticed you use '[2001:67c:4e8:f002::a]' as the IPv6 string (notice the square braces '[]'). Why are these needed? Telegram's dc.ipv6 don't have these braces. Will it still work?

Copy link
Contributor Author

@heffcodex heffcodex Nov 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I will test it today/tomorrow and let you know.

Copy link
Member

@Lonami Lonami Nov 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Easiest way is to manually set a different datacenter on the session file so when you connect the library will need to change to your closest one.

Copy link
Contributor Author

@heffcodex heffcodex Nov 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, as I've just tested, in case of ipv6 connection, the server address MUST be in '[]' and the port MUST be 80, or server/proxy will return an Bad Gateway or Timeout error.

@Lonami
Copy link
Member

@Lonami Lonami commented Nov 13, 2017

I'm testing this pull request now and while I can't connect I found this:

socket.has_ipv6
This constant contains a boolean value which indicates if IPv6 is supported on this platform.

So maybe we should check if use_ipv6 and not socket.has_ipv6: raise.

On a different line, my problem when trying to connect with square braces around the IP I get:

    self._socket.connect(address)
socket.gaierror: [Errno -2] Name or service not known

And if I remove the braces:

    self._socket.connect(address)
OSError: [Errno 101] Network is unreachable

socket.has_ipv6 is True on my Linux machine.

@Lonami
Copy link
Member

@Lonami Lonami commented Nov 13, 2017

Even better (and this is one of the reasons why I gave up on adding IPv6 support myself):

>>> socket.has_ipv6
True
>>> socket.getaddrinfo("example.org", 80, proto=socket.IPPROTO_TCP)[1]
(<AddressFamily.AF_INET6: 10>, <SocketKind.SOCK_STREAM: 1>, 6, '', ('2606:2800:220:1:248:1893:25c8:1946', 80, 0, 0))
>>> a = socket.socket(socket.AF_INET6, socket.SOCK_STREAM)
>>> a.connect(('2606:2800:220:1:248:1893:25c8:1946', 80, 0, 0))
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
OSError: [Errno 101] Network is unreachable
>>>
>>> socket.getaddrinfo("example.org", 80, proto=socket.IPPROTO_TCP)[0]
(<AddressFamily.AF_INET: 2>, <SocketKind.SOCK_STREAM: 1>, 6, '', ('93.184.216.34', 80))
>>> a = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
>>> a.connect(('93.184.216.34', 80))
>>> a.close()

@heffcodex
Copy link
Contributor Author

@heffcodex heffcodex commented Nov 13, 2017

AFAIK, even if ipv6 is supported on the target platform, the machine may not connect via ipv6 without having an ipv6 address. It depends on the ISP. An alternative way is to use the ipv6 proxy, for which all these changes were started.
socket.has_ipv6 does not provide reliable information, and it seems to me that there is no difference that it will cause an exception - Telethon or socket module. Therefore, I think check socket.has_ipv6 is superfluous.

@Lonami
Copy link
Member

@Lonami Lonami commented Nov 13, 2017

Alright, thanks for the info. Despite this though, ifconfig does show an IPv6 address on the network adapter I'm using so I think it's safe to assume that I do have IPv6 (also, I can create an IPv6 socket without trouble).

@heffcodex
Copy link
Contributor Author

@heffcodex heffcodex commented Nov 13, 2017

Your network adapter can have an ipv6 address, so socket.has_ipv6 also returns True. But does your ISP network have ipv6 addresses? Most likely, all use of ipv6 ends on your network adapter :)

@Lonami Lonami merged commit ee5915e into LonamiWebs:master Nov 16, 2017
@Lonami
Copy link
Member

@Lonami Lonami commented Nov 16, 2017

Thanks, I will add support for switching between IPv4 to IPv6 and vice versa if a previous connection was made now.

@heffcodex
Copy link
Contributor Author

@heffcodex heffcodex commented Nov 16, 2017

Thank you so much! Many people have already asked me to do pull request, finally they will be happy :)

@Lonami
Copy link
Member

@Lonami Lonami commented Nov 16, 2017

One last note, Telegram as I said returns this for IPv6 addresses:

dc_options=[
    DcOption(
        ipv6=False,
        media_only=False,
        tcpo_only=False,
        cdn=False,
        static=False,
        id=1,
        ip_address='149.154.175.50',
        port=443
    ),
    DcOption(
        ipv6=True,
        media_only=False,
        tcpo_only=False,
        cdn=False,
        static=False,
        id=1,
        ip_address='2001:0b28:f23d:f001:0000:0000:0000:000a',
        port=443
    ),
    ...
],

So I'll add the square braces to surround the IP address if IPv6 is enabled, as you told me it needs to be.

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

Successfully merging this pull request may close these issues.

None yet

3 participants