DVR: Fix issue of SNAT rule for DVR with floating ip

With current code, there are 2 issues.

1) The prevent snat rule that is added for floating ip will be
cleaned, when restarting the l3 agent. Without this rule, the fixed
ip will be SNATed to floating ip, even if the network request is to
an internal IP.

2) The prevent snat rule will not be cleaned, even if the external
device(rfp device) is deleted. So, when the floating ips are removed
from DVR router, there are still dump rules in iptables. Restarting
the l3 agent can clean these dump rules.

The fix in this patch will handle DVR floating ip nat rules at the
same step to handle nat rules for other routers(legacy router, dvr
edge router)

After the change in [1], the fip nat rules for external port have
been extracted together into a method. Add all rules in that method
in the same step can fix the issue of ping floating ip, but reply
with fixed ip.

[1] https://review.openstack.org/#/c/286392/

Change-Id: I018232c03f5df2237a11b48ac877793d1cb5c1bf
Closes-Bug: #1549311
Related-Bug: #1462154

neutron/agent/l3/dvr_edge_router.py

@@ -174,6 +174,9 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter):
         return host == self.host
 
     def _handle_router_snat_rules(self, ex_gw_port, interface_name):
+        super(DvrEdgeRouter, self)._handle_router_snat_rules(
+            ex_gw_port, interface_name)
+
         if not self._is_this_snat_host():
             return
         if not self.get_ex_gw_port():
@@ -186,7 +189,8 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter):
         with self.snat_iptables_manager.defer_apply():
             self._empty_snat_chains(self.snat_iptables_manager)
 
-            # NOTE DVR doesn't add the jump to float snat like the super class.
+            # NOTE: DVR adds the jump to float snat via super class,
+            # but that is in the router namespace and not snat.
 
             self._add_snat_rules(ex_gw_port, self.snat_iptables_manager,
                                  interface_name)

neutron/agent/l3/dvr_fip_ns.py

@@ -269,8 +269,6 @@ class FipNamespace(namespaces.Namespace):
         # add default route for the link local interface
         device = ip_lib.IPDevice(rtr_2_fip_name, namespace=ri.ns_name)
         device.route.add_gateway(str(fip_2_rtr.ip), table=FIP_RT_TBL)
-        #setup the NAT rules and chains
-        ri._handle_fip_nat_rules(rtr_2_fip_name)
 
     def scan_fip_ports(self, ri):
         # don't scan if not dvr or count is not None

neutron/agent/l3/dvr_local_router.py

@@ -55,23 +55,6 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase):
                    (i['host'] == self.host) or
                    (i.get('dest_host') == self.host))]
 
-    def _handle_fip_nat_rules(self, interface_name):
-        """Configures NAT rules for Floating IPs for DVR."""
-        self.iptables_manager.ipv4['nat'].empty_chain('POSTROUTING')
-        self.iptables_manager.ipv4['nat'].empty_chain('snat')
-
-        # Add back the jump to float-snat
-        self.iptables_manager.ipv4['nat'].add_rule('snat', '-j $float-snat')
-
-        # And add the NAT rule back
-        rule = ('POSTROUTING', '! -i %(interface_name)s '
-                '! -o %(interface_name)s -m conntrack ! '
-                '--ctstate DNAT -j ACCEPT' %
-                {'interface_name': interface_name})
-        self.iptables_manager.ipv4['nat'].add_rule(*rule)
-
-        self.iptables_manager.apply()
-
     def floating_ip_added_dist(self, fip, fip_cidr):
         """Add floating IP to FIP namespace."""
         floating_ip = fip['floating_ip_address']
@@ -411,7 +394,40 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase):
             self._snat_redirect_remove(gateway, p, internal_interface)
 
     def _handle_router_snat_rules(self, ex_gw_port, interface_name):
-        pass
+        """Configures NAT rules for Floating IPs for DVR."""
+
+        self.iptables_manager.ipv4['nat'].empty_chain('POSTROUTING')
+        self.iptables_manager.ipv4['nat'].empty_chain('snat')
+        self.iptables_manager.ipv4['mangle'].empty_chain('mark')
+
+        ex_gw_port = self.get_ex_gw_port()
+        if not ex_gw_port:
+            return
+
+        ext_device_name = self.get_external_device_interface_name(ex_gw_port)
+        floatingips = self.get_floating_ips()
+        if not ext_device_name or not floatingips:
+            # Without router to fip device, or without any floating ip,
+            # the snat rules should not be added
+            return
+
+        # We can surely get ipv4 external gateway address, as we have
+        # ex_gw_port and floating ip here
+        ipv4_fixed_ips = [ip for ip in ex_gw_port['fixed_ips']
+                          if netaddr.IPAddress(ip['ip_address']).version == 4]
+        if not ipv4_fixed_ips:
+            return
+        ex_gw_ip = ipv4_fixed_ips[0]['ip_address']
+
+        # Add back the jump to float-snat
+        self.iptables_manager.ipv4['nat'].add_rule('snat', '-j $float-snat')
+
+        rules = self.external_gateway_nat_fip_rules(ex_gw_ip, ext_device_name)
+        for rule in rules:
+            self.iptables_manager.ipv4['nat'].add_rule(*rule)
+        rules = self.external_gateway_mangle_rules(ext_device_name)
+        for rule in rules:
+            self.iptables_manager.ipv4['mangle'].add_rule(*rule)
 
     def _get_address_scope_mark(self):
         # Prepare address scope iptables rule for internal ports

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

@@ -23,6 +23,7 @@ from neutron.agent.l3 import agent as neutron_l3_agent
 from neutron.agent.l3 import dvr_snat_ns
 from neutron.agent.l3 import namespaces
 from neutron.agent.linux import ip_lib
+from neutron.agent.linux import iptables_manager
 from neutron.agent.linux import utils
 from neutron.common import constants as l3_constants
 from neutron.extensions import portbindings
@@ -435,6 +436,59 @@ class TestDvrRouter(framework.L3AgentTestFramework):
             router_ns, floating_ips[0]['fixed_ip_address'])
         self.assertNotEqual(fip_rule_prio_1, fip_rule_prio_2)
 
+    def test_prevent_snat_rule_exist_on_restarted_agent(self):
+        self.agent.conf.agent_mode = 'dvr_snat'
+        router_info = self.generate_dvr_router_info()
+        router = self.manage_router(self.agent, router_info)
+        ext_port = router.get_ex_gw_port()
+        gw_ip = self._port_first_ip_cidr(ext_port).partition('/')[0]
+        rfp_devicename = router.get_external_device_interface_name(ext_port)
+        prevent_snat_rule = router.external_gateway_nat_fip_rules(
+            gw_ip, rfp_devicename)
+
+        def is_prevent_snat_rule_exist(router_iptables_manager):
+            rules = router_iptables_manager.get_rules_for_table('nat')
+            return all(str(iptables_manager.IptablesRule(
+                nat_rule[0], nat_rule[1])) in rules
+                for nat_rule in prevent_snat_rule)
+
+        self.assertTrue(is_prevent_snat_rule_exist(router.iptables_manager))
+
+        restarted_agent = neutron_l3_agent.L3NATAgentWithStateReport(
+            self.agent.host, self.agent.conf)
+        restarted_router = self.manage_router(restarted_agent, router_info)
+
+        self.assertTrue(is_prevent_snat_rule_exist(
+            restarted_router.iptables_manager))
+
+    def test_ping_floatingip_reply_with_floatingip(self):
+        router_info = self.generate_dvr_router_info()
+        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]
+
+        br_int = framework.get_ovs_bridge(
+            self.agent.conf.ovs_integration_bridge)
+
+        src_machine, dst_machine = self.useFixture(
+            machine_fixtures.PeerMachines(
+                br_int,
+                net_helpers.increment_ip_cidr(router_ip_cidr),
+                router_ip)).machines
+
+        dst_fip = '19.4.4.10'
+        router.router[l3_constants.FLOATINGIP_KEY] = []
+        self._add_fip(router, dst_fip,
+                      fixed_address=dst_machine.ip,
+                      host=self.agent.conf.host)
+        router.process(self.agent)
+
+        # 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 _get_fixed_ip_rule_priority(self, namespace, fip):
         iprule = ip_lib.IPRule(namespace)
         lines = iprule.rule._as_root([4], ['show']).splitlines()

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

@@ -1699,15 +1699,13 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
             'foo_router_id',
             {},
             **self.ri_kwargs)
-        ri.iptables_manager = mock.Mock()
+        ri.iptables_manager = mock.MagicMock()
         ri._is_this_snat_host = mock.Mock(return_value=True)
-        ri.get_ex_gw_port = mock.Mock(return_value=mock.ANY)
+        ri.get_ex_gw_port = mock.Mock(return_value=None)
 
-        with mock.patch.object(dvr_router.LOG, 'debug') as log_debug:
-            ri._handle_router_snat_rules(mock.ANY, mock.ANY)
+        ri._handle_router_snat_rules(None, mock.ANY)
         self.assertIsNone(ri.snat_iptables_manager)
         self.assertFalse(ri.iptables_manager.called)
-        self.assertTrue(log_debug.called)
 
     def test_handle_router_snat_rules_add_back_jump(self):
         ri = l3router.RouterInfo(_uuid(), {}, **self.ri_kwargs)