-
Notifications
You must be signed in to change notification settings - Fork 67
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 option for extra whitelisted paths #63
Conversation
Needs docs on how to use this at least. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we expecting any paths to be whitelisted in near future?
@@ -218,7 +219,8 @@ private boolean isPathWhiteListed(String path) { | |||
|| path.startsWith(V1_INFO_PATH) | |||
|| path.startsWith(V1_NODE_PATH) | |||
|| path.startsWith(UI_API_STATS_PATH) | |||
|| path.startsWith(OAUTH_PATH); | |||
|| path.startsWith(OAUTH_PATH) | |||
|| extraWhitelistPaths.stream().anyMatch(s -> path.startsWith(s)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To make this code look cleaner, I am wondering if all the paths could be derived from a config list.
We can provide a default (hardcoded initialization) list for the existing paths and then append the extraWhitelistPaths
Also, how much overhead is stream
going to add? This should be a really performance code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have not looked at the code but... will this only load at start up time of the server.. then performance wont matter.. also wont matter if the reload is done on demand via a trigger in the UI or a REST endpoint .. I would prefer these two methods over reloading automatically all the time. That would only be okay if we know there is basically no performance impact.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code get's called for every request sent to the cluster and hence the comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Java should evaluate chained ||
operators lazily, so as soon as any of these conditions are true it will skip the rest. Because of that, I think it is a good idea to retain the current structure and order them so that the most latency sensitive paths such as v1/statement
are first in the list, even if it is a little ugly.
Are we expecting any paths to be whitelisted in near future?
This is useful for commercial extensions to Trino such as Starburst. Starburst has extra endpoints for its features such as data products and the access control API.
Lastly, we could clean up this list. At least /ui/api/stats
should probably be removed. If someone is using it (it is an internal endpoint, not intended to be exposed but who knows...) they could add it back in through the new config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can do that cleanup later
Yes ... there are custom paths that need to be whitelisted for Starburst products and other custom deployments where Trino is embedded with more plugins and setups. It also potentially allows you to proxy metrics and whatever else you want to expose on additional end points on the Trino coordinator and workers. Furthermore we will work towards a new client protocol for Trino .. and can use this feature to support whatever endpoints it will surface on .. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a few minor edits and resolving the conflict needed. Then this should be good.
docs/configuration.md
Outdated
Each component of the Trino Gateway will have a corresponding node in the configuration yaml. | ||
|
||
## Proxying additional paths | ||
By default, Trino Gateway will only proxy requests to paths starting with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, Trino Gateway will only proxy requests to paths starting with | |
By default, Trino Gateway only proxies requests to paths starting with |
docs/configuration.md
Outdated
## Proxying additional paths | ||
By default, Trino Gateway will only proxy requests to paths starting with | ||
`/v1/statement`, `/v1/query`, `/ui`, `/v1/info`, `/v1/node`, | ||
`/ui/api/stats` and `/oauth`. If you would like to proxy additional paths, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
`/ui/api/stats` and `/oauth`. If you would like to proxy additional paths, | |
`/ui/api/stats` and `/oauth`. | |
If you want to proxy additional paths, |
docs/configuration.md
Outdated
`/v1/statement`, `/v1/query`, `/ui`, `/v1/info`, `/v1/node`, | ||
`/ui/api/stats` and `/oauth`. If you would like to proxy additional paths, | ||
you can add them by adding the `extraWhitelistPaths` node to your gateway | ||
configuration yaml. For example adding |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
configuration yaml. For example adding | |
configuration yaml: |
docs/configuration.md
Outdated
- "/ext/faster" | ||
``` | ||
|
||
would allow requests to paths starting with any of the above to be forwarded |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example enables additional proxying of any requests to path starting with the specified paths.
cd2ce7a
to
ac633f2
Compare
ac633f2
to
c2a4c5d
Compare
Thank you .. looks great now. |
This submission allows configuring extra paths to proxy without a code change. This is useful for cases in which Trino has been extended to add extra APIs, and cases in which access to one of the paths not whitelisted by default is desired.