Add QoSFilters first in the chain. (#12625)

* Add QoSFilters first in the chain.

When a request is suspended and later resumed due to QoS constraints,
its filter chain is restarted. Placing QoSFilters first in the chain
avoids double-execution of other filters.

Fixes an issue where requests deferred by QoS would report 403 Forbidden
due to double-execution of SecuritySanityCheckFilter.

* Smaller changes.

* Add QoS filters in BaseJettyTest.

* Remove unused parameter.

server/src/main/java/org/apache/druid/server/initialization/jetty/JettyBindings.java

@@ -22,7 +22,6 @@ package org.apache.druid.server.initialization.jetty;
 import com.google.common.collect.ImmutableMap;
 import com.google.inject.Binder;
 import com.google.inject.multibindings.Multibinder;
-import org.apache.druid.java.util.common.logger.Logger;
 import org.eclipse.jetty.server.Handler;
 import org.eclipse.jetty.servlets.QoSFilter;
 
@@ -33,22 +32,14 @@ import java.util.Map;
 
 public class JettyBindings
 {
-  private static final Logger log = new Logger(JettyBindings.class);
-
   private JettyBindings()
   {
     // No instantiation.
   }
 
-  public static void addQosFilter(Binder binder, String paths, int maxRequests)
+  public static void addQosFilter(Binder binder, String path, int maxRequests)
   {
-    if (maxRequests <= 0) {
+    addQosFilter(binder, new String[]{path}, maxRequests);
-      return;
-    }
-
-    Multibinder.newSetBinder(binder, ServletFilterHolder.class)
-               .addBinding()
-               .toInstance(new QosFilterHolder(new String[]{paths}, maxRequests));
   }
 
   public static void addQosFilter(Binder binder, String[] paths, int maxRequests)
@@ -57,7 +48,7 @@ public class JettyBindings
       return;
     }
 
-    Multibinder.newSetBinder(binder, ServletFilterHolder.class)
+    Multibinder.newSetBinder(binder, QosFilterHolder.class)
                .addBinding()
                .toInstance(new QosFilterHolder(paths, maxRequests));
   }
@@ -69,7 +60,7 @@ public class JettyBindings
                .to(handlerClass);
   }
 
-  private static class QosFilterHolder implements ServletFilterHolder
+  public static class QosFilterHolder implements ServletFilterHolder
   {
     private final String[] paths;
     private final int maxRequests;

server/src/main/java/org/apache/druid/server/initialization/jetty/JettyServerInitUtils.java

@@ -54,11 +54,28 @@ public class JettyServerInitUtils
     return gzipHandler;
   }
 
+  /**
+   * Add any filters that were registered with {@link JettyBindings#addQosFilter}. These must be added first in
+   * the filter chain, because when a request is suspended and later resumed due to QoS constraints, its filter
+   * chain is restarted. Placing QoSFilters first in the chain avoids double-execution of other filters.
+   */
+  public static void addQosFilters(ServletContextHandler handler, Injector injector)
+  {
+    final Set<JettyBindings.QosFilterHolder> filters =
+        injector.getInstance(Key.get(new TypeLiteral<Set<JettyBindings.QosFilterHolder>>() {}));
+    addFilters(handler, filters);
+  }
+
   public static void addExtensionFilters(ServletContextHandler handler, Injector injector)
   {
-    Set<ServletFilterHolder> extensionFilters = injector.getInstance(Key.get(new TypeLiteral<Set<ServletFilterHolder>>(){}));
+    final Set<ServletFilterHolder> filters =
+        injector.getInstance(Key.get(new TypeLiteral<Set<ServletFilterHolder>>() {}));
+    addFilters(handler, filters);
+  }
 
-    for (ServletFilterHolder servletFilterHolder : extensionFilters) {
+  public static void addFilters(ServletContextHandler handler, Set<? extends ServletFilterHolder> filterHolders)
+  {
+    for (ServletFilterHolder servletFilterHolder : filterHolders) {
       // Check the Filter first to guard against people who don't read the docs and return the Class even
       // when they have an instance.
       FilterHolder holder;

server/src/main/java/org/apache/druid/server/initialization/jetty/JettyServerModule.java

@@ -133,6 +133,7 @@ public class JettyServerModule extends JerseyServletModule
 
     // Add empty binding for Handlers so that the injector returns an empty set if none are provided by extensions.
     Multibinder.newSetBinder(binder, Handler.class);
+    Multibinder.newSetBinder(binder, JettyBindings.QosFilterHolder.class);
     Multibinder.newSetBinder(binder, ServletFilterHolder.class)
                .addBinding()
                .to(StandardResponseHeaderFilterHolder.class);

server/src/test/java/org/apache/druid/server/initialization/BaseJettyTest.java

@@ -150,6 +150,7 @@ public abstract class BaseJettyTest
     {
       final ServletContextHandler root = new ServletContextHandler(ServletContextHandler.SESSIONS);
       root.addServlet(new ServletHolder(new DefaultServlet()), "/*");
+      JettyServerInitUtils.addQosFilters(root, injector);
       JettyServerInitUtils.addExtensionFilters(root, injector);
       root.addFilter(GuiceFilter.class, "/*", null);
 

services/src/main/java/org/apache/druid/cli/CliOverlord.java

@@ -396,6 +396,7 @@ public class CliOverlord extends ServerRunnable
       final ObjectMapper jsonMapper = injector.getInstance(Key.get(ObjectMapper.class, Json.class));
       final AuthenticatorMapper authenticatorMapper = injector.getInstance(AuthenticatorMapper.class);
 
+      JettyServerInitUtils.addQosFilters(root, injector);
       AuthenticationUtils.addSecuritySanityCheckFilter(root, jsonMapper);
 
       // perform no-op authorization/authentication for these resources

services/src/main/java/org/apache/druid/cli/CoordinatorJettyServerInitializer.java

@@ -26,7 +26,6 @@ import com.google.inject.Injector;
 import com.google.inject.Key;
 import com.google.inject.servlet.GuiceFilter;
 import org.apache.druid.guice.annotations.Json;
-import org.apache.druid.server.coordinator.DruidCoordinatorConfig;
 import org.apache.druid.server.http.OverlordProxyServlet;
 import org.apache.druid.server.http.RedirectFilter;
 import org.apache.druid.server.initialization.ServerConfig;
@@ -51,21 +50,19 @@ import java.util.Properties;
  */
 class CoordinatorJettyServerInitializer implements JettyServerInitializer
 {
-  private static List<String> UNSECURED_PATHS = ImmutableList.of(
+  private static final List<String> UNSECURED_PATHS = ImmutableList.of(
       "/coordinator/false",
       "/overlord/false",
       "/status/health",
       "/druid/coordinator/v1/isLeader"
   );
 
-  private final DruidCoordinatorConfig config;
   private final boolean beOverlord;
   private final ServerConfig serverConfig;
 
   @Inject
-  CoordinatorJettyServerInitializer(DruidCoordinatorConfig config, Properties properties, ServerConfig serverConfig)
+  CoordinatorJettyServerInitializer(Properties properties, ServerConfig serverConfig)
   {
-    this.config = config;
     this.beOverlord = CliCoordinator.isOverlord(properties);
     this.serverConfig = serverConfig;
   }
@@ -84,6 +81,7 @@ class CoordinatorJettyServerInitializer implements JettyServerInitializer
     final ObjectMapper jsonMapper = injector.getInstance(Key.get(ObjectMapper.class, Json.class));
     final AuthenticatorMapper authenticatorMapper = injector.getInstance(AuthenticatorMapper.class);
 
+    JettyServerInitUtils.addQosFilters(root, injector);
     AuthenticationUtils.addSecuritySanityCheckFilter(root, jsonMapper);
 
     // perform no-op authorization/authentication for these resources

services/src/main/java/org/apache/druid/cli/MiddleManagerJettyServerInitializer.java

@@ -72,6 +72,7 @@ class MiddleManagerJettyServerInitializer implements JettyServerInitializer
     final ObjectMapper jsonMapper = injector.getInstance(Key.get(ObjectMapper.class, Json.class));
     final AuthenticatorMapper authenticatorMapper = injector.getInstance(AuthenticatorMapper.class);
 
+    JettyServerInitUtils.addQosFilters(root, injector);
     AuthenticationUtils.addSecuritySanityCheckFilter(root, jsonMapper);
 
     // perform no-op authorization/authentication for these resources

services/src/main/java/org/apache/druid/cli/QueryJettyServerInitializer.java

@@ -98,6 +98,7 @@ public class QueryJettyServerInitializer implements JettyServerInitializer
     final ObjectMapper jsonMapper = injector.getInstance(Key.get(ObjectMapper.class, Json.class));
     final AuthenticatorMapper authenticatorMapper = injector.getInstance(AuthenticatorMapper.class);
 
+    JettyServerInitUtils.addQosFilters(root, injector);
     AuthenticationUtils.addSecuritySanityCheckFilter(root, jsonMapper);
 
     // perform no-op authorization for these resources

services/src/main/java/org/apache/druid/cli/RouterJettyServerInitializer.java

@@ -118,6 +118,7 @@ public class RouterJettyServerInitializer implements JettyServerInitializer
     final ObjectMapper jsonMapper = injector.getInstance(Key.get(ObjectMapper.class, Json.class));
     final AuthenticatorMapper authenticatorMapper = injector.getInstance(AuthenticatorMapper.class);
 
+    JettyServerInitUtils.addQosFilters(root, injector);
     AuthenticationUtils.addSecuritySanityCheckFilter(root, jsonMapper);
 
     // perform no-op authorization/authentication for these resources