Handle connection to IPv6 hosts #51

dangel101 · 2020-04-07T20:22:25Z

This fix handles adding IPv6 hosts to inventory, which currently fails with the error:
Failed to add host to inventory: SSH error - not found; check DNS or /etc/hosts

@pcuzner @jmolmo please review

dangel101 · 2020-04-07T20:36:35Z

@mnecas
@mwperina

mwperina · 2020-04-07T21:46:40Z

runner_service/utils.py

        try:
-            s.connect((self.host, self.port))
+            if ipaddress.ip_address(self.host) == 4:


Hmm, unless I'm missing something this will raise ValueError if you have FQDN specified in host. I think we should use something like this:

ipv6_addr = True try: ipaddress.IPv6Address(self.host) except ipaddress.AddressValueError: ipv6_addr = False if ipv6_addr: s = socket.socket(socket.AF_INET6, socket.SOCK_STREAM) s.connect((self.host, self.port, 0, 0)) else: s = socket.socket(socket.AF_INET, socket.SOCK_STREAM) s.connect((self.host, self.port))

Marcin is there any better solution how to support all possible combination of host specification (FQDN, IPv4 or IPv6)?

@tinez

I think it's better to start off with socket.getaddrinfo. The way it works, is it returns a list of 5-element tuples that can be used to pass to socket.socket.

If you use it with FQDN it will return ipv6 and/or ipv4 addresses in the order that is determined by DNS resolving configuration in your OS i.e.:

>>> pp(socket.getaddrinfo("localhost", 22, 0, socket.SOCK_STREAM)) [(<AddressFamily.AF_INET6: 10>, <SocketKind.SOCK_STREAM: 1>, 6, '', ('::1', 22, 0, 0)), (<AddressFamily.AF_INET: 2>, <SocketKind.SOCK_STREAM: 1>, 6, '', ('127.0.0.1', 22))]

if you use ipv4 it will limit the results to ipv4 addresses only, i.e.:

>>> pp(socket.getaddrinfo("127.0.0.1", 22, 0, socket.SOCK_STREAM)) [(<AddressFamily.AF_INET: 2>, <SocketKind.SOCK_STREAM: 1>, 6, '', ('127.0.0.1', 22))]

analogical scenario for ipv6:

>>> pp(socket.getaddrinfo("::1", 22, 0, socket.SOCK_STREAM)) [(<AddressFamily.AF_INET6: 10>, <SocketKind.SOCK_STREAM: 1>, 6, '', ('::1', 22, 0, 0))]

for non-resolvable hosts it will return the socket.gaierror:

>>> socket.getaddrinfo("imaginary.host.address", 22, 0, socket.SOCK_STREAM) Traceback (most recent call last): File "<stdin>", line 1, in <module> File "/usr/lib64/python3.7/socket.py", line 752, in getaddrinfo for res in _socket.getaddrinfo(host, port, family, type, proto, flags): socket.gaierror: [Errno -2] Name or service not known

tinez · 2020-04-14T09:18:59Z

runner_service/utils.py

@@ -206,9 +207,25 @@ def timeout_handler():
            raise SSHTimeout

        socket.setdefaulttimeout(self.timeout)
-        s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
+        socket.getaddrinfo(self.host, self.port, 0, 0, socket.SOL_TCP)


I think you didn't understood my previous comment - if you use the return value of the socket.getaddrinfo you can actually get rid of all the if-ology below. Also, it's probably better to use socket.socket as a context manager. Smth like this should do:

try: family, _, _, _, addr = socket.getaddrinfo(self.host, self.port, 0, socket.SOCK_STREAM)[0] except socket.gaierror: raise HostNotFound with socket.socket(family, socket.SOCK_STREAM) as s: try: s.connect(addr) except ConnectionRefusedError: raise SSHNotAccessible except socket.timeout: raise SSHTimeout

mwperina

Seems OK to me

jmolmo · 2020-04-16T12:26:34Z

LGTM

tinez · 2020-04-16T13:19:25Z

runner_service/utils.py

@@ -4,6 +4,7 @@
 import socket
 import sys
 import getpass
+import ipaddress


Not needed anymore

+1 if we not using it, please remove

tinez · 2020-04-16T13:23:15Z

runner_service/utils.py

+                raise SSHTimeout
+            else:
+                s.shutdown(socket.SHUT_RDWR)
+                s.close()


Since we're using the context manager we don't need call s.close() by ourselves anymore.

tinez · 2020-04-16T13:28:34Z

runner_service/utils.py

        try:
-            s.connect((self.host, self.port))
+            family, type, proto, canonname, sockaddr = socket.getaddrinfo(self.host, self.port, 0, 0, socket.SOL_TCP)[0]


type, proto, canonname, - we don't actually use any of these, so it's better to replace them with _, _, _, - it's a convention.
Also, since we're explicitly asking for a SOCK_STREAM type of socket during creation, it would be nice to pass that to the type argument, so (self.host, self.port, 0, socket.SOCK_STREAM, socket.SOL_TCP) would be better.

+1 again! if we're not using the vars, use _, to prevent flake errors

tinez · 2020-04-16T13:31:41Z

runner_service/utils.py

-            s.shutdown(socket.SHUT_RDWR)
-            s.close()
+
+        with socket.socket(family, socket.SOCK_STREAM) as s:


We were specifying socket.SOL_TCP during socket.getaddrinfo as protocol, so let's do the same here: (family, socket.SOCK_STREAM, socket.SOL_TCP)

makes sense to me

This fix handles adding IPv6 hosts to inventory, which currently fails with the error: Failed to add host to inventory: SSH error - <host-ip> not found; check DNS or /etc/hosts

pcuzner

lgtm - thanks!

tinez · 2020-04-20T08:29:09Z

Looks good, thanks Dana!

dominikholler · 2020-04-30T09:32:32Z

@tinez out of curiosity , why socket.create_connection is not used here?

tinez · 2020-04-30T10:39:14Z

@dominikholler simply haven't thought of that... but indeed it seems it would also work and even make the code a bit more simple.

mwperina · 2020-04-30T12:42:29Z

Resolves #49

mwperina requested changes Apr 7, 2020

View reviewed changes

tinez reviewed Apr 14, 2020

View reviewed changes

dangel101 force-pushed the B1820988_ipv6_connection_issue branch from 3e66bc8 to 7f51e6f Compare April 16, 2020 11:01

mwperina approved these changes Apr 16, 2020

View reviewed changes

tinez reviewed Apr 16, 2020

View reviewed changes

Handle connection to IPv6 hosts

46b8787

This fix handles adding IPv6 hosts to inventory, which currently fails with the error: Failed to add host to inventory: SSH error - <host-ip> not found; check DNS or /etc/hosts

dangel101 force-pushed the B1820988_ipv6_connection_issue branch from 7f51e6f to 46b8787 Compare April 19, 2020 11:03

pcuzner approved these changes Apr 20, 2020

View reviewed changes

mwperina merged commit 3569765 into ansible:master Apr 20, 2020

mwperina mentioned this pull request Apr 30, 2020

add ipv6 support to ansible hosts #49

Closed

Handle connection to IPv6 hosts #51

Handle connection to IPv6 hosts #51

dangel101 commented Apr 7, 2020 •

edited

dangel101 commented Apr 7, 2020

mwperina Apr 7, 2020

mwperina Apr 7, 2020

tinez Apr 8, 2020

tinez Apr 14, 2020 •

edited

mwperina left a comment

jmolmo commented Apr 16, 2020

tinez Apr 16, 2020

pcuzner Apr 17, 2020

tinez Apr 16, 2020 •

edited

pcuzner Apr 17, 2020

tinez Apr 16, 2020 •

edited

pcuzner Apr 17, 2020

tinez Apr 16, 2020

pcuzner Apr 17, 2020

pcuzner left a comment

tinez commented Apr 20, 2020

dominikholler commented Apr 30, 2020

tinez commented Apr 30, 2020

mwperina commented Apr 30, 2020

Handle connection to IPv6 hosts #51

Handle connection to IPv6 hosts #51

Conversation

dangel101 commented Apr 7, 2020 • edited

dangel101 commented Apr 7, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinez Apr 14, 2020 • edited

Choose a reason for hiding this comment

mwperina left a comment

Choose a reason for hiding this comment

jmolmo commented Apr 16, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinez Apr 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tinez Apr 16, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pcuzner left a comment

Choose a reason for hiding this comment

tinez commented Apr 20, 2020

dominikholler commented Apr 30, 2020

tinez commented Apr 30, 2020

mwperina commented Apr 30, 2020

dangel101 commented Apr 7, 2020 •

edited

tinez Apr 14, 2020 •

edited

tinez Apr 16, 2020 •

edited

tinez Apr 16, 2020 •

edited