From 68b724eec91c9115cda4d2207b1942985bb4a001 Mon Sep 17 00:00:00 2001 From: Michael Graeb Date: Mon, 17 Jun 2024 09:01:19 -0700 Subject: [PATCH 01/10] require operation_name for DEFAULT meta-requests --- .../aws/s3/private/s3_default_meta_request.h | 2 +- include/aws/s3/private/s3_request.h | 4 +- include/aws/s3/private/s3_util.h | 8 +- include/aws/s3/s3_client.h | 8 +- source/s3.c | 74 ++++++++++++++++++- source/s3_client.c | 9 ++- source/s3_default_meta_request.c | 29 ++++---- source/s3_meta_request.c | 6 +- source/s3_request.c | 5 +- source/s3_util.c | 27 ------- tests/s3_tester.c | 2 + 11 files changed, 115 insertions(+), 59 deletions(-) diff --git a/include/aws/s3/private/s3_default_meta_request.h b/include/aws/s3/private/s3_default_meta_request.h index 4ce93f91a..da2b8a714 100644 --- a/include/aws/s3/private/s3_default_meta_request.h +++ b/include/aws/s3/private/s3_default_meta_request.h @@ -18,7 +18,7 @@ struct aws_s3_meta_request_default { /* Actual type for the single request (may be AWS_S3_REQUEST_TYPE_UNKNOWN) */ enum aws_s3_request_type request_type; - /* S3 operation name for the single request (NULL if unknown) */ + /* S3 operation name for the single request */ struct aws_string *operation_name; /* Members to only be used when the mutex in the base type is locked. */ diff --git a/include/aws/s3/private/s3_request.h b/include/aws/s3/private/s3_request.h index b77cf5231..c132246fc 100644 --- a/include/aws/s3/private/s3_request.h +++ b/include/aws/s3/private/s3_request.h @@ -85,7 +85,7 @@ struct aws_s3_request_metrics { struct aws_string *host_address; /* The the request ID header value. */ struct aws_string *request_id; - /* S3 operation name for the request (NULL if unknown) */ + /* S3 operation name for the request */ struct aws_string *operation_name; /* The type of request made */ enum aws_s3_request_type request_type; @@ -185,7 +185,7 @@ struct aws_s3_request { /* Actual S3 type for the single request (may be AWS_S3_REQUEST_TYPE_UNKNOWN) */ enum aws_s3_request_type request_type; - /* S3 operation name for the single request (e.g. "CompleteMultipartUpload") (NULL if unknown) */ + /* S3 operation name for the single request (e.g. "CompleteMultipartUpload") */ struct aws_string *operation_name; /* Members of this structure will be repopulated each time the request is sent. If the request fails, and needs to diff --git a/include/aws/s3/private/s3_util.h b/include/aws/s3/private/s3_util.h index b5aaa4e1c..1dbc172c3 100644 --- a/include/aws/s3/private/s3_util.h +++ b/include/aws/s3/private/s3_util.h @@ -10,7 +10,7 @@ #include #include -#include +#include #if ASSERT_LOCK_HELD # define ASSERT_SYNCED_DATA_LOCK_HELD(object) \ @@ -175,6 +175,12 @@ extern const struct aws_byte_cursor g_delete_method; AWS_S3_API extern const uint32_t g_s3_max_num_upload_parts; +/** + * Returns AWS_S3_REQUEST_TYPE_UNKNOWN if name doesn't map to an enum value. + */ +AWS_S3_API +enum aws_s3_request_type aws_s3_request_type_from_operation_name(struct aws_byte_cursor name); + /** * Cache and initial the signing config based on the client. * diff --git a/include/aws/s3/s3_client.h b/include/aws/s3/s3_client.h index 0989934c6..f2c8e0941 100644 --- a/include/aws/s3/s3_client.h +++ b/include/aws/s3/s3_client.h @@ -116,6 +116,7 @@ enum aws_s3_request_type { AWS_S3_REQUEST_TYPE_UPLOAD_PART_COPY, AWS_S3_REQUEST_TYPE_COPY_OBJECT, AWS_S3_REQUEST_TYPE_PUT_OBJECT, + AWS_S3_REQUEST_TYPE_CREATE_SESSION, /* Max enum value */ AWS_S3_REQUEST_TYPE_MAX, @@ -574,11 +575,12 @@ struct aws_s3_meta_request_options { enum aws_s3_meta_request_type type; /** - * Optional. * The S3 operation name (e.g. "CreateBucket"). - * This will only be used when type is AWS_S3_META_REQUEST_TYPE_DEFAULT; + * This must be set if type is AWS_S3_META_REQUEST_TYPE_DEFAULT; * it is automatically populated for other meta-request types. * This name is used to fill out details in metrics and error reports. + * It's also used to check for niche behavior specific to some operations + * (e.g. "CompleteMultipartUpload" may return an error, even though the status-code was 200 OK). */ struct aws_byte_cursor operation_name; @@ -780,7 +782,7 @@ struct aws_s3_meta_request_result { * "PutObject, "CreateMultipartUpload", "UploadPart", "CompleteMultipartUpload", or others. * For AWS_S3_META_REQUEST_TYPE_DEFAULT, this is the same value passed to * aws_s3_meta_request_options.operation_name. - * NULL if the meta request failed for another reason, or the operation name is not known. */ + * NULL if the meta request failed for another reason. */ struct aws_string *error_response_operation_name; /* Response status of the failed request or of the entire meta request. */ diff --git a/source/s3.c b/source/s3.c index 2ca1b3d75..6286262db 100644 --- a/source/s3.c +++ b/source/s3.c @@ -5,6 +5,7 @@ #include #include +#include #include #include @@ -75,6 +76,56 @@ static struct aws_log_subject_info_list s_s3_log_subject_list = { .count = AWS_ARRAY_SIZE(s_s3_log_subject_infos), }; +struct aws_s3_request_type_operation_name_pair { + enum aws_s3_request_type type; + struct aws_byte_cursor name; +}; + +#define DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(TYPE, NAME) \ + [TYPE] = {.type = TYPE, .name = AWS_BYTE_CUR_INIT_FROM_STRING_LITERAL(NAME)} + +static struct aws_s3_request_type_operation_name_pair s_s3_request_type_operation_name_array[] = { + DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_HEAD_OBJECT, "HeadObject"), + DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_GET_OBJECT, "GetObject"), + DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_LIST_PARTS, "ListParts"), + DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_CREATE_MULTIPART_UPLOAD, "CreateMultipartUpload"), + DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_UPLOAD_PART, "UploadPart"), + DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_ABORT_MULTIPART_UPLOAD, "AbortMultipartUpload"), + DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_COMPLETE_MULTIPART_UPLOAD, "CompleteMultipartUpload"), + DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_UPLOAD_PART_COPY, "UploadPartCopy"), + DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_COPY_OBJECT, "CopyObject"), + DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_PUT_OBJECT, "PutObject"), + DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_CREATE_SESSION, "CreateSession"), +}; + +/* Hash-table for case-insensitive lookup from operation-name -> request-type. + * key is `struct aws_byte_cursor *`, pointing into array above. + * value is `enum aws_s3_request_type *`, pointing into array above. */ +static struct aws_hash_table s_s3_operation_name_to_request_type_table; + +const char *aws_s3_request_type_operation_name(enum aws_s3_request_type type) { + + if (type >= 0 && type < AWS_ARRAY_SIZE(s_s3_request_type_operation_name_array)) { + const struct aws_s3_request_type_operation_name_pair *pair = &s_s3_request_type_operation_name_array[type]; + /* The array may have gaps in it */ + if (pair->name.len > 0) { + return (const char *)pair->name.ptr; + } + } + + return ""; +} + +enum aws_s3_request_type aws_s3_request_type_from_operation_name(struct aws_byte_cursor name) { + struct aws_hash_element *elem = NULL; + aws_hash_table_find(&s_s3_operation_name_to_request_type_table, &name, &elem); + if (elem != NULL) { + enum aws_s3_request_type *type = elem->value; + return *type; + } + return AWS_S3_REQUEST_TYPE_UNKNOWN; +} + static bool s_library_initialized = false; static struct aws_allocator *s_library_allocator = NULL; static struct aws_s3_platform_info_loader *s_loader; @@ -97,6 +148,26 @@ void aws_s3_library_init(struct aws_allocator *allocator) { aws_register_log_subject_info_list(&s_s3_log_subject_list); s_loader = aws_s3_platform_info_loader_new(allocator); AWS_FATAL_ASSERT(s_loader); + + /* Initialize s_s3_operation_name_to_request_type_table */ + int err = aws_hash_table_init( + &s_s3_operation_name_to_request_type_table, + allocator, + AWS_ARRAY_SIZE(s_s3_request_type_operation_name_array) /*initial_size*/, + aws_hash_byte_cursor_ptr_ignore_case, + (aws_hash_callback_eq_fn *)aws_byte_cursor_eq_ignore_case, + NULL /*destroy_key*/, + NULL /*destroy_value*/); + AWS_FATAL_ASSERT(!err); + for (size_t i = 0; i < AWS_ARRAY_SIZE(s_s3_request_type_operation_name_array); ++i) { + struct aws_s3_request_type_operation_name_pair *pair = &s_s3_request_type_operation_name_array[i]; + /* The array may have gaps in it */ + if (pair->name.len != 0) { + err = aws_hash_table_put(&s_s3_operation_name_to_request_type_table, &pair->name, &pair->type, NULL); + AWS_FATAL_ASSERT(!err); + } + } + s_library_initialized = true; } @@ -118,9 +189,10 @@ void aws_s3_library_clean_up(void) { } s_library_initialized = false; - s_loader = aws_s3_platform_info_loader_release(s_loader); aws_thread_join_all_managed(); + aws_hash_table_clean_up(&s_s3_operation_name_to_request_type_table); + s_loader = aws_s3_platform_info_loader_release(s_loader); aws_unregister_log_subject_info_list(&s_s3_log_subject_list); aws_unregister_error_info(&s_error_list); aws_http_library_clean_up(); diff --git a/source/s3_client.c b/source/s3_client.c index eeb1b59d0..90e6adbad 100644 --- a/source/s3_client.c +++ b/source/s3_client.c @@ -1319,6 +1319,13 @@ static struct aws_s3_meta_request *s_s3_client_meta_request_factory_default( return aws_s3_meta_request_copy_object_new(client->allocator, client, options); } case AWS_S3_META_REQUEST_TYPE_DEFAULT: + if (options->operation_name.len == 0) { + AWS_LOGF_ERROR( + AWS_LS_S3_META_REQUEST, "Could not create Default Meta Request; operation name is required"); + aws_raise_error(AWS_ERROR_INVALID_ARGUMENT); + return NULL; + } + return aws_s3_meta_request_default_new( client->allocator, client, @@ -1733,7 +1740,7 @@ static bool s_s3_client_should_update_meta_request( /* CreateSession has high priority to bypass the checks. */ if (meta_request->type == AWS_S3_META_REQUEST_TYPE_DEFAULT) { struct aws_s3_meta_request_default *meta_request_default = meta_request->impl; - if (aws_string_eq_c_str(meta_request_default->operation_name, "CreateSession")) { + if (meta_request_default->request_type == AWS_S3_REQUEST_TYPE_CREATE_SESSION) { return true; } } diff --git a/source/s3_default_meta_request.c b/source/s3_default_meta_request.c index 0e759ff61..ac1748303 100644 --- a/source/s3_default_meta_request.c +++ b/source/s3_default_meta_request.c @@ -60,6 +60,7 @@ struct aws_s3_meta_request *aws_s3_meta_request_default_new( AWS_PRECONDITION(client); AWS_PRECONDITION(options); AWS_PRECONDITION(options->message); + AWS_PRECONDITION(request_type != AWS_S3_REQUEST_TYPE_UNKNOWN || options->operation_name.len != 0); struct aws_byte_cursor request_method; if (aws_http_message_get_request_method(options->message, &request_method)) { @@ -105,25 +106,24 @@ struct aws_s3_meta_request *aws_s3_meta_request_default_new( } meta_request_default->content_length = (size_t)content_length; - meta_request_default->request_type = request_type; - - /* Try to get operation name. - * When internal aws-c-s3 code creates a default meta-request, - * a valid request_type is always passed in, and we can get its operation name. - * When external users create a default meta-request, they may have provided - * operation name in the options. */ - const char *operation_name = aws_s3_request_type_operation_name(request_type); - if (operation_name[0] != '\0') { - meta_request_default->operation_name = aws_string_new_from_c_str(allocator, operation_name); - } else if (options->operation_name.len != 0) { + + if (request_type == AWS_S3_REQUEST_TYPE_UNKNOWN) { + /* use name to get type */ meta_request_default->operation_name = aws_string_new_from_cursor(allocator, &options->operation_name); + meta_request_default->request_type = aws_s3_request_type_from_operation_name(options->operation_name); + } else { + /* use type to get name */ + meta_request_default->request_type = request_type; + meta_request_default->operation_name = + aws_string_new_from_c_str(allocator, aws_s3_request_type_operation_name(request_type)); + AWS_ASSERT(meta_request_default->operation_name->len != 0); } AWS_LOGF_DEBUG( AWS_LS_S3_META_REQUEST, "id=%p Created new Default Meta Request. operation=%s", (void *)meta_request_default, - meta_request_default->operation_name ? aws_string_c_str(meta_request_default->operation_name) : "?"); + aws_string_c_str(meta_request_default->operation_name)); return &meta_request_default->base; } @@ -170,9 +170,8 @@ static bool s_s3_meta_request_default_update( 1 /*part_number*/, AWS_S3_REQUEST_FLAG_RECORD_RESPONSE_HEADERS); - /* Default meta-request might know operation name, despite not knowing valid request_type. - * If so, pass the name along. */ - if (request->operation_name == NULL && meta_request_default->operation_name != NULL) { + /* If request_type didn't map to a name, copy over the name passed in by user */ + if (request->operation_name == NULL) { request->operation_name = aws_string_new_from_string(meta_request->allocator, meta_request_default->operation_name); } diff --git a/source/s3_meta_request.c b/source/s3_meta_request.c index c27f15efc..a8478b76d 100644 --- a/source/s3_meta_request.c +++ b/source/s3_meta_request.c @@ -2176,10 +2176,8 @@ void aws_s3_meta_request_result_setup( result->error_response_body, meta_request->allocator, &failed_request->send_data.response_body); } - if (failed_request->operation_name != NULL) { - result->error_response_operation_name = - aws_string_new_from_string(meta_request->allocator, failed_request->operation_name); - } + result->error_response_operation_name = + aws_string_new_from_string(meta_request->allocator, failed_request->operation_name); } result->response_status = response_status; diff --git a/source/s3_request.c b/source/s3_request.c index aab3d9526..d54f70065 100644 --- a/source/s3_request.c +++ b/source/s3_request.c @@ -180,10 +180,7 @@ struct aws_s3_request_metrics *aws_s3_request_metrics_new( AWS_ASSERT(metrics->req_resp_info_metrics.host_address != NULL); metrics->req_resp_info_metrics.request_type = request->request_type; - - if (request->operation_name != NULL) { - metrics->req_resp_info_metrics.operation_name = aws_string_new_from_string(allocator, request->operation_name); - } + metrics->req_resp_info_metrics.operation_name = aws_string_new_from_string(allocator, request->operation_name); metrics->time_metrics.start_timestamp_ns = -1; metrics->time_metrics.end_timestamp_ns = -1; diff --git a/source/s3_util.c b/source/s3_util.c index 04870e876..5eaa91183 100644 --- a/source/s3_util.c +++ b/source/s3_util.c @@ -73,33 +73,6 @@ const struct aws_byte_cursor g_user_agent_header_unknown = AWS_BYTE_CUR_INIT_FRO const uint32_t g_s3_max_num_upload_parts = 10000; const size_t g_s3_min_upload_part_size = MB_TO_BYTES(5); -const char *aws_s3_request_type_operation_name(enum aws_s3_request_type type) { - switch (type) { - case AWS_S3_REQUEST_TYPE_HEAD_OBJECT: - return "HeadObject"; - case AWS_S3_REQUEST_TYPE_GET_OBJECT: - return "GetObject"; - case AWS_S3_REQUEST_TYPE_LIST_PARTS: - return "ListParts"; - case AWS_S3_REQUEST_TYPE_CREATE_MULTIPART_UPLOAD: - return "CreateMultipartUpload"; - case AWS_S3_REQUEST_TYPE_UPLOAD_PART: - return "UploadPart"; - case AWS_S3_REQUEST_TYPE_ABORT_MULTIPART_UPLOAD: - return "AbortMultipartUpload"; - case AWS_S3_REQUEST_TYPE_COMPLETE_MULTIPART_UPLOAD: - return "CompleteMultipartUpload"; - case AWS_S3_REQUEST_TYPE_UPLOAD_PART_COPY: - return "UploadPartCopy"; - case AWS_S3_REQUEST_TYPE_COPY_OBJECT: - return "CopyObject"; - case AWS_S3_REQUEST_TYPE_PUT_OBJECT: - return "PutObject"; - default: - return ""; - } -} - void copy_http_headers(const struct aws_http_headers *src, struct aws_http_headers *dest) { AWS_PRECONDITION(src); AWS_PRECONDITION(dest); diff --git a/tests/s3_tester.c b/tests/s3_tester.c index 10342a3c4..b9ee69ffc 100644 --- a/tests/s3_tester.c +++ b/tests/s3_tester.c @@ -1501,6 +1501,8 @@ int aws_s3_tester_send_meta_request_with_options( (meta_request_options.type == AWS_S3_META_REQUEST_TYPE_DEFAULT && options->default_type_options.mode == AWS_S3_TESTER_DEFAULT_TYPE_MODE_GET)) { + meta_request_options.operation_name = aws_byte_cursor_from_c_str("GetObject"); + struct aws_http_message *message = aws_s3_test_get_object_request_new( allocator, aws_byte_cursor_from_string(host_name), options->get_options.object_path); ASSERT_SUCCESS(aws_s3_message_util_set_multipart_request_path( From 7910f27d8d217bd97a28e2d21695772d28a53dda Mon Sep 17 00:00:00 2001 From: Michael Graeb Date: Mon, 17 Jun 2024 13:54:41 -0700 Subject: [PATCH 02/10] add name to paginated operations --- include/aws/s3/private/s3_paginator.h | 5 +++++ source/s3_list_objects.c | 1 + source/s3_list_parts.c | 1 + source/s3_paginator.c | 7 +++++++ tests/s3_data_plane_tests.c | 1 + 5 files changed, 15 insertions(+) diff --git a/include/aws/s3/private/s3_paginator.h b/include/aws/s3/private/s3_paginator.h index d1a5a3f3f..94e2c3415 100644 --- a/include/aws/s3/private/s3_paginator.h +++ b/include/aws/s3/private/s3_paginator.h @@ -82,6 +82,11 @@ struct aws_s3_paginator_params { * Parameters for initiating paginated operation. All values are copied out or re-seated and reference counted. */ struct aws_s3_paginated_operation_params { + /** + * The S3 operation name (e.g. "ListParts"). Must not be empty. + */ + struct aws_byte_cursor operation_name; + /** * Name of the top level result node. Must not be empty. */ diff --git a/source/s3_list_objects.c b/source/s3_list_objects.c index 767900d88..7de747f6c 100644 --- a/source/s3_list_objects.c +++ b/source/s3_list_objects.c @@ -230,6 +230,7 @@ struct aws_s3_paginator *aws_s3_initiate_list_objects( aws_ref_count_init(&operation_data->ref_count, operation_data, s_ref_count_zero_callback); struct aws_s3_paginated_operation_params operation_params = { + .operation_name = aws_byte_cursor_from_c_str("ListObjectsV2"), .next_message = s_construct_next_request_http_message, .on_result_node_encountered_fn = s_on_list_bucket_result_node_encountered, .on_paginated_operation_cleanup = s_on_paginator_cleanup, diff --git a/source/s3_list_parts.c b/source/s3_list_parts.c index 2ede97e10..629a682bd 100644 --- a/source/s3_list_parts.c +++ b/source/s3_list_parts.c @@ -220,6 +220,7 @@ struct aws_s3_paginated_operation *aws_s3_list_parts_operation_new( aws_ref_count_init(&operation_data->ref_count, operation_data, s_ref_count_zero_callback); struct aws_s3_paginated_operation_params operation_params = { + .operation_name = aws_byte_cursor_from_c_str("ListParts"), .next_message = s_construct_next_request_http_message, .on_result_node_encountered_fn = s_xml_on_ListPartsResult_child, .on_paginated_operation_cleanup = s_on_paginator_cleanup, diff --git a/source/s3_paginator.c b/source/s3_paginator.c index 57e2fb731..b6d9f3986 100644 --- a/source/s3_paginator.c +++ b/source/s3_paginator.c @@ -27,6 +27,7 @@ enum operation_state { struct aws_s3_paginated_operation { struct aws_allocator *allocator; + struct aws_string *operation_name; struct aws_string *result_xml_node_name; struct aws_string *continuation_xml_node_name; @@ -73,6 +74,10 @@ static void s_operation_ref_count_zero_callback(void *arg) { operation->on_paginated_operation_cleanup(operation->user_data); } + if (operation->operation_name) { + aws_string_destroy(operation->operation_name); + } + if (operation->result_xml_node_name) { aws_string_destroy(operation->result_xml_node_name); } @@ -158,6 +163,7 @@ struct aws_s3_paginated_operation *aws_s3_paginated_operation_new( aws_mem_calloc(allocator, 1, sizeof(struct aws_s3_paginated_operation)); operation->allocator = allocator; + operation->operation_name = aws_string_new_from_cursor(allocator, ¶ms->operation_name); operation->result_xml_node_name = aws_string_new_from_cursor(allocator, ¶ms->result_xml_node_name); operation->continuation_xml_node_name = aws_string_new_from_cursor(allocator, ¶ms->continuation_token_node_name); @@ -433,6 +439,7 @@ int aws_s3_paginator_continue(struct aws_s3_paginator *paginator, const struct a .user_data = paginator, .signing_config = (struct aws_signing_config_aws *)signing_config, .type = AWS_S3_META_REQUEST_TYPE_DEFAULT, + .operation_name = aws_byte_cursor_from_string(paginator->operation->operation_name), .body_callback = s_receive_body_callback, .finish_callback = s_on_request_finished, .message = paginated_request_message, diff --git a/tests/s3_data_plane_tests.c b/tests/s3_data_plane_tests.c index 4ce6a2d12..b715a4d4d 100644 --- a/tests/s3_data_plane_tests.c +++ b/tests/s3_data_plane_tests.c @@ -4345,6 +4345,7 @@ static int s_test_s3_meta_request_default(struct aws_allocator *allocator, void /* Pass the request through as a default request so that it goes through as-is. */ options.type = AWS_S3_META_REQUEST_TYPE_DEFAULT; + options.operation_name = aws_byte_cursor_from_c_str("GetObject"); options.message = message; struct aws_s3_meta_request_test_results meta_request_test_results; From ce1db879c411c23d27aca968a3b0b300a1b0b00a Mon Sep 17 00:00:00 2001 From: Michael Graeb Date: Tue, 18 Jun 2024 14:23:28 -0700 Subject: [PATCH 03/10] test_s3_request_type_from_operation_name --- tests/CMakeLists.txt | 1 + tests/s3_util_tests.c | 40 +++++++++++++++++++++++++++++++++++++++- 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 110109ea0..02108b4a6 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -208,6 +208,7 @@ add_net_test_case(test_s3_bad_endpoint) add_net_test_case(test_s3_different_endpoints) add_test_case(test_s3_request_type_operation_name) +add_test_case(test_s3_request_type_from_operation_name) add_test_case(test_s3_replace_quote_entities) add_test_case(test_s3_strip_quotes) add_test_case(test_s3_parse_request_range_header) diff --git a/tests/s3_util_tests.c b/tests/s3_util_tests.c index 511e3b346..298b4684d 100644 --- a/tests/s3_util_tests.c +++ b/tests/s3_util_tests.c @@ -24,8 +24,8 @@ AWS_TEST_CASE(test_s3_request_type_operation_name, s_test_s3_request_type_operation_name) static int s_test_s3_request_type_operation_name(struct aws_allocator *allocator, void *ctx) { - (void)allocator; (void)ctx; + aws_s3_library_init(allocator); /* sanity check */ ASSERT_STR_EQUALS("HeadObject", aws_s3_request_type_operation_name(AWS_S3_REQUEST_TYPE_HEAD_OBJECT)); @@ -43,6 +43,44 @@ static int s_test_s3_request_type_operation_name(struct aws_allocator *allocator ASSERT_STR_EQUALS("", aws_s3_request_type_operation_name(AWS_S3_REQUEST_TYPE_MAX)); ASSERT_STR_EQUALS("", aws_s3_request_type_operation_name(-1)); + aws_s3_library_clean_up(); + return 0; +} + +AWS_TEST_CASE(test_s3_request_type_from_operation_name, s_test_s3_request_type_from_operation_name) +static int s_test_s3_request_type_from_operation_name(struct aws_allocator *allocator, void *ctx) { + (void)ctx; + aws_s3_library_init(allocator); + + /* sanity check */ + ASSERT_INT_EQUALS( + AWS_S3_REQUEST_TYPE_HEAD_OBJECT, + aws_s3_request_type_from_operation_name(aws_byte_cursor_from_c_str("HeadObject"))); + + /* check is case-insensitive (it's a good idea to be case-insensitive, right?) */ + ASSERT_INT_EQUALS( + AWS_S3_REQUEST_TYPE_HEAD_OBJECT, + aws_s3_request_type_from_operation_name(aws_byte_cursor_from_c_str("headobject"))); + + /* check that all valid enums can round-trip: enum -> name -> enum */ + for (enum aws_s3_request_type type = AWS_S3_REQUEST_TYPE_UNKNOWN + 1; type < AWS_S3_REQUEST_TYPE_MAX; ++type) { + const char *operation_name = aws_s3_request_type_operation_name(type); + ASSERT_NOT_NULL(operation_name); + ASSERT_TRUE(strlen(operation_name) > 1); + + ASSERT_INT_EQUALS(type, aws_s3_request_type_from_operation_name(aws_byte_cursor_from_c_str(operation_name))); + } + + /* check that invalid operation names give back UNKNOWN */ + ASSERT_INT_EQUALS( + AWS_S3_REQUEST_TYPE_UNKNOWN, + aws_s3_request_type_from_operation_name(aws_byte_cursor_from_c_str("MyFakeOperationName"))); + ASSERT_INT_EQUALS( + AWS_S3_REQUEST_TYPE_UNKNOWN, aws_s3_request_type_from_operation_name(aws_byte_cursor_from_c_str(""))); + ASSERT_INT_EQUALS( + AWS_S3_REQUEST_TYPE_UNKNOWN, aws_s3_request_type_from_operation_name(aws_byte_cursor_from_array(NULL, 0))); + + aws_s3_library_clean_up(); return 0; } From e9de3065ee02fc3d38e58d39452b481d43c3f4b0 Mon Sep 17 00:00:00 2001 From: Michael Graeb Date: Tue, 18 Jun 2024 15:45:25 -0700 Subject: [PATCH 04/10] pre-existing-async-error-xml --- tests/test_helper/README.md | 1 + tests/test_helper/test_helper.py | 30 ++++++++++++++++++++++++++---- 2 files changed, 27 insertions(+), 4 deletions(-) diff --git a/tests/test_helper/README.md b/tests/test_helper/README.md index 8c874a687..7124f190a 100644 --- a/tests/test_helper/README.md +++ b/tests/test_helper/README.md @@ -32,6 +32,7 @@ python3 test_helper.py clean + `pre-existing-10MB` + `pre-existing-1MB` + `pre-existing-empty` + + `pre-existing-error-xml` + with `--large_objects` enabled, several large objects will also be uploaded. Currently, only aws-c-s3's tests require these files, the aws-crt-*** repos do not: - `pre-existing-256MB` - `pre-existing-256MB-@` diff --git a/tests/test_helper/test_helper.py b/tests/test_helper/test_helper.py index b74d5a7d6..7f3e0eaac 100755 --- a/tests/test_helper/test_helper.py +++ b/tests/test_helper/test_helper.py @@ -24,7 +24,6 @@ MB = 1024*1024 GB = 1024*1024*1024 - parser = argparse.ArgumentParser() parser.add_argument( 'action', @@ -52,18 +51,35 @@ PUBLIC_BUCKET_NAME = BUCKET_NAME_BASE + "-public" +# upload an object whose body is identical to an "async error" aka "200 error" +ASYNC_ERROR_XML = ( + '\n' + '' + 'InternalError' + 'We encountered an internal error. Please try again.' + '656c76696e6727732072657175657374' + 'Uuag1LuByRx9e6j5Onimru9pO4ZVKnJ2Qz7/C1NPcfTWAtRPfTaOFg==' + '' +) + def create_bytes(size): return bytearray([1] * size) -def put_pre_existing_objects(size, keyname, bucket=BUCKET_NAME_BASE, sse=None, public_read=False, client=s3_client): - if size == 0: +def put_pre_existing_objects(size_or_body, keyname, bucket=BUCKET_NAME_BASE, + sse=None, public_read=False, content_type=None, + client=s3_client): + if size_or_body == 0: client.put_object(Bucket=bucket, Key=keyname) print(f"Object {keyname} uploaded") return - body = create_bytes(size) + if isinstance(size_or_body, int): + body = create_bytes(size_or_body) + else: + body = size_or_body + args = {'Bucket': bucket, 'Key': keyname, 'Body': body} if sse == 'aes256': args['ServerSideEncryption'] = 'AES256' @@ -77,6 +93,10 @@ def put_pre_existing_objects(size, keyname, bucket=BUCKET_NAME_BASE, sse=None, p if public_read: args['ACL'] = 'public-read' + + if content_type: + args['ContentType'] = content_type + try: client.put_object(**args) except botocore.exceptions.ClientError as e: @@ -151,6 +171,8 @@ def create_bucket_with_lifecycle(availability_zone=None, client=s3_client): 1*MB, 'pre-existing-1MB-@', bucket=bucket_name) put_pre_existing_objects( 0, 'pre-existing-empty', bucket=bucket_name) + put_pre_existing_objects( + ASYNC_ERROR_XML, 'pre-existing-async-error-xml', bucket=bucket_name, content_type='application/xml') if args.large_objects: put_pre_existing_objects( 256*MB, 'pre-existing-256MB', bucket=bucket_name) From 6f3d227d40a83117a4e184124bde868393965df0 Mon Sep 17 00:00:00 2001 From: Michael Graeb Date: Tue, 18 Jun 2024 17:24:54 -0700 Subject: [PATCH 05/10] add unit tests --- source/s3_meta_request.c | 8 ++++-- tests/CMakeLists.txt | 2 ++ tests/s3_data_plane_tests.c | 47 ++++++++++++++++++++++++++++++++ tests/s3_mock_server_tests.c | 1 + tests/s3_tester.c | 4 +-- tests/s3_tester.h | 1 + tests/test_helper/test_helper.py | 2 +- 7 files changed, 59 insertions(+), 6 deletions(-) diff --git a/source/s3_meta_request.c b/source/s3_meta_request.c index a8478b76d..265bff604 100644 --- a/source/s3_meta_request.c +++ b/source/s3_meta_request.c @@ -1496,9 +1496,11 @@ static void s_s3_meta_request_send_request_finish( /* Return whether the response to this request might contain an error, even though we got 200 OK. * see: https://repost.aws/knowledge-center/s3-resolve-200-internalerror */ static bool s_should_check_for_error_despite_200_OK(const struct aws_s3_request *request) { - /* We handle async error for every request BUT get object. */ - struct aws_s3_meta_request *meta_request = request->meta_request; - if (meta_request->type == AWS_S3_META_REQUEST_TYPE_GET_OBJECT) { + /* We handle async error for every thing EXCEPT GetObject. + * + * Note that we check the aws_s3_request_type (not the aws_s3_meta_request_type), + * in case someone is using a DEFAULT meta-request to send GetObject */ + if (request->request_type == AWS_S3_REQUEST_TYPE_GET_OBJECT) { return false; } return true; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 02108b4a6..ca0389da5 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -71,6 +71,8 @@ add_net_test_case(test_s3_get_object_multiple) add_net_test_case(test_s3_get_object_multiple_serial) add_net_test_case(test_s3_get_object_sse_kms) add_net_test_case(test_s3_get_object_sse_aes256) +add_net_test_case(test_s3_get_object_async_error_xml) +add_net_test_case(test_s3_default_get_object_async_error_xml) add_net_test_case(test_s3_get_object_backpressure_small_increments) add_net_test_case(test_s3_get_object_backpressure_big_increments) add_net_test_case(test_s3_get_object_backpressure_initial_size_zero) diff --git a/tests/s3_data_plane_tests.c b/tests/s3_data_plane_tests.c index b715a4d4d..cf1bdf49f 100644 --- a/tests/s3_data_plane_tests.c +++ b/tests/s3_data_plane_tests.c @@ -1430,6 +1430,45 @@ static int s_test_s3_get_object_sse_aes256(struct aws_allocator *allocator, void allocator, AWS_S3_TLS_ENABLED, AWS_S3_TESTER_SEND_META_REQUEST_SSE_AES256, g_pre_existing_object_aes256_10MB); } +/* Assert that GetObject can download an object whose body is XML identical to an "async error" aka "200 error": + * \nInternalError... */ +AWS_TEST_CASE(test_s3_get_object_async_error_xml, s_test_s3_get_object_async_error_xml) +static int s_test_s3_get_object_async_error_xml(struct aws_allocator *allocator, void *ctx) { + (void)ctx; + + return s_test_s3_get_object_helper( + allocator, AWS_S3_TLS_ENABLED, 0 /*extra_meta_request_flag*/, g_pre_existing_object_async_error_xml); +} + +/* Same as above, but send the "GetObject" via AWS_S3_META_REQUEST_TYPE_DEFAULT + * (instead of the typical AWS_S3_META_REQUEST_TYPE_GET_OBJECT) */ +AWS_TEST_CASE(test_s3_default_get_object_async_error_xml, s_test_s3_default_get_object_async_error_xml) +static int s_test_s3_default_get_object_async_error_xml(struct aws_allocator *allocator, void *ctx) { + (void)ctx; + + struct aws_s3_meta_request_test_results meta_request_test_results; + aws_s3_meta_request_test_results_init(&meta_request_test_results, allocator); + + struct aws_s3_tester_meta_request_options options = { + .allocator = allocator, + .meta_request_type = AWS_S3_META_REQUEST_TYPE_DEFAULT, + .validate_type = AWS_S3_TESTER_VALIDATE_TYPE_EXPECT_SUCCESS, + .default_type_options = + { + .mode = AWS_S3_TESTER_DEFAULT_TYPE_MODE_GET, + .operation_name = aws_byte_cursor_from_c_str("GetObject"), + }, + .get_options = + { + .object_path = g_pre_existing_object_async_error_xml, + }, + }; + ASSERT_SUCCESS(aws_s3_tester_send_meta_request_with_options(NULL, &options, NULL)); + + aws_s3_meta_request_test_results_clean_up(&meta_request_test_results); + return 0; +} + /** * Test read-backpressure functionality by repeatedly: * - letting the download stall @@ -3425,6 +3464,7 @@ static int s_test_s3_round_trip_default_get(struct aws_allocator *allocator, voi .default_type_options = { .mode = AWS_S3_TESTER_DEFAULT_TYPE_MODE_GET, + .operation_name = aws_byte_cursor_from_c_str("GetObject"), }, }; @@ -4130,6 +4170,7 @@ static int s_test_s3_round_trip_mpu_default_get_fc(struct aws_allocator *allocat .default_type_options = { .mode = AWS_S3_TESTER_DEFAULT_TYPE_MODE_GET, + .operation_name = aws_byte_cursor_from_c_str("GetObject"), }, .finish_callback = s_s3_test_no_validate_checksum, .headers_callback = s_s3_validate_headers_checksum_unset, @@ -4739,6 +4780,7 @@ static int s_test_s3_default_fail_headers_callback(struct aws_allocator *allocat .default_type_options = { .mode = AWS_S3_TESTER_DEFAULT_TYPE_MODE_GET, + .operation_name = aws_byte_cursor_from_c_str("GetObject"), }, .get_options = { @@ -4808,6 +4850,7 @@ static int s_test_s3_default_invoke_headers_callback_on_error(struct aws_allocat .default_type_options = { .mode = AWS_S3_TESTER_DEFAULT_TYPE_MODE_GET, + .operation_name = aws_byte_cursor_from_c_str("GetObject"), }, .get_options = { @@ -4844,6 +4887,7 @@ static int s_test_s3_default_invoke_headers_callback_cancels_on_error(struct aws .default_type_options = { .mode = AWS_S3_TESTER_DEFAULT_TYPE_MODE_GET, + .operation_name = aws_byte_cursor_from_c_str("GetObject"), }, .get_options = { @@ -4970,6 +5014,7 @@ static int s_test_s3_default_fail_body_callback(struct aws_allocator *allocator, .default_type_options = { .mode = AWS_S3_TESTER_DEFAULT_TYPE_MODE_GET, + .operation_name = aws_byte_cursor_from_c_str("GetObject"), }, .get_options = { @@ -5536,6 +5581,7 @@ static int s_test_s3_default_sending_meta_request_user_agent(struct aws_allocato .default_type_options = { .mode = AWS_S3_TESTER_DEFAULT_TYPE_MODE_GET, + .operation_name = aws_byte_cursor_from_c_str("GetObject"), }, .get_options = { @@ -5724,6 +5770,7 @@ static int s_test_s3_range_requests(struct aws_allocator *allocator, void *ctx) .default_type_options = { .mode = AWS_S3_TESTER_DEFAULT_TYPE_MODE_GET, + .operation_name = aws_byte_cursor_from_c_str("GetObject"), }, .get_options = { diff --git a/tests/s3_mock_server_tests.c b/tests/s3_mock_server_tests.c index deab3e64e..07e165b4d 100644 --- a/tests/s3_mock_server_tests.c +++ b/tests/s3_mock_server_tests.c @@ -238,6 +238,7 @@ TEST_CASE(multipart_download_checksum_with_retry_mock_server) { .default_type_options = { .mode = AWS_S3_TESTER_DEFAULT_TYPE_MODE_GET, + .operation_name = aws_byte_cursor_from_c_str("GetObject"), }, .mock_server = true, .validate_type = AWS_S3_TESTER_VALIDATE_TYPE_EXPECT_FAILURE, diff --git a/tests/s3_tester.c b/tests/s3_tester.c index b9ee69ffc..7ee08f3be 100644 --- a/tests/s3_tester.c +++ b/tests/s3_tester.c @@ -55,6 +55,8 @@ const struct aws_byte_cursor g_pre_existing_object_kms_10MB = const struct aws_byte_cursor g_pre_existing_object_aes256_10MB = AWS_BYTE_CUR_INIT_FROM_STRING_LITERAL("/pre-existing-10MB-aes256"); const struct aws_byte_cursor g_pre_existing_empty_object = AWS_BYTE_CUR_INIT_FROM_STRING_LITERAL("/pre-existing-empty"); +const struct aws_byte_cursor g_pre_existing_object_async_error_xml = + AWS_BYTE_CUR_INIT_FROM_STRING_LITERAL("/pre-existing-async-error-xml"); const struct aws_byte_cursor g_put_object_prefix = AWS_BYTE_CUR_INIT_FROM_STRING_LITERAL("/upload/put-object-test"); const struct aws_byte_cursor g_upload_folder = AWS_BYTE_CUR_INIT_FROM_STRING_LITERAL("/upload"); @@ -1501,8 +1503,6 @@ int aws_s3_tester_send_meta_request_with_options( (meta_request_options.type == AWS_S3_META_REQUEST_TYPE_DEFAULT && options->default_type_options.mode == AWS_S3_TESTER_DEFAULT_TYPE_MODE_GET)) { - meta_request_options.operation_name = aws_byte_cursor_from_c_str("GetObject"); - struct aws_http_message *message = aws_s3_test_get_object_request_new( allocator, aws_byte_cursor_from_string(host_name), options->get_options.object_path); ASSERT_SUCCESS(aws_s3_message_util_set_multipart_request_path( diff --git a/tests/s3_tester.h b/tests/s3_tester.h index bce308d79..290cf8d61 100644 --- a/tests/s3_tester.h +++ b/tests/s3_tester.h @@ -486,6 +486,7 @@ extern const struct aws_byte_cursor g_pre_existing_object_1MB; extern const struct aws_byte_cursor g_pre_existing_object_10MB; extern const struct aws_byte_cursor g_pre_existing_object_kms_10MB; extern const struct aws_byte_cursor g_pre_existing_object_aes256_10MB; +extern const struct aws_byte_cursor g_pre_existing_object_async_error_xml; extern const struct aws_byte_cursor g_pre_existing_empty_object; extern const struct aws_byte_cursor g_put_object_prefix; diff --git a/tests/test_helper/test_helper.py b/tests/test_helper/test_helper.py index 7f3e0eaac..9cf3e14b7 100755 --- a/tests/test_helper/test_helper.py +++ b/tests/test_helper/test_helper.py @@ -64,7 +64,7 @@ def create_bytes(size): - return bytearray([1] * size) + return bytes(size) def put_pre_existing_objects(size_or_body, keyname, bucket=BUCKET_NAME_BASE, From 406d68ecfb1dfd959c155a7f8b232033f2de3fad Mon Sep 17 00:00:00 2001 From: Michael Graeb Date: Wed, 19 Jun 2024 01:00:40 +0000 Subject: [PATCH 06/10] fix shared-libs --- source/s3.c | 1 + 1 file changed, 1 insertion(+) diff --git a/source/s3.c b/source/s3.c index 6286262db..cd9e8f090 100644 --- a/source/s3.c +++ b/source/s3.c @@ -4,6 +4,7 @@ */ #include +#include #include #include From 48a06d856ddcd488d4f281c1d3d1a916399b3579 Mon Sep 17 00:00:00 2001 From: Michael Graeb Date: Wed, 19 Jun 2024 09:32:53 -0700 Subject: [PATCH 07/10] trivial comment & naming edits --- include/aws/s3/s3_client.h | 5 ++++- source/s3.c | 6 +++--- tests/CMakeLists.txt | 4 ++-- tests/s3_data_plane_tests.c | 10 ++++++---- 4 files changed, 15 insertions(+), 10 deletions(-) diff --git a/include/aws/s3/s3_client.h b/include/aws/s3/s3_client.h index f2c8e0941..1be7816f2 100644 --- a/include/aws/s3/s3_client.h +++ b/include/aws/s3/s3_client.h @@ -576,8 +576,11 @@ struct aws_s3_meta_request_options { /** * The S3 operation name (e.g. "CreateBucket"). - * This must be set if type is AWS_S3_META_REQUEST_TYPE_DEFAULT; + * This MUST be set if type is AWS_S3_META_REQUEST_TYPE_DEFAULT; * it is automatically populated for other meta-request types. + * The canonical names for all S3 operations are listed here: + * https://docs.aws.amazon.com/AmazonS3/latest/API/API_Operations_Amazon_Simple_Storage_Service.html + * * This name is used to fill out details in metrics and error reports. * It's also used to check for niche behavior specific to some operations * (e.g. "CompleteMultipartUpload" may return an error, even though the status-code was 200 OK). diff --git a/source/s3.c b/source/s3.c index cd9e8f090..bf7982073 100644 --- a/source/s3.c +++ b/source/s3.c @@ -99,9 +99,9 @@ static struct aws_s3_request_type_operation_name_pair s_s3_request_type_operatio DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_CREATE_SESSION, "CreateSession"), }; -/* Hash-table for case-insensitive lookup from operation-name -> request-type. - * key is `struct aws_byte_cursor *`, pointing into array above. - * value is `enum aws_s3_request_type *`, pointing into array above. */ +/* Hash-table for case-insensitive lookup, from operation-name -> request-type. + * key is operation-name (stored as `struct aws_byte_cursor*`, pointing into array above). + * value is request-type (stored as `enum aws_s3_request_type *`, pointing into array above). */ static struct aws_hash_table s_s3_operation_name_to_request_type_table; const char *aws_s3_request_type_operation_name(enum aws_s3_request_type type) { diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index ca0389da5..5e407625b 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -71,8 +71,8 @@ add_net_test_case(test_s3_get_object_multiple) add_net_test_case(test_s3_get_object_multiple_serial) add_net_test_case(test_s3_get_object_sse_kms) add_net_test_case(test_s3_get_object_sse_aes256) -add_net_test_case(test_s3_get_object_async_error_xml) -add_net_test_case(test_s3_default_get_object_async_error_xml) +add_net_test_case(test_s3_get_object_looks_like_async_error_xml) +add_net_test_case(test_s3_default_get_object_looks_like_async_error_xml) add_net_test_case(test_s3_get_object_backpressure_small_increments) add_net_test_case(test_s3_get_object_backpressure_big_increments) add_net_test_case(test_s3_get_object_backpressure_initial_size_zero) diff --git a/tests/s3_data_plane_tests.c b/tests/s3_data_plane_tests.c index cf1bdf49f..3b5f04908 100644 --- a/tests/s3_data_plane_tests.c +++ b/tests/s3_data_plane_tests.c @@ -1432,8 +1432,8 @@ static int s_test_s3_get_object_sse_aes256(struct aws_allocator *allocator, void /* Assert that GetObject can download an object whose body is XML identical to an "async error" aka "200 error": * \nInternalError... */ -AWS_TEST_CASE(test_s3_get_object_async_error_xml, s_test_s3_get_object_async_error_xml) -static int s_test_s3_get_object_async_error_xml(struct aws_allocator *allocator, void *ctx) { +AWS_TEST_CASE(test_s3_get_object_looks_like_async_error_xml, s_test_s3_get_object_looks_like_async_error_xml) +static int s_test_s3_get_object_looks_like_async_error_xml(struct aws_allocator *allocator, void *ctx) { (void)ctx; return s_test_s3_get_object_helper( @@ -1442,8 +1442,10 @@ static int s_test_s3_get_object_async_error_xml(struct aws_allocator *allocator, /* Same as above, but send the "GetObject" via AWS_S3_META_REQUEST_TYPE_DEFAULT * (instead of the typical AWS_S3_META_REQUEST_TYPE_GET_OBJECT) */ -AWS_TEST_CASE(test_s3_default_get_object_async_error_xml, s_test_s3_default_get_object_async_error_xml) -static int s_test_s3_default_get_object_async_error_xml(struct aws_allocator *allocator, void *ctx) { +AWS_TEST_CASE( + test_s3_default_get_object_looks_like_async_error_xml, + s_test_s3_default_get_object_looks_like_async_error_xml) +static int s_test_s3_default_get_object_looks_like_async_error_xml(struct aws_allocator *allocator, void *ctx) { (void)ctx; struct aws_s3_meta_request_test_results meta_request_test_results; From 550678b3f3d46954d4a3571556f481955ef2ef7d Mon Sep 17 00:00:00 2001 From: Michael Graeb Date: Wed, 19 Jun 2024 13:32:26 -0700 Subject: [PATCH 08/10] add more detail to operation_name docs --- include/aws/s3/s3_client.h | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/include/aws/s3/s3_client.h b/include/aws/s3/s3_client.h index 1be7816f2..a23f332d7 100644 --- a/include/aws/s3/s3_client.h +++ b/include/aws/s3/s3_client.h @@ -578,12 +578,20 @@ struct aws_s3_meta_request_options { * The S3 operation name (e.g. "CreateBucket"). * This MUST be set if type is AWS_S3_META_REQUEST_TYPE_DEFAULT; * it is automatically populated for other meta-request types. - * The canonical names for all S3 operations are listed here: + * The canonical operation names are listed here: * https://docs.aws.amazon.com/AmazonS3/latest/API/API_Operations_Amazon_Simple_Storage_Service.html * * This name is used to fill out details in metrics and error reports. - * It's also used to check for niche behavior specific to some operations - * (e.g. "CompleteMultipartUpload" may return an error, even though the status-code was 200 OK). + * 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 https://repost.aws/knowledge-center/s3-resolve-200-internalerror). + * If you used AWS_S3_META_REQUEST_TYPE_DEFAULT to do GetObject, but mis-named + * it "Download", and the object looked like XML with an error code, + * then the meta-request would fail. You may log the full response body, + * and leak sensitive data. */ struct aws_byte_cursor operation_name; From 4d52adbc6083c1f07c4fd13882acadb862dd1f19 Mon Sep 17 00:00:00 2001 From: Michael Graeb Date: Wed, 19 Jun 2024 14:59:19 -0700 Subject: [PATCH 09/10] avoid allocating copies of known operation name strings --- include/aws/s3/private/s3_util.h | 11 +++ source/s3.c | 120 +++++++++++++++++++------------ source/s3_default_meta_request.c | 20 +++--- source/s3_request.c | 5 +- 4 files changed, 98 insertions(+), 58 deletions(-) diff --git a/include/aws/s3/private/s3_util.h b/include/aws/s3/private/s3_util.h index 1dbc172c3..f9c95f940 100644 --- a/include/aws/s3/private/s3_util.h +++ b/include/aws/s3/private/s3_util.h @@ -181,6 +181,17 @@ extern const uint32_t g_s3_max_num_upload_parts; AWS_S3_API enum aws_s3_request_type aws_s3_request_type_from_operation_name(struct aws_byte_cursor name); +/** + * Return operation name for aws_s3_request_type as static-lifetime aws_string* + * or NULL if the type doesn't map to an actual operation. + * For example: + * AWS_S3_REQUEST_TYPE_HEAD_OBJECT -> static aws_string "HeadObject" + * AWS_S3_REQUEST_TYPE_UNKNOWN -> NULL + * AWS_S3_REQUEST_TYPE_MAX -> NULL + */ +AWS_S3_API +struct aws_string *aws_s3_request_type_to_operation_name_static_string(enum aws_s3_request_type type); + /** * Cache and initial the signing config based on the client. * diff --git a/source/s3.c b/source/s3.c index bf7982073..41e6b2643 100644 --- a/source/s3.c +++ b/source/s3.c @@ -77,44 +77,90 @@ static struct aws_log_subject_info_list s_s3_log_subject_list = { .count = AWS_ARRAY_SIZE(s_s3_log_subject_infos), }; -struct aws_s3_request_type_operation_name_pair { +struct aws_s3_request_type_info { enum aws_s3_request_type type; - struct aws_byte_cursor name; + struct aws_string *name_string; + struct aws_byte_cursor name_cursor; }; -#define DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(TYPE, NAME) \ - [TYPE] = {.type = TYPE, .name = AWS_BYTE_CUR_INIT_FROM_STRING_LITERAL(NAME)} - -static struct aws_s3_request_type_operation_name_pair s_s3_request_type_operation_name_array[] = { - DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_HEAD_OBJECT, "HeadObject"), - DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_GET_OBJECT, "GetObject"), - DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_LIST_PARTS, "ListParts"), - DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_CREATE_MULTIPART_UPLOAD, "CreateMultipartUpload"), - DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_UPLOAD_PART, "UploadPart"), - DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_ABORT_MULTIPART_UPLOAD, "AbortMultipartUpload"), - DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_COMPLETE_MULTIPART_UPLOAD, "CompleteMultipartUpload"), - DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_UPLOAD_PART_COPY, "UploadPartCopy"), - DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_COPY_OBJECT, "CopyObject"), - DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_PUT_OBJECT, "PutObject"), - DEFINE_REQUEST_TYPE_OPERATION_NAME_PAIR(AWS_S3_REQUEST_TYPE_CREATE_SESSION, "CreateSession"), -}; +static struct aws_s3_request_type_info s_s3_request_type_info_array[AWS_S3_REQUEST_TYPE_MAX]; /* Hash-table for case-insensitive lookup, from operation-name -> request-type. * key is operation-name (stored as `struct aws_byte_cursor*`, pointing into array above). * value is request-type (stored as `enum aws_s3_request_type *`, pointing into array above). */ static struct aws_hash_table s_s3_operation_name_to_request_type_table; -const char *aws_s3_request_type_operation_name(enum aws_s3_request_type type) { +static void s_s3_request_type_register(enum aws_s3_request_type type, const struct aws_string *name) { + + AWS_PRECONDITION(type >= 0 && type < AWS_ARRAY_SIZE(s_s3_request_type_info_array)); + AWS_PRECONDITION(AWS_IS_ZEROED(s_s3_request_type_info_array[type])); + + struct aws_s3_request_type_info *info = &s_s3_request_type_info_array[type]; + info->type = type; + info->name_string = (struct aws_string *)name; + info->name_cursor = aws_byte_cursor_from_string(name); + + int err = aws_hash_table_put(&s_s3_operation_name_to_request_type_table, &info->name_cursor, &info->type, NULL); + AWS_FATAL_ASSERT(!err); +} + +AWS_STATIC_STRING_FROM_LITERAL(s_HeadObject_str, "HeadObject"); +AWS_STATIC_STRING_FROM_LITERAL(s_GetObject_str, "GetObject"); +AWS_STATIC_STRING_FROM_LITERAL(s_ListParts_str, "ListParts"); +AWS_STATIC_STRING_FROM_LITERAL(s_CreateMultipartUpload_str, "CreateMultipartUpload"); +AWS_STATIC_STRING_FROM_LITERAL(s_UploadPart_str, "UploadPart"); +AWS_STATIC_STRING_FROM_LITERAL(s_AbortMultipartUpload_str, "AbortMultipartUpload"); +AWS_STATIC_STRING_FROM_LITERAL(s_CompleteMultipartUpload_str, "CompleteMultipartUpload"); +AWS_STATIC_STRING_FROM_LITERAL(s_UploadPartCopy_str, "UploadPartCopy"); +AWS_STATIC_STRING_FROM_LITERAL(s_CopyObject_str, "CopyObject"); +AWS_STATIC_STRING_FROM_LITERAL(s_PutObject_str, "PutObject"); +AWS_STATIC_STRING_FROM_LITERAL(s_CreateSession_str, "CreateSession"); + +static void s_s3_request_type_info_init(struct aws_allocator *allocator) { + int err = aws_hash_table_init( + &s_s3_operation_name_to_request_type_table, + allocator, + AWS_ARRAY_SIZE(s_s3_request_type_info_array) /*initial_size*/, + aws_hash_byte_cursor_ptr_ignore_case, + (aws_hash_callback_eq_fn *)aws_byte_cursor_eq_ignore_case, + NULL /*destroy_key*/, + NULL /*destroy_value*/); + AWS_FATAL_ASSERT(!err); + + s_s3_request_type_register(AWS_S3_REQUEST_TYPE_HEAD_OBJECT, s_HeadObject_str); + s_s3_request_type_register(AWS_S3_REQUEST_TYPE_GET_OBJECT, s_GetObject_str); + s_s3_request_type_register(AWS_S3_REQUEST_TYPE_LIST_PARTS, s_ListParts_str); + s_s3_request_type_register(AWS_S3_REQUEST_TYPE_CREATE_MULTIPART_UPLOAD, s_CreateMultipartUpload_str); + s_s3_request_type_register(AWS_S3_REQUEST_TYPE_UPLOAD_PART, s_UploadPart_str); + s_s3_request_type_register(AWS_S3_REQUEST_TYPE_ABORT_MULTIPART_UPLOAD, s_AbortMultipartUpload_str); + s_s3_request_type_register(AWS_S3_REQUEST_TYPE_COMPLETE_MULTIPART_UPLOAD, s_CompleteMultipartUpload_str); + s_s3_request_type_register(AWS_S3_REQUEST_TYPE_UPLOAD_PART_COPY, s_UploadPartCopy_str); + s_s3_request_type_register(AWS_S3_REQUEST_TYPE_COPY_OBJECT, s_CopyObject_str); + s_s3_request_type_register(AWS_S3_REQUEST_TYPE_PUT_OBJECT, s_PutObject_str); + s_s3_request_type_register(AWS_S3_REQUEST_TYPE_CREATE_SESSION, s_CreateSession_str); +} + +static void s_s3_request_type_info_clean_up(void) { + aws_hash_table_clean_up(&s_s3_operation_name_to_request_type_table); +} + +struct aws_string *aws_s3_request_type_to_operation_name_static_string(enum aws_s3_request_type type) { + + if (type >= 0 && type < AWS_ARRAY_SIZE(s_s3_request_type_info_array)) { + struct aws_s3_request_type_info *info = &s_s3_request_type_info_array[type]; + return info->name_string; + } - if (type >= 0 && type < AWS_ARRAY_SIZE(s_s3_request_type_operation_name_array)) { - const struct aws_s3_request_type_operation_name_pair *pair = &s_s3_request_type_operation_name_array[type]; - /* The array may have gaps in it */ - if (pair->name.len > 0) { - return (const char *)pair->name.ptr; - } + return NULL; +} + +const char *aws_s3_request_type_operation_name(enum aws_s3_request_type type) { + struct aws_string *name_string = aws_s3_request_type_to_operation_name_static_string(type); + if (name_string != NULL) { + return aws_string_c_str(name_string); } - return ""; + return NULL; } enum aws_s3_request_type aws_s3_request_type_from_operation_name(struct aws_byte_cursor name) { @@ -149,25 +195,7 @@ void aws_s3_library_init(struct aws_allocator *allocator) { aws_register_log_subject_info_list(&s_s3_log_subject_list); s_loader = aws_s3_platform_info_loader_new(allocator); AWS_FATAL_ASSERT(s_loader); - - /* Initialize s_s3_operation_name_to_request_type_table */ - int err = aws_hash_table_init( - &s_s3_operation_name_to_request_type_table, - allocator, - AWS_ARRAY_SIZE(s_s3_request_type_operation_name_array) /*initial_size*/, - aws_hash_byte_cursor_ptr_ignore_case, - (aws_hash_callback_eq_fn *)aws_byte_cursor_eq_ignore_case, - NULL /*destroy_key*/, - NULL /*destroy_value*/); - AWS_FATAL_ASSERT(!err); - for (size_t i = 0; i < AWS_ARRAY_SIZE(s_s3_request_type_operation_name_array); ++i) { - struct aws_s3_request_type_operation_name_pair *pair = &s_s3_request_type_operation_name_array[i]; - /* The array may have gaps in it */ - if (pair->name.len != 0) { - err = aws_hash_table_put(&s_s3_operation_name_to_request_type_table, &pair->name, &pair->type, NULL); - AWS_FATAL_ASSERT(!err); - } - } + s_s3_request_type_info_init(allocator); s_library_initialized = true; } @@ -192,7 +220,7 @@ void aws_s3_library_clean_up(void) { s_library_initialized = false; aws_thread_join_all_managed(); - aws_hash_table_clean_up(&s_s3_operation_name_to_request_type_table); + s_s3_request_type_info_clean_up(); s_loader = aws_s3_platform_info_loader_release(s_loader); aws_unregister_log_subject_info_list(&s_s3_log_subject_list); aws_unregister_error_info(&s_error_list); diff --git a/source/s3_default_meta_request.c b/source/s3_default_meta_request.c index ac1748303..f1b43b791 100644 --- a/source/s3_default_meta_request.c +++ b/source/s3_default_meta_request.c @@ -107,16 +107,20 @@ struct aws_s3_meta_request *aws_s3_meta_request_default_new( meta_request_default->content_length = (size_t)content_length; - if (request_type == AWS_S3_REQUEST_TYPE_UNKNOWN) { - /* use name to get type */ - meta_request_default->operation_name = aws_string_new_from_cursor(allocator, &options->operation_name); + /* If request_type is unknown, look it up from operation name */ + if (request_type != AWS_S3_REQUEST_TYPE_UNKNOWN) { + meta_request_default->request_type = request_type; + } else { meta_request_default->request_type = aws_s3_request_type_from_operation_name(options->operation_name); + } + + /* If we have a static string for this operation name, use that. + * Otherwise, copy the operation_name passed in by user. */ + struct aws_string *static_operation_name = aws_s3_request_type_to_operation_name_static_string(request_type); + if (static_operation_name != NULL) { + meta_request_default->operation_name = static_operation_name; } else { - /* use type to get name */ - meta_request_default->request_type = request_type; - meta_request_default->operation_name = - aws_string_new_from_c_str(allocator, aws_s3_request_type_operation_name(request_type)); - AWS_ASSERT(meta_request_default->operation_name->len != 0); + meta_request_default->operation_name = aws_string_new_from_cursor(allocator, &options->operation_name); } AWS_LOGF_DEBUG( diff --git a/source/s3_request.c b/source/s3_request.c index d54f70065..2c352755d 100644 --- a/source/s3_request.c +++ b/source/s3_request.c @@ -33,10 +33,7 @@ struct aws_s3_request *aws_s3_request_new( request->request_tag = request_tag; request->request_type = request_type; - const char *operation_name = aws_s3_request_type_operation_name(request_type); - if (operation_name[0] != '\0') { - request->operation_name = aws_string_new_from_c_str(request->allocator, operation_name); - } + request->operation_name = aws_s3_request_type_to_operation_name_static_string(request_type); request->part_number = part_number; request->record_response_headers = (flags & AWS_S3_REQUEST_FLAG_RECORD_RESPONSE_HEADERS) != 0; From 180458ecd6368576687aba7808afda57edd2a251 Mon Sep 17 00:00:00 2001 From: Michael Graeb Date: Wed, 19 Jun 2024 15:17:12 -0700 Subject: [PATCH 10/10] thanks tests. saving my butt again and again --- source/s3.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/s3.c b/source/s3.c index 41e6b2643..7906657a2 100644 --- a/source/s3.c +++ b/source/s3.c @@ -160,7 +160,7 @@ const char *aws_s3_request_type_operation_name(enum aws_s3_request_type type) { return aws_string_c_str(name_string); } - return NULL; + return ""; } enum aws_s3_request_type aws_s3_request_type_from_operation_name(struct aws_byte_cursor name) {