From: Yalei Wang <yalei.wang@intel.com>
Date: Wed, 21 Jan 2015 09:48:56 +0000 (+0800)
Subject: Move the assignment of existing_floating_ips before try block
X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=b117e8ec854dd0a26d04beb7e1effedffec3cb3a;p=openstack-build%2Fneutron-build.git

Move the assignment of existing_floating_ips before try block

In function _process_external() in agent/l3/agent.py, the call to
iptables_manager.defer_apply() may throw an exception, making a
later call to _update_fip_statuses() use an un-initialized value.
This will throw its own UnboundLocalError, with the result being
no iptables rules will be applied.

Added tests to cover both the defer_apply() code exception
processing, as well as this new case where we might jump to
_update_fip_statuses() without having done any work on floating
IP addresses.

Change-Id: I0045effc9319772127758be4aacca02ab5c236cd
Closes-Bug: #1413111
---

diff --git a/neutron/agent/l3/agent.py b/neutron/agent/l3/agent.py
index 4935614bc..642efca82 100644
--- a/neutron/agent/l3/agent.py
+++ b/neutron/agent/l3/agent.py
@@ -533,6 +533,7 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback,
                 ri.disable_keepalived()
 
     def _process_external(self, ri):
+        existing_floating_ips = ri.floating_ips
         try:
             with ri.iptables_manager.defer_apply():
                 self._process_external_gateway(ri)
@@ -543,7 +544,6 @@ class L3NATAgent(firewall_l3_agent.FWaaSL3AgentRpcCallback,
                     return
 
                 # Process SNAT/DNAT rules and addresses for floating IPs
-                existing_floating_ips = ri.floating_ips
                 if ri.router['distributed']:
                     self.create_dvr_fip_interfaces(ri, ex_gw_port)
                 ri.process_snat_dnat_for_fip()
diff --git a/neutron/agent/linux/iptables_manager.py b/neutron/agent/linux/iptables_manager.py
index 5adf1d227..9efbd3879 100644
--- a/neutron/agent/linux/iptables_manager.py
+++ b/neutron/agent/linux/iptables_manager.py
@@ -384,8 +384,9 @@ class IptablesManager(object):
             try:
                 self.defer_apply_off()
             except Exception:
-                raise n_exc.IpTablesApplyException('Failure applying ip '
-                                                   'tables rules')
+                msg = _LE('Failure applying iptables rules')
+                LOG.exception(msg)
+                raise n_exc.IpTablesApplyException(msg)
 
     def defer_apply_on(self):
         self.iptables_apply_deferred = True
diff --git a/neutron/tests/unit/test_iptables_manager.py b/neutron/tests/unit/test_iptables_manager.py
index e0017a459..564e46186 100644
--- a/neutron/tests/unit/test_iptables_manager.py
+++ b/neutron/tests/unit/test_iptables_manager.py
@@ -18,10 +18,12 @@ import sys
 
 import mock
 from oslo_config import cfg
+import testtools
 
 from neutron.agent.common import config as a_cfg
 from neutron.agent.linux import iptables_comments as ic
 from neutron.agent.linux import iptables_manager
+from neutron.common import exceptions as n_exc
 from neutron.tests import base
 from neutron.tests import tools
 
@@ -247,6 +249,12 @@ class IptablesManagerStateFulTestCase(base.BaseTestCase):
         self.assertEqual(iptables_manager.get_chain_name(name, wrap=True),
                          name[:11])
 
+    def test_defer_apply_with_exception(self):
+        self.iptables._apply = mock.Mock(side_effect=Exception)
+        with testtools.ExpectedException(n_exc.IpTablesApplyException):
+            with self.iptables.defer_apply():
+                pass
+
     def _extend_with_ip6tables_filter(self, expected_calls, filter_dump):
         expected_calls.insert(2, (
             mock.call(['ip6tables-save', '-c'],
diff --git a/neutron/tests/unit/test_l3_agent.py b/neutron/tests/unit/test_l3_agent.py
index 200febe44..0bccd1d6f 100644
--- a/neutron/tests/unit/test_l3_agent.py
+++ b/neutron/tests/unit/test_l3_agent.py
@@ -1290,6 +1290,30 @@ class TestBasicRouterOperations(BasicRouterOperationsFramework):
                 mock.ANY, ri.router_id,
                 {fip_id: l3_constants.FLOATINGIP_STATUS_ERROR})
 
+    def test_process_external_iptables_exception(self):
+        agent = l3_agent.L3NATAgent(HOSTNAME, self.conf)
+        with mock.patch.object(
+            agent.plugin_rpc,
+            'update_floatingip_statuses') as mock_update_fip_status:
+            fip_id = _uuid()
+            router = prepare_router_data(num_internal_ports=1)
+            router[l3_constants.FLOATINGIP_KEY] = [
+                {'id': fip_id,
+                 'floating_ip_address': '8.8.8.8',
+                 'fixed_ip_address': '7.7.7.7',
+                 'port_id': router[l3_constants.INTERFACE_KEY][0]['id']}]
+
+            ri = l3router.RouterInfo(router['id'], router, **self.ri_kwargs)
+            ri.iptables_manager._apply = mock.Mock(side_effect=Exception)
+            agent._process_external(ri)
+            # Assess the call for putting the floating IP into Error
+            # was performed
+            mock_update_fip_status.assert_called_once_with(
+                mock.ANY, ri.router_id,
+                {fip_id: l3_constants.FLOATINGIP_STATUS_ERROR})
+
+            self.assertEqual(ri.iptables_manager._apply.call_count, 1)
+
     def test_handle_router_snat_rules_distributed_without_snat_manager(self):
         ri = dvr_router.DvrRouter(
             HOSTNAME,