Add compatibility for Ipv6 address - #138

Open

koolok wants to merge 4 commits into

fluent:masterfrom

koolok commented Apr 24, 2018

Fixes: #137

Currently the library does not support Ipv6 as host adress. To make it compatible with Ipv6 I added a function that check if host is Ipv6 and if the address is Ipv6 we can connect to Ipv6 socket otherwise we can fallback to Ipv4 socket.

Add compatibility for Ipv6 address

8595b7e

arcivanov commented Apr 24, 2018

Member

This doesn't work which is indicated by the build not passing, obviously.
IPv6 support doesn't just include IPv6 address string, but also support for AAAA records of a hostname.

arcivanov reviewed

View reviewed changes

fluent/sender.py Outdated

+ try :
+ socket.inet_pton(socket.AF_INET6, self.host)
+ return True
+ except :

arcivanovApr 24, 2018•
edited
Loading

Member

PEP8 issue. Formatting all over the place. Bare except is generally frowned upon as it intercepts SystemExit and KeyboardInterrupt.

arcivanov reviewed

View reviewed changes

fluent/sender.py Outdated

+ socket.inet_aton(self.host)
+ return False
+ except Exception as e:
+ raise e

arcivanovApr 24, 2018

Member

So what if host is FQDN???

valentin benozillo added 2 commits

April 25, 2018 09:00

Fix how to check if the host is IPv6, work with domain name

3105f94

PIP8 ok

Fix how to check if the host is IPv6, work with domain name

6b4efa3

PIP8 ok
w to check if the host is IPv6, work with domain name�OC

koolok commented Apr 25, 2018

Author

@arcivanov I tried to check if the host use IPv6, even if the host is FQDN by using socket.getaddrinfo.
But the tests are still failing so can you please explain your comment #138 (review)

koolok commented May 2, 2018

Author

@arcivanov Can you please have a look on the changes and tell me whats exactly is wrong with the logic. Actualy I need this feature, any help will be appreciated.

arcivanov reviewed

View reviewed changes

fluent/sender.py Outdated

+ def _is_ipv6_host(self):
+ try:
+ socket.getaddrinfo(self.host, None, socket.AF_INET6)

arcivanovMay 2, 2018

Member

This will result in host that has AAAA records to prefer AF_INET6 even if AF_INET IPv4 addresses are available. This is not the current behavior and will potentially break tons of stuff for people who are not expecting this behavior.

Fix: use Ipv4 by default

edebe05

coveralls commented May 3, 2018

Coverage decreased (-0.8%) to 99.196% when pulling edebe05 on koolok:ipv6 into 7389fb6 on fluent:master.

4 similar comments

coveralls commented May 3, 2018

Coverage decreased (-0.8%) to 99.196% when pulling edebe05 on koolok:ipv6 into 7389fb6 on fluent:master.

coveralls commented May 3, 2018

Coverage decreased (-0.8%) to 99.196% when pulling edebe05 on koolok:ipv6 into 7389fb6 on fluent:master.

coveralls commented May 3, 2018

Coverage decreased (-0.8%) to 99.196% when pulling edebe05 on koolok:ipv6 into 7389fb6 on fluent:master.

coveralls commented May 3, 2018

Coverage decreased (-0.8%) to 99.196% when pulling edebe05 on koolok:ipv6 into 7389fb6 on fluent:master.

arcivanov commented May 3, 2018•
edited
Loading

Member

Please write the unit tests to ensure both paths are covered and I'll merge this.

https://coveralls.io/builds/16815534/source?filename=fluent%2Fsender.py#L129

mcipperly mentioned this pull request

Add test for IPv6 functionality #174

Open

ananalaghbar commented Apr 4, 2022

Hi Guys, when you are going to merge this fix?

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment