yaooqinn / spark-ranger

已经合入(apache/incubator-kyuubi) ACL Management for Apache Spark SQL with Apache Ranger.
https://yaooqinn.github.io/spark-ranger/
Apache License 2.0
54 stars 56 forks source link

Add support for ranger 2.0 #9

Closed PedroRossi closed 4 years ago

PedroRossi commented 4 years ago

As mentioned in https://github.com/yaooqinn/spark-ranger/issues/4, this PR proposes adding another version of the RangerAdminClientImpl with complies with the RangerAdminClient interface which has more methods since Ranger 2.0

Changes made:

PedroRossi commented 4 years ago

@yaooqinn I noticed you already add the travis config on https://github.com/yaooqinn/spark-ranger/pull/7, should I remove this config from my PR then?

PedroRossi commented 4 years ago

@yaooqinn I also noticed that the config ranger.plugin.spark.policy.source.impl in the file ranger-spark-security.xml is not being used on the tests (I tried using a random name and the tests were still ok) and I also noticed that you have a sample policy, how are the tests working with it? I didn't understand this part well

codecov[bot] commented 4 years ago

Codecov Report

Merging #9 into master will increase coverage by 0.8%. The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             master      #9     +/-   ##
==========================================
+ Coverage        40%   40.8%   +0.8%     
- Complexity       96     104      +8     
==========================================
  Files            25      25             
  Lines           855     865     +10     
  Branches        283     286      +3     
==========================================
+ Hits            342     353     +11     
+ Misses          407     319     -88     
- Partials        106     193     +87
Impacted Files Coverage Δ Complexity Δ
...lyst/optimizer/RangerSparkRowFilterExtension.scala 60.37% <0%> (-1.03%) 16% <0%> (ø)
.../org/apache/spark/sql/hive/PrivilegesBuilder.scala 19.8% <0%> (+0.96%) 0% <0%> (ø) :arrow_down:
...yst/optimizer/RangerSparkAuthorizerExtension.scala 32.96% <0%> (+9.59%) 17% <0%> (+8%) :arrow_up:

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 a078b5a...17ab356. Read the comment docs.

yaooqinn commented 4 years ago

@yaooqinn I also noticed that the config ranger.plugin.spark.policy.source.impl in the file ranger-spark-security.xml is not being used on the tests (I tried using a random name and the tests were still ok) and I also noticed that you have a sample policy, how are the tests working with it? I didn't understand this part well

Hmm... I don' t remember well. So can we just rm this conf and the class to see if it works well?

PedroRossi commented 4 years ago

We can rm the class but not the conf, I already tested it, but not for all of the Ranger versions.

Going to test this later then send a report here

yaooqinn commented 4 years ago

I see. If that config is configured wrong or missed, the org.apache.ranger.admin.client.RangerAdminRESTClient will be used.

I guess we just need class RangerAdminClientImpl extends RangerAdminRESTClient to fix the issue here.

PedroRossi commented 4 years ago

RangerAdminRESTClient

This fixed the issue and it is better because we don't need to change the pom in this case

PedroRossi commented 4 years ago

@yaooqinn I also noticed that the config ranger.plugin.spark.policy.source.impl in the file ranger-spark-security.xml is not being used on the tests (I tried using a random name and the tests were still ok) and I also noticed that you have a sample policy, how are the tests working with it? I didn't understand this part well

Hmm... I don' t remember well. So can we just rm this conf and the class to see if it works well?

It works fine without this conf, we can remove this class and this conf since we do not leverage a Ranger for retrieving the policy

yaooqinn commented 4 years ago

Let's keep them, this class helps to debug when we add more policies to test resources.

yaooqinn commented 4 years ago

LGTM, could you also update README?

yaooqinn commented 4 years ago

merged to master, thanks for your help:)