From 6829d34c150fa1cd41064786e76d41dfccef3ef3 Mon Sep 17 00:00:00 2001 From: Dmitry Tantsur Date: Thu, 17 Mar 2016 17:49:36 +0100 Subject: [PATCH] Bind to interface routable to the ironic host, not a random one Binding to the first interface that has an IP address is error-prone: there is no guarantee that ironic can reach us via this inteface. It is much safer to detect the interface facing ironic and bind to it. Unused LookupAgentInterfaceError exception is deleted. The TinyIPA build also requires iptables dependency at build time to insert the required kernel modules. Closes-Bug: #1558956 Change-Id: I9586805e6c7f52a50834bc03efeb72d1faa6cb65 --- imagebuild/tinyipa/build_files/finalreqs.lst | 1 + ironic_python_agent/agent.py | 77 ++++---- ironic_python_agent/errors.py | 9 - ironic_python_agent/tests/unit/test_agent.py | 179 +++++++++++++----- ironic_python_agent/tests/unit/test_errors.py | 1 - .../advertise-address-c3b152fe475fb539.yaml | 5 + 6 files changed, 176 insertions(+), 96 deletions(-) create mode 100644 releasenotes/notes/advertise-address-c3b152fe475fb539.yaml diff --git a/imagebuild/tinyipa/build_files/finalreqs.lst b/imagebuild/tinyipa/build_files/finalreqs.lst index e9356cb70..677bd084a 100644 --- a/imagebuild/tinyipa/build_files/finalreqs.lst +++ b/imagebuild/tinyipa/build_files/finalreqs.lst @@ -3,6 +3,7 @@ coreutils.tcz dmidecode.tcz gdisk.tcz hdparm.tcz +iproute2.tcz parted.tcz python.tcz raid-dm-3.16.6-tinycore64.tcz diff --git a/ironic_python_agent/agent.py b/ironic_python_agent/agent.py index 1716edac1..75a7e8aab 100644 --- a/ironic_python_agent/agent.py +++ b/ironic_python_agent/agent.py @@ -15,11 +15,14 @@ import os import random import select +import socket import threading import time +from oslo_concurrency import processutils from oslo_log import log import pkg_resources +from six.moves.urllib import parse as urlparse from stevedore import extension from wsgiref import simple_server @@ -30,6 +33,7 @@ from ironic_python_agent.extensions import base from ironic_python_agent import hardware from ironic_python_agent import inspector from ironic_python_agent import ironic_api_client +from ironic_python_agent import utils LOG = log.getLogger(__name__) @@ -183,6 +187,21 @@ class IronicPythonAgent(base.ExecuteCommandMixin): version=self.version ) + def _get_route_source(self, dest): + """Get the IP address to send packages to destination.""" + try: + out, _err = utils.execute('ip', 'route', 'get', dest) + except (EnvironmentError, processutils.ProcessExecutionError) as e: + LOG.warning('Cannot get route to host %(dest)s: %(err)s', + {'dest': dest, 'err': e}) + return + + try: + return out.strip().split('\n')[0].split('src')[1].strip() + except IndexError: + LOG.warning('No route to host %(dest)s, route record: %(rec)s', + {'dest': dest, 'rec': out}) + def set_agent_advertise_addr(self): """Set advertised IP address for the agent, if not already set. @@ -190,52 +209,38 @@ class IronicPythonAgent(base.ExecuteCommandMixin): find a better one. If the agent's network interface is None, replace that as well. - :raises: LookupAgentInterfaceError if a valid network interface cannot - be found. :raises: LookupAgentIPError if an IP address could not be found """ if self.advertise_address[0] is not None: return - if self.network_interface is None: - ifaces = self.get_agent_network_interfaces() + found_ip = None + if self.network_interface is not None: + # TODO(dtantsur): deprecate this + found_ip = hardware.dispatch_to_managers('get_ipv4_addr', + self.network_interface) else: - ifaces = [self.network_interface] + url = urlparse.urlparse(self.api_url) + ironic_host = url.hostname + # Try resolving it in case it's not an IP address + try: + ironic_host = socket.gethostbyname(ironic_host) + except socket.gaierror: + LOG.debug('Count not resolve %s, maybe no DNS', ironic_host) - attempts = 0 - while (attempts < self.ip_lookup_attempts): - for iface in ifaces: - found_ip = hardware.dispatch_to_managers('get_ipv4_addr', - iface) - if found_ip is not None: - self.advertise_address = (found_ip, - self.advertise_address[1]) - self.network_interface = iface - return - attempts += 1 - time.sleep(self.ip_lookup_sleep) + for attempt in range(self.ip_lookup_attempts): + found_ip = self._get_route_source(ironic_host) + if found_ip: + break - raise errors.LookupAgentIPError('Agent could not find a valid IP ' - 'address.') + time.sleep(self.ip_lookup_sleep) - def get_agent_network_interfaces(self): - """Get a list of all network interfaces available. - - Excludes loopback connections. - - :returns: list of network interfaces available. - :raises: LookupAgentInterfaceError if a valid interface could not - be found. - """ - iface_list = [iface.serialize()['name'] for iface in - hardware.dispatch_to_managers('list_network_interfaces')] - iface_list = [name for name in iface_list if 'lo' not in name] - - if len(iface_list) == 0: - raise errors.LookupAgentInterfaceError('Agent could not find a ' - 'valid network interface.') + if found_ip: + self.advertise_address = (found_ip, + self.advertise_address[1]) else: - return iface_list + raise errors.LookupAgentIPError('Agent could not find a valid IP ' + 'address.') def get_node_uuid(self): """Get UUID for Ironic node. diff --git a/ironic_python_agent/errors.py b/ironic_python_agent/errors.py index 6fae45857..59ec6400c 100644 --- a/ironic_python_agent/errors.py +++ b/ironic_python_agent/errors.py @@ -136,15 +136,6 @@ class LookupAgentIPError(IronicAPIError): super(LookupAgentIPError, self).__init__(details) -class LookupAgentInterfaceError(IronicAPIError): - """Error raised when agent interface lookup fails.""" - - message = 'Error finding network interface for Ironic Agent' - - def __init__(self, details): - super(LookupAgentInterfaceError, self).__init__(details) - - class ImageDownloadError(RESTError): """Error raised when an image cannot be downloaded.""" diff --git a/ironic_python_agent/tests/unit/test_agent.py b/ironic_python_agent/tests/unit/test_agent.py index 350028332..516e6d53f 100644 --- a/ironic_python_agent/tests/unit/test_agent.py +++ b/ironic_python_agent/tests/unit/test_agent.py @@ -13,9 +13,11 @@ # limitations under the License. import json +import socket import time import mock +from oslo_concurrency import processutils from oslo_config import cfg from oslotest import base as test_base import pkg_resources @@ -28,6 +30,7 @@ from ironic_python_agent import errors from ironic_python_agent.extensions import base from ironic_python_agent import hardware from ironic_python_agent import inspector +from ironic_python_agent import utils EXPECTED_ERROR = RuntimeError('command execution failed') @@ -228,56 +231,6 @@ class TestBaseAgent(test_base.BaseTestCase): self.agent.heartbeater.start.assert_called_once_with() - @mock.patch('os.read') - @mock.patch('select.poll') - @mock.patch('time.sleep', return_value=None) - @mock.patch.object(hardware.GenericHardwareManager, - 'list_network_interfaces') - @mock.patch.object(hardware.GenericHardwareManager, 'get_ipv4_addr') - def test_ipv4_lookup(self, - mock_get_ipv4, - mock_list_net, - mock_time_sleep, - mock_poll, - mock_read): - homeless_agent = agent.IronicPythonAgent('https://fake_api.example.' - 'org:8081/', - (None, 9990), - ('192.0.2.1', 9999), - 3, - 10, - None, - 300, - 1, - 'agent_ipmitool', - False) - - mock_poll.return_value.poll.return_value = True - mock_read.return_value = 'a' - - # Can't find network interfaces, and therefore can't find IP - mock_list_net.return_value = [] - mock_get_ipv4.return_value = None - self.assertRaises(errors.LookupAgentInterfaceError, - homeless_agent.set_agent_advertise_addr) - - # Can look up network interfaces, but not IP. Network interface not - # set, because no interface yields an IP. - mock_ifaces = [hardware.NetworkInterface('eth0', '00:00:00:00:00:00'), - hardware.NetworkInterface('eth1', '00:00:00:00:00:01')] - mock_list_net.return_value = mock_ifaces - - self.assertRaises(errors.LookupAgentIPError, - homeless_agent.set_agent_advertise_addr) - self.assertEqual(6, mock_get_ipv4.call_count) - self.assertIsNone(homeless_agent.network_interface) - - # First interface eth0 has no IP, second interface eth1 has an IP - mock_get_ipv4.side_effect = [None, '1.1.1.1'] - homeless_agent.heartbeater.run() - self.assertEqual(('1.1.1.1', 9990), homeless_agent.advertise_address) - self.assertEqual('eth1', homeless_agent.network_interface) - def test_async_command_success(self): result = base.AsyncCommandResult('foo_command', {'fail': False}, foo_execute) @@ -382,3 +335,129 @@ class TestAgentStandalone(test_base.BaseTestCase): self.assertFalse(self.agent.heartbeater.called) self.assertFalse(self.agent.api_client.lookup_node.called) + + +@mock.patch.object(socket, 'gethostbyname', autospec=True) +@mock.patch.object(utils, 'execute', autospec=True) +class TestAdvertiseAddress(test_base.BaseTestCase): + def setUp(self): + super(TestAdvertiseAddress, self).setUp() + + self.agent = agent.IronicPythonAgent( + api_url='https://fake_api.example.org:8081/', + advertise_address=(None, 9990), + listen_address=('0.0.0.0', 9999), + ip_lookup_attempts=5, + ip_lookup_sleep=10, + network_interface=None, + lookup_timeout=300, + lookup_interval=1, + driver_name='agent_ipmitool', + standalone=False) + + def test_advertise_address_provided(self, mock_exec, mock_gethostbyname): + self.agent.advertise_address = ('1.2.3.4', 9990) + + self.agent.set_agent_advertise_addr() + + self.assertEqual(('1.2.3.4', 9990), self.agent.advertise_address) + self.assertFalse(mock_exec.called) + self.assertFalse(mock_gethostbyname.called) + + @mock.patch.object(hardware.GenericHardwareManager, 'get_ipv4_addr', + autospec=True) + def test_with_network_interface(self, mock_get_ipv4, mock_exec, + mock_gethostbyname): + self.agent.network_interface = 'em1' + mock_get_ipv4.return_value = '1.2.3.4' + + self.agent.set_agent_advertise_addr() + + self.assertEqual(('1.2.3.4', 9990), self.agent.advertise_address) + mock_get_ipv4.assert_called_once_with(mock.ANY, 'em1') + self.assertFalse(mock_exec.called) + self.assertFalse(mock_gethostbyname.called) + + @mock.patch.object(hardware.GenericHardwareManager, 'get_ipv4_addr', + autospec=True) + def test_with_network_interface_failed(self, mock_get_ipv4, mock_exec, + mock_gethostbyname): + self.agent.network_interface = 'em1' + mock_get_ipv4.return_value = None + + self.assertRaises(errors.LookupAgentIPError, + self.agent.set_agent_advertise_addr) + + mock_get_ipv4.assert_called_once_with(mock.ANY, 'em1') + self.assertFalse(mock_exec.called) + self.assertFalse(mock_gethostbyname.called) + + def test_route_with_ip(self, mock_exec, mock_gethostbyname): + self.agent.api_url = 'http://1.2.1.2:8081/v1' + mock_gethostbyname.side_effect = socket.gaierror() + mock_exec.return_value = ( + """1.2.1.2 via 192.168.122.1 dev eth0 src 192.168.122.56 + cache """, + "" + ) + + self.agent.set_agent_advertise_addr() + + self.assertEqual(('192.168.122.56', 9990), + self.agent.advertise_address) + mock_exec.assert_called_once_with('ip', 'route', 'get', '1.2.1.2') + mock_gethostbyname.assert_called_once_with('1.2.1.2') + + def test_route_with_host(self, mock_exec, mock_gethostbyname): + mock_gethostbyname.return_value = '1.2.1.2' + mock_exec.return_value = ( + """1.2.1.2 via 192.168.122.1 dev eth0 src 192.168.122.56 + cache """, + "" + ) + + self.agent.set_agent_advertise_addr() + + self.assertEqual(('192.168.122.56', 9990), + self.agent.advertise_address) + mock_exec.assert_called_once_with('ip', 'route', 'get', '1.2.1.2') + mock_gethostbyname.assert_called_once_with('fake_api.example.org') + + @mock.patch.object(time, 'sleep', autospec=True) + def test_route_retry(self, mock_sleep, mock_exec, mock_gethostbyname): + mock_gethostbyname.return_value = '1.2.1.2' + mock_exec.side_effect = [ + processutils.ProcessExecutionError('boom'), + ( + "Error: some error text", + "" + ), + ( + """1.2.1.2 via 192.168.122.1 dev eth0 src 192.168.122.56 + cache """, + "" + ) + ] + + self.agent.set_agent_advertise_addr() + + self.assertEqual(('192.168.122.56', 9990), + self.agent.advertise_address) + mock_exec.assert_called_with('ip', 'route', 'get', '1.2.1.2') + mock_gethostbyname.assert_called_once_with('fake_api.example.org') + mock_sleep.assert_called_with(10) + self.assertEqual(3, mock_exec.call_count) + self.assertEqual(2, mock_sleep.call_count) + + @mock.patch.object(time, 'sleep', autospec=True) + def test_route_failed(self, mock_sleep, mock_exec, mock_gethostbyname): + mock_gethostbyname.return_value = '1.2.1.2' + mock_exec.side_effect = processutils.ProcessExecutionError('boom') + + self.assertRaises(errors.LookupAgentIPError, + self.agent.set_agent_advertise_addr) + + mock_exec.assert_called_with('ip', 'route', 'get', '1.2.1.2') + mock_gethostbyname.assert_called_once_with('fake_api.example.org') + self.assertEqual(5, mock_exec.call_count) + self.assertEqual(5, mock_sleep.call_count) diff --git a/ironic_python_agent/tests/unit/test_errors.py b/ironic_python_agent/tests/unit/test_errors.py index 778f3a631..a904edb7f 100644 --- a/ironic_python_agent/tests/unit/test_errors.py +++ b/ironic_python_agent/tests/unit/test_errors.py @@ -103,7 +103,6 @@ class TestErrors(test_base.BaseTestCase): (errors.HeartbeatError(DETAILS), SAME_DETAILS), (errors.LookupNodeError(DETAILS), SAME_DETAILS), (errors.LookupAgentIPError(DETAILS), SAME_DETAILS), - (errors.LookupAgentInterfaceError(DETAILS), SAME_DETAILS), (errors.ImageDownloadError('image_id', DETAILS), DIFF_CL_DETAILS), (errors.ImageChecksumError( diff --git a/releasenotes/notes/advertise-address-c3b152fe475fb539.yaml b/releasenotes/notes/advertise-address-c3b152fe475fb539.yaml new file mode 100644 index 000000000..9ed81cf1e --- /dev/null +++ b/releasenotes/notes/advertise-address-c3b152fe475fb539.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - IPA will now advertise IP address via which the Ironic API is routed + instead of using the first available. + See https://bugs.launchpad.net/ironic-python-agent/+bug/1558956.