Skip to content

Support X-Trino-Role header #322

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.

Sign up for GitHub

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 2 commits into
base: main
Choose a base branch
from
Open

Support X-Trino-Role header #322

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
57c2f9c
Move config.properties under trino folder
ssheikin Nov 7, 2025
91e6b3e
Support `X-Trino-Role` header
ssheikin Nov 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,7 @@ jobs:
--name trino \
--net trino \
--volume "$(pwd)/test-data/trino/test-trino-config.properties:/etc/trino/config.properties" \
--volume "$(pwd)/test-data/trino/catalog/hive.properties:/etc/trino/catalog/hive.properties" \
trinodb/trino:468

echo "Starting Grafana..."
Expand All @@ -97,6 +98,20 @@ jobs:
--volume "$(pwd):/var/lib/grafana/plugins/trino" \
--env "GF_PLUGINS_ALLOW_LOADING_UNSIGNED_PLUGINS=trino-datasource" \
grafana/grafana:11.4.0

echo "Waiting for Trino to be ready..."
while true; do
if docker logs trino 2>&1 | grep -q '======== SERVER STARTED ========'; then
echo "Trino is ready!"
break
fi
echo "Waiting for Trino..."
sleep 5
done

echo "Preconfiguring trino..."
docker exec trino trino --user admin --execute "GRANT admin TO USER grafana IN hive;"
echo "Done."

- name: End to end test
run: |
Expand Down
5 changes: 5 additions & 0 deletions pkg/trino/datasource-context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
const (
accessTokenKey = "accessToken"
trinoUserHeader = "X-Trino-User"
trinoRoleHeader = "X-Trino-Role"
trinoClientTagsKey = "X-Trino-Client-Tags"
bearerPrefix = "Bearer "
)
Expand Down Expand Up @@ -41,6 +42,10 @@ func (ds *SQLDatasourceWithTrinoUserContext) QueryData(ctx context.Context, req
ctx = context.WithValue(ctx, trinoUserHeader, user)
}

if settings.Role != "" {
ctx = context.WithValue(ctx, trinoRoleHeader, "system=ROLE{"+settings.Role+"}")
Copy link
Member

@nineinchnick nineinchnick Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note there's an open PR to support roles directly in the Go driver: trinodb/trino-go-client#144

It's not required here, but we had a discussion there about the format in which to specify roles. We tried being consistent with other drivers, like JDBC and Python.

Copy link
Member

@kokosing kokosing Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. What is the golden standard here we should follow? https://trino.io/docs/current/client/jdbc.html ?

Authorization roles to use for catalogs, specified as a list of key-value pairs for the catalog and role. For example, catalog1:roleA;catalog2:roleB sets roleA for catalog1 and roleB for catalog2.

That makes sense. As I understand for system role we simply say system:sysadmin. @ssheikin that sounds like good way to expose support to all kind of roles and makes it easy to test.

Copy link
Author

@ssheikin ssheikin Nov 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nineinchnick @kokosing
This was my initial approach. In order to reduce scope and moving parts the PR was simplified.
The current PR allows to specify system role in a very simple format.
An improvement to set roles for catalogs in the jdbc compliant format may follow up.
The code for http client header is identical to #309

Copy link
Member

@kokosing kokosing Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was my initial approach.

Hm... I was under impression that initial approach was similar, but a bit different than in JDBC. That your approach followed the protocol implementation, not the JDBC format.

An improvement to set roles for catalogs in the jdbc compliant format may follow up.
I think this is part of the protocol (UX), changing semantics of the field seems like a breaking change. If it is not a big deal I would prefer to do it once and don't come if not needed.

Copy link
Member

@nineinchnick nineinchnick Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do it right and be consistent with other drivers.

Also note the releases are blocked until either #303 or #321 is merged.

Copy link
Member

@Flgado Flgado Nov 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I left a comment here but maybe is better to add it here.
This PR is almost ready to be merged, roles will be supported through DSN and also in this case as map passed on the context.

}

if settings.ClientTags != "" {
ctx = context.WithValue(ctx, trinoClientTagsKey, settings.ClientTags)
}
Expand Down
5 changes: 5 additions & 0 deletions pkg/trino/datasource.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ func (s *TrinoDatasource) SetQueryArgs(ctx context.Context, headers http.Header)

user := ctx.Value(trinoUserHeader)
accessToken := ctx.Value(accessTokenKey)
role := ctx.Value(trinoRoleHeader)
clientTags := ctx.Value(trinoClientTagsKey)

if user != nil {
Expand All @@ -93,6 +94,10 @@ func (s *TrinoDatasource) SetQueryArgs(ctx context.Context, headers http.Header)
args = append(args, sql.Named(accessTokenKey, accessToken.(string)))
}

if role != nil {
args = append(args, sql.Named(trinoRoleHeader, role.(string)))
}

if clientTags != nil {
args = append(args, sql.Named(trinoClientTagsKey, clientTags.(string)))
}
Expand Down
1 change: 1 addition & 0 deletions pkg/trino/models/settings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ type TrinoDatasourceSettings struct {
ClientId string `json:"clientId"`
ClientSecret string `json:"clientSecret"`
ImpersonationUser string `json:"impersonationUser"`
Role string `json:"role"`
ClientTags string `json:"clientTags"`
}

Expand Down
16 changes: 16 additions & 0 deletions src/ConfigEditor.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ export class ConfigEditor extends PureComponent<Props, State> {
const onImpersonationUserChange = (event: ChangeEvent<HTMLInputElement>) => {
onOptionsChange({...options, jsonData: {...options.jsonData, impersonationUser: event.target.value}})
};
const onRoleChange = (event: ChangeEvent<HTMLInputElement>) => {
onOptionsChange({...options, jsonData: {...options.jsonData, role: event.target.value}})
};
const onClientTagsChange = (event: ChangeEvent<HTMLInputElement>) => {
onOptionsChange({...options, jsonData: {...options.jsonData, clientTags: event.target.value}})
};
Expand Down Expand Up @@ -75,6 +78,19 @@ export class ConfigEditor extends PureComponent<Props, State> {
/>
</InlineField>
</div>
<div className="gf-form-inline">
<InlineField
label="Role"
tooltip="The role to be used connecting to Trino"
labelWidth={26}
>
<Input
value={options.jsonData?.role ?? ''}
onChange={onRoleChange}
width={40}
/>
</InlineField>
</div>
<div className="gf-form-inline">
<InlineField
label="Client Tags"
Expand Down
37 changes: 37 additions & 0 deletions src/e2e.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,12 @@ async function setupDataSourceWithClientTags(page: Page, clientTags: string) {
await page.getByTestId('data-testid Data source settings page Save and Test button').click();
}

async function setupDataSourceWithRole(page: Page, role: string) {
await page.getByTestId('data-testid Datasource HTTP settings url').fill('http://trino:8080');
await page.locator('div').filter({hasText: /^Role$/}).locator('input').fill(role);
await page.getByTestId('data-testid Data source settings page Save and Test button').click();
}

async function runQueryAndCheckResults(page: Page) {
await page.getByLabel(EXPORT_DATA).click();
await page.getByTestId('data-testid TimePicker Open Button').click();
Expand Down Expand Up @@ -91,3 +97,34 @@ test('test with client tags', async ({ page }) => {
await setupDataSourceWithClientTags(page, 'tag1,tag2,tag3');
await runQueryAndCheckResults(page);
});

test('test with role', async ({ page }) => {
await login(page);
await goToTrinoSettings(page);
await setupDataSourceWithRole(page, '},system=ALL,hive=ROLE{admin');
Copy link
Member

@kokosing kokosing Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how you can set a role like this?

await runRoleQuery(page);
await expect(page.getByTestId('data-testid table body')).toContainText(/.*admin.*/);

});

test('test without role', async ({ page }) => {
await login(page);
await goToTrinoSettings(page);
await setupDataSourceWithRole(page, '');
await runRoleQuery(page);
await expect(page.getByText(/Access Denied: Cannot show roles/)).toBeVisible();
});

async function runRoleQuery(page: Page) {
Copy link
Member

@kokosing kokosing Nov 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why utility functions are on top and why some are on the bottom?

Copy link
Author

@ssheikin ssheikin Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The good practice, at least in trino java code to put utility functions on the bottom.
I'd put setupDataSourceWithRole on the bottom too, but very similar functions were on the top, so it looks there more natural.
Please let me know if you have an idea how to improve that.

Copy link
Member

@nineinchnick nineinchnick Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can reorder functions in this file in a separate commit

Copy link
Author

@ssheikin ssheikin Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's not for free - the git history of the file will be overriden.

await page.getByLabel(EXPORT_DATA).click();
await page.locator('div').filter({hasText: /^Format asChoose$/}).locator('svg').click();
await page.getByRole('option', {name: 'Table'}).click();
await setQuery(page, 'SHOW ROLES FROM hive')
await page.getByTestId('data-testid Code editor container').click();
await page.getByTestId('data-testid RefreshPicker run button').click();
}

async function setQuery(page: Page, query: string) {
await page.getByTestId('data-testid Code editor container').click({ clickCount: 4 });
await page.keyboard.type(query);
}
1 change: 1 addition & 0 deletions src/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ export interface TrinoDataSourceOptions extends DataSourceJsonData {
tokenUrl?: string;
clientId?: string;
impersonationUser?: string;
role?: string;
clientTags?: string;
}
/**
Expand Down
5 changes: 5 additions & 0 deletions test-data/trino/catalog/hive.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
connector.name=hive
hive.metastore=file
hive.metastore.catalog.dir=/tmp/metastore
hive.security=sql-standard
fs.hadoop.enabled=true