Skip to content

Commit

Permalink
Fix support for services connected via IPv6 (cms-dev#1141)
Browse files Browse the repository at this point in the history
* Fix support for services connected via IPv6

Use the correct address family when connecting to the various RPC services.

The solution is basically the same suggested here:
cms-dev#157 (comment)

This work is the result of the work of @edomora97, @mark03 and @dariost.

* Mock getaddrinfo in test_background_connect

Calling getaddrinfo somehow breaks the mocking interface of socket,
replacing the MagicMock with the original function. This would break the
assert and the test in general.
To prevent so getaddrinfo is mocked as well, making it return just an
address without side effects.

* Try all the resolved addresses

- Try to resolve only with the SOCK_STREAM type
- Try to connect to all the resolved IP addresses, sometimes the first
  in the list is not the correct one (e.g. IPv4/v6)
- Remove redundant getaddrinfo in _connection_handler

* Stop after the first connection

* Pass family, type and proto to socket.socket()

As suggested in https://docs.python.org/3/library/socket.html#socket.getaddrinfo

* Fix tests
  • Loading branch information
edomora97 authored and stefano-maggiolo committed Dec 21, 2021
1 parent 176a7c9 commit 0c102eb
Show file tree
Hide file tree
Showing 3 changed files with 38 additions and 16 deletions.
30 changes: 23 additions & 7 deletions cms/io/rpc.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
# Copyright © 2010-2018 Stefano Maggiolo <[email protected]>
# Copyright © 2010-2012 Matteo Boscariol <[email protected]>
# Copyright © 2013-2017 Luca Wehrstedt <[email protected]>
# Copyright © 2019 Edoardo Morassutto <[email protected]>
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU Affero General Public License as
Expand Down Expand Up @@ -478,13 +479,28 @@ def _connect(self):
"""
try:
sock = gevent.socket.socket(socket.AF_INET, socket.SOCK_STREAM)
sock.connect(self.remote_address)
except OSError as error:
logger.debug("Couldn't connect to %s: %s.",
self._repr_remote(), error)
else:
self.initialize(sock, self.remote_service_coord)
# Try to resolve the address, this can lead to many possible
# addresses, we'll try all of them.
addresses = gevent.socket.getaddrinfo(
self.remote_address.ip,
self.remote_address.port,
type=socket.SOCK_STREAM)
except socket.gaierror:
logger.warning("Cannot resolve %s.", self.remote_address)
raise

for family, type, proto, _canonname, sockaddr in addresses:
try:
host, port, *rest = sockaddr
logger.debug("Trying to connect to %s at port %d.", host, port)
sock = gevent.socket.socket(family, type, proto)
sock.connect(sockaddr)
except OSError as error:
logger.debug("Couldn't connect to %s at %s port %d: %s.",
self._repr_remote(), host, port, error)
else:
self.initialize(sock, self.remote_service_coord)
break

def _run(self):
"""Maintain the connection up, if required.
Expand Down
9 changes: 2 additions & 7 deletions cms/io/service.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
# Copyright © 2010-2016 Stefano Maggiolo <[email protected]>
# Copyright © 2010-2012 Matteo Boscariol <[email protected]>
# Copyright © 2013 Luca Wehrstedt <[email protected]>
# Copyright © 2019 Edoardo Morassutto <[email protected]>
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU Affero General Public License as
Expand Down Expand Up @@ -156,13 +157,7 @@ def _connection_handler(self, sock, address):
connection.
"""
try:
ipaddr, port = address
ipaddr = gevent.socket.gethostbyname(ipaddr)
address = Address(ipaddr, port)
except OSError:
logger.warning("Unexpected error.", exc_info=True)
return
address = Address(address[0], address[1])
remote_service = RemoteServiceServer(self, address)
remote_service.handle(sock)

Expand Down
15 changes: 13 additions & 2 deletions cmstestsuite/unit_tests/io/rpc_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
# Contest Management System - http://cms-dev.github.io/
# Copyright © 2014-2017 Luca Wehrstedt <[email protected]>
# Copyright © 2015 Stefano Maggiolo <[email protected]>
# Copyright © 2019 Edoardo Morassutto <[email protected]>
#
# This program is free software: you can redistribute it and/or modify
# it under the terms of the GNU Affero General Public License as
Expand All @@ -21,6 +22,7 @@
"""

import socket
import unittest
from unittest.mock import Mock, patch

Expand Down Expand Up @@ -231,7 +233,14 @@ def test_method_return_list(self):
self.assertEqual(result.value, ["Hello", 42, "World"])

@patch("cms.io.rpc.gevent.socket.socket")
def test_background_connect(self, socket_mock):
@patch("cms.io.rpc.gevent.socket.getaddrinfo")
def test_background_connect(self, getaddrinfo_mock, socket_mock):
# Calling getaddrinfo breaks the mocking of the socket, so it is mocked
# as well. It returns the addresses associated with the service, in
# this case just one.
getaddrinfo_mock.return_value = [
(gevent.socket.AF_INET, gevent.socket.SOCK_STREAM, 6, "",
(self.host, self.port))]
# Patch the connect method of sockets so that it blocks until
# we set the done_event (we will do so at the end of the test).
connect_mock = socket_mock.return_value.connect
Expand All @@ -255,7 +264,9 @@ def test_background_connect(self, socket_mock):
# event triggered.
done_event.set()
gevent.sleep()
connect_mock.assert_called_once_with(Address(self.host, self.port))
getaddrinfo_mock.assert_called_once_with(self.host, self.port,
type=socket.SOCK_STREAM)
connect_mock.assert_called_once_with((self.host, self.port))

def test_autoreconnect1(self):
client = self.get_client(ServiceCoord("Foo", 0), auto_retry=0.002)
Expand Down

0 comments on commit 0c102eb

Please sign in to comment.