HDDS-4519. Return forbidden instead of interval server error from s3g… #1642

ChenSammi · 2020-12-01T08:52:27Z

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.

… when user doesn't have the resouce permission.

ChenSammi · 2020-12-01T08:53:42Z

Hi, @bharatviswa504 and @elek, would you help to review the patch at your convenient time?

elek

+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)

ChenSammi · 2020-12-03T12:36:47Z

+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.

bharatviswa504 · 2020-12-04T05:19:27Z

hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java

+          currentIterator =
+              getNextListOfKeys(currentValue.getName()).iterator();
+        } catch (IOException e) {
+          throw new RuntimeException(e);


Any specific reason for throwing RuntimeException here?

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.

bharatviswa504 · 2020-12-04T05:55:30Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3ErrorTable.java

      "InternalError", "We encountered an internal error. Please try again.",
      HTTP_SERVER_ERROR);

+  public static final OS3Exception PERMISSION_DENIED = new OS3Exception(


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.

+1 for return 403

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

bharatviswa504

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.

xiaoyuyao

LGTM, just comments inline...

xiaoyuyao · 2020-12-05T15:02:02Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3ErrorTable.java

      "InternalError", "We encountered an internal error. Please try again.",
      HTTP_SERVER_ERROR);

+  public static final OS3Exception PERMISSION_DENIED = new OS3Exception(


+1 for return 403

xiaoyuyao · 2020-12-05T15:02:50Z

hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java

      if (ex.getResult() == ResultCodes.NO_SUCH_MULTIPART_UPLOAD_ERROR) {
        throw S3ErrorTable.newError(NO_SUCH_UPLOAD,
            uploadID);
+      } else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) {


Is it possible to have a common helper that does the OM result code => s3 error translation?

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.

ChenSammi · 2020-12-10T02:43:45Z

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.

ChenSammi · 2020-12-10T12:34:59Z

@bharatviswa504 @xiaoyuyao , uploaded a new commit to address the comments. Would you take another look at your convenient time?

bharatviswa504

+1 LGTM

bharatviswa504 · 2020-12-15T00:04:58Z

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.

ChenSammi · 2020-12-15T07:40:31Z

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.

ChenSammi · 2020-12-15T07:41:48Z

Thanks @bharatviswa504 and @xiaoyuyao for the code review.

HDDS-4519. Return forbidden instead of interval server error from s3g…

bba1a84

… when user doesn't have the resouce permission.

ChenSammi requested review from bharatviswa504 and elek December 1, 2020 08:53

elek approved these changes Dec 1, 2020

View reviewed changes

Fix checkstyle; Add more test cases

748aab0

bharatviswa504 reviewed Dec 4, 2020

View reviewed changes

xiaoyuyao reviewed Dec 5, 2020

View reviewed changes

address comments

d4cd1f8

bharatviswa504 approved these changes Dec 15, 2020

View reviewed changes

ChenSammi merged commit 0957975 into apache:master Dec 15, 2020

HDDS-4519. Return forbidden instead of interval server error from s3g… #1642

HDDS-4519. Return forbidden instead of interval server error from s3g… #1642

Conversation

ChenSammi commented Dec 1, 2020

Uh oh!

ChenSammi commented Dec 1, 2020

Uh oh!

elek left a comment

Choose a reason for hiding this comment

Uh oh!

ChenSammi commented Dec 3, 2020

Uh oh!

bharatviswa504 Dec 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharatviswa504 Dec 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bharatviswa504 Dec 4, 2020

Choose a reason for hiding this comment

Uh oh!

xiaoyuyao Dec 5, 2020

Choose a reason for hiding this comment

Uh oh!

ChenSammi Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

bharatviswa504 left a comment

Choose a reason for hiding this comment

Uh oh!

xiaoyuyao left a comment

Choose a reason for hiding this comment

Uh oh!

xiaoyuyao Dec 5, 2020

Choose a reason for hiding this comment

Uh oh!

xiaoyuyao Dec 5, 2020

Choose a reason for hiding this comment

Uh oh!

ChenSammi Dec 10, 2020

Choose a reason for hiding this comment

Uh oh!

ChenSammi commented Dec 10, 2020

Uh oh!

ChenSammi commented Dec 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bharatviswa504 left a comment

Choose a reason for hiding this comment

Uh oh!

bharatviswa504 commented Dec 15, 2020

Uh oh!

ChenSammi commented Dec 15, 2020

Uh oh!

ChenSammi commented Dec 15, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bharatviswa504 Dec 4, 2020 •

edited

Loading

bharatviswa504 Dec 4, 2020 •

edited

Loading

ChenSammi commented Dec 10, 2020 •

edited

Loading