Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 7 commits into from
Oct 28, 2024

Conversation

jerqi
Copy link
Collaborator

@jerqi jerqi commented Oct 21, 2024

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.

@jerqi jerqi marked this pull request as draft October 21, 2024 09:28
@jerqi jerqi self-assigned this Oct 21, 2024
@jerqi jerqi force-pushed the ISSUE-5180 branch 2 times, most recently from cf024cf to 804bb2f Compare October 25, 2024 06:13
@jerqi 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 jerqi added the branch-0.7 Automatically cherry-pick commit to branch-0.7 label Oct 25, 2024
@jerqi 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 jerqi marked this pull request as ready for review October 25, 2024 06:17
@jerqi jerqi force-pushed the ISSUE-5180 branch 2 times, most recently from 1bde48c to c56df51 Compare October 25, 2024 06:24
Copy link
Contributor

@jerryshao jerryshao left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How can you make sure that 1000 is enough?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's twice of update interval.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

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

generateRangerSparkSecurityXML();

setEnv(HADOOP_USER_NAME, "test");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed.

Comment on lines 298 to 310
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);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These cases have been tested by the method testCreateTable.

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added.


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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added.


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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added.

@jerqi jerqi closed this Oct 28, 2024
@jerqi jerqi reopened this Oct 28, 2024
build.gradle.kts Outdated
Comment on lines 166 to 170
param.environment("HADOOP_USER_NAME", "anonymous")
if (project.name == "authorization-ranger") {
param.environment("HADOOP_USER_NAME", "test")
} else {
param.environment("HADOOP_USER_NAME", "anonymous")
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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")
  }
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Copy link
Member

@xunliu xunliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@xunliu xunliu merged commit b5ff4ba into apache:main Oct 28, 2024
26 checks passed
github-actions bot pushed a commit that referenced this pull request Oct 28, 2024
…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.
jerqi added a commit to qqqttt123/gravitino that referenced this pull request Oct 28, 2024
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
branch-0.7 Automatically cherry-pick commit to branch-0.7
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Subtask] Add more integration tests for the access control
3 participants