Gargi-jais11 commented on code in PR #11759:
URL: https://github.com/apache/ozone/pull/11759#discussion_r4374475969
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyDeleteRequest.java:
##########
@@ -70,6 +75,23 @@ public void testPreExecuteFailure(String testKeyName,
assertEquals(OMException.ResultCodes.INVALID_KEY_NAME,
omException.getResult());
}
+ @Test
+ public void preExecutePermissionDeniedWhenAclEnabled() {
Review Comment:
This test would pass without the change: overriding **checkKeyAcls** still
caused preExecute to throw PERMISSION_DENIED before. Please:
- After assertThrows, call OMAuditLogger.log(req.getAuditBuilder()) (same as
OMExecutionFlow).
- verify(auditLogger).logWrite(...) and assert op=DELETE_KEY, ret=FAILURE,
and assertSame(e, message.getThrowable()).
- Prefer getOmKeyDeleteRequest(...) (or a small helper on OMKeyRequestTests)
instead of new OMKeyDeleteRequest(...) so FSO subclasses can reuse the test.
##########
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/key/OMKeyDeleteRequest.java:
##########
@@ -88,7 +88,17 @@ public OMRequest preExecute(OzoneManager ozoneManager)
throws IOException {
OzoneManagerProtocolProtos.KeyArgs.Builder newKeyArgs =
keyArgs.toBuilder().setModificationTime(Time.now()).setKeyName(keyPath);
- KeyArgs resolvedArgs = resolveBucketAndCheckAcls(ozoneManager, newKeyArgs);
+ KeyArgs resolvedArgs;
+ try {
+ resolvedArgs = resolveBucketAndCheckAcls(ozoneManager, newKeyArgs);
+ } catch (IOException ex) {
+ // Ensure audit log captures preExecute failures
+ markForAudit(ozoneManager.getAuditLogger(),
+ buildAuditMessage(OMAction.DELETE_KEY,
+ buildLightKeyArgsAuditMap(newKeyArgs.build()), ex,
Review Comment:
Failure audit uses requested volume/bucket from newKeyArgs. Success path in
**validateAndUpdateCache** uses resolved keyArgs after link resolution. For a
bucket link, denied delete may show the link name while a successful delete
shows the source bucket.
Consider resolving first and then audit.
##########
hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/key/TestOMKeyRenameRequest.java:
##########
@@ -56,6 +62,25 @@ public void createParentKey() throws Exception {
dbToKey = omMetadataManager.getOzoneKey(volumeName, bucketName, toKeyName);
}
+ @Test
+ public void preExecutePermissionDeniedWhenAclEnabled() {
Review Comment:
Same as the delete test: this passes without the fix and doesn't cover FSO.
Please check the audit entry, and build the denied request through an
overridable method.
--
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]
