Skip to content

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

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.

Sign up for GitHub

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

Merged
ChenSammi merged 3 commits into from
Dec 15, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bba1a84
HDDS-4519. Return forbidden instead of interval server error from s3g…
ChenSammi Dec 1, 2020
748aab0
Fix checkstyle; Add more test cases
ChenSammi Dec 3, 2020
d4cd1f8
address comments
ChenSammi Dec 10, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 14 additions & 12 deletions hadoop-ozone/client/src/main/java/org/apache/hadoop/ozone/client/OzoneBucket.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,7 +517,8 @@ public long getUsedBytes() {
* @param keyPrefix Bucket prefix to match
* @return {@code Iterator<OzoneKey>}
*/
public Iterator<? extends OzoneKey> listKeys(String keyPrefix) {
public Iterator<? extends OzoneKey> listKeys(String keyPrefix)
throws IOException{
return listKeys(keyPrefix, null);
}

Expand All @@ -532,7 +533,7 @@ public Iterator<? extends OzoneKey> listKeys(String keyPrefix) {
* @return {@code Iterator<OzoneKey>}
*/
public Iterator<? extends OzoneKey> listKeys(String keyPrefix,
String prevKey) {
String prevKey) throws IOException {
return new KeyIterator(keyPrefix, prevKey);
}

Expand Down Expand Up @@ -760,7 +761,6 @@ public OzoneMultipartUploadList listMultipartUploads(String prefix)
private class KeyIterator implements Iterator<OzoneKey> {

private String keyPrefix = null;

private Iterator<OzoneKey> currentIterator;
private OzoneKey currentValue;

Expand All @@ -771,7 +771,7 @@ private class KeyIterator implements Iterator<OzoneKey> {
* The returned keys match key prefix.
* @param keyPrefix
*/
KeyIterator(String keyPrefix, String prevKey) {
KeyIterator(String keyPrefix, String prevKey) throws IOException{
this.keyPrefix = keyPrefix;
this.currentValue = null;
this.currentIterator = getNextListOfKeys(prevKey).iterator();
Expand All @@ -780,7 +780,12 @@ private class KeyIterator implements Iterator<OzoneKey> {
@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);
Copy link
Contributor

@bharatviswa504 bharatviswa504 Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any specific reason for throwing RuntimeException here?

Copy link
Contributor

@bharatviswa504 bharatviswa504 Dec 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

}
}
return currentIterator.hasNext();
}
Expand All @@ -799,13 +804,10 @@ public OzoneKey next() {
* @param prevKey
* @return {@code List<OzoneKey>}
*/
private List<OzoneKey> getNextListOfKeys(String prevKey) {
try {
return proxy.listKeys(volumeName, name, keyPrefix, prevKey,
listCacheSize);
} catch (IOException e) {
throw new RuntimeException(e);
}
private List<OzoneKey> getNextListOfKeys(String prevKey) throws
IOException {
return proxy.listKeys(volumeName, name, keyPrefix, prevKey,
listCacheSize);
}
}
}
4 changes: 3 additions & 1 deletion hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/OzoneManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1805,7 +1805,9 @@ public boolean checkAcls(ResourceType resType, StoreType storeType,
obj.getKeyName());
throw new OMException("User " + context.getClientUgi().getUserName() +
" doesn't have " + context.getAclRights() +
" permission to access " + obj.getResourceType(),
" permission to access " + obj.getResourceType() + " " +
obj.getVolumeName() + " " + obj.getBucketName() + " " +
obj.getKeyName(),
ResultCodes.PERMISSION_DENIED);
}
return false;
Expand Down
2 changes: 1 addition & 1 deletion .../ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicOzoneClientAdapterImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ public FileStatusAdapter getFileStatus(String key, URI uri,


@Override
public Iterator<BasicKeyInfo> listKeys(String pathKey) {
public Iterator<BasicKeyInfo> listKeys(String pathKey) throws IOException{
incrementCounter(Statistic.OBJECTS_LIST, 1);
return new IteratorAdapter(bucket.listKeys(pathKey));
}
Expand Down
2 changes: 1 addition & 1 deletion ...fs-common/src/main/java/org/apache/hadoop/fs/ozone/BasicRootedOzoneClientAdapterImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -615,7 +615,7 @@ public Collection<FileStatus> getTrashRoots(boolean allUsers,
}

@Override
public Iterator<BasicKeyInfo> listKeys(String pathStr) {
public Iterator<BasicKeyInfo> listKeys(String pathStr) throws IOException {
incrementCounter(Statistic.OBJECTS_LIST, 1);
OFSPath ofsPath = new OFSPath(pathStr);
String key = ofsPath.getKeyName();
Expand Down
2 changes: 1 addition & 1 deletion hadoop-ozone/ozonefs-common/src/main/java/org/apache/hadoop/fs/ozone/OzoneClientAdapter.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ OzoneFSOutputStream createFile(String key, short replication,

boolean deleteObjects(List<String> keyName);

Iterator<BasicKeyInfo> listKeys(String pathKey);
Iterator<BasicKeyInfo> listKeys(String pathKey) throws IOException;

List<FileStatusAdapter> listStatus(String keyName, boolean recursive,
String startKey, long numEntries, URI uri,
Expand Down
49 changes: 35 additions & 14 deletions hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/BucketEndpoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,19 +116,26 @@ public Response list(
if (startAfter == null && marker != null) {
startAfter = marker;
}
if (startAfter != null && continueToken != null) {
// If continuation token and start after both are provided, then we
// ignore start After
ozoneKeyIterator = bucket.listKeys(prefix, decodedToken.getLastKey());
} else if (startAfter != null && continueToken == null) {
ozoneKeyIterator = bucket.listKeys(prefix, startAfter);
} else if (startAfter == null && continueToken != null){
ozoneKeyIterator = bucket.listKeys(prefix, decodedToken.getLastKey());
} else {
ozoneKeyIterator = bucket.listKeys(prefix);
try {
if (startAfter != null && continueToken != null) {
// If continuation token and start after both are provided, then we
// ignore start After
ozoneKeyIterator = bucket.listKeys(prefix, decodedToken.getLastKey());
} else if (startAfter != null && continueToken == null) {
ozoneKeyIterator = bucket.listKeys(prefix, startAfter);
} else if (startAfter == null && continueToken != null) {
ozoneKeyIterator = bucket.listKeys(prefix, decodedToken.getLastKey());
} else {
ozoneKeyIterator = bucket.listKeys(prefix);
}
} catch (OMException ex) {
if (ex.getResult() == ResultCodes.PERMISSION_DENIED) {
throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, bucketName);
} else {
throw ex;
}
}


ListObjectResponse response = new ListObjectResponse();
response.setDelimiter(delimiter);
response.setName(bucketName);
Expand Down Expand Up @@ -229,8 +236,16 @@ public Response listMultipartUploads(

OzoneBucket bucket = getBucket(bucketName);

OzoneMultipartUploadList ozoneMultipartUploadList =
bucket.listMultipartUploads(prefix);
OzoneMultipartUploadList ozoneMultipartUploadList;
try {
ozoneMultipartUploadList = bucket.listMultipartUploads(prefix);
} catch (OMException exception) {
if (exception.getResult() == ResultCodes.PERMISSION_DENIED) {
throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED,
prefix);
}
throw exception;
}

ListMultipartUploadsResult result = new ListMultipartUploadsResult();
result.setBucket(bucketName);
Expand Down Expand Up @@ -282,6 +297,8 @@ public Response delete(@PathParam("bucket") String bucketName)
} else if (ex.getResult() == ResultCodes.BUCKET_NOT_FOUND) {
throw S3ErrorTable.newError(S3ErrorTable
.NO_SUCH_BUCKET, bucketName);
} else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) {
throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, bucketName);
} else {
throw ex;
}
Expand Down Expand Up @@ -315,7 +332,11 @@ public MultiDeleteResponse multiDelete(@PathParam("bucket") String bucketName,
result.addDeleted(new DeletedObject(keyToDelete.getKey()));
}
} catch (OMException ex) {
if (ex.getResult() != ResultCodes.KEY_NOT_FOUND) {
if (ex.getResult() == ResultCodes.PERMISSION_DENIED) {
result.addError(
new Error(keyToDelete.getKey(), "PermissionDenied",
ex.getMessage()));
} else if (ex.getResult() != ResultCodes.KEY_NOT_FOUND) {
result.addError(
new Error(keyToDelete.getKey(), "InternalError",
ex.getMessage()));
Expand Down
29 changes: 22 additions & 7 deletions hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/EndpointBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,8 @@ protected OzoneBucket getBucket(String bucketName)
if (ex.getResult() == ResultCodes.BUCKET_NOT_FOUND
|| ex.getResult() == ResultCodes.VOLUME_NOT_FOUND) {
throw S3ErrorTable.newError(S3ErrorTable.NO_SUCH_BUCKET, bucketName);
} else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) {
throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, bucketName);
} else {
throw ex;
}
Expand All @@ -91,11 +93,13 @@ protected OzoneVolume getVolume() throws IOException {
* @throws IOException
*/
protected String createS3Bucket(String bucketName) throws
IOException {
IOException, OS3Exception {
try {
client.getObjectStore().createS3Bucket(bucketName);
} catch (OMException ex) {
if (ex.getResult() != ResultCodes.BUCKET_ALREADY_EXISTS) {
if (ex.getResult() == ResultCodes.PERMISSION_DENIED) {
throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, bucketName);
} else if (ex.getResult() != ResultCodes.BUCKET_ALREADY_EXISTS) {
// S3 does not return error for bucket already exists, it just
// returns the location.
throw ex;
Expand All @@ -110,8 +114,16 @@ protected String createS3Bucket(String bucketName) throws
* @throws IOException in case the bucket cannot be deleted.
*/
public void deleteS3Bucket(String s3BucketName)
throws IOException {
client.getObjectStore().deleteS3Bucket(s3BucketName);
throws IOException, OS3Exception {
try {
client.getObjectStore().deleteS3Bucket(s3BucketName);
} catch (OMException ex) {
if (ex.getResult() == ResultCodes.PERMISSION_DENIED) {
throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED,
s3BucketName);
}
throw ex;
}
}

/**
Expand All @@ -123,7 +135,7 @@ public void deleteS3Bucket(String s3BucketName)
* @return {@code Iterator<OzoneBucket>}
*/
public Iterator<? extends OzoneBucket> listS3Buckets(String prefix)
throws IOException {
throws IOException, OS3Exception {
return iterateBuckets(volume -> volume.listBuckets(prefix));
}

Expand All @@ -138,18 +150,21 @@ public Iterator<? extends OzoneBucket> listS3Buckets(String prefix)
* @return {@code Iterator<OzoneBucket>}
*/
public Iterator<? extends OzoneBucket> listS3Buckets(String prefix,
String previousBucket) throws IOException {
String previousBucket) throws IOException, OS3Exception {
return iterateBuckets(volume -> volume.listBuckets(prefix, previousBucket));
}

private Iterator<? extends OzoneBucket> iterateBuckets(
Function<OzoneVolume, Iterator<? extends OzoneBucket>> query)
throws IOException {
throws IOException, OS3Exception{
try {
return query.apply(getVolume());
} catch (OMException e) {
if (e.getResult() == ResultCodes.VOLUME_NOT_FOUND) {
return Collections.emptyIterator();
} else if (e.getResult() == ResultCodes.PERMISSION_DENIED) {
throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED,
"listBuckets");
} else {
throw e;
}
Expand Down
23 changes: 22 additions & 1 deletion hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/endpoint/ObjectEndpoint.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -209,6 +209,9 @@ public Response put(
" considered as Unix Paths. Path has Violated FS Semantics " +
"which caused put operation to fail.");
throw os3Exception;
} else if ((((OMException) ex).getResult() ==
ResultCodes.PERMISSION_DENIED)) {
throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, keyPath);
}
}
throw ex;
Expand Down Expand Up @@ -320,6 +323,8 @@ public Response get(
if (ex.getResult() == ResultCodes.KEY_NOT_FOUND) {
throw S3ErrorTable.newError(S3ErrorTable
.NO_SUCH_KEY, keyPath);
} else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) {
throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, keyPath);
} else {
throw ex;
}
Expand Down Expand Up @@ -357,6 +362,8 @@ public Response head(
if (ex.getResult() == ResultCodes.KEY_NOT_FOUND) {
// Just return 404 with no content
return Response.status(Status.NOT_FOUND).build();
} else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) {
throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, keyPath);
} else {
throw ex;
}
Expand Down Expand Up @@ -426,6 +433,8 @@ public Response delete(
} else if (ex.getResult() == ResultCodes.KEY_NOT_FOUND) {
//NOT_FOUND is not a problem, AWS doesn't throw exception for missing
// keys. Just return 204
} else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) {
throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, keyPath);
} else {
throw ex;
}
Expand Down Expand Up @@ -474,9 +483,12 @@ public Response initializeMultipartUpload(

return Response.status(Status.OK).entity(
multipartUploadInitiateResponse).build();
} catch (IOException ex) {
} catch (OMException ex) {
LOG.error("Error in Initiate Multipart Upload Request for bucket: {}, " +
"key: {}", bucket, key, ex);
if (ex.getResult() == ResultCodes.PERMISSION_DENIED) {
throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED, key);
}
throw ex;
}
}
Expand Down Expand Up @@ -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) {
Copy link
Contributor

@xiaoyuyao xiaoyuyao Dec 5, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@ChenSammi ChenSammi Dec 10, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED,
bucket + "/" + key);
}
throw ex;
}
Expand Down Expand Up @@ -675,6 +690,9 @@ private Response listParts(String bucket, String key, String uploadID,
if (ex.getResult() == ResultCodes.NO_SUCH_MULTIPART_UPLOAD_ERROR) {
throw S3ErrorTable.newError(NO_SUCH_UPLOAD,
uploadID);
} else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) {
throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED,
bucket + "/" + key + "/" + uploadID);
}
throw ex;
}
Expand Down Expand Up @@ -760,6 +778,9 @@ private CopyObjectResponse copyObject(String copyHeader,
throw S3ErrorTable.newError(S3ErrorTable.NO_SUCH_KEY, sourceKey);
} else if (ex.getResult() == ResultCodes.BUCKET_NOT_FOUND) {
throw S3ErrorTable.newError(S3ErrorTable.NO_SUCH_BUCKET, sourceBucket);
} else if (ex.getResult() == ResultCodes.PERMISSION_DENIED) {
throw S3ErrorTable.newError(S3ErrorTable.ACCESS_DENIED,
destBucket + "/" + destkey);
}
throw ex;
} finally {
Expand Down
4 changes: 4 additions & 0 deletions hadoop-ozone/s3gateway/src/main/java/org/apache/hadoop/ozone/s3/exception/S3ErrorTable.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@

import static java.net.HttpURLConnection.HTTP_BAD_REQUEST;
import static java.net.HttpURLConnection.HTTP_CONFLICT;
import static java.net.HttpURLConnection.HTTP_FORBIDDEN;
import static java.net.HttpURLConnection.HTTP_NOT_FOUND;
import static java.net.HttpURLConnection.HTTP_SERVER_ERROR;
import static org.apache.hadoop.ozone.s3.util.S3Consts.RANGE_NOT_SATISFIABLE;
Expand Down Expand Up @@ -105,6 +106,9 @@ private S3ErrorTable() {
"InternalError", "We encountered an internal error. Please try again.",
HTTP_SERVER_ERROR);

public static final OS3Exception ACCESS_DENIED = new OS3Exception(
"AccessDenied", "User doesn't have the right to access this " +
"resource.", HTTP_FORBIDDEN);

/**
* Create a new instance of Error.
Expand Down
Loading