Add fip nat rules even if router disables shared snat

For legacy router, there are some iptables rules added for external gateway
port. Some of these rules are for shared snat, some are for floating ip.

When user disables shared snat of router gateway, some of the iptables rules
that floating ip needs will not be added to router. This will cause the
reported bug, ping floating ip but reply with fixed ip.

The fix will add the iptables rules that floating ip needs, no matter if
router enables shared snat. A functional test is also added for the issue.

Change-Id: I3cf4dff90f47a720a2e6a92c9ede2bc067ebd6e7
Closes-Bug: #1551530

neutron/agent/l3/router_info.py

@@ -694,19 +694,12 @@ class RouterInfo(object):
         gw_port = self._router.get('gw_port')
         self._handle_router_snat_rules(gw_port, interface_name)
 
-    def external_gateway_nat_postroute_rules(self, interface_name):
+    def external_gateway_nat_fip_rules(self, ex_gw_ip, interface_name):
         dont_snat_traffic_to_internal_ports_if_not_to_floating_ip = (
             'POSTROUTING', '! -i %(interface_name)s '
                            '! -o %(interface_name)s -m conntrack ! '
                            '--ctstate DNAT -j ACCEPT' %
                            {'interface_name': interface_name})
-        return [dont_snat_traffic_to_internal_ports_if_not_to_floating_ip]
-
-    def external_gateway_nat_snat_rules(self, ex_gw_ip, interface_name):
-        snat_normal_external_traffic = (
-            'snat', '-o %s -j SNAT --to-source %s' %
-                    (interface_name, ex_gw_ip))
-
         # Makes replies come back through the router to reverse DNAT
         ext_in_mark = self.agent_conf.external_ingress_mark
         snat_internal_traffic_to_floating_ip = (
@@ -714,9 +707,15 @@ class RouterInfo(object):
                     '-m conntrack --ctstate DNAT '
                     '-j SNAT --to-source %s'
                     % (ext_in_mark, l3_constants.ROUTER_MARK_MASK, ex_gw_ip))
-        return [snat_normal_external_traffic,
+        return [dont_snat_traffic_to_internal_ports_if_not_to_floating_ip,
                 snat_internal_traffic_to_floating_ip]
 
+    def external_gateway_nat_snat_rules(self, ex_gw_ip, interface_name):
+        snat_normal_external_traffic = (
+            'snat', '-o %s -j SNAT --to-source %s' %
+                    (interface_name, ex_gw_ip))
+        return [snat_normal_external_traffic]
+
     def external_gateway_mangle_rules(self, interface_name):
         mark = self.agent_conf.external_ingress_mark
         mark_packets_entering_external_gateway_port = (
@@ -740,19 +739,20 @@ class RouterInfo(object):
             for ip_addr in ex_gw_port['fixed_ips']:
                 ex_gw_ip = ip_addr['ip_address']
                 if netaddr.IPAddress(ex_gw_ip).version == 4:
-                    rules = self.external_gateway_nat_postroute_rules(
-                        interface_name)
-                    for rule in rules:
-                        iptables_manager.ipv4['nat'].add_rule(*rule)
                     if self._snat_enabled:
                         rules = self.external_gateway_nat_snat_rules(
                             ex_gw_ip, interface_name)
                         for rule in rules:
                             iptables_manager.ipv4['nat'].add_rule(*rule)
-                        rules = self.external_gateway_mangle_rules(
+
-                            interface_name)
+                    rules = self.external_gateway_nat_fip_rules(
-                        for rule in rules:
+                        ex_gw_ip, interface_name)
-                            iptables_manager.ipv4['mangle'].add_rule(*rule)
+                    for rule in rules:
+                        iptables_manager.ipv4['nat'].add_rule(*rule)
+                    rules = self.external_gateway_mangle_rules(interface_name)
+                    for rule in rules:
+                        iptables_manager.ipv4['mangle'].add_rule(*rule)
+
                     break
 
     def _handle_router_snat_rules(self, ex_gw_port, interface_name):

neutron/tests/functional/agent/l3/framework.py

@@ -498,3 +498,14 @@ class L3AgentTestFramework(base.BaseSudoTestCase):
                                         namespace, interface, ip_address):
         self.assertIn(
             ip_address, self._get_addresses_on_device(namespace, interface))
+
+    def _assert_ping_reply_from_expected_address(
+        self, ping_result, expected_address):
+        ping_results = ping_result.split('\n')
+        self.assertGreater(
+            len(ping_results), 1,
+            "The result from ping should be multiple lines")
+        self.assertIn(
+            expected_address, ping_results[1],
+            ("Expect to see %s in the reply of ping, but failed" %
+             expected_address))

neutron/tests/functional/agent/l3/test_legacy_router.py

@@ -193,13 +193,12 @@ class L3AgentTestCase(framework.L3AgentTestFramework):
             routers_deleted=[],
             routers_deleted_during_resync=routers_deleted_during_resync)
 
-    def test_fip_connection_from_same_subnet(self):
+    def _setup_fip_with_fixed_ip_from_same_subnet(self, enable_snat):
-        '''Test connection to floatingip which is associated with
+        """Setup 2 FakeMachines from same subnet, one with floatingip
-           fixed_ip on the same subnet of the source fixed_ip.
+        associated.
-           In other words it confirms that return packets surely
+        """
-           go through the router.
+        router_info = self.generate_router_info(enable_ha=False,
-        '''
+                                                enable_snat=enable_snat)
-        router_info = self.generate_router_info(enable_ha=False)
         router = self.manage_router(self.agent, router_info)
         router_ip_cidr = self._port_first_ip_cidr(router.internal_ports[0])
         router_ip = router_ip_cidr.partition('/')[0]
@@ -218,6 +217,16 @@ class L3AgentTestCase(framework.L3AgentTestFramework):
         self._add_fip(router, dst_fip, fixed_address=dst_machine.ip)
         router.process(self.agent)
 
+        return src_machine, dst_machine, dst_fip
+
+    def test_fip_connection_from_same_subnet(self):
+        '''Test connection to floatingip which is associated with
+           fixed_ip on the same subnet of the source fixed_ip.
+           In other words it confirms that return packets surely
+           go through the router.
+        '''
+        src_machine, dst_machine, dst_fip = (
+            self._setup_fip_with_fixed_ip_from_same_subnet(enable_snat=True))
         protocol_port = net_helpers.get_free_namespace_port(
             l3_constants.PROTO_NAME_TCP, dst_machine.namespace)
         # client sends to fip
@@ -228,6 +237,16 @@ class L3AgentTestCase(framework.L3AgentTestFramework):
         self.addCleanup(netcat.stop_processes)
         self.assertTrue(netcat.test_connectivity())
 
+    def test_ping_floatingip_reply_with_floatingip(self):
+        src_machine, _, dst_fip = (
+            self._setup_fip_with_fixed_ip_from_same_subnet(enable_snat=False))
+
+        # Verify that the ping replys with fip
+        ns_ip_wrapper = ip_lib.IPWrapper(src_machine.namespace)
+        result = ns_ip_wrapper.netns.execute(
+            ['ping', '-c', 1, '-W', 5, dst_fip])
+        self._assert_ping_reply_from_expected_address(result, dst_fip)
+
     def _setup_address_scope(self, internal_address_scope1,
                              internal_address_scope2, gw_address_scope=None):
         router_info = self.generate_router_info(enable_ha=False,

neutron/tests/unit/agent/l3/test_agent.py

@@ -1148,11 +1148,11 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         # IpTablesRule instances
         nat_rules_delta = [r for r in orig_nat_rules
                            if r not in ri.iptables_manager.ipv4['nat'].rules]
-        self.assertEqual(2, len(nat_rules_delta))
+        self.assertEqual(1, len(nat_rules_delta))
         mangle_rules_delta = [
             r for r in orig_mangle_rules
             if r not in ri.iptables_manager.ipv4['mangle'].rules]
-        self.assertEqual(2, len(mangle_rules_delta))
+        self.assertEqual(1, len(mangle_rules_delta))
         self._verify_snat_mangle_rules(nat_rules_delta, mangle_rules_delta,
                                        router)
         self.assertEqual(1, self.send_adv_notif.call_count)
@@ -1175,11 +1175,11 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         # IpTablesRule instances
         nat_rules_delta = [r for r in ri.iptables_manager.ipv4['nat'].rules
                            if r not in orig_nat_rules]
-        self.assertEqual(2, len(nat_rules_delta))
+        self.assertEqual(1, len(nat_rules_delta))
         mangle_rules_delta = [
             r for r in ri.iptables_manager.ipv4['mangle'].rules
             if r not in orig_mangle_rules]
-        self.assertEqual(2, len(mangle_rules_delta))
+        self.assertEqual(1, len(mangle_rules_delta))
         self._verify_snat_mangle_rules(nat_rules_delta, mangle_rules_delta,
                                        router)
         self.assertEqual(1, self.send_adv_notif.call_count)
@@ -1245,15 +1245,15 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         # Get NAT rules with the gw_port
         router['gw_port'] = gw_port
         ri = l3router.RouterInfo(router['id'], router, **self.ri_kwargs)
-        p = ri.external_gateway_nat_postroute_rules
+        p = ri.external_gateway_nat_fip_rules
         s = ri.external_gateway_nat_snat_rules
         attrs_to_mock = dict(
             [(a, mock.DEFAULT) for a in
-                ['external_gateway_nat_postroute_rules',
+                ['external_gateway_nat_fip_rules',
                  'external_gateway_nat_snat_rules']]
         )
         with mock.patch.multiple(ri, **attrs_to_mock) as mocks:
-            mocks['external_gateway_nat_postroute_rules'].side_effect = p
+            mocks['external_gateway_nat_fip_rules'].side_effect = p
             mocks['external_gateway_nat_snat_rules'].side_effect = s
             self._process_router_instance_for_agent(agent, ri, router)
             new_nat_rules = ri.iptables_manager.ipv4['nat'].rules[:]
@@ -1261,13 +1261,13 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
             # NAT rules should only change for dual_stack operation
             if dual_stack:
                 self.assertTrue(
-                    mocks['external_gateway_nat_postroute_rules'].called)
+                    mocks['external_gateway_nat_fip_rules'].called)
                 self.assertTrue(
                     mocks['external_gateway_nat_snat_rules'].called)
                 self.assertNotEqual(orig_nat_rules, new_nat_rules)
             else:
                 self.assertFalse(
-                    mocks['external_gateway_nat_postroute_rules'].called)
+                    mocks['external_gateway_nat_fip_rules'].called)
                 self.assertFalse(
                     mocks['external_gateway_nat_snat_rules'].called)
                 self.assertEqual(orig_nat_rules, new_nat_rules)