From 608003408b3aba6a8290427257460287d8a4ce96 Mon Sep 17 00:00:00 2001 From: armando-migliaccio Date: Tue, 5 Aug 2014 12:35:37 -0700 Subject: [PATCH] Fix AttributeError when setting external gateway on DVR router DVR routers will have this manager initialized only after one or more subnets have been attached to the router. To address the issue, make sure the manager is defined and handle the snat rules appropriately. This patch also makes _update_arp_entry more defensive; this is because the arp update process can be affected by the same issue: the router may not have internal ports at the time the request come in. This is likely when VM's port creation and router configuration overlap slightly. Closes-bug: #1353006 Change-Id: Ib46852f5b264e5ef2e2d499d3351f8974e393011 Co-authored-by: Rajeev Grover --- neutron/agent/l3_agent.py | 22 +++++++++++++--------- neutron/tests/unit/test_l3_agent.py | 23 +++++++++++++++++++++++ 2 files changed, 36 insertions(+), 9 deletions(-) diff --git a/neutron/agent/l3_agent.py b/neutron/agent/l3_agent.py index f632996d232..c2b551421f4 100644 --- a/neutron/agent/l3_agent.py +++ b/neutron/agent/l3_agent.py @@ -258,6 +258,7 @@ class RouterInfo(object): root_helper=root_helper, use_ipv6=use_ipv6, namespace=self.ns_name) + self.snat_iptables_manager = None self.routes = [] # DVR Data # Linklocal subnet for router and floating IP namespace link @@ -951,10 +952,13 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, manager.Manager): # 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 - if ri.router['distributed']: + if not ri.router['distributed']: + iptables_manager = ri.iptables_manager + elif ri.snat_iptables_manager: iptables_manager = ri.snat_iptables_manager else: - iptables_manager = ri.iptables_manager + LOG.debug("DVR router: no snat rules to be handled") + return iptables_manager.ipv4['nat'].empty_chain('POSTROUTING') iptables_manager.ipv4['nat'].empty_chain('snat') @@ -1212,11 +1216,10 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, manager.Manager): self._snat_redirect_add(ri, gateway['fixed_ips'][0] ['ip_address'], p, id_name) - if self.conf.agent_mode == 'dvr_snat' and ( - ri.router['gw_port_host'] == self.host): - if snat_ports: - self._create_dvr_gateway(ri, ex_gw_port, interface_name, - snat_ports) + if (self.conf.agent_mode == 'dvr_snat' and + ri.router['gw_port_host'] == self.host): + self._create_dvr_gateway(ri, ex_gw_port, interface_name, + snat_ports) for port in snat_ports: for ip in port['fixed_ips']: self._update_arp_entry(ri, ip['ip_address'], @@ -1591,9 +1594,10 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback, manager.Manager): self._queue.add(update) def _update_arp_entry(self, ri, ip, mac, subnet_id, operation): - """Add or delete arp entry into router namespace.""" + """Add or delete arp entry into router namespace for the subnet.""" port = self.get_internal_port(ri, subnet_id) - if 'id' in port: + # update arp entry only if the subnet is attached to the router + if port: ip_cidr = str(ip) + '/32' try: # TODO(mrsmith): optimize the calls below for bulk calls diff --git a/neutron/tests/unit/test_l3_agent.py b/neutron/tests/unit/test_l3_agent.py index b10353e89e4..42d0d09ef3f 100644 --- a/neutron/tests/unit/test_l3_agent.py +++ b/neutron/tests/unit/test_l3_agent.py @@ -840,6 +840,16 @@ class TestBasicRouterOperations(base.BaseTestCase): agent.add_arp_entry(None, payload) self.assertFalse(agent._update_arp_entry.called) + def test__update_arp_entry_with_no_subnet(self): + agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) + ri = l3_agent.RouterInfo( + 'foo_router_id', mock.ANY, True, + {'distributed': True, 'gw_port_host': HOSTNAME}) + with mock.patch.object(l3_agent.ip_lib, 'IPDevice') as f: + agent._update_arp_entry(ri, mock.ANY, mock.ANY, + 'foo_subnet_id', 'add') + self.assertFalse(f.call_count) + def test_del_arp_entry(self): agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) router = prepare_router_data(num_internal_ports=2) @@ -1456,6 +1466,19 @@ class TestBasicRouterOperations(base.BaseTestCase): mock.ANY, ri.router_id, {fip_id: l3_constants.FLOATINGIP_STATUS_ERROR}) + def test_handle_router_snat_rules_distributed_without_snat_manager(self): + ri = l3_agent.RouterInfo( + 'foo_router_id', mock.ANY, True, {'distributed': True}) + ri.iptables_manager = mock.Mock() + + agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) + with mock.patch.object(l3_agent.LOG, 'debug') as log_debug: + agent._handle_router_snat_rules( + ri, mock.ANY, mock.ANY, mock.ANY, mock.ANY) + self.assertIsNone(ri.snat_iptables_manager) + self.assertFalse(ri.iptables_manager.called) + self.assertTrue(log_debug.called) + def test_handle_router_snat_rules_add_back_jump(self): agent = l3_agent.L3NATAgent(HOSTNAME, self.conf) ri = mock.MagicMock()