Privsepify ipv4 forwarding enablement.

Follows the usual pattern, but with some nice cleanups to unit test
code as we get closer to the end of this journey.

Change-Id: Ie10b2f171842d19e55fbad3d751660e11bfce2cb

Author: mikalstill

nova/network/linux_net.py

@@ -736,19 +736,12 @@ def floating_forward_rules(floating_ip, fixed_ip, device):
     return rules
 
 
-def _enable_ipv4_forwarding():
-    sysctl_key = 'net.ipv4.ip_forward'
-    stdout, stderr = _execute('sysctl', '-n', sysctl_key)
-    if stdout.strip() is not '1':
-        _execute('sysctl', '-w', '%s=1' % sysctl_key, run_as_root=True)
-
-
 @utils.synchronized('lock_gateway', external=True)
 def initialize_gateway_device(dev, network_ref):
     if not network_ref:
         return
 
-    _enable_ipv4_forwarding()
+    nova.privsep.linux_net.enable_ipv4_forwarding()
 
     # NOTE(vish): The ip for dnsmasq has to be the first address on the
     #             bridge for it to respond to requests properly

nova/privsep/linux_net.py

@@ -232,3 +232,18 @@ def clean_conntrack(fixed_ip):
                              check_exit_code=[0, 1])
     except processutils.ProcessExecutionError:
         LOG.exception('Error deleting conntrack entries for %s', fixed_ip)
+
+
+def enable_ipv4_forwarding():
+    if not ipv4_forwarding_check():
+        _enable_ipv4_forwarding_inner()
+
+
+def ipv4_forwarding_check():
+    with open('/proc/sys/net/ipv4/ip_forward', 'r') as f:
+        return f.readline().strip() == '1'
+
+
+@nova.privsep.sys_admin_pctxt.entrypoint
+def _enable_ipv4_forwarding_inner():
+    processutils.execute('sysctl', '-w', 'net.ipv4.ip_forward=1')

nova/tests/functional/api_sample_tests/api_sample_base.py

@@ -126,6 +126,10 @@ def setUp(self):
         # NOTE(mikal): this is used to stub away privsep helpers
         def fake_noop(*args, **kwargs):
             return '', ''
+
+        def fake_true(*args, **kwargs):
+            return True
+
         self.stub_out('nova.privsep.linux_net.add_bridge', fake_noop)
         self.stub_out('nova.privsep.linux_net.set_device_mtu', fake_noop)
         self.stub_out('nova.privsep.linux_net.set_device_enabled', fake_noop)
@@ -135,6 +139,10 @@ def fake_noop(*args, **kwargs):
         self.stub_out('nova.privsep.linux_net.change_ip', fake_noop)
         self.stub_out('nova.privsep.linux_net.address_command_deprecated',
                       fake_noop)
+        self.stub_out('nova.privsep.linux_net.ipv4_forwarding_check',
+                      fake_true)
+        self.stub_out('nova.privsep.linux_net._enable_ipv4_forwarding_inner',
+                      fake_noop)
 
         if self.availability_zones:
             self.useFixture(

nova/tests/unit/network/test_linux_net.py

@@ -875,31 +875,23 @@ def fake_remove(bridge, gateway):
         ]
         self.assertEqual(expected, executes)
 
-    @mock.patch.object(utils, 'execute')
     @mock.patch('nova.privsep.linux_net.routes_show')
     @mock.patch('nova.privsep.linux_net.route_delete')
     @mock.patch('nova.privsep.linux_net.route_add_deprecated')
     @mock.patch('nova.privsep.linux_net.lookup_ip')
     @mock.patch('nova.privsep.linux_net.change_ip')
     @mock.patch('nova.privsep.linux_net.address_command_deprecated')
-    def _test_initialize_gateway(self, existing, expected,
+    def _test_initialize_gateway(self, existing,
                                  mock_address_command, mock_change_ip,
                                  mock_lookup_ip, mock_route_add,
                                  mock_route_delete, mock_routes,
-                                 mock_execute, routes='',
+                                 routes='',
                                  routes_show_called=True, deleted_routes=None,
                                  added_routes=None, changed_interfaces=None,
                                  address_commands=None):
         self.flags(fake_network=False)
         mock_lookup_ip.return_value = (existing, '')
-        executes = []
-
-        def fake_execute(*args, **kwargs):
-            executes.append(args)
-            if args[0] == 'sysctl':
-                return '1\n', ''
 
-        mock_execute.side_effect = fake_execute
         mock_routes.return_value = (routes, '')
         mock_lookup_ip.return_value = (existing, '')
 
@@ -908,8 +900,6 @@ def fake_execute(*args, **kwargs):
                    'broadcast': '192.168.1.255',
                    'cidr_v6': '2001:db8::/64'}
         self.driver.initialize_gateway_device('eth0', network)
-        self.assertEqual(expected, executes)
-        self.assertTrue(mock_execute.called)
         self.assertTrue(mock_lookup_ip.called)
 
         if routes_show_called:
@@ -923,18 +913,17 @@ def fake_execute(*args, **kwargs):
         if address_commands:
             mock_address_command.assert_has_calls(address_commands)
 
-    def test_initialize_gateway_moves_wrong_ip(self):
+    @mock.patch('nova.privsep.linux_net.ipv4_forwarding_check',
+                return_value=True)
+    def test_initialize_gateway_moves_wrong_ip(self, mock_forwarding_check):
         existing = ("2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> "
             "    mtu 1500 qdisc pfifo_fast state UNKNOWN qlen 1000\n"
             "    link/ether de:ad:be:ef:be:ef brd ff:ff:ff:ff:ff:ff\n"
             "    inet 192.168.0.1/24 brd 192.168.0.255 scope global eth0\n"
             "    inet6 dead::beef:dead:beef:dead/64 scope link\n"
             "    valid_lft forever preferred_lft forever\n")
-        expected = [
-            ('sysctl', '-n', 'net.ipv4.ip_forward'),
-        ]
         self._test_initialize_gateway(
-            existing, expected,
+            existing,
             changed_interfaces=[mock.call('eth0', '2001:db8::/64')],
             address_commands=[
                 mock.call('eth0', 'del', ['192.168.0.1/24', 'brd',
@@ -947,19 +936,19 @@ def test_initialize_gateway_moves_wrong_ip(self):
                                           'scope', 'global'])]
         )
 
-    def test_initialize_gateway_ip_with_dynamic_flag(self):
+    @mock.patch('nova.privsep.linux_net.ipv4_forwarding_check',
+                return_value=True)
+    def test_initialize_gateway_ip_with_dynamic_flag(self,
+                                                     mock_forwarding_check):
         existing = ("2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> "
             "    mtu 1500 qdisc pfifo_fast state UNKNOWN qlen 1000\n"
             "    link/ether de:ad:be:ef:be:ef brd ff:ff:ff:ff:ff:ff\n"
             "    inet 192.168.0.1/24 brd 192.168.0.255 scope global "
             "dynamic eth0\n"
             "    inet6 dead::beef:dead:beef:dead/64 scope link\n"
             "    valid_lft forever preferred_lft forever\n")
-        expected = [
-            ('sysctl', '-n', 'net.ipv4.ip_forward'),
-        ]
         self._test_initialize_gateway(
-            existing, expected,
+            existing,
             changed_interfaces=[mock.call('eth0', '2001:db8::/64')],
             address_commands=[
                 mock.call('eth0', 'del',
@@ -972,7 +961,9 @@ def test_initialize_gateway_ip_with_dynamic_flag(self):
                            'scope', 'global'])]
         )
 
-    def test_initialize_gateway_resets_route(self):
+    @mock.patch('nova.privsep.linux_net.ipv4_forwarding_check',
+                return_value=True)
+    def test_initialize_gateway_resets_route(self, mock_forwarding_check):
         routes = ("default via 192.168.0.1 dev eth0\n"
                   "192.168.100.0/24 via 192.168.0.254 dev eth0 proto static\n")
         existing = ("2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> "
@@ -981,11 +972,8 @@ def test_initialize_gateway_resets_route(self):
             "    inet 192.168.0.1/24 brd 192.168.0.255 scope global eth0\n"
             "    inet6 dead::beef:dead:beef:dead/64 scope link\n"
             "    valid_lft forever preferred_lft forever\n")
-        expected = [
-            ('sysctl', '-n', 'net.ipv4.ip_forward'),
-        ]
         self._test_initialize_gateway(
-            existing, expected, routes=routes,
+            existing, routes=routes,
             deleted_routes=[mock.call('eth0', 'default'),
                             mock.call('eth0', '192.168.100.0/24')],
             added_routes=[mock.call(['default', 'via', '192.168.0.1',
@@ -1005,33 +993,31 @@ def test_initialize_gateway_resets_route(self):
                            'scope', 'global'])]
         )
 
-    def test_initialize_gateway_no_move_right_ip(self):
+    @mock.patch('nova.privsep.linux_net.ipv4_forwarding_check',
+                return_value=True)
+    def test_initialize_gateway_no_move_right_ip(self, mock_forwarding_check):
         existing = ("2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> "
             "    mtu 1500 qdisc pfifo_fast state UNKNOWN qlen 1000\n"
             "    link/ether de:ad:be:ef:be:ef brd ff:ff:ff:ff:ff:ff\n"
             "    inet 192.168.1.1/24 brd 192.168.1.255 scope global eth0\n"
             "    inet 192.168.0.1/24 brd 192.168.0.255 scope global eth0\n"
             "    inet6 dead::beef:dead:beef:dead/64 scope link\n"
             "    valid_lft forever preferred_lft forever\n")
-        expected = [
-            ('sysctl', '-n', 'net.ipv4.ip_forward'),
-        ]
         self._test_initialize_gateway(
-            existing, expected,
-            routes_show_called=False,
+            existing, routes_show_called=False,
             changed_interfaces=[mock.call('eth0', '2001:db8::/64')])
+        mock_forwarding_check.assert_called()
 
-    def test_initialize_gateway_add_if_blank(self):
+    @mock.patch('nova.privsep.linux_net.ipv4_forwarding_check',
+                return_value=True)
+    def test_initialize_gateway_add_if_blank(self, mock_forwarding_check):
         existing = ("2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> "
             "    mtu 1500 qdisc pfifo_fast state UNKNOWN qlen 1000\n"
             "    link/ether de:ad:be:ef:be:ef brd ff:ff:ff:ff:ff:ff\n"
             "    inet6 dead::beef:dead:beef:dead/64 scope link\n"
             "    valid_lft forever preferred_lft forever\n")
-        expected = [
-            ('sysctl', '-n', 'net.ipv4.ip_forward'),
-        ]
         self._test_initialize_gateway(
-            existing, expected,
+            existing,
             changed_interfaces=[mock.call('eth0', '2001:db8::/64')],
             address_commands=[
                 mock.call('eth0', 'add',

nova/tests/unit/network/test_manager.py

@@ -942,7 +942,12 @@ def test_quota_driver_type(self):
     @mock.patch('nova.privsep.linux_net.lookup_ip', return_value=('', ''))
     @mock.patch('nova.privsep.linux_net.change_ip')
     @mock.patch('nova.privsep.linux_net.address_command_deprecated')
-    def test_vpn_allocate_fixed_ip(self, mock_address_command,
+    @mock.patch('nova.privsep.linux_net.ipv4_forwarding_check',
+                return_value=False)
+    @mock.patch('nova.privsep.linux_net._enable_ipv4_forwarding_inner')
+    def test_vpn_allocate_fixed_ip(self, mock_forwarding_enable,
+                                   mock_forwarding_check,
+                                   mock_address_command,
                                    mock_change_ip, mock_lookup_ip,
                                    mock_routes_show, mock_enabled,
                                    mock_add_bridge):
@@ -984,8 +989,13 @@ def test_vpn_allocate_fixed_ip(self, mock_address_command,
     @mock.patch('nova.privsep.linux_net.lookup_ip', return_value=('', ''))
     @mock.patch('nova.privsep.linux_net.change_ip')
     @mock.patch('nova.privsep.linux_net.address_command_deprecated')
-    def test_allocate_fixed_ip(self, mock_address_command, mock_change_ip,
-                               mock_lookup_ip, mock_routes_show, mock_enabled,
+    @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_fixed_ip(self, mock_forwarding_enable,
+                               mock_forwarding_check, mock_address_command,
+                               mock_change_ip, mock_lookup_ip,
+                               mock_routes_show, mock_enabled,
                                mock_add_bridge):
         self.stubs.Set(self.network,
                 '_do_trigger_security_group_members_refresh_for_instance',
@@ -1710,8 +1720,12 @@ def fake8(*args, **kwargs):
     @mock.patch('nova.privsep.linux_net.lookup_ip', return_value=('', ''))
     @mock.patch('nova.privsep.linux_net.change_ip')
     @mock.patch('nova.privsep.linux_net.address_command_deprecated')
+    @mock.patch('nova.privsep.linux_net.ipv4_forwarding_check',
+                return_value=False)
+    @mock.patch('nova.privsep.linux_net._enable_ipv4_forwarding_inner')
     def test_add_fixed_ip_instance_without_vpn_requested_networks(
-            self, mock_address_command, mock_change_ip, mock_lookup_ip,
+            self, mock_forwarding_enable, mock_forwarding_check,
+            mock_address_command, mock_change_ip, mock_lookup_ip,
             mock_routes_show, mock_enabled, mock_add_bridge):
         self.stubs.Set(self.network,
                 '_do_trigger_security_group_members_refresh_for_instance',
@@ -2870,7 +2884,12 @@ def setUp(self):
     @mock.patch('nova.privsep.linux_net.change_ip')
     @mock.patch('nova.privsep.linux_net.clean_conntrack')
     @mock.patch('nova.privsep.linux_net.address_command_deprecated')
-    def test_allocate_for_instance(self, mock_clean_conntrack,
+    @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_forwarding_check,
+                                   mock_clean_conntrack,
                                    mock_address_command,
                                    mock_change_ip, mock_lookup_ip,
                                    mock_routes_show, mock_unbind, mock_bind,
@@ -2947,7 +2966,12 @@ def test_allocate_for_instance_illegal_network(self):
     @mock.patch('nova.privsep.linux_net.lookup_ip', return_value=('', ''))
     @mock.patch('nova.privsep.linux_net.change_ip')
     @mock.patch('nova.privsep.linux_net.address_command_deprecated')
-    def test_allocate_for_instance_with_mac(self, mock_address_command,
+    @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_with_mac(self, mock_forwarding_enable,
+                                            mock_forwarding_check,
+                                            mock_address_command,
                                             mock_change_ip,
                                             mock_lookup_ip,
                                             mock_routes_show,

nova/tests/unit/privsep/test_linux_net.py

@@ -115,3 +115,20 @@ def test_create_tap_dev_multiqueue_tunctl_raises(self, mock_execute):
         self.assertRaises(processutils.ProcessExecutionError,
                           nova.privsep.linux_net.create_tap_dev,
                           'tap42', multiqueue=True)
+
+    @mock.patch('nova.privsep.linux_net.ipv4_forwarding_check',
+                return_value=False)
+    @mock.patch('oslo_concurrency.processutils.execute')
+    def test_enable_ipv4_forwarding_required(self, mock_execute, mock_check):
+        nova.privsep.linux_net.enable_ipv4_forwarding()
+        mock_check.assert_called_once()
+        mock_execute.assert_called_once_with(
+            'sysctl', '-w', 'net.ipv4.ip_forward=1')
+
+    @mock.patch('nova.privsep.linux_net.ipv4_forwarding_check',
+                return_value=True)
+    @mock.patch('oslo_concurrency.processutils.execute')
+    def test_enable_ipv4_forwarding_redundant(self, mock_execute, mock_check):
+        nova.privsep.linux_net.enable_ipv4_forwarding()
+        mock_check.assert_called_once()
+        mock_execute.assert_not_called()

nova/tests/unit/virt/xenapi/test_xenapi.py

@@ -1141,7 +1141,12 @@ def test_spawn_injects_auto_disk_config_to_xenstore(
     @mock.patch('nova.privsep.linux_net.set_device_macaddr')
     @mock.patch('nova.privsep.linux_net.change_ip')
     @mock.patch('nova.privsep.linux_net.address_command_deprecated')
-    def test_spawn_vlanmanager(self, mock_address_command_deprecated,
+    @mock.patch('nova.privsep.linux_net.ipv4_forwarding_check',
+                return_value=False)
+    @mock.patch('nova.privsep.linux_net._enable_ipv4_forwarding_inner')
+    def test_spawn_vlanmanager(self, mock_forwarding_enable,
+                               mock_forwarding_check,
+                               mock_address_command_horrid,
                                mock_change_ip, mock_set_macaddr,
                                mock_set_enabled, mock_set_mtu, mock_add_bridge,
                                mock_create_vifs):