Skip to content

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Flgado
Copy link
Member

@Flgado Flgado commented May 15, 2025

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:

  • A 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 role map
c := &Config{
    ServerURI: "https://user@localhost:8080",
    Roles: map[string]string{"hive": "analyst", "system": "admin"},
}

using directly the dns connection

dns := ...?roles=catalog1:role1;catalog2:role2

@cla-bot cla-bot bot added the cla-signed label May 15, 2025
@Flgado Flgado requested a review from nineinchnick May 18, 2025 14:05
@@ -390,6 +403,26 @@ func newConn(dsn string) (*Conn, error) {
queryTimeout = &d
}

var formatedRoles string
if rolesStr := query.Get("roles"); rolesStr != "" {
Copy link
Member

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).

Copy link
Member Author

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 !

Copy link
Member

@nineinchnick nineinchnick May 21, 2025

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.

Copy link
Member Author

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},

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

2 participants