SOLR-18041 PathExclusionFilter extracted from SolrDispatchFilter (#3981)

Collects and simplify all path exclusion logic into a single class configured in web.xml and makes our reliance on jetty's DefaultServlet (or whatever is configured with the name 'default') for serving static content explicit rather than an implicit feature one has to figure out (or already know about).

Author: gus-asf

changelog/unreleased/SOLR-18041.yml

@@ -0,0 +1,8 @@
+# See https://github.com/apache/solr/blob/main/dev-docs/changelog.adoc
+title: SOLR 18041 - Path exclusions for the admin UI are now defined in a separate servlet filter
+type: other # added, changed, fixed, deprecated, removed, dependency_update, security, other
+authors:
+  - name: Gus Heck
+links:
+  - name: SOLR-18041
+    url: https://issues.apache.org/jira/browse/SOLR-18041

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

@@ -0,0 +1,80 @@
+/*
+ * 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 jakarta.servlet.FilterChain;
+import jakarta.servlet.FilterConfig;
+import jakarta.servlet.RequestDispatcher;
+import jakarta.servlet.ServletException;
+import jakarta.servlet.http.HttpFilter;
+import jakarta.servlet.http.HttpServletRequest;
+import jakarta.servlet.http.HttpServletResponse;
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.List;
+import java.util.regex.Matcher;
+import java.util.regex.Pattern;
+
+/**
+ * Filter to identify paths that should be processed by Jetty's DefaultServlet. Typically, these
+ * paths contain static resources that need to be returned verbatim, with appropriate content type,
+ * which Jetty will determine via
+ *
+ * <p>{@code org.eclipse.jetty.http.MimeTypes#getMimeByExtension(java.lang.String)}
+ */
+public class PathExclusionFilter extends HttpFilter {
+
+  private List<Pattern> excludePatterns;
+
+  boolean shouldBeExcluded(HttpServletRequest request) {
+    String requestPath = ServletUtils.getPathAfterContext(request);
+    if (excludePatterns != null) {
+      return excludePatterns.stream().map(p -> p.matcher(requestPath)).anyMatch(Matcher::lookingAt);
+    }
+    return false;
+  }
+
+  @Override
+  public void init(FilterConfig config) throws ServletException {
+    String patternConfig = config.getInitParameter("excludePatterns");
+    if (patternConfig != null) {
+      String[] excludeArray = patternConfig.split(",");
+      this.excludePatterns = Arrays.stream(excludeArray).map(Pattern::compile).toList();
+    }
+    super.init(config);
+  }
+
+  @Override
+  protected void doFilter(HttpServletRequest req, HttpServletResponse res, FilterChain chain)
+      throws IOException, ServletException {
+    if (shouldBeExcluded(req)) {
+      // N.B. "default" is the name for org.eclipse.jetty.ee10.servlet.DefaultServlet
+      // configured in solr/server/etc/webdefault.xml if it doesn't exist something is
+      // very wrong.
+      RequestDispatcher defaultServlet = req.getServletContext().getNamedDispatcher("default");
+      if (defaultServlet == null) {
+        res.sendError(
+            500,
+            "Server Misconfiguration: cannot find default servlet (normally defined as org.eclipse.jetty.ee10.servlet.DefaultServlet in webdefault.xml)");
+      } else {
+        defaultServlet.forward(req, res);
+      }
+    } else {
+      chain.doFilter(req, res);
+    }
+  }
+}

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

@@ -19,7 +19,6 @@
 
 import io.opentelemetry.api.trace.Span;
 import io.opentelemetry.context.Context;
-import jakarta.servlet.FilterChain;
 import jakarta.servlet.ReadListener;
 import jakarta.servlet.ServletException;
 import jakarta.servlet.ServletInputStream;
@@ -33,10 +32,6 @@
 import java.io.InputStream;
 import java.io.OutputStream;
 import java.lang.invoke.MethodHandles;
-import java.util.ArrayList;
-import java.util.List;
-import java.util.regex.Matcher;
-import java.util.regex.Pattern;
 import org.apache.solr.common.SolrException;
 import org.apache.solr.common.SolrException.ErrorCode;
 import org.apache.solr.common.util.Utils;
@@ -131,45 +126,6 @@ public void close() {
     };
   }
 
-  static boolean excludedPath(
-      List<Pattern> excludePatterns,
-      HttpServletRequest request,
-      HttpServletResponse response,
-      FilterChain chain)
-      throws IOException, ServletException {
-    String requestPath = getPathAfterContext(request);
-    // No need to even create the HttpSolrCall object if this path is excluded.
-    if (excludePatterns != null) {
-      for (Pattern p : excludePatterns) {
-        Matcher matcher = p.matcher(requestPath);
-        if (matcher.lookingAt()) {
-          if (chain != null) {
-            chain.doFilter(request, response);
-          }
-          return true;
-        }
-      }
-    }
-    return false;
-  }
-
-  static boolean excludedPath(
-      List<Pattern> excludePatterns, HttpServletRequest request, HttpServletResponse response)
-      throws IOException, ServletException {
-    return excludedPath(excludePatterns, request, response, null);
-  }
-
-  static void configExcludes(PathExcluder excluder, String patternConfig) {
-    if (patternConfig != null) {
-      String[] excludeArray = patternConfig.split(",");
-      List<Pattern> patterns = new ArrayList<>();
-      excluder.setExcludePatterns(patterns);
-      for (String element : excludeArray) {
-        patterns.add(Pattern.compile(element));
-      }
-    }
-  }
-
   /**
    * 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.

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

@@ -17,8 +17,6 @@
 package org.apache.solr.servlet;
 
 import static org.apache.solr.servlet.ServletUtils.closeShield;
-import static org.apache.solr.servlet.ServletUtils.configExcludes;
-import static org.apache.solr.servlet.ServletUtils.excludedPath;
 import static org.apache.solr.util.tracing.TraceUtils.getSpan;
 import static org.apache.solr.util.tracing.TraceUtils.setTracer;
 
@@ -67,7 +65,7 @@
 // servlets that are more focused in scope. This should become possible now that we have a
 // ServletContextListener for startup/shutdown of CoreContainer that sets up a service from which
 // things like CoreContainer can be requested. (or better yet injected)
-public class SolrDispatchFilter extends HttpFilter implements PathExcluder {
+public class SolrDispatchFilter extends HttpFilter {
   private static final Logger log = LoggerFactory.getLogger(MethodHandles.lookup().lookupClass());
 
   private CoreContainerProvider containerProvider;
@@ -78,11 +76,6 @@ public class SolrDispatchFilter extends HttpFilter implements PathExcluder {
 
   private HttpSolrCallFactory solrCallFactory;
 
-  @Override
-  public void setExcludePatterns(List<Pattern> excludePatterns) {
-    this.excludePatterns = excludePatterns;
-  }
-
   private List<Pattern> excludePatterns;
 
   public final boolean isV2Enabled = V2ApiUtils.isEnabled();
@@ -133,7 +126,6 @@ public void init(FilterConfig config) throws ServletException {
         log.trace("SolrDispatchFilter.init(): {}", this.getClass().getClassLoader());
       }
 
-      configExcludes(this, config.getInitParameter("excludePatterns"));
     } catch (Throwable t) {
       // catch this so our filter still works
       log.error("Could not start Dispatch Filter.", t);
@@ -157,9 +149,6 @@ public CoreContainer getCores() throws UnavailableException {
           "Set the thread contextClassLoader for all 3rd party dependencies that we cannot control")
   public void doFilter(HttpServletRequest request, HttpServletResponse response, FilterChain chain)
       throws IOException, ServletException {
-    if (excludedPath(excludePatterns, request, response, chain)) {
-      return;
-    }
 
     try (var mdcSnapshot = MDCSnapshot.create()) {
       assert null != mdcSnapshot; // prevent compiler warning

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

@@ -61,6 +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.PathExclusionFilter;
 import org.apache.solr.servlet.SolrDispatchFilter;
 import org.apache.solr.util.SocketProxy;
 import org.apache.solr.util.TimeOut;
@@ -109,8 +110,9 @@ public class JettySolrRunner {
 
   private Server server;
 
-  volatile FilterHolder dispatchFilter;
   volatile FilterHolder debugFilter;
+  volatile FilterHolder pathExcludeFilter;
+  volatile FilterHolder dispatchFilter;
 
   private int jettyPort = -1;
 
@@ -398,14 +400,35 @@ public void contextInitialized(ServletContextEvent event) {
       for (Map.Entry<ServletHolder, String> entry : config.extraServlets.entrySet()) {
         root.addServlet(entry.getKey(), entry.getValue());
       }
+      // TODO: This needs to be driven by a parsing of web.xml eventually
+      //  though we still want to avoid classpath scanning.
+
+      // this path excludes filter isn't actually necessary for any tests, but it's being
+      // added for parity with the live application.
+      pathExcludeFilter = root.getServletHandler().newFilterHolder(Source.EMBEDDED);
+      pathExcludeFilter.setHeldClass(PathExclusionFilter.class);
+      pathExcludeFilter.setInitParameter("excludePatterns", excludePatterns);
+
+      // This is our main workhorse
       dispatchFilter = root.getServletHandler().newFilterHolder(Source.EMBEDDED);
       dispatchFilter.setHeldClass(SolrDispatchFilter.class);
-      dispatchFilter.setInitParameter("excludePatterns", excludePatterns);
+
       // Map dispatchFilter in same path as in web.xml
+      root.addFilter(pathExcludeFilter, "/*", EnumSet.of(DispatcherType.REQUEST));
       root.addFilter(dispatchFilter, "/*", EnumSet.of(DispatcherType.REQUEST));
 
       // Default servlet as a fall-through
-      root.addServlet(Servlet404.class, "/");
+      ServletHolder defaultHolder = root.getServletHandler().newServletHolder(Source.EMBEDDED);
+
+      // considered adding DefaultServlet.class here but perhaps that might grant our unit tests
+      // the power to serve static resources on the build machines? Not sure, so I'll just give a
+      // name to our existing hack. The tests passed without this, but it will ensure that if anyone
+      // ever hits the PathExcludeFilter in the unit test they get a 404 as before not a 500
+      defaultHolder.setHeldClass(Servlet404.class);
+      defaultHolder.setName("default");
+      root.addServlet(defaultHolder, "/");
+
+      // TODO: end area that should be driven by web.xml and webdefault.xml
       chain = root;
     }
 

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

@@ -26,11 +26,11 @@
   </listener>
   <!-- Any path (name) registered in solrconfig.xml will be sent to that filter -->
   <filter>
-    <filter-name>SolrRequestFilter</filter-name>
-    <filter-class>org.apache.solr.servlet.SolrDispatchFilter</filter-class>
+    <filter-name>PathExclusionsFilter</filter-name>
+    <filter-class>org.apache.solr.servlet.PathExclusionFilter</filter-class>
     <!--
-    Exclude patterns is a list of directories that would be short circuited by the
-    SolrDispatchFilter. It includes all Admin UI related static content.
+    Exclude patterns is a list of directories that would be short-circuited by this
+    Filter. It includes all Admin UI related static content.
     NOTE: It is NOT a pattern but only matches the start of the HTTP ServletPath.
     -->
     <init-param>
@@ -39,6 +39,16 @@
     </init-param>
   </filter>
 
+  <filter-mapping>
+    <filter-name>PathExclusionsFilter</filter-name>
+    <url-pattern>/*</url-pattern>
+  </filter-mapping>
+
+  <filter>
+    <filter-name>SolrRequestFilter</filter-name>
+    <filter-class>org.apache.solr.servlet.SolrDispatchFilter</filter-class>
+  </filter>
+
   <filter-mapping>
     <filter-name>SolrRequestFilter</filter-name>
     <url-pattern>/*</url-pattern>