Correct inconsistent enable_snat management

Neutron resets enable_snat attribute when external_gateway_info is
cleared but not when external_gateway_info is only updated which
implies the following sets of actions have different behaviors:

  neutron router-gateway-set router1 pub1 --disable-snat
  neutron router-gateway-set router1 pub2

enable_snat is False after the last command

  neutron router-gateway-set router1 pub1 --disable-snat
  neutron router-gateway-clear router1
  neutron router-gateway-set router1 pub2

enable_snat is True after the 2nd command resets the gateway AND
enable_snat.

This change proposes to always reset the attribute enable_snat when
enable_snat is not provided in external_gateway_info on POST/PUT for
consistency.

APIImpact
Change-Id: Ibab289936c55b1cf9614b44a4f18f54c959ee9e8
Closes-Bug: #1384146

diff --git a/neutron/db/l3_gwmode_db.py b/neutron/db/l3_gwmode_db.py
index 6d4b3306180ff16c0842a73143dd2e2a4498e943..558adff15eb241719ba57090ba437a21a1c8c927 100644 (file)
--- a/neutron/db/l3_gwmode_db.py
+++ b/neutron/db/l3_gwmode_db.py
@@ -55,9 +55,8 @@ class L3_NAT_dbonly_mixin(l3_db.L3_NAT_dbonly_mixin):
         # Load the router only if necessary
         if not router:
             router = self._get_router(context, router_id)
-        # if enable_snat is not specified use the value
-        # stored in the database (default:True)
-        enable_snat = not info or info.get('enable_snat', router.enable_snat)
+        # if enable_snat is not specified then use the default value (True)
+        enable_snat = not info or info.get('enable_snat', True)
         with context.session.begin(subtransactions=True):
             router.enable_snat = enable_snat
 

diff --git a/neutron/tests/unit/extensions/test_l3_ext_gw_mode.py b/neutron/tests/unit/extensions/test_l3_ext_gw_mode.py
index 291c88fcb66309f40724362c670f240a4c2c1085..cba57a6bf84483d3e78785545eb16ebe55906e31 100644 (file)
--- a/neutron/tests/unit/extensions/test_l3_ext_gw_mode.py
+++ b/neutron/tests/unit/extensions/test_l3_ext_gw_mode.py
@@ -209,7 +209,14 @@ class TestL3GwModeMixin(testlib_api.SqlTestCase):
         self.context.session.add(self.router)
         self.context.session.flush()
 
-    def _test_update_router_gw(self, gw_info, expected_enable_snat):
+    def _test_update_router_gw(self, current_enable_snat, gw_info=None,
+                               expected_enable_snat=True):
+        if not current_enable_snat:
+            previous_gw_info = {'network_id': self.ext_net_id,
+                                'enable_snat': current_enable_snat}
+            self.target_object._update_router_gw_info(
+                self.context, self.router.id, previous_gw_info)
+
         self.target_object._update_router_gw_info(
             self.context, self.router.id, gw_info)
         router = self.target_object._get_router(
@@ -224,16 +231,29 @@ class TestL3GwModeMixin(testlib_api.SqlTestCase):
         self.assertEqual(expected_enable_snat, router.enable_snat)
 
     def test_update_router_gw_with_gw_info_none(self):
-        self._test_update_router_gw(None, True)
+        self._test_update_router_gw(current_enable_snat=True)
+
+    def test_update_router_gw_without_info_and_snat_disabled_previously(self):
+        self._test_update_router_gw(current_enable_snat=False)
 
     def test_update_router_gw_with_network_only(self):
         info = {'network_id': self.ext_net_id}
-        self._test_update_router_gw(info, True)
+        self._test_update_router_gw(current_enable_snat=True, gw_info=info)
+
+    def test_update_router_gw_with_network_and_snat_disabled_previously(self):
+        info = {'network_id': self.ext_net_id}
+        self._test_update_router_gw(current_enable_snat=False, gw_info=info)
 
     def test_update_router_gw_with_snat_disabled(self):
         info = {'network_id': self.ext_net_id,
                 'enable_snat': False}
-        self._test_update_router_gw(info, False)
+        self._test_update_router_gw(
+            current_enable_snat=True, gw_info=info, expected_enable_snat=False)
+
+    def test_update_router_gw_with_snat_enabled(self):
+        info = {'network_id': self.ext_net_id,
+                'enable_snat': True}
+        self._test_update_router_gw(current_enable_snat=False, gw_info=info)
 
     def test_make_router_dict_no_ext_gw(self):
         self._reset_ext_gw()