Copilot commented on code in PR #11683:
URL: https://github.com/apache/ozone/pull/11683#discussion_r4362653773
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyRequest.java:
##########
@@ -156,6 +156,7 @@ protected KeyArgs resolveBucketLink(
protected KeyArgs resolveBucketAndCheckKeyAcls(KeyArgs keyArgs,
OzoneManager ozoneManager, IAccessAuthorizer.ACLType aclType)
throws IOException {
+ toOzoneAcls(keyArgs.getAclsList());
KeyArgs resolvedArgs = resolveBucketLink(ozoneManager, keyArgs);
// check Acl
checkKeyAcls(ozoneManager, resolvedArgs.getVolumeName(),
Review Comment:
`toOzoneAcls(keyArgs.getAclsList())` is invoked unconditionally in
`preExecute`-related flow. If the deployment has `ignoreClientACLs` enabled
(used elsewhere in this class when *applying* client ACLs), this change can
cause requests that previously succeeded (because client ACLs were ignored) to
now fail with `INVALID_REQUEST` when a client sends malformed/unknown ACLs. If
the intent is to keep `ignoreClientACLs` semantics (ignore rather than reject),
guard this validation behind the same condition used for applying client ACLs
(and/or clear the client ACL list when ignoring). If the intent is to reject
malformed ACLs regardless of `ignoreClientACLs`, it would be good to make that
explicit and add/adjust tests that cover the `ignoreClientACLs=true`
configuration to lock in the behavior.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/OMClientRequest.java:
##########
@@ -632,6 +637,33 @@ public static String isValidKeyPath(String path) throws
OMException {
return OzoneFSUtils.isValidKeyPath(path, true);
}
+ /**
+ * Converts ACLs sent by the client.
+ * @throws OMException with INVALID_REQUEST if any of the ACLs is malformed
+ */
+ public static List<OzoneAcl> toOzoneAcls(List<OzoneAclInfo> acls) throws
OMException {
+ final List<OzoneAcl> ozoneAcls;
+ try {
+ ozoneAcls = OzoneAclUtil.fromProtobuf(acls);
+ } catch (IllegalArgumentException e) {
+ throw new OMException("Invalid ACL: " + e.getMessage(), INVALID_REQUEST);
+ }
+ checkAclRights(ozoneAcls);
+ return ozoneAcls;
+ }
Review Comment:
When wrapping `IllegalArgumentException` into `OMException`, the original
exception is discarded, which makes logs and debugging harder (stack trace and
root cause are lost). Prefer preserving the cause if `OMException` supports it
(e.g., a constructor that accepts a `Throwable`) or by setting the cause via
`initCause`, so operational debugging retains the original parsing failure
context.
--
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]
