From d5ebd36a37d65475bf52bbeeda995cc8fabae629 Mon Sep 17 00:00:00 2001 From: Carl Baldwin Date: Thu, 9 Jul 2015 19:19:00 +0000 Subject: [PATCH] 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 | 19 +++++--------- neutron/agent/l3/dvr_local_router.py | 3 +-- neutron/agent/l3/router_info.py | 31 +++++------------------ neutron/tests/unit/agent/l3/test_agent.py | 11 ++++---- 4 files changed, 21 insertions(+), 43 deletions(-) diff --git a/neutron/agent/l3/dvr_edge_router.py b/neutron/agent/l3/dvr_edge_router.py index 72e6412fb1c..b68af5cdecf 100644 --- 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 1336fd2b9a3..805b31ed7f7 100755 --- 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 8bf08f6af31..43a7c1d4575 100644 --- 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 09416ba0a17..7e07022a35f 100644 --- 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