HDDS-16187. [STS] Fix Latent S3 DeleteObjects Issue - #11019
Conversation
chihsuan
left a comment
chihsuan
left a comment
There was a problem hiding this comment.
Thanks for the patch! @fmorg-git The AccessDenied translation looks correct.
I left a few inline comments, mostly around the cases the new handling doesn't cover yet and the audit record on the throw path.
Also, could we add a unit test alongside the smoke test? TestPermissionCheck#testDeleteKeys already has the mocks needed.
| } catch (IOException ex) { | ||
| LOG.error("Delete key failed: {}", ex.getMessage()); | ||
| getMetrics().updateDeleteKeyFailureStats(startNanos); | ||
| final OMException omEx = (OMException) HddsClientUtils.containsException(ex, OMException.class); |
There was a problem hiding this comment.
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_EXPIRED and BUCKET_NOT_FOUND retain their S3 response?
| 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); |
There was a problem hiding this comment.
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.
Updated to add unit tests - fc80f77 |
There was a problem hiding this comment.
Pull request overview
This PR fixes an S3 Gateway behavior where DeleteObjects could return HTTP 500 when OM permission/STS token failures were surfaced as an IOException wrapping an OMException. The change ensures those OM failures are detected and translated into the appropriate S3 error response (e.g., AccessDenied, ExpiredToken), aligning behavior for latent S3 and STS-backed requests.
Changes:
- Translate
IOException-wrappedOMExceptionfailures duringBucketEndpoint#multiDeleteinto proper S3 errors instead of returning an internal error response. - Centralize MultiDelete failure audit logging into a helper method and ensure it’s invoked for translated OM failures.
- Add unit and smoke-test coverage for STS session policies denying
DeleteObjects.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java | Detects wrapped OMException during multi-delete and maps it to the correct OS3Exception/S3 error; refactors audit failure logging. |
| hadoop-ozone/s3gateway/src/test/java/org/apache/hadoop/ozone/s3/endpoint/TestPermissionCheck.java | Adds parameterized coverage to verify DeleteObjects translates contained OM failures (access denied, expired token, no such bucket). |
| hadoop-ozone/dist/src/main/smoketest/security/ozone-secure-sts.robot | Adds an STS smoke test asserting a GetObject-only session policy denies DeleteObjects (expects AccessDenied). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
ChenSammi
left a comment
ChenSammi
left a comment
There was a problem hiding this comment.
LGTM. Wait for the CI to pass.
Thanks @fmorg-git , and @chihsuan for the review.
|
Thanks for the review and merge. |
Please describe your PR in detail:
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-16187
How was this patch tested?
smoke tests