From aab4b7a0e2504c04e08389145bcb1414dea63631 Mon Sep 17 00:00:00 2001 From: Balazs Gibizer Date: Thu, 24 Oct 2019 17:01:02 +0200 Subject: [PATCH] Use admin neutron client to query ports for binding The compute service updates the binding:profile of the neutron port during server create. If the port has resource_request then the 'allocation' key need to point to the resource provider the port is allocating resources. Unfortunately this code used a non admin client to query the port data and therefore if the original server create request was sent by a non admin user the returned port does not have its resource_request filled and as a consequence nova does not add the allocation key to the binding profile. This patch makes sure that the port is queried with an admin client. There is a tempest test change that reproduces the issue: https://review.opendev.org/#/c/690934 Change-Id: Icc631cf2e81a5c78cb7fb1d0b625d19bd8f5a274 Closes-Bug: #1849657 --- nova/network/neutronv2/api.py | 16 +++++--- nova/tests/unit/network/test_neutronv2.py | 49 ++++++++++++----------- 2 files changed, 36 insertions(+), 29 deletions(-) diff --git a/nova/network/neutronv2/api.py b/nova/network/neutronv2/api.py index 25217a7eb4a8..067c3019d636 100644 --- a/nova/network/neutronv2/api.py +++ b/nova/network/neutronv2/api.py @@ -965,6 +965,10 @@ class API(base_api.NetworkAPI): # We do not want to create a new neutron session for each call neutron = get_client(context) + # We always need admin_client to build nw_info, + # we sometimes need it when updating ports + admin_client = get_client(context, admin=True) + # # Validate ports and networks with neutron. The requested_ports_dict # variable is a dict, keyed by port ID, of ports that were on the user @@ -972,9 +976,15 @@ class API(base_api.NetworkAPI): # NetworkRequest objects for any networks or ports specifically # requested by the user, which again may be empty. # + + # NOTE(gibi): we use the admin_client here to ensure that the returned + # ports has the resource_request attribute filled as later we use this + # information to decide when to add allocation key to the port binding. + # See bug 1849657. requested_ports_dict, ordered_networks = ( self._validate_requested_port_ids( - context, instance, neutron, requested_networks, attach=attach)) + context, instance, admin_client, requested_networks, + attach=attach)) nets = self._validate_requested_network_ids( context, instance, neutron, requested_networks, ordered_networks) @@ -1022,10 +1032,6 @@ class API(base_api.NetworkAPI): # Update existing and newly created ports # - # We always need admin_client to build nw_info, - # we sometimes need it when updating ports - admin_client = get_client(context, admin=True) - ordered_nets, ordered_port_ids, preexisting_port_ids, \ created_port_ids = self._update_ports_for_instance( context, instance, diff --git a/nova/tests/unit/network/test_neutronv2.py b/nova/tests/unit/network/test_neutronv2.py index 6d80d80c6e24..d6cfef5a5a3c 100644 --- a/nova/tests/unit/network/test_neutronv2.py +++ b/nova/tests/unit/network/test_neutronv2.py @@ -468,7 +468,6 @@ class TestNeutronv2Base(test.TestCase): mock_refresh, mock_populate, net_idx=1, requested_networks=None, exception=None, - get_client_admin_call=True, context=None, **kwargs): ctxt = context or self.context @@ -614,12 +613,9 @@ class TestNeutronv2Base(test.TestCase): ctxt, self.instance, False, requested_networks, bind_host_id=bind_host_id) - if get_client_admin_call: - mock_get_client.assert_has_calls([ - mock.call(ctxt), mock.call(ctxt, admin=True)], - any_order=True) - else: - mock_get_client.assert_called_once_with(mock.ANY) + mock_get_client.assert_has_calls([ + mock.call(ctxt), mock.call(ctxt, admin=True)], + any_order=True) mock_refresh.assert_not_called() @@ -1177,8 +1173,7 @@ class TestNeutronv2(TestNeutronv2Base): def test_allocate_for_instance_2(self): # Allocate one port in two networks env. self._test_allocate_for_instance( - net_idx=2, exception=exception.NetworkAmbiguous, - get_client_admin_call=False) + net_idx=2, exception=exception.NetworkAmbiguous) def test_allocate_for_instance_accepts_only_portid(self): # Make sure allocate_for_instance works when only a portid is provided @@ -1199,8 +1194,7 @@ class TestNeutronv2(TestNeutronv2Base): def test_allocate_for_instance_without_requested_networks(self): self._test_allocate_for_instance( - net_idx=3, exception=exception.NetworkAmbiguous, - get_client_admin_call=False) + net_idx=3, exception=exception.NetworkAmbiguous) def test_allocate_for_instance_with_requested_non_available_network(self): """verify that a non available network is ignored. @@ -1240,7 +1234,6 @@ class TestNeutronv2(TestNeutronv2Base): # raised. self._test_allocate_for_instance_with_virtual_interface( net_idx=4, exception=exception.SecurityGroupCannotBeApplied, - get_client_admin_call=False, _break='post_list_extensions') def test_allocate_for_instance_with_invalid_network_id(self): @@ -1250,7 +1243,6 @@ class TestNeutronv2(TestNeutronv2Base): self._test_allocate_for_instance(net_idx=9, requested_networks=requested_networks, exception=exception.NetworkNotFound, - get_client_admin_call=False, _break='post_list_networks') def test_allocate_for_instance_with_requested_networks_with_fixedip(self): @@ -1279,7 +1271,9 @@ class TestNeutronv2(TestNeutronv2Base): nwinfo = self.api.allocate_for_instance(self.context, self.instance, False, None) self.assertEqual(0, len(nwinfo)) - mock_get_client.assert_called_once_with(self.context) + mock_get_client.assert_has_calls([ + mock.call(self.context), + mock.call(self.context, admin=True)]) mocked_client.list_networks.assert_has_calls([ mock.call(tenant_id=self.instance.project_id, shared=False), mock.call(shared=True)]) @@ -1389,7 +1383,9 @@ class TestNeutronv2(TestNeutronv2Base): self.api.allocate_for_instance, self.context, self.instance, False, requested_networks=requested_networks) - mock_get_client.assert_called_once_with(self.context) + mock_get_client.assert_has_calls([ + mock.call(self.context), + mock.call(self.context, admin=True)]) mocked_client.list_networks.assert_called_once_with( id=[uuids.my_netid1, uuids.my_netid2]) mocked_client.create_port.assert_called_once_with(port_req_body) @@ -1410,7 +1406,9 @@ class TestNeutronv2(TestNeutronv2Base): self.assertRaises(test.TestingException, self.api.allocate_for_instance, self.context, self.instance, False, requested_networks) - mock_get_client.assert_called_once_with(self.context) + mock_get_client.assert_has_calls([ + mock.call(self.context), + mock.call(self.context, admin=True)]) mock_get_available.assert_called_once_with( self.context, self.instance.project_id, [], neutron=mocked_client, auto_allocate=False) @@ -1429,9 +1427,8 @@ class TestNeutronv2(TestNeutronv2Base): objects=[objects.NetworkRequest(port_id=uuids.portid_1)]) self._test_allocate_for_instance( requested_networks=requested_networks, - exception=exception.PortInUse, - get_client_admin_call=False, - _break='pre_list_networks', _device=True) + exception=exception.PortInUse, _break='pre_list_networks', + _device=True) def test_allocate_for_instance_port_not_found(self): # If a port is not found, an exception should be raised. @@ -1440,7 +1437,6 @@ class TestNeutronv2(TestNeutronv2Base): self._test_allocate_for_instance( requested_networks=requested_networks, exception=exception.PortNotFound, - get_client_admin_call=False, _break='pre_list_networks') def test_allocate_for_instance_port_invalid_tenantid(self): @@ -1450,7 +1446,6 @@ class TestNeutronv2(TestNeutronv2Base): self._test_allocate_for_instance( requested_networks=requested_networks, exception=exception.PortNotUsable, - get_client_admin_call=False, _break='pre_list_networks') @mock.patch.object(neutronapi, 'get_client') @@ -1471,7 +1466,9 @@ class TestNeutronv2(TestNeutronv2Base): self.assertRaises(exception.ExternalNetworkAttachForbidden, self.api.allocate_for_instance, self.context, self.instance, False, None) - mock_get_client.assert_called_once_with(self.context) + mock_get_client.assert_has_calls([ + mock.call(self.context), + mock.call(self.context, admin=True)]) mocked_client.list_networks.assert_has_calls([ mock.call(tenant_id=self.instance.project_id, shared=False), mock.call(shared=True)]) @@ -1496,7 +1493,9 @@ class TestNeutronv2(TestNeutronv2Base): exception.NetworkAmbiguous, self.api.allocate_for_instance, self.context, self.instance, False, None) - mock_get_client.assert_called_once_with(self.context) + mock_get_client.assert_has_calls([ + mock.call(self.context), + mock.call(self.context, admin=True)]) mocked_client.list_networks.assert_has_calls([ mock.call(tenant_id=self.instance.project_id, shared=False), mock.call(shared=True)]) @@ -6811,6 +6810,9 @@ class TestAllocateForInstance(test.NoDBTestCase): self.assertEqual(result[0], {"id": uuids.created}) self.assertEqual(result[1], {"id": uuids.preexist}) + mock_validate_ports.assert_called_once_with( + self.context, self.instance, "admin", None, attach=False) + def test_ensure_no_port_binding_failure_raises(self): port = { 'id': uuids.port_id, @@ -7180,7 +7182,6 @@ class TestNeutronv2NeutronHostnameDNS(TestNeutronv2Base): self._test_allocate_for_instance( requested_networks=requested_networks, exception=exception.PortNotUsableDNS, - get_client_admin_call=False, dns_extension=True, _break='pre_list_networks', _dns_name='my-instance')