diff --git a/api/src/org/apache/cloudstack/acl/APILimitChecker.java b/api/src/org/apache/cloudstack/acl/APILimitChecker.java index 3a1db705e70..110742c059d 100644 --- a/api/src/org/apache/cloudstack/acl/APILimitChecker.java +++ b/api/src/org/apache/cloudstack/acl/APILimitChecker.java @@ -16,6 +16,8 @@ // under the License. package org.apache.cloudstack.acl; +import org.apache.cloudstack.api.ServerApiException; + import com.cloud.user.Account; import com.cloud.utils.component.Adapter; @@ -24,5 +26,5 @@ import com.cloud.utils.component.Adapter; */ public interface APILimitChecker extends Adapter { // Interface for checking if the account is over its api limit - boolean isUnderLimit(Account account); + void checkLimit(Account account) throws ServerApiException; } diff --git a/client/pom.xml b/client/pom.xml index 1bbae1f7d08..7ebe50c48f9 100644 --- a/client/pom.xml +++ b/client/pom.xml @@ -30,6 +30,11 @@ cloud-plugin-acl-static-role-based ${project.version} + + org.apache.cloudstack + cloud-plugin-api-limit-account-based + ${project.version} + org.apache.cloudstack cloud-plugin-api-discovery diff --git a/client/tomcatconf/components.xml.in b/client/tomcatconf/components.xml.in index bb39839c820..dcff94a77e0 100755 --- a/client/tomcatconf/components.xml.in +++ b/client/tomcatconf/components.xml.in @@ -56,6 +56,9 @@ under the License. + + + @@ -180,6 +183,11 @@ under the License. + + 1 + 25 + 50000 + diff --git a/plugins/api/rate-limit/src/org/apache/cloudstack/api/commands/admin/ratelimit/ResetApiLimitCmd.java b/plugins/api/rate-limit/src/org/apache/cloudstack/api/command/admin/ratelimit/ResetApiLimitCmd.java similarity index 98% rename from plugins/api/rate-limit/src/org/apache/cloudstack/api/commands/admin/ratelimit/ResetApiLimitCmd.java rename to plugins/api/rate-limit/src/org/apache/cloudstack/api/command/admin/ratelimit/ResetApiLimitCmd.java index 8029ab3707a..3c612faf302 100644 --- a/plugins/api/rate-limit/src/org/apache/cloudstack/api/commands/admin/ratelimit/ResetApiLimitCmd.java +++ b/plugins/api/rate-limit/src/org/apache/cloudstack/api/command/admin/ratelimit/ResetApiLimitCmd.java @@ -14,7 +14,7 @@ // KIND, either express or implied. See the License for the // specific language governing permissions and limitations // under the License. -package org.apache.cloudstack.api.commands.admin.ratelimit; +package org.apache.cloudstack.api.command.admin.ratelimit; import org.apache.cloudstack.api.*; import org.apache.cloudstack.api.response.AccountResponse; diff --git a/plugins/api/rate-limit/src/org/apache/cloudstack/api/command/user/ratelimit/GetApiLimitCmd.java b/plugins/api/rate-limit/src/org/apache/cloudstack/api/command/user/ratelimit/GetApiLimitCmd.java index 3f9e4eb4503..0397fa8ab54 100644 --- a/plugins/api/rate-limit/src/org/apache/cloudstack/api/command/user/ratelimit/GetApiLimitCmd.java +++ b/plugins/api/rate-limit/src/org/apache/cloudstack/api/command/user/ratelimit/GetApiLimitCmd.java @@ -27,7 +27,7 @@ import org.apache.cloudstack.api.Parameter; import org.apache.cloudstack.api.PlugService; import org.apache.cloudstack.api.ServerApiException; import org.apache.cloudstack.api.BaseCmd.CommandType; -import org.apache.cloudstack.api.commands.admin.ratelimit.ResetApiLimitCmd; +import org.apache.cloudstack.api.command.admin.ratelimit.ResetApiLimitCmd; import org.apache.cloudstack.api.response.AccountResponse; import org.apache.cloudstack.api.response.ApiLimitResponse; import org.apache.cloudstack.api.response.PhysicalNetworkResponse; diff --git a/plugins/api/rate-limit/src/org/apache/cloudstack/ratelimit/ApiRateLimitService.java b/plugins/api/rate-limit/src/org/apache/cloudstack/ratelimit/ApiRateLimitService.java index e7ba9d45fc3..8c9d49b007f 100644 --- a/plugins/api/rate-limit/src/org/apache/cloudstack/ratelimit/ApiRateLimitService.java +++ b/plugins/api/rate-limit/src/org/apache/cloudstack/ratelimit/ApiRateLimitService.java @@ -16,8 +16,8 @@ // under the License. package org.apache.cloudstack.ratelimit; +import org.apache.cloudstack.api.command.admin.ratelimit.ResetApiLimitCmd; import org.apache.cloudstack.api.command.user.ratelimit.GetApiLimitCmd; -import org.apache.cloudstack.api.commands.admin.ratelimit.ResetApiLimitCmd; import org.apache.cloudstack.api.response.ApiLimitResponse; import org.apache.cloudstack.api.response.ListResponse; diff --git a/plugins/api/rate-limit/src/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java b/plugins/api/rate-limit/src/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java index e14f65d383e..00f39af1e1e 100644 --- a/plugins/api/rate-limit/src/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java +++ b/plugins/api/rate-limit/src/org/apache/cloudstack/ratelimit/ApiRateLimitServiceImpl.java @@ -25,19 +25,18 @@ import net.sf.ehcache.CacheManager; import org.apache.log4j.Logger; -import com.cloud.configuration.Config; -import com.cloud.configuration.dao.ConfigurationDao; import org.apache.cloudstack.acl.APILimitChecker; +import org.apache.cloudstack.api.BaseCmd; +import org.apache.cloudstack.api.ServerApiException; +import org.apache.cloudstack.api.command.admin.ratelimit.ResetApiLimitCmd; import org.apache.cloudstack.api.command.user.ratelimit.GetApiLimitCmd; -import org.apache.cloudstack.api.commands.admin.ratelimit.ResetApiLimitCmd; import org.apache.cloudstack.api.response.ApiLimitResponse; -import com.cloud.network.element.NetworkElement; import com.cloud.user.Account; import com.cloud.user.UserContext; +import com.cloud.utils.PropertiesUtil; import com.cloud.utils.component.AdapterBase; -import com.cloud.utils.component.Inject; -@Local(value = NetworkElement.class) +@Local(value = APILimitChecker.class) public class ApiRateLimitServiceImpl extends AdapterBase implements APILimitChecker, ApiRateLimitService { private static final Logger s_logger = Logger.getLogger(ApiRateLimitServiceImpl.class); @@ -51,33 +50,40 @@ public class ApiRateLimitServiceImpl extends AdapterBase implements APILimitChec */ private int maxAllowed = 30; - @Inject - ConfigurationDao _configDao; - - private LimitStore _store; + private LimitStore _store = null; @Override public boolean configure(String name, Map params) throws ConfigurationException { super.configure(name, params); - // get global configured duration and max values - String duration = _configDao.getValue(Config.ApiLimitInterval.key()); - if (duration != null ){ - timeToLive = Integer.parseInt(duration); + + if (_store == null) { + // not configured yet, note that since this class is both adapter + // and pluggableService, so this method + // may be invoked twice in ComponentLocator. + // get global configured duration and max values + Object duration = params.get("api.throttling.interval"); + if (duration != null) { + timeToLive = Integer.parseInt((String) duration); + } + Object maxReqs = params.get("api.throttling.max"); + if (maxReqs != null) { + maxAllowed = Integer.parseInt((String) maxReqs); + } + // create limit store + EhcacheLimitStore cacheStore = new EhcacheLimitStore(); + int maxElements = 10000; + Object cachesize = params.get("api.throttling.cachesize"); + if ( cachesize != null ){ + maxElements = Integer.parseInt((String)cachesize); + } + CacheManager cm = CacheManager.create(); + Cache cache = new Cache("api-limit-cache", maxElements, false, false, timeToLive, timeToLive); + cm.addCache(cache); + s_logger.info("Limit Cache created: " + cache.toString()); + cacheStore.setCache(cache); + _store = cacheStore; } - String maxReqs = _configDao.getValue(Config.ApiLimitMax.key()); - if ( maxReqs != null){ - maxAllowed = Integer.parseInt(maxReqs); - } - // create limit store - EhcacheLimitStore cacheStore = new EhcacheLimitStore(); - int maxElements = 10000; //TODO: what should be the proper number here? - CacheManager cm = CacheManager.create(); - Cache cache = new Cache("api-limit-cache", maxElements, true, false, timeToLive, timeToLive); - cm.addCache(cache); - s_logger.info("Limit Cache created: " + cache.toString()); - cacheStore.setCache(cache); - _store = cacheStore; return true; @@ -123,9 +129,8 @@ public class ApiRateLimitServiceImpl extends AdapterBase implements APILimitChec } - @Override - public boolean isUnderLimit(Account account) { + public void checkLimit(Account account) throws ServerApiException { Long accountId = account.getId(); StoreEntry entry = _store.get(accountId); @@ -140,17 +145,21 @@ public class ApiRateLimitServiceImpl extends AdapterBase implements APILimitChec int current = entry.incrementAndGet(); if (current <= maxAllowed) { - return true; + return; } else { - return false; + long expireAfter = entry.getExpireDuration(); + s_logger.warn("The given user has reached his/her account api limit, please retry after " + expireAfter + " ms."); + throw new ServerApiException(BaseCmd.API_LIMIT_EXCEED, "The given user has reached his/her account api limit, please retry after " + + expireAfter + " ms."); } } @Override - public String[] getPropertiesFiles() { - return new String[] { "api-limit_commands.properties" }; + public Map getProperties() { + return PropertiesUtil.processConfigFile(new String[] + { "api-limit_commands.properties" }); } diff --git a/plugins/api/rate-limit/src/org/apache/cloudstack/ratelimit/StoreEntryImpl.java b/plugins/api/rate-limit/src/org/apache/cloudstack/ratelimit/StoreEntryImpl.java index 40965d9da8e..e8143e52370 100644 --- a/plugins/api/rate-limit/src/org/apache/cloudstack/ratelimit/StoreEntryImpl.java +++ b/plugins/api/rate-limit/src/org/apache/cloudstack/ratelimit/StoreEntryImpl.java @@ -47,7 +47,7 @@ public class StoreEntryImpl implements StoreEntry { if ( isExpired() ) return 0; // already expired else { - return (expiry - System.currentTimeMillis()) * 1000; + return expiry - System.currentTimeMillis(); } } diff --git a/plugins/api/rate-limit/test/org/apache/cloudstack/ratelimit/ApiRateLimitTest.java b/plugins/api/rate-limit/test/org/apache/cloudstack/ratelimit/ApiRateLimitTest.java index 0e2080ab2f1..ef3cf6d7948 100644 --- a/plugins/api/rate-limit/test/org/apache/cloudstack/ratelimit/ApiRateLimitTest.java +++ b/plugins/api/rate-limit/test/org/apache/cloudstack/ratelimit/ApiRateLimitTest.java @@ -23,8 +23,9 @@ import java.util.concurrent.Executors; import javax.naming.ConfigurationException; +import org.apache.cloudstack.api.ServerApiException; +import org.apache.cloudstack.api.command.admin.ratelimit.ResetApiLimitCmd; import org.apache.cloudstack.api.command.user.ratelimit.GetApiLimitCmd; -import org.apache.cloudstack.api.commands.admin.ratelimit.ResetApiLimitCmd; import org.apache.cloudstack.api.response.ApiLimitResponse; import org.apache.cloudstack.ratelimit.ApiRateLimitServiceImpl; import org.junit.Before; @@ -43,16 +44,10 @@ import static org.mockito.Mockito.*; public class ApiRateLimitTest { static ApiRateLimitServiceImpl _limitService = new ApiRateLimitServiceImpl(); - static ConfigurationDao _configDao = mock(ConfigurationDao.class); private static long acctIdSeq = 0L; @BeforeClass public static void setUp() throws ConfigurationException { - _limitService._configDao = _configDao; - - // No global configuration set, will set in each test case - when(_configDao.getValue(Config.ApiLimitInterval.key())).thenReturn(null); - when(_configDao.getValue(Config.ApiLimitMax.key())).thenReturn(null); _limitService.configure("ApiRateLimitTest", Collections. emptyMap()); } @@ -62,6 +57,16 @@ public class ApiRateLimitTest { return new AccountVO(acctIdSeq++); } + private boolean isUnderLimit(Account key){ + try{ + _limitService.checkLimit(key); + return true; + } + catch (ServerApiException ex){ + return false; + } + } + @Test public void sequentialApiAccess() { int allowedRequests = 1; @@ -69,10 +74,10 @@ public class ApiRateLimitTest { _limitService.setTimeToLive(1); Account key = createFakeAccount(); - assertTrue("Allow for the first request", _limitService.isUnderLimit(key)); + assertTrue("Allow for the first request", isUnderLimit(key)); assertFalse("Second request should be blocked, since we assume that the two api " - + " accesses take less than a second to perform", _limitService.isUnderLimit(key)); + + " accesses take less than a second to perform", isUnderLimit(key)); } @Test @@ -84,11 +89,11 @@ public class ApiRateLimitTest { Account key = createFakeAccount(); for (int i = 0; i < allowedRequests; i++) { - assertTrue("We should allow " + allowedRequests + " requests per second", _limitService.isUnderLimit(key)); + assertTrue("We should allow " + allowedRequests + " requests per second", isUnderLimit(key)); } - assertFalse("We should block >" + allowedRequests + " requests per second", _limitService.isUnderLimit(key)); + assertFalse("We should block >" + allowedRequests + " requests per second", isUnderLimit(key)); } @Test @@ -121,7 +126,7 @@ public class ApiRateLimitTest { try { startGate.await(); - isUsable[j] = _limitService.isUnderLimit(key); + isUsable[j] = isUnderLimit(key); } catch (InterruptedException e) { e.printStackTrace(); @@ -155,12 +160,12 @@ public class ApiRateLimitTest { Account key = this.createFakeAccount(); - assertTrue("The first request should be allowed", _limitService.isUnderLimit(key)); + assertTrue("The first request should be allowed", isUnderLimit(key)); // Allow the token to expire Thread.sleep(1001); - assertTrue("Another request after interval should be allowed as well", _limitService.isUnderLimit(key)); + assertTrue("Another request after interval should be allowed as well", isUnderLimit(key)); } @Test @@ -171,16 +176,16 @@ public class ApiRateLimitTest { Account key = this.createFakeAccount(); - assertTrue("The first request should be allowed", _limitService.isUnderLimit(key)); + assertTrue("The first request should be allowed", isUnderLimit(key)); - assertFalse("Another request should be blocked", _limitService.isUnderLimit(key)); + assertFalse("Another request should be blocked", isUnderLimit(key)); ResetApiLimitCmd cmd = new ResetApiLimitCmd(); cmd.setAccountId(key.getId()); _limitService.resetApiLimit(cmd); - assertTrue("Another request should be allowed after reset counter", _limitService.isUnderLimit(key)); + assertTrue("Another request should be allowed after reset counter", isUnderLimit(key)); } /* Disable this since I cannot mock Static method UserContext.current() @@ -193,7 +198,7 @@ public class ApiRateLimitTest { Account key = this.createFakeAccount(); for ( int i = 0; i < 5; i++ ){ - assertTrue("Issued 5 requests", _limitService.isUnderLimit(key)); + assertTrue("Issued 5 requests", isUnderLimit(key)); } GetApiLimitCmd cmd = new GetApiLimitCmd(); diff --git a/server/src/com/cloud/api/ApiServer.java b/server/src/com/cloud/api/ApiServer.java index b2a6a87d9e5..1d15acf0b6f 100755 --- a/server/src/com/cloud/api/ApiServer.java +++ b/server/src/com/cloud/api/ApiServer.java @@ -556,12 +556,9 @@ public class ApiServer implements HttpRequestHandler { if (userId != null) { User user = ApiDBUtils.findUserById(userId); if (apiThrottlingEnabled){ - // go through each API limit checker - if (!isRequestAllowed(user)) { - //FIXME: more detailed message regarding when he/she can retry - s_logger.warn("The given user has reached his/her account api limit, please retry later"); - throw new ServerApiException(BaseCmd.API_LIMIT_EXCEED, "The given user has reached his/her account api limit"); - } + // go through each API limit checker, throw exception inside adapter implementation so that message + // can contain some detailed information only known for each adapter implementation. + checkRequestLimit(user); } if (!isCommandAvailable(user, commandName)) { s_logger.debug("The given command:" + commandName + " does not exist or it is not available for user with id:" + userId); @@ -803,20 +800,19 @@ public class ApiServer implements HttpRequestHandler { } - private boolean isRequestAllowed(User user) { + private void checkRequestLimit(User user) throws ServerApiException { Account account = ApiDBUtils.findAccountById(user.getAccountId()); if ( _accountMgr.isRootAdmin(account.getType()) ){ // no api throttling for root admin - return true; + return; } for (APILimitChecker apiChecker : _apiLimitCheckers) { // Fail the checking if any checker fails to verify - if (!apiChecker.isUnderLimit(account)) - return false; - } - return true; + apiChecker.checkLimit(account); + } } + private boolean doesCommandExist(String apiName) { for (APIChecker apiChecker : _apiAccessCheckers) { // If any checker has api info on the command, return true diff --git a/server/src/com/cloud/configuration/Config.java b/server/src/com/cloud/configuration/Config.java index ae7651c846e..e6bf3d543de 100755 --- a/server/src/com/cloud/configuration/Config.java +++ b/server/src/com/cloud/configuration/Config.java @@ -361,9 +361,7 @@ public enum Config { null, "Limits number of snapshots that can be handled by the host concurrently; default is NULL - unlimited", null), // API throttling - ApiLimitInterval("Advanced", ManagementServer.class, Long.class, "api.throttling.interval", "1", "The default time interval in seconds used to set account based api limit", null), - ApiLimitMax("Advanced", ManagementServer.class, Long.class, "api.throttling.max", "25", "The max number of API requests within api.throttling.interval duration", null), - ApiLimitEnabled("Advanced", ManagementServer.class, Boolean.class, "api.throttling.enabled", "true", "If true, api throttline feature is enabled", "true,false"); + ApiLimitEnabled("Advanced", ManagementServer.class, Boolean.class, "api.throttling.enable", "true", "If true, api throttline feature is enabled", "true,false"); private final String _category; private final Class _componentClass; diff --git a/server/test/com/cloud/api/ListPerfTest.java b/server/test/com/cloud/api/ListPerfTest.java index eb98d9187fe..e5d277a314f 100644 --- a/server/test/com/cloud/api/ListPerfTest.java +++ b/server/test/com/cloud/api/ListPerfTest.java @@ -17,6 +17,9 @@ package com.cloud.api; import java.util.HashMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.ExecutorService; +import java.util.concurrent.Executors; import org.junit.Before; import org.junit.Test; @@ -163,6 +166,57 @@ public class ListPerfTest extends APITest { } + @Test + public void testMultiListAccounts() throws Exception { + // log in using normal user + login("demo", "password"); + // issue list Accounts calls + final HashMap params = new HashMap(); + params.put("response", "json"); + params.put("listAll", "true"); + params.put("sessionkey", sessionKey); + int clientCount = 6; + Runnable[] clients = new Runnable[clientCount]; + final boolean[] isUsable = new boolean[clientCount]; + final CountDownLatch startGate = new CountDownLatch(1); + + final CountDownLatch endGate = new CountDownLatch(clientCount); + + + for (int i = 0; i < isUsable.length; ++i) { + final int j = i; + clients[j] = new Runnable() { + + /** + * {@inheritDoc} + */ + @Override + public void run() { + try { + startGate.await(); + + System.out.println(sendRequest("listAccounts", params)); + + } catch (InterruptedException e) { + e.printStackTrace(); + } finally { + endGate.countDown(); + } + } + }; + } + + ExecutorService executor = Executors.newFixedThreadPool(clientCount); + + for (Runnable runnable : clients) { + executor.execute(runnable); + } + + startGate.countDown(); + + endGate.await(); + + } } diff --git a/setup/db/db/schema-40to410.sql b/setup/db/db/schema-40to410.sql index bf3fb303e5b..a6b102a057c 100644 --- a/setup/db/db/schema-40to410.sql +++ b/setup/db/db/schema-40to410.sql @@ -142,6 +142,8 @@ UPDATE `cloud`.`conditions` set uuid=id WHERE uuid is NULL; INSERT IGNORE INTO `cloud`.`configuration` VALUES ('Advanced', 'DEFAULT', 'management-server', '"detail.batch.query.size"', '2000', 'Default entity detail batch query size for listing'); +INSERT IGNORE INTO `cloud`.`configuration` VALUES ('Advanced', 'DEFAULT', 'management-server', 'api.throttling.enabled', 'true, 'enable api rate limiting'); + --- DB views for list api --- use cloud;