Add cookie based routing for selected paths by willmostly · Pull Request #188 · trinodb/trino-gateway

willmostly · 2024-01-25T16:28:07Z

This adds an additional routing strategy for the Gateway. Currently the Gateway checks all incoming requests for a query ID. If one is found, the cluster running that query is looked up and the query is routed to that cluster. All other requests are sent to a semi random backend cluster in the routing group chosen by the RoutingGroupSelector based on the information present in the request. These routing strategies are not sufficient to solve issues such as #165. For #165, the Gateway needs the capability to route a request to the same backend that a previous request was sent to.

This adds the ability to add a session cookie, and to map that session cookie to a specific backend. This is lower precedence than the queryID based routing. Cookie based routing is performed only for paths in a configurable list. OAuth with multiple backends in a routing group (#165) can be solved by configuring cookiePaths: ['/oauth2/callback']. Finally, a cookie will be deleted if a request is made to a patch in the logOutCookiePaths list.

mosabua · 2024-01-25T19:45:29Z

I have not reviewed the code yet but the description of this PR should be improved and added to the docs.

Also we should consider how this relates to the work from @vishalya in terms of pluggable strategies.

Imho we should have a default ordered list of strategies that are used out of the box, but it should be possible to activate and deactivate any strategy and also potentially change the order/priority.

vishalya · 2024-02-28T00:32:25Z

Does the rule engine take a lower precedence for a cookie path?

Chaho12 · 2024-02-28T01:33:28Z

Please rebase after #265 gets merged. + docs on precedence over rule/queryid based routing would be great
This PR is also important as it fixes bug to handle multiple backends in same group.

willmostly · 2024-02-29T01:52:45Z

Does the rule engine take a lower precedence for a cookie path?

Yes. Similar to a queryId, the presence of a cookie along with a path in cookiePaths signifies that a request is related to a previous one, so you don't want to send the request to any backend besides the one the previous requests were sent to. As noted in the TODO comments, I'd like to support more sophisticated decision making on whether or not sticky routing is used, but for now looking at the path is enough to support the oauth handshake and potentially other use cases.

Hopefully the relationships between the different routing strategies are more clear now that this is rebased and includes the refactoring I did of of QueryIdCachingProxyHandler.

mosabua

Started review .. will continue later

mosabua · 2024-03-07T00:45:01Z

First review round completed. Looks like its in pretty good shape .. please rebase, update naming and such and let me know when its ready again.

Chaho12

~~Is cookiepath config~~

Chaho12 · 2024-03-08T05:34:54Z

+session cookie if none exists. If a cookie exists, route the request to
+the backend associated with that cookie.
+
+`removeCookiePaths`: If the request URI starts with a path in this list,


Why would you need user to configure this? Other than logout?

oneonestar

Skimming the first half

oneonestar · 2024-03-14T06:01:13Z

I saw a NPE error but the test passed. Is this expected?

./mvnw clean test -Dtest=TestGatewayHaMultipleBackend
...
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
!    THIS APPLICATION HAS NO HEALTHCHECKS. THIS MEANS YOU WILL NEVER KNOW      !
!     IF IT DIES IN PRODUCTION, WHICH MEANS YOU WILL NEVER KNOW IF YOU'RE      !
!    LETTING YOUR USERS DOWN. YOU SHOULD ADD A HEALTHCHECK FOR EACH OF YOUR    !
!         APPLICATION'S DEPENDENCIES WHICH FULLY (BUT LIGHTLY) TESTS IT.       !
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!
2024-03-14T05:59:05.553Z	INFO	main	org.eclipse.jetty.server.handler.ContextHandler	Started i.d.j.MutableServletContextHandler@14f35726{/,null,AVAILABLE}
2024-03-14T05:59:05.553Z	INFO	main	org.eclipse.jetty.server.AbstractConnector	Started application@30b16aa6{HTTP/1.1, (http/1.1)}{0.0.0.0:30575}
2024-03-14T05:59:05.554Z	INFO	main	org.eclipse.jetty.server.AbstractConnector	Started admin@1228a579{HTTP/1.1, (http/1.1)}{0.0.0.0:31088}
2024-03-14T05:59:05.554Z	INFO	main	org.eclipse.jetty.server.Server	Started Server@6a1b51a7{STARTING}[11.0.19,sto=30000] @17768ms
2024-03-14T05:59:05.556Z	INFO	main	io.trino.gateway.ha.HaGatewayTestUtils	okhttp3.RequestBody$2@72b16e26
2024-03-14T05:59:05.691Z	INFO	pool-8-thread-7 - POST /entity?entityType=GATEWAY_BACKEND	io.trino.gateway.ha.resource.EntityEditorResource	Setting up the backend trino1 with healthy state
2024-03-14T05:59:05.691Z	INFO	pool-8-thread-7 - POST /entity?entityType=GATEWAY_BACKEND	io.trino.gateway.ha.router.RoutingManager	backend trino1 isHealthy true
2024-03-14T05:59:05.716Z	INFO	main	io.trino.gateway.ha.HaGatewayTestUtils	okhttp3.RequestBody$2@5575b4c2
2024-03-14T05:59:05.732Z	INFO	pool-8-thread-2 - POST /entity?entityType=GATEWAY_BACKEND	io.trino.gateway.ha.resource.EntityEditorResource	Setting up the backend trino2 with healthy state
2024-03-14T05:59:05.732Z	INFO	pool-8-thread-2 - POST /entity?entityType=GATEWAY_BACKEND	io.trino.gateway.ha.router.RoutingManager	backend trino2 isHealthy true
2024-03-14T05:59:05.732Z	INFO	main	io.trino.gateway.ha.HaGatewayTestUtils	okhttp3.RequestBody$2@472cf6af
2024-03-14T05:59:05.747Z	INFO	pool-8-thread-8 - POST /entity?entityType=GATEWAY_BACKEND	io.trino.gateway.ha.resource.EntityEditorResource	Setting up the backend custom with healthy state
2024-03-14T05:59:05.747Z	INFO	pool-8-thread-8 - POST /entity?entityType=GATEWAY_BACKEND	io.trino.gateway.ha.router.RoutingManager	backend custom isHealthy true
2024-03-14T05:59:05.763Z	INFO	qtp1976110789-76	io.trino.gateway.ha.handler.QueryIdCachingProxyHandler	Rerouting [http://127.0.0.1:20110/v1/statement]--> [http://localhost:32791/v1/statement]
2024-03-14T05:59:05.763Z	INFO	qtp1976110789-76	io.trino.gateway.ha.handler.QueryIdCachingProxyHandler	Processing request endpoint: [/v1/statement], payload: [SELECT 1]
2024-03-14T05:59:05.773Z	INFO	qtp1976110789-93	io.trino.gateway.ha.handler.QueryIdCachingProxyHandler	user from X-Trino-User
2024-03-14T05:59:05.802Z	INFO	qtp1976110789-91	io.trino.gateway.ha.handler.QueryIdCachingProxyHandler	Rerouting [http://127.0.0.1:20110/v1/statement]--> [http://localhost:32792/v1/statement]
2024-03-14T05:59:05.802Z	INFO	qtp1976110789-91	io.trino.gateway.ha.handler.QueryIdCachingProxyHandler	Processing request endpoint: [/v1/statement], payload: [SELECT 1]
2024-03-14T05:59:05.819Z	INFO	qtp1976110789-72	io.trino.gateway.ha.handler.QueryIdCachingProxyHandler	user from X-Trino-User
2024-03-14T05:59:05.835Z	INFO	qtp1976110789-77	io.trino.gateway.ha.handler.QueryIdCachingProxyHandler	Rerouting [http://127.0.0.1:20110/oauth2]--> [http://localhost:21561/oauth2]
2024-03-14T05:59:05.880Z	ERROR	qtp1976110789-91	io.trino.gateway.ha.handler.QueryIdCachingProxyHandler	Error in proxying falling back to super call
java.lang.NullPointerException: Cannot read the array length because "array" is null
	at java.base/java.util.Arrays.stream(Arrays.java:5528)
	at io.trino.gateway.ha.handler.QueryIdCachingProxyHandler.setCookie(QueryIdCachingProxyHandler.java:306)
	at io.trino.gateway.ha.handler.QueryIdCachingProxyHandler.postConnectionHook(QueryIdCachingProxyHandler.java:350)
	at io.trino.gateway.proxyserver.ProxyServletImpl.onResponseContent(ProxyServletImpl.java:133)
	at org.eclipse.jetty.proxy.ProxyServlet$ProxyResponseListener.onContent(ProxyServlet.java:217)
	at org.eclipse.jetty.client.api.Response$AsyncContentListener.onContent(Response.java:197)
	at org.eclipse.jetty.client.ResponseNotifier.notifyContent(ResponseNotifier.java:150)
	at org.eclipse.jetty.client.ResponseNotifier.notifyContent(ResponseNotifier.java:134)
	at org.eclipse.jetty.client.HttpReceiver$ContentListeners.notifyContent(HttpReceiver.java:711)
	at org.eclipse.jetty.client.HttpReceiver.plainResponseContent(HttpReceiver.java:381)
	at org.eclipse.jetty.client.HttpReceiver.responseContent(HttpReceiver.java:364)
	at org.eclipse.jetty.client.http.HttpReceiverOverHTTP.content(HttpReceiverOverHTTP.java:344)
	at org.eclipse.jetty.http.HttpParser.parseContent(HttpParser.java:1854)
	at org.eclipse.jetty.http.HttpParser.parseNext(HttpParser.java:1565)
	at org.eclipse.jetty.client.http.HttpReceiverOverHTTP.parse(HttpReceiverOverHTTP.java:221)
	at org.eclipse.jetty.client.http.HttpReceiverOverHTTP.process(HttpReceiverOverHTTP.java:160)
	at org.eclipse.jetty.client.http.HttpReceiverOverHTTP.receive(HttpReceiverOverHTTP.java:91)
	at org.eclipse.jetty.client.http.HttpChannelOverHTTP.receive(HttpChannelOverHTTP.java:97)
	at org.eclipse.jetty.client.http.HttpConnectionOverHTTP.onFillable(HttpConnectionOverHTTP.java:207)
	at org.eclipse.jetty.io.AbstractConnection$ReadCallback.succeeded(AbstractConnection.java:314)
	at org.eclipse.jetty.io.FillInterest.fillable(FillInterest.java:100)
	at org.eclipse.jetty.io.SelectableChannelEndPoint$1.run(SelectableChannelEndPoint.java:53)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.runTask(AdaptiveExecutionStrategy.java:421)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.consumeTask(AdaptiveExecutionStrategy.java:390)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.tryProduce(AdaptiveExecutionStrategy.java:277)
	at org.eclipse.jetty.util.thread.strategy.AdaptiveExecutionStrategy.run(AdaptiveExecutionStrategy.java:199)
	at org.eclipse.jetty.util.thread.ReservedThreadExecutor$ReservedThread.run(ReservedThreadExecutor.java:411)
	at org.eclipse.jetty.util.thread.QueuedThreadPool.runJob(QueuedThreadPool.java:969)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.doRunJob(QueuedThreadPool.java:1194)
	at org.eclipse.jetty.util.thread.QueuedThreadPool$Runner.run(QueuedThreadPool.java:1149)
	at java.base/java.lang.Thread.run(Thread.java:1583)


2024-03-14T05:59:05.912Z	INFO	qtp1976110789-78	io.trino.gateway.ha.handler.QueryIdCachingProxyHandler	Rerouting [http://127.0.0.1:20110/oauth2/callback]--> [http://localhost:21561/oauth2/callback]
2024-03-14T05:59:05.912Z	WARN	qtp1976110789-78	io.trino.gateway.ha.handler.QueryIdCachingProxyHandler	Proxy Target not set on request, unable to decipher HOST header
2024-03-14T05:59:05.919Z	WARN	qtp1976110789-76	io.trino.gateway.ha.router.CookieCacheManager	Error saving cookie node0s8y2ljvxnnxi10fedegmdxxqj0 for backend null: org.javalite.activejdbc.DBException: org.h2.jdbc.JdbcSQLException: Unique index or primary key violation: "PRIMARY_KEY_2 ON PUBLIC.COOKIE_BACKEND_LOOKUP(COOKIE) VALUES ('node0s8y2ljvxnnxi10fedegmdxxqj0', 1)"; SQL statement:
INSERT INTO cookie_backend_lookup (cookie, created_timestamp) VALUES (?, ?) [23505-192], query: INSERT INTO cookie_backend_lookup (cookie, created_timestamp) VALUES (?, ?), params: node0s8y2ljvxnnxi10fedegmdxxqj0, 1710395945917
2024-03-14T05:59:05.944Z	INFO	qtp1976110789-74	io.trino.gateway.ha.handler.QueryIdCachingProxyHandler	Rerouting [http://127.0.0.1:20110/oauth2/callback]--> [http://localhost:32791/oauth2/callback]
2024-03-14T05:59:05.944Z	WARN	qtp1976110789-74	io.trino.gateway.ha.handler.QueryIdCachingProxyHandler	Proxy Target not set on request, unable to decipher HOST header
2024-03-14T05:59:05.998Z	INFO	qtp1976110789-91	io.trino.gateway.ha.handler.QueryIdCachingProxyHandler	Rerouting [http://127.0.0.1:20110/v1/custom/extra]--> [http://localhost:21561/v1/custom/extra]
[INFO] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 18.43 s -- in io.trino.gateway.ha.TestGatewayHaMultipleBackend
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 4, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] Reactor Summary for trino-gateway-parent 7-SNAPSHOT:
[INFO]
[INFO] trino-gateway-parent ............................... SUCCESS [  3.014 s]
[INFO] gateway-ha ......................................... SUCCESS [ 47.937 s]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time:  51.126 s
[INFO] Finished at: 2024-03-14T14:59:07+09:00
[INFO] ------------------------------------------------------------------------

oneonestar · 2024-03-14T06:27:51Z

I don't understand why we have to store the cookie into database.
If the cookie contains enough information, we could decide which backend to go by just looking at it right?

willmostly · 2024-03-14T13:14:59Z

I don't understand why we have to store the cookie into database. If the cookie contains enough information, we could decide which backend to go by just looking at it right?

The cookie only contains a session id, it does not identify the backend. Are you suggesting using a custom cookie instead of the Jetty session cookie?

oneonestar · 2024-03-14T13:32:27Z

The cookie only contains a session id, it does not identify the backend. Are you suggesting using a custom cookie instead of the Jetty session cookie?

Yea. I'm not familiar with HTTP cookie. Could a cookie be trino_routing_cookie=<this_goes_to_backend1>?

willmostly · 2024-03-14T13:47:06Z

Yea. I'm not familiar with HTTP cookie. Could a cookie be trino_routing_cookie=<this_goes_to_backend1>?

It could, the content of a cookie can be anything under 4kB. They also may have attributes such as max-age.

IMO the decision between using Jetty's ootb session management, as I'm doing here, and a custom cookie like GATEWAYSESSIONCOOKIE backend=trino.xyz.com comes down to whether Jetty's built in support offers maintainability advantages that outweight the additional burden on the DB. I could go either way.

willmostly · 2024-04-03T20:28:01Z

            Callback callback)
    {
        try {
-            String requestPath = request.getRequestURI();


this code block was extracted to recordBackendForQueryId

willmostly · 2024-04-03T20:31:41Z

@mosabua I will update the docs once I get confirmation from @oneonestar, @Chaho12 and @vishalya that the new approach is directionally correct

mosabua · 2024-04-03T21:04:19Z

Sounds good

oneonestar · 2024-04-04T04:04:38Z

Excellent! Signed cookie and stateless server are exactly what I thought of.
I'll review it closely later.

oneonestar

We could store the routing info in JWT and then store it in cookie.
Advantages:

Delegate the signature / verification to JWT lib
Able to update the HMAC algorithm in the future (eg. HmacSHA256 -> ECDSASHA256)
Some fields are standardised, can avoid ambiguous (iat = issue at, exp = expiration time)

Chaho12 · 2024-04-12T04:55:03Z

+to determine which backend handled the previous request based solely on
+the request URI and body.
+
+#### Cookie Based Routing Configuration


Please do add examples :)

oneonestar

Looks good. Just a few minor comments.

mosabua

I think this is in a good enough shape now and we should merge. Any objections @oneonestar @vishalya @Chaho12 ?

Chaho12

LGTM~

mosabua · 2024-04-25T15:04:48Z

Merging this. Any further concerns can be addressed in follow up PRs.

cla-bot Bot added the cla-signed label Jan 25, 2024

willmostly mentioned this pull request Feb 2, 2024

Refactor rewriteTarget #234

Merged

willmostly force-pushed the pin_oauth_handshake branch from 62d579a to 4d0854c Compare February 29, 2024 01:44

willmostly requested review from Chaho12 and vishalya February 29, 2024 01:55

willmostly force-pushed the pin_oauth_handshake branch 2 times, most recently from 1d61bb9 to 3e9102a Compare March 5, 2024 23:29

mosabua requested changes Mar 6, 2024

View reviewed changes

mosabua reviewed Mar 7, 2024

View reviewed changes

Comment thread docs/routing-logic.md Outdated

mosabua requested changes Mar 7, 2024

View reviewed changes

Chaho12 reviewed Mar 8, 2024

View reviewed changes

willmostly force-pushed the pin_oauth_handshake branch 3 times, most recently from 45eb2d0 to d9e342c Compare March 13, 2024 03:25

willmostly requested review from Chaho12 and mosabua March 13, 2024 03:27

oneonestar reviewed Mar 13, 2024

View reviewed changes

willmostly force-pushed the pin_oauth_handshake branch from d9e342c to 5a659c3 Compare March 14, 2024 01:27

willmostly requested a review from oneonestar March 14, 2024 01:31

oneonestar reviewed Mar 14, 2024

View reviewed changes

Comment thread gateway-ha/src/main/java/io/trino/gateway/ha/handler/QueryIdCachingProxyHandler.java Outdated

willmostly force-pushed the pin_oauth_handshake branch 2 times, most recently from 8ccc6f8 to 3e67db8 Compare April 3, 2024 20:25

willmostly commented Apr 3, 2024

View reviewed changes

willmostly requested a review from oneonestar April 3, 2024 20:29

oneonestar reviewed Apr 4, 2024

View reviewed changes

Chaho12 reviewed Apr 8, 2024

View reviewed changes

willmostly force-pushed the pin_oauth_handshake branch 4 times, most recently from 6795791 to 84f93c1 Compare April 11, 2024 02:03

Chaho12 reviewed Apr 12, 2024

View reviewed changes

willmostly force-pushed the pin_oauth_handshake branch 2 times, most recently from b32b0dd to 2b31fad Compare April 17, 2024 21:55

oneonestar self-requested a review April 18, 2024 01:57

oneonestar reviewed Apr 18, 2024

View reviewed changes

Comment thread gateway-ha/src/test/java/io/trino/gateway/ha/TestGatewayHaMultipleBackend.java Outdated

willmostly force-pushed the pin_oauth_handshake branch from 2b31fad to 5b1e222 Compare April 19, 2024 02:17

willmostly and others added 2 commits April 18, 2024 22:28

Add cookie based routing for selected paths

725b5dd

Document routing logic and cookie routing configuration

34c9b34

willmostly force-pushed the pin_oauth_handshake branch from 5b1e222 to 34c9b34 Compare April 19, 2024 02:28

mosabua approved these changes Apr 23, 2024

View reviewed changes

Chaho12 approved these changes Apr 24, 2024

View reviewed changes

mosabua merged commit a6ea834 into trinodb:main Apr 25, 2024

github-actions Bot added this to the 8 milestone Apr 25, 2024

mosabua mentioned this pull request May 2, 2024

Add release notes for Trino Gateway 8 #298

Merged

oneonestar mentioned this pull request Jul 4, 2024

OAuth handshake to Web UI fails with multiple backends #165

Closed

Conversation

willmostly commented Jan 25, 2024

Uh oh!

mosabua commented Jan 25, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vishalya commented Feb 28, 2024

Uh oh!

Chaho12 commented Feb 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

willmostly commented Feb 29, 2024

Uh oh!

mosabua left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mosabua commented Mar 7, 2024

Uh oh!

Chaho12 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Chaho12 Mar 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oneonestar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

oneonestar commented Mar 14, 2024

Uh oh!

oneonestar commented Mar 14, 2024

Uh oh!

willmostly commented Mar 14, 2024

Uh oh!

oneonestar commented Mar 14, 2024

Uh oh!

willmostly commented Mar 14, 2024

Uh oh!

willmostly Apr 3, 2024

Choose a reason for hiding this comment

Uh oh!

willmostly commented Apr 3, 2024

Uh oh!

mosabua commented Apr 3, 2024

Uh oh!

oneonestar commented Apr 4, 2024

Uh oh!

mosabua commented Jan 25, 2024 •

edited

Loading

Chaho12 commented Feb 28, 2024 •

edited

Loading

Chaho12 left a comment •

edited

Loading