From 1f6e888472d3102f574a088286f92cb8feff4c86 Mon Sep 17 00:00:00 2001 From: Gian Merlino Date: Tue, 14 Jun 2022 13:37:00 -0700 Subject: [PATCH] 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. --- .../initialization/jetty/JettyBindings.java | 17 ++++----------- .../jetty/JettyServerInitUtils.java | 21 +++++++++++++++++-- .../jetty/JettyServerModule.java | 1 + .../server/initialization/BaseJettyTest.java | 1 + .../org/apache/druid/cli/CliOverlord.java | 1 + .../CoordinatorJettyServerInitializer.java | 8 +++---- .../MiddleManagerJettyServerInitializer.java | 1 + .../cli/QueryJettyServerInitializer.java | 1 + .../cli/RouterJettyServerInitializer.java | 1 + 9 files changed, 32 insertions(+), 20 deletions(-) diff --git a/server/src/main/java/org/apache/druid/server/initialization/jetty/JettyBindings.java b/server/src/main/java/org/apache/druid/server/initialization/jetty/JettyBindings.java index 9942acfca54..835564dc0b4 100644 --- a/server/src/main/java/org/apache/druid/server/initialization/jetty/JettyBindings.java +++ b/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) { - return; - } - - Multibinder.newSetBinder(binder, ServletFilterHolder.class) - .addBinding() - .toInstance(new QosFilterHolder(new String[]{paths}, maxRequests)); + addQosFilter(binder, new String[]{path}, 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; diff --git a/server/src/main/java/org/apache/druid/server/initialization/jetty/JettyServerInitUtils.java b/server/src/main/java/org/apache/druid/server/initialization/jetty/JettyServerInitUtils.java index bdcd7dbf9a4..fc4213b16ad 100644 --- a/server/src/main/java/org/apache/druid/server/initialization/jetty/JettyServerInitUtils.java +++ b/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 filters = + injector.getInstance(Key.get(new TypeLiteral>() {})); + addFilters(handler, filters); + } + public static void addExtensionFilters(ServletContextHandler handler, Injector injector) { - Set extensionFilters = injector.getInstance(Key.get(new TypeLiteral>(){})); + final Set filters = + injector.getInstance(Key.get(new TypeLiteral>() {})); + addFilters(handler, filters); + } - for (ServletFilterHolder servletFilterHolder : extensionFilters) { + public static void addFilters(ServletContextHandler handler, Set 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; diff --git a/server/src/main/java/org/apache/druid/server/initialization/jetty/JettyServerModule.java b/server/src/main/java/org/apache/druid/server/initialization/jetty/JettyServerModule.java index b28f061cece..5d4c6532e27 100644 --- a/server/src/main/java/org/apache/druid/server/initialization/jetty/JettyServerModule.java +++ b/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); diff --git a/server/src/test/java/org/apache/druid/server/initialization/BaseJettyTest.java b/server/src/test/java/org/apache/druid/server/initialization/BaseJettyTest.java index 8029f44e72c..422d336e3bf 100644 --- a/server/src/test/java/org/apache/druid/server/initialization/BaseJettyTest.java +++ b/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); diff --git a/services/src/main/java/org/apache/druid/cli/CliOverlord.java b/services/src/main/java/org/apache/druid/cli/CliOverlord.java index ff9d2002c89..c08dac3aeeb 100644 --- a/services/src/main/java/org/apache/druid/cli/CliOverlord.java +++ b/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 diff --git a/services/src/main/java/org/apache/druid/cli/CoordinatorJettyServerInitializer.java b/services/src/main/java/org/apache/druid/cli/CoordinatorJettyServerInitializer.java index 08f8e19361b..1f59901f1d8 100644 --- a/services/src/main/java/org/apache/druid/cli/CoordinatorJettyServerInitializer.java +++ b/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 UNSECURED_PATHS = ImmutableList.of( + private static final List 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 diff --git a/services/src/main/java/org/apache/druid/cli/MiddleManagerJettyServerInitializer.java b/services/src/main/java/org/apache/druid/cli/MiddleManagerJettyServerInitializer.java index bdf674aa59f..6e8f7151dfe 100644 --- a/services/src/main/java/org/apache/druid/cli/MiddleManagerJettyServerInitializer.java +++ b/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 diff --git a/services/src/main/java/org/apache/druid/cli/QueryJettyServerInitializer.java b/services/src/main/java/org/apache/druid/cli/QueryJettyServerInitializer.java index 814895ae0dd..b03e320dafa 100644 --- a/services/src/main/java/org/apache/druid/cli/QueryJettyServerInitializer.java +++ b/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 diff --git a/services/src/main/java/org/apache/druid/cli/RouterJettyServerInitializer.java b/services/src/main/java/org/apache/druid/cli/RouterJettyServerInitializer.java index a959f91e3bc..e4644225fb7 100644 --- a/services/src/main/java/org/apache/druid/cli/RouterJettyServerInitializer.java +++ b/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