From 7bfd88aa37d414d437dae9125a91bbc036eff059 Mon Sep 17 00:00:00 2001 From: Vasyl Saienko Date: Thu, 19 Jan 2017 13:16:54 +0200 Subject: [PATCH] Do not use user token in neutron client Ironic requires admin rights in Neutron to perform certain operations like (create provisioning/cleaning ports, unbind port, update mac_address). Previously only admins were able to use ironic, with keystone policy implementation it is possible that baremetal admin doesn't have enough rights (admin rights) in Neutron with leads to neutron operations failures. This patch ensures that we do not pass user token to neutron client, and always pick admin session. Closes-Bug: #1657675 Change-Id: If17d5501062075fb8d6ca0eb4f2f38c87e2c2cc3 --- ironic/common/neutron.py | 14 +++++------ ironic/dhcp/neutron.py | 5 ++-- ironic/drivers/modules/network/common.py | 12 ++++------ ironic/drivers/modules/network/flat.py | 2 +- ironic/drivers/modules/network/neutron.py | 5 ++-- ironic/tests/unit/common/test_neutron.py | 24 ++++--------------- ironic/tests/unit/dhcp/test_neutron.py | 15 ++++-------- .../drivers/modules/network/test_common.py | 18 ++++++-------- .../drivers/modules/network/test_neutron.py | 12 +++++----- ...er-not-neutron-admin-f163df90ab520dad.yaml | 5 ++++ 10 files changed, 42 insertions(+), 70 deletions(-) create mode 100644 releasenotes/notes/fix-baremetal-admin-user-not-neutron-admin-f163df90ab520dad.yaml diff --git a/ironic/common/neutron.py b/ironic/common/neutron.py index 8efecdcb5b..5d4c8342c9 100644 --- a/ironic/common/neutron.py +++ b/ironic/common/neutron.py @@ -69,20 +69,19 @@ def get_client(token=None): return clientv20.Client(**params) -def unbind_neutron_port(port_id, client=None, token=None): +def unbind_neutron_port(port_id, client=None): """Unbind a neutron port Remove a neutron port's binding profile and host ID so that it returns to an unbound state. :param port_id: Neutron port ID. - :param token: Optional auth token. :param client: Optional a Neutron client object. :raises: NetworkError """ if not client: - client = get_client(token) + client = get_client() body = {'port': {'binding:host_id': '', 'binding:profile': {}}} @@ -97,15 +96,14 @@ def unbind_neutron_port(port_id, client=None, token=None): raise exception.NetworkError(msg) -def update_port_address(port_id, address, token=None): +def update_port_address(port_id, address): """Update a port's mac address. :param port_id: Neutron port id. :param address: new MAC address. - :param token: optional auth token. :raises: FailedToUpdateMacOnPort """ - client = get_client(token) + client = get_client() port_req_body = {'port': {'mac_address': address}} try: @@ -178,7 +176,7 @@ def add_ports_to_network(task, network_uuid, is_flat=False, :raises: NetworkError :returns: a dictionary in the form {port.uuid: neutron_port['id']} """ - client = get_client(task.context.auth_token) + client = get_client() node = task.node # If Security Groups are specified, verify that they exist @@ -283,7 +281,7 @@ def remove_neutron_ports(task, params): :param params: Dict of params to filter ports. :raises: NetworkError """ - client = get_client(task.context.auth_token) + client = get_client() node_uuid = task.node.uuid try: diff --git a/ironic/dhcp/neutron.py b/ironic/dhcp/neutron.py index ef706ecae2..f9b0b9e878 100644 --- a/ironic/dhcp/neutron.py +++ b/ironic/dhcp/neutron.py @@ -125,8 +125,7 @@ class NeutronDHCPApi(base.BaseDHCP): vif_list = [vif for pdict in vifs.values() for vif in pdict.values()] for vif in vif_list: try: - self.update_port_dhcp_opts(vif, options, - token=task.context.auth_token) + self.update_port_dhcp_opts(vif, options) except exception.FailedToUpdateDHCPOptOnPort: failures.append(vif) @@ -263,7 +262,7 @@ class NeutronDHCPApi(base.BaseDHCP): :returns: List of IP addresses associated with task's ports/portgroups. """ - client = neutron.get_client(task.context.auth_token) + client = neutron.get_client() port_ip_addresses = self._get_ip_addresses(task, task.ports, client) portgroup_ip_addresses = self._get_ip_addresses( diff --git a/ironic/drivers/modules/network/common.py b/ironic/drivers/modules/network/common.py index 800e8a7994..bde6aab2e6 100644 --- a/ironic/drivers/modules/network/common.py +++ b/ironic/drivers/modules/network/common.py @@ -155,9 +155,7 @@ class VIFPortIDMixin(object): port_obj.extra.get('vif_port_id')) if 'address' in port_obj.obj_what_changed(): if vif: - neutron.update_port_address(vif, - port_obj.address, - token=context.auth_token) + neutron.update_port_address(vif, port_obj.address) if 'extra' in port_obj.obj_what_changed(): original_port = objects.Port.get_by_id(context, port_obj.id) @@ -176,7 +174,7 @@ class VIFPortIDMixin(object): 'opt_value': updated_client_id} api.provider.update_port_dhcp_opts( - vif, [client_id_opt], token=context.auth_token) + vif, [client_id_opt]) # Log warning if there is no VIF and an instance # is associated with the node. elif node.instance_uuid: @@ -223,9 +221,7 @@ class VIFPortIDMixin(object): pg_vif = (portgroup_obj.internal_info.get(TENANT_VIF_KEY) or portgroup_obj.extra.get('vif_port_id')) if pg_vif: - neutron.update_port_address(pg_vif, - portgroup_obj.address, - token=context.auth_token) + neutron.update_port_address(pg_vif, portgroup_obj.address) if 'extra' in portgroup_obj.obj_what_changed(): original_portgroup = objects.Portgroup.get_by_id(context, @@ -294,7 +290,7 @@ class VIFPortIDMixin(object): # Check if the requested vif_id is a neutron port. If it is # then attempt to update the port's MAC address. try: - client = neutron.get_client(task.context.auth_token) + client = neutron.get_client() client.show_port(vif_id) except neutron_exceptions.NeutronClientException: # NOTE(sambetts): If a client error occurs this is because diff --git a/ironic/drivers/modules/network/flat.py b/ironic/drivers/modules/network/flat.py index aadc9614d5..ecff720e05 100644 --- a/ironic/drivers/modules/network/flat.py +++ b/ironic/drivers/modules/network/flat.py @@ -69,7 +69,7 @@ class FlatNetwork(common.VIFPortIDMixin, neutron.NeutronNetworkInterfaceMixin, if not host_id: return - client = neutron.get_client(task.context.auth_token) + client = neutron.get_client() for port_like_obj in task.ports + task.portgroups: vif_port_id = ( port_like_obj.internal_info.get('tenant_vif_port_id') or diff --git a/ironic/drivers/modules/network/neutron.py b/ironic/drivers/modules/network/neutron.py index bfd0272582..f42b1de28e 100644 --- a/ironic/drivers/modules/network/neutron.py +++ b/ironic/drivers/modules/network/neutron.py @@ -162,7 +162,7 @@ class NeutronNetwork(common.VIFPortIDMixin, portmap = neutron.get_node_portmap(task) - client = neutron.get_client(task.context.auth_token) + client = neutron.get_client() pobj_without_vif = 0 for port_like_obj in ports + portgroups: vif_port_id = ( @@ -244,5 +244,4 @@ class NeutronNetwork(common.VIFPortIDMixin, port_like_obj.extra.get('vif_port_id')) if not vif_port_id: continue - neutron.unbind_neutron_port(vif_port_id, - token=task.context.auth_token) + neutron.unbind_neutron_port(vif_port_id) diff --git a/ironic/tests/unit/common/test_neutron.py b/ironic/tests/unit/common/test_neutron.py index f6175f2e44..799d5d0b5d 100644 --- a/ironic/tests/unit/common/test_neutron.py +++ b/ironic/tests/unit/common/test_neutron.py @@ -609,20 +609,6 @@ class TestUnbindPort(base.TestCase): mock_client.return_value.update_port.assert_called_once_with(port_id, body) - def test_unbind_neutron_port_token_passed(self, mock_client): - port_id = 'fake-port-id' - token = 'token' - body = { - 'port': { - 'binding:host_id': '', - 'binding:profile': {} - } - } - neutron.unbind_neutron_port(port_id, token=token) - mock_client.assert_called_once_with(token) - mock_client.return_value.update_port.assert_called_once_with(port_id, - body) - @mock.patch.object(neutron, 'LOG') def test_unbind_neutron_port_failure(self, mock_log, mock_client): mock_client.return_value.update_port.side_effect = ( @@ -634,24 +620,22 @@ class TestUnbindPort(base.TestCase): } } port_id = 'fake-port-id' - token = 'token' self.assertRaises(exception.NetworkError, neutron.unbind_neutron_port, - port_id, token=token) - mock_client.assert_called_once_with(token) + port_id) + mock_client.assert_called_once_with() mock_client.return_value.update_port.assert_called_once_with(port_id, body) mock_log.exception.assert_called_once() def test_unbind_neutron_port(self, mock_client): port_id = 'fake-port-id' - token = 'token' body = { 'port': { 'binding:host_id': '', 'binding:profile': {} } } - neutron.unbind_neutron_port(port_id, token=token) - mock_client.assert_called_once_with(token) + neutron.unbind_neutron_port(port_id) + mock_client.assert_called_once_with() mock_client.return_value.update_port.assert_called_once_with(port_id, body) diff --git a/ironic/tests/unit/dhcp/test_neutron.py b/ironic/tests/unit/dhcp/test_neutron.py index b429699fc6..a0643c3f0c 100644 --- a/ironic/tests/unit/dhcp/test_neutron.py +++ b/ironic/tests/unit/dhcp/test_neutron.py @@ -117,8 +117,7 @@ class TestNeutron(db_base.DbTestCase): opts = pxe_utils.dhcp_options_for_instance(task) api = dhcp_factory.DHCPFactory() api.update_dhcp(task, opts) - mock_updo.assert_called_once_with('vif-uuid', opts, - token=self.context.auth_token) + mock_updo.assert_called_once_with('vif-uuid', opts) @mock.patch('ironic.dhcp.neutron.NeutronDHCPApi.update_port_dhcp_opts') @mock.patch('ironic.common.network.get_node_vif_ids') @@ -179,8 +178,7 @@ class TestNeutron(db_base.DbTestCase): api = dhcp_factory.DHCPFactory() api.update_dhcp(task, opts) mock_ts.assert_called_with(30) - mock_updo.assert_called_once_with(mock.ANY, 'vif-uuid', opts, - token=self.context.auth_token) + mock_updo.assert_called_once_with(mock.ANY, 'vif-uuid', opts) @mock.patch.object(neutron, 'LOG', autospec=True) @mock.patch('time.sleep', autospec=True) @@ -201,8 +199,7 @@ class TestNeutron(db_base.DbTestCase): self.assertIn('Setting the port delay to 15 for SSH', mock_log.warning.call_args[0][0]) mock_ts.assert_called_with(15) - mock_updo.assert_called_once_with(mock.ANY, 'vif-uuid', opts, - token=self.context.auth_token) + mock_updo.assert_called_once_with(mock.ANY, 'vif-uuid', opts) @mock.patch.object(neutron, 'LOG', autospec=True) @mock.patch('time.sleep', autospec=True) @@ -223,8 +220,7 @@ class TestNeutron(db_base.DbTestCase): "Waiting %d seconds for Neutron.", 30) mock_log.warning.assert_not_called() mock_ts.assert_called_with(30) - mock_updo.assert_called_once_with(mock.ANY, 'vif-uuid', opts, - token=self.context.auth_token) + mock_updo.assert_called_once_with(mock.ANY, 'vif-uuid', opts) @mock.patch.object(neutron, 'LOG', autospec=True) @mock.patch.object(neutron.NeutronDHCPApi, 'update_port_dhcp_opts', @@ -241,8 +237,7 @@ class TestNeutron(db_base.DbTestCase): api.update_dhcp(task, opts) mock_log.debug.assert_not_called() mock_log.warning.assert_not_called() - mock_updo.assert_called_once_with(mock.ANY, 'vif-uuid', opts, - token=self.context.auth_token) + mock_updo.assert_called_once_with(mock.ANY, 'vif-uuid', opts) def test__get_fixed_ip_address(self): port_id = 'fake-port-id' diff --git a/ironic/tests/unit/drivers/modules/network/test_common.py b/ironic/tests/unit/drivers/modules/network/test_common.py index 5064c00af8..bea4a63766 100644 --- a/ironic/tests/unit/drivers/modules/network/test_common.py +++ b/ironic/tests/unit/drivers/modules/network/test_common.py @@ -257,7 +257,7 @@ class TestVifPortIDMixin(db_base.DbTestCase): self.port.refresh() self.assertEqual("fake_vif_id", self.port.internal_info.get( common.TENANT_VIF_KEY)) - mock_client.assert_called_once_with(None) + mock_client.assert_called_once_with() mock_upa.assert_called_once_with("fake_vif_id", self.port.address) @mock.patch.object(common, 'get_free_port_like_object', autospec=True) @@ -292,7 +292,7 @@ class TestVifPortIDMixin(db_base.DbTestCase): self.assertRaisesRegexp( exception.NetworkError, "can not update Neutron port", self.interface.vif_attach, task, vif) - mock_client.assert_called_once_with(None) + mock_client.assert_called_once_with() def test_vif_detach_in_extra(self): with task_manager.acquire(self.context, self.node.id) as task: @@ -402,8 +402,7 @@ class TestVifPortIDMixin(db_base.DbTestCase): with task_manager.acquire(self.context, self.node.id) as task: self.interface.port_changed(task, self.port) mac_update_mock.assert_called_once_with( - self.port.extra['vif_port_id'], - new_address, token=task.context.auth_token) + self.port.extra['vif_port_id'], new_address) self.assertFalse(mock_warn.called) @mock.patch.object(neutron_common, 'update_port_address', autospec=True) @@ -417,8 +416,7 @@ class TestVifPortIDMixin(db_base.DbTestCase): self.interface.port_changed, task, self.port) mac_update_mock.assert_called_once_with( - self.port.extra['vif_port_id'], - new_address, token=task.context.auth_token) + self.port.extra['vif_port_id'], new_address) @mock.patch.object(neutron_common, 'update_port_address', autospec=True) def test_port_changed_address_no_vif_id(self, mac_update_mock): @@ -437,7 +435,7 @@ class TestVifPortIDMixin(db_base.DbTestCase): with task_manager.acquire(self.context, self.node.id) as task: self.interface.port_changed(task, self.port) dhcp_update_mock.assert_called_once_with( - 'fake-id', expected_dhcp_opts, token=self.context.auth_token) + 'fake-id', expected_dhcp_opts) @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', autospec=True) @@ -656,8 +654,7 @@ class TestVifPortIDMixin(db_base.DbTestCase): pg.address = new_address with task_manager.acquire(self.context, self.node.id) as task: self.interface.portgroup_changed(task, pg) - mac_update_mock.assert_called_once_with('fake-id', new_address, - token=self.context.auth_token) + mac_update_mock.assert_called_once_with('fake-id', new_address) @mock.patch.object(neutron_common, 'update_port_address', autospec=True) def test_update_portgroup_remove_address(self, mac_update_mock): @@ -724,8 +721,7 @@ class TestVifPortIDMixin(db_base.DbTestCase): self.assertRaises(exception.FailedToUpdateMacOnPort, self.interface.portgroup_changed, task, pg) - mac_update_mock.assert_called_once_with('fake-id', new_address, - token=self.context.auth_token) + mac_update_mock.assert_called_once_with('fake-id', new_address) @mock.patch.object(common_utils, 'warn_about_deprecated_extra_vif_port_id', autospec=True) diff --git a/ironic/tests/unit/drivers/modules/network/test_neutron.py b/ironic/tests/unit/drivers/modules/network/test_neutron.py index efe0f22fa7..d7eb0fb888 100644 --- a/ironic/tests/unit/drivers/modules/network/test_neutron.py +++ b/ironic/tests/unit/drivers/modules/network/test_neutron.py @@ -220,7 +220,7 @@ class NeutronInterfaceTestCase(db_base.DbTestCase): with task_manager.acquire(self.context, self.node.id) as task: self.interface.unconfigure_tenant_networks(task) mock_unbind_port.assert_called_once_with( - self.port.extra['vif_port_id'], token=None) + self.port.extra['vif_port_id']) def test_configure_tenant_networks_no_ports_for_node(self): n = utils.create_test_node(self.context, network_interface='neutron', @@ -243,7 +243,7 @@ class NeutronInterfaceTestCase(db_base.DbTestCase): 'associated with node', self.interface.configure_tenant_networks, task) - client_mock.assert_called_once_with(task.context.auth_token) + client_mock.assert_called_once_with() upd_mock.assert_not_called() self.assertIn('No neutron ports or portgroups are associated with', log_mock.error.call_args[0][0]) @@ -270,7 +270,7 @@ class NeutronInterfaceTestCase(db_base.DbTestCase): 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(task.context.auth_token) + client_mock.assert_called_once_with() upd_mock.assert_called_once_with(self.port.extra['vif_port_id'], expected_body) @@ -283,7 +283,7 @@ class NeutronInterfaceTestCase(db_base.DbTestCase): self.assertRaisesRegexp( exception.NetworkError, 'Could not add', self.interface.configure_tenant_networks, task) - client_mock.assert_called_once_with(task.context.auth_token) + client_mock.assert_called_once_with() @mock.patch.object(neutron_common, 'get_client') def _test_configure_tenant_networks(self, client_mock, is_client_id=False, @@ -339,7 +339,7 @@ class NeutronInterfaceTestCase(db_base.DbTestCase): [{'opt_name': 'client-id', 'opt_value': client_ids[1]}]) with task_manager.acquire(self.context, self.node.id) as task: self.interface.configure_tenant_networks(task) - client_mock.assert_called_once_with(task.context.auth_token) + client_mock.assert_called_once_with() if vif_int_info: portid1 = self.port.internal_info['tenant_vif_port_id'] portid2 = second_port.internal_info['tenant_vif_port_id'] @@ -413,7 +413,7 @@ class NeutronInterfaceTestCase(db_base.DbTestCase): } with task_manager.acquire(self.context, self.node.id) as task: self.interface.configure_tenant_networks(task) - client_mock.assert_called_once_with(task.context.auth_token) + client_mock.assert_called_once_with() upd_mock.assert_has_calls( [mock.call(self.port.extra['vif_port_id'], call1_body), mock.call(pg.extra['vif_port_id'], call2_body)] diff --git a/releasenotes/notes/fix-baremetal-admin-user-not-neutron-admin-f163df90ab520dad.yaml b/releasenotes/notes/fix-baremetal-admin-user-not-neutron-admin-f163df90ab520dad.yaml new file mode 100644 index 0000000000..f9f1faabf4 --- /dev/null +++ b/releasenotes/notes/fix-baremetal-admin-user-not-neutron-admin-f163df90ab520dad.yaml @@ -0,0 +1,5 @@ +--- +fixes: + - An issue when baremetal admin user doesn't have enough rights (admin) + in Neutron by always picking neutron user from ironic config + and avoiding passing client token.