[KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code #3278

pan3793 · 2022-08-19T09:12:41Z

Why are the changes needed?

This is the followup of #3235, the main change is introdude JdbcUtils to simplify code, and allow empty password for Jdbc auth.

Jdbc connection pool has been removed because JdbcAuthenticationProviderImpl will be created on each connection, we can improve to use singleton in the future

How was this patch tested?

Add some test cases that check the changes thoroughly including negative and positive cases if possible
Add screenshots for manual tests if appropriate
Run test locally before make a pull request

pan3793 · 2022-08-19T09:34:44Z

cc @bowenliang123

bowenliang123 · 2022-08-19T09:48:59Z

Well learned from this pr. It is much resuable and elegant in scala style after refactoring.

turboFei · 2022-08-19T10:34:20Z

JdbcAuthenticationProviderImplSuite:
- authenticate tests *** FAILED ***
  "requirement failed: JDBC driver class is not configured." did not contain "JDBC url is not configured" (JdbcAuthenticationProviderImplSuite.scala:106)

codecov-commenter · 2022-08-20T06:11:50Z

Codecov Report

Merging #3278 (2863cae) into master (97b14f8) will decrease coverage by 0.00%.
The diff coverage is 76.62%.

@@             Coverage Diff              @@
##             master    #3278      +/-   ##
============================================
- Coverage     51.37%   51.37%   -0.01%     
  Complexity       13       13              
============================================
  Files           468      469       +1     
  Lines         26222    26229       +7     
  Branches       3633     3630       -3     
============================================
+ Hits          13472    13474       +2     
- Misses        11467    11469       +2     
- Partials       1283     1286       +3

Impacted Files	Coverage Δ
...uthentication/JdbcAuthenticationProviderImpl.scala	`73.21% <68.88%> (-6.54%)`	⬇️
.../main/scala/org/apache/kyuubi/util/JdbcUtils.scala	`87.50% <87.50%> (ø)`
...apache/kyuubi/engine/JpsApplicationOperation.scala	`77.41% <0.00%> (-3.23%)`	⬇️
...pache/kyuubi/engine/YarnApplicationOperation.scala	`64.81% <0.00%> (-1.86%)`	⬇️
...ain/scala/org/apache/kyuubi/engine/EngineRef.scala	`74.07% <0.00%> (-0.93%)`	⬇️
...uthentication/LdapAuthenticationProviderImpl.scala	`95.23% <0.00%> (-0.15%)`	⬇️
...g/apache/kyuubi/operation/BatchJobSubmission.scala	`74.40% <0.00%> (ø)`
...he/kyuubi/ha/client/etcd/EtcdDiscoveryClient.scala	`68.12% <0.00%> (+0.62%)`	⬆️

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

pan3793 · 2022-08-20T06:13:33Z

@turboFei UT has been enhanced and CI passed now, please take a look again.

bowenliang123 · 2022-08-20T08:30:04Z

@pan3793 May I suggest to change change a config name from kyuubi.authentication.jdbc.username to kyuubi.authentication.jdbc.user ?

kyuubi.authentication.jdbc.username was introduced in my initial code for issue 3222, but I think user is than suitable name than username, sinceDriverManager.getConnection takesuser param in Java and we also have kyuubi.metadata.store.jdbc.user in KuyuubiConf.

Or I could fill anther PR for this very soon to fix this, better for release with 1.6.0.

bowenliang123 · 2022-08-19T10:44:10Z

...src/main/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImpl.scala

-      }
+    if (!query.contains("${username}")) {
+      warn("Query SQL does not contains '${username}' placeholder")
    }


Suggested change

}

}

if (!query.contains("${password}")) {

warn("Query SQL does not contains '${password}' placeholder")

}

How about adding check for ${password} placeholder here, as auth method checked password not blank in first place? @pan3793

added warning message

bowenliang123 · 2022-08-19T10:55:39Z

...src/main/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImpl.scala

        s" or contains blank space")
    }

-    if (StringUtils.isBlank(password)) {


Shall we bring back password not blank checking? If auth method allow passing with missing password, the whole authentication relies on the confidentiality of username alone. @pan3793

I think we should allow empty password, and the auth query will fail the athentication is password is required.

bowenliang123 · 2022-08-19T11:11:55Z

...src/main/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImpl.scala

-        throw new AuthenticationException(s"Password does not match or no such user. user:" +
-          s" $user , password length: ${password.length}")
+      debug(s"prepared auth query: $preparedQuery")
+      JdbcUtils.executeQuery(preparedQuery) { stmt =>


shall we a add a query time out for JdbcUtils.executeQuery to prevent blocking out of connection timeout ?

we can extend KyuubiConf to allow configuring arbitrary properties, but it's out of scope in this PR, better do it in new PR.

kyuubi-common/src/test/scala/org/apache/kyuubi/util/JdbcUtilsSuite.scala

...est/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImplSuite.scala

…uite.scala Co-authored-by: Bowen Liang <[email protected]>

…ication/JdbcAuthenticationProviderImplSuite.scala Co-authored-by: Bowen Liang <[email protected]>

pan3793 · 2022-08-20T09:06:05Z

Or I could fill anther PR for this very soon to fix this, better for release with 1.6.0.

You can send a followup PR if you have time :)

bowenliang123

LGTM.

For the change in kuubi.authentication.jdbc.user config name, i will fill a PR soon after this PR merged.

pan3793 · 2022-08-20T10:22:57Z

Thanks, merging to master/1.6

### _Why are the changes needed?_ This is the followup of #3235, the main change is introdude `JdbcUtils` to simplify code, and allow empty password for Jdbc auth. Jdbc connection pool has been removed because `JdbcAuthenticationProviderImpl` will be created on each connection, we can improve to use singleton in the future ### _How was this patch tested?_ - [ ] Add some test cases that check the changes thoroughly including negative and positive cases if possible - [ ] Add screenshots for manual tests if appropriate - [x] [Run test](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request Closes #3278 from pan3793/jdbc-followup. Closes #3222 2863cae [Cheng Pan] Update kyuubi-common/src/test/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImplSuite.scala 51a9c45 [Cheng Pan] Update kyuubi-common/src/test/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImplSuite.scala eee3c55 [Cheng Pan] Update kyuubi-common/src/test/scala/org/apache/kyuubi/util/JdbcUtilsSuite.scala d02bb99 [Cheng Pan] nit e001b5b [Cheng Pan] nit 8cf5cd6 [Cheng Pan] nit 032f2df [Cheng Pan] nit 8a42f18 [Cheng Pan] nit c7893fd [Cheng Pan] JdbcUtilsSuite f97f2d9 [Cheng Pan] remove pool a8812d0 [Cheng Pan] move render result set to test 83d7d4c [Cheng Pan] fix ut db787a4 [Cheng Pan] nit 864f9dd [Cheng Pan] nit b60decf [Cheng Pan] nit 8c66e0b [Cheng Pan] nit 2063c43 [Cheng Pan] [KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code Authored-by: Cheng Pan <[email protected]> Signed-off-by: Cheng Pan <[email protected]> (cherry picked from commit d0f75e8) Signed-off-by: Cheng Pan <[email protected]>

bowenliang123 · 2022-08-20T14:28:26Z

...src/main/scala/org/apache/kyuubi/service/authentication/JdbcAuthenticationProviderImpl.scala

+      } { resultSet =>
+        if (resultSet == null || !resultSet.next()) {
+          throw new AuthenticationException("Password does not match or no such user. " +
+            s"user: $user, password: $redactedPasswd")


@pan3793 It seems throwing woring redacted password with authdb password. It should throws with connection password. Let me fix it in next PR?

Oh, my fault, thanks for catching this issue. Sure, let's fix it in followup

Good. And we have the fix and ut in #3288 now.

pan3793 added this to the v1.6.0 milestone Aug 19, 2022

pan3793 marked this pull request as ready for review August 19, 2022 09:12

github-actions bot added kind:build module:common labels Aug 19, 2022

pan3793 added 4 commits August 19, 2022 17:43

[KYUUBI apache#3222][FOLLOWUP] Introdude JdbcUtils to simplify code

2063c43

nit

8c66e0b

nit

b60decf

nit

864f9dd

pan3793 force-pushed the jdbc-followup branch from 673c3a4 to 864f9dd Compare August 19, 2022 09:43

nit

db787a4

pan3793 added 3 commits August 19, 2022 18:59

fix ut

83d7d4c

move render result set to test

a8812d0

remove pool

f97f2d9

pan3793 self-assigned this Aug 19, 2022

pan3793 added 5 commits August 19, 2022 20:58

JdbcUtilsSuite

c7893fd

nit

8a42f18

nit

032f2df

nit

8cf5cd6

nit

e001b5b

pan3793 requested a review from turboFei August 20, 2022 06:12

bowenliang123 reviewed Aug 20, 2022

View reviewed changes

pan3793 and others added 3 commits August 20, 2022 16:51

nit

d02bb99

Update kyuubi-common/src/test/scala/org/apache/kyuubi/util/JdbcUtilsS…

eee3c55

…uite.scala Co-authored-by: Bowen Liang <[email protected]>

Update kyuubi-common/src/test/scala/org/apache/kyuubi/service/authent…

51a9c45

…ication/JdbcAuthenticationProviderImplSuite.scala Co-authored-by: Bowen Liang <[email protected]>

Update kyuubi-common/src/test/scala/org/apache/kyuubi/service/authent…

2863cae

…ication/JdbcAuthenticationProviderImplSuite.scala Co-authored-by: Bowen Liang <[email protected]>

bowenliang123 approved these changes Aug 20, 2022

View reviewed changes

pan3793 closed this in d0f75e8 Aug 20, 2022

pan3793 deleted the jdbc-followup branch August 20, 2022 13:16

bowenliang123 reviewed Aug 20, 2022

View reviewed changes

-    }
+    }
+    if (!query.contains("${password}")) {
+      warn("Query SQL does not contains '${password}' placeholder")
+    }

[KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code #3278

[KYUUBI #3222][FOLLOWUP] Introdude JdbcUtils to simplify code #3278

Uh oh!

Conversation

pan3793 commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why are the changes needed?

How was this patch tested?

Uh oh!

pan3793 commented Aug 19, 2022

Uh oh!

bowenliang123 commented Aug 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

turboFei commented Aug 19, 2022

Uh oh!

codecov-commenter commented Aug 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pan3793 commented Aug 20, 2022

Uh oh!

bowenliang123 commented Aug 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan3793 Aug 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pan3793 commented Aug 20, 2022

Uh oh!

bowenliang123 left a comment

Choose a reason for hiding this comment

Uh oh!

pan3793 commented Aug 20, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pan3793 Aug 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

pan3793 commented Aug 19, 2022 •

edited

Loading

bowenliang123 commented Aug 19, 2022 •

edited

Loading

codecov-commenter commented Aug 20, 2022 •

edited

Loading

pan3793 Aug 20, 2022 •

edited

Loading

pan3793 Aug 20, 2022 •

edited

Loading