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 <rajeev.grover@hp.com>

diff --git a/neutron/agent/l3_agent.py b/neutron/agent/l3_agent.py
index f632996d23208f87f8447ca61fcdd21071fdfdc7..c2b551421f416a2f9aabfbdc1a9192dded3df645 100644 (file)
--- 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 b10353e89e4198252e5b42869fec20f4da2db503..42d0d09ef3fa72bfa148852b7115fee7def850b5 100644 (file)
--- 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()