HDDS-13855. Move ACL check in Volume, Bucket and Keys requests to preExecute by ss77892 · Pull Request #9653 · apache/ozone

ss77892 · 2026-01-20T17:48:59Z

What changes were proposed in this pull request?

HDDS-13855. Move ACL check in Volume, Bucket and Keys requests to preexecute.

Please describe your PR in detail:
We are moving ACL checks from the validate cache method to the pre-execute stage to avoid multiple executions on all Ratis nodes. That's the second attempt, the first pr : #9297. In this PR all previous comments were addressed.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-13855

How was this patch tested?

Manual tests, UT

… execute

jojochuang

A lot of request handlers such as OMKeyAclRequest operates on OBS buckets. They have a sibling request like OMKeyAclRequestWithFSO, which operates on FSO buckets.

They are very similar but unfortunately are different request handlers for different buckets, and we need to move the acl check to preExecute() for them too.

jojochuang · 2026-01-24T04:38:41Z

      bucket = resolvedBucket.realBucket();
      key = objectParser.getKey();

-      // check Acl


we need to repeat this for OMKeyAclRequestWithFSO.

github-actions · 2026-02-18T00:09:05Z

This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days.

github-actions · 2026-02-25T00:09:38Z

Thank you for your contribution. This PR is being closed due to inactivity. Please contact a maintainer if you would like to reopen it.

Copilot

Pull request overview

Moves ACL enforcement for OM volume/bucket/key/prefix operations from validateAndUpdateCache to preExecute so authorization is evaluated only on the leader (avoiding repeated external ACL/Ranger calls across all Ratis nodes).

Changes:

Add preExecute ACL checks (with audit logging on failure) across volume, bucket, key, and prefix request types; remove corresponding checks from validateAndUpdateCache.
Update several request preExecute implementations to preserve/ensure UserInfo is present (via super.preExecute / getUserIfNotExists).
Extend HA integration testing to validate leader-specific ACL enforcement for more operations (quota/owner/delete, bucket property/owner/delete, bulk delete/rename, key ACL ops).

Reviewed changes

Copilot reviewed 21 out of 21 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeSetAclRequest.java	Delegates ACL enforcement to parent `preExecute`; updates request build to use `OMRequest` from `super.preExecute`.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeRemoveAclRequest.java	Same pattern as volume set ACL: rely on parent `preExecute` and rebuild from returned request.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeAddAclRequest.java	Same pattern as volume set/remove ACL.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/acl/OMVolumeAclRequest.java	Introduces volume ACL check (and audit-on-failure) in `preExecute`; removes ACL check from `validateAndUpdateCache`.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeSetQuotaRequest.java	Moves volume quota ACL check + audit-on-failure into `preExecute`; removes from `validateAndUpdateCache`.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeSetOwnerRequest.java	Moves volume set-owner ACL check + audit-on-failure into `preExecute`; removes from `validateAndUpdateCache`.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/volume/OMVolumeDeleteRequest.java	Adds `preExecute` ACL enforcement + audit-on-failure for delete volume; removes from `validateAndUpdateCache`.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/acl/prefix/OMPrefixAclRequest.java	Adds prefix ACL enforcement + audit-on-failure in `preExecute`; removes from `validateAndUpdateCache`.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/acl/OMKeySetAclRequestWithFSO.java	Builds request from `super.preExecute` result to avoid re-setting user info while still updating modification time.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/acl/OMKeyRemoveAclRequestWithFSO.java	Same as key set ACL (FSO): rebuild from `super.preExecute` request.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/acl/OMKeyAddAclRequestWithFSO.java	Same as key set/remove ACL (FSO).
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/acl/OMKeyAclRequestWithFSO.java	Adds key ACL enforcement + audit-on-failure in `preExecute`; removes from `validateAndUpdateCache`.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/acl/OMKeyAclRequest.java	Adds key ACL enforcement + audit-on-failure in `preExecute`; removes from `validateAndUpdateCache`.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysRenameRequest.java	Adds leader-only ACL filtering for bulk rename in `preExecute`; removes per-pair ACL checks from `validateAndUpdateCache`.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java	Adds leader-only ACL filtering for bulk delete in `preExecute`; removes per-key ACL checks from `validateAndUpdateCache`.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketSetAclRequest.java	Calls `super.preExecute` so bucket ACL checks happen in parent `preExecute`.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/acl/OMBucketAclRequest.java	Adds bucket ACL enforcement + audit-on-failure in `preExecute`; removes from `validateAndUpdateCache`.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketSetPropertyRequest.java	Moves bucket-property ACL check + audit-on-failure to `preExecute`; removes from `validateAndUpdateCache`.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketSetOwnerRequest.java	Moves bucket set-owner ACL check + audit-on-failure to `preExecute`; removes from `validateAndUpdateCache`.
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/bucket/OMBucketDeleteRequest.java	Adds `preExecute` ACL enforcement + audit-on-failure for delete bucket; removes from `validateAndUpdateCache`.
hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOMHALeaderSpecificACLEnforcement.java	Adds HA integration tests validating leader-specific ACL enforcement for multiple operations.

Comments suppressed due to low confidence (1)

hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeysDeleteRequest.java:215

The per-key try/catch that previously handled exceptions (and populated unDeletedKeys/keyToError while continuing) has been removed. With the current loop, any IOException/InvalidPathException thrown by getOmKeyInfo(), getOzoneKeyStatus(), or addKeyToAppropriateList() will abort the entire bulk delete request, rather than returning a partial delete response for the remaining keys. Consider restoring per-key exception handling so a single bad key/path doesn’t fail the whole batch, and so keyToError can be populated consistently.

        String keyName = deleteKeyArgs.getKeys(indexFailed);
        String objectKey =
            omMetadataManager.getOzoneKey(volumeName, bucketName, keyName);
        OmKeyInfo omKeyInfo = getOmKeyInfo(ozoneManager, omMetadataManager,
            volumeName, bucketName, keyName);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-14T01:07:49Z

  @Override
  public OMRequest preExecute(OzoneManager ozoneManager) throws IOException {
+    super.preExecute(ozoneManager);
    long modificationTime = Time.now();


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).

Copilot · 2026-04-14T01:07:50Z

+    // 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);


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.

Clients that rely solely on OM delete-keys response fields may no longer see per-key ACL denials, which weakens observable failure semantics even though audit may still capture them. This needs to be addressed.

Copilot · 2026-04-14T01:07:50Z

+    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)


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).

Suggested change

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)

return getOmRequest().toBuilder()

github-actions · 2026-05-06T00:19:13Z

This PR has been marked as stale due to 21 days of inactivity. Please comment or remove the stale label to keep it open. Otherwise, it will be automatically closed in 7 days.

sarvekshayr

Thanks @ss77892 for working on this.

It would be good if we could revert those whitespace changes to keep the diff clean.

sarvekshayr · 2026-05-19T06:16:44Z

-          checkKeyAcls(ozoneManager, volumeName, bucketName, keyName,
-              IAccessAuthorizer.ACLType.DELETE, OzoneObj.ResourceType.KEY,
-              volumeOwner);
-          perfMetrics.setDeleteKeysAclCheckLatencyNs(Time.monotonicNowNanos() - startNanosDeleteKeysAclCheckLatency);


deleteKeysAclCheckLatencyNs metric is no longer updated after moving out the ACL check from validateAndUpdateCache(). Let's restore the metric in OMKeysDeleteRequest.preExecute().

sarvekshayr · 2026-05-19T06:34:25Z

+    // 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);


Clients that rely solely on OM delete-keys response fields may no longer see per-key ACL denials, which weakens observable failure semantics even though audit may still capture them. This needs to be addressed.

HDDS-13855. Move ACL check in Volume, Bucket and Keys requests to pre…

9cb3372

… execute

ss77892 changed the title ~~HDDS-13855. Move ACL check in Volume, Bucket and Keys requests to pre…~~ HDDS-13855. Move ACL check in Volume, Bucket and Keys requests to preExecute Jan 20, 2026

jojochuang reviewed Jan 24, 2026

View reviewed changes

ACL requests with FSO refactored

6e75f53

github-actions Bot added the stale label Feb 18, 2026

github-actions Bot closed this Feb 25, 2026

jojochuang reopened this Mar 31, 2026

jojochuang removed the stale label Mar 31, 2026

fmorg-git mentioned this pull request Mar 31, 2026

HDDS-14935. [STS] Handle Latent Inconsistencies in S3 API Acl Checks #10009

Open

jojochuang requested a review from Copilot April 14, 2026 01:02

Copilot started reviewing on behalf of jojochuang April 14, 2026 01:02 View session

Copilot AI reviewed Apr 14, 2026

View reviewed changes

github-actions Bot added stale and removed stale labels May 6, 2026

jojochuang requested a review from ivandika3 May 15, 2026 18:33

sarvekshayr reviewed May 19, 2026

View reviewed changes

Conversation

ss77892 commented Jan 20, 2026

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

jojochuang left a comment

Choose a reason for hiding this comment

Uh oh!

jojochuang Jan 24, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Feb 18, 2026

Uh oh!

github-actions Bot commented Feb 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

sarvekshayr May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Apr 14, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 6, 2026

Uh oh!

sarvekshayr left a comment

Choose a reason for hiding this comment

Uh oh!

sarvekshayr May 19, 2026

Choose a reason for hiding this comment

Uh oh!

sarvekshayr May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants