wip

awslabs · Oct 3, 2024 · 06b61be · 06b61be
1 parent 879eb7f
commit 06b61be
Show file tree

Hide file tree

Showing 7 changed files with 203 additions and 63 deletions.
diff --git a/include/aws/s3/s3_client.h b/include/aws/s3/s3_client.h
@@ -830,6 +830,17 @@ struct aws_s3_meta_request_options {
      * This is just used as an estimate, so it's okay to provide an approximate value if the exact size is unknown.
      */
     const uint64_t *object_size_hint;
+
+    /*
+     * @Deprecated
+     * THIS IS AN EXPERIMENTAL AND UNSTABLE API
+     *
+     * This option can be used to redirect the request to the correct endpoint if we receive a 301 PermanentRedirect
+     * error from S3. It will only redirect the default request, CreateMultipartUpload, or the first request of
+     * GetObject one time to the correct endpoint and region. This option has a performance penalty and thus is not recommended to
+     * enable; instead, you should specify the correct endpoint and region for your request.
+     */
+    bool enable_region_redirect;
 };
 
 /* Result details of a meta request.

diff --git a/source/s3_client.c b/source/s3_client.c
@@ -2311,57 +2311,57 @@ void aws_s3_client_notify_connection_finished(
 
             connection->http_connection = NULL;
         }
-        if (error_code == AWS_ERROR_S3_PERMANENT_REDIRECT) {
-            // TODO: waahm7 do the redirect
-            // TODO: Only do this for createMPU, discovery request or default request once.
-
-            struct aws_byte_cursor xml_doc = aws_byte_cursor_from_buf(&request->send_data.response_body);
-            struct aws_byte_cursor endpoint_cursor = {0};
-            const char *xml_path[] = {"Error", "Endpoint", NULL};
-            if (aws_xml_get_body_at_path(request->allocator, xml_doc, xml_path, &endpoint_cursor) != AWS_OP_SUCCESS) {
-                goto reset_connection;
-            }
-            struct aws_byte_cursor region_cursor;
-            aws_http_headers_get(request->send_data.response_headers, g_bucket_region_header_name, &region_cursor);
-
-            AWS_LOGF_ERROR(
-                AWS_LS_S3_CLIENT,
-                "waahm7: endpoint: " PRInSTR ", region: " PRInSTR "",
-                AWS_BYTE_CURSOR_PRI(endpoint_cursor),
-                AWS_BYTE_CURSOR_PRI(region_cursor));
-
-            // Update the region
-            // TODO: Do I need any locks or things around this?
-            // TODO: What is this and where to set the region?
-            // aws_string_destroy(meta_request->cached_signing_config->region);
-            // meta_request->cached_signing_config->region = aws_string_new_from_cursor(request->allocator,
-            // &region_cursor);
-
-            // Update the endpoint
-            // TODO: Do we need to care about S3Express?
-            // TODO: Make sure we don't hold the client lock
-            aws_s3_endpoint_release(meta_request->endpoint);
-            aws_s3_endpoint_release(connection->endpoint);
-            // TODO: Do we need to parse?
-            struct aws_uri host_uri;
-            if (aws_uri_init_parse(&host_uri, client->allocator, &endpoint_cursor)) {
-                goto reset_connection;
-            }
-            struct aws_string *endpoint_host_name =
-                aws_string_new_from_cursor(client->allocator, aws_uri_host_name(&host_uri));
-            aws_uri_clean_up(&host_uri);
-
-            aws_s3_client_lock_synced_data(client);
-
-            struct aws_s3_endpoint *endpoint = NULL;
-            // TODO: fix https and port
-            if (s_create_s3_endpoint_synced(endpoint_host_name, client, true, 0, &endpoint)) {
-                // TODO: error handling
-            }
-            meta_request->endpoint = endpoint;
-            connection->endpoint = aws_s3_endpoint_acquire(endpoint, true);
-            aws_s3_client_unlock_synced_data(client);
-        }
+        // if (error_code == AWS_ERROR_S3_PERMANENT_REDIRECT) {
+        //     // TODO: waahm7 do the redirect
+        //     // TODO: Only do this for createMPU, discovery request or default request once.
+
+        //     struct aws_byte_cursor xml_doc = aws_byte_cursor_from_buf(&request->send_data.response_body);
+        //     struct aws_byte_cursor endpoint_cursor = {0};
+        //     const char *xml_path[] = {"Error", "Endpoint", NULL};
+        //     if (aws_xml_get_body_at_path(request->allocator, xml_doc, xml_path, &endpoint_cursor) != AWS_OP_SUCCESS) {
+        //         goto reset_connection;
+        //     }
+        //     struct aws_byte_cursor region_cursor;
+        //     aws_http_headers_get(request->send_data.response_headers, g_bucket_region_header_name, &region_cursor);
+
+        //     AWS_LOGF_ERROR(
+        //         AWS_LS_S3_CLIENT,
+        //         "waahm7: endpoint: " PRInSTR ", region: " PRInSTR "",
+        //         AWS_BYTE_CURSOR_PRI(endpoint_cursor),
+        //         AWS_BYTE_CURSOR_PRI(region_cursor));
+
+        //     // Update the region
+        //     // TODO: Do I need any locks or things around this?
+        //     // TODO: What is this and where to set the region?
+        //     // aws_string_destroy(meta_request->cached_signing_config->region);
+        //     // meta_request->cached_signing_config->region = aws_string_new_from_cursor(request->allocator,
+        //     // &region_cursor);
+
+        //     // Update the endpoint
+        //     // TODO: Do we need to care about S3Express?
+        //     // TODO: Make sure we don't hold the client lock
+        //     aws_s3_endpoint_release(meta_request->endpoint);
+        //     aws_s3_endpoint_release(connection->endpoint);
+        //     // TODO: Do we need to parse?
+        //     struct aws_uri host_uri;
+        //     if (aws_uri_init_parse(&host_uri, client->allocator, &endpoint_cursor)) {
+        //         goto reset_connection;
+        //     }
+        //     struct aws_string *endpoint_host_name =
+        //         aws_string_new_from_cursor(client->allocator, aws_uri_host_name(&host_uri));
+        //     aws_uri_clean_up(&host_uri);
+
+        //     aws_s3_client_lock_synced_data(client);
+
+        //     struct aws_s3_endpoint *endpoint = NULL;
+        //     // TODO: fix https and port
+        //     if (s_create_s3_endpoint_synced(endpoint_host_name, client, true, 0, &endpoint)) {
+        //         // TODO: error handling
+        //     }
+        //     meta_request->endpoint = endpoint;
+        //     connection->endpoint = aws_s3_endpoint_acquire(endpoint, true);
+        //     aws_s3_client_unlock_synced_data(client);
+        // }
 
         /* Ask the retry strategy to schedule a retry of the request. */
         if (aws_retry_strategy_schedule_retry(

diff --git a/source/s3_meta_request.c b/source/s3_meta_request.c
@@ -1602,7 +1602,7 @@ void aws_s3_meta_request_send_request_finish_default(
         if (error_code == AWS_ERROR_S3_INVALID_RESPONSE_STATUS ||
             error_code == AWS_ERROR_S3_INTERNAL_PART_SIZE_MISMATCH_RETRYING_WITH_RANGE ||
             error_code == AWS_ERROR_S3_NON_RECOVERABLE_ASYNC_ERROR ||
-            // error_code == AWS_ERROR_S3_PERMANENT_REDIRECT ||
+            error_code == AWS_ERROR_S3_PERMANENT_REDIRECT ||
             error_code == AWS_ERROR_S3_RESPONSE_CHECKSUM_MISMATCH || meta_request_finishing) {
             finish_code = AWS_S3_CONNECTION_FINISH_CODE_FAILED;
             if (error_code == AWS_ERROR_S3_INTERNAL_PART_SIZE_MISMATCH_RETRYING_WITH_RANGE) {

diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt
@@ -67,6 +67,8 @@ add_net_test_case(test_s3_get_object_tls_disabled)
 add_net_test_case(test_s3_get_object_tls_enabled)
 add_net_test_case(test_s3_get_object_tls_default)
 add_net_test_case(test_s3_get_object_region_redirect)
+add_net_test_case(test_s3_put_object_region_redirect)
+add_net_test_case(test_s3_meta_request_default_delete_object_region_redirect)
 add_net_test_case(test_s3_get_object_less_than_part_size)
 add_net_test_case(test_s3_get_object_file_path)
 add_net_test_case(test_s3_get_object_file_path_create_new)

diff --git a/tests/s3_data_plane_tests.c b/tests/s3_data_plane_tests.c
@@ -1036,9 +1036,8 @@ static int s_test_s3_get_object_helper(
     struct aws_tls_ctx *context = aws_tls_client_ctx_new(allocator, &tls_context_options);
     aws_tls_connection_options_init_from_ctx(&tls_connection_options, context);
 #endif
-
-    struct aws_string *endpoint =
-        aws_s3_tester_build_endpoint_string(allocator, &g_test_bucket_name, &g_test_s3_region);
+    struct aws_byte_cursor region = aws_byte_cursor_from_c_str("us-east-1");
+    struct aws_string *endpoint = aws_s3_tester_build_endpoint_string(allocator, &g_test_bucket_name, &region);
     struct aws_byte_cursor endpoint_cursor = aws_byte_cursor_from_string(endpoint);
 
     tls_connection_options.server_name = aws_string_new_from_cursor(allocator, &endpoint_cursor);
@@ -1109,7 +1108,6 @@ AWS_TEST_CASE(test_s3_get_object_region_redirect, s_test_s3_get_object_region_re
 static int s_test_s3_get_object_region_redirect(struct aws_allocator *allocator, void *ctx) {
     (void)ctx;
 
-    struct aws_byte_cursor region = aws_byte_cursor_from_c_str("us-west-2");
     ASSERT_SUCCESS(s_test_s3_get_object_helper(allocator, AWS_S3_TLS_DEFAULT, 0, g_pre_existing_object_1MB));
 
     return 0;
@@ -2013,9 +2011,9 @@ static int s_test_s3_put_object_helper(
     struct aws_tls_ctx *context = aws_tls_client_ctx_new(allocator, &tls_context_options);
     aws_tls_connection_options_init_from_ctx(&tls_connection_options, context);
 #endif
+    struct aws_byte_cursor region = aws_byte_cursor_from_c_str("us-east-1");
 
-    struct aws_string *endpoint =
-        aws_s3_tester_build_endpoint_string(allocator, &g_test_bucket_name, &g_test_s3_region);
+    struct aws_string *endpoint = aws_s3_tester_build_endpoint_string(allocator, &g_test_bucket_name, &region);
     struct aws_byte_cursor endpoint_cursor = aws_byte_cursor_from_string(endpoint);
 
     tls_connection_options.server_name = aws_string_new_from_cursor(allocator, &endpoint_cursor);
@@ -2042,7 +2040,7 @@ static int s_test_s3_put_object_helper(
     struct aws_s3_client *client = aws_s3_client_new(allocator, &client_config);
 
     ASSERT_SUCCESS(aws_s3_tester_send_put_object_meta_request(
-        &tester, client, 10, AWS_S3_TESTER_SEND_META_REQUEST_EXPECT_SUCCESS | extra_meta_request_flag, NULL));
+        &tester, client, 4, AWS_S3_TESTER_SEND_META_REQUEST_EXPECT_SUCCESS | extra_meta_request_flag, NULL));
 
     aws_string_destroy(endpoint);
 
@@ -2086,6 +2084,15 @@ static int s_test_s3_put_object_tls_default(struct aws_allocator *allocator, voi
     return 0;
 }
 
+AWS_TEST_CASE(test_s3_put_object_region_redirect, s_test_s3_put_object_region_redirect)
+static int s_test_s3_put_object_region_redirect(struct aws_allocator *allocator, void *ctx) {
+    (void)ctx;
+
+    ASSERT_SUCCESS(s_test_s3_put_object_helper(allocator, AWS_S3_TLS_DEFAULT, 0));
+
+    return 0;
+}
+
 AWS_TEST_CASE(test_s3_multipart_put_object_with_acl, s_test_s3_multipart_put_object_with_acl)
 static int s_test_s3_multipart_put_object_with_acl(struct aws_allocator *allocator, void *ctx) {
     (void)ctx;
@@ -4694,6 +4701,83 @@ static int s_test_s3_meta_request_default(struct aws_allocator *allocator, void
     return 0;
 }
 
+AWS_TEST_CASE(
+    test_s3_meta_request_default_delete_object_region_redirect,
+    s_test_s3_meta_request_default_delete_object_region_redirect)
+static int s_test_s3_meta_request_default_delete_object_region_redirect(struct aws_allocator *allocator, void *ctx) {
+    (void)ctx;
+
+    struct aws_s3_tester tester;
+    AWS_ZERO_STRUCT(tester);
+    ASSERT_SUCCESS(aws_s3_tester_init(allocator, &tester));
+
+    struct aws_s3_client_config client_config;
+    AWS_ZERO_STRUCT(client_config);
+
+    ASSERT_SUCCESS(aws_s3_tester_bind_client(
+        &tester, &client_config, AWS_S3_TESTER_BIND_CLIENT_REGION | AWS_S3_TESTER_BIND_CLIENT_SIGNING));
+
+    struct aws_s3_client *client = aws_s3_client_new(allocator, &client_config);
+    struct aws_byte_cursor region = aws_byte_cursor_from_c_str("us-east-1");
+
+    struct aws_string *host_name = aws_s3_tester_build_endpoint_string(allocator, &g_test_bucket_name, &region);
+
+    /* Put together a simple S3 Get Object request. */
+    struct aws_http_message *message = aws_s3_test_delete_object_request_new(
+        allocator, aws_byte_cursor_from_string(host_name), g_pre_existing_object_1MB);
+
+    struct aws_s3_meta_request_options options;
+    AWS_ZERO_STRUCT(options);
+
+    /* 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("DeleteObject");
+    options.message = message;
+
+    struct aws_s3_meta_request_test_results meta_request_test_results;
+    aws_s3_meta_request_test_results_init(&meta_request_test_results, allocator);
+
+    ASSERT_SUCCESS(aws_s3_tester_bind_meta_request(&tester, &options, &meta_request_test_results));
+
+    struct aws_s3_meta_request *meta_request = aws_s3_client_make_meta_request(client, &options);
+
+    ASSERT_TRUE(meta_request != NULL);
+
+    /* Wait for the request to finish. */
+    aws_s3_tester_wait_for_meta_request_finish(&tester);
+
+    aws_s3_tester_lock_synced_data(&tester);
+
+    ASSERT_TRUE(tester.synced_data.finish_error_code == AWS_ERROR_SUCCESS);
+
+    aws_s3_tester_unlock_synced_data(&tester);
+
+    /* Check the size of the metrics should be the same as the number of
+    requests, which should be 1 */
+    ASSERT_UINT_EQUALS(1, aws_array_list_length(&meta_request_test_results.synced_data.metrics));
+    struct aws_s3_request_metrics *metrics = NULL;
+    aws_array_list_back(&meta_request_test_results.synced_data.metrics, (void **)&metrics);
+
+    // ASSERT_SUCCESS(aws_s3_tester_validate_get_object_results(&meta_request_test_results, 0));
+
+    meta_request = aws_s3_meta_request_release(meta_request);
+
+    aws_s3_tester_wait_for_meta_request_shutdown(&tester);
+    aws_s3_meta_request_test_results_clean_up(&meta_request_test_results);
+
+    aws_http_message_release(message);
+    message = NULL;
+
+    aws_string_destroy(host_name);
+    host_name = NULL;
+
+    client = aws_s3_client_release(client);
+
+    aws_s3_tester_clean_up(&tester);
+
+    return 0;
+}
+
 AWS_TEST_CASE(test_s3_error_missing_file, s_test_s3_error_missing_file)
 static int s_test_s3_error_missing_file(struct aws_allocator *allocator, void *ctx) {
     (void)ctx;

diff --git a/tests/s3_tester.c b/tests/s3_tester.c
@@ -1083,6 +1083,7 @@ void aws_s3_tester_wait_for_client_shutdown(struct aws_s3_tester *tester) {
     tester->synced_data.client_shutdown = false;
     aws_s3_tester_unlock_synced_data(tester);
 }
+
 struct aws_http_message *aws_s3_test_get_object_request_new(
     struct aws_allocator *allocator,
     struct aws_byte_cursor host,
@@ -1120,6 +1121,43 @@ struct aws_http_message *aws_s3_test_get_object_request_new(
     return NULL;
 }
 
+struct aws_http_message *aws_s3_test_delete_object_request_new(
+    struct aws_allocator *allocator,
+    struct aws_byte_cursor host,
+    struct aws_byte_cursor key) {
+
+    struct aws_http_message *message = aws_http_message_new_request(allocator);
+
+    if (message == NULL) {
+        return NULL;
+    }
+
+    struct aws_http_header host_header = {.name = g_host_header_name, .value = host};
+
+    if (aws_http_message_add_header(message, host_header)) {
+        goto error_clean_up_message;
+    }
+
+    if (aws_http_message_set_request_method(message, aws_http_method_delete)) {
+        goto error_clean_up_message;
+    }
+
+    if (aws_http_message_set_request_path(message, key)) {
+        goto error_clean_up_message;
+    }
+
+    return message;
+
+error_clean_up_message:
+
+    if (message != NULL) {
+        aws_http_message_release(message);
+        message = NULL;
+    }
+
+    return NULL;
+}
+
 struct aws_s3_client_vtable *aws_s3_tester_patch_client_vtable(
     struct aws_s3_tester *tester,
     struct aws_s3_client *client,
@@ -1890,8 +1928,8 @@ int aws_s3_tester_send_get_object_meta_request(
     uint32_t flags,
     struct aws_s3_meta_request_test_results *out_results) {
 
-    struct aws_string *host_name =
-        aws_s3_tester_build_endpoint_string(tester->allocator, &g_test_bucket_name, &g_test_s3_region);
+    struct aws_byte_cursor region = aws_byte_cursor_from_c_str("us-east-1");
+    struct aws_string *host_name = aws_s3_tester_build_endpoint_string(tester->allocator, &g_test_bucket_name, &region);
 
     /* Put together a simple S3 Get Object request. */
     struct aws_http_message *message =
@@ -2000,9 +2038,9 @@ int aws_s3_tester_send_put_object_meta_request(
 
     struct aws_byte_cursor test_body_cursor = aws_byte_cursor_from_buf(&test_buffer);
     struct aws_input_stream *input_stream = aws_input_stream_new_from_cursor(allocator, &test_body_cursor);
+    struct aws_byte_cursor region = aws_byte_cursor_from_c_str("us-east-1");
 
-    struct aws_string *host_name =
-        aws_s3_tester_build_endpoint_string(allocator, &g_test_bucket_name, &g_test_s3_region);
+    struct aws_string *host_name = aws_s3_tester_build_endpoint_string(allocator, &g_test_bucket_name, &region);
 
     char object_path_buffer[128] = "";
 

diff --git a/tests/s3_tester.h b/tests/s3_tester.h
@@ -355,6 +355,11 @@ struct aws_http_message *aws_s3_test_get_object_request_new(
     struct aws_byte_cursor host,
     struct aws_byte_cursor key);
 
+struct aws_http_message *aws_s3_test_delete_object_request_new(
+    struct aws_allocator *allocator,
+    struct aws_byte_cursor host,
+    struct aws_byte_cursor key);
+
 struct aws_http_message *aws_s3_test_put_object_request_new(
     struct aws_allocator *allocator,
     struct aws_byte_cursor *host,