Skip to content

HDDS-15982. Fix UnsupportedOperationException and add idempotency to RangerClientMultiTenantAccessController #10872

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

Open
MahmoudHassanen99 wants to merge 4 commits into
base: master
Choose a base branch
from
+100 −10
Open

HDDS-15982. Fix UnsupportedOperationException and add idempotency to RangerClientMultiTenantAccessController #10872

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
6d51fab
HDDS-15982. Fix UnsupportedOperationException and add idempotency to …
MahmoudHassanen99 Jul 25, 2026
3870804
HDDS-15982. Address code review feedback for Ranger client
MahmoudHassanen99 Jul 26, 2026
b47008f
Add co-author
MahmoudHassanen99 Jul 26, 2026
c1c9529
Fix checkstyle import spacing
MahmoudHassanen99 Jul 26, 2026
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
110 changes: 100 additions & 10 deletions .../java/org/apache/hadoop/ozone/om/multitenant/RangerClientMultiTenantAccessController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ public class RangerClientMultiTenantAccessController implements

private static final int HTTP_STATUS_CODE_UNAUTHORIZED = 401;
private static final int HTTP_STATUS_CODE_BAD_REQUEST = 400;
private static final int HTTP_STATUS_CODE_NOT_FOUND = 404;

private final RangerClient client;
private final String rangerServiceName;
Expand Down Expand Up @@ -171,6 +172,37 @@ private void decodeRSEStatusCodes(RangerServiceException rse) {
}
}

/**
* Returns true if the RangerServiceException indicates a duplicate object
* (HTTP 400 with "already exists" in the message).
* Used to implement idempotent create operations.
*/
private static boolean isDuplicateException(RangerServiceException e) {
if (e.getStatus() == null) {
return false;
}
if (e.getStatus().getStatusCode() != HTTP_STATUS_CODE_BAD_REQUEST) {
return false;
}
String msg = e.getMessage();
return msg != null && (msg.contains("already exists")
|| msg.contains("duplicate")
|| msg.contains("DUPLICATE"));
}

/**
* Returns true if the RangerServiceException indicates a not-found condition
* (HTTP 404). Used to implement tolerant delete operations.
*/
private static boolean isNotFoundException(RangerServiceException e) {
return e.getStatus() != null
&& e.getStatus().getStatusCode() == HTTP_STATUS_CODE_NOT_FOUND;
}

// =========================================================================
// Policy operations
// =========================================================================

@Override
public Policy createPolicy(Policy policy) throws IOException {
if (LOG.isDebugEnabled()) {
Expand All @@ -181,6 +213,19 @@ public Policy createPolicy(Policy policy) throws IOException {
try {
rangerPolicy = client.createPolicy(toRangerPolicy(policy));
} catch (RangerServiceException e) {
// If the policy already exists, fetch and return it
// instead of failing. This makes tenant creation idempotent.
if (isDuplicateException(e)) {
LOG.warn("Policy {} already exists in Ranger, fetching existing policy.",
policy.getName());
try {
rangerPolicy = client.getPolicy(rangerServiceName, policy.getName());
return fromRangerPolicy(rangerPolicy);

@smengcl smengcl Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I do not think this fully guarantees idempotency for createPolicy. It could turn a duplicate error into success without confirming that the existing Ranger policy represents the requested resources, ACLs, roles, and labels.

For example, if an earlier tenant-create attempt left a policy for volume A and the retry requests volume B, this branch could return success while Ranger still contains the policy for A. The Ranger background sync appears to match policies by name, so it may not repair that mismatch.

The same concern may apply to createRole, including user membership and nested role/admin membership. Could we either:

  • verify that the existing object matches the requested state, then fail or update it when it differs
  • remove the generalized idempotency changes from this PR and handle state-aware reconciliation under a separate Jira

} catch (RangerServiceException e2) {
decodeRSEStatusCodes(e2);
throw new IOException(e2);
}
}
decodeRSEStatusCodes(e);
throw new IOException(e);
}
Expand Down Expand Up @@ -248,11 +293,21 @@ public void deletePolicy(String policyName) throws IOException {
try {
client.deletePolicy(rangerServiceName, policyName);
} catch (RangerServiceException e) {
// If the policy does not exist, silently return.
// This makes tenant deletion tolerant of partial previous state.
if (isNotFoundException(e)) {
LOG.warn("Policy {} not found in Ranger during delete - assuming already deleted.", policyName);
return;
}
decodeRSEStatusCodes(e);
throw new IOException(e);
}
}

// =========================================================================
// Role operations
// =========================================================================

@Override
public Role createRole(Role role) throws IOException {
if (LOG.isDebugEnabled()) {
Expand All @@ -264,6 +319,22 @@ public Role createRole(Role role) throws IOException {
rangerRole = client.createRole(rangerServiceName,
toRangerRole(role, shortName));
} catch (RangerServiceException e) {
// If the role already exists (HTTP 400 duplicate), fetch
// and return the existing role instead of throwing IOException.
// This makes tenant creation idempotent — safe to retry after a
// partial failure from a previous ozone tenant create attempt.
if (isDuplicateException(e)) {
LOG.warn("Role {} already exists in Ranger, fetching existing role.",
role.getName());
try {
RangerRole existingRole = client.getRole(
role.getName(), shortName, rangerServiceName);
return fromRangerRole(existingRole);
} catch (RangerServiceException e2) {
decodeRSEStatusCodes(e2);
throw new IOException(e2);
}
}
decodeRSEStatusCodes(e);
throw new IOException(e);
}
Expand Down Expand Up @@ -313,6 +384,13 @@ public void deleteRole(String roleName) throws IOException {
try {
client.deleteRole(roleName, shortName, rangerServiceName);
} catch (RangerServiceException e) {
// If the role does not exist, silently return.
// This makes tenant deletion tolerant of partial previous state,
// e.g. when one role was deleted but another was not.
if (isNotFoundException(e)) {

@smengcl smengcl Jul 29, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ranger 2.8 appears to report a missing role from delete-by-name as HTTP 400 rather than 404. Accepting every HTTP 400 could also hide real failures, such as deletion being rejected because the role is still referenced.

Since RangerServiceException does not expose a structured Ranger error code, could we use a narrowly scoped compatibility workaround and document why the response text is inspected?

+  private static boolean isRoleNotFoundException(
+      RangerServiceException e) {
+    if (isNotFoundException(e)) {
+      return true;
+    }
+
+    // Ranger 2.8 returns HTTP 400 instead of 404 when deleting a role
+    // that does not exist. RangerServiceException exposes no structured
+    // Ranger error code, so inspect the response text as a workaround.
+    String message = e.getMessage();
+    return e.getStatus() != null
+        && e.getStatus().getStatusCode() == HTTP_STATUS_CODE_BAD_REQUEST
+        && message != null
+        && message.contains("Role with name")
+        && message.contains("does not exist");
+  }
+
   ...

-      if (isNotFoundException(e)) {
+      if (isRoleNotFoundException(e)) {

Could you also test both the missing-role response and an unrelated HTTP 400 response to confirm that the latter is still propagated?

LOG.warn("Role {} not found in Ranger during delete - assuming already deleted.", roleName);
return;
}
decodeRSEStatusCodes(e);
throw new IOException(e);
}
Expand All @@ -333,6 +411,10 @@ public long getRangerServicePolicyVersion() throws IOException {
return policyVersion == null ? -1L : policyVersion;
}

// =========================================================================
// Private conversion helpers
// =========================================================================

private static List<RangerRole.RoleMember> toRangerRoleMembers(
Map<String, Boolean> members) {
return members.entrySet().stream()
Expand Down Expand Up @@ -433,23 +515,20 @@ private RangerPolicy toRangerPolicy(Policy policy) {
rangerPolicy.setService(rangerServiceName);
rangerPolicy.setPolicyLabels(new ArrayList<>(policy.getLabels()));

// Add resources.
// Add resources — always use new ArrayList to ensure mutability.
Map<String, RangerPolicy.RangerPolicyResource> resource = new HashMap<>();
// Add volumes.
if (!policy.getVolumes().isEmpty()) {
RangerPolicy.RangerPolicyResource volumeResources =
new RangerPolicy.RangerPolicyResource();
volumeResources.setValues(new ArrayList<>(policy.getVolumes()));
resource.put("volume", volumeResources);
}
// Add buckets.
if (!policy.getBuckets().isEmpty()) {
RangerPolicy.RangerPolicyResource bucketResources =
new RangerPolicy.RangerPolicyResource();
bucketResources.setValues(new ArrayList<>(policy.getBuckets()));
resource.put("bucket", bucketResources);
}
// Add keys.
if (!policy.getKeys().isEmpty()) {
RangerPolicy.RangerPolicyResource keyResources =
new RangerPolicy.RangerPolicyResource();
Expand All @@ -462,21 +541,27 @@ private RangerPolicy toRangerPolicy(Policy policy) {
rangerPolicy.setDescription(policy.getDescription().get());
}

// Use an explicit mutable ArrayList for policy items to prevent
// UnsupportedOperationException when Ranger model returns unmodifiable list.
List<RangerPolicy.RangerPolicyItem> policyItems = new ArrayList<>();

// Add users to the policy.
for (Map.Entry<String, Collection<Acl>> userAcls:
policy.getUserAcls().entrySet()) {
RangerPolicy.RangerPolicyItem item = new RangerPolicy.RangerPolicyItem();
item.setUsers(Collections.singletonList(userAcls.getKey()));

// Use mutable ArrayList for accesses.
List<RangerPolicy.RangerPolicyItemAccess> accesses = new ArrayList<>();
for (Acl acl: userAcls.getValue()) {
RangerPolicy.RangerPolicyItemAccess access =
new RangerPolicy.RangerPolicyItemAccess();
access.setIsAllowed(acl.isAllowed());
access.setType(aclToString.get(acl.getAclType()));
item.getAccesses().add(access);
accesses.add(access);
}

rangerPolicy.getPolicyItems().add(item);
item.setAccesses(accesses);
policyItems.add(item);
}

// Add roles to the policy.
Expand All @@ -485,17 +570,22 @@ private RangerPolicy toRangerPolicy(Policy policy) {
RangerPolicy.RangerPolicyItem item = new RangerPolicy.RangerPolicyItem();
item.setRoles(Collections.singletonList(roleAcls.getKey()));

// Use mutable ArrayList for accesses.
List<RangerPolicy.RangerPolicyItemAccess> accesses = new ArrayList<>();
for (Acl acl: roleAcls.getValue()) {
RangerPolicy.RangerPolicyItemAccess access =
new RangerPolicy.RangerPolicyItemAccess();
access.setIsAllowed(acl.isAllowed());
access.setType(aclToString.get(acl.getAclType()));
item.getAccesses().add(access);
accesses.add(access);
}

rangerPolicy.getPolicyItems().add(item);
item.setAccesses(accesses);
policyItems.add(item);
}

// Set the fully populated mutable list on rangerPolicy.
rangerPolicy.setPolicyItems(policyItems);

return rangerPolicy;
}
}