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')