From 8039a9543b2b3153fba02ba15b68aaea722561c4 Mon Sep 17 00:00:00 2001 From: Srinivas Sakhamuri Date: Thu, 4 Jan 2018 15:18:02 -0500 Subject: [PATCH] Ensure cleanup is performed correctly on roles When using existing users (for e.g. LDAP setup) rally when specified roles context it tends to assign and remove roles, which can be problematic if the roles are already assigned to the users. This fix checks whether the user already a member of the role, if so it doesn't attempt to cleanup Closes-Bug: #1720270 Change-Id: I8638eb0e59fdea2fdbbdf87728da70530fd22e14 --- .../openstack/context/keystone/roles.py | 26 ++++++++++++++----- .../openstack/context/keystone/test_roles.py | 18 +++++++++---- 2 files changed, 33 insertions(+), 11 deletions(-) diff --git a/rally/plugins/openstack/context/keystone/roles.py b/rally/plugins/openstack/context/keystone/roles.py index 5f1a6009..69fd83c0 100644 --- a/rally/plugins/openstack/context/keystone/roles.py +++ b/rally/plugins/openstack/context/keystone/roles.py @@ -64,6 +64,12 @@ class RoleGenerator(context.Context): raise exceptions.NotFoundException( "There is no role with name `%s`" % context_role) + def _get_user_role_ids(self, user_id, project_id): + keystone = identity.Identity(osclients.Clients(self.credential)) + user_roles = keystone.list_roles(user_id=user_id, + project_id=project_id) + return [role.id for role in user_roles] + def _get_consumer(self, func_name): def consume(cache, args): role_id, user_id, project_id = args @@ -90,21 +96,29 @@ class RoleGenerator(context.Context): "role_id": role.id, "threads": threads}) for user in self.context["users"]: - args = (role.id, user["id"], user["tenant_id"]) - queue.append(args) + if "roles" not in user: + user["roles"] = self._get_user_role_ids( + user["id"], + user["tenant_id"]) + user["assigned_roles"] = [] + if role.id not in user["roles"]: + args = (role.id, user["id"], user["tenant_id"]) + queue.append(args) + user["assigned_roles"].append(role.id) broker.run(publish, self._get_consumer("add_role"), threads) self.context["roles"] = roles_dict def cleanup(self): - """Remove all roles from users.""" + """Remove assigned roles from users.""" threads = self.workers def publish(queue): for role_id in self.context["roles"]: - LOG.debug("Removing role %s from all users" % role_id) + LOG.debug("Removing assigned role %s from all users" % role_id) for user in self.context["users"]: - args = (role_id, user["id"], user["tenant_id"]) - queue.append(args) + if role_id in user["assigned_roles"]: + args = (role_id, user["id"], user["tenant_id"]) + queue.append(args) broker.run(publish, self._get_consumer("revoke_role"), threads) diff --git a/tests/unit/plugins/openstack/context/keystone/test_roles.py b/tests/unit/plugins/openstack/context/keystone/test_roles.py index 96ad8b23..0272a865 100644 --- a/tests/unit/plugins/openstack/context/keystone/test_roles.py +++ b/tests/unit/plugins/openstack/context/keystone/test_roles.py @@ -87,8 +87,10 @@ class RoleGeneratorTestCase(test.TestCase): ctx = roles.RoleGenerator(self.context) ctx.context["roles"] = {"r1": "test_role1", "r2": "test_role2"} - ctx.context["users"] = [{"id": "u1", "tenant_id": "t1"}, - {"id": "u2", "tenant_id": "t2"}] + ctx.context["users"] = [{"id": "u1", "tenant_id": "t1", + "assigned_roles": ["r1", "r2"]}, + {"id": "u2", "tenant_id": "t2", + "assigned_roles": ["r1", "r2"]}] ctx.credential = mock.MagicMock() ctx.cleanup() calls = [ @@ -107,17 +109,23 @@ class RoleGeneratorTestCase(test.TestCase): mock_osclients.Clients.return_value = fc self.create_default_roles_and_patch_add_remove_functions(fc) + def _get_user_role_ids_side_effect(user_id, project_id): + return ["r1", "r2"] if user_id == "u3" else [] + with roles.RoleGenerator(self.context) as ctx: ctx.context["users"] = [{"id": "u1", "tenant_id": "t1"}, - {"id": "u2", "tenant_id": "t2"}] + {"id": "u2", "tenant_id": "t2"}, + {"id": "u3", "tenant_id": "t3"}] + ctx._get_user_role_ids = mock.MagicMock() + ctx._get_user_role_ids.side_effect = _get_user_role_ids_side_effect ctx.setup() ctx.credential = mock.MagicMock() calls = [ mock.call(user="u1", role="r1", tenant="t1"), mock.call(user="u2", role="r1", tenant="t2"), mock.call(user="u1", role="r2", tenant="t1"), - mock.call(user="u2", role="r2", tenant="t2") + mock.call(user="u2", role="r2", tenant="t2"), ] fc.keystone().roles.add_user_role.assert_has_calls(calls, any_order=True) @@ -128,7 +136,7 @@ class RoleGeneratorTestCase(test.TestCase): self.assertEqual(2, len(ctx.context["roles"])) self.assertEqual(2, len(fc.keystone().roles.list())) - # Cleanup (called by content manager) + # Cleanup (called by context manager) self.assertEqual(2, len(fc.keystone().roles.list())) self.assertEqual(4, fc.keystone().roles.add_user_role.call_count) self.assertEqual(4, fc.keystone().roles.remove_user_role.call_count)