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.

Sign up for GitHub

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

Jump to bottom

[#3963] feat(core): Apache Ranger Hive authorization pushdown #4515

Merged
merged 23 commits into from
Aug 22, 2024
Merged

[#3963] feat(core): Apache Ranger Hive authorization pushdown #4515

merged 23 commits into from
Aug 22, 2024

Conversation

xunliu
Copy link
Member

@xunliu xunliu commented Aug 14, 2024 •
edited
Loading

What changes were proposed in this pull request?

Use Gravitino authorization plugin to authorize Ranger Hive.

  1. Added RangerHiveAuthorization modul
  2. Added RangerAuthorizationPlugin abstract class.
  3. Extend RangerHiveAuthorizationPlugin class process Hive authorization pushdown
  4. Added Ranger client extension

Why are the changes needed?

Fix: #3963

Does this PR introduce any user-facing change?

  1. Added RangerHiveAuthorizationPlugin interface

How was this patch tested?

RangerHiveIT passed.

@xunliu xunliu requested a review from yuqi1129 August 14, 2024 11:18
@lw-yang
Copy link
Contributor

lw-yang commented Aug 14, 2024

I found that there are differences between the construction the ranger policy and our implementation.

If the policyName is roleName + securableObjectFullName, it's possible that there will be many policies for the same resource, which seems unnecessary.

In our system, each policy corresponds to a single resource, and the policy is named based on the resource name. Within Ranger, we create roles in ranger, instead of putting the role in the policyName, we will put the role and role privileges into policyItems.

@xunliu xunliu force-pushed the issue-3963 branch 5 times, most recently from f957505 to 0d7b9bc Compare August 15, 2024 02:59
yuqi1129
yuqi1129 reviewed Aug 15, 2024
View reviewed changes
api/src/main/java/org/apache/gravitino/authorization/RoleChange.java Outdated Show resolved Hide resolved
api/src/main/java/org/apache/gravitino/authorization/RoleChange.java Outdated Show resolved Hide resolved
api/src/main/java/org/apache/gravitino/authorization/RoleChange.java Show resolved Hide resolved
api/src/main/java/org/apache/gravitino/authorization/RoleChange.java Outdated Show resolved Hide resolved
authorizations/authorization-ranger/build.gradle.kts Outdated Show resolved Hide resolved
...r/src/main/java/org/apache/gravitino/authorization/ranger/RangerHiveAuthorizationPlugin.java Outdated Show resolved Hide resolved
...r/src/main/java/org/apache/gravitino/authorization/ranger/RangerHiveAuthorizationPlugin.java Outdated Show resolved Hide resolved
...r/src/main/java/org/apache/gravitino/authorization/ranger/RangerHiveAuthorizationPlugin.java Outdated Show resolved Hide resolved
...r/src/main/java/org/apache/gravitino/authorization/ranger/RangerHiveAuthorizationPlugin.java Outdated Show resolved Hide resolved
...r/src/main/java/org/apache/gravitino/authorization/ranger/RangerHiveAuthorizationPlugin.java Outdated Show resolved Hide resolved
@xunliu xunliu force-pushed the issue-3963 branch 9 times, most recently from 18e9038 to 4f37b45 Compare August 18, 2024 22:48
@xunliu xunliu added the branch-0.6 Automatically cherry-pick commit to branch-0.6 label Aug 18, 2024
@xunliu xunliu force-pushed the issue-3963 branch 2 times, most recently from 02a2531 to eddc014 Compare August 19, 2024 01:46
@xunliu
Copy link
Member Author

xunliu commented Aug 19, 2024

@lw-yang @yuqi1129 I refactor this PR, Please help me review it again, thanks!

@xunliu
Copy link
Member Author

xunliu commented Aug 19, 2024

I found that there are differences between the construction the ranger policy and our implementation.

If the policyName is roleName + securableObjectFullName, it's possible that there will be many policies for the same resource, which seems unnecessary.

In our system, each policy corresponds to a single resource, and the policy is named based on the resource name. Within Ranger, we create roles in ranger, instead of putting the role in the policyName, we will put the role and role privileges into policyItems.

hi @lw-yang
So does Gravitino.

yuqi1129
yuqi1129 reviewed Aug 19, 2024
View reviewed changes
authorizations/authorization-ranger/build.gradle.kts Outdated Show resolved Hide resolved
...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java Outdated Show resolved Hide resolved
...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java Outdated Show resolved Hide resolved
...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java Outdated Show resolved Hide resolved
...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java Outdated Show resolved Hide resolved
...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java Outdated Show resolved Hide resolved
...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java Outdated Show resolved Hide resolved
...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java Outdated Show resolved Hide resolved
...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java Show resolved Hide resolved
...anger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java Show resolved Hide resolved
@xunliu xunliu mentioned this pull request Aug 19, 2024
Merged
@xunliu
Copy link
Member Author

xunliu commented Aug 19, 2024

@yuqi1129 I fixed all problems based on your comments, Please help me review again. Thanks.

jerqi
jerqi reviewed Aug 21, 2024
View reviewed changes
...thorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java Outdated
return access.getType().equals(privilege);
});
if (matchPrivilege
&& !policyItem.getUsers().isEmpty()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do we need getUsers is empty and getGroups is empty?

Copy link
Member Author

Choose a reason for hiding this comment

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

We can only remove this policy item if there are no users or groups

Copy link
Collaborator

Choose a reason for hiding this comment

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

We just remove roles here, not remove policyItem.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, you are right.
I fix this problem and add a test case.

@xunliu
Copy link
Member Author

xunliu commented Aug 21, 2024 •
edited
Loading

Only one point:

  1. Some privileges like use schema, use catalog, create catalog could be pushed down underlying system, too. The check logic will throw exception for them.

I created EPIC #4615 to track these issue.

yuqi1129
yuqi1129 approved these changes Aug 22, 2024
View reviewed changes
Copy link
Contributor

@yuqi1129 yuqi1129 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 92b8348 into apache:main Aug 22, 2024
25 checks passed
github-actions bot pushed a commit that referenced this pull request Aug 22, 2024
@xunliu @yuqi1129 @web-flow
[#3963] feat(core): Apache Ranger Hive authorization pushdown (#4515)
a2fec7a 
### What changes were proposed in this pull request?

Use Gravitino authorization plugin to authorize Ranger Hive.
1. Added RangerHiveAuthorization modul
2. Added RangerAuthorizationPlugin abstract class.
3. Extend RangerHiveAuthorizationPlugin class process Hive authorization
pushdown
4. Added Ranger client extension

### Why are the changes needed?

Fix: #3963

### Does this PR introduce _any_ user-facing change?

1. Added RangerHiveAuthorizationPlugin interface

### How was this patch tested?

RangerHiveIT passed.

---------

Co-authored-by: yuqi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jerqi jerqi jerqi left review comments

@lw-yang lw-yang lw-yang left review comments

@yuqi1129 yuqi1129 yuqi1129 approved these changes

Assignees

@xunliu xunliu

@yuqi1129 yuqi1129

Labels
branch-0.6 Automatically cherry-pick commit to branch-0.6
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Subtask] Apache Ranger Hive authorization pushdown
4 participants
@xunliu @lw-yang @jerqi @yuqi1129