Skip to content
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

Jump to bottom

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

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

wants to merge 1 commit into from
+30 −0

Conversation

Madhukar525722
Copy link
Contributor

@Madhukar525722 Madhukar525722 commented Sep 12, 2024 •
edited
Loading

…ab from JDBC connection properties

🔍 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 🔖

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Test Plan 🧪

Behavior Without This Pull Request ⚰️

Behavior With This Pull Request 🎉

Related Unit Tests

Checklist 📝

Be nice. Be informative.

@codecov-commenter
Copy link

codecov-commenter commented Sep 12, 2024 •
edited
Loading

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 ⚠️
Additional details and impacted files 
@@           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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@wForget wForget changed the title KYUUBI #6689 [FEATURE] Extract kyuubiClientPrincipal/kyuubiClientKeyt… [KYUUBI #6689] Extract kyuubiClientPrincipal/kyuubiClientKeytab from JDBC connection properties Sep 13, 2024
wForget
wForget approved these changes Sep 13, 2024
View reviewed changes
Copy link
Member

@wForget wForget left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM

@wForget
Copy link
Member

wForget commented Sep 13, 2024

and cc @pan3793

@Madhukar525722
Copy link
Contributor Author

Hi @pan3793 , Please review the change

@pan3793
Copy link
Member

pan3793 commented Sep 23, 2024

what about kyuubiServerPrincipal and pricipal?

@Madhukar525722
Copy link
Contributor Author

Madhukar525722 commented Sep 23, 2024 •
edited
Loading

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

@wForget
Copy link
Member

wForget commented Sep 26, 2024

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
Copy link
Member

pan3793 commented Sep 29, 2024

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
Copy link
Contributor Author

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

wForget
wForget approved these changes Oct 23, 2024
View reviewed changes
Copy link
Member

@wForget wForget left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@Madhukar525722
Copy link
Contributor Author

HI @pan3793, Gentle ping

@pan3793
Copy link
Member

pan3793 commented Oct 29, 2024

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

@Madhukar525722 Madhukar525722 changed the title [KYUUBI #6689] Extract kyuubiClientPrincipal/kyuubiClientKeytab from JDBC connection properties [KYUUBI #6689] Extract kyuubiClientPrincipal/kyuubiClientKeytab/kyuubiServerPrincipal/principal from JDBC connection properties Oct 29, 2024
@Madhukar525722
KYUUBI apache#6689 [FEATURE] Extract kyuubiClientPrincipal/kyuubiClie…
062cd08 
…ntKeytab/kyuubiServerPrincipal/Principal from JDBC connection properties
@Madhukar525722
Copy link
Contributor Author

HI @pan3793, I have changed as suggested.

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

@wForget wForget wForget approved these changes

Assignees
No one assigned
Labels
module:ctl module:hive
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE] Extract kyuubiClientPrincipal/kyuubiClientKeytab from JDBC connection properties
4 participants
@Madhukar525722 @codecov-commenter @wForget @pan3793