Add request filter for early user and query parsing #748

vishalya · 2025-08-11T20:03:14Z

Description

The parsing of the user and query was happening at different places and at a later stages, this PR structures it to happen at a early stage and a single point in time.

Additional context and related issues

Early Request Parsing: New JAX-RS filters have been added to intercept incoming requests. This filter is responsible for parsing essential user and query information at the earliest stage of processing under the PRE_AUTHENTICATION and PRE_AUTHORIZATION.
Use of ContainerRequestContext: The implementation now uses ContainerRequestContext instead of HttpServletRequest to access request details, as this is the object available within the JAX-RS filter context.
Injecting Request Attributes: The extracted user and query information is now passed along via request attributes, making it easily accessible in the later stages of request routing

Release notes

(X) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required, with the following suggested text:

gateway-ha/src/main/java/io/trino/gateway/ha/config/HaGatewayConfiguration.java

gateway-ha/src/main/java/io/trino/gateway/ha/router/TrinoQueryProperties.java

vishalya · 2025-08-12T16:50:50Z

Addressed the review comments.

RoeyoOgen · 2025-08-17T06:55:56Z

gateway-ha/src/main/java/io/trino/gateway/ha/security/QueryMetadataParser.java

+    {
+        String path = requestContext.getUriInfo().getRequestUri().getPath();
+        RequestAnalyzerConfig reqAnalyzerconfig = haGatewayConfiguration.getRequestAnalyzerConfig();
+        if (!reqAnalyzerconfig.isAnalyzeRequest() ||


why do you in some places (like here) explicitly call reqAnalyzerconfig.isAnalyzeRequest(), and in others (FileBasedRoutingGroupSelector innit) do you do analyzeRequest = requestAnalyzerConfig.isAnalyzeRequest();

Here, I need a reference to haGatewayConfiguration for the isPathWhiteListed in addition to RequestAnalyzerConfig, so the pattern of usage is different.

RoeyoOgen · 2025-08-17T06:56:18Z

gateway-ha/src/main/java/io/trino/gateway/ha/security/QueryMetadataParser.java

+            return;
+        }
+
+        log.info("Processing query metadata");


maybe add queryID?

maybe change to debug and not info? could flood logs as its done for every req.

We may not have query id on certain requests but converting it into debug.

RoeyoOgen · 2025-08-17T07:01:13Z

gateway-ha/src/main/java/io/trino/gateway/ha/security/QueryUserInfoParser.java

+/**
+ *
+ * A filter to parse the query request headers related to user and store the user information as a property
+ * in the request for later use
+ */


Suggested change

/**

*

* A filter to parse the query request headers related to user and store the user information as a property

* in the request for later use

*/

/**

* A filter which parses and extracts Trino user identity from incoming request headers

* and stores it in the request context property TRINO_REQUEST_USER

* for downstream filters and handlers to use.

*/

RoeyoOgen · 2025-08-17T07:06:53Z

gateway-ha/src/main/java/io/trino/gateway/ha/security/QueryUserInfoParser.java

+    }
+
+    @Override
+    public void filter(ContainerRequestContext requestContext)


NIT: filter both parses user info and stores it in request context. Perhaps split user extraction logic into a util func?

RoeyoOgen · 2025-08-17T07:08:23Z

gateway-ha/src/main/java/io/trino/gateway/ha/security/QueryMetadataParser.java

+            throws IOException
+    {
+        String path = requestContext.getUriInfo().getRequestUri().getPath();
+        RequestAnalyzerConfig reqAnalyzerconfig = haGatewayConfiguration.getRequestAnalyzerConfig();


Suggested change

RequestAnalyzerConfig reqAnalyzerconfig = haGatewayConfiguration.getRequestAnalyzerConfig();

RequestAnalyzerConfig reqAnalyzerConfig = haGatewayConfiguration.getRequestAnalyzerConfig();

missing caps

https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#avoid-abbreviations

RoeyoOgen · 2025-08-17T07:17:33Z

gateway-ha/src/test/java/io/trino/gateway/ha/handler/TestQueryIdCachingProxyHandler.java

+                .httpHeader("X-Trino-Catalog", defaultCatalog)
+                .httpHeader("X-trino-catalog", defaultCatalog)


why is there both capital T and lower t? in -Trino-?
official docs only has uppercase.
(applies to below as well)

Making it consistent.

RoeyoOgen · 2025-08-17T10:20:55Z

gateway-ha/src/test/java/io/trino/gateway/ha/util/QueryRequestMock.java

+    private ContainerRequestContext requestContext = mock(ContainerRequest.class);
+    private HttpServletRequest mockRequest = mock(HttpServletRequest.class);


mockRequest and requestContext are reused, do you want to retain state across multiple test runs ?

The idea is that a new object of QueryRequestMock will be created for each test.

RoeyoOgen · 2025-08-17T10:23:52Z

gateway-ha/src/test/java/io/trino/gateway/ha/security/TestQueryMetadataParser.java

+        MediaType mediaType = new MediaType("application", "json", java.util.Map.of("charset", "UTF-8"));
+        when(requestContext.getMediaType()).thenReturn(mediaType);
+
+        String json = "Select xyz from cat1.schema1.table1";


Suggested change

String json = "Select xyz from cat1.schema1.table1";

String queryText = "Select xyz from cat1.schema1.table1";

RoeyoOgen · 2025-08-17T10:25:41Z

gateway-ha/src/test/java/io/trino/gateway/ha/security/TestQueryMetadataParser.java

+        verify(requestContext).setProperty(eq(TRINO_QUERY_PROPERTIES), captor.capture());
+
+        // Optionally, check that the entity stream was read (if your filter does so)
+        verify(requestContext).getEntityStream();


you check here the mock was called, perhaps check also that buffering worked or query was parsed?

Added the buffering check.

RoeyoOgen · 2025-08-17T10:26:18Z

gateway-ha/src/test/java/io/trino/gateway/ha/security/TestQueryMetadataParser.java

+    {
+        HaGatewayConfiguration config = new HaGatewayConfiguration();
+        requestAnalyzerConfig = new RequestAnalyzerConfig();
+        requestAnalyzerConfig.setAnalyzeRequest(true);


Only tests the happy path when = true. What about false?

Added some false paths too.

ebyhr · 2025-08-26T07:30:55Z

gateway-ha/src/test/java/io/trino/gateway/ha/security/TestQueryUserInfoParser.java

+import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.when;
+
+class TestQueryUserInfoParser


https://trino.io/docs/current/develop/tests.html

Test classes should be defined as package-private and final.

ebyhr · 2025-08-26T07:32:11Z

gateway-ha/src/test/java/io/trino/gateway/ha/security/TestQueryUserInfoParser.java

+{
+    private QueryUserInfoParser filter;
+
+    @BeforeEach


https://trino.io/docs/current/develop/tests.html#keep-test-setup-and-teardown-simple

ebyhr · 2025-08-26T07:32:31Z

gateway-ha/src/test/java/io/trino/gateway/ha/security/TestQueryUserInfoParser.java

+        catch (URISyntaxException e) {
+            throw new RuntimeException(e);
+        }


Remove redundant catch block.

ebyhr · 2025-08-26T07:32:48Z

gateway-ha/src/test/java/io/trino/gateway/ha/security/TestQueryUserInfoParser.java

+
+        UriInfo uriInfo = mock(ExtendedUriInfo.class);
+        try {
+            when(uriInfo.getRequestUri()).thenReturn(new URI("http://localhost" + HttpUtils.V1_STATEMENT_PATH));


Static import V1_STATEMENT_PATH.

ebyhr · 2025-08-26T07:34:25Z

gateway-ha/src/test/java/io/trino/gateway/ha/router/TestExternalRoutingGroupSelector.java

@@ -89,7 +89,7 @@ static RulesExternalConfiguration provideRoutingRuleExternalConfig()

    @Test
    void testByRoutingRulesExternalEngine()
-            throws URISyntaxException
+            throws URISyntaxException, IOException


Throwing Exception is fine in test methods.

ebyhr · 2025-08-26T07:35:01Z

gateway-ha/src/main/java/io/trino/gateway/ha/security/util/Priorities.java

+ */
+package io.trino.gateway.ha.security.util;
+
+public final class Priorities


The name is too general.

ebyhr · 2025-08-26T07:35:11Z

gateway-ha/src/main/java/io/trino/gateway/ha/security/util/Priorities.java

+    public static final int HEADER_DECORATOR = 3000;
+    public static final int ENTITY_CODER = 4000;
+    public static final int USER = 5000;
+    public static final int ROUTE_TO_BACKEND = 6000;


Remove unsued codes.

ebyhr · 2025-08-26T07:35:41Z

gateway-ha/src/main/java/io/trino/gateway/ha/security/QueryUserInfoParser.java

+        implements ContainerRequestFilter
+{
+    private static final Logger log = Logger.get(QueryUserInfoParser.class);
+    private final HaGatewayConfiguration haGatewayConfiguration;


Don't put mutable objects in fields. Same for other classes.

ebyhr · 2025-08-26T07:37:14Z

gateway-ha/src/main/java/io/trino/gateway/ha/security/QueryMetadataParser.java

+            throws IOException
+    {
+        String path = requestContext.getUriInfo().getRequestUri().getPath();
+        RequestAnalyzerConfig reqAnalyzerconfig = haGatewayConfiguration.getRequestAnalyzerConfig();


https://github.com/trinodb/trino/blob/master/.github/DEVELOPMENT.md#avoid-abbreviations

oneonestar · 2025-09-02T07:54:14Z

How about using a single filter to parse all the data instead of having multiple filters?
We can store all the necessary data in a single object.
I think this would simplify the code and also help avoid dependency or execution order issues between multiple filters in the future.

vishalya · 2025-09-03T13:23:08Z

I would like to keep it separate as they are different and will be parsed at different priorities (user info might need to be parsed at pre-authentication and query might be parsed at pre-authorization), The query information may not be present for all the requests.

vishalya · 2025-09-16T21:03:14Z

Added a new class PathFilter to reuse the logic in the filters.

vishalya · 2025-09-16T21:03:28Z

@ebyhr - I have addressed the review comments.

Chaho12 · 2025-09-17T13:36:24Z

gateway-ha/src/main/java/io/trino/gateway/ha/router/TrinoQueryProperties.java

+
+        String charset = mediaType.getParameters().get("charset");
+        if (!StandardCharsets.UTF_8.name().equalsIgnoreCase(charset)) {
+            return;


should we log a warning that it is missing charset? because request without charset is quite common isn't it?
sth like this?

log.debug("Request charset is not UTF-8 (%s), skipping query parsing", charset);

Chaho12 · 2025-09-17T13:37:22Z

gateway-ha/src/main/java/io/trino/gateway/ha/router/TrinoQueryProperties.java

 import java.net.URLDecoder;
+import java.nio.charset.StandardCharsets;


in like 93, we already imported static StandardCharsets.UTF_8. We should use that instead

Chaho12 · 2025-09-17T13:59:09Z

gateway-ha/src/main/java/io/trino/gateway/ha/security/QueryUserInfoParser.java

+
+        TrinoRequestUser user = new TrinoRequestUser.TrinoRequestUserProvider(requestAnalyzerConfig).getInstance(requestContext);
+        requestContext.setProperty(TRINO_REQUEST_USER, user);
+        log.debug("Parsed user %s", user.getUser().orElse("None"));


Nit.

Suggested change

log.debug("Parsed user %s", user.getUser().orElse("None"));

log.debug("Parsed user: %s", user.getUser().orElse("None"));

Chaho12 · 2025-09-17T14:00:56Z

gateway-ha/src/main/java/io/trino/gateway/ha/security/QueryUserInfoParser.java

+            return;
+        }
+
+        RequestAnalyzerConfig requestAnalyzerConfig = new RequestAnalyzerConfig();


Do we need to create a new RequestAnalyzerConfig object instead of reusing the configuration that was already injected into the class? Then there is no need to extract and recreate fields.

Chaho12 · 2025-09-17T14:02:28Z

gateway-ha/src/main/java/io/trino/gateway/ha/router/PathFilter.java

+            @Named("statementPaths") List<String> statementPaths,
+            @Named("extraWhitelistPaths") List<String> extraWhitelistPaths)
+    {
+        this.statementPaths = Set.copyOf(statementPaths);


nit. log that it cannot be null?

Suggested change

this.statementPaths = Set.copyOf(statementPaths);

this.statementPaths = Set.copyOf(requireNonNull(statementPaths, "statementPaths cannot be null"));

Chaho12 · 2025-09-17T14:03:50Z

gateway-ha/src/main/java/io/trino/gateway/ha/router/PathFilter.java

+        this.extraWhitelistPatterns = requireNonNull(extraWhitelistPaths).stream()
+                .map(Pattern::compile)
+                .collect(toImmutableList());


same. tell pattern cannot be null and throw the pattern exception log

Suggested change

this.extraWhitelistPatterns = requireNonNull(extraWhitelistPaths).stream()

.map(Pattern::compile)

.collect(toImmutableList());

this.extraWhitelistPatterns = requireNonNull(extraWhitelistPaths, "extraWhitelistPaths cannot be null").stream()

.map(pattern -> {

try {

return Pattern.compile(pattern);

} catch (PatternSyntaxException e) {

throw new IllegalArgumentException("Invalid regex pattern: " + pattern, e);

}

})

.collect(toImmutableList());

Chaho12 · 2025-09-17T14:05:49Z

gateway-ha/src/main/java/io/trino/gateway/ha/security/QueryMetadataParser.java

+            return;
+        }
+
+        log.debug("Processing query metadata");


nit. make it for useful and tell path

Suggested change

log.debug("Processing query metadata");

log.debug("Processing query metadata for path: %s", path);

Chaho12 · 2025-09-17T14:10:20Z

gateway-ha/src/main/java/io/trino/gateway/ha/security/QueryMetadataParser.java

+        TrinoQueryProperties queryProps = new TrinoQueryProperties(requestContext,
+                isClientsUseV2Format,
+                maxBodySize);
+
+        requestContext.setProperty(TRINO_QUERY_PROPERTIES, queryProps);


How about try catch exception as TrinoQueryProperties could throw exception?
Cause gateway should continue processing even if query parsing fails

Chaho12 · 2025-09-17T14:16:01Z

gateway-ha/src/main/java/io/trino/gateway/ha/router/TrinoQueryProperties.java

+        }
+
+        String charset = mediaType.getParameters().get("charset");
+        if (!StandardCharsets.UTF_8.name().equalsIgnoreCase(charset)) {


We should first check if charset is null first.

Chaho12 · 2025-09-17T14:18:11Z

gateway-ha/src/test/java/io/trino/gateway/ha/router/TestPathFilter.java

+        // Should not allow other paths
+        assertThat(statementOnlyFilter.isPathWhiteListed("/api/custom")).isFalse();
+        assertThat(statementOnlyFilter.isPathWhiteListed("/health")).isFalse();
+    }


can you add test for configuration validatinons? Test null statement paths, null whitelist paths, invalid regex pattern etc.

cla-bot bot added the cla-signed label Aug 11, 2025

vishalya requested review from oneonestar, RoeyoOgen, Chaho12 and andythsu August 11, 2025 20:03

Chaho12 reviewed Aug 12, 2025

View reviewed changes

vishalya force-pushed the parse-user-query branch from 5c57364 to 28caef2 Compare August 12, 2025 16:50

RoeyoOgen reviewed Aug 17, 2025

View reviewed changes

ebyhr requested changes Aug 26, 2025

View reviewed changes

vishalya force-pushed the parse-user-query branch 2 times, most recently from 239f66a to 68e1860 Compare August 29, 2025 20:45

vishalya force-pushed the parse-user-query branch from 68e1860 to b97df7f Compare September 16, 2025 17:32

Add request filters for early user and query parsing

b202da0

vishalya force-pushed the parse-user-query branch from b97df7f to b202da0 Compare September 16, 2025 20:36

Chaho12 reviewed Sep 17, 2025

View reviewed changes

-/**
- *
- * A filter to parse the query request headers related to user and store the user information as a property
- * in the request for later use
- */
+/**
+ * A filter which parses and extracts Trino user identity from incoming request headers
+ * and stores it in the request context property TRINO_REQUEST_USER
+ * for downstream filters and handlers to use.
+ */

	RequestAnalyzerConfig reqAnalyzerconfig = haGatewayConfiguration.getRequestAnalyzerConfig();
	RequestAnalyzerConfig reqAnalyzerConfig = haGatewayConfiguration.getRequestAnalyzerConfig();

		.httpHeader("X-Trino-Catalog", defaultCatalog)
		.httpHeader("X-trino-catalog", defaultCatalog)

		private ContainerRequestContext requestContext = mock(ContainerRequest.class);
		private HttpServletRequest mockRequest = mock(HttpServletRequest.class);

	String json = "Select xyz from cat1.schema1.table1";
	String queryText = "Select xyz from cat1.schema1.table1";

		import java.net.URLDecoder;
		import java.nio.charset.StandardCharsets;

	log.debug("Parsed user %s", user.getUser().orElse("None"));
	log.debug("Parsed user: %s", user.getUser().orElse("None"));

	this.statementPaths = Set.copyOf(statementPaths);
	this.statementPaths = Set.copyOf(requireNonNull(statementPaths, "statementPaths cannot be null"));

-        this.extraWhitelistPatterns = requireNonNull(extraWhitelistPaths).stream()
-                .map(Pattern::compile)
-                .collect(toImmutableList());
+        this.extraWhitelistPatterns = requireNonNull(extraWhitelistPaths, "extraWhitelistPaths cannot be null").stream()
+                .map(pattern -> {
+                    try {
+                        return Pattern.compile(pattern);
+                    } catch (PatternSyntaxException e) {
+                        throw new IllegalArgumentException("Invalid regex pattern: " + pattern, e);
+                    }
+                })
+                .collect(toImmutableList());

	log.debug("Processing query metadata");
	log.debug("Processing query metadata for path: %s", path);

Add request filter for early user and query parsing #748

Are you sure you want to change the base?

Add request filter for early user and query parsing #748

Uh oh!

Conversation

vishalya commented Aug 11, 2025

Description

Additional context and related issues

Release notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

vishalya commented Aug 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

oneonestar commented Sep 2, 2025

Uh oh!

vishalya commented Sep 3, 2025

Uh oh!

vishalya commented Sep 16, 2025

Uh oh!

vishalya commented Sep 16, 2025

Uh oh!

Chaho12 Sep 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Chaho12 Sep 17, 2025 •

edited

Loading

Chaho12 Sep 17, 2025 •

edited

Loading