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

diff --git a/quantum/agent/l3_agent.py b/quantum/agent/l3_agent.py
index 1cccb78a8f4014a02d165c6f8aaaaf17f9f3fc71..3b41eb45a61e1d0593831f63874e48330e8bd4ec 100644 (file)
--- a/quantum/agent/l3_agent.py
+++ b/quantum/agent/l3_agent.py
@@ -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):

diff --git a/quantum/tests/unit/test_l3_agent.py b/quantum/tests/unit/test_l3_agent.py
index eeefde451961f102eab2f02cbc299e1d90dbaa67..6eb2f234fb6508683cc74f941149d54ba51f16fa 100644 (file)
--- a/quantum/tests/unit/test_l3_agent.py
+++ b/quantum/tests/unit/test_l3_agent.py
@@ -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