-
Notifications
You must be signed in to change notification settings - Fork 633
HDDS-16187. [STS] Fix Latent S3 DeleteObjects Issue #11019
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -46,6 +46,7 @@ | |
| import javax.ws.rs.core.MediaType; | ||
| import javax.ws.rs.core.Response; | ||
| import org.apache.commons.lang3.StringUtils; | ||
| import org.apache.hadoop.hdds.scm.client.HddsClientUtils; | ||
| import org.apache.hadoop.ozone.audit.AuditEventStatus; | ||
| import org.apache.hadoop.ozone.audit.AuditMessage; | ||
| import org.apache.hadoop.ozone.audit.S3GAction; | ||
|
|
@@ -351,7 +352,14 @@ public MultiDeleteResponse multiDelete( | |
| throw newError(S3ErrorTable.MALFORMED_XML, bucketName); | ||
| } | ||
|
|
||
| OzoneBucket bucket = getVolume().getBucket(bucketName); | ||
| final OzoneBucket bucket; | ||
| try { | ||
| bucket = getVolume().getBucket(bucketName); | ||
| } catch (OMException ex) { | ||
| throw newError(bucketName, ex); | ||
| } catch (IOException ex) { | ||
| throw newError(S3ErrorTable.INTERNAL_ERROR, bucketName, ex); | ||
| } | ||
| MultiDeleteResponse result = new MultiDeleteResponse(); | ||
| List<String> deleteKeys = new ArrayList<>(); | ||
|
|
||
|
|
@@ -380,8 +388,12 @@ public MultiDeleteResponse multiDelete( | |
| } | ||
| getMetrics().updateDeleteKeySuccessStats(startNanos); | ||
| } catch (IOException ex) { | ||
| LOG.error("Delete key failed: {}", ex.getMessage()); | ||
| getMetrics().updateDeleteKeyFailureStats(startNanos); | ||
| final OMException omEx = (OMException) HddsClientUtils.containsException(ex, OMException.class); | ||
| if (omEx != null && S3ErrorTable.translateResultCode(omEx) == S3ErrorTable.ACCESS_DENIED) { | ||
| throw newError(S3ErrorTable.ACCESS_DENIED, bucketName, omEx); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This throw skips the audit block below, so rejected multi-delete requests leave no failure audit record. I wonder if we should audit the failure before propagating it.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. updated - fc80f77 |
||
| } | ||
| LOG.error("Delete key failed: {}", ex.getMessage()); | ||
| result.addError( | ||
| new Error("ALL", "InternalError", | ||
| ex.getMessage())); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, we only handle ACCESS_DENIED, and non-ACCESS_DENIED OM errors still return HTTP 200 with ALL/InternalError. Should we translate every contained OMException here so that errors like
TOKEN_EXPIREDandBUCKET_NOT_FOUNDretain their S3 response?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated - fc80f77