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

@@ -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

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:
+        found_ip = None
-            ifaces = self.get_agent_network_interfaces()
+        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
+            for attempt in range(self.ip_lookup_attempts):
-        while (attempts < self.ip_lookup_attempts):
+                found_ip = self._get_route_source(ironic_host)
-            for iface in ifaces:
+                if found_ip:
-                found_ip = hardware.dispatch_to_managers('get_ipv4_addr',
+                    break
-                                                         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)
 
-        raise errors.LookupAgentIPError('Agent could not find a valid IP '
+                time.sleep(self.ip_lookup_sleep)
-                                        'address.')
 
-    def get_agent_network_interfaces(self):
+        if found_ip:
-        """Get a list of all network interfaces available.
+            self.advertise_address = (found_ip,
-
+                                      self.advertise_address[1])
-        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.')
         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.

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."""
 

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)

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(

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.