Copilot commented on code in PR #9653:
URL: https://github.com/apache/ozone/pull/9653#discussion_r3076604165
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java:
##########
@@ -88,6 +88,64 @@ public OMKeysDeleteRequest(OMRequest omRequest, BucketLayout
bucketLayout) {
super(omRequest, bucketLayout);
}
+ @Override
+ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
+ super.preExecute(ozoneManager);
+
+ DeleteKeysRequest deleteKeysRequest =
getOmRequest().getDeleteKeysRequest();
+ DeleteKeyArgs deleteKeyArgs = deleteKeysRequest.getDeleteKeys();
+
+ String volumeName = deleteKeyArgs.getVolumeName();
+ String bucketName = deleteKeyArgs.getBucketName();
+ List<String> keys = deleteKeyArgs.getKeysList();
+
+ // Resolve bucket link
+ ResolvedBucket resolvedBucketObj = ozoneManager.resolveBucketLink(
+ Pair.of(volumeName, bucketName));
+ String resolvedVolume = resolvedBucketObj.realVolume();
+ String resolvedBucket = resolvedBucketObj.realBucket();
+
+ // ACL check during preExecute - filter out keys that fail ACL check
+ List<String> keysPassingAcl = new ArrayList<>();
+ if (ozoneManager.getAclsEnabled()) {
+ for (String keyName : keys) {
+ try {
+ checkKeyAcls(ozoneManager, resolvedVolume, resolvedBucket, keyName,
+ IAccessAuthorizer.ACLType.DELETE, OzoneObj.ResourceType.KEY);
+ keysPassingAcl.add(keyName);
Review Comment:
Filtering out keys that fail ACL in preExecute changes the bulk deleteKeys
contract: validateAndUpdateCache will no longer be able to report
PERMISSION_DENIED/ACCESS_DENIED per key (eg via unDeletedKeys/keyToError), and
the request can return OK while silently skipping user-supplied keys. If the
intent is to keep partial-delete semantics, consider carrying denied keys
through in a way validateAndUpdateCache can include them in the response;
otherwise consider failing the whole request with PERMISSION_DENIED when any
key fails ACL rather than silently dropping keys.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketSetAclRequest.java:
##########
@@ -54,6 +54,7 @@ public class OMBucketSetAclRequest extends OMBucketAclRequest
{
@Override
public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
+ super.preExecute(ozoneManager);
long modificationTime = Time.now();
Review Comment:
Now that this method calls super.preExecute(ozoneManager),
OMClientRequest.preExecute will already populate UserInfo via
getUserIfNotExists(). The method later rebuilds the request and sets UserInfo
again via getUserInfo(), which can drop that fallback in non-RPC/internal call
paths (Server.getRemoteUser() may be null). Consider capturing the OMRequest
returned by super.preExecute and building from it without overriding UserInfo,
or switch the later setUserInfo call to getUserIfNotExists(ozoneManager).
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysRenameRequest.java:
##########
@@ -77,6 +81,76 @@ public OMKeysRenameRequest(OMRequest omRequest, BucketLayout
bucketLayout) {
super(omRequest, bucketLayout);
}
+ @Override
+ public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
+ super.preExecute(ozoneManager);
+
+ RenameKeysRequest renameKeysRequest =
getOmRequest().getRenameKeysRequest();
+ RenameKeysArgs renameKeysArgs = renameKeysRequest.getRenameKeysArgs();
+
+ String volumeName = renameKeysArgs.getVolumeName();
+ String bucketName = renameKeysArgs.getBucketName();
+
+ // Resolve bucket link
+ ResolvedBucket resolvedBucketObj = ozoneManager.resolveBucketLink(
+ Pair.of(volumeName, bucketName));
+ String resolvedVolume = resolvedBucketObj.realVolume();
+ String resolvedBucket = resolvedBucketObj.realBucket();
+
+ // ACL check during preExecute - filter out key pairs that fail ACL check
+ List<RenameKeysMap> keyPairsPassingAcl = new ArrayList<>();
+ if (ozoneManager.getAclsEnabled()) {
+ for (RenameKeysMap renameKey : renameKeysArgs.getRenameKeysMapList()) {
+ String fromKeyName = renameKey.getFromKeyName();
+ String toKeyName = renameKey.getToKeyName();
+
+ // Skip empty key names - they will be handled in
validateAndUpdateCache
+ if (fromKeyName.isEmpty() || toKeyName.isEmpty()) {
+ keyPairsPassingAcl.add(renameKey);
+ continue;
+ }
+
+ try {
+ // Check ACLs: DELETE permission on source key, CREATE permission on
destination key
+ checkKeyAcls(ozoneManager, resolvedVolume, resolvedBucket,
fromKeyName,
+ IAccessAuthorizer.ACLType.DELETE, OzoneObj.ResourceType.KEY);
+ checkKeyAcls(ozoneManager, resolvedVolume, resolvedBucket, toKeyName,
+ IAccessAuthorizer.ACLType.CREATE, OzoneObj.ResourceType.KEY);
+ keyPairsPassingAcl.add(renameKey);
+ } catch (IOException ex) {
+ // Log ACL failure but continue processing other key pairs
+ LOG.warn("ACL check failed for rename {} -> {} during preExecute,
key pair will be skipped: {}",
+ fromKeyName, toKeyName, ex.getMessage());
+ // Audit the failure
+ Map<String, String> auditMap = new LinkedHashMap<>();
+ auditMap.put(VOLUME, resolvedVolume);
+ auditMap.put(BUCKET, resolvedBucket);
+ auditMap.put(SRC_KEY, fromKeyName);
+ auditMap.put(DST_KEY, toKeyName);
+ markForAudit(ozoneManager.getAuditLogger(),
+ buildAuditMessage(OMAction.RENAME_KEYS, auditMap, ex,
+ getOmRequest().getUserInfo()));
+ }
+ }
+ } else {
+ // ACLs not enabled, all key pairs pass
+ keyPairsPassingAcl.addAll(renameKeysArgs.getRenameKeysMapList());
+ }
+
+ // Build modified request with only key pairs that passed ACL check
+ RenameKeysArgs.Builder modifiedArgs = renameKeysArgs.toBuilder()
+ .clearRenameKeysMap()
+ .addAllRenameKeysMap(keyPairsPassingAcl);
+
+ RenameKeysRequest.Builder modifiedRequest = renameKeysRequest.toBuilder()
+ .setRenameKeysArgs(modifiedArgs);
+
+ return getOmRequest().toBuilder()
+ .setRenameKeysRequest(modifiedRequest)
Review Comment:
Filtering out rename key-pairs that fail ACL in preExecute makes the request
succeed without reporting those pairs in UnRenamedKeys / PARTIAL_RENAME
(previously ACL failures were surfaced per pair). This is a client-visible
behavior change: callers may observe OK even though some requested renames were
skipped. Consider either (a) failing the whole request with PERMISSION_DENIED
when any pair fails ACL, or (b) preserving partial-rename semantics by ensuring
denied pairs are still represented in the response (eg by carrying them through
so validateAndUpdateCache can include them in unRenamedKeys).
```suggestion
return getOmRequest().toBuilder()
```
--
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]
