HDDS-13855. Move ACL check to preExecute in Volume, Bucket and Keys requests #9297
Conversation
sarvekshayr
left a comment
sarvekshayr
left a comment
There was a problem hiding this comment.
Thanks @ss77892 for the patch.
Just wanted to clarify one thing around the ACL handling in OMKeysDeleteRequest and OMKeysRenameRequest.
In the new implementation, the logic throws an exception on the first ACL failure.
In the old implementation, ACL failures were collected per key, allowing partial success.
Is this change in behaviour intentional? Should the request fail immediately on the first ACL violation?
| auditMap.put("volume", resolvedVolume); | ||
| auditMap.put("bucket", resolvedBucket); | ||
| auditMap.put("fromKey", fromKeyName); | ||
| auditMap.put("toKey", toKeyName); |
There was a problem hiding this comment.
To keep the audit map keys consistent across the codebase, can we avoid hardcoding here?
We should either use the existing constants or add new ones if needed for consistency.
adoroszlai
changed the title
|
@yandrey321 Can you take a look at this change? |
| // Add test user as admin on current leader | ||
| OzoneManager currentLeader = cluster.getOMLeader(); | ||
| addAdminToSpecificOM(currentLeader, TEST_USER); | ||
| assertTrue(currentLeader.getOmAdminUsernames().contains(TEST_USER)); |
There was a problem hiding this comment.
nit: please use assertThat(...).contains(...) for better assertion failure message. Similarly, doesNotContain(...) instead of assertFalse(...).
| super.preExecute(ozoneManager); | ||
| DeleteVolumeRequest deleteVolumeRequest = | ||
| getOmRequest().getDeleteVolumeRequest(); | ||
| Preconditions.checkNotNull(deleteVolumeRequest); |
There was a problem hiding this comment.
nit: prefer JDK's Objects.requireNonNull
| Preconditions.checkNotNull(deleteVolumeRequest); | |
| Objects.requireNonNull(deleteVolumeRequest); |
|
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. |
|
Thank you for your contribution. This PR is being closed due to inactivity. If needed, feel free to reopen it. |
4 participants
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.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-13855
How was this patch tested?
Manual tests, UT