Enforce two-tuple input for socket.connect()

[kevinconway]

The results from socket.getaddrinfo() stored in sockaddr differ from IPv4 and IPv6. The IPv4 values are a two-tuple of (HOST, PORT). However, the IPv6 values are a four-tuple of (HOST, PORT, FLOW_INFO, SCOPE_ID). This behaviour is documented on https://docs.python.org/2/library/socket.html#socket.getaddrinfo. Unfortunately, the error thrown is not in the socket.error family which causes a stack trace rather than skipping the IPv6 connection.

The additional values returned for the IPv6 address are invalid for calls to connect which expects only a two-tuple matching the IPv4 output. There is an additional function in the socket modules, create_connection, which accounts for this issue. However, this function would not produce the correct socket implementation should one other than the standard lib be given.

Forcing the tuple length to two will cause the call to connect to raise an error in the socket.error family for IPv6 connections which, in turn, will cause the existing code to skip it and fall back on the IPv4 connection.

[awestendorf]

@rocco66 Can you comment on this? It was your IPv6 support in #66 that Kevin is trying to fix. Without an IPv6 host to test against, and having only confirmed that your patch did not break IPv4, I'm inclined to revert your PR.

[kevinconway]

The only way to have the connect function work with an IPv6 address is to initialize the instance with socket.AF_INET6 as the first parameter. This could be accomplished by passing in klass=partial(socket.socket, socket.AF_INET6). I'm not clear on where that would take place if I wanted to set it since it must be passed in during the connect call of the Transport. The examples only show use of the string keyword shortcuts for using transports.

If it's possible to bind the SocketTransport to only the standard lib socket we could also make use of socket.create_connection.

[kevinconway]

I was thinking about this some more. If we assume that any value given as klass is an API compatible socket implementation we should be able to automatically switch between IPv4 and IPv6 connections as available in the environment. For example, here is a potential alternate implementation:

    def connect(self, (host, port), klass=socket.socket):
        '''
        Connect assuming a host and port tuple.
        '''
        self._host = "%s:%s" % (host, port)
        infos = socket.getaddrinfo(host, port, 0, 0, socket.IPPROTO_TCP)
        if not infos:
            raise socket.error("getaddrinfo returns an empty list")
        # Sort addresses such that the new IPv6 address is first.
        for _family, _socktype, _proto, _canonname, sockaddr in sorted(infos, key=lambda info: len(info[-1])):

            try:
                # Check if the IP is a valid IPv6 address. Fallback to IPv4 if not.
                socket.inet_pton(socket.AF_INET6, sockaddr[0])
                self._sock = klass(socket.AF_INET6)
            except socket.error:
                 self._sock = klass()

            self._sock.setblocking(True)
            self._sock.settimeout(self.connection._connect_timeout)
            if self.connection._sock_opts:
                for k, v in self.connection._sock_opts.iteritems():
                    family, type = k
                    self._sock.setsockopt(family, type, v)
            self._sock.connect(sockaddr[:2])

            # After connecting, switch to full-blocking mode.
            self._sock.settimeout(None)

This will cause connections to default to IPv6 if there is a valid IPv6 address resolved from the given hostname/IP address. Otherwise it will fall back to an IPv4 connection if found. If no connections are found the existing behaviour of raising an exception is preserved.

Let me know if this implementation is preferable and I can change the patch.

[kevinconway]

Hey folks. I added a patch to the pull request that actually allows for IPv6. The first patch simply fixed IPv4 connections when an IPv6 address is present. This patch prioritizes IPv6 if it exists with a fallback to IPv4.

I'm looking at adding some tests for the new behaviour. I noticed the tests used a value of ('host', 5309) for connections. Do you have special entries in your hosts file to account for this? The tests fail when I run them because that host and IP cannot be resolved. I can modify the values to ('localhost', 5309), but that also requires that something listen on that port. Any suggestions?

[awestendorf]

The tests are using Chai for mocking. I like to use sensible but not-normal values in expectations and mocks so it's easier to see how parameters influence the test. The ('host', 5309) tuple is matched here

I don't have an IPv6 host to test bu I can look over your changes and confirm that they still operate for IPv4. What I see looks good so far.

[kevinconway]

@awestendorf Are the tests for the project passing as is? It looks like #66 caused a regression in the socket_transport tests by adding a call to the socket module's socket.getaddrinfo() function. This function fails if the hostname and port do not resolve to an actual TCP listener. The test values of ('host', 5309) won't resolve unless you happen to have rabbitmq running and 'host' resolves to some address. It looks like we'll need to either monkey patch the call to socket.getaddrinfo() or add getaddrinfo as an additional arg similar to klass for socket. Any preference?

Either way, putting socket.getaddrinfo() into our control for testing will allow us to write tests to simulate IPv6 vs IPv4 availability in unit tests.

Current test output from master:

======================================================================
ERROR: test_connect_with_klass_arg (tests.unit.transports.socket_transport_test.SocketTransportTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vagrant/venv/lib/python2.7/site-packages/chai/chai.py", line 63, in wrapper
    func(self, *args, **kwargs)
  File "/home/vagrant/patch/haigha/tests/unit/transports/socket_transport_test.py", line 71, in test_connect_with_klass_arg
    self.transport.connect(('host', 5309), klass=klass)
  File "/home/vagrant/patch/haigha/haigha/transports/socket_transport.py", line 32, in connect
    infos = socket.getaddrinfo(host, port, 0, 0, socket.IPPROTO_TCP)
gaierror: [Errno -2] Name or service not known

======================================================================
ERROR: test_connect_with_no_klass_arg (tests.unit.transports.socket_transport_test.SocketTransportTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/vagrant/venv/lib/python2.7/site-packages/chai/chai.py", line 63, in wrapper
    func(self, *args, **kwargs)
  File "/home/vagrant/patch/haigha/tests/unit/transports/socket_transport_test.py", line 49, in test_connect_with_no_klass_arg
    self.transport.connect(('host', 5309))
  File "/home/vagrant/patch/haigha/haigha/transports/socket_transport.py", line 32, in connect
    infos = socket.getaddrinfo(host, port, 0, 0, socket.IPPROTO_TCP)
gaierror: [Errno -2] Name or service not known

----------------------------------------------------------------------

[awestendorf]

Nope, looks like the tests were broken in master. I fixed them and will be looking into Travis for CI.

[kevinconway]

I added tests to demonstrate the behaviour when there is only an IPv6 address available as well as when both are available and priority is given to the IPv6 address.

[vitaly-krugl]

Guys, just wanted to give you my kudos for the disciplined approach to unit tests in haigha. This is sorely lacking in some of the competing packages.

[vitaly-krugl]

Is any of this related to issue #76?

TypeError: getsockaddrarg() takes exactly 2 arguments (4 given)

Thanks

[kevinconway]

Yes, I believe this patch addresses that issue.

[vitaly-krugl]

@kevinconway @awestendorf: I coded up this example of how connectivity and error reporting (raising of exceptions) should work well: https://github.com/vitaly-krugl/inetpy/blob/master/inetpy/connect.py. See connect_tcp and the underlying connect_from_addr_infos for details. This code is based on the example in https://docs.python.org/2/library/socket.html.

This addresses the ordering, error-reporting, exception raising, and connectivity issues that I raised in my code review feedback.

You are welcome to reuse my implementation in Haigha.

Best, Vitaly

[vitaly-krugl]

@kevinconway, I created the PR #80 to demonstrate what I had in mind. It's fully functional, compliant with ordering rules, attempts connection with every compatible address until it succeeds or exhausts them all, and doesn't fake socket.error exceptions.

Please feel free to pull it in directly.

Best, Vitaly

[kevinconway]

@vitaly-krugl @awestendorf

Ready for another round of reviewing. I believe I've incorporated all of the feedback from @vitaly-krugl. The code no longer reorders the address info, now uses the results of getaddrinfo to construct the socket rather than guessing the protocol, and raises an internal haigha exception rather than a socket error on failure.

Give the patch another look when you both get a chance and let me know what you think.

[vitaly-krugl]

@kevinconway, thanks for the update. Please see my code review feedback.

[kevinconway]

@vitaly-krugl: I appreciate the time you're putting in to your reviews. I've only read over it briefly, but it looks like good feedback. I'll read more deeply when I get a chance and roll out an update.

[awestendorf]

@vitaly-krugl Seconded, thank you for the very thorough review. I agree with all your notes and added some comments where appropriate. Thank you @kevinconway for continuing to iterate on this, the final result will be well worth it. Next up, TLS :smile:

[kevinconway]

@vitaly-krugl @awestendorf Hey team. Just rolled out an update based on the feedback you both left. When you get an opportunity give the patch another review. Once we're at a stable point I'll squash the commits and push again before the merge.

[vitaly-krugl]

Thanks @kevinconway, I provided feedback on several areas, but would you mind fixing that up and squashing all your changes into a single commit before further code review? This would help the reviewer detect problems that might be introduced during squashing.

Also, please run pylint and address findings related to your changes, if any. Thx!

[vitaly-krugl]

Hi @kevinconway, I am off the grid for the next few days. I will review the changes next week, unless @awestendorf beats me to it. Best.

[vitaly-krugl]

Thanks @kevinconway, I completed the code review and we're almost there. Please address my new feedback above regarding:

• _sock_opts
• any_order (x 2)
• "Creation and configuration of a socket apparently can also fail"