SOLR-18046 RateLimitFilter, rename EssentialSolrRequestFilter

changelog/unreleased/SOLR-18046.yml

@@ -0,0 +1,8 @@
+# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
+title: Ratelimiting now exists in it's own Servlet filter which can be customized or removed.
+type: other # added, changed, fixed, deprecated, removed, dependency_update, security, other
+authors:
+  - name: Gus Heck
+links:
+  - name: SOLR-18046
+    url: https://issues.apache.org/jira/browse/SOLR-18046

solr/core/src/java/org/apache/solr/core/RateLimiterConfig.java

@@ -17,14 +17,14 @@
 
 package org.apache.solr.core;
 
-import static org.apache.solr.servlet.RateLimitManager.DEFAULT_CONCURRENT_REQUESTS;
-import static org.apache.solr.servlet.RateLimitManager.DEFAULT_SLOT_ACQUISITION_TIMEOUT_MS;
-
 import org.apache.solr.client.solrj.SolrRequest;
 import org.apache.solr.client.solrj.request.beans.RateLimiterPayload;
 
 public class RateLimiterConfig {
   public static final String RL_CONFIG_KEY = "rate-limiters";
+  public static final int DEFAULT_CONCURRENT_REQUESTS =
+      (Runtime.getRuntime().availableProcessors()) * 3;
+  public static final long DEFAULT_SLOT_ACQUISITION_TIMEOUT_MS = -1;
 
   public final SolrRequest.SolrRequestType requestType;
   public final boolean isEnabled;

solr/core/src/java/org/apache/solr/servlet/CoreContainerAwareHttpFilter.java

@@ -33,7 +33,6 @@ public abstract class CoreContainerAwareHttpFilter extends HttpFilter {
 
   @Override
   public void init(FilterConfig config) throws ServletException {
-    super.init(config);
     containerProvider = CoreContainerProvider.serviceForContext(config.getServletContext());
     if (log.isTraceEnabled()) {
       log.trace("{}.init(): {}", this.getClass().getName(), this.getClass().getClassLoader());
@@ -47,13 +46,4 @@ public void init(FilterConfig config) throws ServletException {
   public CoreContainer getCores() throws UnavailableException {
     return containerProvider.getCoreContainer();
   }
-
-  // TODO: not fond of having these here, but RateLimiter initialization can be sorted out later
-  RateLimitManager getRateLimitManager() {
-    return containerProvider.getRateLimitManager();
-  }
-
-  void replaceRateLimitManager(RateLimitManager rlm) {
-    containerProvider.setRateLimitManager(rlm);
-  }
 }

solr/core/src/java/org/apache/solr/servlet/CoreContainerProvider.java

@@ -23,7 +23,6 @@
 import static org.apache.solr.servlet.SolrDispatchFilter.SOLR_LOG_LEVEL;
 import static org.apache.solr.servlet.SolrDispatchFilter.SOLR_LOG_MUTECONSOLE;
 
-import com.google.common.annotations.VisibleForTesting;
 import jakarta.servlet.ServletContext;
 import jakarta.servlet.ServletContextEvent;
 import jakarta.servlet.ServletContextListener;
@@ -45,16 +44,13 @@
 import org.apache.lucene.store.MMapDirectory;
 import org.apache.lucene.util.VectorUtil;
 import org.apache.solr.client.api.util.SolrVersion;
-import org.apache.solr.cloud.ZkController;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.SolrException.ErrorCode;
-import org.apache.solr.common.cloud.SolrZkClient;
 import org.apache.solr.common.util.EnvUtils;
 import org.apache.solr.core.CoreContainer;
 import org.apache.solr.core.NodeConfig;
 import org.apache.solr.core.SolrCore;
 import org.apache.solr.core.SolrXmlConfig;
-import org.apache.solr.servlet.RateLimitManager.Builder;
 import org.apache.solr.util.StartupLoggingUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -67,8 +63,6 @@
 public class CoreContainerProvider implements ServletContextListener {
   private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
   private CoreContainer cores;
-  // TODO: this probably should not live here...
-  private RateLimitManager rateLimitManager;
 
   /**
    * Acquires an instance from the context. Never null.
@@ -187,21 +181,6 @@ private void init(ServletContext servletContext) {
 
       coresInit = createCoreContainer(computeSolrHome(servletContext), extraProperties);
 
-      SolrZkClient zkClient = null;
-      ZkController zkController = coresInit.getZkController();
-
-      if (zkController != null) {
-        zkClient = zkController.getZkClient();
-      }
-
-      Builder builder = new Builder(zkClient);
-
-      this.rateLimitManager = builder.build();
-
-      if (zkController != null) {
-        zkController.zkStateReader.registerClusterPropertiesListener(this.rateLimitManager);
-      }
-
       if (log.isDebugEnabled()) {
         log.debug("user.dir={}", System.getProperty("user.dir"));
       }
@@ -364,13 +343,4 @@ protected CoreContainer createCoreContainer(Path solrHome, Properties nodeProps)
     coreContainer.load();
     return coreContainer;
   }
-
-  public RateLimitManager getRateLimitManager() {
-    return rateLimitManager;
-  }
-
-  @VisibleForTesting
-  void setRateLimitManager(RateLimitManager rateLimitManager) {
-    this.rateLimitManager = rateLimitManager;
-  }
 }

solr/core/src/java/org/apache/solr/servlet/LoadAdminUiServlet.java

@@ -16,7 +16,7 @@
  */
 package org.apache.solr.servlet;
 
-import static org.apache.solr.servlet.EssentialSolrRequestFilter.CORE_CONTAINER_REQUEST_ATTRIBUTE;
+import static org.apache.solr.servlet.RequiredSolrRequestFilter.CORE_CONTAINER_REQUEST_ATTRIBUTE;
 
 import com.google.common.net.HttpHeaders;
 import jakarta.servlet.http.HttpServlet;

solr/core/src/java/org/apache/solr/servlet/RateLimitFilter.java

@@ -0,0 +1,76 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements.  See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.solr.servlet;
+
+import com.google.common.annotations.VisibleForTesting;
+import jakarta.servlet.FilterChain;
+import jakarta.servlet.FilterConfig;
+import jakarta.servlet.ServletException;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+import java.io.IOException;
+import org.apache.solr.common.SolrException;
+
+public class RateLimitFilter extends CoreContainerAwareHttpFilter {
+  private RateLimitManager rateLimitManager;
+
+  @Override
+  public void init(FilterConfig config) throws ServletException {
+    super.init(config);
+    RateLimitManager.Builder builder = new RateLimitManager.Builder();
+    builder.withZk(getCores().getZkController());
+    this.rateLimitManager = builder.build();
+  }
+
+  @Override
+  protected void doFilter(HttpServletRequest req, HttpServletResponse res, FilterChain chain)
+      throws IOException, ServletException {
+    RateLimitManager rateLimitManager = getRateLimitManager();
+    try (RequestRateLimiter.SlotReservation accepted = rateLimitManager.handleRequest(req)) {
+      if (accepted == null) {
+        res.sendError(
+            SolrException.ErrorCode.TOO_MANY_REQUESTS.code, RateLimitManager.ERROR_MESSAGE);
+        return;
+      }
+      // todo: this shouldn't be required, tracing and rate limiting should be independently
+      //  composable
+      ServletUtils.traceHttpRequestExecution2(
+          req,
+          res,
+          () -> {
+            try {
+              chain.doFilter(req, res);
+            } catch (Exception e) {
+              throw new ExceptionWhileTracing(e);
+            }
+          });
+    } catch (InterruptedException e1) {
+      Thread.currentThread().interrupt();
+      throw new SolrException(SolrException.ErrorCode.SERVER_ERROR, e1.getMessage());
+    }
+  }
+
+  public RateLimitManager getRateLimitManager() {
+    return rateLimitManager;
+  }
+
+  @VisibleForTesting
+  void setRateLimitManager(RateLimitManager rateLimitManager) {
+    this.rateLimitManager = rateLimitManager;
+  }
+}

solr/core/src/java/org/apache/solr/servlet/RateLimitManager.java

@@ -28,8 +28,8 @@
 import java.util.concurrent.ConcurrentHashMap;
 import net.jcip.annotations.ThreadSafe;
 import org.apache.solr.client.solrj.SolrRequest;
+import org.apache.solr.cloud.ZkController;
 import org.apache.solr.common.cloud.ClusterPropertiesListener;
-import org.apache.solr.common.cloud.SolrZkClient;
 import org.apache.solr.core.RateLimiterConfig;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
@@ -49,9 +49,6 @@ public class RateLimitManager implements ClusterPropertiesListener {
 
   public static final String ERROR_MESSAGE =
       "Too many requests for this request type. Please try after some time or increase the quota for this request type";
-  public static final int DEFAULT_CONCURRENT_REQUESTS =
-      (Runtime.getRuntime().availableProcessors()) * 3;
-  public static final long DEFAULT_SLOT_ACQUISITION_TIMEOUT_MS = -1;
   private final ConcurrentHashMap<String, RequestRateLimiter> requestRateLimiterMap;
 
   public RateLimitManager() {
@@ -175,18 +172,24 @@ public RequestRateLimiter getRequestRateLimiter(SolrRequest.SolrRequestType requ
   }
 
   public static class Builder {
-    protected SolrZkClient solrZkClient;
+    protected ZkController solrZk;
 
-    public Builder(SolrZkClient solrZkClient) {
-      this.solrZkClient = solrZkClient;
+    public Builder() {}
+
+    @SuppressWarnings("UnusedReturnValue")
+    public Builder withZk(ZkController zkClient) {
+      solrZk = zkClient;
+      return this;
     }
 
     public RateLimitManager build() {
       RateLimitManager rateLimitManager = new RateLimitManager();
-
       rateLimitManager.registerRequestRateLimiter(
-          new QueryRateLimiter(solrZkClient), SolrRequest.SolrRequestType.QUERY);
-
+          new QueryRateLimiter(solrZk == null ? null : solrZk.getZkClient()),
+          SolrRequest.SolrRequestType.QUERY);
+      if (solrZk != null) {
+        solrZk.zkStateReader.registerClusterPropertiesListener(rateLimitManager);
+      }
       return rateLimitManager;
     }
   }

solr/core/src/java/org/apache/solr/servlet/EssentialSolrRequestFilter.java → solr/core/src/java/org/apache/solr/servlet/RequiredSolrRequestFilter.java

@@ -35,10 +35,10 @@
  * It is expected that solr will fail to function as intended without the conditions initialized in
  * this filter. Before adding anything to this filter ask yourself if a user could run without the
  * feature you are adding (either with an alternative implementation or without it at all to reduce
- * cpu/memory/dependencies). If it is not essential, it should have its own separate filter. Also,
- * please include a comment indicating why the thing added is essential.
+ * cpu/memory/dependencies). If it is not required, it should have its own separate filter. Also,
+ * please include a comment indicating why the thing added here is required.
  */
-public class EssentialSolrRequestFilter extends CoreContainerAwareHttpFilter {
+public class RequiredSolrRequestFilter extends CoreContainerAwareHttpFilter {
 
   // Best to put constant here because solr is not supposed to be functional (or compile)
   // without this filter.
@@ -55,41 +55,38 @@ protected void doFilter(HttpServletRequest req, HttpServletResponse res, FilterC
     // this autocloseable is here to invoke MDCSnapshot.close() which restores captured state
     try (var mdcSnapshot = MDCSnapshot.create()) {
 
-      // MDC logging *shouldn't* be essential but currently is, see SOLR-18050.
-      // Our use of SLF4J indicates that we intend logging to be pluggable, and
-      // some implementations won't have an MDC, so having it as essential limits
-      // logging implementations.
+      // MDC logging *shouldn't* be required but currently is, see SOLR-18050.
       log.trace("MDC snapshot recorded {}", mdcSnapshot); // avoid both compiler and ide warning.
       MDCLoggingContext.reset();
       MDCLoggingContext.setNode(getCores());
 
-      // This is essential to accommodate libraries that (annoyingly) use
+      // This is required to accommodate libraries that (annoyingly) use
       // Thread.currentThread().getContextClassLoader()
       Thread.currentThread().setContextClassLoader(getCores().getResourceLoader().getClassLoader());
 
       // set a request timer which can be reused by requests if needed
-      // Request Timer is essential for QueryLimits functionality as well as
+      // Request Timer is required for QueryLimits functionality as well as
       // timing our requests.
       req.setAttribute(SolrRequestParsers.REQUEST_TIMER_SERVLET_ATTRIBUTE, new RTimerTree());
 
       // put the core container in request attribute
-      // This is essential for the LoadAdminUiServlet class. Removing it will cause 404
+      // This is required for the LoadAdminUiServlet class. Removing it will cause 404
       req.setAttribute(CORE_CONTAINER_REQUEST_ATTRIBUTE, getCores());
       chain.doFilter(req, res);
     } finally {
       // cleanups for above stuff
       MDCLoggingContext.reset();
       Thread.currentThread().setContextClassLoader(contextClassLoader);
 
-      // This is an essential safety valve to ensure we don't accidentally bleed information
+      // This is a required safety valve to ensure we don't accidentally bleed information
       // between requests.
       SolrRequestInfo.reset();
       if (!req.isAsyncStarted()) { // jetty's proxy uses this
 
-        // essential to avoid SOLR-8453 and SOLR-8683
+        // required to avoid SOLR-8453 and SOLR-8683
         ServletUtils.consumeInputFully(req, res);
 
-        // essential to remove temporary files created during multipart requests.
+        // required to remove temporary files created during multipart requests.
         SolrRequestParsers.cleanupMultipartFiles(req);
       }
     }

solr/core/src/java/org/apache/solr/servlet/ServletUtils.java

@@ -32,8 +32,6 @@
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.lang.invoke.MethodHandles;
-import org.apache.solr.common.SolrException;
-import org.apache.solr.common.SolrException.ErrorCode;
 import org.apache.solr.common.util.Utils;
 import org.apache.solr.logging.MDCLoggingContext;
 import org.apache.solr.util.tracing.TraceUtils;
@@ -126,34 +124,6 @@ public void close() {
     };
   }
 
-  /**
-   * Enforces rate limiting for a request. Should be converted to a servlet filter at some point.
-   * Currently, this is tightly coupled with request tracing which is not ideal either.
-   *
-   * @param request The request to limit
-   * @param response The associated response
-   * @param limitedExecution code that will be traced
-   */
-  static void rateLimitRequest(
-      RateLimitManager rateLimitManager,
-      HttpServletRequest request,
-      HttpServletResponse response,
-      Runnable limitedExecution)
-      throws ServletException, IOException {
-    try (RequestRateLimiter.SlotReservation accepted = rateLimitManager.handleRequest(request)) {
-      if (accepted == null) {
-        response.sendError(ErrorCode.TOO_MANY_REQUESTS.code, RateLimitManager.ERROR_MESSAGE);
-        return;
-      }
-      // todo: this shouldn't be required, tracing and rate limiting should be independently
-      // composable
-      traceHttpRequestExecution2(request, response, limitedExecution);
-    } catch (InterruptedException e) {
-      Thread.currentThread().interrupt();
-      throw new SolrException(ErrorCode.SERVER_ERROR, e.getMessage());
-    }
-  }
-
   /**
    * Sets up tracing for an HTTP request. Perhaps should be converted to a servlet filter at some
    * point.
@@ -162,7 +132,7 @@ static void rateLimitRequest(
    * @param response The associated response
    * @param tracedExecution the executed code
    */
-  private static void traceHttpRequestExecution2(
+  static void traceHttpRequestExecution2(
       HttpServletRequest request, HttpServletResponse response, Runnable tracedExecution)
       throws ServletException, IOException {
     Context context = TraceUtils.extractContext(request);
@@ -181,10 +151,6 @@ private static void traceHttpRequestExecution2(
       }
       tracedExecution.run();
     } catch (ExceptionWhileTracing e) {
-      if (e.e instanceof SolrAuthenticationException) {
-        // done, the response and status code have already been sent
-        return;
-      }
       if (e.e instanceof ServletException) {
         throw (ServletException) e.e;
       }