Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for path prefix routing using "X-Forwarded-Prefix" header #22227

Merged
merged 2 commits into from
Jul 4, 2024

Conversation

dain
Copy link
Member

@dain dain commented Jun 2, 2024

Description

This change allows the server to support routing with a path prefix,
enabling better compatibility with reverse proxies and load balancers
that use the "X-Forwarded-Prefix" header.

This feature is only enabled when http-server.process-forwarded is
true

NOTE: this does not add support to any of the client implementations, and may not work with any existing clients.

Additional context and related issues

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
(x) Release notes are required, with the following suggested text:

# General
* Add suppot for processing `X-Forwarded-Prefix` header when `http-server.process-forwarded` is enabled. ({issue}`issuenumber`)

@dain dain requested a review from electrum June 2, 2024 01:25
@cla-bot cla-bot bot added the cla-signed label Jun 2, 2024
@dain
Copy link
Member Author

dain commented Jun 2, 2024

Note there are no tests for this. I have no idea how we would test this. I also don't know if any Trino clients support this.

@losipiuk
Copy link
Member

losipiuk commented Jun 3, 2024

NOTE: this does not add support to any of the client implementations, and may not work with any existing clients.

What do we need it for then?

@dain
Copy link
Member Author

dain commented Jun 3, 2024

@losipiuk I was thinking about this this morning, and I think we can do unit tests to verify the correct URL creation. There are 3 areas that I know are affected by this change on the server:

  • Client protocol itself - TestQueryResource could have a test that verifies the url are as expected
  • UI - TestWebUi and TestServer cover x-forwarded already, so we just need to extend this
  • Oauth - Not sure exactally, but there are a number of OauthUI tests
    As the clients are updated (likely a separate PR), we would want tests that cover these. To make life eaier we could have a testing servelet filter that pretends to be a reverse proxy and modifies the stuff in the HTTP servelet request. It isn't hard but is really annoying as the Servlet APIs are hard to work with.

@findepi
Copy link
Member

findepi commented Jun 4, 2024

Does this add support for hosting Trino as eg http://localhost:8080/my-trino (with request going thru a proxy and path rewrite)?

@wendigo
Copy link
Contributor

wendigo commented Jun 5, 2024

@findepi yes. That's how I understand this header as well

@wendigo wendigo mentioned this pull request Jun 5, 2024
The first call is overwritten by the second call
Copy link

github-actions bot commented Jul 4, 2024

This pull request has gone a while without any activity. Tagging the Trino developer relations team: @bitsondatadev @colebow @mosabua

@github-actions github-actions bot added the stale label Jul 4, 2024
This change allows the server to support routing with a path prefix,
enabling better compatibility with reverse proxies and load balancers
that use the "X-Forwarded-Prefix" header.

This feature is only enabled when `http-server.process-forwarded` is
true
@dain dain force-pushed the x-forwarded-prefix branch from 6ac466d to 9fd1342 Compare July 4, 2024 17:18
@mosabua mosabua added stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed. and removed stale labels Jul 4, 2024
@mosabua
Copy link
Member

mosabua commented Jul 4, 2024

Would be great to get this merged and also ideally add some documentation...

@dain dain merged commit 896545f into trinodb:master Jul 4, 2024
94 of 95 checks passed
@dain dain deleted the x-forwarded-prefix branch July 4, 2024 20:22
@dain
Copy link
Member Author

dain commented Jul 4, 2024

@mosabua I'm not sure if we want to promote this until the clients support it.

@github-actions github-actions bot added this to the 452 milestone Jul 4, 2024
@mosabua
Copy link
Member

mosabua commented Jul 4, 2024

@mosabua I'm not sure if we want to promote this until the clients support it.

Hm .. does it not work with clients at all or only in the edge case where catalog and schema are specified in the URL .. I kinda think we should document anyway and just say what does and doesnt work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed stale-ignore Use this label on PRs that should be ignored by the stale bot so they are not flagged or closed.
Development

Successfully merging this pull request may close these issues.

5 participants