From 3dccebce77d0080d5b5eb3bd9ffb3c77a52bd10a Mon Sep 17 00:00:00 2001 From: Abhishek Kumar Date: Wed, 23 Oct 2024 13:02:33 +0530 Subject: [PATCH] make dynamicapichecker cache confgurable, fix test Signed-off-by: Abhishek Kumar --- .../apache/cloudstack/acl/RoleService.java | 5 +++ .../acl/DynamicRoleBasedAPIAccessChecker.java | 32 +++++++++++++------ .../cloudstack/acl/RoleManagerImpl.java | 2 +- test/integration/smoke/test_dynamicroles.py | 15 ++++++++- 4 files changed, 43 insertions(+), 11 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/acl/RoleService.java b/api/src/main/java/org/apache/cloudstack/acl/RoleService.java index d18c25f43df..aa870f34030 100644 --- a/api/src/main/java/org/apache/cloudstack/acl/RoleService.java +++ b/api/src/main/java/org/apache/cloudstack/acl/RoleService.java @@ -30,6 +30,11 @@ public interface RoleService { ConfigKey EnableDynamicApiChecker = new ConfigKey<>("Advanced", Boolean.class, "dynamic.apichecker.enabled", "false", "If set to true, this enables the dynamic role-based api access checker and disables the default static role-based api access checker.", true); + ConfigKey DynamicApiCheckerCachePeriod = new ConfigKey<>("Advanced", Integer.class, + "dynamic.apichecker.cache.period", "0", + "Defines the expiration time in seconds for the Dynamic API Checker cache, determining how long cached data is retained before being refreshed. If set to zero then caching will be disabled", + false); + boolean isEnabled(); /** diff --git a/plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java b/plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java index 4c1dd621413..ac28d4d2cd6 100644 --- a/plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java +++ b/plugins/acl/dynamic-role-based/src/main/java/org/apache/cloudstack/acl/DynamicRoleBasedAPIAccessChecker.java @@ -51,8 +51,9 @@ public class DynamicRoleBasedAPIAccessChecker extends AdapterBase implements API private List services; private Map> annotationRoleBasedApisMap = new HashMap>(); - final private LazyCache accountCache; - final private LazyCache>> rolePermissionsCache; + private LazyCache accountCache; + private LazyCache>> rolePermissionsCache; + private int cachePeriod; private static final Logger LOGGER = Logger.getLogger(DynamicRoleBasedAPIAccessChecker.class.getName()); @@ -61,10 +62,6 @@ public class DynamicRoleBasedAPIAccessChecker extends AdapterBase implements API for (RoleType roleType : RoleType.values()) { annotationRoleBasedApisMap.put(roleType, new HashSet()); } - accountCache = new LazyCache<>(32, 60, - this::getAccountFromId); - rolePermissionsCache = new LazyCache<>(32, 60, - this::getRolePermissions); } @Override @@ -127,16 +124,30 @@ public class DynamicRoleBasedAPIAccessChecker extends AdapterBase implements API return new Pair<>(accountRole, roleService.findAllPermissionsBy(accountRole.getId())); } + protected Pair> getRolePermissionsUsingCache(long roleId) { + if (cachePeriod > 0) { + return rolePermissionsCache.get(roleId); + } + return getRolePermissions(roleId); + } + + protected Account getAccountFromIdUsingCache(long accountId) { + if (cachePeriod > 0) { + return accountCache.get(accountId); + } + return getAccountFromId(accountId); + } + @Override public boolean checkAccess(User user, String commandName) throws PermissionDeniedException { if (!isEnabled()) { return true; } - Account account = accountCache.get(user.getAccountId()); + Account account = getAccountFromIdUsingCache(user.getAccountId()); if (account == null) { throw new PermissionDeniedException(String.format("Account for user id [%s] cannot be found", user.getUuid())); } - Pair> roleAndPermissions = rolePermissionsCache.get(account.getRoleId()); + Pair> roleAndPermissions = getRolePermissionsUsingCache(account.getRoleId()); final Role accountRole = roleAndPermissions.first(); if (accountRole == null) { throw new PermissionDeniedException(String.format("Account role for user id [%s] cannot be found.", user.getUuid())); @@ -153,7 +164,7 @@ public class DynamicRoleBasedAPIAccessChecker extends AdapterBase implements API } public boolean checkAccess(Account account, String commandName) { - Pair> roleAndPermissions = rolePermissionsCache.get(account.getRoleId()); + Pair> roleAndPermissions = getRolePermissionsUsingCache(account.getRoleId()); final Role accountRole = roleAndPermissions.first(); if (accountRole == null) { throw new PermissionDeniedException(String.format("The account [%s] has role null or unknown.", account)); @@ -198,6 +209,9 @@ public class DynamicRoleBasedAPIAccessChecker extends AdapterBase implements API @Override public boolean configure(String name, Map params) throws ConfigurationException { super.configure(name, params); + cachePeriod = Math.max(0, RoleService.DynamicApiCheckerCachePeriod.value()); + accountCache = new LazyCache<>(32, cachePeriod, this::getAccountFromId); + rolePermissionsCache = new LazyCache<>(32, cachePeriod, this::getRolePermissions); return true; } diff --git a/server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java b/server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java index 08a0dc56080..1bb5fb1685e 100644 --- a/server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/acl/RoleManagerImpl.java @@ -486,7 +486,7 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu @Override public ConfigKey[] getConfigKeys() { - return new ConfigKey[] {RoleService.EnableDynamicApiChecker}; + return new ConfigKey[] {RoleService.EnableDynamicApiChecker, RoleService.DynamicApiCheckerCachePeriod}; } @Override diff --git a/test/integration/smoke/test_dynamicroles.py b/test/integration/smoke/test_dynamicroles.py index b91ba9c2eba..e404835fbb8 100644 --- a/test/integration/smoke/test_dynamicroles.py +++ b/test/integration/smoke/test_dynamicroles.py @@ -18,7 +18,7 @@ from marvin.cloudstackAPI import * from marvin.cloudstackTestCase import cloudstackTestCase from marvin.cloudstackException import CloudstackAPIException -from marvin.lib.base import Account, Role, RolePermission +from marvin.lib.base import Account, Role, RolePermission, Configurations from marvin.lib.utils import cleanup_resources from nose.plugins.attrib import attr from random import shuffle @@ -26,6 +26,7 @@ from random import shuffle import copy import random import re +import time class TestData(object): @@ -109,6 +110,14 @@ class TestDynamicRoles(cloudstackTestCase): self.testdata["account"], roleid=self.role.id ) + + cache_period_config = Configurations.list( + self.apiclient, + name='dynamic.apichecker.cache.period' + )[0] + + self.cache_period = int(cache_period_config.value) + self.cleanup = [ self.account, self.rolepermission, @@ -623,6 +632,8 @@ class TestDynamicRoles(cloudstackTestCase): testdata ) + time.sleep(self.cache_period + 5) + userApiClient = self.getUserApiClient(self.account.name, domain=self.account.domain, role_type=self.account.roletype) # Perform listApis check @@ -645,6 +656,8 @@ class TestDynamicRoles(cloudstackTestCase): self.dbclient.execute("insert into role_permissions (uuid, role_id, rule, permission, sort_order) values (UUID(), %d, '%s', '%s', %d)" % (roleId, rule, perm.upper(), sortOrder)) sortOrder += 1 + time.sleep(self.cache_period + 5) + userApiClient = self.getUserApiClient(self.account.name, domain=self.account.domain, role_type=self.account.roletype) # Perform listApis check