-
Notifications
You must be signed in to change notification settings - Fork 121
Add forward proto header configuration for cluster monitoring #729
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 forward proto header configuration for cluster monitoring #729
Conversation
2be12cb to
5b715e9
Compare
Chaho12
left a 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.
@andythsu Can you check this?
gateway-ha/src/main/java/io/trino/gateway/ha/config/BackendStateConfiguration.java
Outdated
Show resolved
Hide resolved
gateway-ha/src/main/java/io/trino/gateway/ha/clustermonitor/ClusterStatsHttpMonitor.java
Outdated
Show resolved
Hide resolved
f541a47 to
c52f01d
Compare
vishalya
left a 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.
LGTM
|
On a second thought, is this PR necessary? We are currently using |
|
@andythsu https://trino.io/docs/current/security/tls.html#use-a-load-balancer-to-terminate-tls-https has more details, but when a Trino cluster is behind a LoadBalancer/Gateway it usually just doesn't accept https connections ( and only accepts non-secure http connections) . I think in your case, both Gateway and Trino clusters maybe accepting https connections, but that maynot always be the case. This is because the Trino clusters don't have a certificate which is globally trusted, only the LoadBalancer/Gateway is mounted with a trusted certificate. |
|
We could also consider adding this header by default for all cluster health http calls. For the /v1/statement endpoints this header is added by default here ( unless users specifically set routing.addXForwardedHeaders to false ). |
|
Hello all, is this good to merge now ? Thanks! |
gateway-ha/src/main/java/io/trino/gateway/ha/config/BackendStateConfiguration.java
Outdated
Show resolved
Hide resolved
c52f01d to
542fa24
Compare
542fa24 to
1b5ce30
Compare
Description
As documented at https://trino.io/docs/current/security/tls.html#use-a-load-balancer-to-terminate-tls-https, when Trino is behind a loadbalancer or proxy like Trino Gateway, it's common that the TLS is terminated at Trino Gateway.
Correspondingly when Trino Gateway forwards the request to Trino clusters, Gateway adds X-Forwarded-* headers in code here. Relevant documentation is here where users can optionally disable this by setting routing.addXForwardedHeaders to false.
This MR is to add the same Header while making health check calls to get cluster stats like queued queries or running queries. Since it's possible that TLS is terminated at Gateway, a similar header would be required when making the http calls to fetch the cluster stats, for example using the /metrics or /v1/jmx/mbean endpoints
If such a header isn't added, the http call to fetch metrics would fail with an error like:
Additional context and related issues
Release notes
( ) This is not user-visible or is docs only, and no release notes are required.
(x) Release notes are required, with the following suggested text: