smengcl commented on code in PR #10872:
URL: https://github.com/apache/ozone/pull/10872#discussion_r3672244059
##########
hadoop-ozone/multitenancy-ranger/src/main/java/org/apache/hadoop/ozone/om/multitenant/RangerClientMultiTenantAccessController.java:
##########
@@ -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)) {
Review Comment:
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?
```diff
+ 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?
##########
hadoop-ozone/multitenancy-ranger/src/main/java/org/apache/hadoop/ozone/om/multitenant/RangerClientMultiTenantAccessController.java:
##########
@@ -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);
Review Comment:
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
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
