Remove perform_snat_action indirection

This indirection seems complicated to me.  I don't know the history
behind it but it made some of the address scope work more difficult
than I think it needs to be.

Change-Id: I1e716135b542c09ec852f6ab7af5153a65803ba3
Partially-Implements: blueprint address-scopes

diff --git a/neutron/agent/l3/dvr_edge_router.py b/neutron/agent/l3/dvr_edge_router.py
index 72e6412fb1cdfd0cb9f918c3b55407de2d1831f8..b68af5cdecfbbf350e19a49dac2dafda3ec3f25f 100644 (file)
--- a/neutron/agent/l3/dvr_edge_router.py
+++ b/neutron/agent/l3/dvr_edge_router.py
@@ -149,8 +149,12 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter):
                       self.router['id'])
         return host == self.host
 
-    def _handle_router_snat_rules(self, ex_gw_port,
-                                  interface_name, action):
+    def _handle_router_snat_rules(self, ex_gw_port, interface_name):
+        if not self._is_this_snat_host():
+            return
+        if not self.get_ex_gw_port():
+            return
+
         if not self.snat_iptables_manager:
             LOG.debug("DVR router: no snat rules to be handled")
             return
@@ -161,13 +165,4 @@ class DvrEdgeRouter(dvr_local_router.DvrLocalRouter):
             # NOTE DVR doesn't add the jump to float snat like the super class.
 
             self._add_snat_rules(ex_gw_port, self.snat_iptables_manager,
-                                 interface_name, action)
-
-    def perform_snat_action(self, snat_callback, *args):
-        # NOTE DVR skips this step in a few cases...
-        if not self.get_ex_gw_port():
-            return
-        if not self._is_this_snat_host():
-            return
-
-        super(DvrEdgeRouter, self).perform_snat_action(snat_callback, *args)
+                                 interface_name)

diff --git a/neutron/agent/l3/dvr_local_router.py b/neutron/agent/l3/dvr_local_router.py
index 1336fd2b9a3f3467409cdb2f7c1110b6fdac162d..805b31ed7f79c60c82fa36b01cd1a872c0d7547b 100755 (executable)
--- a/neutron/agent/l3/dvr_local_router.py
+++ b/neutron/agent/l3/dvr_local_router.py
@@ -357,8 +357,7 @@ class DvrLocalRouter(dvr_router_base.DvrRouterBase):
             internal_interface = self.get_internal_device_name(p['id'])
             self._snat_redirect_remove(gateway, p, internal_interface)
 
-    def _handle_router_snat_rules(self, ex_gw_port,
-                                  interface_name, action):
+    def _handle_router_snat_rules(self, ex_gw_port, interface_name):
         pass
 
     def process_external(self, agent):

diff --git a/neutron/agent/l3/router_info.py b/neutron/agent/l3/router_info.py
index 8bf08f6af314c9d6ec0bb465989ff57774b6d8aa..43a7c1d457550851fbe725ffc1ee7be1c6933d76 100644 (file)
--- a/neutron/agent/l3/router_info.py
+++ b/neutron/agent/l3/router_info.py
@@ -45,7 +45,6 @@ class RouterInfo(object):
         self.router_id = router_id
         self.ex_gw_port = None
         self._snat_enabled = None
-        self._snat_action = None
         self.internal_ports = []
         self.floating_ips = set()
         # Invoke the setter for establishing initial SNAT action
@@ -97,13 +96,6 @@ class RouterInfo(object):
             return
         # enable_snat by default if it wasn't specified by plugin
         self._snat_enabled = self._router.get('enable_snat', True)
-        # Set a SNAT action for the router
-        if self._router.get('gw_port'):
-            self._snat_action = ('add_rules' if self._snat_enabled
-                                 else 'remove_rules')
-        elif self.ex_gw_port:
-            # Gateway port was removed, remove rules
-            self._snat_action = 'remove_rules'
 
     @property
     def is_ha(self):
@@ -119,14 +111,6 @@ class RouterInfo(object):
     def get_external_device_interface_name(self, ex_gw_port):
         return self.get_external_device_name(ex_gw_port['id'])
 
-    def perform_snat_action(self, snat_callback, *args):
-        # Process SNAT rules for attached subnets
-        if self._snat_action:
-            snat_callback(self._router.get('gw_port'),
-                          *args,
-                          action=self._snat_action)
-        self._snat_action = None
-
     def _update_routing_table(self, operation, route):
         cmd = ['ip', 'route', operation, 'to', route['destination'],
                'via', route['nexthop']]
@@ -534,8 +518,8 @@ class RouterInfo(object):
                                prefix=EXTERNAL_DEV_PREFIX)
 
         # Process SNAT rules for external gateway
-        self.perform_snat_action(self._handle_router_snat_rules,
-                                 interface_name)
+        gw_port = self._router.get('gw_port')
+        self._handle_router_snat_rules(gw_port, interface_name)
 
     def external_gateway_nat_rules(self, ex_gw_ip, interface_name):
         mark = self.agent_conf.external_ingress_mark
@@ -562,8 +546,8 @@ class RouterInfo(object):
         iptables_manager.ipv4['mangle'].empty_chain('mark')
 
     def _add_snat_rules(self, ex_gw_port, iptables_manager,
-                        interface_name, action):
-        if action == 'add_rules' and ex_gw_port:
+                        interface_name):
+        if self._snat_enabled and ex_gw_port:
             # ex_gw_port should not be None in this case
             # NAT rules are added only if ex_gw_port has an IPv4 address
             for ip_addr in ex_gw_port['fixed_ips']:
@@ -578,16 +562,14 @@ class RouterInfo(object):
                         iptables_manager.ipv4['mangle'].add_rule(*rule)
                     break
 
-    def _handle_router_snat_rules(self, ex_gw_port,
-                                  interface_name, action):
+    def _handle_router_snat_rules(self, ex_gw_port, interface_name):
         self._empty_snat_chains(self.iptables_manager)
 
         self.iptables_manager.ipv4['nat'].add_rule('snat', '-j $float-snat')
 
         self._add_snat_rules(ex_gw_port,
                              self.iptables_manager,
-                             interface_name,
-                             action)
+                             interface_name)
 
     def process_external(self, agent):
         existing_floating_ips = self.floating_ips
@@ -633,4 +615,5 @@ class RouterInfo(object):
 
         # Update ex_gw_port and enable_snat on the router info cache
         self.ex_gw_port = self.get_ex_gw_port()
+        # TODO(Carl) FWaaS uses this.  Why is it set after processing is done?
         self.enable_snat = self.router.get('enable_snat')

diff --git a/neutron/tests/unit/agent/l3/test_agent.py b/neutron/tests/unit/agent/l3/test_agent.py
index 09416ba0a176d7dacf0755a683a7f571f69e8244..7e07022a35fa0e29a047852f01c0c75a3085d2c7 100644 (file)
--- a/neutron/tests/unit/agent/l3/test_agent.py
+++ b/neutron/tests/unit/agent/l3/test_agent.py
@@ -1489,10 +1489,11 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
             {},
             **self.ri_kwargs)
         ri.iptables_manager = mock.Mock()
+        ri._is_this_snat_host = mock.Mock(return_value=True)
+        ri.get_ex_gw_port = mock.Mock(return_value=mock.ANY)
 
-        with mock.patch.object(dvr_router.LOG,
-                               'debug') as log_debug:
-            ri._handle_router_snat_rules(mock.ANY, mock.ANY, mock.ANY)
+        with mock.patch.object(dvr_router.LOG, 'debug') as log_debug:
+            ri._handle_router_snat_rules(mock.ANY, mock.ANY)
         self.assertIsNone(ri.snat_iptables_manager)
         self.assertFalse(ri.iptables_manager.called)
         self.assertTrue(log_debug.called)
@@ -1502,7 +1503,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         ri.iptables_manager = mock.MagicMock()
         port = {'fixed_ips': [{'ip_address': '192.168.1.4'}]}
 
-        ri._handle_router_snat_rules(port, "iface", "add_rules")
+        ri._handle_router_snat_rules(port, "iface")
 
         nat = ri.iptables_manager.ipv4['nat']
         nat.empty_chain.assert_any_call('snat')
@@ -1518,7 +1519,7 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
         ri = l3router.RouterInfo(_uuid(), {}, **self.ri_kwargs)
         ex_gw_port = {'fixed_ips': [{'ip_address': '192.168.1.4'}]}
         ri.router = {'distributed': False}
-        ri._handle_router_snat_rules(ex_gw_port, "iface", "add_rules")
+        ri._handle_router_snat_rules(ex_gw_port, "iface")
 
         nat_rules = map(str, ri.iptables_manager.ipv4['nat'].rules)
         wrap_name = ri.iptables_manager.wrap_name