From cc78a7fbad1ef7bfb71d0775280d6e891686ab7a Mon Sep 17 00:00:00 2001 From: Monty Taylor Date: Mon, 23 Jan 2017 13:21:24 +0100 Subject: [PATCH] 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 --- ...ix-supplemental-fips-c9cd58aac12eb30e.yaml | 7 ++ shade/meta.py | 29 ++++-- shade/tests/unit/test_meta.py | 94 ++++++++++++++++++- 3 files changed, 116 insertions(+), 14 deletions(-) create mode 100644 releasenotes/notes/fix-supplemental-fips-c9cd58aac12eb30e.yaml diff --git a/releasenotes/notes/fix-supplemental-fips-c9cd58aac12eb30e.yaml b/releasenotes/notes/fix-supplemental-fips-c9cd58aac12eb30e.yaml new file mode 100644 index 000000000..66a5f33c9 --- /dev/null +++ b/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. diff --git a/shade/meta.py b/shade/meta.py index b90f704be..27951e308 100644 --- a/shade/meta.py +++ b/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 '' diff --git a/shade/tests/unit/test_meta.py b/shade/tests/unit/test_meta.py index 096af7d62..efecabce3 100644 --- a/shade/tests/unit/test_meta.py +++ b/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')