Skip to content

Commit

Permalink
Expose operationName for S3 requests (#800)
Browse files Browse the repository at this point in the history
**Issue:**
If an `S3MetaRequestType.PUT_OBJECT` failed, you couldn't tell exactly which S3 operation failed. Was it PutObject? StartMultipartUpload? UploadPart? CompleteMultipartUpload?

**Description of changes:**
- Expose S3 operation name in `S3FinishedResponseContext`
- Add `S3MetaRequestOptions.withOperationName(String)`, so users sending requests of type `S3MetaRequestType.DEFAULT` can pass the actual operation name through.
    - You _SHOULD_ set this for `DEFAULT` requests. It will be required in the very near future
- aws-lc: AWS-LC-FIPS-2.0.12 -> AWS-LC-FIPS-2.0.13
  • Loading branch information
graebm authored Jun 21, 2024
1 parent d3ab776 commit a902272
Show file tree
Hide file tree
Showing 11 changed files with 124 additions and 22 deletions.
5 changes: 3 additions & 2 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -184,9 +184,10 @@ set(TARGET_LIB_DIR "${AWS_LIBRARY_OUTPUT_DIR}/${TARGET_OS}/${TARGET_ARCH}/${AWS_

# shared lib that contains the CRT and JNI bindings, to be loaded by java
add_library(${PROJECT_NAME} SHARED ${CRT_JAVA_HEADERS} ${CRT_JAVA_SRC})
aws_use_package(aws-c-http)

# link the high-level libraries that will recursively pull in the rest
# (don't repeat dependencies here, or the linker will spit out warnings)
aws_use_package(aws-c-mqtt)
aws_use_package(aws-c-auth)
aws_use_package(aws-c-event-stream)
aws_use_package(aws-c-s3)

Expand Down
2 changes: 1 addition & 1 deletion crt/aws-lc
15 changes: 13 additions & 2 deletions src/main/java/software/amazon/awssdk/crt/s3/S3Client.java
Original file line number Diff line number Diff line change
Expand Up @@ -141,6 +141,14 @@ public S3MetaRequest makeMetaRequest(S3MetaRequestOptions options) {
throw new IllegalArgumentException("S3Client.makeMetaRequest has invalid options; Response Handler cannot be null.");
}

String operationName = options.getOperationName();
if (operationName == null && options.getMetaRequestType() == S3MetaRequestOptions.MetaRequestType.DEFAULT) {
// The upcoming release of aws-c-s3 v0.6.0 will require an operation name for DEFAULT meta-requests.
// Set a default value for now, in case this code gets mixed with the upcoming aws-c-s3 release.
// But when we take the upcoming release for real, change this code to throw IllegalArgumentException.
operationName = new String("DEFAULT");
}

S3MetaRequest metaRequest = new S3MetaRequest();
S3MetaRequestResponseHandlerNativeAdapter responseHandlerNativeAdapter = new S3MetaRequestResponseHandlerNativeAdapter(
options.getResponseHandler());
Expand All @@ -163,7 +171,9 @@ public S3MetaRequest makeMetaRequest(S3MetaRequestOptions options) {
: new ChecksumConfig();

long metaRequestNativeHandle = s3ClientMakeMetaRequest(getNativeHandle(), metaRequest, region.getBytes(UTF8),
options.getMetaRequestType().getNativeValue(), checksumConfig.getChecksumLocation().getNativeValue(),
options.getMetaRequestType().getNativeValue(),
operationName == null ? null : operationName.getBytes(UTF8),
checksumConfig.getChecksumLocation().getNativeValue(),
checksumConfig.getChecksumAlgorithm().getNativeValue(), checksumConfig.getValidateChecksum(),
ChecksumAlgorithm.marshallAlgorithmsForJNI(checksumConfig.getValidateChecksumAlgorithmList()),
httpRequestBytes, options.getHttpRequest().getBodyStream(), requestFilePath, signingConfig,
Expand Down Expand Up @@ -232,7 +242,8 @@ private static native long s3ClientNew(S3Client thisObj, byte[] region, long cli
private static native void s3ClientDestroy(long client);

private static native long s3ClientMakeMetaRequest(long clientId, S3MetaRequest metaRequest, byte[] region,
int metaRequestType, int checksumLocation, int checksumAlgorithm, boolean validateChecksum,
int metaRequestType, byte[] operationName,
int checksumLocation, int checksumAlgorithm, boolean validateChecksum,
int[] validateAlgorithms, byte[] httpRequestBytes,
HttpRequestBodyStream httpRequestBodyStream, byte[] requestFilePath,
AwsSigningConfig signingConfig, S3MetaRequestResponseHandlerNativeAdapter responseHandlerNativeAdapter,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ public class S3FinishedResponseContext {
private final int errorCode;
private final int responseStatus;
private final byte[] errorPayload;
private final String errorOperationName;
private final ChecksumAlgorithm checksumAlgorithm;
private final boolean didValidateChecksum;

Expand All @@ -24,10 +25,11 @@ public class S3FinishedResponseContext {
* didValidateChecksum which is true if the response was validated.
* cause of the error such as a Java exception in a callback. Maybe NULL if there was no exception in a callback.
*/
S3FinishedResponseContext(final int errorCode, final int responseStatus, final byte[] errorPayload, final ChecksumAlgorithm checksumAlgorithm, final boolean didValidateChecksum, Throwable cause, final HttpHeader[] errorHeaders) {
S3FinishedResponseContext(final int errorCode, final int responseStatus, final byte[] errorPayload, final String errorOperationName, final ChecksumAlgorithm checksumAlgorithm, final boolean didValidateChecksum, Throwable cause, final HttpHeader[] errorHeaders) {
this.errorCode = errorCode;
this.responseStatus = responseStatus;
this.errorPayload = errorPayload;
this.errorOperationName = errorOperationName;
this.checksumAlgorithm = checksumAlgorithm;
this.didValidateChecksum = didValidateChecksum;
this.cause = cause;
Expand All @@ -53,6 +55,18 @@ public byte[] getErrorPayload() {
return this.errorPayload;
}

/**
* @return the name of the S3 operation that failed, in the case of a failed HTTP response.
* For example, if {@link S3MetaRequestOptions.MetaRequestType#PUT_OBJECT} fails
* this could be "PutObject", "CreateMultipartUpload", "UploadPart",
* "CompleteMultipartUpload", or others. For {@link S3MetaRequestOptions.MetaRequestType#DEFAULT},
* this is the name passed to {@link S3MetaRequestOptions#withOperationName}.
* May be null.
*/
public String getErrorOperationName() {
return this.errorOperationName;
}

/*
* if no checksum is found, or the request finished with an error the Algorithm will be None,
* otherwise the algorithm will correspond to the one attached to the object when uploaded.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ private static Map<Integer, MetaRequestType> buildEnumMapping() {
}

private MetaRequestType metaRequestType;
private String operationName;
private ChecksumConfig checksumConfig;
private HttpRequest httpRequest;
private Path requestFilePath;
Expand All @@ -99,6 +100,38 @@ public MetaRequestType getMetaRequestType() {
return metaRequestType;
}

/**
* The S3 operation name (eg: "CreateBucket"),
* this should be set for {@link MetaRequestType#DEFAULT},
* it is ignored for other meta request types since the operation is implicit.
*
* See <a href="https://docs.aws.amazon.com/AmazonS3/latest/API/API_Operations_Amazon_Simple_Storage_Service.html">
* S3 API documentation</a> for the canonical list of names.
*
* This name is used to fill out details in metrics and error reports.
* It also drives some operation-specific behavior.
* If you pass the wrong name, you risk getting the wrong behavior.
*
* For example, every operation except "GetObject" has its response checked
* for error, even if the HTTP status-code was 200 OK
* (see <a href=https://repost.aws/knowledge-center/s3-resolve-200-internalerror>knowledge center</a>).
* If you used the {@link MetaRequestType#DEFAULT DEFAULT} type to do
* <a href="https://docs.aws.amazon.com/AmazonS3/latest/API/API_GetObject.html">GetObject</a>,
* but mis-named it "Download", and the object looked like XML with an error code,
* then the meta-request would fail. You risk logging the full response body,
* and leaking sensitive data.
* @param operationName the operation name for this {@link MetaRequestType#DEFAULT} meta request
* @return this
*/
public S3MetaRequestOptions withOperationName(String operationName) {
this.operationName = operationName;
return this;
}

public String getOperationName() {
return operationName;
}

/**
* The config related to checksum used for the meta request. See {@link ChecksumConfig} for details.
* @param checksumConfig The checksum config used for the meta request
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ int onResponseBody(byte[] bodyBytesIn, long objectRangeStart, long objectRangeEn
return this.responseHandler.onResponseBody(ByteBuffer.wrap(bodyBytesIn), objectRangeStart, objectRangeEnd);
}

void onFinished(int errorCode, int responseStatus, byte[] errorPayload, int checksumAlgorithm, boolean didValidateChecksum, Throwable cause, final ByteBuffer headersBlob) {
void onFinished(int errorCode, int responseStatus, byte[] errorPayload, String errorOperationName, int checksumAlgorithm, boolean didValidateChecksum, Throwable cause, final ByteBuffer headersBlob) {
HttpHeader[] errorHeaders = headersBlob == null ? null : HttpHeader.loadHeadersFromMarshalledHeadersBlob(headersBlob);
S3FinishedResponseContext context = new S3FinishedResponseContext(errorCode, responseStatus, errorPayload, ChecksumAlgorithm.getEnumValueFromInteger(checksumAlgorithm), didValidateChecksum, cause, errorHeaders);
S3FinishedResponseContext context = new S3FinishedResponseContext(errorCode, responseStatus, errorPayload, errorOperationName, ChecksumAlgorithm.getEnumValueFromInteger(checksumAlgorithm), didValidateChecksum, cause, errorHeaders);
this.responseHandler.onFinished(context);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2044,6 +2044,7 @@
"int",
"int",
"byte[]",
"java.lang.String",
"int",
"boolean",
"java.lang.Throwable",
Expand Down Expand Up @@ -2122,4 +2123,4 @@
}
]
}
]
]
4 changes: 2 additions & 2 deletions src/native/java_class_ids.c
Original file line number Diff line number Diff line change
Expand Up @@ -714,8 +714,8 @@ static void s_cache_s3_meta_request_response_handler_native_adapter_properties(J
(*env)->GetMethodID(env, cls, "onResponseBody", "([BJJ)I");
AWS_FATAL_ASSERT(s3_meta_request_response_handler_native_adapter_properties.onResponseBody);

s3_meta_request_response_handler_native_adapter_properties.onFinished =
(*env)->GetMethodID(env, cls, "onFinished", "(II[BIZLjava/lang/Throwable;Ljava/nio/ByteBuffer;)V");
s3_meta_request_response_handler_native_adapter_properties.onFinished = (*env)->GetMethodID(
env, cls, "onFinished", "(II[BLjava/lang/String;IZLjava/lang/Throwable;Ljava/nio/ByteBuffer;)V");
AWS_FATAL_ASSERT(s3_meta_request_response_handler_native_adapter_properties.onFinished);

s3_meta_request_response_handler_native_adapter_properties.onResponseHeaders =
Expand Down
34 changes: 34 additions & 0 deletions src/native/s3_client.c
Original file line number Diff line number Diff line change
Expand Up @@ -755,6 +755,19 @@ static void s_on_s3_meta_request_finish_callback(
error_response_cursor = aws_byte_cursor_from_buf(error_response_body);
}
jbyteArray jni_payload = aws_jni_byte_array_from_cursor(env, &error_response_cursor);

jobject jni_operation_name = NULL;
if (meta_request_result->error_response_operation_name != NULL) {
jni_operation_name = aws_jni_string_from_string(env, meta_request_result->error_response_operation_name);
if (jni_operation_name == NULL) {
AWS_LOGF_ERROR(
AWS_LS_S3_META_REQUEST,
"id=%p: Ignored Exception from S3MetaRequest.onFinished.aws_jni_string_from_string",
(void *)meta_request);
aws_jni_check_and_clear_exception(env);
}
}

/* Only propagate java_exception if crt error code is callback failure */
jthrowable java_exception =
meta_request_result->error_code == AWS_ERROR_HTTP_CALLBACK_FAILURE ? callback_data->java_exception : NULL;
Expand Down Expand Up @@ -791,6 +804,7 @@ static void s_on_s3_meta_request_finish_callback(
meta_request_result->error_code,
meta_request_result->response_status,
jni_payload,
jni_operation_name,
meta_request_result->validation_algorithm,
meta_request_result->did_validate,
java_exception,
Expand All @@ -806,6 +820,10 @@ static void s_on_s3_meta_request_finish_callback(
(*env)->DeleteLocalRef(env, jni_payload);
}

if (jni_operation_name) {
(*env)->DeleteLocalRef(env, jni_operation_name);
}

aws_byte_buf_clean_up(&headers_buf);
if (java_error_headers_buffer) {
(*env)->DeleteLocalRef(env, java_error_headers_buffer);
Expand Down Expand Up @@ -943,6 +961,7 @@ JNIEXPORT jlong JNICALL Java_software_amazon_awssdk_crt_s3_S3Client_s3ClientMake
jobject java_s3_meta_request_jobject,
jbyteArray jni_region,
jint meta_request_type,
jbyteArray jni_operation_name,
jint checksum_location,
jint checksum_algorithm,
jboolean validate_response,
Expand All @@ -960,6 +979,8 @@ JNIEXPORT jlong JNICALL Java_software_amazon_awssdk_crt_s3_S3Client_s3ClientMake

struct aws_allocator *allocator = aws_jni_get_allocator();
struct aws_s3_client *client = (struct aws_s3_client *)jni_s3_client;
struct aws_byte_cursor operation_name;
AWS_ZERO_STRUCT(operation_name);
struct aws_byte_cursor request_filepath;
AWS_ZERO_STRUCT(request_filepath);
struct aws_s3_meta_request_resume_token *resume_token =
Expand Down Expand Up @@ -998,6 +1019,17 @@ JNIEXPORT jlong JNICALL Java_software_amazon_awssdk_crt_s3_S3Client_s3ClientMake
AWS_OP_SUCCESS == aws_apply_java_http_request_changes_to_native_request(
env, jni_marshalled_message_data, jni_http_request_body_stream, request_message));

if (jni_operation_name) {
operation_name = aws_jni_byte_cursor_from_jbyteArray_acquire(env, jni_operation_name);
if (operation_name.ptr == NULL) {
goto done;
}
if (operation_name.len == 0) {
aws_jni_throw_illegal_argument_exception(env, "Operation name cannot be empty");
goto done;
}
}

if (jni_request_filepath) {
request_filepath = aws_jni_byte_cursor_from_jbyteArray_acquire(env, jni_request_filepath);
if (request_filepath.ptr == NULL) {
Expand Down Expand Up @@ -1049,6 +1081,7 @@ JNIEXPORT jlong JNICALL Java_software_amazon_awssdk_crt_s3_S3Client_s3ClientMake

struct aws_s3_meta_request_options meta_request_options = {
.type = meta_request_type,
.operation_name = operation_name,
.checksum_config = &checksum_config,
.message = request_message,
.send_filepath = request_filepath,
Expand Down Expand Up @@ -1079,6 +1112,7 @@ JNIEXPORT jlong JNICALL Java_software_amazon_awssdk_crt_s3_S3Client_s3ClientMake
aws_s3_meta_request_resume_token_release(resume_token);
aws_jni_byte_cursor_from_jbyteArray_release(env, jni_region, region);
aws_http_message_release(request_message);
aws_jni_byte_cursor_from_jbyteArray_release(env, jni_operation_name, operation_name);
aws_jni_byte_cursor_from_jbyteArray_release(env, jni_request_filepath, request_filepath);
aws_uri_clean_up(&endpoint);
if (success) {
Expand Down
23 changes: 14 additions & 9 deletions src/test/java/software/amazon/awssdk/crt/test/S3ClientTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
import org.junit.Assert;
import org.junit.Assume;
import org.junit.Test;

import software.amazon.awssdk.crt.CRT;
import software.amazon.awssdk.crt.CrtRuntimeException;
import software.amazon.awssdk.crt.Log;
import software.amazon.awssdk.crt.auth.credentials.Credentials;
Expand All @@ -17,6 +19,7 @@
import software.amazon.awssdk.crt.s3.S3MetaRequestOptions.MetaRequestType;
import software.amazon.awssdk.crt.utils.ByteBufferUtils;

import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.assertThrows;
Expand Down Expand Up @@ -103,8 +106,9 @@ private S3Client createS3Client(S3ClientOptions options) {
}

private RuntimeException makeExceptionFromFinishedResponseContext(S3FinishedResponseContext context) {
return new RuntimeException(String.format("error code:(%d) response status code(%d), error payload(%s)",
return new RuntimeException(String.format("error code:(%d)(%s) response status code(%d), error payload(%s)",
context.getErrorCode(),
CRT.awsErrorName(context.getErrorCode()),
context.getResponseStatus(),
new String(context.getErrorPayload(), java.nio.charset.StandardCharsets.UTF_8),
context.getCause()));
Expand Down Expand Up @@ -330,7 +334,7 @@ public void onFinished(S3FinishedResponseContext context) {
Assert.fail(ex.getMessage());
}
}

@Test
public void testS3GetWithSizeHint() {
skipIfAndroid();
Expand Down Expand Up @@ -380,7 +384,7 @@ public void onFinished(S3FinishedResponseContext context) {
}

@Test
public void testS3GetErrorHeadersAreReported() {
public void testS3GetErrorFinishedResponseContextHasAllData() {
skipIfAndroid();
skipIfNetworkUnavailable();
Assume.assumeTrue(hasAwsCredentials());
Expand All @@ -390,12 +394,7 @@ public void testS3GetErrorHeadersAreReported() {
S3MetaRequestResponseHandler responseHandler = new S3MetaRequestResponseHandler() {

@Override
public int onResponseBody(ByteBuffer bodyBytesIn, long objectRangeStart, long objectRangeEnd) {
byte[] bytes = new byte[bodyBytesIn.remaining()];
bodyBytesIn.get(bytes);
Log.log(Log.LogLevel.Info, Log.LogSubject.JavaCrtS3, "Body Response: " + Arrays.toString(bytes));
return 0;
}
public int onResponseBody(ByteBuffer bodyBytesIn, long objectRangeStart, long objectRangeEnd) { return 0; }

@Override
public void onFinished(S3FinishedResponseContext context) {
Expand All @@ -404,6 +403,12 @@ public void onFinished(S3FinishedResponseContext context) {
try {
assertNotNull(context.getErrorHeaders());
assertTrue(context.getErrorCode() > 0);
assertTrue(context.getResponseStatus() >= 400);
assertNotNull(context.getErrorPayload());
String payload = new String(context.getErrorPayload(), StandardCharsets.UTF_8);
assertTrue(payload.contains("<Error>"));
assertNotNull(context.getErrorOperationName());
assertFalse(context.getErrorOperationName().isEmpty());
onFinishedFuture.complete(0); // Assertions passed
} catch (AssertionError e) {
onFinishedFuture.complete(-1); // Assertions failed
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,11 @@ public void onFinished(S3FinishedResponseContext context) {

AwsSigningConfig config = AwsSigningConfig.getDefaultS3SigningConfig(properties.getRegion(), null);
S3MetaRequestOptions metaRequestOptions = new S3MetaRequestOptions()
.withMetaRequestType(MetaRequestType.DEFAULT).withHttpRequest(httpRequest)
.withResponseHandler(responseHandler).withSigningConfig(config);
.withMetaRequestType(MetaRequestType.DEFAULT)
.withOperationName("CreateSession")
.withHttpRequest(httpRequest)
.withResponseHandler(responseHandler)
.withSigningConfig(config);

S3MetaRequest metaRequest = client.makeMetaRequest(metaRequestOptions);
future.whenComplete((r,t) -> {
Expand Down

0 comments on commit a902272

Please sign in to comment.