diff --git a/quantum/agent/l3_agent.py b/quantum/agent/l3_agent.py index 1cccb78a8f4..3b41eb45a61 100644 --- 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 eeefde45196..6eb2f234fb6 100644 --- 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