[#5180][part-1] improvement(test): Add more integration tests about access control #5190

jerqi · 2024-10-21T09:28:37Z

What changes were proposed in this pull request?

Add more integration tests about access control.
Fix the can't create a role in authorization plugin with CREATE_ROLE, MANAGE_USERS.
Fix creating a role contains metalake, it won't take effect.

Why are the changes needed?

Fix: #5180

Does this PR introduce any user-facing change?

No.

How was this patch tested?

Just tests.

jerryshao

Just two general comments on IT, for the Ranger related code it would be better @xunliu can help to review.

jerryshao · 2024-10-25T09:01:34Z

...rc/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveE2EIT.java

+    // the policy Sleep time must be greater than the policy update interval
+    // (ranger.plugin.spark.policy.pollIntervalMs) in the
+    // `resources/ranger-spark-security.xml.template`
+    Thread.sleep(1000L);


How can you make sure that 1000 is enough?

It's twice of update interval.

jerryshao · 2024-10-25T09:03:19Z

...rc/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveE2EIT.java

+      }
+    } catch (Exception e) {
+      throw new IllegalStateException("Failed to set environment variable", e);
+    }


Can you please also add the tests about alter table, and also verify the ownership mechanism if possible?

I have added the case about alter table. You can see 332 Line.
The ownership is related to rename/delete the metadata object. I will add it later.

xunliu · 2024-10-28T04:03:00Z

...rc/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveE2EIT.java

@@ -122,6 +143,8 @@ public void startIntegrationTest() throws Exception {

    generateRangerSparkSecurityXML();

+    setEnv(HADOOP_USER_NAME, "test");


I think setEnv() function will change the OS environment variable. Maybe affected other IT case. So I think we should use environment variable in Gradle script.

xunliu · 2024-10-28T04:09:43Z

...rc/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveE2EIT.java

+    String readWriteRole = "readWriteRole";
+    SecurableObject securableObject =
+        SecurableObjects.ofMetalake(
+            metalakeName,
+            Lists.newArrayList(
+                Privileges.UseSchema.allow(),
+                Privileges.CreateSchema.allow(),
+                Privileges.CreateTable.allow(),
+                Privileges.SelectTable.allow(),
+                Privileges.ModifyTable.allow()));
+    String userName1 = System.getenv(HADOOP_USER_NAME);
+    metalake.createRole(readWriteRole, Collections.emptyMap(), Lists.newArrayList(securableObject));
+    metalake.grantRolesToUser(Lists.newArrayList(readWriteRole), userName1);


I think we need to add some AccessControlException tests before the grant role.

These cases have been tested by the method testCreateTable.

xunliu · 2024-10-28T04:15:01Z

...rc/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveE2EIT.java

+  @Test
+  void testCreateTable() throws InterruptedException {
+    // First, create a role for creating a database and grant role to the user
+    String createSchemaRole = "createSchemaRole";


I think we can use currentFunName() replace "createSchemaRole", keep role name unique.
The same can be done for role names in other test functions

xunliu · 2024-10-28T04:18:32Z

...rc/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveE2EIT.java

+    // Granted this role to the spark execution user `HADOOP_USER_NAME`
+    String userName1 = System.getenv(HADOOP_USER_NAME);
+    metalake.grantRolesToUser(Lists.newArrayList(roleName), userName1);
+


I think it better to add some Spark tests in the here, confirm the role is created successfully.

xunliu · 2024-10-28T04:19:41Z

...rc/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveE2EIT.java

+
+    // Delete the role
+    metalake.deleteRole(roleName);
+


I think it better to add some Spark tests in the here, confirm the role is deleted successfully.

xunliu · 2024-10-28T04:19:56Z

...rc/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveE2EIT.java

+
+    // Grant the role again
+    metalake.grantRolesToUser(Lists.newArrayList(roleName), userName1);
+


I think it better to add some Spark tests in the here, confirm the role is created successfully again.

xunliu · 2024-10-28T09:48:42Z

build.gradle.kts

-      param.environment("HADOOP_USER_NAME", "anonymous")
+      if (project.name == "authorization-ranger") {
+        param.environment("HADOOP_USER_NAME", "test")
+      } else {
+        param.environment("HADOOP_USER_NAME", "anonymous")
+      }


I think we can user-define environment variable HADOOP_USER_NAME in the submodule

// If the environment variable `HADOOP_USER_NAME` is not customized in submodule, // then set it to "anonymous" if (param.environment["HADOOP_USER_NAME"] == null) { param.environment("HADOOP_USER_NAME", "anonymous") }

and add these code in the authorization-ranger submodule

tasks.test { ...... doFirst { environment("HADOOP_USER_NAME", "test") } }

xunliu

LGTM

…ccess control (#5190) ### What changes were proposed in this pull request? 1) Add more integration tests about access control. 2) Fix the can't create a role in authorization plugin with `CREATE_ROLE`, `MANAGE_USERS`. 3) Fix creating a role contains metalake, it won't take effect. ### Why are the changes needed? Fix: #5180 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Just tests.

…bout access control (apache#5190) ### What changes were proposed in this pull request? 1) Add more integration tests about access control. 2) Fix the can't create a role in authorization plugin with `CREATE_ROLE`, `MANAGE_USERS`. 3) Fix creating a role contains metalake, it won't take effect. ### Why are the changes needed? Fix: apache#5180 ### Does this PR introduce _any_ user-facing change? No. ### How was this patch tested? Just tests.

jerqi marked this pull request as draft October 21, 2024 09:28

jerqi self-assigned this Oct 21, 2024

jerqi force-pushed the ISSUE-5180 branch 2 times, most recently from cf024cf to 804bb2f Compare October 25, 2024 06:13

jerqi changed the title ~~[#5180] improvement(test): Add more integration tests about access control~~ [#5180] improvement(test): Add more integration tests about access control (part I) Oct 25, 2024

jerqi added the branch-0.7 Automatically cherry-pick commit to branch-0.7 label Oct 25, 2024

jerqi changed the title ~~[#5180] improvement(test): Add more integration tests about access control (part I)~~ [#5180][part-1] improvement(test): Add more integration tests about access control Oct 25, 2024

jerqi requested review from xunliu, yuqi1129 and jerryshao October 25, 2024 06:17

jerqi marked this pull request as ready for review October 25, 2024 06:17

jerqi force-pushed the ISSUE-5180 branch 2 times, most recently from 1bde48c to c56df51 Compare October 25, 2024 06:24

jerqi added 3 commits October 25, 2024 15:31

Add more tests

54f3f3b

revert some test cases

fb3ab74

fix ut

b1e8da6

jerqi force-pushed the ISSUE-5180 branch from 68f6415 to b1e8da6 Compare October 25, 2024 07:32

fix style

001fa8b

jerryshao reviewed Oct 25, 2024

View reviewed changes

xunliu reviewed Oct 28, 2024

View reviewed changes

jerqi added 2 commits October 28, 2024 14:50

address comment

fc42ddc

address comment

f7048ee

jerqi closed this Oct 28, 2024

jerqi reopened this Oct 28, 2024

xunliu reviewed Oct 28, 2024

View reviewed changes

address comment

696c59b

xunliu approved these changes Oct 28, 2024

View reviewed changes

xunliu merged commit b5ff4ba into apache:main Oct 28, 2024
26 checks passed

jerqi mentioned this pull request Oct 28, 2024

[Subtask] Fail to grant a role with CREATE_ROLE #5215

Closed

jerqi mentioned this pull request Oct 28, 2024

[Subtask] Ignore privileges in RangerAuthorization that are not supported #4617

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[#5180][part-1] improvement(test): Add more integration tests about access control #5190

[#5180][part-1] improvement(test): Add more integration tests about access control #5190

jerqi commented Oct 21, 2024 •

edited

Loading

jerryshao left a comment

jerryshao Oct 25, 2024

jerqi Oct 25, 2024

jerryshao Oct 25, 2024

jerqi Oct 25, 2024

xunliu Oct 28, 2024

jerqi Oct 28, 2024

xunliu Oct 28, 2024

jerqi Oct 28, 2024

xunliu Oct 28, 2024

jerqi Oct 28, 2024

xunliu Oct 28, 2024

jerqi Oct 28, 2024

xunliu Oct 28, 2024

jerqi Oct 28, 2024

xunliu Oct 28, 2024

jerqi Oct 28, 2024

xunliu Oct 28, 2024

jerqi Oct 28, 2024

xunliu left a comment

		@@ -122,6 +143,8 @@ public void startIntegrationTest() throws Exception {

		generateRangerSparkSecurityXML();

		setEnv(HADOOP_USER_NAME, "test");


		// Grant the role again
		metalake.grantRolesToUser(Lists.newArrayList(roleName), userName1);

[#5180][part-1] improvement(test): Add more integration tests about access control #5190

[#5180][part-1] improvement(test): Add more integration tests about access control #5190

Conversation

jerqi commented Oct 21, 2024 • edited Loading

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

jerryshao left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xunliu left a comment

Choose a reason for hiding this comment

jerqi commented Oct 21, 2024 •

edited

Loading