From 21d83d3e55c2b2d595616cbaa8064fcd54e5fb7e Mon Sep 17 00:00:00 2001 From: Rory Date: Tue, 25 Feb 2025 17:27:23 +0800 Subject: [PATCH] [#4620] improvement(authz): Throw the necessary exception when Ranger plugin the exception --- .../ranger/RangerAuthorizationPlugin.java | 44 +++++++++++-------- .../authorization/ranger/RangerHelper.java | 15 +++++-- .../ranger/integration/test/RangerHiveIT.java | 8 +++- 3 files changed, 45 insertions(+), 22 deletions(-) diff --git a/authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java b/authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java index 7292d537452..777d431140e 100644 --- a/authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java +++ b/authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerAuthorizationPlugin.java @@ -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")) { + // Ignore exception to support idempotent operation + LOG.warn("Ranger delete role: {} failed!", role, e); + } else { + throw new AuthorizationPluginException( + "Fail to delete role %s exception: %s", role, e.getMessage()); + } } return Boolean.TRUE; } @@ -292,14 +297,13 @@ public Boolean onRoleUpdated(Role role, RoleChange... changes) List 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(); @@ -576,8 +580,9 @@ public Boolean onGrantedRolesToUser(List 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()); } }); @@ -611,8 +616,9 @@ public Boolean onRevokedRolesFromUser(List 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()); } }); @@ -646,8 +652,9 @@ public Boolean onGrantedRolesToGroup(List 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; @@ -678,8 +685,9 @@ public Boolean onRevokedRolesFromGroup(List 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()); } }); diff --git a/authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java b/authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java index 1ec65daea22..a4e3e15f93f 100644 --- a/authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java +++ b/authorizations/authorization-ranger/src/main/java/org/apache/gravitino/authorization/ranger/RangerHelper.java @@ -236,7 +236,7 @@ protected GrantRevokeRoleRequest createGrantRevokeRoleRequest( Set 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!"); } @@ -278,9 +278,18 @@ protected RangerRole createRangerRoleIfNotExists(String roleName, boolean isOwne 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); + + // 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); + } else { + throw new AuthorizationPluginException( + "Failed to check role(%s) whether exists in the Ranger! e: %s", + roleName, e.getMessage()); + } } + try { if (rangerRole == null) { rangerRole = new RangerRole(roleName, RangerHelper.MANAGED_BY_GRAVITINO, null, null, null); diff --git a/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveIT.java b/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveIT.java index 6a32ab9ea2a..0198065b9a6 100644 --- a/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveIT.java +++ b/authorizations/authorization-ranger/src/test/java/org/apache/gravitino/authorization/ranger/integration/test/RangerHiveIT.java @@ -138,6 +138,9 @@ public void testOnRoleCreated() { RoleEntity role = mock3TableRole(currentFunName()); Assertions.assertTrue(rangerAuthHivePlugin.onRoleCreated(role)); verifyRoleInRanger(rangerAuthHivePlugin, role); + + // Repeat to create the same to verify the idempotent operation + Assertions.assertTrue(rangerAuthHivePlugin.onRoleCreated(role)); } @Test @@ -266,6 +269,9 @@ public void testOnRoleDeleted() { Assertions.assertTrue(rangerAuthHivePlugin.onRoleDeleted(role)); // Check if the policy is deleted assertFindManagedPolicyItems(role, false); + + // Repeat to delete the same role to verify the idempotent operation + Assertions.assertTrue(rangerAuthHivePlugin.onRoleDeleted(role)); } @Test @@ -294,7 +300,7 @@ public void testOnRoleDeleted2() { // delete this role Assertions.assertTrue(rangerAuthHivePlugin.onRoleDeleted(role)); - // Because this metaobject has owner, so the policy should not be deleted + // Because this metadata object has owner, so the policy should not be deleted assertFindManagedPolicyItems(role, false); }