apache / kyuubi

Apache Kyuubi is a distributed and multi-tenant gateway to provide serverless SQL on data warehouses and lakehouses.
https://kyuubi.apache.org/
Apache License 2.0
2.11k stars 915 forks source link

[KYUUBI #6689] Extract kyuubiClientPrincipal/kyuubiClientKeytab/kyuubiServerPrincipal/principal from JDBC connection properties #6693

Open Madhukar525722 opened 2 months ago

Madhukar525722 commented 2 months ago

โ€ฆab from JDBC connection properties

:mag: Description

Issue References ๐Ÿ”—

This pull request fixes #6689

Describe Your Solution ๐Ÿ”ง

It extracts the values of AUTH_KYUUBI_CLIENT_PRINCIPAL, AUTH_KYUUBI_CLIENT_KEYTAB, AUTH_KYUUBI_SERVER_PRINCIPAL and AUTH_PRINCIPAL, then passes it to connParams , to be used for establishing connections.

Types of changes :bookmark:

Test Plan ๐Ÿงช

Behavior Without This Pull Request :coffin:

Behavior With This Pull Request :tada:

Related Unit Tests


Checklist ๐Ÿ“

Be nice. Be informative.

codecov-commenter commented 2 months ago

Codecov Report

Attention: Patch coverage is 0% with 18 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (550f1fc) to head (062cd08). Report is 53 commits behind head on master.

Files with missing lines Patch % Lines
...c/main/java/org/apache/kyuubi/jdbc/hive/Utils.java 0.00% 18 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #6693 +/- ## ======================================= Coverage 0.00% 0.00% ======================================= Files 684 687 +3 Lines 42237 42457 +220 Branches 5755 5801 +46 ======================================= - Misses 42237 42457 +220 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

wForget commented 2 months ago

and cc @pan3793

Madhukar525722 commented 2 months ago

Hi @pan3793 , Please review the change

pan3793 commented 2 months ago

what about kyuubiServerPrincipal and pricipal?

Madhukar525722 commented 1 month ago

Hi @pan3793, Actually the need for kyuubiServerPrincipal was not mentioned in the issue. I will be adding it as well, soon. cc @wForget

wForget commented 1 month ago

what about kyuubiServerPrincipal and pricipal?

My original intention was to pass in the client's authentication information in properties. kyuubiServerPrincipal can be provided by zk and we don't need to get it here.

pan3793 commented 1 month ago

kyuubiServerPrincipal can be provided by zk

this is disabled by default, because the lower Hive client does not support parsing that.

It would be weird if kyuubiClientPrincipal works but kyuubiServerPrincipal does not. Let's keep them synced to avoid surprising users.

Madhukar525722 commented 1 month ago

Hi @pan3793 , I have updated the required. Please review

Madhukar525722 commented 3 weeks ago

HI @pan3793, Gentle ping

pan3793 commented 3 weeks ago

LGTM, thank you, @Madhukar525722, please update the PR title and desc to match the code

Madhukar525722 commented 2 weeks ago

HI @pan3793, I have changed as suggested.