openstack/neutron
Code Issues Proposed changes

Add fip nat rules even if router disables shared snat

Browse Source 
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
This commit is contained in:
Hong Hui Xiao 2016-03-01 05:42:42 +00:00
parent 113e59b12e
commit cea149212e
4 changed files with 63 additions and 33 deletions
neutron
agent/l3
router_info.py
tests
functional/agent/l3
framework.pytest_legacy_router.py
unit/agent/l3
test_agent.py

34
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)
for rule in rules:
iptables_manager.ipv4['mangle'].add_rule(*rule)
rules = self.external_gateway_nat_fip_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)
for rule in rules:
iptables_manager.ipv4['mangle'].add_rule(*rule)
break
def _handle_router_snat_rules(self, ex_gw_port, interface_name):

11
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))

33
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):
'''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.
'''
router_info = self.generate_router_info(enable_ha=False)
def _setup_fip_with_fixed_ip_from_same_subnet(self, enable_snat):
"""Setup 2 FakeMachines from same subnet, one with floatingip
associated.
"""
router_info = self.generate_router_info(enable_ha=False,
enable_snat=enable_snat)
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,

18
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)