[#4620] improvement(authz): Throw the necessary exception when handling Ranger plugin exception #6515
Conversation
…Ranger plugin the exception
|
@xunliu can you please help to review? |
| // The client will return a error message contains `doesn't have permission` if the role does | ||
| // not exist, then create it. | ||
| if (e.getMessage().contains("User doesn't have permissions to get details")) { | ||
| LOG.warn("The role({}) does not exist in the Ranger!, e: {}", roleName, e); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
-
Yes, we should use info log.
-
Usually we use admin user to do these operations, so this case shouldn't occur.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I have no better way to check this. Do you have suggestion?
jerqi
changed the title
|
lgtm |
| @@ -273,8 +273,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 (e.getMessage().contains("No RangerRole found for name")) { | |||
There was a problem hiding this comment.
The e.getMessage().contains("No RangerRole found for name") is Not a stable way. I think we can use rangerClient.getRole()? to double check it.
There was a problem hiding this comment.
if role doesn't, getRole will return exception. It isn't a good way.
| // The client will return a error message contains `doesn't have permission` if the role does | ||
| // not exist, then create it. | ||
| if (e.getMessage().contains("User doesn't have permissions to get details")) { | ||
| LOG.warn("The role({}) does not exist in the Ranger!, e: {}", roleName, e); |
There was a problem hiding this comment.
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.
What changes were proposed in this pull request?
Don't ignore the necessary exception. We should throw necessary exception from underlying system,
Why are the changes needed?
Fix: #4620
Does this PR introduce any user-facing change?
No.
How was this patch tested?
New uts and existing uts.