Skip to content

Conversation

@SteNicholas
Copy link
Member

@SteNicholas SteNicholas commented Feb 22, 2022

Why are the changes needed?

Upgrade libthrift to 0.16.0 due to CVE-2020-13949 and the coming upstream change of Spark apache/spark#34362

What changed in this PR?

  • Upgrade libthrift to 0.16.0

  • Shade and relocate thrift and hive-service-rpc classes in kyuubi-spark-engine, it's necessary to avoide conflicting with old thrift libs bundled in Spark binary releases.

  • Due to thrift change the method signature, the subclasses those interfaces in Kyuubi also need to modify to pass compile.
    We rely on Hive 2.3.9 jars in some components, e.g. kyuubi-hive-jdbc, LocalMetaServer in kyuubi-server test classes.

Some classes in Hive jars compiled against old thrift interfaces which are not compatible with thrift 0.16.0, it causes runtime link error, we found the following classes which breaks the test and copied them with necessary modification to make it work with thrift 0.16.0.

- `TFramedTransport`
- `TFilterTransport`
- `TUGIAssumingTransport`
- `TUGIContainingTransport`
  • Next Steps, I think it's worth to do in separated PRs.

    • Recover the HiveDelegationTokenProviderSuite, one approach is use an isolate classloader to load HMS classes and thrift 0.9.3 classes from Maven, this approach can also be used for the planed Zoopkeeper upgrading to help us verficating the compatibility of Zookeeper Server 3.4.x.
    • Rewrite kyuubi-hive-jdbc to make it decouple with Hive jars, because there maybe other places which may not work with thrift 0.16.0 but the UTs does not cover.

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

pan3793 commented Feb 22, 2022

We need to shade thrift libs into kyuubi-spark-sql-engine because Spark bundles old thrift libs in binary release.

@SteNicholas SteNicholas force-pushed the KYUUBI-1948 branch 3 times, most recently from 47696f7 to 40ffacc Compare February 22, 2022 04:03
@SteNicholas SteNicholas requested a review from pan3793 February 22, 2022 04:05
@SteNicholas SteNicholas requested a review from pan3793 February 22, 2022 05:11
@SteNicholas SteNicholas force-pushed the KYUUBI-1948 branch 6 times, most recently from 4300576 to 12ef0c1 Compare February 23, 2022 06:09

test("obtain hive delegation token") {
// Ignore the test because LocalMetaServer can not work with Thrift 0.16.0.
ignore("obtain hive delegation token") {
Copy link
Member

Choose a reason for hiding this comment

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

cc @zhouyifan279
We can not start HiveMetastore using thrift 0.16.0, because the Interface method signature changed, the class is not compatible.

Copy link
Member

Choose a reason for hiding this comment

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

Shall we govern these ignored tests with flaky tests together? cc @yanghua

Copy link
Contributor

Choose a reason for hiding this comment

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

I will create a PR later to fix it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Shall we govern these ignored tests with flaky tests together? cc @yanghua

I suggest we split them into two categories. The ignored tests may have other reasons not only flaky tests?

@pan3793
Copy link
Member

pan3793 commented Feb 23, 2022

Summary of this PR,

  1. Upgrade libthrift to 0.16.0 due to CVE issues and the coming upstream change of Spark
  2. Shade and relocate thrift and hive-service-rpc classes in kyuubi-spark-engine, it's necessary to avoding conflict with old thrift libs bundled in Spark binary releases.
  3. Due to thrift change the method signature, the subclasses of Kyuubi also need to change to pass compile.
  4. We rely on Hive 2.3.9 jars in some components, e.g. kyuubi-hive-jdbc, LocalMetaServer in kyuubi-server test classes. Some classes in Hive jars compiled against old thrift interfaces which are not compatible with thrift 0.16.0, it causes runtime link error, we found all broken tests and copy the broken classes with necessary modification to make it work with thrift 0.16.0, but there may still risk somewhere the UT not covered.

@SteNicholas SteNicholas requested a review from pan3793 February 23, 2022 08:20
@codecov-commenter
Copy link

Codecov Report

Merging #1953 (898effc) into master (f25e5c9) will decrease coverage by 0.53%.
The diff coverage is 1.49%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #1953      +/-   ##
============================================
- Coverage     60.89%   60.35%   -0.54%     
  Complexity       69       69              
============================================
  Files           302      305       +3     
  Lines         14797    14859      +62     
  Branches       1915     1927      +12     
============================================
- Hits           9010     8968      -42     
- Misses         5020     5111      +91     
- Partials        767      780      +13     
Impacted Files Coverage Δ
...in/scala/org/apache/kyuubi/config/KyuubiConf.scala 96.22% <ø> (-0.09%) ⬇️
...apache/kyuubi/service/TBinaryFrontendService.scala 88.09% <ø> (-1.27%) ⬇️
...a/org/apache/kyuubi/service/TFrontendService.scala 94.79% <0.00%> (-0.72%) ⬇️
.../authentication/HadoopThriftAuthBridgeServer.scala 60.82% <ø> (ø)
...ervice/authentication/TSetIpAddressProcessor.scala 77.41% <ø> (ø)
...rg/apache/hadoop/hive/thrift/TFilterTransport.java 0.00% <0.00%> (ø)
...he/hadoop/hive/thrift/TUGIContainingTransport.java 0.00% <0.00%> (ø)
...doop/hive/thrift/client/TUGIAssumingTransport.java 0.00% <0.00%> (ø)
.../org/apache/kyuubi/jdbc/hive/KyuubiConnection.java 4.37% <0.00%> (ø)
.../apache/kyuubi/client/KyuubiSyncThriftClient.scala 91.66% <100.00%> (ø)
... and 14 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f25e5c9...898effc. Read the comment docs.

@pan3793 pan3793 requested a review from yaooqinn February 23, 2022 09:40
@yaooqinn
Copy link
Member

Thanks for your summary, @pan3793

Some classes in Hive jars compiled against old thrift interfaces which are not compatible with thrift 0.16.0, it causes runtime link error, we found all broken tests and copy the broken classes with necessary modification to make it work with thrift 0.16.0, but there may still risk somewhere the UT not covered.

BTW, please change Some classes exactly what they are, and move the summary to the PR desc.

@yaooqinn
Copy link
Member

the test failure here seems relevant, can we check on that? @SteNicholas

@pan3793
Copy link
Member

pan3793 commented Feb 23, 2022

Updated PR description, also added Next Steps @yaooqinn

Copy link
Member

@yaooqinn yaooqinn left a comment

Choose a reason for hiding this comment

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

Excellent work @SteNicholas
+1, LGTM

@yaooqinn yaooqinn closed this in 770499c Feb 23, 2022
@yaooqinn
Copy link
Member

thanks, merged to master

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants