diff --git a/ironic/common/neutron.py b/ironic/common/neutron.py index 1cd34fd16f..28cfc73cfe 100644 --- a/ironic/common/neutron.py +++ b/ironic/common/neutron.py @@ -12,6 +12,7 @@ import copy +from keystoneauth1 import loading as ks_loading import netaddr from neutronclient.common import exceptions as neutron_exceptions from neutronclient.v2_0 import client as clientv20 @@ -79,6 +80,49 @@ def get_client(token=None, context=None): timeout=CONF.neutron.request_timeout) +def _get_conf_client(context): + """Retrieve a neutron client connection using conf parameters. + + :param context: request context, + instance of ironic.common.context.RequestContext + :returns: A neutron client. + """ + + auth = ks_loading.load_auth_from_conf_options(CONF, 'neutron') + session = ks_loading.load_session_from_conf_options( + CONF, + 'neutron', + auth=auth) + endpoint = keystone.get_endpoint('neutron', session=session, + auth=auth) + return clientv20.Client(session=session, + auth=auth, + endpoint_override=endpoint, + retries=CONF.neutron.retries, + global_request_id=context.global_id, + timeout=CONF.neutron.request_timeout) + + +def update_neutron_port(context, port_id, update_body, client=None): + """Undate a neutron port + + Uses neutron client from conf client to update a neutron client + an unbound state. + + :param context: request context, + instance of ironic.common.context.RequestContext + :param port_id: Neutron port ID. + :param update_body: Body of update + :param client: Optional Neutron client + """ + if not client: + # verify that user can see the port before updating it + get_client(context=context).show_port(port_id) + client = _get_conf_client(context) + + return client.update_port(port_id, update_body) + + def unbind_neutron_port(port_id, client=None, context=None): """Unbind a neutron port @@ -92,20 +136,17 @@ def unbind_neutron_port(port_id, client=None, context=None): :raises: NetworkError """ - if not client: - client = get_client(context=context) - body_unbind = {'port': {'binding:host_id': '', 'binding:profile': {}}} body_reset_mac = {'port': {'mac_address': None}} try: - client.update_port(port_id, body_unbind) + update_neutron_port(context, port_id, body_unbind, client) # NOTE(hjensas): We need to reset the mac address in a separate step. # Exception PortBound will be raised by neutron as it refuses to # update the mac address of a bound port if we attempt to unbind and # reset the mac in the same call. - client.update_port(port_id, body_reset_mac) + update_neutron_port(context, port_id, body_reset_mac, client) # NOTE(vsaienko): Ignore if port was deleted before calling vif detach. except neutron_exceptions.PortNotFoundClient: LOG.info('Port %s was not found while unbinding.', port_id) @@ -142,10 +183,10 @@ def update_port_address(port_id, address, context=None): msg = (_("Failed to remove the current binding from " "Neutron port %s, while updating its MAC " "address.") % port_id) - unbind_neutron_port(port_id, client=client, context=context) + unbind_neutron_port(port_id, context=context) msg = (_("Failed to update MAC address on Neutron port %s.") % port_id) - client.update_port(port_id, port_req_body) + update_neutron_port(context, port_id, port_req_body) # Restore original binding:profile and host_id if binding_host_id: @@ -154,7 +195,7 @@ def update_port_address(port_id, address, context=None): port_req_body = {'port': {'binding:host_id': binding_host_id, 'binding:profile': binding_profile}} - client.update_port(port_id, port_req_body) + update_neutron_port(context, port_id, port_req_body) except (neutron_exceptions.NeutronClientException, exception.NetworkError): LOG.exception(msg) raise exception.FailedToUpdateMacOnPort(port_id=port_id) @@ -193,7 +234,7 @@ def _verify_security_groups(security_groups, client): raise exception.NetworkError(msg) -def _add_ip_addresses_for_ipv6_stateful(port, client): +def _add_ip_addresses_for_ipv6_stateful(context, port, client): """Add additional IP addresses to the ipv6 stateful neutron port When network booting with DHCPv6-stateful we cannot control the CLID/IAID @@ -216,7 +257,7 @@ def _add_ip_addresses_for_ipv6_stateful(port, client): fixed_ips.append({'subnet_id': subnet['id']}) body = {'port': {'fixed_ips': fixed_ips}} - client.update_port(port['port']['id'], body) + update_neutron_port(context, port['port']['id'], body) def add_ports_to_network(task, network_uuid, security_groups=None): @@ -254,8 +295,13 @@ def add_ports_to_network(task, network_uuid, security_groups=None): 'network_id': network_uuid, 'admin_state_up': True, 'binding:vnic_type': VNIC_BAREMETAL, - 'device_owner': 'baremetal:none', + } + } + # separate out fields that can only be updated by admins + update_body = { + 'port': { 'binding:host_id': node.uuid, + 'device_owner': 'baremetal:none', } } if security_groups: @@ -283,14 +329,15 @@ def add_ports_to_network(task, network_uuid, security_groups=None): for ironic_port in ports_to_create: # Start with a clean state for each port port_body = copy.deepcopy(body) + update_port_body = copy.deepcopy(update_body) # Skip ports that are missing required information for deploy. if not validate_port_info(node, ironic_port): failures.append(ironic_port.uuid) continue - port_body['port']['mac_address'] = ironic_port.address + update_port_body['port']['mac_address'] = ironic_port.address binding_profile = {'local_link_information': [portmap[ironic_port.uuid]]} - port_body['port']['binding:profile'] = binding_profile + update_port_body['port']['binding:profile'] = binding_profile if not ironic_port.pxe_enabled: LOG.debug("Adding port %(port)s to network %(net)s for " @@ -306,7 +353,7 @@ def add_ports_to_network(task, network_uuid, security_groups=None): 'port %(port_id)s, hostname %(hostname)s', {'port_id': ironic_port.uuid, 'hostname': link_info['hostname']}) - port_body['port']['binding:host_id'] = link_info['hostname'] + update_port_body['port']['binding:host_id'] = link_info['hostname'] # TODO(hamdyk): use portbindings.VNIC_SMARTNIC from neutron-lib port_body['port']['binding:vnic_type'] = VNIC_SMARTNIC @@ -319,11 +366,13 @@ def add_ports_to_network(task, network_uuid, security_groups=None): port_body['port']['extra_dhcp_opts'] = extra_dhcp_opts try: if is_smart_nic: - wait_for_host_agent(client, - port_body['port']['binding:host_id']) + wait_for_host_agent( + client, update_port_body['port']['binding:host_id']) port = client.create_port(port_body) + update_neutron_port(task.context, port['port']['id'], + update_port_body) if CONF.neutron.dhcpv6_stateful_address_count > 1: - _add_ip_addresses_for_ipv6_stateful(port, client) + _add_ip_addresses_for_ipv6_stateful(task.context, port, client) if is_smart_nic: wait_for_port_status(client, port['port']['id'], 'ACTIVE') except neutron_exceptions.NeutronClientException as e: diff --git a/ironic/dhcp/neutron.py b/ironic/dhcp/neutron.py index a118eb2d48..2245614554 100644 --- a/ironic/dhcp/neutron.py +++ b/ironic/dhcp/neutron.py @@ -61,8 +61,7 @@ class NeutronDHCPApi(base.BaseDHCP): port_id, dhcp_options, token=token, context=context) port_req_body = {'port': {'extra_dhcp_opts': dhcp_options}} try: - neutron.get_client(token=token, context=context).update_port( - port_id, port_req_body) + neutron.update_neutron_port(context, port_id, port_req_body) except neutron_client_exc.NeutronClientException: LOG.exception("Failed to update Neutron port %s.", port_id) raise exception.FailedToUpdateDHCPOptOnPort(port_id=port_id) diff --git a/ironic/drivers/modules/network/common.py b/ironic/drivers/modules/network/common.py index c1d5b4bfff..736249b69c 100644 --- a/ironic/drivers/modules/network/common.py +++ b/ironic/drivers/modules/network/common.py @@ -283,7 +283,7 @@ def plug_port_to_tenant_network(task, port_like_obj, client=None): neutron.wait_for_host_agent(client, body['port']['binding:host_id']) try: - client.update_port(vif_id, body) + neutron.update_neutron_port(task.context, vif_id, body) if is_smart_nic: neutron.wait_for_port_status(client, vif_id, 'ACTIVE') except neutron_exceptions.ConnectionFailed as e: diff --git a/ironic/drivers/modules/network/flat.py b/ironic/drivers/modules/network/flat.py index 06f7ff8cfb..b11de097e8 100644 --- a/ironic/drivers/modules/network/flat.py +++ b/ironic/drivers/modules/network/flat.py @@ -55,7 +55,6 @@ class FlatNetwork(common.NeutronVIFPortIDMixin, def _bind_flat_ports(self, task): LOG.debug("Binding flat network ports") - client = neutron.get_client(context=task.context) for port_like_obj in task.ports + task.portgroups: vif_port_id = ( port_like_obj.internal_info.get(common.TENANT_VIF_KEY) @@ -71,7 +70,8 @@ class FlatNetwork(common.NeutronVIFPortIDMixin, } } try: - client.update_port(vif_port_id, body) + neutron.update_neutron_port(task.context, + vif_port_id, body) except neutron_exceptions.NeutronClientException as e: msg = (_('Unable to set binding:host_id for ' 'neutron port %(port_id)s. Error: ' diff --git a/ironic/tests/unit/common/test_neutron.py b/ironic/tests/unit/common/test_neutron.py index 86b9b23464..81d8c99be3 100644 --- a/ironic/tests/unit/common/test_neutron.py +++ b/ironic/tests/unit/common/test_neutron.py @@ -145,6 +145,104 @@ class TestNeutronClient(base.TestCase): self.assertEqual(0, mock_sauth.call_count) +class TestNeutronConfClient(base.TestCase): + + def setUp(self): + super(TestNeutronConfClient, self).setUp() + # NOTE(pas-ha) register keystoneauth dynamic options manually + plugin = kaloading.get_plugin_loader('password') + opts = kaloading.get_auth_plugin_conf_options(plugin) + self.cfg_fixture.register_opts(opts, group='neutron') + self.config(retries=2, + group='neutron') + self.config(username='test-admin-user', + project_name='test-admin-tenant', + password='test-admin-password', + auth_url='test-auth-uri', + auth_type='password', + interface='internal', + service_type='network', + timeout=10, + group='neutron') + # force-reset the global session object + neutron._NEUTRON_SESSION = None + self.context = context.RequestContext(global_request_id='global') + + @mock.patch('keystoneauth1.loading.load_auth_from_conf_options', + autospec=True, return_value=mock.sentinel.auth) + @mock.patch('keystoneauth1.loading.load_session_from_conf_options', + autospec=True, return_value=mock.sentinel.session) + @mock.patch('ironic.common.keystone.get_endpoint', autospec=True, + return_value='neutron_url') + @mock.patch.object(client.Client, "__init__", return_value=None, + autospec=True) + def test_get_neutron_conf_client(self, mock_client, mock_get_endpoint, + mock_session, mock_auth): + neutron._get_conf_client(self.context) + mock_client.assert_called_once_with(mock.ANY, # this is 'self' + session=mock.sentinel.session, + auth=mock.sentinel.auth, retries=2, + endpoint_override='neutron_url', + global_request_id='global', + timeout=45) + + +class TestUpdateNeutronPort(base.TestCase): + def setUp(self): + super(TestUpdateNeutronPort, self).setUp() + + self.uuid = uuidutils.generate_uuid() + self.context = context.RequestContext() + self.update_body = {'port': {}} + + @mock.patch.object(neutron, 'get_client', autospec=True) + @mock.patch.object(neutron, '_get_conf_client', autospec=True) + def test_update_neutron_port(self, conf_client_mock, client_mock): + client_mock.return_value.show_port.return_value = {'port': {}} + conf_client_mock.return_value.update_port.return_value = {'port': {}} + + neutron.update_neutron_port(self.context, self.uuid, self.update_body) + + client_mock.assert_called_once_with(context=self.context) + client_mock.return_value.show_port.assert_called_once_with(self.uuid) + conf_client_mock.assert_called_once_with(self.context) + conf_client_mock.return_value.update_port.assert_called_once_with( + self.uuid, self.update_body) + + @mock.patch.object(neutron, 'get_client', autospec=True) + @mock.patch.object(neutron, '_get_conf_client', autospec=True) + def test_update_neutron_port_with_client(self, conf_client_mock, + client_mock): + client_mock.return_value.show_port.return_value = {'port': {}} + conf_client_mock.return_value.update_port.return_value = {'port': {}} + client = mock.Mock() + client.update_port.return_value = {'port': {}} + + neutron.update_neutron_port(self.context, self.uuid, self.update_body, + client) + + self.assertFalse(client_mock.called) + self.assertFalse(conf_client_mock.called) + client.update_port.assert_called_once_with(self.uuid, self.update_body) + + @mock.patch.object(neutron, 'get_client', autospec=True) + @mock.patch.object(neutron, '_get_conf_client', autospec=True) + def test_update_neutron_port_with_exception(self, conf_client_mock, + client_mock): + client_mock.return_value.show_port.side_effect = \ + neutron_client_exc.NeutronClientException + conf_client_mock.return_value.update_port.return_value = {'port': {}} + + self.assertRaises( + neutron_client_exc.NeutronClientException, + neutron.update_neutron_port, + self.context, self.uuid, self.update_body) + + client_mock.assert_called_once_with(context=self.context) + client_mock.return_value.show_port.assert_called_once_with(self.uuid) + self.assertFalse(conf_client_mock.called) + + class TestNeutronNetworkActions(db_base.DbTestCase): _CLIENT_ID = ( @@ -172,7 +270,8 @@ class TestNeutronNetworkActions(db_base.DbTestCase): patcher.start() self.addCleanup(patcher.stop) - def _test_add_ports_to_network(self, is_client_id, + @mock.patch.object(neutron, 'update_neutron_port', autospec=True) + def _test_add_ports_to_network(self, update_mock, is_client_id, security_groups=None, add_all_ports=False): # Ports will be created only if pxe_enabled is True @@ -192,14 +291,18 @@ class TestNeutronNetworkActions(db_base.DbTestCase): extra['client-id'] = self._CLIENT_ID port.extra = extra port.save() - expected_body = { + expected_create_body = { 'port': { 'network_id': self.network_uuid, 'admin_state_up': True, 'binding:vnic_type': 'baremetal', + 'device_id': self.node.uuid, + } + } + expected_update_body = { + 'port': { 'device_owner': 'baremetal:none', 'binding:host_id': self.node.uuid, - 'device_id': self.node.uuid, 'mac_address': port.address, 'binding:profile': { 'local_link_information': [port.local_link_connection] @@ -207,16 +310,17 @@ class TestNeutronNetworkActions(db_base.DbTestCase): } } if security_groups: - expected_body['port']['security_groups'] = security_groups + expected_create_body['port']['security_groups'] = security_groups if is_client_id: - expected_body['port']['extra_dhcp_opts'] = ( + expected_create_body['port']['extra_dhcp_opts'] = ( [{'opt_name': '61', 'opt_value': self._CLIENT_ID}]) if add_all_ports: - expected_body2 = copy.deepcopy(expected_body) - expected_body2['port']['mac_address'] = port2.address - expected_body2['fixed_ips'] = [] + expected_create_body2 = copy.deepcopy(expected_create_body) + expected_update_body2 = copy.deepcopy(expected_update_body) + expected_update_body2['port']['mac_address'] = port2.address + expected_create_body2['fixed_ips'] = [] neutron_port2 = {'id': '132f871f-eaec-4fed-9475-0d54465e0f01', 'mac_address': port2.address, 'fixed_ips': []} @@ -237,12 +341,21 @@ class TestNeutronNetworkActions(db_base.DbTestCase): task, self.network_uuid, security_groups=security_groups) self.assertEqual(expected, ports) if add_all_ports: - calls = [mock.call(expected_body), - mock.call(expected_body2)] - self.client_mock.create_port.assert_has_calls(calls) + create_calls = [mock.call(expected_create_body), + mock.call(expected_create_body2)] + update_calls = [ + mock.call(self.context, self.neutron_port['id'], + expected_update_body), + mock.call(self.context, neutron_port2['id'], + expected_update_body2)] + self.client_mock.create_port.assert_has_calls(create_calls) + update_mock.assert_has_calls(update_calls) else: self.client_mock.create_port.assert_called_once_with( - expected_body) + expected_create_body) + update_mock.assert_called_once_with( + self.context, self.neutron_port['id'], + expected_update_body) def test_add_ports_to_network(self): self._test_add_ports_to_network(is_client_id=False, @@ -261,7 +374,8 @@ class TestNeutronNetworkActions(db_base.DbTestCase): self._test_add_ports_to_network(is_client_id=False, security_groups=sg_ids) - def test__add_ip_addresses_for_ipv6_stateful(self): + @mock.patch.object(neutron, 'update_neutron_port', autospec=True) + def test__add_ip_addresses_for_ipv6_stateful(self, mock_update): subnet_id = uuidutils.generate_uuid() self.client_mock.show_subnet.return_value = { 'subnet': { @@ -285,11 +399,12 @@ class TestNeutronNetworkActions(db_base.DbTestCase): } neutron._add_ip_addresses_for_ipv6_stateful( + self.context, {'port': self.neutron_port}, self.client_mock ) - self.client_mock.update_port.assert_called_once_with( - self.neutron_port['id'], expected_body) + mock_update.assert_called_once_with( + self.context, self.neutron_port['id'], expected_body) def test_verify_sec_groups(self): sg_ids = [] @@ -371,20 +486,25 @@ class TestNeutronNetworkActions(db_base.DbTestCase): def test_add_ports_with_client_id_to_network(self): self._test_add_ports_to_network(is_client_id=True) + @mock.patch.object(neutron, 'update_neutron_port', autospec=True) @mock.patch.object(neutron, 'validate_port_info', autospec=True) - def test_add_ports_to_network_instance_uuid(self, vpi_mock): + def test_add_ports_to_network_instance_uuid(self, vpi_mock, update_mock): self.node.instance_uuid = uuidutils.generate_uuid() self.node.network_interface = 'neutron' self.node.save() port = self.ports[0] - expected_body = { + expected_create_body = { 'port': { 'network_id': self.network_uuid, 'admin_state_up': True, 'binding:vnic_type': 'baremetal', + 'device_id': self.node.instance_uuid, + } + } + expected_update_body = { + 'port': { 'device_owner': 'baremetal:none', 'binding:host_id': self.node.uuid, - 'device_id': self.node.instance_uuid, 'mac_address': port.address, 'binding:profile': { 'local_link_information': [port.local_link_connection] @@ -398,7 +518,11 @@ class TestNeutronNetworkActions(db_base.DbTestCase): with task_manager.acquire(self.context, self.node.uuid) as task: ports = neutron.add_ports_to_network(task, self.network_uuid) self.assertEqual(expected, ports) - self.client_mock.create_port.assert_called_once_with(expected_body) + self.client_mock.create_port.assert_called_once_with( + expected_create_body) + update_mock.assert_called_once_with(self.context, + self.neutron_port['id'], + expected_update_body) self.assertTrue(vpi_mock.called) @mock.patch.object(neutron, 'rollback_ports', autospec=True) @@ -413,8 +537,9 @@ class TestNeutronNetworkActions(db_base.DbTestCase): self.network_uuid) rollback_mock.assert_called_once_with(task, self.network_uuid) + @mock.patch.object(neutron, 'update_neutron_port', autospec=True) @mock.patch.object(neutron, 'LOG', autospec=True) - def test_add_network_create_some_ports_fail(self, log_mock): + def test_add_network_create_some_ports_fail(self, log_mock, update_mock): object_utils.create_test_port( self.context, node_id=self.node.id, uuid=uuidutils.generate_uuid(), @@ -788,10 +913,11 @@ class TestNeutronNetworkActions(db_base.DbTestCase): neutron.wait_for_port_status, self.client_mock, 'port_id', 'DOWN') + @mock.patch.object(neutron, 'update_neutron_port', autospec=True) @mock.patch.object(neutron, 'wait_for_host_agent', autospec=True) @mock.patch.object(neutron, 'wait_for_port_status', autospec=True) def test_add_smartnic_port_to_network( - self, wait_port_mock, wait_agent_mock): + self, wait_port_mock, wait_agent_mock, update_mock): # Ports will be created only if pxe_enabled is True self.node.network_interface = 'neutron' self.node.save() @@ -809,14 +935,18 @@ class TestNeutronNetworkActions(db_base.DbTestCase): port.is_smartnic = True port.save() - expected_body = { + expected_create_body = { 'port': { 'network_id': self.network_uuid, 'admin_state_up': True, 'binding:vnic_type': 'smart-nic', + 'device_id': self.node.uuid, + } + } + expected_update_body = { + 'port': { 'device_owner': 'baremetal:none', 'binding:host_id': port.local_link_connection['hostname'], - 'device_id': self.node.uuid, 'mac_address': port.address, 'binding:profile': { 'local_link_information': [port.local_link_connection] @@ -832,7 +962,9 @@ class TestNeutronNetworkActions(db_base.DbTestCase): ports = neutron.add_ports_to_network(task, self.network_uuid) self.assertEqual(expected, ports) self.client_mock.create_port.assert_called_once_with( - expected_body) + expected_create_body) + update_mock.assert_called_once_with( + self.context, self.neutron_port['id'], expected_update_body) wait_agent_mock.assert_called_once_with( self.client_mock, 'hostname') wait_port_mock.assert_called_once_with( @@ -935,18 +1067,20 @@ class TestUpdatePortAddress(base.TestCase): super(TestUpdatePortAddress, self).setUp() self.context = context.RequestContext() - def test_update_port_address(self, mock_client): + @mock.patch.object(neutron, 'update_neutron_port', autospec=True) + def test_update_port_address(self, mock_unp, mock_client): address = 'fe:54:00:77:07:d9' port_id = 'fake-port-id' expected = {'port': {'mac_address': address}} mock_client.return_value.show_port.return_value = {} neutron.update_port_address(port_id, address, context=self.context) - mock_client.return_value.update_port.assert_called_once_with(port_id, - expected) + mock_unp.assert_called_once_with(self.context, port_id, expected) + @mock.patch.object(neutron, 'update_neutron_port', autospec=True) @mock.patch.object(neutron, 'unbind_neutron_port', autospec=True) - def test_update_port_address_with_binding(self, mock_unp, mock_client): + def test_update_port_address_with_binding(self, mock_unp, mock_update, + mock_client): address = 'fe:54:00:77:07:d9' port_id = 'fake-port-id' @@ -954,19 +1088,22 @@ class TestUpdatePortAddress(base.TestCase): 'port': {'binding:host_id': 'host', 'binding:profile': 'foo'}} - calls = [mock.call(port_id, {'port': {'mac_address': address}}), - mock.call(port_id, {'port': {'binding:host_id': 'host', + calls = [mock.call(self.context, port_id, + {'port': {'mac_address': address}}), + mock.call(self.context, port_id, + {'port': {'binding:host_id': 'host', 'binding:profile': 'foo'}})] neutron.update_port_address(port_id, address, context=self.context) mock_unp.assert_called_once_with( port_id, - client=mock_client(context=self.context), context=self.context) - mock_client.return_value.update_port.assert_has_calls(calls) + mock_update.assert_has_calls(calls) + @mock.patch.object(neutron, 'update_neutron_port', autospec=True) @mock.patch.object(neutron, 'unbind_neutron_port', autospec=True) - def test_update_port_address_without_binding(self, mock_unp, mock_client): + def test_update_port_address_without_binding(self, mock_unp, mock_update, + mock_client): address = 'fe:54:00:77:07:d9' port_id = 'fake-port-id' expected = {'port': {'mac_address': address}} @@ -975,7 +1112,7 @@ class TestUpdatePortAddress(base.TestCase): neutron.update_port_address(port_id, address, context=self.context) self.assertFalse(mock_unp.called) - mock_client.return_value.update_port.assert_any_call(port_id, expected) + mock_update.assert_any_call(self.context, port_id, expected) def test_update_port_address_show_failed(self, mock_client): address = 'fe:54:00:77:07:d9' @@ -1002,17 +1139,18 @@ class TestUpdatePortAddress(base.TestCase): port_id, address, context=self.context) mock_unp.assert_called_once_with( port_id, - client=mock_client(context=self.context), context=self.context) self.assertFalse(mock_client.return_value.update_port.called) + @mock.patch.object(neutron, 'update_neutron_port', autospec=True) @mock.patch.object(neutron, 'unbind_neutron_port', autospec=True) def test_update_port_address_with_exception(self, mock_unp, + mock_update, mock_client): address = 'fe:54:00:77:07:d9' port_id = 'fake-port-id' mock_client.return_value.show_port.return_value = {} - mock_client.return_value.update_port.side_effect = ( + mock_update.side_effect = ( neutron_client_exc.NeutronClientException()) self.assertRaises(exception.FailedToUpdateMacOnPort, @@ -1020,14 +1158,14 @@ class TestUpdatePortAddress(base.TestCase): port_id, address, context=self.context) -@mock.patch.object(neutron, 'get_client', autospec=True) +@mock.patch.object(neutron, 'update_neutron_port', autospec=True) class TestUnbindPort(base.TestCase): def setUp(self): super(TestUnbindPort, self).setUp() self.context = context.RequestContext() - def test_unbind_neutron_port_client_passed(self, mock_client): + def test_unbind_neutron_port_client_passed(self, mock_unp): port_id = 'fake-port-id' body_unbind = { 'port': { @@ -1040,20 +1178,18 @@ class TestUnbindPort(base.TestCase): 'mac_address': None } } + client = mock.MagicMock() update_calls = [ - mock.call(port_id, body_unbind), - mock.call(port_id, body_reset_mac) + mock.call(self.context, port_id, body_unbind, client), + mock.call(self.context, port_id, body_reset_mac, client) ] - neutron.unbind_neutron_port(port_id, - mock_client(context=self.context), - context=self.context) - self.assertEqual(1, mock_client.call_count) - mock_client.return_value.update_port.assert_has_calls(update_calls) + neutron.unbind_neutron_port(port_id, client, context=self.context) + self.assertEqual(2, mock_unp.call_count) + mock_unp.assert_has_calls(update_calls) @mock.patch.object(neutron, 'LOG', autospec=True) - def test_unbind_neutron_port_failure(self, mock_log, mock_client): - mock_client.return_value.update_port.side_effect = ( - neutron_client_exc.NeutronClientException()) + def test_unbind_neutron_port_failure(self, mock_log, mock_unp): + mock_unp.side_effect = (neutron_client_exc.NeutronClientException()) body = { 'port': { 'binding:host_id': '', @@ -1063,12 +1199,10 @@ class TestUnbindPort(base.TestCase): port_id = 'fake-port-id' self.assertRaises(exception.NetworkError, neutron.unbind_neutron_port, port_id, context=self.context) - mock_client.assert_called_once_with(context=self.context) - mock_client.return_value.update_port.assert_called_once_with(port_id, - body) + mock_unp.assert_called_once_with(self.context, port_id, body, None) mock_log.exception.assert_called_once() - def test_unbind_neutron_port(self, mock_client): + def test_unbind_neutron_port(self, mock_unp): port_id = 'fake-port-id' body_unbind = { 'port': { @@ -1082,17 +1216,16 @@ class TestUnbindPort(base.TestCase): } } update_calls = [ - mock.call(port_id, body_unbind), - mock.call(port_id, body_reset_mac) + mock.call(self.context, port_id, body_unbind, None), + mock.call(self.context, port_id, body_reset_mac, None) ] neutron.unbind_neutron_port(port_id, context=self.context) - mock_client.assert_called_once_with(context=self.context) - mock_client.return_value.update_port.assert_has_calls(update_calls) + mock_unp.assert_has_calls(update_calls) @mock.patch.object(neutron, 'LOG', autospec=True) - def test_unbind_neutron_port_not_found(self, mock_log, mock_client): + def test_unbind_neutron_port_not_found(self, mock_log, mock_unp): port_id = 'fake-port-id' - mock_client.return_value.update_port.side_effect = ( + mock_unp.side_effect = ( neutron_client_exc.PortNotFoundClient()) body = { 'port': { @@ -1101,9 +1234,7 @@ class TestUnbindPort(base.TestCase): } } neutron.unbind_neutron_port(port_id, context=self.context) - mock_client.assert_called_once_with(context=self.context) - mock_client.return_value.update_port.assert_called_once_with(port_id, - body) + mock_unp.assert_called_once_with(self.context, port_id, body, None) mock_log.info.assert_called_once_with('Port %s was not found while ' 'unbinding.', port_id) diff --git a/ironic/tests/unit/dhcp/test_neutron.py b/ironic/tests/unit/dhcp/test_neutron.py index 4a147e21ca..29983e7d30 100644 --- a/ironic/tests/unit/dhcp/test_neutron.py +++ b/ironic/tests/unit/dhcp/test_neutron.py @@ -48,8 +48,8 @@ class TestNeutron(db_base.DbTestCase): dhcp_factory.DHCPFactory._dhcp_provider = None - @mock.patch('ironic.common.neutron.get_client', autospec=True) - def test_update_port_dhcp_opts(self, client_mock): + @mock.patch('ironic.common.neutron.update_neutron_port', autospec=True) + def test_update_port_dhcp_opts(self, update_mock): opts = [{'opt_name': 'bootfile-name', 'opt_value': 'pxelinux.0'}, {'opt_name': 'tftp-server', @@ -63,14 +63,14 @@ class TestNeutron(db_base.DbTestCase): with task_manager.acquire(self.context, self.node.uuid) as task: api.provider.update_port_dhcp_opts(port_id, opts, context=task.context) - client_mock.return_value.update_port.assert_called_once_with( - port_id, expected) + update_mock.assert_called_once_with( + task.context, port_id, expected) - @mock.patch('ironic.common.neutron.get_client', autospec=True) - def test_update_port_dhcp_opts_with_exception(self, client_mock): + @mock.patch('ironic.common.neutron.update_neutron_port', autospec=True) + def test_update_port_dhcp_opts_with_exception(self, update_mock): opts = [{}] port_id = 'fake-port-id' - client_mock.return_value.update_port.side_effect = ( + update_mock.side_effect = ( neutron_client_exc.NeutronClientException()) api = dhcp_factory.DHCPFactory() diff --git a/ironic/tests/unit/drivers/modules/network/test_common.py b/ironic/tests/unit/drivers/modules/network/test_common.py index b8b4182300..eedd907f67 100644 --- a/ironic/tests/unit/drivers/modules/network/test_common.py +++ b/ironic/tests/unit/drivers/modules/network/test_common.py @@ -400,22 +400,26 @@ class TestCommonFunctions(db_base.DbTestCase): common.get_free_port_like_object, task, self.vif_id, {'physnet2'}) + @mock.patch.object(neutron_common, 'update_neutron_port', autospec=True) @mock.patch.object(neutron_common, 'get_client', autospec=True) - def test_plug_port_to_tenant_network_client(self, mock_gc): + def test_plug_port_to_tenant_network_client(self, mock_gc, mock_update): self.port.internal_info = {common.TENANT_VIF_KEY: self.vif_id} self.port.save() with task_manager.acquire(self.context, self.node.id) as task: common.plug_port_to_tenant_network(task, self.port, client=mock.MagicMock()) self.assertFalse(mock_gc.called) + self.assertTrue(mock_update.called) + @mock.patch.object(neutron_common, 'update_neutron_port', autospec=True) @mock.patch.object(neutron_common, 'get_client', autospec=True) - def test_plug_port_to_tenant_network_no_client(self, mock_gc): + def test_plug_port_to_tenant_network_no_client(self, mock_gc, mock_update): self.port.internal_info = {common.TENANT_VIF_KEY: self.vif_id} self.port.save() with task_manager.acquire(self.context, self.node.id) as task: common.plug_port_to_tenant_network(task, self.port) self.assertTrue(mock_gc.called) + self.assertTrue(mock_update.called) @mock.patch.object(neutron_common, 'get_client', autospec=True) def test_plug_port_to_tenant_network_no_tenant_vif(self, mock_gc): @@ -432,9 +436,10 @@ class TestCommonFunctions(db_base.DbTestCase): @mock.patch.object(neutron_common, 'wait_for_host_agent', autospec=True) @mock.patch.object(neutron_common, 'wait_for_port_status', autospec=True) + @mock.patch.object(neutron_common, 'update_neutron_port', autospec=True) @mock.patch.object(neutron_common, 'get_client', autospec=True) def test_plug_port_to_tenant_network_smartnic_port( - self, mock_gc, wait_port_mock, wait_agent_mock): + self, mock_gc, mock_update, wait_port_mock, wait_agent_mock): nclient = mock.MagicMock() mock_gc.return_value = nclient local_link_connection = self.port.local_link_connection @@ -449,6 +454,7 @@ class TestCommonFunctions(db_base.DbTestCase): nclient, 'hostname') wait_port_mock.assert_called_once_with( nclient, self.vif_id, 'ACTIVE') + self.assertTrue(mock_update.called) class TestVifPortIDMixin(db_base.DbTestCase): diff --git a/ironic/tests/unit/drivers/modules/network/test_flat.py b/ironic/tests/unit/drivers/modules/network/test_flat.py index 10edc6f9df..186827166d 100644 --- a/ironic/tests/unit/drivers/modules/network/test_flat.py +++ b/ironic/tests/unit/drivers/modules/network/test_flat.py @@ -170,10 +170,8 @@ class TestFlatInterface(db_base.DbTestCase): self.port.refresh() self.assertNotIn('cleaning_vif_port_id', self.port.internal_info) - @mock.patch.object(neutron, 'get_client') - def test__bind_flat_ports_set_binding_host_id(self, client_mock): - upd_mock = mock.Mock() - client_mock.return_value.update_port = upd_mock + @mock.patch.object(neutron, 'update_neutron_port') + def test__bind_flat_ports_set_binding_host_id(self, update_mock): extra = {'vif_port_id': 'foo'} utils.create_test_port(self.context, node_id=self.node.id, address='52:54:00:cf:2d:33', extra=extra, @@ -183,12 +181,10 @@ class TestFlatInterface(db_base.DbTestCase): 'mac_address': '52:54:00:cf:2d:33'}} with task_manager.acquire(self.context, self.node.id) as task: self.interface._bind_flat_ports(task) - upd_mock.assert_called_once_with('foo', exp_body) + update_mock.assert_called_once_with(self.context, 'foo', exp_body) - @mock.patch.object(neutron, 'get_client') - def test__bind_flat_ports_set_binding_host_id_portgroup(self, client_mock): - upd_mock = mock.Mock() - client_mock.return_value.update_port = upd_mock + @mock.patch.object(neutron, 'update_neutron_port') + def test__bind_flat_ports_set_binding_host_id_portgroup(self, update_mock): internal_info = {'tenant_vif_port_id': 'foo'} utils.create_test_portgroup( self.context, node_id=self.node.id, internal_info=internal_info, @@ -204,8 +200,9 @@ class TestFlatInterface(db_base.DbTestCase): 'mac_address': '52:54:00:cf:2d:31'}} with task_manager.acquire(self.context, self.node.id) as task: self.interface._bind_flat_ports(task) - upd_mock.assert_has_calls([ - mock.call('bar', exp_body1), mock.call('foo', exp_body2)]) + update_mock.assert_has_calls([ + mock.call(self.context, 'bar', exp_body1), + mock.call(self.context, 'foo', exp_body2)]) @mock.patch.object(neutron, 'unbind_neutron_port') def test__unbind_flat_ports(self, unbind_neutron_port_mock): @@ -234,10 +231,9 @@ class TestFlatInterface(db_base.DbTestCase): [mock.call('foo', context=self.context), mock.call('bar', context=self.context)]) - @mock.patch.object(neutron, 'get_client') - def test__bind_flat_ports_set_binding_host_id_raise(self, client_mock): - client_mock.return_value.update_port.side_effect = \ - (neutron_exceptions.ConnectionFailed()) + @mock.patch.object(neutron, 'update_neutron_port') + def test__bind_flat_ports_set_binding_host_id_raise(self, update_mock): + update_mock.side_effect = (neutron_exceptions.ConnectionFailed()) extra = {'vif_port_id': 'foo'} utils.create_test_port(self.context, node_id=self.node.id, address='52:54:00:cf:2d:33', extra=extra, diff --git a/ironic/tests/unit/drivers/modules/network/test_neutron.py b/ironic/tests/unit/drivers/modules/network/test_neutron.py index 5584b00a88..2d083a740d 100644 --- a/ironic/tests/unit/drivers/modules/network/test_neutron.py +++ b/ironic/tests/unit/drivers/modules/network/test_neutron.py @@ -572,10 +572,11 @@ class NeutronInterfaceTestCase(db_base.DbTestCase): log_mock.error.call_args[0][0]) @mock.patch.object(neutron_common, 'wait_for_host_agent', autospec=True) + @mock.patch.object(neutron_common, 'update_neutron_port') @mock.patch.object(neutron_common, 'get_client') @mock.patch.object(neutron, 'LOG') def test_configure_tenant_networks_multiple_ports_one_vif_id( - self, log_mock, client_mock, wait_agent_mock): + self, log_mock, client_mock, update_mock, wait_agent_mock): expected_body = { 'port': { 'binding:vnic_type': 'baremetal', @@ -585,20 +586,20 @@ class NeutronInterfaceTestCase(db_base.DbTestCase): 'mac_address': '52:54:00:cf:2d:32' } } - upd_mock = mock.Mock() - client_mock.return_value.update_port = upd_mock with task_manager.acquire(self.context, self.node.id) as task: self.interface.configure_tenant_networks(task) client_mock.assert_called_once_with(context=task.context) - upd_mock.assert_called_once_with(self.port.extra['vif_port_id'], - expected_body) + update_mock.assert_called_once_with(self.context, + self.port.extra['vif_port_id'], + expected_body) @mock.patch.object(neutron_common, 'wait_for_host_agent', autospec=True) + @mock.patch.object(neutron_common, 'update_neutron_port') @mock.patch.object(neutron_common, 'get_client') def test_configure_tenant_networks_update_fail(self, client_mock, + update_mock, wait_agent_mock): - client = client_mock.return_value - client.update_port.side_effect = neutron_exceptions.ConnectionFailed( + update_mock.side_effect = neutron_exceptions.ConnectionFailed( reason='meow') with task_manager.acquire(self.context, self.node.id) as task: self.assertRaisesRegex( @@ -607,12 +608,12 @@ class NeutronInterfaceTestCase(db_base.DbTestCase): client_mock.assert_called_once_with(context=task.context) @mock.patch.object(neutron_common, 'wait_for_host_agent', autospec=True) + @mock.patch.object(neutron_common, 'update_neutron_port') @mock.patch.object(neutron_common, 'get_client') - def _test_configure_tenant_networks(self, client_mock, wait_agent_mock, + def _test_configure_tenant_networks(self, client_mock, update_mock, + wait_agent_mock, is_client_id=False, vif_int_info=False): - upd_mock = mock.Mock() - client_mock.return_value.update_port = upd_mock if vif_int_info: kwargs = {'internal_info': { 'tenant_vif_port_id': uuidutils.generate_uuid()}} @@ -668,9 +669,9 @@ class NeutronInterfaceTestCase(db_base.DbTestCase): else: portid1 = self.port.extra['vif_port_id'] portid2 = second_port.extra['vif_port_id'] - upd_mock.assert_has_calls( - [mock.call(portid1, port1_body), - mock.call(portid2, port2_body)], + update_mock.assert_has_calls( + [mock.call(self.context, portid1, port1_body), + mock.call(self.context, portid2, port2_body)], any_order=True ) @@ -693,11 +694,12 @@ class NeutronInterfaceTestCase(db_base.DbTestCase): self._test_configure_tenant_networks(is_client_id=True) @mock.patch.object(neutron_common, 'wait_for_host_agent', autospec=True) + @mock.patch.object(neutron_common, 'update_neutron_port', autospec=True) @mock.patch.object(neutron_common, 'get_client', autospec=True) @mock.patch.object(neutron_common, 'get_local_group_information', autospec=True) def test_configure_tenant_networks_with_portgroups( - self, glgi_mock, client_mock, wait_agent_mock): + self, glgi_mock, client_mock, update_mock, wait_agent_mock): pg = utils.create_test_portgroup( self.context, node_id=self.node.id, address='ff:54:00:cf:2d:32', extra={'vif_port_id': uuidutils.generate_uuid()}) @@ -717,8 +719,6 @@ class NeutronInterfaceTestCase(db_base.DbTestCase): 'port_id': 'Ethernet1/2', 'switch_info': 'switch2'} ) - upd_mock = mock.Mock() - client_mock.return_value.update_port = upd_mock local_group_info = {'a': 'b'} glgi_mock.return_value = local_group_info expected_body = { @@ -747,9 +747,11 @@ class NeutronInterfaceTestCase(db_base.DbTestCase): self.interface.configure_tenant_networks(task) client_mock.assert_called_once_with(context=task.context) glgi_mock.assert_called_once_with(task, pg) - upd_mock.assert_has_calls( - [mock.call(self.port.extra['vif_port_id'], call1_body), - mock.call(pg.extra['vif_port_id'], call2_body)] + update_mock.assert_has_calls( + [mock.call(self.context, self.port.extra['vif_port_id'], + call1_body), + mock.call(self.context, pg.extra['vif_port_id'], + call2_body)] ) def test_need_power_on_true(self): diff --git a/releasenotes/notes/neutron-port-update-598183909d44396c.yaml b/releasenotes/notes/neutron-port-update-598183909d44396c.yaml new file mode 100644 index 0000000000..548874948f --- /dev/null +++ b/releasenotes/notes/neutron-port-update-598183909d44396c.yaml @@ -0,0 +1,8 @@ +--- +features: + - | + Changes neutron port updates to use auth values from Ironic's neutron + conf, preventing issues that can arise when a non-admin user manages + Ironic nodes. A check is added to the port update function to verify that + the user can actually see the port. This adds an additional Neutron + request call to all port updates.