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

[#4620] improvement(authz): Throw the necessary exception when handling Ranger plugin exception #6515

Merged
merged 7 commits into from
Mar 4, 2025
Merged

[#4620] improvement(authz): Throw the necessary exception when handling Ranger plugin exception #6515

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
3ed323b
[#4620] improvement(authz): Throw the necessary exception when Ranger…
jerqi Feb 25, 2025
8e4b01f
change log level
jerqi Feb 26, 2025
b3b1627
address comment
jerqi Feb 28, 2025
1507a14
fix ut
jerqi Feb 28, 2025
343f070
fix
jerqi Feb 28, 2025
88b52ec
ut
jerqi Feb 28, 2025
1847c5e
Address comment
jerqi Mar 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ...in/src/main/java/org/apache/gravitino/authorization/chain/ChainedAuthorizationPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,11 @@ public Boolean onRoleAcquired(Role role) throws AuthorizationPluginException {

@Override
public Boolean onRoleDeleted(Role role) throws AuthorizationPluginException {
onRoleUpdated(
role,
role.securableObjects().stream()
.map(securableObject -> RoleChange.removeSecurableObject(role.name(), securableObject))
.toArray(RoleChange[]::new));
return chainedAction(plugin -> plugin.onRoleDeleted(role));
}

Expand Down
118 changes: 71 additions & 47 deletions ...er/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,8 +77,8 @@ public abstract class RangerAuthorizationPlugin

protected String metalake;
protected final String rangerServiceName;
protected final RangerClientExtension rangerClient;
protected final RangerHelper rangerHelper;
protected RangerClientExtension rangerClient;
protected RangerHelper rangerHelper;
@VisibleForTesting public final String rangerAdminName;

protected RangerAuthorizationPlugin(String metalake, Map<String, String> config) {
Expand Down Expand Up @@ -108,6 +108,26 @@ public String getMetalake() {
return metalake;
}

@VisibleForTesting
public RangerHelper getRangerHelper() {
return rangerHelper;
}

@VisibleForTesting
public void setRangerHelper(RangerHelper rangerHelper) {
this.rangerHelper = rangerHelper;
}

@VisibleForTesting
public RangerClientExtension getRangerClient() {
return rangerClient;
}

@VisibleForTesting
public void setRangerClient(RangerClientExtension rangerClient) {
this.rangerClient = rangerClient;
}

/**
* Set the Ranger policy resource defines rule.
*
Expand Down Expand Up @@ -273,8 +293,13 @@ public Boolean onRoleDeleted(Role role) throws AuthorizationPluginException {
rangerClient.deleteRole(
rangerHelper.generateGravitinoRoleName(role.name()), rangerAdminName, rangerServiceName);
} catch (RangerServiceException e) {
// Ignore exception to support idempotent operation
LOG.warn("Ranger delete role: {} failed!", role, e);
if (rangerHelper.getRangerRole(role.name()) == null) {
// Ignore exception to support idempotent operation
LOG.info("Ranger delete role: {} failed!", role, e);
} else {
throw new AuthorizationPluginException(
"Fail to delete role %s exception: %s", role, e.getMessage());
}
}
return Boolean.TRUE;
}
Expand All @@ -292,14 +317,13 @@ public Boolean onRoleUpdated(Role role, RoleChange... changes)

List<AuthorizationSecurableObject> authzSecurableObjects =
translatePrivilege(securableObject);
authzSecurableObjects.stream()
.forEach(
authzSecurableObject -> {
if (!doAddSecurableObject(role.name(), authzSecurableObject)) {
throw new AuthorizationPluginException(
"Failed to add the securable object to the Ranger policy!");
}
});
authzSecurableObjects.forEach(
authzSecurableObject -> {
if (!doAddSecurableObject(role.name(), authzSecurableObject)) {
throw new AuthorizationPluginException(
"Failed to add the securable object to the Ranger policy!");
}
});
} else if (change instanceof RoleChange.RemoveSecurableObject) {
SecurableObject securableObject =
((RoleChange.RemoveSecurableObject) change).getSecurableObject();
Expand Down Expand Up @@ -337,16 +361,14 @@ public Boolean onRoleUpdated(Role role, RoleChange... changes)
translatePrivilege(oldSecurableObject);
List<AuthorizationSecurableObject> rangerNewSecurableObjects =
translatePrivilege(newSecurableObject);
rangerOldSecurableObjects.stream()
.forEach(
AuthorizationSecurableObject -> {
removeSecurableObject(role.name(), AuthorizationSecurableObject);
});
rangerNewSecurableObjects.stream()
.forEach(
AuthorizationSecurableObject -> {
doAddSecurableObject(role.name(), AuthorizationSecurableObject);
});
rangerOldSecurableObjects.forEach(
AuthorizationSecurableObject -> {
removeSecurableObject(role.name(), AuthorizationSecurableObject);
});
rangerNewSecurableObjects.forEach(
AuthorizationSecurableObject -> {
doAddSecurableObject(role.name(), AuthorizationSecurableObject);
});
} else {
throw new IllegalArgumentException(
"Unsupported role change type: "
Expand Down Expand Up @@ -499,23 +521,21 @@ public Boolean onOwnerSet(MetadataObject metadataObject, Owner preOwner, Owner n
LOG.warn("Grant owner role: {} failed!", ownerRoleName, e);
}

rangerSecurableObjects.stream()
.forEach(
rangerSecurableObject -> {
RangerPolicy policy = findManagedPolicy(rangerSecurableObject);
try {
if (policy == null) {
policy = addOwnerRoleToNewPolicy(rangerSecurableObject, ownerRoleName);
rangerClient.createPolicy(policy);
} else {
rangerHelper.updatePolicyOwnerRole(policy, ownerRoleName);
rangerClient.updatePolicy(policy.getId(), policy);
}
} catch (RangerServiceException e) {
throw new AuthorizationPluginException(
e, "Failed to add the owner to the Ranger!");
}
});
rangerSecurableObjects.forEach(
rangerSecurableObject -> {
RangerPolicy policy = findManagedPolicy(rangerSecurableObject);
try {
if (policy == null) {
policy = addOwnerRoleToNewPolicy(rangerSecurableObject, ownerRoleName);
rangerClient.createPolicy(policy);
} else {
rangerHelper.updatePolicyOwnerRole(policy, ownerRoleName);
rangerClient.updatePolicy(policy.getId(), policy);
}
} catch (RangerServiceException e) {
throw new AuthorizationPluginException(e, "Failed to add the owner to the Ranger!");
}
});
break;
case SCHEMA:
case TABLE:
Expand Down Expand Up @@ -576,8 +596,9 @@ public Boolean onGrantedRolesToUser(List<Role> roles, User user)
try {
rangerClient.grantRole(rangerServiceName, grantRevokeRoleRequest);
} catch (RangerServiceException e) {
// Ignore exception, support idempotent operation
LOG.warn("Grant role: {} to user: {} failed!", role, user, e);
throw new AuthorizationPluginException(
"Fail to grant role %s to user %s, exception: %s",
role.name(), user.name(), e.getMessage());
}
Copy link
Member

Choose a reason for hiding this comment

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

Another consideration as to why you don't throw an exception is to support idempotent operations.
Gravitino -> Ranger
Maybe the data was deleted in the last operation of the Ranger, and now it's being operated again. You can't throw an exception.
If you want to throw an exception, you have to cover the idempotent operation, with a test case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You origin test code have covered the logic. You can see RangerHiveIT#testOnGrantedRolesToUser

});

Expand Down Expand Up @@ -611,8 +632,9 @@ public Boolean onRevokedRolesFromUser(List<Role> roles, User user)
try {
rangerClient.revokeRole(rangerServiceName, grantRevokeRoleRequest);
} catch (RangerServiceException e) {
// Ignore exception to support idempotent operation
LOG.warn("Revoke role: {} from user: {} failed!", role, user, e);
throw new AuthorizationPluginException(
"Fail to revoke role %s from user %s, exception: %s",
role.name(), user.name(), e.getMessage());
}
});

Expand Down Expand Up @@ -646,8 +668,9 @@ public Boolean onGrantedRolesToGroup(List<Role> roles, Group group)
try {
rangerClient.grantRole(rangerServiceName, grantRevokeRoleRequest);
} catch (RangerServiceException e) {
// Ignore exception to support idempotent operation
LOG.warn("Grant role: {} to group: {} failed!", role, group, e);
throw new AuthorizationPluginException(
"Fail to grant role: %s to group %s, exception: %s.",
role, group, e.getMessage());
}
});
return Boolean.TRUE;
Expand Down Expand Up @@ -678,8 +701,9 @@ public Boolean onRevokedRolesFromGroup(List<Role> roles, Group group)
try {
rangerClient.revokeRole(rangerServiceName, grantRevokeRoleRequest);
} catch (RangerServiceException e) {
// Ignore exception to support idempotent operation
LOG.warn("Revoke role: {} from group: {} failed!", role, group, e);
throw new AuthorizationPluginException(
"Fail to revoke role %s from group %s, exception: %s",
role.name(), group.name(), e.getMessage());
}
});

Expand Down
31 changes: 23 additions & 8 deletions ...rization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,7 +236,7 @@ protected GrantRevokeRoleRequest createGrantRevokeRoleRequest(
Set<String> groups =
StringUtils.isEmpty(groupName) ? Sets.newHashSet() : Sets.newHashSet(groupName);

if (users.size() == 0 && groups.size() == 0) {
if (users.isEmpty() && groups.isEmpty()) {
throw new AuthorizationPluginException("The user and group cannot be empty!");
}

Expand Down Expand Up @@ -274,13 +274,8 @@ protected RangerRole createRangerRoleIfNotExists(String roleName, boolean isOwne
GRAVITINO_METALAKE_OWNER_ROLE, GRAVITINO_CATALOG_OWNER_ROLE, GRAVITINO_OWNER_ROLE));
}

RangerRole rangerRole = null;
try {
rangerRole = rangerClient.getRole(roleName, rangerAdminName, rangerServiceName);
} catch (RangerServiceException e) {
// ignore exception, If the role does not exist, then create it.
LOG.warn("The role({}) does not exist in the Ranger!", roleName);
}
RangerRole rangerRole = getRangerRole(roleName);

try {
if (rangerRole == null) {
rangerRole = new RangerRole(roleName, RangerHelper.MANAGED_BY_GRAVITINO, null, null, null);
Expand All @@ -293,6 +288,26 @@ protected RangerRole createRangerRoleIfNotExists(String roleName, boolean isOwne
return rangerRole;
}

public RangerRole getRangerRole(String roleName) {
RangerRole rangerRole = null;
try {
rangerRole = rangerClient.getRole(roleName, rangerAdminName, rangerServiceName);
} catch (RangerServiceException e) {

// The client will return a error message contains `doesn't have permission` if the role does
// not exist, then create it.
if (e.getMessage() != null
&& e.getMessage().contains("User doesn't have permissions to get details")) {
LOG.warn("The role({}) does not exist in the Ranger!, e: {}", roleName, e);
Copy link
Collaborator

Choose a reason for hiding this comment

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

IIUC, this method ensures that that a specific ranger role exists.
If the role is not there yet, we'll create it.
This means we may want to print an info log rather than a warning, right?

If the user cannot check if a role exists or not, we still allow the function to continue. Does this mean that a user can create a role but he/she cannot view it?
The role becomes Write-Only for him/her?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. Yes, we should use info log.

  2. Usually we use admin user to do these operations, so this case shouldn't occur.

Copy link
Member

Choose a reason for hiding this comment

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

The e.getMessage().contains("User doesn't have permissions to get details") is Not a stable way. I think we can use another way to double check it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The e.getMessage().contains("User doesn't have permissions to get details") is Not a stable way. I think we can use another way to double check it.

Good point. Maybe we need to treat this seriously. For whatever permission related errors, always raise a special exception type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have no better way to check this. Do you have suggestion?

} else {
throw new AuthorizationPluginException(
"Failed to check role(%s) whether exists in the Ranger! e: %s",
roleName, e.getMessage());
}
}
return rangerRole;
}

protected void updatePolicyOwner(RangerPolicy policy, Owner preOwner, Owner newOwner) {
// Find matching policy items based on the owner's privileges
List<RangerPolicy.RangerPolicyItem> matchPolicyItems =
Expand Down
Loading