You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by GitBox <gi...@apache.org> on 2022/08/17 13:02:38 UTC
[GitHub] [incubator-kyuubi] yikf opened a new pull request, #3262: Privileges should be checked only once in `RuleAuthorization`
yikf opened a new pull request, #3262:
URL: https://github.com/apache/incubator-kyuubi/pull/3262
<!--
Thanks for sending a pull request!
Here are some tips for you:
1. If this is your first time, please read our contributor guidelines: https://kyuubi.readthedocs.io/en/latest/community/contributions.html
2. If the PR is related to an issue in https://github.com/apache/incubator-kyuubi/issues, add '[KYUUBI #XXXX]' in your PR title, e.g., '[KYUUBI #XXXX] Your PR title ...'.
3. If the PR is unfinished, add '[WIP]' in your PR title, e.g., '[WIP][KYUUBI #XXXX] Your PR title ...'.
-->
### _Why are the changes needed?_
<!--
Please clarify why the changes are needed. For instance,
1. If you add a feature, you can talk about the use case of it.
2. If you fix a bug, you can clarify why it is a bug.
-->
fix https://github.com/apache/incubator-kyuubi/issues/3226
### _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](https://kyuubi.apache.org/docs/latest/develop_tools/testing.html#running-tests) locally before make a pull request
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] codecov-commenter commented on pull request #3262: Privileges should be checked only once in `RuleAuthorization`
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #3262:
URL: https://github.com/apache/incubator-kyuubi/pull/3262#issuecomment-1219056568
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/3262?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) Report
> Merging [#3262](https://codecov.io/gh/apache/incubator-kyuubi/pull/3262?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (6e85468) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/a3b6c6756781145f4a7a4f640d059a058ae21c3f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (a3b6c67) will **decrease** coverage by `0.01%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head 6e85468 differs from pull request most recent head fc93df3. Consider uploading reports for the commit fc93df3 to get more accurate results
```diff
@@ Coverage Diff @@
## master #3262 +/- ##
============================================
- Coverage 51.26% 51.24% -0.02%
Complexity 13 13
============================================
Files 468 468
Lines 26145 26150 +5
Branches 3627 3627
============================================
- Hits 13402 13400 -2
- Misses 11463 11467 +4
- Partials 1280 1283 +3
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/3262?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [.../plugin/spark/authz/ranger/RuleAuthorization.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3262/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1Ymktc3BhcmstYXV0aHovc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvcGx1Z2luL3NwYXJrL2F1dGh6L3Jhbmdlci9SdWxlQXV0aG9yaXphdGlvbi5zY2FsYQ==) | `80.00% <100.00%> (+2.85%)` | :arrow_up: |
| [...rg/apache/kyuubi/ctl/cmd/log/LogBatchCommand.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3262/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWN0bC9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jdGwvY21kL2xvZy9Mb2dCYXRjaENvbW1hbmQuc2NhbGE=) | `78.00% <0.00%> (-2.00%)` | :arrow_down: |
| [...ain/scala/org/apache/kyuubi/engine/EngineRef.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3262/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvRW5naW5lUmVmLnNjYWxh) | `74.07% <0.00%> (-0.93%)` | :arrow_down: |
| [...yuubi/server/metadata/jdbc/JDBCMetadataStore.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3262/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvbWV0YWRhdGEvamRiYy9KREJDTWV0YWRhdGFTdG9yZS5zY2FsYQ==) | `89.27% <0.00%> (-0.70%)` | :arrow_down: |
| [...he/kyuubi/ha/client/etcd/EtcdDiscoveryClient.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3262/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWhhL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2hhL2NsaWVudC9ldGNkL0V0Y2REaXNjb3ZlcnlDbGllbnQuc2NhbGE=) | `67.50% <0.00%> (-0.63%)` | :arrow_down: |
| [.../apache/kyuubi/server/api/v1/BatchesResource.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3262/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvYXBpL3YxL0JhdGNoZXNSZXNvdXJjZS5zY2FsYQ==) | `70.05% <0.00%> (-0.51%)` | :arrow_down: |
| [...a/org/apache/kyuubi/service/TFrontendService.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3262/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2aWNlL1RGcm9udGVuZFNlcnZpY2Uuc2NhbGE=) | `91.17% <0.00%> (-0.30%)` | :arrow_down: |
| [...g/apache/kyuubi/operation/BatchJobSubmission.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3262/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9vcGVyYXRpb24vQmF0Y2hKb2JTdWJtaXNzaW9uLnNjYWxh) | `74.40% <0.00%> (ø)` | |
| [...pache/kyuubi/engine/YarnApplicationOperation.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3262/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation#diff-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9lbmdpbmUvWWFybkFwcGxpY2F0aW9uT3BlcmF0aW9uLnNjYWxh) | `64.81% <0.00%> (ø)` | |
:mega: We’re building smart automated test selection to slash your CI/CD build times. [Learn more](https://about.codecov.io/iterative-testing/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a diff in pull request #3262: Privileges should be checked only once in `RuleAuthorization`
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #3262:
URL: https://github.com/apache/incubator-kyuubi/pull/3262#discussion_r948685876
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RangerSparkExtensionSuite.scala:
##########
@@ -59,6 +61,63 @@ abstract class RangerSparkExtensionSuite extends AnyFunSuite
s"Permission denied: user [$user] does not have [$privilege] privilege on [$resource]"
}
+ test("RuleAuthorization: Should check privileges once only.") {
Review Comment:
let's add `[KYUUBI #3226]` here
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RangerSparkExtensionSuite.scala:
##########
@@ -59,6 +61,63 @@ abstract class RangerSparkExtensionSuite extends AnyFunSuite
s"Permission denied: user [$user] does not have [$privilege] privilege on [$resource]"
}
+ test("RuleAuthorization: Should check privileges once only.") {
+ val logicalPlan = NoopCommand("RuleAuthorization", Seq.empty)
+ val rule = new RuleAuthorization(spark)
+
+ (1 until 10).foreach { ignore =>
+ rule.apply(logicalPlan)
+ }
+
+ assert(logicalPlan.getTagValue(KYUUBI_AUTH_TAG).getOrElse(0) == 1)
+ }
+
+ test("RuleAuthorization: Another session should also check even if the plan is cached.") {
Review Comment:
ditto
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleAuthorization.scala:
##########
@@ -22,20 +22,35 @@ import scala.collection.mutable.ArrayBuffer
import org.apache.spark.sql.SparkSession
import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.TreeNodeTag
import org.apache.kyuubi.plugin.spark.authz.{ObjectType, _}
import org.apache.kyuubi.plugin.spark.authz.ObjectType._
+import org.apache.kyuubi.plugin.spark.authz.ranger.RuleAuthorization.KYUUBI_AUTH_TAG
import org.apache.kyuubi.plugin.spark.authz.util.AuthZUtils._
class RuleAuthorization(spark: SparkSession) extends Rule[LogicalPlan] {
override def apply(plan: LogicalPlan): LogicalPlan = {
- RuleAuthorization.checkPrivileges(spark, plan)
+ plan match {
Review Comment:
simply
```scala
override def apply(plan: LogicalPlan): LogicalPlan = plan match {
case p if p.getTagValue(KYUUBI_AUTH_TAG).contains(true) =>
balala..
case p => p
}
```
?
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleAuthorization.scala:
##########
@@ -22,20 +22,35 @@ import scala.collection.mutable.ArrayBuffer
import org.apache.spark.sql.SparkSession
import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.TreeNodeTag
import org.apache.kyuubi.plugin.spark.authz.{ObjectType, _}
import org.apache.kyuubi.plugin.spark.authz.ObjectType._
+import org.apache.kyuubi.plugin.spark.authz.ranger.RuleAuthorization.KYUUBI_AUTH_TAG
import org.apache.kyuubi.plugin.spark.authz.util.AuthZUtils._
class RuleAuthorization(spark: SparkSession) extends Rule[LogicalPlan] {
override def apply(plan: LogicalPlan): LogicalPlan = {
- RuleAuthorization.checkPrivileges(spark, plan)
+ plan match {
+ case p if shouldAuth(p) =>
+ RuleAuthorization.checkPrivileges(spark, p)
+ p.setTagValue(KYUUBI_AUTH_TAG, p.getTagValue(KYUUBI_AUTH_TAG).getOrElse(0) + 1)
+
+ case _ => // do nothing if checked privileges already.
+ }
plan
}
+
+ def shouldAuth(plan: LogicalPlan): Boolean = {
+ val authNum = plan.getTagValue(KYUUBI_AUTH_TAG).getOrElse(0)
+ authNum == 0
+ }
}
object RuleAuthorization {
+ val KYUUBI_AUTH_TAG = TreeNodeTag[Int]("__KYUUBI_AUTH_TAG")
Review Comment:
1 Use boolean?
2 use AUTHZ? auth is ambiguous for authenticate and authroize
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] ulysses-you commented on pull request #3262: [KYUUBI #3226] Privileges should be checked only once in `RuleAuthorization`
Posted by GitBox <gi...@apache.org>.
ulysses-you commented on PR #3262:
URL: https://github.com/apache/incubator-kyuubi/pull/3262#issuecomment-1219292292
thanks, merging to master
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] yikf commented on a diff in pull request #3262: Privileges should be checked only once in `RuleAuthorization`
Posted by GitBox <gi...@apache.org>.
yikf commented on code in PR #3262:
URL: https://github.com/apache/incubator-kyuubi/pull/3262#discussion_r948611574
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleAuthorization.scala:
##########
@@ -22,20 +22,30 @@ import scala.collection.mutable.ArrayBuffer
import org.apache.spark.sql.SparkSession
import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.TreeNodeTag
import org.apache.kyuubi.plugin.spark.authz.{ObjectType, _}
import org.apache.kyuubi.plugin.spark.authz.ObjectType._
+import org.apache.kyuubi.plugin.spark.authz.ranger.RuleAuthorization.KYUUBI_AUTH_TAG
import org.apache.kyuubi.plugin.spark.authz.util.AuthZUtils._
class RuleAuthorization(spark: SparkSession) extends Rule[LogicalPlan] {
override def apply(plan: LogicalPlan): LogicalPlan = {
- RuleAuthorization.checkPrivileges(spark, plan)
+ plan match {
+ case p if !p.getTagValue(KYUUBI_AUTH_TAG).getOrElse(false) =>
Review Comment:
good catch, will add test suites to verify this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on pull request #3262: [KYUUBI #3226] Privileges should be checked only once in `RuleAuthorization`
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on PR #3262:
URL: https://github.com/apache/incubator-kyuubi/pull/3262#issuecomment-1219108626
please fix code style
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] ulysses-you closed pull request #3262: [KYUUBI #3226] Privileges should be checked only once in `RuleAuthorization`
Posted by GitBox <gi...@apache.org>.
ulysses-you closed pull request #3262: [KYUUBI #3226] Privileges should be checked only once in `RuleAuthorization`
URL: https://github.com/apache/incubator-kyuubi/pull/3262
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] yikf commented on pull request #3262: Privileges should be checked only once in `RuleAuthorization`
Posted by GitBox <gi...@apache.org>.
yikf commented on PR #3262:
URL: https://github.com/apache/incubator-kyuubi/pull/3262#issuecomment-1219059401
@yaooqinn tests is added to verify the multi-session & cache plan scenario;
Even if plan is cached, check is performed for every new query, this is because `QueryExecution.optimizedPlan` did not copy PlanTag when `withCachedData.clone()` [1], but it seems to be in line with our expectations
[1] [Spark QueryExecution base code](https://github.com/apache/spark/blob/master/sql/core/src/main/scala/org/apache/spark/sql/execution/QueryExecution.scala#L126)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] yaooqinn commented on a diff in pull request #3262: Privileges should be checked only once in `RuleAuthorization`
Posted by GitBox <gi...@apache.org>.
yaooqinn commented on code in PR #3262:
URL: https://github.com/apache/incubator-kyuubi/pull/3262#discussion_r948602829
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RuleAuthorization.scala:
##########
@@ -22,20 +22,30 @@ import scala.collection.mutable.ArrayBuffer
import org.apache.spark.sql.SparkSession
import org.apache.spark.sql.catalyst.plans.logical.LogicalPlan
import org.apache.spark.sql.catalyst.rules.Rule
+import org.apache.spark.sql.catalyst.trees.TreeNodeTag
import org.apache.kyuubi.plugin.spark.authz.{ObjectType, _}
import org.apache.kyuubi.plugin.spark.authz.ObjectType._
+import org.apache.kyuubi.plugin.spark.authz.ranger.RuleAuthorization.KYUUBI_AUTH_TAG
import org.apache.kyuubi.plugin.spark.authz.util.AuthZUtils._
class RuleAuthorization(spark: SparkSession) extends Rule[LogicalPlan] {
override def apply(plan: LogicalPlan): LogicalPlan = {
- RuleAuthorization.checkPrivileges(spark, plan)
+ plan match {
+ case p if !p.getTagValue(KYUUBI_AUTH_TAG).getOrElse(false) =>
Review Comment:
what if this plan is cached? for group mode, multiple client users may share the same cache manager, can we add a unit test to verify
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org
[GitHub] [incubator-kyuubi] yikf commented on pull request #3262: [KYUUBI #3226] Privileges should be checked only once in `RuleAuthorization`
Posted by GitBox <gi...@apache.org>.
yikf commented on PR #3262:
URL: https://github.com/apache/incubator-kyuubi/pull/3262#issuecomment-1219097614
thanks @yaooqinn @ulysses-you for review, addressed comment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@kyuubi.apache.org
For additional commands, e-mail: notifications-help@kyuubi.apache.org