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

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)

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

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

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