Privsep the ebtables modification code.

The same pattern as previous changes, although with some slightly
bigger unit test fallout.

Change-Id: Iaa59f346030680f7dc38cf17c907b31d2292a837
Co-Authored-By: Stephen Finucane <[email protected]>

Author: mikalstill

nova/network/linux_net.py

@@ -1491,30 +1491,27 @@ def _exec_ebtables(table, rule, insert_rule=True, check_exit_code=True):
         #            other error (like a rule doesn't exist) so we have to
         #            to parse stderr.
         try:
-            cmd = ['ebtables', '--concurrent', '-t', table]
-            if insert_rule:
-                cmd.append('-I')
-            else:
-                cmd.append('-D')
-            cmd.extend(rule)
-
-            _execute(*cmd, check_exit_code=[0], run_as_root=True)
+            nova.privsep.linux_net.modify_ebtables(table, rule,
+                                                   insert_rule=insert_rule)
         except processutils.ProcessExecutionError as exc:
             # See if we can retry the error.
             if any(error in exc.stderr for error in retry_strings):
                 if count > attempts and check_exit_code:
-                    LOG.warning('%s failed. Not Retrying.', ' '.join(cmd))
+                    LOG.warning('Rule edit for %s failed. Not Retrying.',
+                                table)
                     raise
                 else:
                     # We need to sleep a bit before retrying
-                    LOG.warning("%(cmd)s failed. Sleeping %(time)s "
-                                "seconds before retry.",
-                                {'cmd': ' '.join(cmd), 'time': sleep})
+                    LOG.warning('Rule edit for %(table)s failed. '
+                                'Sleeping %(time)s seconds before retry.',
+                                {'table': table, 'time': sleep})
                     time.sleep(sleep)
             else:
                 # Not eligible for retry
                 if check_exit_code:
-                    LOG.warning('%s failed. Not Retrying.', ' '.join(cmd))
+                    LOG.warning('Rule edit for %s failed and not eligible '
+                                'for retry.',
+                                table)
                     raise
                 else:
                     return

nova/privsep/linux_net.py

@@ -247,3 +247,15 @@ def ipv4_forwarding_check():
 @nova.privsep.sys_admin_pctxt.entrypoint
 def _enable_ipv4_forwarding_inner():
     processutils.execute('sysctl', '-w', 'net.ipv4.ip_forward=1')
+
+
+@nova.privsep.sys_admin_pctxt.entrypoint
+def modify_ebtables(table, rule, insert_rule=True):
+    cmd = ['ebtables', '--concurrent', '-t', table]
+    if insert_rule:
+        cmd.append('-I')
+    else:
+        cmd.append('-D')
+    cmd.extend(rule)
+
+    processutils.execute(*cmd, check_exit_code=[0])

nova/tests/unit/network/test_linux_net.py

@@ -17,7 +17,6 @@
 import datetime
 import os
 import re
-import time
 
 import mock
 import netifaces
@@ -793,7 +792,8 @@ def test_dnsmasq_execute_use_network_dns_servers(self):
         ]
         self._test_dnsmasq_execute(extra_expected=expected)
 
-    def test_isolated_host(self):
+    @mock.patch('nova.privsep.linux_net.modify_ebtables')
+    def test_isolated_host(self, mock_modify_ebtables):
         self.flags(fake_network=False,
                    share_dhcp_address=True)
         executes = []
@@ -824,34 +824,47 @@ def fake_ensure(bridge, interface, network, gateway):
         driver.plug(network, 'fakemac')
 
         expected = [
-            ('ebtables', '--concurrent', '-t', 'filter', '-D', 'INPUT', '-p',
-             'ARP', '-i', iface, '--arp-ip-dst', dhcp, '-j', 'DROP'),
-            ('ebtables', '--concurrent', '-t', 'filter', '-I', 'INPUT', '-p',
-             'ARP', '-i', iface, '--arp-ip-dst', dhcp, '-j', 'DROP'),
-            ('ebtables', '--concurrent', '-t', 'filter', '-D', 'OUTPUT', '-p',
-             'ARP', '-o', iface, '--arp-ip-src', dhcp, '-j', 'DROP'),
-            ('ebtables', '--concurrent', '-t', 'filter', '-I', 'OUTPUT', '-p',
-             'ARP', '-o', iface, '--arp-ip-src', dhcp, '-j', 'DROP'),
-            ('ebtables', '--concurrent', '-t', 'filter', '-D', 'FORWARD',
-             '-p', 'IPv4', '-i', iface, '--ip-protocol', 'udp',
-             '--ip-destination-port', '67:68', '-j', 'DROP'),
-            ('ebtables', '--concurrent', '-t', 'filter', '-I', 'FORWARD',
-             '-p', 'IPv4', '-i', iface, '--ip-protocol', 'udp',
-             '--ip-destination-port', '67:68', '-j', 'DROP'),
-            ('ebtables', '--concurrent', '-t', 'filter', '-D', 'FORWARD',
-             '-p', 'IPv4', '-o', iface, '--ip-protocol', 'udp',
-             '--ip-destination-port', '67:68', '-j', 'DROP'),
-            ('ebtables', '--concurrent', '-t', 'filter', '-I', 'FORWARD',
-             '-p', 'IPv4', '-o', iface, '--ip-protocol', 'udp',
-             '--ip-destination-port', '67:68', '-j', 'DROP'),
             ('iptables-save', '-c'),
             ('iptables-restore', '-c'),
             ('ip6tables-save', '-c'),
             ('ip6tables-restore', '-c'),
         ]
         self.assertEqual(expected, executes)
-
-        executes = []
+        mock_modify_ebtables.assert_has_calls([
+            mock.call('filter',
+                      ['INPUT', '-p', 'ARP', '-i', iface, '--arp-ip-dst',
+                       dhcp, '-j', 'DROP'],
+                      insert_rule=False),
+            mock.call('filter',
+                      ['INPUT', '-p', 'ARP', '-i', iface, '--arp-ip-dst',
+                       dhcp, '-j', 'DROP'],
+                      insert_rule=True),
+            mock.call('filter',
+                      ['OUTPUT', '-p', 'ARP', '-o', iface, '--arp-ip-src',
+                       dhcp, '-j', 'DROP'],
+                      insert_rule=False),
+            mock.call('filter',
+                      ['OUTPUT', '-p', 'ARP', '-o', iface, '--arp-ip-src',
+                       dhcp, '-j', 'DROP'],
+                      insert_rule=True),
+            mock.call('filter',
+                      ['FORWARD', '-p', 'IPv4', '-i', iface, '--ip-protocol',
+                       'udp', '--ip-destination-port', '67:68', '-j', 'DROP'],
+                      insert_rule=False),
+            mock.call('filter',
+                      ['FORWARD', '-p', 'IPv4', '-i', iface, '--ip-protocol',
+                       'udp', '--ip-destination-port', '67:68', '-j', 'DROP'],
+                      insert_rule=True),
+            mock.call('filter',
+                      ['FORWARD', '-p', 'IPv4', '-o', iface, '--ip-protocol',
+                       'udp', '--ip-destination-port', '67:68', '-j', 'DROP'],
+                      insert_rule=False),
+            mock.call('filter',
+                      ['FORWARD', '-p', 'IPv4', '-o', iface, '--ip-protocol',
+                       'udp', '--ip-destination-port', '67:68', '-j', 'DROP'],
+                      insert_rule=True)])
+
+        mock_modify_ebtables.reset_mock()
 
         def fake_remove(bridge, gateway):
             return
@@ -861,19 +874,23 @@ def fake_remove(bridge, gateway):
             fake_remove)
 
         driver.unplug(network)
-        expected = [
-            ('ebtables', '--concurrent', '-t', 'filter', '-D', 'INPUT', '-p',
-             'ARP', '-i', iface, '--arp-ip-dst', dhcp, '-j', 'DROP'),
-            ('ebtables', '--concurrent', '-t', 'filter', '-D', 'OUTPUT', '-p',
-             'ARP', '-o', iface, '--arp-ip-src', dhcp, '-j', 'DROP'),
-            ('ebtables', '--concurrent', '-t', 'filter', '-D', 'FORWARD',
-             '-p', 'IPv4', '-i', iface, '--ip-protocol', 'udp',
-             '--ip-destination-port', '67:68', '-j', 'DROP'),
-            ('ebtables', '--concurrent', '-t', 'filter', '-D', 'FORWARD',
-             '-p', 'IPv4', '-o', iface, '--ip-protocol', 'udp',
-             '--ip-destination-port', '67:68', '-j', 'DROP'),
-        ]
-        self.assertEqual(expected, executes)
+        mock_modify_ebtables.assert_has_calls([
+            mock.call('filter',
+                      ['INPUT', '-p', 'ARP', '-i', iface, '--arp-ip-dst',
+                       dhcp, '-j', 'DROP'],
+                      insert_rule=False),
+            mock.call('filter',
+                      ['OUTPUT', '-p', 'ARP', '-o', iface, '--arp-ip-src',
+                       dhcp, '-j', 'DROP'],
+                      insert_rule=False),
+            mock.call('filter',
+                      ['FORWARD', '-p', 'IPv4', '-i', iface, '--ip-protocol',
+                       'udp', '--ip-destination-port', '67:68', '-j', 'DROP'],
+                      insert_rule=False),
+            mock.call('filter',
+                      ['FORWARD', '-p', 'IPv4', '-o', iface, '--ip-protocol',
+                       'udp', '--ip-destination-port', '67:68', '-j', 'DROP'],
+                      insert_rule=False)])
 
     @mock.patch('nova.privsep.linux_net.routes_show')
     @mock.patch('nova.privsep.linux_net.route_delete')
@@ -1211,85 +1228,66 @@ def fake_execute(*cmd, **kwargs):
             driver.ensure_bridge('brq1234567-89', '')
             device_exists.assert_called_once_with('brq1234567-89')
 
-    def test_exec_ebtables_success(self):
-        executes = []
-
-        def fake_execute(*args, **kwargs):
-            executes.append(args)
-            return "", ""
-
-        with mock.patch.object(self.driver, '_execute',
-                               side_effect=fake_execute):
-            self.driver._exec_ebtables('fake', ['fake'])
-            self.assertEqual(1, len(executes))
-
-    def _ebtables_race_stderr(self):
-        return (u"Unable to update the kernel. Two possible causes:\n"
-                "1. Multiple ebtables programs were executing simultaneously."
-                " The ebtables\n userspace tool doesn't by default support "
-                "multiple ebtables programs running\n concurrently. The "
-                "ebtables option --concurrent or a tool like flock can be\n "
-                "used to support concurrent scripts that update the ebtables "
-                "kernel tables.\n2. The kernel doesn't support a certain "
-                "ebtables extension, consider\n recompiling your kernel or "
-                "insmod the extension.\n.\n")
-
-    def test_exec_ebtables_fail_all(self):
-        executes = []
-
-        def fake_sleep(interval):
-            pass
-
-        def fake_execute(*args, **kwargs):
-            executes.append(args)
-            raise processutils.ProcessExecutionError('error',
-                    stderr=self._ebtables_race_stderr())
-
-        with mock.patch.object(time, 'sleep', side_effect=fake_sleep), \
-                mock.patch.object(self.driver, '_execute',
-                                  side_effect=fake_execute):
-            self.assertRaises(processutils.ProcessExecutionError,
-                              self.driver._exec_ebtables, 'fake', ['fake'])
-            max_calls = CONF.ebtables_exec_attempts
-            self.assertEqual(max_calls, len(executes))
-
-    def test_exec_ebtables_fail_no_retry(self):
-        executes = []
-
-        def fake_sleep(interval):
-            pass
-
-        def fake_execute(*args, **kwargs):
-            executes.append(args)
-            raise processutils.ProcessExecutionError('error',
-                    stderr="Sorry, rule does not exist")
-
-        with mock.patch.object(time, 'sleep', side_effect=fake_sleep), \
-                mock.patch.object(self.driver, '_execute',
-                              side_effect=fake_execute):
-            self.assertRaises(processutils.ProcessExecutionError,
-                              self.driver._exec_ebtables, 'fake', 'fake')
-            self.assertEqual(1, len(executes))
-
-    def test_exec_ebtables_fail_once(self):
-        executes = []
-
-        def fake_sleep(interval):
-            pass
-
-        def fake_execute(*args, **kwargs):
-            executes.append(args)
-            if len(executes) == 1:
-                raise processutils.ProcessExecutionError('error',
-                        stderr=self._ebtables_race_stderr())
-            else:
-                return "", ""
-
-        with mock.patch.object(time, 'sleep', side_effect=fake_sleep), \
-                mock.patch.object(self.driver, '_execute',
-                                  side_effect=fake_execute):
-            self.driver._exec_ebtables('fake', 'fake')
-            self.assertEqual(2, len(executes))
+    @mock.patch('nova.privsep.linux_net.modify_ebtables',
+                return_value=('', ''))
+    def test_exec_ebtables_success(self, mock_modify_ebtables):
+        self.driver._exec_ebtables('fake', 'fake')
+        mock_modify_ebtables.assert_called()
+
+    @mock.patch('nova.privsep.linux_net.modify_ebtables',
+                side_effect=processutils.ProcessExecutionError(
+                    'error',
+                    stderr=(u'Unable to update the kernel. Two possible '
+                            'causes:\n1. Multiple ebtables programs were '
+                            'executing simultaneously. The ebtables\n '
+                            'userspace tool doesn\'t by default support '
+                            'multiple ebtables programs running\n '
+                            'concurrently. The ebtables option --concurrent '
+                            'or a tool like flock can be\n used to support '
+                            'concurrent scripts that update the ebtables '
+                            'kernel tables.\n2. The kernel doesn\'t support '
+                            'a certain ebtables extension, consider\n '
+                            'recompiling your kernel or insmod the '
+                            'extension.\n.\n')))
+    @mock.patch('time.sleep')
+    def test_exec_ebtables_fail_all(self, mock_sleep, mock_modify_ebtables):
+        self.flags(ebtables_exec_attempts=5)
+        self.assertRaises(processutils.ProcessExecutionError,
+                          self.driver._exec_ebtables, 'fake', 'fake')
+        self.assertEqual(5, mock_modify_ebtables.call_count)
+
+    @mock.patch('nova.privsep.linux_net.modify_ebtables',
+                side_effect=processutils.ProcessExecutionError(
+                    'error',
+                    stderr=(u'Sorry, rule does not exist')))
+    @mock.patch('time.sleep')
+    def test_exec_ebtables_fail_no_retry(self, mock_sleep,
+                                         mock_modify_ebtables):
+        self.assertRaises(processutils.ProcessExecutionError,
+                          self.driver._exec_ebtables, 'fake', 'fake')
+        mock_modify_ebtables.assert_called()
+
+    @mock.patch('nova.privsep.linux_net.modify_ebtables',
+                side_effect=[
+                    processutils.ProcessExecutionError(
+                        'error',
+                        stderr=(u'Unable to update the kernel. Two possible '
+                                'causes:\n1. Multiple ebtables programs were '
+                                'executing simultaneously. The ebtables\n '
+                                'userspace tool doesn\'t by default support '
+                                'multiple ebtables programs running\n '
+                                'concurrently. The ebtables option '
+                                '--concurrent or a tool like flock can be\n '
+                                'used to support concurrent scripts that '
+                                'update the ebtables kernel tables.\n2. The '
+                                'kernel doesn\'t support a certain ebtables '
+                                'extension, consider\n recompiling your '
+                                'kernel or insmod the extension.\n.\n')),
+                    ('', '')])
+    @mock.patch('time.sleep')
+    def test_exec_ebtables_fail_once(self, mock_sleep, mock_modify_ebtables):
+        self.driver._exec_ebtables('fake', 'fake')
+        self.assertEqual(2, mock_modify_ebtables.call_count)
 
     @mock.patch('os.path.exists', return_value=True)
     @mock.patch('nova.privsep.linux_net.set_device_disabled')

nova/tests/unit/network/test_manager.py

@@ -884,7 +884,9 @@ def setUp(self):
     @mock.patch('nova.objects.floating_ip.FloatingIPList.get_by_host')
     @mock.patch('nova.network.linux_net.iptables_manager._apply')
     @mock.patch('nova.privsep.linux_net.bind_ip')
-    def test_init_host_iptables_defer_apply(self, bind_ip, iptable_apply,
+    @mock.patch('nova.privsep.linux_net.modify_ebtables')
+    def test_init_host_iptables_defer_apply(self, modify_ebtables,
+                                            bind_ip, iptable_apply,
                                             floating_get_by_host,
                                             fixed_get_by_id):
         def get_by_id(context, fixed_ip_id, **kwargs):
@@ -1770,8 +1772,10 @@ def test_add_fixed_ip_instance_without_vpn_requested_networks(
     @mock.patch('nova.privsep.linux_net.bind_ip')
     @mock.patch('nova.privsep.linux_net.unbind_ip')
     @mock.patch('nova.privsep.linux_net.clean_conntrack')
+    @mock.patch('nova.privsep.linux_net.modify_ebtables')
     def test_ip_association_and_allocation_of_other_project(
-            self, clean_conntrack, unbind_ip, bind_ip, net_get, fixed_get):
+            self, modify_ebtables, clean_conntrack, unbind_ip, bind_ip,
+            net_get, fixed_get):
         """Makes sure that we cannot deallocaate or disassociate
         a public IP of other project.
         """
@@ -2089,7 +2093,9 @@ def test_get_networks_by_uuids_ordering(self):
     @mock.patch('nova.objects.floating_ip.FloatingIPList.get_by_host')
     @mock.patch('nova.network.linux_net.iptables_manager._apply')
     @mock.patch('nova.privsep.linux_net.bind_ip')
-    def test_init_host_iptables_defer_apply(self, bind_ip, iptable_apply,
+    @mock.patch('nova.privsep.linux_net.modify_ebtables')
+    def test_init_host_iptables_defer_apply(self, modify_ebtables, bind_ip,
+                                            iptable_apply,
                                             floating_get_by_host,
                                             fixed_get_by_id):
         def get_by_id(context, fixed_ip_id, **kwargs):
@@ -2887,7 +2893,9 @@ def setUp(self):
     @mock.patch('nova.privsep.linux_net.ipv4_forwarding_check',
                 return_value=False)
     @mock.patch('nova.privsep.linux_net._enable_ipv4_forwarding_inner')
-    def test_allocate_for_instance(self, mock_forwarding_enable,
+    @mock.patch('nova.privsep.linux_net.modify_ebtables')
+    def test_allocate_for_instance(self, mock_modify_ebtables,
+                                   mock_forwarding_enable,
                                    mock_forwarding_check,
                                    mock_clean_conntrack,
                                    mock_address_command,
@@ -3133,7 +3141,10 @@ def test_double_deallocation(self):
 
     @mock.patch('nova.privsep.linux_net.unbind_ip')
     @mock.patch('nova.privsep.linux_net.clean_conntrack')
-    def test_deallocation_deleted_instance(self, mock_clean_conntrack,
+    @mock.patch('nova.privsep.linux_net.modify_ebtables')
+    def test_deallocation_deleted_instance(self,
+                                           mock_modify_ebtables,
+                                           mock_clean_conntrack,
                                            mock_unbind_ip):
         self.stubs.Set(self.network, '_teardown_network_on_host',
                        lambda *args, **kwargs: None)
@@ -3156,7 +3167,10 @@ def test_deallocation_deleted_instance(self, mock_clean_conntrack,
 
     @mock.patch('nova.privsep.linux_net.unbind_ip')
     @mock.patch('nova.privsep.linux_net.clean_conntrack')
-    def test_deallocation_duplicate_floating_ip(self, mock_clean_conntrack,
+    @mock.patch('nova.privsep.linux_net.modify_ebtables')
+    def test_deallocation_duplicate_floating_ip(self,
+                                                mock_modify_ebtables,
+                                                mock_clean_conntrack,
                                                 mock_unbind_ip):
         self.stubs.Set(self.network, '_teardown_network_on_host',
                        lambda *args, **kwargs: None)