Fix handling of floating IP association info in Nicira plugin

Bug 1223902

This patch fixes the _update_fix_assoc routine in the Nicira plugin,
ensuring the association of a floating IP with a router is removed
when the floating IP is disassociated.
This patch also adds a unit test for validating behaviour when a floating
IP is associated to a port on a different router, which was an
uncovered use case. This new unit test required some refactoring in
test_l3_plugin.py.

Change-Id: Ibfd53a637f4d14a368b0aeff289c15f993df58dd

diff --git a/neutron/plugins/nicira/NeutronPlugin.py b/neutron/plugins/nicira/NeutronPlugin.py
index 1eb4fad95335dc3e1f3304ec6ebddd28a412ab51..4150b8025da6f305c1c14b90fa18cf200dafe947 100644 (file)
--- a/neutron/plugins/nicira/NeutronPlugin.py
+++ b/neutron/plugins/nicira/NeutronPlugin.py
@@ -1767,16 +1767,6 @@ class NvpPluginV2(addr_pair_db.AllowedAddressPairsMixin,
                 context,
                 fip,
                 floatingip_db['floating_network_id'])
-
-        # Retrieve and delete existing NAT rules, if any
-        if not router_id and floatingip_db.get('fixed_port_id'):
-            # This happens if we're disassociating. Need to explicitly
-            # find the router serving this floating IP
-            tmp_fip = fip.copy()
-            tmp_fip['port_id'] = floatingip_db['fixed_port_id']
-            _pid, internal_ip, router_id = self.get_assoc_data(
-                context, tmp_fip, floatingip_db['floating_network_id'])
-
         return (port_id, internal_ip, router_id)
 
     def _update_fip_assoc(self, context, fip, floatingip_db, external_port):
@@ -1785,6 +1775,8 @@ class NvpPluginV2(addr_pair_db.AllowedAddressPairsMixin,
         Overrides method from base class.
         The method is augmented for creating NAT rules in the process.
         """
+        # Store router currently serving the floating IP
+        old_router_id = floatingip_db.router_id
         port_id, internal_ip, router_id = self._get_fip_assoc_data(
             context, fip, floatingip_db)
         floating_ip = floatingip_db['floating_ip_address']
@@ -1795,14 +1787,31 @@ class NvpPluginV2(addr_pair_db.AllowedAddressPairsMixin,
                                                 internal_ip,
                                                 router_id)
             # Fetch logical port of router's external gateway
+        # Fetch logical port of router's external gateway
+        nvp_floating_ips = self._build_ip_address_list(
+            context.elevated(), external_port['fixed_ips'])
+        floating_ip = floatingip_db['floating_ip_address']
+        # Retrieve and delete existing NAT rules, if any
+        if old_router_id:
+            # Retrieve the current internal ip
+            _p, _s, old_internal_ip = self._internal_fip_assoc_data(
+                context, {'id': floatingip_db.id,
+                          'port_id': floatingip_db.fixed_port_id,
+                          'fixed_ip_address': floatingip_db.fixed_ip_address,
+                          'tenant_id': floatingip_db.tenant_id})
+            nvp_gw_port_id = nvplib.find_router_gw_port(
+                context, self.cluster, old_router_id)['uuid']
+            self._retrieve_and_delete_nat_rules(
+                context, floating_ip, old_internal_ip, old_router_id)
+            nvplib.update_lrouter_port_ips(
+                self.cluster, old_router_id, nvp_gw_port_id,
+                ips_to_add=[], ips_to_remove=nvp_floating_ips)
+
+        if router_id:
             nvp_gw_port_id = nvplib.find_router_gw_port(
                 context, self.cluster, router_id)['uuid']
-            nvp_floating_ips = self._build_ip_address_list(
-                context.elevated(), external_port['fixed_ips'])
-            LOG.debug(_("Address list for NVP logical router "
-                        "port:%s"), nvp_floating_ips)
             # Re-create NAT rules only if a port id is specified
-            if 'port_id' in fip and fip['port_id']:
+            if fip.get('port_id'):
                 try:
                     # Create new NAT rules
                     nvplib.create_lrouter_dnat_rule(
@@ -1839,15 +1848,6 @@ class NvpPluginV2(addr_pair_db.AllowedAddressPairsMixin,
                                    'internal_ip': internal_ip})
                     msg = _("Failed to update NAT rules for floatingip update")
                     raise nvp_exc.NvpPluginException(err_msg=msg)
-            elif floatingip_db['fixed_port_id']:
-                # This is a disassociation.
-                # Remove floating IP address from logical router port
-                internal_ip = None
-                nvplib.update_lrouter_port_ips(self.cluster,
-                                               router_id,
-                                               nvp_gw_port_id,
-                                               ips_to_add=[],
-                                               ips_to_remove=nvp_floating_ips)
 
         floatingip_db.update({'fixed_ip_address': internal_ip,
                               'fixed_port_id': port_id,

diff --git a/neutron/tests/unit/test_l3_plugin.py b/neutron/tests/unit/test_l3_plugin.py
index 1819f2c7f34e58f927387428428644d0d7800782..39e687f245c4b4790b7e2091a046bf2d7fa66b31 100644 (file)
--- a/neutron/tests/unit/test_l3_plugin.py
+++ b/neutron/tests/unit/test_l3_plugin.py
@@ -474,35 +474,43 @@ class L3NatTestCaseMixin(object):
                             r['router']['id'],
                             public_sub['subnet']['network_id'])
 
+    @contextlib.contextmanager
+    def floatingip_no_assoc_with_public_sub(
+        self, private_sub, fmt=None, set_context=False, public_sub=None):
+        self._set_net_external(public_sub['subnet']['network_id'])
+        with self.router() as r:
+            floatingip = None
+            try:
+                self._add_external_gateway_to_router(
+                    r['router']['id'],
+                    public_sub['subnet']['network_id'])
+                self._router_interface_action('add', r['router']['id'],
+                                              private_sub['subnet']['id'],
+                                              None)
+
+                floatingip = self._make_floatingip(
+                    fmt or self.fmt,
+                    public_sub['subnet']['network_id'],
+                    set_context=set_context)
+                yield floatingip, r
+            finally:
+                if floatingip:
+                    self._delete('floatingips',
+                                 floatingip['floatingip']['id'])
+                self._router_interface_action('remove', r['router']['id'],
+                                              private_sub['subnet']['id'],
+                                              None)
+                self._remove_external_gateway_from_router(
+                    r['router']['id'],
+                    public_sub['subnet']['network_id'])
+
     @contextlib.contextmanager
     def floatingip_no_assoc(self, private_sub, fmt=None, set_context=False):
         with self.subnet(cidr='12.0.0.0/24') as public_sub:
-            self._set_net_external(public_sub['subnet']['network_id'])
-            with self.router() as r:
-                floatingip = None
-                try:
-                    self._add_external_gateway_to_router(
-                        r['router']['id'],
-                        public_sub['subnet']['network_id'])
-                    self._router_interface_action('add', r['router']['id'],
-                                                  private_sub['subnet']['id'],
-                                                  None)
-
-                    floatingip = self._make_floatingip(
-                        fmt or self.fmt,
-                        public_sub['subnet']['network_id'],
-                        set_context=set_context)
-                    yield floatingip
-                finally:
-                    if floatingip:
-                        self._delete('floatingips',
-                                     floatingip['floatingip']['id'])
-                    self._router_interface_action('remove', r['router']['id'],
-                                                  private_sub['subnet']['id'],
-                                                  None)
-                    self._remove_external_gateway_from_router(
-                        r['router']['id'],
-                        public_sub['subnet']['network_id'])
+            with self.floatingip_no_assoc_with_public_sub(
+                private_sub, fmt, set_context, public_sub) as (f, r):
+                # Yield only the floating ip object
+                yield f
 
 
 class L3NatTestCaseBase(L3NatTestCaseMixin):
@@ -1243,6 +1251,68 @@ class L3NatTestCaseBase(L3NatTestCaseMixin):
                 self.assertEqual(body['floatingip']['fixed_ip_address'],
                                  ip_address)
 
+    def test_floatingip_update_different_router(self):
+        # Create subnet with different CIDRs to account for plugins which
+        # do not support overlapping IPs
+        with contextlib.nested(self.subnet(cidr='10.0.0.0/24'),
+                               self.subnet(cidr='10.0.1.0/24')) as (
+                                   s1, s2):
+            with contextlib.nested(self.port(subnet=s1),
+                                   self.port(subnet=s2)) as (p1, p2):
+                private_sub1 = {'subnet':
+                                {'id':
+                                 p1['port']['fixed_ips'][0]['subnet_id']}}
+                private_sub2 = {'subnet':
+                                {'id':
+                                 p2['port']['fixed_ips'][0]['subnet_id']}}
+                with self.subnet(cidr='12.0.0.0/24') as public_sub:
+                    with contextlib.nested(
+                            self.floatingip_no_assoc_with_public_sub(
+                                private_sub1, public_sub=public_sub),
+                            self.floatingip_no_assoc_with_public_sub(
+                                private_sub2, public_sub=public_sub)) as (
+                                    (fip1, r1), (fip2, r2)):
+
+                        def assert_no_assoc(fip):
+                            body = self._show('floatingips',
+                                              fip['floatingip']['id'])
+                            self.assertEqual(body['floatingip']['port_id'],
+                                             None)
+                            self.assertIsNone(
+                                body['floatingip']['fixed_ip_address'], None)
+
+                        assert_no_assoc(fip1)
+                        assert_no_assoc(fip2)
+
+                        def associate_and_assert(fip, port):
+                            port_id = port['port']['id']
+                            ip_address = (port['port']['fixed_ips']
+                                          [0]['ip_address'])
+                            body = self._update(
+                                'floatingips', fip['floatingip']['id'],
+                                {'floatingip': {'port_id': port_id}})
+                            self.assertEqual(body['floatingip']['port_id'],
+                                             port_id)
+                            self.assertEqual(
+                                body['floatingip']['fixed_ip_address'],
+                                ip_address)
+                            return body['floatingip']['router_id']
+
+                        fip1_r1_res = associate_and_assert(fip1, p1)
+                        self.assertEqual(fip1_r1_res, r1['router']['id'])
+                        # The following operation will associate the floating
+                        # ip to a different router
+                        fip1_r2_res = associate_and_assert(fip1, p2)
+                        self.assertEqual(fip1_r2_res, r2['router']['id'])
+                        fip2_r1_res = associate_and_assert(fip2, p1)
+                        self.assertEqual(fip2_r1_res, r1['router']['id'])
+                        # disassociate fip1
+                        self._update(
+                            'floatingips', fip1['floatingip']['id'],
+                            {'floatingip': {'port_id': None}})
+                        fip2_r2_res = associate_and_assert(fip2, p2)
+                        self.assertEqual(fip2_r2_res, r2['router']['id'])
+
     def test_floatingip_with_assoc(self):
         with self.floatingip_with_assoc() as fip:
             body = self._show('floatingips', fip['floatingip']['id'])