Use port list to find missing floating ips

It's possible for a cloud to have multiple private networks with overlapping
IP ranges. In that case, the check for missing floating ips can
erroneously match a floating ip for a different server.

Ports are actually unique, and are the foreign key between these things.
Instead of starting with list_floating_ips, start with listing the ports
for the server. In the case where OpenStack isn't broken, this will be
the same number of API calls. In the case where it is, there will be one
extra call per server, but ultimately the output will be more correct -
and the fix for the extra load on the cloud is to fix the nova/neutron
port mapping.

Also, fixed the spelling of supplemental.

Story: 2000845
Change-Id: Ie53a2a144ca2ed812d5441868917996f67b6f454

releasenotes/notes/fix-supplemental-fips-c9cd58aac12eb30e.yaml

@@ -0,0 +1,7 @@
+---
+fixes:
+  - Fixed an issue where shade could report a floating IP being attached
+    to a server erroneously due to only matching on fixed ip. Changed the
+    lookup to match on port ids. This adds an API call in the case where
+    the workaround is needed because of a bug in the cloud, but in most
+    cases it should have no difference.

shade/meta.py

@@ -296,16 +296,14 @@ def expand_server_vars(cloud, server):
     return add_server_interfaces(cloud, server)
 
 
-def _make_address_dict(fip):
+def _make_address_dict(fip, port):
     address = dict(version=4, addr=fip['floating_ip_address'])
     address['OS-EXT-IPS:type'] = 'floating'
-    # MAC address comes from the port, not the FIP. It also doesn't matter
-    # to anyone at the moment, so just make a fake one
-    address['OS-EXT-IPS-MAC:mac_addr'] = 'de:ad:be:ef:be:ef'
+    address['OS-EXT-IPS-MAC:mac_addr'] = port['mac_address']
     return address
 
 
-def _get_suplemental_addresses(cloud, server):
+def _get_supplemental_addresses(cloud, server):
     fixed_ip_mapping = {}
     for name, network in server['addresses'].items():
         for address in network:
@@ -319,11 +317,24 @@ def _get_suplemental_addresses(cloud, server):
         # Don't bother doing this before the server is active, it's a waste
         # of an API call while polling for a server to come up
         if cloud._has_floating_ips() and server['status'] == 'ACTIVE':
-            for fip in cloud.list_floating_ips():
-                if fip['fixed_ip_address'] in fixed_ip_mapping:
+            for port in cloud.search_ports(
+                    filters=dict(device_id=server['id'])):
+                for fip in cloud.search_floating_ips(
+                        filters=dict(port_id=port['id'])):
+                        # This SHOULD return one and only one FIP - but doing
+                        # it as a search/list lets the logic work regardless
+                    if fip['fixed_ip_address'] not in fixed_ip_mapping:
+                        log = _log.setup_logging('shade')
+                        log.debug(
+                            "The cloud returned floating ip %(fip)s attached"
+                            " to server %(server)s but the fixed ip associated"
+                            " with the floating ip in the neutron listing"
+                            " does not exist in the nova listing. Something"
+                            " is exceptionally broken.",
+                            dict(fip=fip['id'], server=server['id']))
                     fixed_net = fixed_ip_mapping[fip['fixed_ip_address']]
                     server['addresses'][fixed_net].append(
-                        _make_address_dict(fip))
+                        _make_address_dict(fip, port))
     except exc.OpenStackCloudException:
         # If something goes wrong with a cloud call, that's cool - this is
         # an attempt to provide additional data and should not block forward
@@ -343,7 +354,7 @@ def add_server_interfaces(cloud, server):
     """
     # First, add an IP address. Set it to '' rather than None if it does
     # not exist to remain consistent with the pre-existing missing values
-    server['addresses'] = _get_suplemental_addresses(cloud, server)
+    server['addresses'] = _get_supplemental_addresses(cloud, server)
     server['public_v4'] = get_server_external_ipv4(cloud, server) or ''
     server['public_v6'] = get_server_external_ipv6(server) or ''
     server['private_v4'] = get_server_private_ip(server, cloud) or ''

shade/tests/unit/test_meta.py

@@ -302,6 +302,7 @@ class TestMeta(base.TestCase):
         mock_has_service.assert_called_with('network')
         mock_list_networks.assert_called_once_with()
 
+    @mock.patch.object(shade.OpenStackCloud, 'list_ports')
     @mock.patch.object(shade.OpenStackCloud, 'list_floating_ips')
     @mock.patch.object(shade.OpenStackCloud, 'list_subnets')
     @mock.patch.object(shade.OpenStackCloud, 'list_server_security_groups')
@@ -316,7 +317,8 @@ class TestMeta(base.TestCase):
             mock_get_volumes,
             mock_list_server_security_groups,
             mock_list_subnets,
-            mock_list_floating_ips):
+            mock_list_floating_ips,
+            mock_list_ports):
         mock_get_image_name.return_value = 'cirros-0.3.4-x86_64-uec'
         mock_get_flavor_name.return_value = 'm1.tiny'
         mock_has_service.return_value = True
@@ -333,7 +335,20 @@ class TestMeta(base.TestCase):
                 'name': 'private',
             },
         ]
-        mock_list_floating_ips.return_value = []
+        mock_list_floating_ips.return_value = [
+            {
+                'port_id': 'test_port_id',
+                'fixed_ip_address': PRIVATE_V4,
+                'floating_ip_address': PUBLIC_V4,
+            }
+        ]
+        mock_list_ports.return_value = [
+            {
+                'id': 'test_port_id',
+                'mac_address': 'fa:16:3e:ae:7d:42',
+                'device_id': 'test-id',
+            }
+        ]
 
         srv = self.cloud.get_openstack_vars(meta.obj_to_dict(fakes.FakeServer(
             id='test-id', name='test-name', status='ACTIVE',
@@ -345,15 +360,16 @@ class TestMeta(base.TestCase):
                 u'OS-EXT-IPS:type': u'fixed',
                 u'addr': PRIVATE_V4,
                 u'version': 4,
-                u'OS-EXT-IPS-MAC:mac_addr':
-                u'fa:16:3e:ae:7d:42'
+                u'OS-EXT-IPS-MAC:mac_addr': u'fa:16:3e:ae:7d:42'
             }]}
         )))
 
         self.assertEqual(PRIVATE_V4, srv['private_v4'])
         mock_has_service.assert_called_with('volume')
         mock_list_networks.assert_called_once_with()
-        mock_list_floating_ips.assert_called_once_with()
+        mock_list_floating_ips.assert_called_once_with(
+            filters={'port_id': 'test_port_id'})
+        mock_list_ports.assert_called_once_with({'device_id': 'test-id'})
 
     @mock.patch.object(shade.OpenStackCloud, 'list_floating_ips')
     @mock.patch.object(shade.OpenStackCloud, 'list_subnets')
@@ -457,6 +473,74 @@ class TestMeta(base.TestCase):
         mock_list_networks.assert_called_once_with()
         mock_list_floating_ips.assert_not_called()
 
+    @mock.patch.object(shade.OpenStackCloud, 'list_floating_ips')
+    @mock.patch.object(shade.OpenStackCloud, 'list_ports')
+    @mock.patch.object(shade.OpenStackCloud, 'list_subnets')
+    @mock.patch.object(shade.OpenStackCloud, 'list_server_security_groups')
+    @mock.patch.object(shade.OpenStackCloud, 'get_volumes')
+    @mock.patch.object(shade.OpenStackCloud, 'get_image_name')
+    @mock.patch.object(shade.OpenStackCloud, 'get_flavor_name')
+    @mock.patch.object(shade.OpenStackCloud, 'has_service')
+    @mock.patch.object(shade.OpenStackCloud, 'list_networks')
+    def test_get_server_cloud_missing_fips(
+            self, mock_list_networks, mock_has_service,
+            mock_get_flavor_name, mock_get_image_name,
+            mock_get_volumes,
+            mock_list_server_security_groups,
+            mock_list_subnets,
+            mock_list_ports,
+            mock_list_floating_ips):
+        self.cloud._floating_ip_source = 'neutron'
+        mock_get_image_name.return_value = 'cirros-0.3.4-x86_64-uec'
+        mock_get_flavor_name.return_value = 'm1.tiny'
+        mock_has_service.return_value = True
+        mock_get_volumes.return_value = []
+        mock_list_subnets.return_value = SUBNETS_WITH_NAT
+        mock_list_floating_ips.return_value = [
+            {
+                'port_id': 'test_port_id',
+                'fixed_ip_address': PRIVATE_V4,
+                'floating_ip_address': PUBLIC_V4,
+            }
+        ]
+        mock_list_ports.return_value = [
+            {
+                'id': 'test_port_id',
+                'mac_address': 'fa:16:3e:ae:7d:42',
+                'device_id': 'test-id',
+            }
+        ]
+        mock_list_networks.return_value = [
+            {
+                'id': 'test_pnztt_net',
+                'name': 'test_pnztt_net',
+                'router:external': False,
+            },
+            {
+                'id': 'private',
+                'name': 'private',
+            },
+        ]
+
+        srv = self.cloud.get_openstack_vars(meta.obj_to_dict(fakes.FakeServer(
+            id='test-id', name='test-name', status='ACTIVE',
+            flavor={u'id': u'1'},
+            image={
+                'name': u'cirros-0.3.4-x86_64-uec',
+                u'id': u'f93d000b-7c29-4489-b375-3641a1758fe1'},
+            addresses={u'test_pnztt_net': [{
+                u'addr': PRIVATE_V4,
+                u'version': 4,
+                'OS-EXT-IPS-MAC:mac_addr': 'fa:16:3e:ae:7d:42',
+            }]}
+        )))
+
+        self.assertEqual(PUBLIC_V4, srv['public_v4'])
+        mock_list_networks.assert_called_once_with()
+        mock_list_floating_ips.assert_called_once_with(
+            filters={'port_id': 'test_port_id'})
+        mock_list_ports.assert_called_once_with({'device_id': 'test-id'})
+
     @mock.patch.object(shade.OpenStackCloud, 'list_floating_ips')
     @mock.patch.object(shade.OpenStackCloud, 'list_subnets')
     @mock.patch.object(shade.OpenStackCloud, 'list_server_security_groups')