From 07a7a269bb34702d1922fe58b98c209674c08e33 Mon Sep 17 00:00:00 2001 From: Kaifeng Wang Date: Mon, 3 Aug 2020 15:38:58 +0000 Subject: [PATCH] Fix console auto port allocation under IPv6 By default _verify_port() only works for IPv4 network, the same port can be allocated to multiple nodes in a IPv6 network because the port checking passed and be used for other nodes. This fix passes the socat_address to the port validation and use the correct address family to do the socket binding. Story: 2007946 Task: 40412 Change-Id: I1355afaa551baee7b9fd7883d2d29342d059c5a0 --- ironic/drivers/modules/console_utils.py | 23 +++++++--- ironic/drivers/modules/ipmitool.py | 7 +-- .../drivers/modules/test_console_utils.py | 43 +++++++++++++++++-- .../unit/drivers/modules/test_ipmitool.py | 5 ++- ...sole-port-alloc-ipv6-26760f53f86209d0.yaml | 5 +++ 5 files changed, 70 insertions(+), 13 deletions(-) create mode 100644 releasenotes/notes/socat-console-port-alloc-ipv6-26760f53f86209d0.yaml diff --git a/ironic/drivers/modules/console_utils.py b/ironic/drivers/modules/console_utils.py index b2f92ba3d2..6e08b67128 100644 --- a/ironic/drivers/modules/console_utils.py +++ b/ironic/drivers/modules/console_utils.py @@ -162,11 +162,24 @@ def _get_port_range(): return start, stop -def _verify_port(port): +def _verify_port(port, host=None): """Check whether specified port is in use.""" - s = socket.socket() + ip_version = None + if host is not None: + try: + ip_version = ipaddress.ip_address(host).version + except ValueError: + # Assume it's a hostname + pass + else: + host = CONF.host + if ip_version == 6: + s = socket.socket(socket.AF_INET6) + else: + s = socket.socket() + try: - s.bind((CONF.host, port)) + s.bind((host, port)) except socket.error: raise exception.Conflict() finally: @@ -174,7 +187,7 @@ def _verify_port(port): @lockutils.synchronized(SERIAL_LOCK) -def acquire_port(): +def acquire_port(host=None): """Returns a free TCP port on current host. Find and returns a free TCP port in the range @@ -187,7 +200,7 @@ def acquire_port(): if port in ALLOCATED_PORTS: continue try: - _verify_port(port) + _verify_port(port, host=host) ALLOCATED_PORTS.add(port) return port except exception.Conflict: diff --git a/ironic/drivers/modules/ipmitool.py b/ironic/drivers/modules/ipmitool.py index 85beca183c..a3b443b581 100644 --- a/ironic/drivers/modules/ipmitool.py +++ b/ironic/drivers/modules/ipmitool.py @@ -807,10 +807,10 @@ def _constructor_checks(driver): _check_temp_dir() -def _allocate_port(task): +def _allocate_port(task, host=None): node = task.node dii = node.driver_internal_info or {} - allocated_port = console_utils.acquire_port() + allocated_port = console_utils.acquire_port(host=host) dii['allocated_ipmi_terminal_port'] = allocated_port node.driver_internal_info = dii node.save() @@ -1411,7 +1411,8 @@ class IPMISocatConsole(IPMIConsole): """ driver_info = _parse_driver_info(task.node) if not driver_info['port']: - driver_info['port'] = _allocate_port(task) + driver_info['port'] = _allocate_port( + task, host=CONF.console.socat_address) try: self._exec_stop_console(driver_info) diff --git a/ironic/tests/unit/drivers/modules/test_console_utils.py b/ironic/tests/unit/drivers/modules/test_console_utils.py index 752fa5fd14..3419abb4a2 100644 --- a/ironic/tests/unit/drivers/modules/test_console_utils.py +++ b/ironic/tests/unit/drivers/modules/test_console_utils.py @@ -23,6 +23,7 @@ import ipaddress import os import random import signal +import socket import string import subprocess import tempfile @@ -668,7 +669,7 @@ class ConsoleUtilsTestCase(db_base.DbTestCase): def test_allocate_port_success(self, mock_verify, mock_ports): self.config(port_range='10000:10001', group='console') port = console_utils.acquire_port() - mock_verify.assert_called_once_with(10000) + mock_verify.assert_called_once_with(10000, host=None) self.assertEqual(port, 10000) mock_ports.add.assert_called_once_with(10000) @@ -679,7 +680,9 @@ class ConsoleUtilsTestCase(db_base.DbTestCase): mock_verify.side_effect = (exception.Conflict, exception.Conflict, None) port = console_utils.acquire_port() - verify_calls = [mock.call(10000), mock.call(10001), mock.call(10002)] + verify_calls = [mock.call(10000, host=None), + mock.call(10001, host=None), + mock.call(10002, host=None)] mock_verify.assert_has_calls(verify_calls) self.assertEqual(port, 10002) mock_ports.add.assert_called_once_with(10002) @@ -691,5 +694,39 @@ class ConsoleUtilsTestCase(db_base.DbTestCase): mock_verify.side_effect = exception.Conflict self.assertRaises(exception.NoFreeIPMITerminalPorts, console_utils.acquire_port) - verify_calls = [mock.call(p) for p in range(10000, 10005)] + verify_calls = [mock.call(p, host=None) for p in range(10000, 10005)] mock_verify.assert_has_calls(verify_calls) + + @mock.patch.object(socket, 'socket', autospec=True) + def test__verify_port_default(self, mock_socket): + self.config(host='localhost.localdomain') + mock_sock = mock.MagicMock() + mock_socket.return_value = mock_sock + console_utils._verify_port(10000) + mock_sock.bind.assert_called_once_with(('localhost.localdomain', + 10000)) + + @mock.patch.object(socket, 'socket', autospec=True) + def test__verify_port_hostname(self, mock_socket): + mock_sock = mock.MagicMock() + mock_socket.return_value = mock_sock + console_utils._verify_port(10000, host='localhost.localdomain') + mock_socket.assert_called_once_with() + mock_sock.bind.assert_called_once_with(('localhost.localdomain', + 10000)) + + @mock.patch.object(socket, 'socket', autospec=True) + def test__verify_port_ipv4(self, mock_socket): + mock_sock = mock.MagicMock() + mock_socket.return_value = mock_sock + console_utils._verify_port(10000, host='1.2.3.4') + mock_socket.assert_called_once_with() + mock_sock.bind.assert_called_once_with(('1.2.3.4', 10000)) + + @mock.patch.object(socket, 'socket', autospec=True) + def test__verify_port_ipv6(self, mock_socket): + mock_sock = mock.MagicMock() + mock_socket.return_value = mock_sock + console_utils._verify_port(10000, host='2001:dead:beef::1') + mock_socket.assert_called_once_with(socket.AF_INET6) + mock_sock.bind.assert_called_once_with(('2001:dead:beef::1', 10000)) diff --git a/ironic/tests/unit/drivers/modules/test_ipmitool.py b/ironic/tests/unit/drivers/modules/test_ipmitool.py index e45aee26f3..c13aae62ac 100644 --- a/ironic/tests/unit/drivers/modules/test_ipmitool.py +++ b/ironic/tests/unit/drivers/modules/test_ipmitool.py @@ -2629,7 +2629,7 @@ class IPMIToolDriverTestCase(Base): with task_manager.acquire(self.context, self.node.uuid) as task: port = ipmi._allocate_port(task) - mock_acquire.assert_called_once_with() + mock_acquire.assert_called_once_with(host=None) self.assertEqual(port, 1234) info = task.node.driver_internal_info self.assertEqual(info['allocated_ipmi_terminal_port'], 1234) @@ -2959,6 +2959,7 @@ class IPMIToolSocatDriverTestCase(IPMIToolShellinaboxTestCase): autospec=True) def test_start_console_alloc_port(self, mock_stop, mock_start, mock_info, mock_alloc): + self.config(socat_address='2001:dead:beef::1', group='console') mock_start.return_value = None mock_info.return_value = {'port': None} mock_alloc.return_value = 1234 @@ -2970,7 +2971,7 @@ class IPMIToolSocatDriverTestCase(IPMIToolShellinaboxTestCase): mock_start.assert_called_once_with( self.console, {'port': 1234}, console_utils.start_socat_console) - mock_alloc.assert_called_once_with(mock.ANY) + mock_alloc.assert_called_once_with(mock.ANY, host='2001:dead:beef::1') @mock.patch.object(ipmi.IPMISocatConsole, '_get_ipmi_cmd', autospec=True) @mock.patch.object(console_utils, 'start_socat_console', diff --git a/releasenotes/notes/socat-console-port-alloc-ipv6-26760f53f86209d0.yaml b/releasenotes/notes/socat-console-port-alloc-ipv6-26760f53f86209d0.yaml new file mode 100644 index 0000000000..f8087363bd --- /dev/null +++ b/releasenotes/notes/socat-console-port-alloc-ipv6-26760f53f86209d0.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - | + Fixes the issue that port auto allocation for the socat console failed to + correctly identify the availablility of ports under IPv6 networks.