-
Notifications
You must be signed in to change notification settings - Fork 73
Add support for catalog roles #144
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
base: master
Are you sure you want to change the base?
Conversation
@@ -390,6 +403,26 @@ func newConn(dsn string) (*Conn, error) { | |||
queryTimeout = &d | |||
} | |||
|
|||
var formatedRoles string | |||
if rolesStr := query.Get("roles"); rolesStr != "" { |
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.
Why would we allow overriding roles in query parameters, instead of setting them for the whole connection? I think this should be only possible using SQL statements, and we actually should handle the X-Set-Role
header coming back from the server (and remove it from unsupportedResponseHeaders
).
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.
Yes, if we want to support it via DNS and use the JDBC format, we’ll need to find a way to convert it to the correct format. So, are you suggesting it should only be sent as an SQL statement? I’ll make the changes. 👍🏻
I also going to take a look to X-Set-Role
thank you !
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.
Sorry, I got confused with the query
variable - I thought it's for the user's SQL query, not the query part of the DSN URL. The check for =ROLE{
is also weird, this would mean we're adding support for some undocumented legacy format.
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.
AAAh I know what I have done wrong !
When I use the config and call the formatDNS
function to setup the dns I am putting the right format already! And is not suppose
So here I am validation if is not on the right format already.
I will change it, I will convert it always to convert to jdbc format and then when it reach to this point I will convert it to the format that trino is waiting for: catalog=ROLE{role1},catalog=ROLE{rol2},
This PR adds support for specifying catalog roles in the Trino Go client via both the DSN string and the
Config
struct. Roles are passed as either:map[string]string
mapping catalogs to roles (e.g.{"hive": "analyst", "system": "admin"}
)The roles are URL-encoded and appended to the DSN as a colon-separated list of
catalog=ROLE{role}
entries.Example
using directly the dns connection