-
Notifications
You must be signed in to change notification settings - Fork 886
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
feat: use native trino client authentication classes #16196
base: main
Are you sure you want to change the base?
Conversation
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
5 similar comments
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
.../src/main/resources/json/schema/entity/services/connections/database/common/azureConfig.json
Outdated
Show resolved
Hide resolved
elif ( | ||
connection.authType | ||
== noConfigAuthenticationTypes.NoConfigAuthenticationTypes.OAuth2 | ||
): | ||
connection.connectionArguments.__root__["auth"] = OAuth2Authentication() | ||
connection.connectionArguments.__root__["http_scheme"] = "https" | ||
|
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.
@mgorsk1 correct me if I'm wrong, but this method would only work if you are running a trino metadata job on your local machine via CLI mode where you have access to a browser, as OAuth2Authentication
would print a link or redirect you to a the same link where you can complete the authentication
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.
that's correct, I added it because that's the scenario we needed and was not covered. The example scenario is:
- trino configured with oauth2
- trino npa used to fetch metadata from whole cluster (ingestion run from om airflow)
- users run profiling jobs using OM SDK from their private environments
- user credentials (collected from oauth2 flow) are used to run profiling jobs only on tables they have access to (we don't want users to run ad-hoc profiling jobs using strong trino npa)
- profiling jobs takes time so we need a mechanism that will collect access_token but also refresh it - OAuth2Authentication handles this scenario while regular JWT doesn't
Hi there 👋 Thanks for your contribution! The OpenMetadata team will review the PR shortly! Once it has been labeled as Let us know if you need any help! |
af94ac9
to
902c7be
Compare
902c7be
to
640ce5a
Compare
Quality Gate passed for 'open-metadata-ingestion'Issues Measures |
Describe your changes:
Fixes
Since Trino client used in trino integration provides classes for different types of authentication (and includes both basic and jwt) classes I worked on refactoring trino/connection.py class to use them directly instead of workarounds with adjusting the connection url because it enables us to streamline inclusion of different authentication mechanisms and also enables us to use
Oauth2Authentication
class in a same way, which can be handy for some use cases. It will also make it super easy to extend in the future with other supported authentication mechanisms such as certificate or kerberos authentication.Type of change:
Checklist:
Fixes <issue-number>: <short explanation>