You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/12/01 08:52:42 UTC
[GitHub] [ozone] ChenSammi opened a new pull request #1642: HDDS-4519. Return forbidden instead of interval server error from s3g…
ChenSammi opened a new pull request #1642:
URL: https://github.com/apache/ozone/pull/1642
https://issues.apache.org/jira/browse/HDDS-4519
Currently s3g will return "internal server error" when user doesn't have the permission to access the resource.
This task is to return HTTP 403 FORBIDDEN in this case.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #1642: HDDS-4519. Return forbidden instead of interval server error from s3g…
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on pull request #1642:
URL: https://github.com/apache/ozone/pull/1642#issuecomment-742494077
@bharatviswa504 @xiaoyuyao , uploaded a new commit to address the comments. Would you help to review again at your convenient time?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi merged pull request #1642: HDDS-4519. Return forbidden instead of interval server error from s3g…
Posted by GitBox <gi...@apache.org>.
ChenSammi merged pull request #1642:
URL: https://github.com/apache/ozone/pull/1642
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #1642: HDDS-4519. Return forbidden instead of interval server error from s3g…
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on pull request #1642:
URL: https://github.com/apache/ozone/pull/1642#issuecomment-745113488
> this
We have such kind of use cases. You are right, S3 doesn't support quota feature. It seems the best we can do is return internal error code along with the quota exceed message to user, so we can tell from the error message that which is true failure reason.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #1642: HDDS-4519. Return forbidden instead of interval server error from s3g…
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #1642:
URL: https://github.com/apache/ozone/pull/1642#discussion_r539801024
##########
File path: hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3ErrorTable.java
##########
@@ -105,6 +106,9 @@ private S3ErrorTable() {
"InternalError", "We encountered an internal error. Please try again.",
HTTP_SERVER_ERROR);
+ public static final OS3Exception PERMISSION_DENIED = new OS3Exception(
Review comment:
Good catch, I will change the description to "AccessDenied".
Although where are multiple Error Codes map to "HTTP 403 Forbidden" , "AccessDenied" is the most appropriate description for this ACL case.
https://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html#RESTErrorResponses
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1642: HDDS-4519. Return forbidden instead of interval server error from s3g…
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1642:
URL: https://github.com/apache/ozone/pull/1642#discussion_r535844460
##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java
##########
@@ -780,7 +780,12 @@ public OzoneMultipartUploadList listMultipartUploads(String prefix)
@Override
public boolean hasNext() {
if(!currentIterator.hasNext() && currentValue != null) {
- currentIterator = getNextListOfKeys(currentValue.getName()).iterator();
+ try {
+ currentIterator =
+ getNextListOfKeys(currentValue.getName()).iterator();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
Review comment:
I see the current code is doing the same.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #1642: HDDS-4519. Return forbidden instead of interval server error from s3g…
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on pull request #1642:
URL: https://github.com/apache/ozone/pull/1642#issuecomment-742198744
>
>
> Overall LGTM, I have one comment on the error code. I agree with @elek here we should have a better way to handle errors instead of updating each method which is a tedious task, but that's how the current code is.
Agree, we need the a more generic way to handle exceptions in s3g. Besides this ACL triggerred access deny failure, there is Quota triggered write failure, which I haven't find a proper s3 errorcode and HTTP code to return to use so far.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1642: HDDS-4519. Return forbidden instead of interval server error from s3g…
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1642:
URL: https://github.com/apache/ozone/pull/1642#discussion_r535843068
##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java
##########
@@ -780,7 +780,12 @@ public OzoneMultipartUploadList listMultipartUploads(String prefix)
@Override
public boolean hasNext() {
if(!currentIterator.hasNext() && currentValue != null) {
- currentIterator = getNextListOfKeys(currentValue.getName()).iterator();
+ try {
+ currentIterator =
+ getNextListOfKeys(currentValue.getName()).iterator();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
Review comment:
Any specific reason for throwing RuntimeException here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1642: HDDS-4519. Return forbidden instead of interval server error from s3g…
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1642:
URL: https://github.com/apache/ozone/pull/1642#discussion_r535854667
##########
File path: hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3ErrorTable.java
##########
@@ -105,6 +106,9 @@ private S3ErrorTable() {
"InternalError", "We encountered an internal error. Please try again.",
HTTP_SERVER_ERROR);
+ public static final OS3Exception PERMISSION_DENIED = new OS3Exception(
Review comment:
Do we think, we want to change it to AccessDenied, to be closer to S3
https://docs.aws.amazon.com/AmazonS3/latest/API/ErrorResponses.html#RESTErrorResponses
AccessDenied 403
https://aws.amazon.com/premiumsupport/knowledge-center/s3-troubleshoot-403/ Even though we don't have S3 Acls, Ozone Acls causing this, but to be close to S3 I think error code having same might be better here IMHO.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi edited a comment on pull request #1642: HDDS-4519. Return forbidden instead of interval server error from s3g…
Posted by GitBox <gi...@apache.org>.
ChenSammi edited a comment on pull request #1642:
URL: https://github.com/apache/ozone/pull/1642#issuecomment-742494077
@bharatviswa504 @xiaoyuyao , uploaded a new commit to address the comments. Would you take another look at your convenient time?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #1642: HDDS-4519. Return forbidden instead of interval server error from s3g…
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on pull request #1642:
URL: https://github.com/apache/ozone/pull/1642#issuecomment-736321751
Hi, @bharatviswa504 and @elek, would you help to review the patch at your convenient time?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on pull request #1642: HDDS-4519. Return forbidden instead of interval server error from s3g…
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #1642:
URL: https://github.com/apache/ozone/pull/1642#issuecomment-744849062
>Besides this ACL triggerred access deny failure, there is Quota triggered write failure, which I haven't find a proper s3 >errorcode and HTTP code to return to use so far.
Does S3 Buckets support quota feature, this problem will occur only when buckets created by shell/ozone java interface used by S3 and have quota supported on these buckets we shall have this issue. If so, this is an internal ozone error, and might be we should construct errorcode from ozone internal code, but S3 tools will not understand this. Here in this kind of scenario, ozone and S3 semantics are combined, which will help to understand for ozone S3 users, but not for S3 tools.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1642: HDDS-4519. Return forbidden instead of interval server error from s3g…
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1642:
URL: https://github.com/apache/ozone/pull/1642#discussion_r535843068
##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java
##########
@@ -780,7 +780,12 @@ public OzoneMultipartUploadList listMultipartUploads(String prefix)
@Override
public boolean hasNext() {
if(!currentIterator.hasNext() && currentValue != null) {
- currentIterator = getNextListOfKeys(currentValue.getName()).iterator();
+ try {
+ currentIterator =
+ getNextListOfKeys(currentValue.getName()).iterator();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
Review comment:
Any reason for throwing RuntimeException here?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #1642: HDDS-4519. Return forbidden instead of interval server error from s3g…
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on pull request #1642:
URL: https://github.com/apache/ozone/pull/1642#issuecomment-737927376
> +1 Thanks the patch, @ChenSammi
>
> My first thought was to suggest using `ExceptionMapper<OMException>` instead of adding this exception handling manually to all places.
>
> Then I realized the problem: you need to add the context (bucket name, key name) to the results which is possible in this way. This patch does it well --> `+1`
>
> /brainstorming on
>
> In the future we may add the context information (bucket, key, volume) to the `OMException` which would make it possible to handle all the exceptions with less code.
>
> /brainstorming off
>
> Also, an additional (slightly unrelated) note: if `OzoneClient` itself couldn't be created (due to malformed auth header) still we throw 500 AFAIK. (Not related to this patch, just sharing my sadness ;-) ). Implementation of the design in #1562 may help on that one.
>
> (Not committing yet, to give a chance to @bharatviswa504 to check this)
Thanks @elek for the code review.
Yes. currently there is no central place to handle common exceptions, such as permission deny, not leader etc. We can improve this part later by refactoring current code structure.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] xiaoyuyao commented on a change in pull request #1642: HDDS-4519. Return forbidden instead of interval server error from s3g…
Posted by GitBox <gi...@apache.org>.
xiaoyuyao commented on a change in pull request #1642:
URL: https://github.com/apache/ozone/pull/1642#discussion_r536804873
##########
File path: hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java
##########
@@ -619,6 +631,9 @@ private Response createMultipartKey(String bucket, String key, long length,
if (ex.getResult() == ResultCodes.NO_SUCH_MULTIPART_UPLOAD_ERROR) {
throw S3ErrorTable.newError(NO_SUCH_UPLOAD,
uploadID);
+ } else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) {
Review comment:
Is it possible to have a common helper that does the OM result code => s3 error translation?
##########
File path: hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3ErrorTable.java
##########
@@ -105,6 +106,9 @@ private S3ErrorTable() {
"InternalError", "We encountered an internal error. Please try again.",
HTTP_SERVER_ERROR);
+ public static final OS3Exception PERMISSION_DENIED = new OS3Exception(
Review comment:
+1 for return 403
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on pull request #1642: HDDS-4519. Return forbidden instead of interval server error from s3g…
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on pull request #1642:
URL: https://github.com/apache/ozone/pull/1642#issuecomment-745114003
Thanks @bharatviswa504 and @xiaoyuyao for the code review.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] ChenSammi commented on a change in pull request #1642: HDDS-4519. Return forbidden instead of interval server error from s3g…
Posted by GitBox <gi...@apache.org>.
ChenSammi commented on a change in pull request #1642:
URL: https://github.com/apache/ozone/pull/1642#discussion_r539808735
##########
File path: hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java
##########
@@ -619,6 +631,9 @@ private Response createMultipartKey(String bucket, String key, long length,
if (ex.getResult() == ResultCodes.NO_SUCH_MULTIPART_UPLOAD_ERROR) {
throw S3ErrorTable.newError(NO_SUCH_UPLOAD,
uploadID);
+ } else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) {
Review comment:
In long term，it's good to have such a mapping table.
But there are many details need further discussion, for example, a error response has HTTP error code and s3 error code. Multiple s3 error codse will map to the same HTTP error code.
Some OM result codes, like QUOTA_EXCEEDED, I quickly go through all HTTP codes, didn't find a perfect match code.
Other OM result codes, such as NOT_SUPPORTED_OPERATION, is just translated to "interval error" in s3g.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1642: HDDS-4519. Return forbidden instead of interval server error from s3g…
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1642:
URL: https://github.com/apache/ozone/pull/1642#discussion_r535844460
##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java
##########
@@ -780,7 +780,12 @@ public OzoneMultipartUploadList listMultipartUploads(String prefix)
@Override
public boolean hasNext() {
if(!currentIterator.hasNext() && currentValue != null) {
- currentIterator = getNextListOfKeys(currentValue.getName()).iterator();
+ try {
+ currentIterator =
+ getNextListOfKeys(currentValue.getName()).iterator();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
Review comment:
I see the current code is doing the same. Do you think we need to change here to handle error?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1642: HDDS-4519. Return forbidden instead of interval server error from s3g…
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1642:
URL: https://github.com/apache/ozone/pull/1642#discussion_r535844460
##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java
##########
@@ -780,7 +780,12 @@ public OzoneMultipartUploadList listMultipartUploads(String prefix)
@Override
public boolean hasNext() {
if(!currentIterator.hasNext() && currentValue != null) {
- currentIterator = getNextListOfKeys(currentValue.getName()).iterator();
+ try {
+ currentIterator =
+ getNextListOfKeys(currentValue.getName()).iterator();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
Review comment:
I see the current code is doing the same. Do you think we need to change here to handle error? (Not related to this PR BTW)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #1642: HDDS-4519. Return forbidden instead of interval server error from s3g…
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1642:
URL: https://github.com/apache/ozone/pull/1642#discussion_r535844460
##########
File path: hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java
##########
@@ -780,7 +780,12 @@ public OzoneMultipartUploadList listMultipartUploads(String prefix)
@Override
public boolean hasNext() {
if(!currentIterator.hasNext() && currentValue != null) {
- currentIterator = getNextListOfKeys(currentValue.getName()).iterator();
+ try {
+ currentIterator =
+ getNextListOfKeys(currentValue.getName()).iterator();
+ } catch (IOException e) {
+ throw new RuntimeException(e);
Review comment:
I see the current code is doing the same. Do you think we need to change here to handle error? (Not related to this PR BTW). I see the reason as hasNext() does not throw any exception.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org