Skip to content
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

HDDS-11457. Internal error on S3 CompleteMultipartUpload if parts are not specified #7195

Merged
merged 2 commits into from
Sep 16, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 5 additions & 0 deletions hadoop-ozone/dist/src/main/smoketest/s3/MultipartUpload.robot
Original file line number Diff line number Diff line change
Expand Up @@ -107,6 +107,11 @@ Test Multipart Upload Complete
${part2Md5Sum} = Execute md5sum /tmp/part2 | awk '{print $1}'
Should Be Equal As Strings ${eTag2} ${part2Md5Sum}

#complete multipart upload without any parts
${result} = Execute AWSS3APICli and checkrc complete-multipart-upload --upload-id ${uploadID} --bucket ${BUCKET} --key ${PREFIX}/multipartKey1 255
Should contain ${result} InvalidRequest
Should contain ${result} must specify at least one part

#complete multipart upload
${result} = Execute AWSS3APICli complete-multipart-upload --upload-id ${uploadID} --bucket ${BUCKET} --key ${PREFIX}/multipartKey1 --multipart-upload 'Parts=[{ETag=${eTag1},PartNumber=1},{ETag=${eTag2},PartNumber=2}]'
Should contain ${result} ${BUCKET}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,11 +19,9 @@

import javax.annotation.Priority;
import javax.inject.Inject;
import javax.ws.rs.WebApplicationException;
import javax.ws.rs.container.ContainerRequestContext;
import javax.ws.rs.container.ContainerRequestFilter;
import javax.ws.rs.container.PreMatching;
import javax.ws.rs.core.Response;
import javax.ws.rs.ext.Provider;

import com.google.common.annotations.VisibleForTesting;
Expand All @@ -41,6 +39,7 @@
import static org.apache.hadoop.ozone.s3.exception.S3ErrorTable.ACCESS_DENIED;
import static org.apache.hadoop.ozone.s3.exception.S3ErrorTable.INTERNAL_ERROR;
import static org.apache.hadoop.ozone.s3.exception.S3ErrorTable.S3_AUTHINFO_CREATION_ERROR;
import static org.apache.hadoop.ozone.s3.util.S3Utils.wrapOS3Exception;

/**
* Filter used to construct string to sign from unfiltered request.
Expand Down Expand Up @@ -116,10 +115,4 @@ public SignatureInfo getSignatureInfo() {
return signatureInfo;
}

private WebApplicationException wrapOS3Exception(OS3Exception os3Exception) {
return new WebApplicationException(os3Exception.getErrorMessage(),
os3Exception,
Response.status(os3Exception.getHttpCode())
.entity(os3Exception.toXml()).build());
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@
import java.lang.reflect.Type;
import javax.ws.rs.ext.Provider;

import static org.apache.hadoop.ozone.s3.exception.S3ErrorTable.INVALID_REQUEST;
import static org.apache.hadoop.ozone.s3.util.S3Consts.S3_XML_NAMESPACE;
import static org.apache.hadoop.ozone.s3.util.S3Utils.wrapOS3Exception;

/**
* Custom unmarshaller to read CompleteMultipartUploadRequest wo namespace.
Expand Down Expand Up @@ -69,6 +71,10 @@ public CompleteMultipartUploadRequest readFrom(
MultivaluedMap<String, String> multivaluedMap,
InputStream inputStream) throws IOException, WebApplicationException {
try {
if (inputStream.available() == 0) {
throw wrapOS3Exception(INVALID_REQUEST.withMessage("You must specify at least one part"));
}

XMLReader xmlReader = saxParserFactory.newSAXParser().getXMLReader();
UnmarshallerHandler unmarshallerHandler =
context.createUnmarshaller().getUnmarshallerHandler();
Expand All @@ -78,8 +84,11 @@ public CompleteMultipartUploadRequest readFrom(
filter.setParent(xmlReader);
filter.parse(new InputSource(inputStream));
return (CompleteMultipartUploadRequest) unmarshallerHandler.getResult();
} catch (WebApplicationException e) {
throw e;
} catch (Exception e) {
throw new WebApplicationException("Can't parse request body to XML.", e);
throw wrapOS3Exception(INVALID_REQUEST.withMessage(e.getMessage()));
}
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
package org.apache.hadoop.ozone.s3.endpoint;

import javax.ws.rs.Produces;
import javax.ws.rs.WebApplicationException;
import javax.ws.rs.core.MediaType;
import javax.ws.rs.core.MultivaluedMap;
import javax.ws.rs.ext.MessageBodyReader;
Expand All @@ -34,6 +33,9 @@
import org.xml.sax.InputSource;
import org.xml.sax.XMLReader;

import static org.apache.hadoop.ozone.s3.exception.S3ErrorTable.INVALID_REQUEST;
import static org.apache.hadoop.ozone.s3.util.S3Utils.wrapOS3Exception;

/**
* Custom unmarshaller to read MultiDeleteRequest w/wo namespace.
*/
Expand Down Expand Up @@ -78,7 +80,7 @@ public MultiDeleteRequest readFrom(Class<MultiDeleteRequest> type,
filter.parse(new InputSource(entityStream));
return (MultiDeleteRequest) unmarshallerHandler.getResult();
} catch (Exception e) {
throw new WebApplicationException("Can't parse request body to XML.", e);
throw wrapOS3Exception(INVALID_REQUEST.withMessage(e.getMessage()));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@
import java.lang.annotation.Annotation;
import java.lang.reflect.Type;

import static org.apache.hadoop.ozone.s3.exception.S3ErrorTable.INVALID_REQUEST;
import static org.apache.hadoop.ozone.s3.util.S3Consts.S3_XML_NAMESPACE;
import static org.apache.hadoop.ozone.s3.util.S3Utils.wrapOS3Exception;

/**
* Custom unmarshaller to read PutBucketAclRequest wo namespace.
Expand Down Expand Up @@ -79,7 +81,7 @@ public S3BucketAcl readFrom(
filter.parse(new InputSource(inputStream));
return (S3BucketAcl)(unmarshallerHandler.getResult());
} catch (Exception e) {
throw new WebApplicationException("Can't parse request body to XML.", e);
throw wrapOS3Exception(INVALID_REQUEST.withMessage(e.getMessage()));
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -158,4 +158,9 @@ public String toXml() {
this.getErrorMessage(), this.getResource(),
this.getRequestId());
}

/** Create a copy with specific message. */
public OS3Exception withMessage(String message) {
return new OS3Exception(code, message, httpCode);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,8 @@
import org.apache.hadoop.hdds.protocol.proto.HddsProtos;
import org.apache.hadoop.ozone.s3.exception.OS3Exception;

import javax.ws.rs.WebApplicationException;
import javax.ws.rs.core.Response;
import java.io.UnsupportedEncodingException;
import java.net.URLDecoder;
import java.net.URLEncoder;
Expand Down Expand Up @@ -116,4 +118,11 @@ public static S3StorageType toS3StorageType(String storageType)
throw newError(INVALID_ARGUMENT, storageType, ex);
}
}

public static WebApplicationException wrapOS3Exception(OS3Exception ex) {
return new WebApplicationException(ex.getErrorMessage(), ex,
Response.status(ex.getHttpCode())
.entity(ex.toXml())
.build());
}
Comment on lines +122 to +127
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: AuthorizationFilter also seems to use the same method, we can also replace the wrapOS3Exception in AuthorizationFilter with this.

}