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

Expose operationName for S3 requests #800

Merged
merged 6 commits into from
Jun 21, 2024
Merged
Show file tree
Hide file tree
Changes from 4 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: 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
9 changes: 7 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,8 @@ public S3MetaRequest makeMetaRequest(S3MetaRequestOptions options) {
throw new IllegalArgumentException("S3Client.makeMetaRequest has invalid options; Response Handler cannot be null.");
}

String operationName = options.getOperationName();

S3MetaRequest metaRequest = new S3MetaRequest();
S3MetaRequestResponseHandlerNativeAdapter responseHandlerNativeAdapter = new S3MetaRequestResponseHandlerNativeAdapter(
options.getResponseHandler());
Expand All @@ -163,7 +165,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 +236,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;
}
waahm7 marked this conversation as resolved.
Show resolved Hide resolved

/*
* 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;
}
waahm7 marked this conversation as resolved.
Show resolved Hide resolved
}

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
Loading