Add support for --extra-header to Trino CLI and JDBC

[Pluies]

Description

Adds an --extra-header flag to the Trino CLI to allow for passing arbitrary HTTP headers to Trino requests.

Additional context and related issues

This can be useful for all sort of header-y things, including passing the X-Trino-Routing-Group header for the presto-gateway, or adding specific values needed by other fanciful architectures. :)

Release notes

( ) This is not user-visible or 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:

# Section
* Add a `--extra-header` argument to the trino-cli to support sending arbitrary HTTP headers to Trino 

[Pluies]

cc @electrum @hashhar , this is a more general rework of #15737, let me know your thoughts! 🙏

[nineinchnick]

Is there an equivalent of this in the JDBC driver?

[Pluies]

Is there an equivalent of this in the JDBC driver?

That's a good question - from a look at the docs, it appears not... At least not in Trino. Presto's JDBC client does support a customHeaders parameter since August 2021 (prestodb/presto@a6f1d0f), but Trino doesn't.

This was discussed in #3179 but not implemented at the time, as user impersonation was implemented with explicit headers and covered the use-case of the original reporter.

We're not using JDBC here so we only need this feature for the Trino CLI, but having support for extra headers in the code makes implementing this for JDBC easier.

[nineinchnick]

I have mixed feelings about adding this if it's not available in the JDBC driver. Let's wait for other reviewers to comment on this.

[dain]

I would be surprised if this get approved. Trino generally does not allow flags to arbitrarily change core infrastructure like this. When we add features we like to start from the core problem and come up with a specific solution for that problem.

[msf]

@dain our usecase is we use presto-gateway to load-balance and front multiple trino clusters. we divide them into groups and would like to be able to use the trino-cli to issue queries to a specific group of clusters.

I understand presto-gateway isn't core trino, but it is a fairly used system and more importantly this design pattern of having a gateway/load-balancer to allow for blue/green or rolling deployments of trino without causing downtime is necessary for us (and many others I'd imagine).

We'd like all our tooling that issues requests to trino (including the trino-cli) to use this gateway and to be able to select a specific group of clusters.

how would you suggest this be approached? should we modify presto-gateway to use a specific existing HTTP API client http header ? the only i see that is somewhat a candidate is X-Trino-Client-Tags and even that.. feels like overloading it for a different purpose

[Pluies]

Especially as Trino's own website lists the presto-gateway in its list of resources, so it would make sense (imho!) to be able to support it from the official CLI 🙏

[mosabua]

Docs need an example

[rwilliams-r7]

Is this PR dead or will it make it into the code base?

[mosabua]

Is this PR dead or will it make it into the code base?

Currently that depends on @Pluies to update and resolve the conflicts. Subsequently it can be reviewed again.

[Pluies]

@mosabua thanks for the ping, this was flying under my radar - updating now!

[Pluies]

PR updated and ready for re-review 👍

@mosabua , there's an example in docs/src/main/sphinx/client/cli.md, do you have another thing in mind for documentation?

[mosabua]

@electrum @dain @nineinchnick - can we decide on a direction for this. Does it maybe fit in given that we are working towards trino-gateway ?

[nineinchnick]

@msf could you suggest a name for a new header to be used for routing?

[msf]

@msf could you suggest a name for a new header to be used for routing?

I would suggest X-Trino-Routing-Group, like mentioned above, which is already used by: https://github.com/trinodb/trino-gateway and other open source projects: https://github.com/search?q=X-Trino-Routing-Group&type=code

[Chaho12]

FYI, I just tested the approach of using X-Trino-Client-Tags with trino-gateway which works brilliantly so either way works :)
Gateway provides routing-rules engine option for those circumstance where it is hard to add X-Trino-Routing-Group request header (JDBC, Trino CLI etc.)

• By default, Trino Gateway reads the X-Trino-Routing-Group request header to route requests. If this header is not specified, requests are sent to default routing group (adhoc).
• https://github.com/trinodb/trino-gateway/blob/main/docs/routing-rules.md

[patrickdemers6]

I opened a similar change in trinodb/trino-go-client#91 and was pointed to this PR.

Depending on the environment and other systems at play, custom headers are a go-to way for passing information between systems. I have use cases for passing an Authorization header with each request, as well as other random headers such as transaction ID and trace ID. I would love to see this happen as it opens many doors for developers with little downside.

[github-actions]

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

[mosabua]

👋 @Pluies @electrum - this PR has become inactive. We hope you are still interested in working on it. Please let us know, and we can try to get reviewers to help with that.

We're working on closing out old and inactive PRs, so if you're too busy or this has too many merge conflicts to be worth picking back up, we'll be making another pass to close it out in a few weeks.

[dprophet]

Interesting. Yet another feature I need. @mosabua

[wendigo]

@Pluies can you rebase this PR? I'll take a look :) I've refactored a lot of things around property handling to make these consistent across CLI/JDBC/Client so this will make sure that once you've add extra headers it will be supported there.

[mosabua]

Haha @dprophet .. I know .. we will want some sort of generic bucket for more stuff in the protocol. This is in great hands with @wendigo and @Pluies, but please do let us know your use cases so we can take them into account and help with reviews (or telling others to review ;-))

[mosabua]

go client and python client already support custom headers, this Pr just catches up the CLI for that. Could @wendigo and @electrum please look at this in general and if this is ok .. can @Pluies rebase?

[mosabua]

See https://github.com/trinodb/trino-go-client/blob/master/trino/trino.go#L289 for go client

[mosabua]

Maybe we also have to add this to the JDBC driver

[Chaho12]

oops manfred, I shared wrong line. here is the actual logic that checks if header args is passed on and starts with X-Trino, to save it to the http.Header.
https://github.com/trinodb/trino-go-client/blob/master/trino/trino.go#L934-L941

e.g. rows, err := db.Query("SELECT current_user", sql.Named("X-Trino-User", string("TestUser"))) would set X-Trino-User header with value of 'TestUser'

• https://github.com/trinodb/trino-go-client/blob/master/trino/trino_test.go#L398

so here we can set `sql.Named('X-Trino-Routing-Group', 'groupLarge') to set trino routing group large

[mosabua]

Thank you @oneonestar .. I think we should figure out how we make this behavior consistent across all the clients .. at least the ones from the trinodb org .. so cli, jdbc, go, js, and python at this stage.

I pinged @wendigo @martint and @electrum so we can discuss as needed and move this forward.

[Pluies]

Sorry I won't have bandwidth to rebase this until mid-December at the least, feel free to take over the PR or close this one and open another more recent one 🙏

Stale

[wendigo]

Rebased and resolved conflicts