You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@kyuubi.apache.org by "yabola (via GitHub)" <gi...@apache.org> on 2023/09/04 11:03:05 UTC
[GitHub] [kyuubi] yabola opened a new pull request, #5248: Support iceberg call statement in ranger authentication
yabola opened a new pull request, #5248:
URL: https://github.com/apache/kyuubi/pull/5248
<!--
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/CONTRIBUTING.html
2. If the PR is related to an issue in https://github.com/apache/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.
-->
### _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.readthedocs.io/en/master/contributing/code/testing.html#running-tests) locally before make a pull request
### _Was this patch authored or co-authored using generative AI tooling?_
<!--
If a generative AI tooling has been used in the process of authoring this patch, please include
phrase 'Generated-by: ' followed by the name of the tool and its version.
If no, write 'No'.
Please refer to the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) for details.
-->
--
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] [kyuubi] bowenliang123 closed pull request #5248: [AUTHZ] Check previleges for Iceberg's CALL to RewriteDataFilesProcedure procedure
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 closed pull request #5248: [AUTHZ] Check previleges for Iceberg's CALL to RewriteDataFilesProcedure procedure
URL: https://github.com/apache/kyuubi/pull/5248
--
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] [kyuubi] bowenliang123 commented on a diff in pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1320918717
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/IcebergCatalogRangerSparkExtensionSuite.scala:
##########
@@ -233,4 +234,69 @@ class IcebergCatalogRangerSparkExtensionSuite extends RangerSparkExtensionSuite
assert(e1.getMessage.contains(s"does not have [select] privilege" +
s" on [$namespace1/$table1]"))
}
+
+ test("CALL RewriteDataFilesProcedure") {
+ val tableName = "table_select_call_command_table"
+ val table = s"$catalogV2.$defaultBob.$tableName"
+ val rewriteDataFiles1 = s"CALL $catalogV2.system.rewrite_data_files " +
+ s"(table => '$table', options => map('min-input-files','2'))"
+ val rewriteDataFiles2 = s"CALL $catalogV2.system.rewrite_data_files " +
+ s"(table => '$table', options => map('min-input-files','3'))"
+ var snapshotId = 0L
+ var snapshotCommand = ""
+
+ withCleanTmpResources(Seq((table, "table"))) {
+ doAs(
+ admin, {
+ sql(s"CREATE TABLE IF NOT EXISTS $table (id int, name string) USING iceberg")
+ sql(s"INSERT INTO $table VALUES (1, 'chenliang'), (2, 'tom')")
+ sql(s"INSERT INTO $table VALUES (3, 'julie'), (4, 'ross')")
+ snapshotId = sql(s"select * from $table.snapshots limit 1")
+ .collect().apply(0).getAs[Long]("snapshot_id")
+ snapshotCommand = s"CALL $catalogV2.system.set_current_snapshot ('$table', $snapshotId)"
Review Comment:
Let's skip testing on `CALL system.set_current_snapshot` procedure this time. Move to the follow-ups.
--
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] [kyuubi] bowenliang123 commented on pull request #5248: [AUTHZ] Check previleges for Iceberg's CALL to RewriteDataFilesProcedure procedure
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#issuecomment-1715180061
"Permission denied: user [someone] does not have [alter] privilege on [iceberg_ns/table_select_call_command_table]"
did not contain
"does not have [select] privilege on [iceberg_ns/table_select_call_command_table], [update] privilege on [iceberg_ns/table_select_call_command_table]"
PrivilegesBuilder extracted two privilege object for input and output.
But their action type both are `PrivilegeObjectActionType.OTHER` (Possible value of PrivilegeObjectActionType are OTHER, INSERT, INSERT_OVERWRITE, UPDATE, DELETE).
Therefore, RuleAuthorization would then treat them as the same privilege object, and check the `alter` privilege only.
1. it seems we could not distinguish them, by 1 for `select` on input table and 1 for `alter` on output table. cc @yaooqinn
2. Or let's check alter privilege only, and skip checking `select` privilege, as the same and only table used in one procedure. @yabola
--
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] [kyuubi] yabola commented on pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "yabola (via GitHub)" <gi...@apache.org>.
yabola commented on PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#issuecomment-1706191828
@bowenliang123 Thank you for your advice.
1. I add new method in `TableCommands` and generate json by `KYUUBI_UPDATE=1 build/mvn clean test -Pgen-policy -pl :kyuubi-spark-authz_2.12 -Dtest=none -DwildcardSuites=org.apache.kyuubi.plugin.spark.authz.gen.JsonSpecFileGenerator` command. Please see if it is suitable.
2. I removed one unnecessary table extractor. But there may a new table extractor may need to be added because the iceberg call command is a new logical plan(`Call(procedure: Procedure, args: Seq[Expression])`). Or do you have a better suggestion?
--
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] [kyuubi] bowenliang123 commented on a diff in pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1318183410
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/serde/tableExtractors.scala:
##########
@@ -145,14 +175,37 @@ class DataSourceV2RelationTableExtractor extends TableExtractor {
val maybeCatalogPlugin = invokeAs[Option[AnyRef]](v2Relation, "catalog")
val maybeCatalog = maybeCatalogPlugin.flatMap(catalogPlugin =>
lookupExtractor[CatalogPluginCatalogExtractor].apply(catalogPlugin))
- val maybeIdentifier = invokeAs[Option[AnyRef]](v2Relation, "identifier")
Review Comment:
I think we need to `TableTableExtractor` for taking `Table` input, instead of use `StringTableExtractor` directly.
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/serde/tableExtractors.scala:
##########
@@ -145,14 +175,37 @@ class DataSourceV2RelationTableExtractor extends TableExtractor {
val maybeCatalogPlugin = invokeAs[Option[AnyRef]](v2Relation, "catalog")
val maybeCatalog = maybeCatalogPlugin.flatMap(catalogPlugin =>
lookupExtractor[CatalogPluginCatalogExtractor].apply(catalogPlugin))
- val maybeIdentifier = invokeAs[Option[AnyRef]](v2Relation, "identifier")
Review Comment:
I think we need to `TableTableExtractor` for taking `Table` input, instead of using `StringTableExtractor` directly.
--
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] [kyuubi] yabola commented on a diff in pull request #5248: [AUTHZ] Check previleges for Iceberg's CALL to RewriteDataFilesProcedure procedure
Posted by "yabola (via GitHub)" <gi...@apache.org>.
yabola commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1322287343
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/IcebergCatalogRangerSparkExtensionSuite.scala:
##########
@@ -226,4 +227,74 @@ class IcebergCatalogRangerSparkExtensionSuite extends RangerSparkExtensionSuite
assert(e1.getMessage.contains(s"does not have [select] privilege" +
s" on [$namespace1/$table1]"))
}
+
+ test("CALL RewriteDataFilesProcedure") {
+ val tableName = "table_select_call_command_table"
+ val table = s"$catalogV2.$namespace1.$tableName"
+ val rewriteDataFiles1 = s"CALL $catalogV2.system.rewrite_data_files " +
+ s"(table => '$table', options => map('min-input-files','2'))"
+ val rewriteDataFiles2 = s"CALL $catalogV2.system.rewrite_data_files " +
+ s"(table => '$table', options => map('min-input-files','3'))"
+
+ withCleanTmpResources(Seq((table, "table"))) {
+ doAs(
+ admin, {
+ sql(s"CREATE TABLE IF NOT EXISTS $table (id int, name string) USING iceberg")
+ sql(s"INSERT INTO $table VALUES (1, 'chenliang'), (2, 'tom')")
+ sql(s"INSERT INTO $table VALUES (3, 'julie'), (4, 'ross')")
+ })
+
+ // user `user_read_only_on_iceberg` has select permission on database `iceberg_ns`
+ interceptContains[AccessControlException](doAs(icebergReadOnlyUser, sql(rewriteDataFiles1)))(
+ s"does not have [update] privilege on [$namespace1/$tableName]")
+ interceptContains[AccessControlException](doAs(icebergReadOnlyUser, sql(rewriteDataFiles2)))(
+ s"does not have [update] privilege on [$namespace1/$tableName]")
+
+ // user `someone` has no permission on table
+ interceptContains[AccessControlException](doAs(someone, sql(rewriteDataFiles1)))(
+ s"does not have [select] privilege on [$namespace1/$tableName]")
+ interceptContains[AccessControlException](doAs(someone, sql(rewriteDataFiles2)))(
+ s"does not have [select] privilege on [$namespace1/$tableName]")
+
+ withSingleCallEnabled {
+ interceptContains[AccessControlException](doAs(someone, sql(rewriteDataFiles1).explain()))(
+ s"does not have [select] privilege on [$namespace1/$tableName]," +
+ s" [update] privilege on [$namespace1/$tableName]")
+ }
+
+ /**
+ * Situation 1:
+ * Two logical plans triggered
+ * when ( input-files(2) >= min-input-files(2) ):
+ *
+ * == Physical Plan 1 ==
+ * Call (1)
+ *
+ * == Physical Plan 2 ==
+ * AppendData (3)
+ * +- * ColumnarToRow (2)
+ * +- BatchScan local.iceberg_ns.call_command_table (1)
+ */
+ doAs(
+ admin, {
+ val result2 = sql(rewriteDataFiles1).collect()
+ // rewrite 2 files
Review Comment:
It means number of data files which were re-written by this command ( 'result to 2 rewritten data files' ) . The explanation of the output of this command can refer to https://iceberg.apache.org/docs/latest/spark-procedures/#rewrite_data_files
--
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] [kyuubi] yabola commented on a diff in pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "yabola (via GitHub)" <gi...@apache.org>.
yabola commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1315734003
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/gen/IcebergCommands.scala:
##########
@@ -49,10 +49,22 @@ object IcebergCommands {
TableCommandSpec(cmd, Seq(tableDesc), queryDescs = Seq(queryDesc))
}
+ val IcebergCallCommand = {
+ val cmd = "org.apache.spark.sql.catalyst.plans.logical.Call"
+ val actionTypeDesc = ActionTypeDesc(actionType = Some(INSERT))
Review Comment:
Thanks, done. And I also change the `DataSourceV2RelationTableExtractor`. In spark org.apache.spark.sql.execution.datasources.v2.DataSourceV2Relation#table will always exist (not option)
--
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] [kyuubi] bowenliang123 commented on pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#issuecomment-1706261329
1. Please move `IcebergCallCommand ` to `IcebergCommands` class as mentioned above, as `org.apache.spark.sql.catalyst.plans.logical.Call` is from Iceberg but not a general plan from Spark engine. And result spec file is check by CI tests.
2. It's feasible to introduce a new table extractor for the Call plan.
3. Try not to introduce `resolveIcebergTable` in `tableExtractors`, and do the extraction by reusing existed extractors following the main procedures. Reversed coupling is always unwelcomed. Any other lakehouse may and will be supported by design with in separated command extractors and specs, while sharing the same common structure.
--
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] [kyuubi] yabola commented on a diff in pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "yabola (via GitHub)" <gi...@apache.org>.
yabola commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1315734003
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/gen/IcebergCommands.scala:
##########
@@ -49,10 +49,22 @@ object IcebergCommands {
TableCommandSpec(cmd, Seq(tableDesc), queryDescs = Seq(queryDesc))
}
+ val IcebergCallCommand = {
+ val cmd = "org.apache.spark.sql.catalyst.plans.logical.Call"
+ val actionTypeDesc = ActionTypeDesc(actionType = Some(INSERT))
Review Comment:
Thanks, done
--
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] [kyuubi] yabola commented on pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "yabola (via GitHub)" <gi...@apache.org>.
yabola commented on PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#issuecomment-1709541937
@bowenliang123 Thank you for the improvement
--
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] [kyuubi] bowenliang123 commented on a diff in pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1319594787
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/IcebergCatalogRangerSparkExtensionSuite.scala:
##########
@@ -235,13 +235,15 @@ class IcebergCatalogRangerSparkExtensionSuite extends RangerSparkExtensionSuite
s" on [$namespace1/$table1]"))
}
- test("CALL RewriteDataFilesProcedure") {
- val tableName = "call_command_table"
- val table = s"$catalogV2.$namespace1.$tableName"
- val rewriteCommand1 = s"CALL $catalogV2.system.rewrite_data_files " +
+ test("CALL RewriteProcedure") {
Review Comment:
We'd better use `RewriteDataFilesProcedure`, refering to `org.apache.iceberg.spark.procedures.RewriteDataFilesProcedure` from Iceberg Spark plugin.
And separate tests for either `rewrite_manifests` and `rewrite_data_files`.
--
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] [kyuubi] bowenliang123 commented on a diff in pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1319243194
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/serde/tableExtractors.scala:
##########
@@ -145,14 +175,37 @@ class DataSourceV2RelationTableExtractor extends TableExtractor {
val maybeCatalogPlugin = invokeAs[Option[AnyRef]](v2Relation, "catalog")
val maybeCatalog = maybeCatalogPlugin.flatMap(catalogPlugin =>
lookupExtractor[CatalogPluginCatalogExtractor].apply(catalogPlugin))
- val maybeIdentifier = invokeAs[Option[AnyRef]](v2Relation, "identifier")
Review Comment:
Please attach the evidence showing the impacts and differences in logical plan details of Iceberg call procedure.
--
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] [kyuubi] yabola commented on a diff in pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "yabola (via GitHub)" <gi...@apache.org>.
yabola commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1319594486
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/serde/tableExtractors.scala:
##########
@@ -145,14 +175,37 @@ class DataSourceV2RelationTableExtractor extends TableExtractor {
val maybeCatalogPlugin = invokeAs[Option[AnyRef]](v2Relation, "catalog")
val maybeCatalog = maybeCatalogPlugin.flatMap(catalogPlugin =>
lookupExtractor[CatalogPluginCatalogExtractor].apply(catalogPlugin))
- val maybeIdentifier = invokeAs[Option[AnyRef]](v2Relation, "identifier")
Review Comment:
When CALL ice.system.rewrite_data_files command does the rewrite operation, two logical plans will be generated, for example as in my newly added unit test:
* This triggers two logical plans( input-files(2) >= min-input-files(2) ):
* == Physical Plan 1 ==
* (1) Call
*
* == Physical Plan 2 ==
* AppendData (3)
* +- * ColumnarToRow (2)
* +- BatchScan local.iceberg_ns.call_command_table (1)
In the second logical plan, the `DataSourceV2Relation#identifier` in `AppendData` will be rewrite task id. So it is a more appropriate way to parse the table name from `DataSourceV2Relation#table`
<img width="1318" alt="image" src="https://github.com/apache/kyuubi/assets/31469905/efc815cb-8dfe-4a9f-8e24-d62883ef6eff">
--
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] [kyuubi] bowenliang123 commented on a diff in pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1319594787
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/IcebergCatalogRangerSparkExtensionSuite.scala:
##########
@@ -235,13 +235,15 @@ class IcebergCatalogRangerSparkExtensionSuite extends RangerSparkExtensionSuite
s" on [$namespace1/$table1]"))
}
- test("CALL RewriteDataFilesProcedure") {
- val tableName = "call_command_table"
- val table = s"$catalogV2.$namespace1.$tableName"
- val rewriteCommand1 = s"CALL $catalogV2.system.rewrite_data_files " +
+ test("CALL RewriteProcedure") {
Review Comment:
We'd better use `RewriteDataFilesProcedure`, refering to `org.apache.iceberg.spark.procedures.RewriteDataFilesProcedure` from Iceberg Spark plugin.
--
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] [kyuubi] yabola commented on a diff in pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "yabola (via GitHub)" <gi...@apache.org>.
yabola commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1319594486
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/serde/tableExtractors.scala:
##########
@@ -145,14 +175,37 @@ class DataSourceV2RelationTableExtractor extends TableExtractor {
val maybeCatalogPlugin = invokeAs[Option[AnyRef]](v2Relation, "catalog")
val maybeCatalog = maybeCatalogPlugin.flatMap(catalogPlugin =>
lookupExtractor[CatalogPluginCatalogExtractor].apply(catalogPlugin))
- val maybeIdentifier = invokeAs[Option[AnyRef]](v2Relation, "identifier")
Review Comment:
When CALL ice.system.rewrite_data_files command does the rewrite operation, two logical plans will be generated, for example in my newly added unit test:
> * This triggers two logical plans( input-files(2) >= min-input-files(2) ):
*
* == Physical Plan 1 ==
* (1) Call
*
* == Physical Plan 2 ==
* AppendData (3)
* +- * ColumnarToRow (2)
* +- BatchScan local.iceberg_ns.call_command_table (1)
In the second logical plan, the `DataSourceV2Relation#identifier` in `AppendData` will be rewrite task id. So it is a more appropriate way to parse the table name from `DataSourceV2Relation#table`
<img width="1318" alt="image" src="https://github.com/apache/kyuubi/assets/31469905/efc815cb-8dfe-4a9f-8e24-d62883ef6eff">
--
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] [kyuubi] yabola commented on a diff in pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "yabola (via GitHub)" <gi...@apache.org>.
yabola commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1319594486
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/serde/tableExtractors.scala:
##########
@@ -145,14 +175,37 @@ class DataSourceV2RelationTableExtractor extends TableExtractor {
val maybeCatalogPlugin = invokeAs[Option[AnyRef]](v2Relation, "catalog")
val maybeCatalog = maybeCatalogPlugin.flatMap(catalogPlugin =>
lookupExtractor[CatalogPluginCatalogExtractor].apply(catalogPlugin))
- val maybeIdentifier = invokeAs[Option[AnyRef]](v2Relation, "identifier")
Review Comment:
When CALL ice.system.rewrite_data_files command does the rewrite operation, two logical plans will be generated, for example in my unit test:
>
* This triggers two logical plans( input-files(2) >= min-input-files(2) ):
*
* == Physical Plan 1 ==
* (1) Call
*
* == Physical Plan 2 ==
* AppendData (3)
* +- * ColumnarToRow (2)
* +- BatchScan local.iceberg_ns.call_command_table (1)
In the second logical plan, the `DataSourceV2Relation#identifier` in `AppendData` will be rewrite task id. So it is a more appropriate way to parse the table name from `DataSourceV2Relation#table`
<img width="1318" alt="image" src="https://github.com/apache/kyuubi/assets/31469905/efc815cb-8dfe-4a9f-8e24-d62883ef6eff">
--
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] [kyuubi] bowenliang123 commented on pull request #5248: Support iceberg call statement in ranger authentication
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#issuecomment-1705981965
Hi @yabola, thanks for the contribution to the Authz plugin. And pleased to see introducing support for CALL statements for Iceberg which are critical to lakehouse lifecycle.
Some advices here,
1. Move the command spec generation to `authz.gen` package's `IcebergCommands`, which then is used in `JsonSpecFileGenerator` for spec json file generation by running `dev/gen/gen_ranger_spec_json.sh` locally . And also running `KYUUBI_UPDATE=0 dev/gen/gen_ranger_spec_json.sh` to check spec file.
2. if possible, try not to introduce a new TableExtractor specifically for Iceberg. Iceberg commands are generally supported with existed extractors.
--
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] [kyuubi] bowenliang123 commented on a diff in pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1318100209
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/IcebergCatalogPrivilegesBuilderSuite.scala:
##########
@@ -125,4 +125,26 @@ class IcebergCatalogPrivilegesBuilderSuite extends V2CommandsPrivilegesSuite {
assert(accessType === AccessType.UPDATE)
}
}
+
+ test("CallRewriteDataFilesProcedure") {
+ val table = "CallRewriteDataFilesProcedure"
+ withV2Table(table) { tableId =>
+ sql(s"CREATE TABLE IF NOT EXISTS $tableId (key int, value String) USING iceberg")
+ sql(s"INSERT INTO $tableId VALUES (1, 'a'), (2, 'b'), (3, 'c')")
+
+ val plan = sql(s"CALL $catalogV2.system.rewrite_data_files " +
+ s"(table => '$tableId', options => map('min-input-files','2')) ").queryExecution.analyzed
+ val (inputs, outputs, operationType) = PrivilegesBuilder.build(plan, spark)
+ assert(operationType === QUERY)
+ assert(outputs.size === 1)
+ val po = outputs.head
+ assert(po.actionType === PrivilegeObjectActionType.UPDATE)
+ assert(po.privilegeObjectType === PrivilegeObjectType.TABLE_OR_VIEW)
+ assertEqualsIgnoreCase(namespace)(po.dbname)
+ assertEqualsIgnoreCase(tableId.split("\\.").last)(po.objectName)
+ assert(inputs.size === 0)
Review Comment:
The SELECT query access privilege of the source table should be required when calling the table rewriting produce.
--
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
Re: [PR] [AUTHZ] Check previleges for Iceberg's CALL to RewriteDataFilesProcedure procedure [kyuubi]
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#issuecomment-1844067811
> Following the TableExtractor adjustments it seems the Iceberg metadata tables (snapshots,manifests,history) are being validated for authorization . Is this expected? Thanks a lot or the help and feedback
#3931 may help, but it has not been accepted.
--
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
Re: [PR] [AUTHZ] Check previleges for Iceberg's CALL to RewriteDataFilesProcedure procedure [kyuubi]
Posted by "elisabetao (via GitHub)" <gi...@apache.org>.
elisabetao commented on PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#issuecomment-1843611203
Following the TableExtractor adjustments it seems the Iceberg metadata tables (snapshots,manifests,history) are being validated for authorization . Is this expected? Thanks a lot or the help and feedback
--
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] [kyuubi] yabola commented on a diff in pull request #5248: [AUTHZ] Check previleges for Iceberg's CALL to RewriteDataFilesProcedure procedure
Posted by "yabola (via GitHub)" <gi...@apache.org>.
yabola commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1322287343
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/IcebergCatalogRangerSparkExtensionSuite.scala:
##########
@@ -226,4 +227,74 @@ class IcebergCatalogRangerSparkExtensionSuite extends RangerSparkExtensionSuite
assert(e1.getMessage.contains(s"does not have [select] privilege" +
s" on [$namespace1/$table1]"))
}
+
+ test("CALL RewriteDataFilesProcedure") {
+ val tableName = "table_select_call_command_table"
+ val table = s"$catalogV2.$namespace1.$tableName"
+ val rewriteDataFiles1 = s"CALL $catalogV2.system.rewrite_data_files " +
+ s"(table => '$table', options => map('min-input-files','2'))"
+ val rewriteDataFiles2 = s"CALL $catalogV2.system.rewrite_data_files " +
+ s"(table => '$table', options => map('min-input-files','3'))"
+
+ withCleanTmpResources(Seq((table, "table"))) {
+ doAs(
+ admin, {
+ sql(s"CREATE TABLE IF NOT EXISTS $table (id int, name string) USING iceberg")
+ sql(s"INSERT INTO $table VALUES (1, 'chenliang'), (2, 'tom')")
+ sql(s"INSERT INTO $table VALUES (3, 'julie'), (4, 'ross')")
+ })
+
+ // user `user_read_only_on_iceberg` has select permission on database `iceberg_ns`
+ interceptContains[AccessControlException](doAs(icebergReadOnlyUser, sql(rewriteDataFiles1)))(
+ s"does not have [update] privilege on [$namespace1/$tableName]")
+ interceptContains[AccessControlException](doAs(icebergReadOnlyUser, sql(rewriteDataFiles2)))(
+ s"does not have [update] privilege on [$namespace1/$tableName]")
+
+ // user `someone` has no permission on table
+ interceptContains[AccessControlException](doAs(someone, sql(rewriteDataFiles1)))(
+ s"does not have [select] privilege on [$namespace1/$tableName]")
+ interceptContains[AccessControlException](doAs(someone, sql(rewriteDataFiles2)))(
+ s"does not have [select] privilege on [$namespace1/$tableName]")
+
+ withSingleCallEnabled {
+ interceptContains[AccessControlException](doAs(someone, sql(rewriteDataFiles1).explain()))(
+ s"does not have [select] privilege on [$namespace1/$tableName]," +
+ s" [update] privilege on [$namespace1/$tableName]")
+ }
+
+ /**
+ * Situation 1:
+ * Two logical plans triggered
+ * when ( input-files(2) >= min-input-files(2) ):
+ *
+ * == Physical Plan 1 ==
+ * Call (1)
+ *
+ * == Physical Plan 2 ==
+ * AppendData (3)
+ * +- * ColumnarToRow (2)
+ * +- BatchScan local.iceberg_ns.call_command_table (1)
+ */
+ doAs(
+ admin, {
+ val result2 = sql(rewriteDataFiles1).collect()
+ // rewrite 2 files
Review Comment:
It means number of data files which were re-written by this command. The explanation of the output of this command can refer to https://iceberg.apache.org/docs/latest/spark-procedures/#rewrite_data_files
--
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] [kyuubi] bowenliang123 commented on a diff in pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1318182303
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/serde/tableExtractors.scala:
##########
@@ -145,14 +175,37 @@ class DataSourceV2RelationTableExtractor extends TableExtractor {
val maybeCatalogPlugin = invokeAs[Option[AnyRef]](v2Relation, "catalog")
val maybeCatalog = maybeCatalogPlugin.flatMap(catalogPlugin =>
lookupExtractor[CatalogPluginCatalogExtractor].apply(catalogPlugin))
- val maybeIdentifier = invokeAs[Option[AnyRef]](v2Relation, "identifier")
Review Comment:
Oh, got your point. Iceberg procedure rewrites this identifier, and makes it irrelevant to the original table.
--
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] [kyuubi] pan3793 commented on pull request #5248: [Draft]Support iceberg call statement in ranger authentication
Posted by "pan3793 (via GitHub)" <gi...@apache.org>.
pan3793 commented on PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#issuecomment-1705076808
@yabola thanks for your contribution. You may want to send a minor PR(e.g. bump iceberg from 1.3.0 to 1.3.1) first to unblock the CI (auto-run without approval)
--
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] [kyuubi] yabola commented on a diff in pull request #5248: [Draft]Support iceberg call statement in ranger authentication
Posted by "yabola (via GitHub)" <gi...@apache.org>.
yabola commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1315367863
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/serde/tableExtractors.scala:
##########
@@ -132,6 +134,34 @@ class IdentifierTableExtractor extends TableExtractor {
}
}
+class IcebergCallCommandExtractor extends TableExtractor {
+ override def apply(spark: SparkSession, callStatementArgs: AnyRef): Option[Table] = {
+ val literals = callStatementArgs.asInstanceOf[Seq[Literal]]
+ if (literals.isEmpty) {
+ throw new IllegalArgumentException(
+ s"Unknown parameters in iceberg call command: $callStatementArgs")
+ }
+ // iceberg will rearrange the parameters according to the parameter order
+ // defined in the procedure, where the table parameters are currently always the first.
+ lookupExtractor[IcebergTableExtractor].apply(spark, literals.head)
Review Comment:
iceberg will rearrange the parameters according to the parameter order defined in the procedure, where the table parameters are currently always the first.
For example `RewriteDataFilesProcedure#PARAMETERS` and in `RewriteDataFilesProcedure#call` take the first parameter.
##########
extensions/spark/kyuubi-spark-authz/src/main/scala/org/apache/kyuubi/plugin/spark/authz/serde/tableExtractors.scala:
##########
@@ -145,14 +175,37 @@ class DataSourceV2RelationTableExtractor extends TableExtractor {
val maybeCatalogPlugin = invokeAs[Option[AnyRef]](v2Relation, "catalog")
val maybeCatalog = maybeCatalogPlugin.flatMap(catalogPlugin =>
lookupExtractor[CatalogPluginCatalogExtractor].apply(catalogPlugin))
- val maybeIdentifier = invokeAs[Option[AnyRef]](v2Relation, "identifier")
Review Comment:
The `identifier` in `DataSourceV2Relation` is not reliable enough. In fact, datasourceV2 will parse out the table based on identifier, so table is more appropriate here.
For example, in iceberg `rewrite_data_files` command, `identifier` is rewrite task id, iceberg catalog will load table by it ( can see in `SparkCatalog#load` the `isPathIdentifier` function)
--
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] [kyuubi] bowenliang123 commented on pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#issuecomment-1706325818
I misunderstood the changes in `tableExtractors`, which turns out the changes are applied in DataSourceV2RelationTableExtractor. But again, it's not a best practice to couple to Iceberg's detail here. Let's think again.
--
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] [kyuubi] yabola commented on pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "yabola (via GitHub)" <gi...@apache.org>.
yabola commented on PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#issuecomment-1713250342
@bowenliang123 Thank you for adding new ranger policy for UT and the relevant code improvements. And is UT failure due to instability?
--
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] [kyuubi] yabola commented on pull request #5248: [AUTHZ] Check previleges for Iceberg's CALL to RewriteDataFilesProcedure procedure
Posted by "yabola (via GitHub)" <gi...@apache.org>.
yabola commented on PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#issuecomment-1715266331
> ```
> "Permission denied: user [someone] does not have [alter] privilege on [iceberg_ns/table_select_call_command_table]"
> did not contain
> "does not have [select] privilege on [iceberg_ns/table_select_call_command_table], [update] privilege on [iceberg_ns/table_select_call_command_table]"
> ```
>
> PrivilegesBuilder extracted two privilege objects for input and output. But their action type both are `PrivilegeObjectActionType.OTHER` (Possible values of PrivilegeObjectActionType are OTHER, INSERT, INSERT_OVERWRITE, UPDATE, DELETE). Therefore, RuleAuthorization would then treat them as the same privilege object, and check the `alter` privilege only.
>
> 1. it seems we could not distinguish them, by 1 for `select` on input table and 1 for `alter` on output table. cc @yaooqinn
> 2. Or let's check alter privilege only, and skip checking `select` privilege, as the same and only table used in one procedure, and no data changed by calling the procedure. @yabola
I think having a single authority is more direct and easier for people to understand.
--
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] [kyuubi] codecov-commenter commented on pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#issuecomment-1708694978
## [Codecov](https://app.codecov.io/gh/apache/kyuubi/pull/5248?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
> Merging [#5248](https://app.codecov.io/gh/apache/kyuubi/pull/5248?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (0deeffd) into [master](https://app.codecov.io/gh/apache/kyuubi/commit/6a23f88b00e03b36f8f68c526128b166f866dc6f?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) (6a23f88) will **not change** coverage.
> Report is 3 commits behind head on master.
> The diff coverage is `0.00%`.
```diff
@@ Coverage Diff @@
## master #5248 +/- ##
======================================
Coverage 0.00% 0.00%
======================================
Files 590 590
Lines 33340 33354 +14
Branches 4405 4407 +2
======================================
- Misses 33340 33354 +14
```
| [Files Changed](https://app.codecov.io/gh/apache/kyuubi/pull/5248?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [...ubi/plugin/spark/authz/serde/tableExtractors.scala](https://app.codecov.io/gh/apache/kyuubi/pull/5248?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1Ymktc3BhcmstYXV0aHovc3JjL21haW4vc2NhbGEvb3JnL2FwYWNoZS9reXV1YmkvcGx1Z2luL3NwYXJrL2F1dGh6L3NlcmRlL3RhYmxlRXh0cmFjdG9ycy5zY2FsYQ==) | `0.00% <0.00%> (ø)` | |
| [...in/lineage/helper/SparkSQLLineageParseHelper.scala](https://app.codecov.io/gh/apache/kyuubi/pull/5248?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-ZXh0ZW5zaW9ucy9zcGFyay9reXV1Ymktc3BhcmstbGluZWFnZS9zcmMvbWFpbi9zY2FsYS9vcmcvYXBhY2hlL2t5dXViaS9wbHVnaW4vbGluZWFnZS9oZWxwZXIvU3BhcmtTUUxMaW5lYWdlUGFyc2VIZWxwZXIuc2NhbGE=) | `0.00% <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=apache)
--
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] [kyuubi] bowenliang123 commented on pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#issuecomment-1709410253
Pleased to see the progress of the added e2e tests which passed CI tests.
Still some concerns in the following changes, that I may have further investigation or proof.
1. Again, change the source field inside the `DataSourceV2RelationTableExtractor`, from "identifier" to `Table` "table"
2. the introduced `StringTableExtractor`
3. forced casting to Literal in `IcebergCallArgsTableExtractor `
--
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] [kyuubi] bowenliang123 commented on pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#issuecomment-1713253516
> @bowenliang123 Thank you for adding new ranger policy for UT and the relevant code improvements. And is UT failure due to instability?
Hi, updated the table spec json just now. I changed the operation time from QUERY to ALTERTABLE_ADDPARTS, which is not a best one but a closer one.
--
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] [kyuubi] bowenliang123 commented on a diff in pull request #5248: [AUTHZ] Check previleges for Iceberg's CALL to RewriteDataFilesProcedure procedure
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1322601989
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/IcebergCatalogRangerSparkExtensionSuite.scala:
##########
@@ -226,4 +227,74 @@ class IcebergCatalogRangerSparkExtensionSuite extends RangerSparkExtensionSuite
assert(e1.getMessage.contains(s"does not have [select] privilege" +
s" on [$namespace1/$table1]"))
}
+
+ test("CALL RewriteDataFilesProcedure") {
+ val tableName = "table_select_call_command_table"
+ val table = s"$catalogV2.$namespace1.$tableName"
+ val rewriteDataFiles1 = s"CALL $catalogV2.system.rewrite_data_files " +
+ s"(table => '$table', options => map('min-input-files','2'))"
+ val rewriteDataFiles2 = s"CALL $catalogV2.system.rewrite_data_files " +
+ s"(table => '$table', options => map('min-input-files','3'))"
+
+ withCleanTmpResources(Seq((table, "table"))) {
+ doAs(
+ admin, {
+ sql(s"CREATE TABLE IF NOT EXISTS $table (id int, name string) USING iceberg")
+ sql(s"INSERT INTO $table VALUES (1, 'chenliang'), (2, 'tom')")
+ sql(s"INSERT INTO $table VALUES (3, 'julie'), (4, 'ross')")
+ })
+
+ // user `user_read_only_on_iceberg` has select permission on database `iceberg_ns`
+ interceptContains[AccessControlException](doAs(icebergReadOnlyUser, sql(rewriteDataFiles1)))(
+ s"does not have [update] privilege on [$namespace1/$tableName]")
+ interceptContains[AccessControlException](doAs(icebergReadOnlyUser, sql(rewriteDataFiles2)))(
+ s"does not have [update] privilege on [$namespace1/$tableName]")
+
+ // user `someone` has no permission on table
+ interceptContains[AccessControlException](doAs(someone, sql(rewriteDataFiles1)))(
+ s"does not have [select] privilege on [$namespace1/$tableName]")
+ interceptContains[AccessControlException](doAs(someone, sql(rewriteDataFiles2)))(
+ s"does not have [select] privilege on [$namespace1/$tableName]")
+
+ withSingleCallEnabled {
+ interceptContains[AccessControlException](doAs(someone, sql(rewriteDataFiles1).explain()))(
+ s"does not have [select] privilege on [$namespace1/$tableName]," +
+ s" [update] privilege on [$namespace1/$tableName]")
+ }
+
+ /**
+ * Situation 1:
+ * Two logical plans triggered
+ * when ( input-files(2) >= min-input-files(2) ):
+ *
+ * == Physical Plan 1 ==
+ * Call (1)
+ *
+ * == Physical Plan 2 ==
+ * AppendData (3)
+ * +- * ColumnarToRow (2)
+ * +- BatchScan local.iceberg_ns.call_command_table (1)
+ */
+ doAs(
+ admin, {
+ val result2 = sql(rewriteDataFiles1).collect()
+ // rewrite 2 files
Review Comment:
OK, thanks. Please mention these details in the description.
--
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] [kyuubi] bowenliang123 commented on a diff in pull request #5248: [AUTHZ] Check previleges for Iceberg's CALL to RewriteDataFilesProcedure procedure
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1321344053
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/IcebergCatalogRangerSparkExtensionSuite.scala:
##########
@@ -226,4 +227,71 @@ class IcebergCatalogRangerSparkExtensionSuite extends RangerSparkExtensionSuite
assert(e1.getMessage.contains(s"does not have [select] privilege" +
s" on [$namespace1/$table1]"))
}
+
+ test("CALL RewriteDataFilesProcedure") {
+ val tableName = "table_select_call_command_table"
+ val table = s"$catalogV2.$namespace1.$tableName"
+ val rewriteDataFiles1 = s"CALL $catalogV2.system.rewrite_data_files " +
+ s"(table => '$table', options => map('min-input-files','2'))"
+ val rewriteDataFiles2 = s"CALL $catalogV2.system.rewrite_data_files " +
+ s"(table => '$table', options => map('min-input-files','3'))"
+
+ withCleanTmpResources(Seq((table, "table"))) {
+ doAs(
+ admin, {
+ sql(s"CREATE TABLE IF NOT EXISTS $table (id int, name string) USING iceberg")
+ sql(s"INSERT INTO $table VALUES (1, 'chenliang'), (2, 'tom')")
+ sql(s"INSERT INTO $table VALUES (3, 'julie'), (4, 'ross')")
+ })
+
+ // user bob has select permission on table
+ interceptContains[AccessControlException](doAs(icebergReadOnlyUser, sql(rewriteDataFiles1)))(
+ s"does not have [update] privilege on [$namespace1/$tableName]")
+ interceptContains[AccessControlException](doAs(icebergReadOnlyUser, sql(rewriteDataFiles2)))(
+ s"does not have [update] privilege on [$namespace1/$tableName]")
+
+ // user someone has no permission on table
+ interceptContains[AccessControlException](doAs(someone, sql(rewriteDataFiles1)))(
+ s"does not have [select] privilege on [$namespace1/$tableName]")
+ interceptContains[AccessControlException](doAs(someone, sql(rewriteDataFiles2)))(
+ s"does not have [select] privilege on [$namespace1/$tableName]")
+
+ withSingleCallEnabled {
+ interceptContains[AccessControlException](doAs(someone, sql(rewriteDataFiles1).explain()))(
+ s"does not have [select] privilege on [$namespace1/$tableName]," +
+ s" [update] privilege on [$namespace1/$tableName]")
+ }
+
+ /**
+ * Situation 1:
+ * Two logical plans triggered
+ * when ( input-files(2) >= min-input-files(2) ):
+ *
+ * == Physical Plan 1 ==
+ * (1) Call
+ *
+ * == Physical Plan 2 ==
+ * AppendData (3)
+ * +- * ColumnarToRow (2)
+ * +- BatchScan local.iceberg_ns.call_command_table (1)
+ */
+ doAs(
+ admin, {
+ val result2 = sql(rewriteDataFiles1).collect()
+ // rewrite 2 files
+ assert(result2(0)(0) === 2)
+ })
+
+ /**
+ * Situation 2:
+ * Only one logical plan triggered
+ * when ( input-files(2) < min-input-files(3) )
Review Comment:
Could you attach the triggered pysical plan for this situation as well? @yabola
--
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] [kyuubi] yabola commented on a diff in pull request #5248: [AUTHZ] Check previleges for Iceberg's CALL to RewriteDataFilesProcedure procedure
Posted by "yabola (via GitHub)" <gi...@apache.org>.
yabola commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1321441282
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/IcebergCatalogRangerSparkExtensionSuite.scala:
##########
@@ -226,4 +227,71 @@ class IcebergCatalogRangerSparkExtensionSuite extends RangerSparkExtensionSuite
assert(e1.getMessage.contains(s"does not have [select] privilege" +
s" on [$namespace1/$table1]"))
}
+
+ test("CALL RewriteDataFilesProcedure") {
+ val tableName = "table_select_call_command_table"
+ val table = s"$catalogV2.$namespace1.$tableName"
+ val rewriteDataFiles1 = s"CALL $catalogV2.system.rewrite_data_files " +
+ s"(table => '$table', options => map('min-input-files','2'))"
+ val rewriteDataFiles2 = s"CALL $catalogV2.system.rewrite_data_files " +
+ s"(table => '$table', options => map('min-input-files','3'))"
+
+ withCleanTmpResources(Seq((table, "table"))) {
+ doAs(
+ admin, {
+ sql(s"CREATE TABLE IF NOT EXISTS $table (id int, name string) USING iceberg")
+ sql(s"INSERT INTO $table VALUES (1, 'chenliang'), (2, 'tom')")
+ sql(s"INSERT INTO $table VALUES (3, 'julie'), (4, 'ross')")
+ })
+
+ // user bob has select permission on table
+ interceptContains[AccessControlException](doAs(icebergReadOnlyUser, sql(rewriteDataFiles1)))(
+ s"does not have [update] privilege on [$namespace1/$tableName]")
+ interceptContains[AccessControlException](doAs(icebergReadOnlyUser, sql(rewriteDataFiles2)))(
+ s"does not have [update] privilege on [$namespace1/$tableName]")
+
+ // user someone has no permission on table
+ interceptContains[AccessControlException](doAs(someone, sql(rewriteDataFiles1)))(
+ s"does not have [select] privilege on [$namespace1/$tableName]")
+ interceptContains[AccessControlException](doAs(someone, sql(rewriteDataFiles2)))(
+ s"does not have [select] privilege on [$namespace1/$tableName]")
+
+ withSingleCallEnabled {
+ interceptContains[AccessControlException](doAs(someone, sql(rewriteDataFiles1).explain()))(
+ s"does not have [select] privilege on [$namespace1/$tableName]," +
+ s" [update] privilege on [$namespace1/$tableName]")
+ }
+
+ /**
+ * Situation 1:
+ * Two logical plans triggered
+ * when ( input-files(2) >= min-input-files(2) ):
+ *
+ * == Physical Plan 1 ==
+ * (1) Call
+ *
+ * == Physical Plan 2 ==
+ * AppendData (3)
+ * +- * ColumnarToRow (2)
+ * +- BatchScan local.iceberg_ns.call_command_table (1)
+ */
+ doAs(
+ admin, {
+ val result2 = sql(rewriteDataFiles1).collect()
+ // rewrite 2 files
+ assert(result2(0)(0) === 2)
+ })
+
+ /**
+ * Situation 2:
+ * Only one logical plan triggered
+ * when ( input-files(2) < min-input-files(3) )
Review Comment:
I have added the physical plan.
--
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] [kyuubi] bowenliang123 commented on a diff in pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1315684686
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/gen/IcebergCommands.scala:
##########
@@ -49,10 +49,22 @@ object IcebergCommands {
TableCommandSpec(cmd, Seq(tableDesc), queryDescs = Seq(queryDesc))
}
+ val IcebergCallCommand = {
+ val cmd = "org.apache.spark.sql.catalyst.plans.logical.Call"
+ val actionTypeDesc = ActionTypeDesc(actionType = Some(INSERT))
Review Comment:
`UPDATE` is the more proper one for the action type.
--
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] [kyuubi] yabola commented on pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "yabola (via GitHub)" <gi...@apache.org>.
yabola commented on PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#issuecomment-1706316310
1. I have moved to `IcebergCallCommand`. Sorry didn't notice...
3. Yes, there should share the same common structure. I think we'd better use the `table` parameter instead of `identifier` here. The reason:
> The identifier in DataSourceV2Relation is not reliable enough. In fact, datasourceV2 will parse out the table based on identifier, so table is more appropriate here.
For example, in iceberg rewrite_data_files command, identifier is rewrite task id, iceberg catalog will load table by it ( can see in SparkCatalog#load the isPathIdentifier function)
--
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] [kyuubi] yabola commented on a diff in pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "yabola (via GitHub)" <gi...@apache.org>.
yabola commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1315734003
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/gen/IcebergCommands.scala:
##########
@@ -49,10 +49,22 @@ object IcebergCommands {
TableCommandSpec(cmd, Seq(tableDesc), queryDescs = Seq(queryDesc))
}
+ val IcebergCallCommand = {
+ val cmd = "org.apache.spark.sql.catalyst.plans.logical.Call"
+ val actionTypeDesc = ActionTypeDesc(actionType = Some(INSERT))
Review Comment:
Thanks, done. And I also change the `DataSourceV2RelationTableExtractor`. In spark `org.apache.spark.sql.execution.datasources.v2.DataSourceV2Relation#table` will always exist (not option)
--
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] [kyuubi] yabola commented on a diff in pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "yabola (via GitHub)" <gi...@apache.org>.
yabola commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1315734003
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/gen/IcebergCommands.scala:
##########
@@ -49,10 +49,22 @@ object IcebergCommands {
TableCommandSpec(cmd, Seq(tableDesc), queryDescs = Seq(queryDesc))
}
+ val IcebergCallCommand = {
+ val cmd = "org.apache.spark.sql.catalyst.plans.logical.Call"
+ val actionTypeDesc = ActionTypeDesc(actionType = Some(INSERT))
Review Comment:
Thanks, done. And I also change the `DataSourceV2RelationTableExtractor`, please also take a look~
--
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] [kyuubi] bowenliang123 commented on a diff in pull request #5248: [AUTHZ] Support checking previleges for Iceberg's CALL statements to system procedures
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1319594787
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/IcebergCatalogRangerSparkExtensionSuite.scala:
##########
@@ -235,13 +235,15 @@ class IcebergCatalogRangerSparkExtensionSuite extends RangerSparkExtensionSuite
s" on [$namespace1/$table1]"))
}
- test("CALL RewriteDataFilesProcedure") {
- val tableName = "call_command_table"
- val table = s"$catalogV2.$namespace1.$tableName"
- val rewriteCommand1 = s"CALL $catalogV2.system.rewrite_data_files " +
+ test("CALL RewriteProcedure") {
Review Comment:
We'd better use RewriteDataFilesProcedure, refering to `org.apache.iceberg.spark.procedures.RewriteDataFilesProcedure` from Iceberg Spark plugin.
--
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] [kyuubi] bowenliang123 commented on a diff in pull request #5248: [AUTHZ] Check previleges for Iceberg's CALL to RewriteDataFilesProcedure procedure
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on code in PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#discussion_r1322241885
##########
extensions/spark/kyuubi-spark-authz/src/test/scala/org/apache/kyuubi/plugin/spark/authz/ranger/IcebergCatalogRangerSparkExtensionSuite.scala:
##########
@@ -226,4 +227,74 @@ class IcebergCatalogRangerSparkExtensionSuite extends RangerSparkExtensionSuite
assert(e1.getMessage.contains(s"does not have [select] privilege" +
s" on [$namespace1/$table1]"))
}
+
+ test("CALL RewriteDataFilesProcedure") {
+ val tableName = "table_select_call_command_table"
+ val table = s"$catalogV2.$namespace1.$tableName"
+ val rewriteDataFiles1 = s"CALL $catalogV2.system.rewrite_data_files " +
+ s"(table => '$table', options => map('min-input-files','2'))"
+ val rewriteDataFiles2 = s"CALL $catalogV2.system.rewrite_data_files " +
+ s"(table => '$table', options => map('min-input-files','3'))"
+
+ withCleanTmpResources(Seq((table, "table"))) {
+ doAs(
+ admin, {
+ sql(s"CREATE TABLE IF NOT EXISTS $table (id int, name string) USING iceberg")
+ sql(s"INSERT INTO $table VALUES (1, 'chenliang'), (2, 'tom')")
+ sql(s"INSERT INTO $table VALUES (3, 'julie'), (4, 'ross')")
+ })
+
+ // user `user_read_only_on_iceberg` has select permission on database `iceberg_ns`
+ interceptContains[AccessControlException](doAs(icebergReadOnlyUser, sql(rewriteDataFiles1)))(
+ s"does not have [update] privilege on [$namespace1/$tableName]")
+ interceptContains[AccessControlException](doAs(icebergReadOnlyUser, sql(rewriteDataFiles2)))(
+ s"does not have [update] privilege on [$namespace1/$tableName]")
+
+ // user `someone` has no permission on table
+ interceptContains[AccessControlException](doAs(someone, sql(rewriteDataFiles1)))(
+ s"does not have [select] privilege on [$namespace1/$tableName]")
+ interceptContains[AccessControlException](doAs(someone, sql(rewriteDataFiles2)))(
+ s"does not have [select] privilege on [$namespace1/$tableName]")
+
+ withSingleCallEnabled {
+ interceptContains[AccessControlException](doAs(someone, sql(rewriteDataFiles1).explain()))(
+ s"does not have [select] privilege on [$namespace1/$tableName]," +
+ s" [update] privilege on [$namespace1/$tableName]")
+ }
+
+ /**
+ * Situation 1:
+ * Two logical plans triggered
+ * when ( input-files(2) >= min-input-files(2) ):
+ *
+ * == Physical Plan 1 ==
+ * Call (1)
+ *
+ * == Physical Plan 2 ==
+ * AppendData (3)
+ * +- * ColumnarToRow (2)
+ * +- BatchScan local.iceberg_ns.call_command_table (1)
+ */
+ doAs(
+ admin, {
+ val result2 = sql(rewriteDataFiles1).collect()
+ // rewrite 2 files
Review Comment:
Do you mean 'result to 2 rewritten data files' or 'exited 2 files before/after procedure'?
--
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] [kyuubi] bowenliang123 commented on pull request #5248: [AUTHZ] Check previleges for Iceberg's CALL to RewriteDataFilesProcedure procedure
Posted by "bowenliang123 (via GitHub)" <gi...@apache.org>.
bowenliang123 commented on PR #5248:
URL: https://github.com/apache/kyuubi/pull/5248#issuecomment-1717033173
Thank you for your contribution and patience. Looking forward to your follow-up PRs in improving Authz support for Iceberg.
Merged to master (1.9.0).
--
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