Ability to limit concurrent rest api calls Even though we have rate limiting over a time window, costly restapis run concurrently by users can still bring down the server. Add a way to limit concurrent requests per user. Only applies to restapis. Change-Id: I0a89cf31199ae9c610be8b32e47d7b122826b605
diff --git a/src/main/java/com/googlesource/gerrit/plugins/quota/AccountLimitsFinder.java b/src/main/java/com/googlesource/gerrit/plugins/quota/AccountLimitsFinder.java index 53651eb..2c24f1d 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/quota/AccountLimitsFinder.java +++ b/src/main/java/com/googlesource/gerrit/plugins/quota/AccountLimitsFinder.java
@@ -23,7 +23,6 @@ import com.google.gerrit.extensions.restapi.TopLevelResource; import com.google.gerrit.server.IdentifiedUser; import com.google.gerrit.server.account.GroupMembership; -import com.google.gerrit.server.group.GroupResource; import com.google.gerrit.server.project.ProjectCache; import com.google.gerrit.server.restapi.group.GroupsCollection; import com.google.inject.Inject; @@ -57,28 +56,35 @@ if (limits.isPresent()) { GroupMembership memberShip = user.getEffectiveGroups(); for (String groupName : limits.get().keySet()) { - try { - GroupResource group = - groupsCollection.parse(TopLevelResource.INSTANCE, IdString.fromDecoded(groupName)); - Optional<GroupDescription.Internal> maybeInternalGroup = group.asInternalGroup(); - if (!maybeInternalGroup.isPresent()) { - log.debug("Ignoring limits for non-internal group ''{}'' in quota.config", groupName); - } else if (memberShip.contains(maybeInternalGroup.get().getGroupUUID())) { - RateLimit groupLimit = limits.get().get(groupName); - if (groupLimit != null) { - return Optional.of(groupLimit); - } - } - } catch (ResourceNotFoundException e) { - log.debug("Ignoring limits for unknown group ''{}'' in quota.config", groupName); - } catch (AuthException e) { - log.debug("Ignoring limits for non-visible group ''{}'' in quota.config", groupName); + RateLimit groupLimit = limits.get().get(groupName); + if (groupLimit != null && isMatching(memberShip, groupName)) { + return Optional.of(groupLimit); } } } return Optional.empty(); } + public boolean isMatching(GroupMembership membership, String groupName) { + try { + Optional<GroupDescription.Internal> maybeInternalGroup = + groupsCollection + .parse(TopLevelResource.INSTANCE, IdString.fromDecoded(groupName)) + .asInternalGroup(); + if (!maybeInternalGroup.isPresent()) { + log.debug("Ignoring limits for non-internal group ''{}'' in quota.config", groupName); + } else if (membership.contains(maybeInternalGroup.get().getGroupUUID())) { + return true; + } + } catch (ResourceNotFoundException e) { + log.debug("Ignoring limits for unknown group ''{}'' in quota.config", groupName); + } catch (AuthException e) { + log.debug("Ignoring limits for non-visible group ''{}'' in quota.config", groupName); + } + + return false; + } + /** * @param type type of rate limit * @return global rate limit
diff --git a/src/main/java/com/googlesource/gerrit/plugins/quota/HttpModule.java b/src/main/java/com/googlesource/gerrit/plugins/quota/HttpModule.java index 28c0a60..8f19b37 100644 --- a/src/main/java/com/googlesource/gerrit/plugins/quota/HttpModule.java +++ b/src/main/java/com/googlesource/gerrit/plugins/quota/HttpModule.java
@@ -51,6 +51,7 @@ @Override protected void configure() { DynamicSet.bind(binder(), AllRequestFilter.class).to(RestApiRateLimiter.class); + DynamicSet.bind(binder(), AllRequestFilter.class).to(MaxConnectionsLimiter.class); bindConstant() .annotatedWith(Names.named(RateMsgHelper.RESTAPI_CONFIGURABLE_MSG_ANNOTATION)) .to(restapiLimitExceededMsg);
diff --git a/src/main/java/com/googlesource/gerrit/plugins/quota/MaxConnectionsLimiter.java b/src/main/java/com/googlesource/gerrit/plugins/quota/MaxConnectionsLimiter.java new file mode 100644 index 0000000..f2b18cd --- /dev/null +++ b/src/main/java/com/googlesource/gerrit/plugins/quota/MaxConnectionsLimiter.java
@@ -0,0 +1,145 @@ +package com.googlesource.gerrit.plugins.quota; + +import com.google.gerrit.httpd.AllRequestFilter; +import com.google.gerrit.server.CurrentUser; +import com.google.gerrit.server.IdentifiedUser; +import com.google.gerrit.server.project.ProjectCache; +import com.google.inject.Inject; +import com.google.inject.Provider; +import com.google.inject.Singleton; +import org.eclipse.jgit.lib.Config; + +import java.io.IOException; +import java.util.*; +import java.util.concurrent.ConcurrentHashMap; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.regex.Matcher; +import java.util.regex.Pattern; +import javax.servlet.FilterChain; +import javax.servlet.ServletException; +import javax.servlet.ServletRequest; +import javax.servlet.ServletResponse; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +@Singleton +public class MaxConnectionsLimiter extends AllRequestFilter { + record Limit(String group, Integer restApiLimit) {} + + private static final String GLOBAL_KEY = "global"; + private static final String CONFIG_KEY = "maxConnectionsPerUserForTask"; + private static final String CONFIG_REST_API = "rest-api"; + private static final Pattern REST_API_REGEX = Pattern.compile("(\\d+)\\s+" + CONFIG_REST_API); + + private static final Pattern REST_API_PATH_PATTERN = + Pattern.compile( + "^/(?:a/)?(access|accounts|changes|config|groups|plugins|projects|tools)/.*$"); + + private final Map<String, Integer> connectionsByUser = new ConcurrentHashMap<>(); + private final Provider<CurrentUser> userProvider; + private final AccountLimitsFinder accountLimitsFinder; + private final List<Limit> limits = new ArrayList<>(2); + private Limit globalLimit; + + @Inject + public MaxConnectionsLimiter( + AccountLimitsFinder accountLimitsFinder, Provider<CurrentUser> userProvider) { + this.accountLimitsFinder = accountLimitsFinder; + this.userProvider = userProvider; + } + + @Inject + void init(ProjectCache projectCache) { + Config cfg = projectCache.getAllProjects().getConfig("quota.config").get(); + for (String group : cfg.getSubsections(AccountLimitsConfig.GROUP_SECTION)) { + String val = cfg.getString(AccountLimitsConfig.GROUP_SECTION, group, CONFIG_KEY); + Matcher matcher = REST_API_REGEX.matcher(val); + if (matcher.matches()) { + limits.add(new Limit(group, Integer.parseInt(matcher.group(1)))); + } + } + + String globalConfig = cfg.getString(GLOBAL_KEY, null, CONFIG_KEY); + if (globalConfig != null) { + Matcher matcher = REST_API_REGEX.matcher(globalConfig); + if (matcher.matches()) { + globalLimit = new Limit(GLOBAL_KEY, Integer.parseInt(matcher.group(1))); + } + } + } + + @Override + public void doFilter(ServletRequest request, ServletResponse response, FilterChain chain) + throws IOException, ServletException { + if (isRestApiRequest(request)) { + CurrentUser currentUser = userProvider.get(); + + if (currentUser.isIdentifiedUser()) { + String userId = currentUser.getAccountId().toString(); + Optional<Integer> limit = getEffectiveRestApiLimit(currentUser.asIdentifiedUser()); + + if (limit.isPresent()) { + if (!canPermitCall(userId, limit.get())) { + ((HttpServletResponse) response) + .sendError(429, "Too Many Requests: rate limited by " + CONFIG_KEY); + return; + } + try { + chain.doFilter(request, response); + } finally { + markCallComplete(userId); + } + return; + } + } + } + + chain.doFilter(request, response); + } + + private boolean isRestApiRequest(ServletRequest req) { + return req instanceof HttpServletRequest + && REST_API_PATH_PATTERN.matcher(((HttpServletRequest) req).getServletPath()).matches(); + } + + private Optional<Integer> getEffectiveRestApiLimit(IdentifiedUser user) { + List<Integer> result = new ArrayList<>(); + + for (Limit limit : limits) { + if (accountLimitsFinder.isMatching(user.getEffectiveGroups(), limit.group())) { + result.add(limit.restApiLimit()); + break; // only consider the first matching group + } + } + + if (globalLimit != null) { + result.add(globalLimit.restApiLimit()); + } + + return result.stream().filter(l -> l > 0).min(Integer::compareTo); + } + + private boolean canPermitCall(String userId, int limit) { + AtomicBoolean permitted = new AtomicBoolean(false); + connectionsByUser.compute( + userId, + (u, c) -> { + int current = (c == null) ? 0 : c; + if (current < limit) { + permitted.setPlain(true); + return current + 1; + } + return current; + }); + return permitted.getPlain(); + } + + private void markCallComplete(String userId) { + connectionsByUser.computeIfPresent( + userId, + (u, c) -> { + c--; + return c <= 0 ? null : c; + }); + } +}
diff --git a/src/main/resources/Documentation/config.md b/src/main/resources/Documentation/config.md index 76a0491..6ae88af 100644 --- a/src/main/resources/Documentation/config.md +++ b/src/main/resources/Documentation/config.md
@@ -255,6 +255,19 @@ `Exceeded rate limit of ${rateLimit} REST API requests/hour (or idle ` `time used up in bursts of max ${burstsLimit} requests)` . +<a id="maxConnectionsPerUserForTask" /> +`maxConnectionsPerUserForTask` +: Even though we have ratelimiting over a window of period, costly restapis +run concurrently by users can still bring down the server. Using the +below config we could limit the concurrent calls. This helps in limiting the +costly calls that can occupy threads for long periods and prevent other shorter +operations by other users from being processed. + +``` + [group "Registered Users"] + maxConnectionsPerUserForTask = 20 rest-api +``` + Task Quota -----------