From 10532404b5f28ac9489bc300e1c9a94aa5ad86c0 Mon Sep 17 00:00:00 2001 From: Hong Hui Xiao Date: Mon, 29 Feb 2016 11:07:15 +0000 Subject: [PATCH] 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/ Conflicts: neutron/agent/l3/dvr_fip_ns.py neutron/tests/functional/agent/l3/test_dvr_router.py Change-Id: I018232c03f5df2237a11b48ac877793d1cb5c1bf Closes-Bug: #1549311 Related-Bug: #1462154 (cherry picked from commit 1cea77b0aafbada6cad89a6fe0f5450004aef4e1) --- neutron/agent/l3/dvr_edge_router.py | 6 ++- neutron/agent/l3/dvr_fip_ns.py | 2 - neutron/agent/l3/dvr_local_router.py | 52 +++++++++++------- .../functional/agent/l3/test_dvr_router.py | 54 +++++++++++++++++++ neutron/tests/unit/agent/l3/test_agent.py | 8 ++- 5 files changed, 96 insertions(+), 26 deletions(-) diff --git a/neutron/agent/l3/dvr_edge_router.py b/neutron/agent/l3/dvr_edge_router.py index 0719b9fabfd..dc285e2d282 100644 --- a/neutron/agent/l3/dvr_edge_router.py +++ b/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) diff --git a/neutron/agent/l3/dvr_fip_ns.py b/neutron/agent/l3/dvr_fip_ns.py index 63f81701664..3bd335283c7 100644 --- a/neutron/agent/l3/dvr_fip_ns.py +++ b/neutron/agent/l3/dvr_fip_ns.py @@ -274,8 +274,6 @@ class FipNamespace(namespaces.Namespace): # add default route for the link local interface rtr_2_fip_dev.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 diff --git a/neutron/agent/l3/dvr_local_router.py b/neutron/agent/l3/dvr_local_router.py index 3f8c940ac71..2104881561e 100644 --- a/neutron/agent/l3/dvr_local_router.py +++ b/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'] @@ -429,7 +412,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 diff --git a/neutron/tests/functional/agent/l3/test_dvr_router.py b/neutron/tests/functional/agent/l3/test_dvr_router.py index 254876fdc74..b244812691f 100644 --- a/neutron/tests/functional/agent/l3/test_dvr_router.py +++ b/neutron/tests/functional/agent/l3/test_dvr_router.py @@ -24,6 +24,7 @@ from neutron.agent.l3 import dvr_fip_ns 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 @@ -521,6 +522,59 @@ class TestDvrRouter(framework.L3AgentTestFramework): self.assertFalse(self._fixed_ip_rule_exists(router_ns, fixed_ip)) self.assertTrue(self._fixed_ip_rule_exists(router_ns, new_fixed_ip)) + 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() diff --git a/neutron/tests/unit/agent/l3/test_agent.py b/neutron/tests/unit/agent/l3/test_agent.py index fea47a4c436..ee2d2964808 100644 --- a/neutron/tests/unit/agent/l3/test_agent.py +++ b/neutron/tests/unit/agent/l3/test_agent.py @@ -1700,15 +1700,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)