SOLR-18044 EssentialSolrRequestFilter to perform non-optional request wrapping operations

changelog/unreleased/SOLR-18044.yml

@@ -0,0 +1,8 @@
+# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
+title: EssentialSolrRequestFilter has been added to perform non-optional request wrapping operations required for proper functioning of Solr
+type: other # added, changed, fixed, deprecated, removed, dependency_update, security, other
+authors:
+  - name: Gus Heck
+links:
+  - name: SOLR-18044
+    url: https://issues.apache.org/jira/browse/SOLR-18044

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

@@ -25,11 +25,20 @@
 import org.apache.solr.common.util.SuppressForbidden;
 import org.apache.solr.logging.MDCLoggingContext;
 import org.apache.solr.logging.MDCSnapshot;
+import org.apache.solr.request.SolrRequestInfo;
+import org.apache.solr.util.RTimerTree;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
-/** Servlet Filter to set up and tear down MDC Logging context for each reqeust. */
-public class MdcLoggingFilter extends CoreContainerAwareHttpFilter {
+/**
+ * Servlet Filter to set up and tear down a various things that downstream code in solr relies on.
+ * 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.
+ */
+public class EssentialSolrRequestFilter extends CoreContainerAwareHttpFilter {
 
   private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
 
@@ -42,16 +51,44 @@ protected void doFilter(HttpServletRequest req, HttpServletResponse res, FilterC
     ClassLoader contextClassLoader = Thread.currentThread().getContextClassLoader();
     // 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.
       log.trace("MDC snapshot recorded {}", mdcSnapshot); // avoid both compiler and ide warning.
       MDCLoggingContext.reset();
       MDCLoggingContext.setNode(getCores());
-      // This doesn't belong here, but for the moment it is here to preserve it's relative
-      // timing of execution for now. Probably I will break this out in a subsequent change
+
+      // This is essential 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
+      // 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
+      req.setAttribute("org.apache.solr.CoreContainer", 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
+      // between requests.
+      SolrRequestInfo.reset();
+      if (!req.isAsyncStarted()) { // jetty's proxy uses this
+
+        // essential to avoid SOLR-8453 and SOLR-8683
+        ServletUtils.consumeInputFully(req, res);
+
+        // essential to remove temporary files created during multipart requests.
+        SolrRequestParsers.cleanupMultipartFiles(req);
+      }
     }
   }
 }

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

@@ -99,7 +99,6 @@
 import org.apache.solr.servlet.cache.HttpCacheHeaderUtil;
 import org.apache.solr.servlet.cache.Method;
 import org.apache.solr.update.processor.DistributingUpdateProcessorFactory;
-import org.apache.solr.util.RTimerTree;
 import org.apache.solr.util.tracing.TraceUtils;
 import org.apache.zookeeper.KeeperException;
 import org.eclipse.jetty.client.HttpClient;
@@ -157,10 +156,6 @@ public HttpSolrCall(
     this.path = ServletUtils.getPathAfterContext(req);
 
     req.setAttribute(HttpSolrCall.class.getName(), this);
-    // set a request timer which can be reused by requests if needed
-    req.setAttribute(SolrRequestParsers.REQUEST_TIMER_SERVLET_ATTRIBUTE, new RTimerTree());
-    // put the core container in request attribute
-    req.setAttribute("org.apache.solr.CoreContainer", cores);
   }
 
   public String getPath() {

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

@@ -44,6 +44,7 @@ public final class LoadAdminUiServlet extends HttpServlet {
       Boolean.parseBoolean(System.getProperty("solr.ui.enabled", "true"));
   // list of comma separated URLs to inject into the CSP connect-src directive
   public static final String SYSPROP_CSP_CONNECT_SRC_URLS = "solr.ui.headers.csp.connect-src.urls";
+  public static final String CORE_CONTAINER_REQUEST_ATTRIBUTE = "org.apache.solr.CoreContainer";
 
   @Override
   public void doGet(HttpServletRequest request, HttpServletResponse response) throws IOException {
@@ -62,7 +63,7 @@ public void doGet(HttpServletRequest request, HttpServletResponse response) thro
 
     // This attribute is set by the SolrDispatchFilter
     String admin = request.getRequestURI().substring(request.getContextPath().length());
-    CoreContainer cores = (CoreContainer) request.getAttribute("org.apache.solr.CoreContainer");
+    CoreContainer cores = (CoreContainer) request.getAttribute(CORE_CONTAINER_REQUEST_ATTRIBUTE);
     try (InputStream in = getServletContext().getResourceAsStream(admin)) {
       if (in != null && cores != null) {
         response.setCharacterEncoding("UTF-8");

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

@@ -37,7 +37,6 @@
 import org.apache.solr.core.CoreContainer;
 import org.apache.solr.core.NodeRoles;
 import org.apache.solr.handler.api.V2ApiUtils;
-import org.apache.solr.request.SolrRequestInfo;
 import org.apache.solr.security.AuditEvent;
 import org.apache.solr.security.AuditEvent.EventType;
 import org.apache.solr.security.AuthenticationPlugin;
@@ -146,25 +145,17 @@ private void doFilterRetry(
       throws IOException, ServletException {
     setTracer(request, getCores().getTracer());
     RateLimitManager rateLimitManager = getRateLimitManager();
-    try {
-      ServletUtils.rateLimitRequest(
-          rateLimitManager,
-          request,
-          response,
-          () -> {
-            try {
-              dispatch(chain, request, response, retry);
-            } catch (IOException | ServletException | SolrAuthenticationException e) {
-              throw new ExceptionWhileTracing(e);
-            }
-          });
-    } finally {
-      SolrRequestInfo.reset();
-      if (!request.isAsyncStarted()) { // jetty's proxy uses this
-        ServletUtils.consumeInputFully(request, response);
-        SolrRequestParsers.cleanupMultipartFiles(request);
-      }
-    }
+    ServletUtils.rateLimitRequest(
+        rateLimitManager,
+        request,
+        response,
+        () -> {
+          try {
+            dispatch(chain, request, response, retry);
+          } catch (IOException | ServletException | SolrAuthenticationException e) {
+            throw new ExceptionWhileTracing(e);
+          }
+        });
   }
 
   private void dispatch(

solr/test-framework/src/java/org/apache/solr/embedded/JettySolrRunner.java

@@ -61,7 +61,7 @@
 import org.apache.solr.core.CoreContainer;
 import org.apache.solr.metrics.SolrMetricManager;
 import org.apache.solr.servlet.CoreContainerProvider;
-import org.apache.solr.servlet.MdcLoggingFilter;
+import org.apache.solr.servlet.EssentialSolrRequestFilter;
 import org.apache.solr.servlet.PathExclusionFilter;
 import org.apache.solr.servlet.SolrDispatchFilter;
 import org.apache.solr.util.SocketProxy;
@@ -113,7 +113,7 @@ public class JettySolrRunner {
 
   volatile FilterHolder debugFilter;
   volatile FilterHolder pathExcludeFilter;
-  volatile FilterHolder mdcLoggingFilter;
+  volatile FilterHolder essentialFilter;
   volatile FilterHolder dispatchFilter;
 
   private int jettyPort = -1;
@@ -412,16 +412,16 @@ public void contextInitialized(ServletContextEvent event) {
       pathExcludeFilter.setInitParameter("excludePatterns", excludePatterns);
 
       // logging context setup
-      mdcLoggingFilter = root.getServletHandler().newFilterHolder(Source.EMBEDDED);
-      mdcLoggingFilter.setHeldClass(MdcLoggingFilter.class);
+      essentialFilter = root.getServletHandler().newFilterHolder(Source.EMBEDDED);
+      essentialFilter.setHeldClass(EssentialSolrRequestFilter.class);
 
       // This is our main workhorse
       dispatchFilter = root.getServletHandler().newFilterHolder(Source.EMBEDDED);
       dispatchFilter.setHeldClass(SolrDispatchFilter.class);
 
       // Map dispatchFilter in same path as in web.xml
       root.addFilter(pathExcludeFilter, "/*", EnumSet.of(DispatcherType.REQUEST));
-      root.addFilter(mdcLoggingFilter, "/*", EnumSet.of(DispatcherType.REQUEST));
+      root.addFilter(essentialFilter, "/*", EnumSet.of(DispatcherType.REQUEST));
       root.addFilter(dispatchFilter, "/*", EnumSet.of(DispatcherType.REQUEST));
 
       // Default servlet as a fall-through

solr/webapp/web/WEB-INF/web.xml

@@ -46,7 +46,7 @@
 
   <filter>
     <filter-name>MDCLoggingFilter</filter-name>
-    <filter-class>org.apache.solr.servlet.MdcLoggingFilter</filter-class>
+    <filter-class>org.apache.solr.servlet.EssentialSolrRequestFilter</filter-class>
   </filter>
 
   <filter-mapping>