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/10/19 11:43:12 UTC
[GitHub] [incubator-kyuubi] zhouyifan279 opened a new pull request, #3665: [Subtask] Get table owner from CatalogTable #3608
zhouyifan279 opened a new pull request, #3665:
URL: https://github.com/apache/incubator-kyuubi/pull/3665
### _Why are the changes needed?_
Subtask of #3607
### _How was this patch tested?_
- [x] 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
--
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 #3665: [KYUUBI #3608][Subtask] Get table owner from CatalogTable
Posted by GitBox <gi...@apache.org>.
codecov-commenter commented on PR #3665:
URL: https://github.com/apache/incubator-kyuubi/pull/3665#issuecomment-1284800789
# [Codecov](https://codecov.io/gh/apache/incubator-kyuubi/pull/3665?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 [#3665](https://codecov.io/gh/apache/incubator-kyuubi/pull/3665?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (2d7e6be) into [master](https://codecov.io/gh/apache/incubator-kyuubi/commit/84297ea466ba3868d6d69e599f5e32a53f5d320b?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) (84297ea) will **increase** coverage by `0.07%`.
> The diff coverage is `100.00%`.
> :exclamation: Current head 2d7e6be differs from pull request most recent head 49a08e0. Consider uploading reports for the commit 49a08e0 to get more accurate results
```diff
@@ Coverage Diff @@
## master #3665 +/- ##
============================================
+ Coverage 51.93% 52.00% +0.07%
Complexity 13 13
============================================
Files 484 485 +1
Lines 27122 27213 +91
Branches 3784 3793 +9
============================================
+ Hits 14086 14153 +67
- Misses 11669 11688 +19
- Partials 1367 1372 +5
```
| [Impacted Files](https://codecov.io/gh/apache/incubator-kyuubi/pull/3665?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | Coverage Δ | |
|---|---|---|
| [...he/kyuubi/plugin/spark/authz/PrivilegeObject.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3665/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-ZXh0ZW5zaW9ucy9zcGFyay9reXV1Ymktc3BhcmstYXV0aHovc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvcGx1Z2luL3NwYXJrL2F1dGh6L1ByaXZpbGVnZU9iamVjdC5zY2FsYQ==) | `85.71% <100.00%> (+2.38%)` | :arrow_up: |
| [.../kyuubi/plugin/spark/authz/PrivilegesBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3665/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-ZXh0ZW5zaW9ucy9zcGFyay9reXV1Ymktc3BhcmstYXV0aHovc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvcGx1Z2luL3NwYXJrL2F1dGh6L1ByaXZpbGVnZXNCdWlsZGVyLnNjYWxh) | `70.85% <100.00%> (+0.27%)` | :arrow_up: |
| [...ubi/plugin/spark/authz/ranger/AccessResource.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3665/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-ZXh0ZW5zaW9ucy9zcGFyay9reXV1Ymktc3BhcmstYXV0aHovc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvcGx1Z2luL3NwYXJrL2F1dGh6L3Jhbmdlci9BY2Nlc3NSZXNvdXJjZS5zY2FsYQ==) | `85.71% <100.00%> (+1.09%)` | :arrow_up: |
| [.../plugin/spark/authz/ranger/RuleAuthorization.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3665/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==) | `81.39% <100.00%> (+2.44%)` | :arrow_up: |
| [.../org/apache/kyuubi/session/KyuubiSessionImpl.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3665/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXNzaW9uL0t5dXViaVNlc3Npb25JbXBsLnNjYWxh) | `75.78% <0.00%> (-5.27%)` | :arrow_down: |
| [...rg/apache/kyuubi/ctl/cmd/log/LogBatchCommand.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3665/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: |
| [...scala/org/apache/kyuubi/config/ConfigBuilder.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3665/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9jb25maWcvQ29uZmlnQnVpbGRlci5zY2FsYQ==) | `95.28% <0.00%> (-1.72%)` | :arrow_down: |
| [...mon/src/main/scala/org/apache/kyuubi/Logging.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3665/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-a3l1dWJpLWNvbW1vbi9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9Mb2dnaW5nLnNjYWxh) | `52.63% <0.00%> (-1.32%)` | :arrow_down: |
| [.../engine/spark/session/SparkSQLSessionManager.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3665/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-ZXh0ZXJuYWxzL2t5dXViaS1zcGFyay1zcWwtZW5naW5lL3NyYy9tYWluL3NjYWxhL29yZy9hcGFjaGUva3l1dWJpL2VuZ2luZS9zcGFyay9zZXNzaW9uL1NwYXJrU1FMU2Vzc2lvbk1hbmFnZXIuc2NhbGE=) | `78.48% <0.00%> (-1.27%)` | :arrow_down: |
| [...che/kyuubi/server/KyuubiTHttpFrontendService.scala](https://codecov.io/gh/apache/incubator-kyuubi/pull/3665/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-a3l1dWJpLXNlcnZlci9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9zZXJ2ZXIvS3l1dWJpVEh0dHBGcm9udGVuZFNlcnZpY2Uuc2NhbGE=) | `64.23% <0.00%> (-0.73%)` | :arrow_down: |
| ... and [8 more](https://codecov.io/gh/apache/incubator-kyuubi/pull/3665/diff?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=The+Apache+Software+Foundation) | |
: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] zhouyifan279 commented on a diff in pull request #3665: [Subtask] Get table owner from CatalogTable #3608
Posted by GitBox <gi...@apache.org>.
zhouyifan279 commented on code in PR #3665:
URL: https://github.com/apache/incubator-kyuubi/pull/3665#discussion_r999330249
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegesBuilderSuite.scala:
##########
@@ -60,12 +60,20 @@ abstract class PrivilegesBuilderSuite extends AnyFunSuite
assert(po.actionType === PrivilegeObjectActionType.OTHER)
assert(po.privilegeObjectType === PrivilegeObjectType.TABLE_OR_VIEW)
assert(po.columns === cols)
+ checkTableOwner(po)
}
protected def checkColumns(query: String, cols: Seq[String]): Unit = {
checkColumns(sql(query).queryExecution.optimizedPlan, cols)
}
+ protected def checkTableOwner(po: PrivilegeObject): Unit = {
+ if (catalogImpl == "hive" && po.privilegeObjectType === PrivilegeObjectType.TABLE_OR_VIEW) {
Review Comment:
Support DataSourceV2 table in next PR
--
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] bowenliang123 commented on a diff in pull request #3665: [KYUUBI #3608][Subtask] Get table owner from CatalogTable
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3665:
URL: https://github.com/apache/incubator-kyuubi/pull/3665#discussion_r999567865
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegeObject.scala:
##########
@@ -43,4 +43,5 @@ case class PrivilegeObject(
actionType: PrivilegeObjectActionType,
dbname: String,
objectName: String,
- @Nonnull columns: Seq[String] = Nil)
+ @Nonnull columns: Seq[String] = Nil,
+ owner: Option[String] = None)
Review Comment:
OK, good to know it.
Let me attach refererence link to `ownerName` field of `HivePrivilegeObject` from hive-exec:
https://github.com/apache/hive/blob/master/ql/src/java/org/apache/hadoop/hive/ql/security/authorization/plugin/HivePrivilegeObject.java#L134
--
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] zhouyifan279 commented on pull request #3665: [KYUUBI #3608][Subtask] Get table owner from CatalogTable
Posted by GitBox <gi...@apache.org>.
zhouyifan279 commented on PR #3665:
URL: https://github.com/apache/incubator-kyuubi/pull/3665#issuecomment-1284838611
Also cc @yaooqinn
--
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] bowenliang123 commented on pull request #3665: [KYUUBI #3608][Subtask] Get table owner from CatalogTable
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on PR #3665:
URL: https://github.com/apache/incubator-kyuubi/pull/3665#issuecomment-1284847037
LGTM.
--
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] bowenliang123 commented on a diff in pull request #3665: [KYUUBI #3608][Subtask] Get table owner from CatalogTable
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3665:
URL: https://github.com/apache/incubator-kyuubi/pull/3665#discussion_r999493235
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegeObject.scala:
##########
@@ -43,4 +43,5 @@ case class PrivilegeObject(
actionType: PrivilegeObjectActionType,
dbname: String,
objectName: String,
- @Nonnull columns: Seq[String] = Nil)
+ @Nonnull columns: Seq[String] = Nil,
+ owner: Option[String] = None)
Review Comment:
`PrivilegeObject` is identifying types and dimensions of what privilege to be checked. This `owner` added here in the constructor is more like extra information about the resource itself rather than the description of privilege. I think it might violate the purpose of this `PrivilegeObject`. Is there any better we could defer it in later privilege checking in RuleAuthorization? Or we just have a flexible map here for now and future extended resource info like maybe `ownerGroup`, `resourceTag` or etc.?
--
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] zhouyifan279 commented on a diff in pull request #3665: [Draft][KYUUBI #3608][Subtask] Get table owner for queries run on SessionCatalog
Posted by GitBox <gi...@apache.org>.
zhouyifan279 commented on code in PR #3665:
URL: https://github.com/apache/incubator-kyuubi/pull/3665#discussion_r1000092348
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RangerSparkExtensionSuite.scala:
##########
@@ -672,6 +672,36 @@ abstract class RangerSparkExtensionSuite extends AnyFunSuite
}
doAs("admin", assert(sql("show tables from global_temp").collect().length == 0))
}
+
+ test("[KYUUBI #3607] Support {OWNER} variable defined in Ranger Policy") {
+ // For now, only hive catalog is supported
+ assume(catalogImpl == "hive")
Review Comment:
After running `IcebergCatalogRangerSparkExtensionSuite`, I found that tests in `RangerSparkExtensionSuite` always run on spark_catalog. As this test won't be used by CatalogV2 suites and is binding with `HiveExternalCatalog`, I will move it into `HiveCatalogRangerSparkExtensionSuite`.
@bowenliang123 Thanks for the adivce.
--
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] zhouyifan279 commented on a diff in pull request #3665: [KYUUBI #3608][Subtask] Get table owner from CatalogTable
Posted by GitBox <gi...@apache.org>.
zhouyifan279 commented on code in PR #3665:
URL: https://github.com/apache/incubator-kyuubi/pull/3665#discussion_r1000092348
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RangerSparkExtensionSuite.scala:
##########
@@ -672,6 +672,36 @@ abstract class RangerSparkExtensionSuite extends AnyFunSuite
}
doAs("admin", assert(sql("show tables from global_temp").collect().length == 0))
}
+
+ test("[KYUUBI #3607] Support {OWNER} variable defined in Ranger Policy") {
+ // For now, only hive catalog is supported
+ assume(catalogImpl == "hive")
Review Comment:
Whether a table supports owner semantic is affected both by `catalogImpl` and by `CatalogExtension` implementation.
After a second thought, I added `hasTableOwner` field into `SparkSessionProvider`. `hasTableOwner` defaults to true.
If tables in chid class of `PrivilegesBuilderSuite` and `RangerSparkExtensionSuite` does not have owner, this child class should override `hasTableOwner` field with a `false`.
--
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] zhouyifan279 commented on a diff in pull request #3665: [KYUUBI #3608][Subtask] Get table owner from CatalogTable
Posted by GitBox <gi...@apache.org>.
zhouyifan279 commented on code in PR #3665:
URL: https://github.com/apache/incubator-kyuubi/pull/3665#discussion_r999543540
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RangerSparkExtensionSuite.scala:
##########
@@ -672,6 +672,36 @@ abstract class RangerSparkExtensionSuite extends AnyFunSuite
}
doAs("admin", assert(sql("show tables from global_temp").collect().length == 0))
}
+
+ test("[KYUUBI #3607] Support {OWNER} variable defined in Ranger Policy") {
+ // For now, only hive catalog is supported
+ assume(catalogImpl == "hive")
Review Comment:
After the Umbrella issue is completed, {OWNER} variable support will not be restricted to hive catalog.
It should work with any `CatalogExtension` providing owner property.
Mabe it is better to cancel test by whether we can get table owner.
```
val (inputs, _) = PrivilegesBuilder.build(sql(select).queryExecution.analyzed, spark)
assume(inputs.nonEmpty && inputs.head.owner.isDefined)
```
--
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] pan3793 commented on pull request #3665: [KYUUBI #3608][Subtask] Get table owner for queries run on SessionCatalog
Posted by GitBox <gi...@apache.org>.
pan3793 commented on PR #3665:
URL: https://github.com/apache/incubator-kyuubi/pull/3665#issuecomment-1285170291
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] zhouyifan279 commented on a diff in pull request #3665: [Draft][KYUUBI #3608][Subtask] Get table owner for queries run on SessionCatalog
Posted by GitBox <gi...@apache.org>.
zhouyifan279 commented on code in PR #3665:
URL: https://github.com/apache/incubator-kyuubi/pull/3665#discussion_r1000092348
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RangerSparkExtensionSuite.scala:
##########
@@ -672,6 +672,36 @@ abstract class RangerSparkExtensionSuite extends AnyFunSuite
}
doAs("admin", assert(sql("show tables from global_temp").collect().length == 0))
}
+
+ test("[KYUUBI #3607] Support {OWNER} variable defined in Ranger Policy") {
+ // For now, only hive catalog is supported
+ assume(catalogImpl == "hive")
Review Comment:
After running `IcebergCatalogRangerSparkExtensionSuite`, I found that tests in `RangerSparkExtensionSuite` always run on spark_catalog.
This test won't be used by CatalogV2 suites and is binding with `HiveExternalCatalog`. I will move it into `HiveCatalogRangerSparkExtensionSuite`.
@bowenliang123 Thanks for the adivce.
--
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] zhouyifan279 commented on pull request #3665: [KYUUBI #3608][Subtask] Get table owner from CatalogTable
Posted by GitBox <gi...@apache.org>.
zhouyifan279 commented on PR #3665:
URL: https://github.com/apache/incubator-kyuubi/pull/3665#issuecomment-1283954643
cc @bowenliang123 @pan3793
--
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] pan3793 closed pull request #3665: [KYUUBI #3608][Subtask] Get table owner for queries run on SessionCatalog
Posted by GitBox <gi...@apache.org>.
pan3793 closed pull request #3665: [KYUUBI #3608][Subtask] Get table owner for queries run on SessionCatalog
URL: https://github.com/apache/incubator-kyuubi/pull/3665
--
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] bowenliang123 commented on a diff in pull request #3665: [KYUUBI #3608][Subtask] Get table owner from CatalogTable
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3665:
URL: https://github.com/apache/incubator-kyuubi/pull/3665#discussion_r999460562
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/RangerSparkExtensionSuite.scala:
##########
@@ -672,6 +672,36 @@ abstract class RangerSparkExtensionSuite extends AnyFunSuite
}
doAs("admin", assert(sql("show tables from global_temp").collect().length == 0))
}
+
+ test("[KYUUBI #3607] Support {OWNER} variable defined in Ranger Policy") {
+ // For now, only hive catalog is supported
+ assume(catalogImpl == "hive")
Review Comment:
how about move this ut to `HiveCatalogRangerSparkExtensionSuite`?
and remove checking the assume and checking catalogImpl to 'hive'
--
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] bowenliang123 commented on a diff in pull request #3665: [KYUUBI #3608][Subtask] Get table owner from CatalogTable
Posted by GitBox <gi...@apache.org>.
bowenliang123 commented on code in PR #3665:
URL: https://github.com/apache/incubator-kyuubi/pull/3665#discussion_r999493235
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegeObject.scala:
##########
@@ -43,4 +43,5 @@ case class PrivilegeObject(
actionType: PrivilegeObjectActionType,
dbname: String,
objectName: String,
- @Nonnull columns: Seq[String] = Nil)
+ @Nonnull columns: Seq[String] = Nil,
+ owner: Option[String] = None)
Review Comment:
`PrivilegeObject` is identifying types and dimensions of what privilege to be checked. This `owner` added here in the constructor is more like extra information about the resource itself rather than the description of privilege. I think it might violate the purpose of this `PrivilegeObject`. Is there any better we could defer it in later privilege checking in RuleAuthorization? Or we just have a flexible map here for now and future extended resource info like maybe `ownerGroup`, `resourceTag` or etc.?
cc @yaooqinn what do you think?
--
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] zhouyifan279 commented on a diff in pull request #3665: [KYUUBI #3608][Subtask] Get table owner from CatalogTable
Posted by GitBox <gi...@apache.org>.
zhouyifan279 commented on code in PR #3665:
URL: https://github.com/apache/incubator-kyuubi/pull/3665#discussion_r999559244
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/PrivilegeObject.scala:
##########
@@ -43,4 +43,5 @@ case class PrivilegeObject(
actionType: PrivilegeObjectActionType,
dbname: String,
objectName: String,
- @Nonnull columns: Seq[String] = Nil)
+ @Nonnull columns: Seq[String] = Nil,
+ owner: Option[String] = None)
Review Comment:
`PrivilegeObject` originates from `HivePrivilegeObject`. In `HivePrivilegeObject`, owner is stored in a field named `ownerName` as a sibling to `dbname` and `columns`.
I'd like to keep the code as it is for now to follow Hive's design. We can optimize the code if things get complicated in the future.
--
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