Fix logic for handling SNAT rules

Bug 1192610

Fixes and simplifies the logic for managing SNAT rules, based
on the assumption that a chain contains SNAT rules for a single
router.
It also fixes another small glitch with SNAT rules not being
removed when a gateway port is destroyed (the glitch did not
affect operations)

Change-Id: Ia95e375459a1f32e93bbe912a268a8ed13859c69
This commit is contained in:
Salvatore Orlando 2013-06-25 03:59:26 +02:00
parent 49d7f87279
commit 8e10c8a5a8
2 changed files with 96 additions and 36 deletions

View File

@ -118,13 +118,12 @@ class RouterInfo(object):
return
# Set a SNAT action for the router
if self._router.get('gw_port'):
if (self._router.get('enable_snat') and not self._snat_enabled):
self._snat_action = 'add_rule'
elif (self._snat_enabled and
not self._router.get('enable_snat')):
self._snat_action = 'remove_rule'
self._snat_action = (
'add_rules' if self._router.get('enable_snat')
else 'remove_rules')
elif self.ex_gw_port:
self._snat_action = 'remove_rule'
# Gateway port was removed, remove rules
self._snat_action = 'remove_rules'
self._snat_enabled = self._router.get('enable_snat')
def ns_name(self):
@ -136,7 +135,7 @@ class RouterInfo(object):
if self._snat_action:
snat_callback(self, self._router.get('gw_port'),
*args, action=self._snat_action)
self._snat_action = None
self._snat_action = None
class L3NATAgent(manager.Manager):
@ -331,7 +330,6 @@ class L3NATAgent(manager.Manager):
p['id'] not in existing_port_ids]
old_ports = [p for p in ri.internal_ports if
p['id'] not in current_port_ids]
for p in new_ports:
self._set_subnet_info(p)
ri.internal_ports.append(p)
@ -348,6 +346,7 @@ class L3NATAgent(manager.Manager):
ex_gw_port_id = (ex_gw_port and ex_gw_port['id'] or
ri.ex_gw_port and ri.ex_gw_port['id'])
interface_name = None
if ex_gw_port_id:
interface_name = self.get_external_device_name(ex_gw_port_id)
if ex_gw_port and not ri.ex_gw_port:
@ -359,9 +358,8 @@ class L3NATAgent(manager.Manager):
interface_name, internal_cidrs)
# Process SNAT rules for external gateway
if ex_gw_port_id:
ri.perform_snat_action(self._handle_router_snat_rules,
internal_cidrs, interface_name)
ri.perform_snat_action(self._handle_router_snat_rules,
internal_cidrs, interface_name)
# Process DNAT rules for floating IPs
if ex_gw_port or ri.ex_gw_port:
@ -373,13 +371,20 @@ class L3NATAgent(manager.Manager):
def _handle_router_snat_rules(self, ri, ex_gw_port, internal_cidrs,
interface_name, action):
ex_gw_ip = ex_gw_port['fixed_ips'][0]['ip_address']
for rule in self.external_gateway_nat_rules(ex_gw_ip,
internal_cidrs,
interface_name):
# This is an internal method so we can assume the caller
# knows which actions are valid and which not
getattr(ri.iptables_manager.ipv4['nat'], action)(*rule)
# Remove all the rules
# This is safe because if use_namespaces is set as False
# then the agent can only configure one router, otherwise
# each router's SNAT rules will be in their own namespace
ri.iptables_manager.ipv4['nat'].empty_chain('POSTROUTING')
ri.iptables_manager.ipv4['nat'].empty_chain('snat')
# And add them back if the action if add_rules
if action == 'add_rules':
# ex_gw_port should not be None in this case
ex_gw_ip = ex_gw_port['fixed_ips'][0]['ip_address']
for rule in self.external_gateway_nat_rules(ex_gw_ip,
internal_cidrs,
interface_name):
ri.iptables_manager.ipv4['nat'].add_rule(*rule)
ri.iptables_manager.apply()
def process_router_floating_ips(self, ri, ex_gw_port):

View File

@ -330,7 +330,7 @@ class TestBasicRouterOperations(base.BaseTestCase):
'via', '10.100.10.30']]
self._check_agent_method_called(agent, expected, namespace)
def _verify_snat_rules(self, rules, router):
def _verify_snat_rules(self, rules, router, negate=False):
interfaces = router[l3_constants.INTERFACE_KEY]
source_cidrs = []
for interface in interfaces:
@ -349,9 +349,12 @@ class TestBasicRouterOperations(base.BaseTestCase):
expected_rules.append('-s %(source_cidr)s -j SNAT --to-source '
'%(source_nat_ip)s' % value_dict)
for r in rules:
self.assertIn(r.rule, expected_rules)
if negate:
self.assertNotIn(r.rule, expected_rules)
else:
self.assertIn(r.rule, expected_rules)
def _prepare_router_data(self, enable_snat=True):
def _prepare_router_data(self, enable_snat=True, num_internal_ports=1):
router_id = _uuid()
ex_gw_port = {'id': _uuid(),
'network_id': _uuid(),
@ -359,18 +362,20 @@ class TestBasicRouterOperations(base.BaseTestCase):
'subnet_id': _uuid()}],
'subnet': {'cidr': '19.4.4.0/24',
'gateway_ip': '19.4.4.1'}}
internal_port = {'id': _uuid(),
'network_id': _uuid(),
'admin_state_up': True,
'fixed_ips': [{'ip_address': '35.4.4.4',
'subnet_id': _uuid()}],
'mac_address': 'ca:fe:de:ad:be:ef',
'subnet': {'cidr': '35.4.4.0/24',
'gateway_ip': '35.4.4.1'}}
int_ports = []
for i in range(0, num_internal_ports):
int_ports.append({'id': _uuid(),
'network_id': _uuid(),
'admin_state_up': True,
'fixed_ips': [{'ip_address': '35.4.%s.4' % i,
'subnet_id': _uuid()}],
'mac_address': 'ca:fe:de:ad:be:ef',
'subnet': {'cidr': '35.4.%s.0/24' % i,
'gateway_ip': '35.4.%s.1' % i}})
router = {
'id': router_id,
l3_constants.INTERFACE_KEY: [internal_port],
l3_constants.INTERFACE_KEY: int_ports,
'enable_snat': enable_snat,
'routes': [],
'gw_port': ex_gw_port}
@ -405,7 +410,6 @@ class TestBasicRouterOperations(base.BaseTestCase):
agent.process_router(ri)
def test_process_router_snat_disabled(self):
agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
router = self._prepare_router_data()
ri = l3_agent.RouterInfo(router['id'], self.conf.root_helper,
@ -418,13 +422,14 @@ class TestBasicRouterOperations(base.BaseTestCase):
# Reassign the router object to RouterInfo
ri.router = router
agent.process_router(ri)
nat_rules_delta = (set(orig_nat_rules) -
set(ri.iptables_manager.ipv4['nat'].rules))
# For some reason set logic does not work well with
# IpTablesRule instances
nat_rules_delta = [r for r in orig_nat_rules
if r not in ri.iptables_manager.ipv4['nat'].rules]
self.assertEqual(len(nat_rules_delta), 2)
self._verify_snat_rules(nat_rules_delta, router)
def test_process_router_snat_enabled(self):
agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
router = self._prepare_router_data(enable_snat=False)
ri = l3_agent.RouterInfo(router['id'], self.conf.root_helper,
@ -437,11 +442,61 @@ class TestBasicRouterOperations(base.BaseTestCase):
# Reassign the router object to RouterInfo
ri.router = router
agent.process_router(ri)
nat_rules_delta = (set(ri.iptables_manager.ipv4['nat'].rules) -
set(orig_nat_rules))
# For some reason set logic does not work well with
# IpTablesRule instances
nat_rules_delta = [r for r in ri.iptables_manager.ipv4['nat'].rules
if r not in orig_nat_rules]
self.assertEqual(len(nat_rules_delta), 2)
self._verify_snat_rules(nat_rules_delta, router)
def test_process_router_interface_added(self):
agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
router = self._prepare_router_data()
ri = l3_agent.RouterInfo(router['id'], self.conf.root_helper,
self.conf.use_namespaces, router=router)
# Process with NAT
agent.process_router(ri)
orig_nat_rules = ri.iptables_manager.ipv4['nat'].rules[:]
# Add an interface and reprocess
router[l3_constants.INTERFACE_KEY].append(
{'id': _uuid(),
'network_id': _uuid(),
'admin_state_up': True,
'fixed_ips': [{'ip_address': '35.4.1.4',
'subnet_id': _uuid()}],
'mac_address': 'ca:fe:de:ad:be:ef',
'subnet': {'cidr': '35.4.1.0/24',
'gateway_ip': '35.4.1.1'}})
# Reassign the router object to RouterInfo
ri.router = router
agent.process_router(ri)
# For some reason set logic does not work well with
# IpTablesRule instances
nat_rules_delta = [r for r in ri.iptables_manager.ipv4['nat'].rules
if r not in orig_nat_rules]
self.assertEqual(len(nat_rules_delta), 1)
self._verify_snat_rules(nat_rules_delta, router)
def test_process_router_interface_removed(self):
agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
router = self._prepare_router_data(num_internal_ports=2)
ri = l3_agent.RouterInfo(router['id'], self.conf.root_helper,
self.conf.use_namespaces, router=router)
# Process with NAT
agent.process_router(ri)
orig_nat_rules = ri.iptables_manager.ipv4['nat'].rules[:]
# Add an interface and reprocess
del router[l3_constants.INTERFACE_KEY][1]
# Reassign the router object to RouterInfo
ri.router = router
agent.process_router(ri)
# For some reason set logic does not work well with
# IpTablesRule instances
nat_rules_delta = [r for r in orig_nat_rules
if r not in ri.iptables_manager.ipv4['nat'].rules]
self.assertEqual(len(nat_rules_delta), 1)
self._verify_snat_rules(nat_rules_delta, router, negate=True)
def testRoutersWithAdminStateDown(self):
agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
self.plugin_api.get_external_network_id.return_value = None