#447 Azure Storage Exceptions Normalization (#1344)

This PR performs normalization of Azure storage exceptions as per #584. It throws exceptions using error code and http status code as per the [azure error docs](https://learn.microsoft.com/en-us/rest/api/storageservices/blob-service-error-codes). It also does a refactor of the `test_storage_exceptions.cpp` using functions from `common.hpp`. `PermissionException` is renamed as `StoragePermissionException` as it was conflicting with another exception.
man-group · Feb 26, 2024 · 4d20f00 · 4d20f00
1 parent aced68b
commit 4d20f00
Show file tree

Hide file tree

Showing 10 changed files with 217 additions and 78 deletions.
diff --git a/cpp/arcticdb/storage/azure/azure_client_wrapper.hpp b/cpp/arcticdb/storage/azure/azure_client_wrapper.hpp
@@ -18,6 +18,31 @@ namespace arcticdb::storage::azure {
 
 static const size_t BATCH_SUBREQUEST_LIMIT = 256; //https://github.com/Azure/azure-sdk-for-python/blob/767facc39f2487504bcde4e627db16a79f96b297/sdk/storage/azure-storage-blob/azure/storage/blob/_container_client.py#L1608
 
+// some common error codes as per https://learn.microsoft.com/en-us/rest/api/storageservices/blob-service-error-codes
+enum class AzureErrorCode {
+    BlobAlreadyExists,
+    BlobNotFound,
+    ContainerNotFound,
+    BlobOperationNotSupported,
+    UnauthorizedBlobOverwrite,
+    InvalidBlobOrBlock,
+    OtherError
+};
+
+inline std::string AzureErrorCode_to_string(AzureErrorCode error) {
+    switch (error) {
+        case AzureErrorCode::BlobAlreadyExists: return "BlobAlreadyExists";
+        case AzureErrorCode::BlobNotFound: return "BlobNotFound";
+        case AzureErrorCode::ContainerNotFound: return "ContainerNotFound";
+        case AzureErrorCode::BlobOperationNotSupported: return "BlobOperationNotSupported";
+        case AzureErrorCode::UnauthorizedBlobOverwrite: return "UnauthorizedBlobOverwrite";
+        case AzureErrorCode::InvalidBlobOrBlock: return "InvalidBlobOrBlock";
+        case AzureErrorCode::OtherError: return "Other Unspecified error";
+    }
+
+    return "Other Unspecified error";
+}
+
     // An abstract class, which is responsible for sending the requests and parsing the responses from Azure.
     // It can be derived as either a real connection to Azure or a mock used for unit tests.
 class AzureClientWrapper {

diff --git a/cpp/arcticdb/storage/azure/azure_mock_client.cpp b/cpp/arcticdb/storage/azure/azure_mock_client.cpp
@@ -30,13 +30,16 @@ std::string operation_to_string(AzureOperation operation){
 std::string MockAzureClient::get_failure_trigger(
         const std::string& blob_name,
         AzureOperation operation_to_fail,
+        const std::string& error_code,
         Azure::Core::Http::HttpStatusCode error_to_fail_with) {
-    return fmt::format("{}#Failure_{}_{}", blob_name, operation_to_string(operation_to_fail), (int)error_to_fail_with);
+    return fmt::format("{}#Failure_{}_{}_{}", blob_name, operation_to_string(operation_to_fail), error_code, (int)error_to_fail_with);
 }
 
-Azure::Core::RequestFailedException get_exception(const std::string& message, Azure::Core::Http::HttpStatusCode status_code) {
+Azure::Core::RequestFailedException get_exception(const std::string& message, const std::string& error_code, Azure::Core::Http::HttpStatusCode status_code) {
     auto rawResponse = std::make_unique<Azure::Core::Http::RawResponse>(0, 0, status_code, message);
+    rawResponse->SetHeader("x-ms-error-code", error_code);
     auto exception = Azure::Core::RequestFailedException(rawResponse);
+    exception.ErrorCode = error_code;
 
     return exception;
 }
@@ -47,11 +50,14 @@ std::optional<Azure::Core::RequestFailedException> has_failure_trigger(const std
     if (position == std::string::npos) return std::nullopt;
 
     try {
-        auto failure_code_string = blob_name.substr(position + failure_string_for_operation.size());
-        auto status_code = Azure::Core::Http::HttpStatusCode(std::stoi(failure_code_string));
-        auto error_message = fmt::format("Simulated Error, message: #{}_{}", failure_string_for_operation, (int) status_code);
-
-        return get_exception(error_message, status_code);
+        auto start = position + failure_string_for_operation.size();
+        auto error_code = blob_name.substr(start, blob_name.find_last_of('_') - start);
+        auto status_code_string = blob_name.substr(blob_name.find_last_of('_') + 1);
+        auto status_code = Azure::Core::Http::HttpStatusCode(std::stoi(status_code_string));
+        auto error_message = fmt::format("Simulated Error, message: operation {}, error code {} statuscode {}",
+                                         operation_to_string(operation), error_code, (int) status_code);
+
+        return get_exception(error_message, error_code, status_code);
     } catch (std::exception&) {
         return std::nullopt;
     }
@@ -82,9 +88,10 @@ Segment MockAzureClient::read_blob(
     }
 
     auto pos = azure_contents.find(blob_name);
-    if (pos == azure_contents.end()){
-        std::string message = fmt::format("Simulated Error, message: #{}_{}", "Read", (int) Azure::Core::Http::HttpStatusCode::NotFound);
-        throw get_exception(message, Azure::Core::Http::HttpStatusCode::NotFound);
+    if (pos == azure_contents.end()) {
+        auto error_code = AzureErrorCode_to_string(AzureErrorCode::BlobNotFound);
+        std::string message = fmt::format("Simulated Error, message: Read failed {} {}", error_code, (int) Azure::Core::Http::HttpStatusCode::NotFound);
+        throw get_exception(message, error_code, Azure::Core::Http::HttpStatusCode::NotFound);
     }
 
     return pos->second;

diff --git a/cpp/arcticdb/storage/azure/azure_mock_client.hpp b/cpp/arcticdb/storage/azure/azure_mock_client.hpp
@@ -51,6 +51,7 @@ class MockAzureClient : public AzureClientWrapper {
     static std::string get_failure_trigger(
             const std::string& blob_name,
             AzureOperation operation_to_fail,
+            const std::string& error_code,
             Azure::Core::Http::HttpStatusCode error_to_fail_with);
 
 private:

diff --git a/cpp/arcticdb/storage/azure/azure_storage.cpp b/cpp/arcticdb/storage/azure/azure_storage.cpp
@@ -47,6 +47,60 @@ namespace fg = folly::gen;
 
 namespace detail {
 
+// TODO: fix this temporary workaround to read error code. azure-sdk-cpp client sometimes doesn't properly set the error code.
+//  This issue has been raised on the sdk repo https://github.com/Azure/azure-sdk-for-cpp/issues/5369. Once fixed, we should no longer need the following function and would just read e.ErrorCode.
+std::string get_error_code(const Azure::Core::RequestFailedException& e) {
+    auto error_code = e.ErrorCode;
+
+    if(error_code.empty() && e.RawResponse ) {
+        auto headers_map = e.RawResponse->GetHeaders();
+        if(auto ec = headers_map.find("x-ms-error-code") ; ec != headers_map.end()) {
+            error_code = ec->second;
+        }
+    }
+    return error_code;
+}
+
+void raise_azure_exception(const Azure::Core::RequestFailedException& e) {
+    auto error_code = get_error_code(e);
+    auto status_code = e.StatusCode;
+    std::string error_message;
+
+    if(status_code == Azure::Core::Http::HttpStatusCode::NotFound && error_code == AzureErrorCode_to_string(AzureErrorCode::BlobNotFound)) {
+        throw KeyNotFoundException(fmt::format("Key Not Found Error: AzureError#{} {}: {}",
+                                               static_cast<int>(status_code), error_code, e.ReasonPhrase));
+    }
+
+    if(status_code == Azure::Core::Http::HttpStatusCode::Unauthorized || status_code == Azure::Core::Http::HttpStatusCode::Forbidden) {
+        raise<ErrorCode::E_PERMISSION>(fmt::format("Permission error: AzureError#{} {}: {}",
+                                                   static_cast<int>(status_code), error_code, e.ReasonPhrase));
+    }
+
+    if(static_cast<int>(status_code) >= 500) {
+        error_message = fmt::format("Unexpected Server Error: AzureError#{} {}: {} {}",
+                                    static_cast<int>(status_code), error_code, e.ReasonPhrase, e.what());
+    } else {
+        error_message = fmt::format("Unexpected Error: AzureError#{} {}: {} {}",
+                                    static_cast<int>(status_code), error_code, e.ReasonPhrase, e.what());
+    }
+
+    raise<ErrorCode::E_UNEXPECTED_AZURE_ERROR>(error_message);
+}
+
+bool is_expected_error_type(const std::string& error_code, Azure::Core::Http::HttpStatusCode status_code) {
+    return status_code == Azure::Core::Http::HttpStatusCode::NotFound && (error_code == AzureErrorCode_to_string(AzureErrorCode::BlobNotFound) ||
+                                                                          error_code == AzureErrorCode_to_string(AzureErrorCode::ContainerNotFound));
+}
+
+void raise_if_unexpected_error(const Azure::Core::RequestFailedException& e) {
+    auto error_code = get_error_code(e);
+    auto status_code = e.StatusCode;
+
+    if(!is_expected_error_type(error_code, status_code)) {
+        raise_azure_exception(e);
+    }
+}
+
 template<class KeyBucketizer>
 void do_write_impl(
     Composite<KeySegmentPair>&& kvs,
@@ -72,12 +126,8 @@ void do_write_impl(
                     try {
                         azure_client.write_blob(blob_name, std::move(seg), upload_option, request_timeout);
                     }
-                    catch (const Azure::Core::RequestFailedException& e){
-                        util::raise_rte("Failed to write azure with key '{}' {} {}: {}",
-                                            k,
-                                            blob_name,
-                                            static_cast<int>(e.StatusCode),
-                                            e.ReasonPhrase);
+                    catch (const Azure::Core::RequestFailedException& e) {
+                        raise_azure_exception(e);
                     }
 
                 }
@@ -118,12 +168,13 @@ void do_read_impl(Composite<VariantKey> && ks,
             for (auto& k : group.values()) {
                 auto key_type_dir = key_type_folder(root_folder, variant_key_type(k));
                 auto blob_name = object_path(b.bucketize(key_type_dir, k), k);
-                try{
+                try {
                     visitor(k, azure_client.read_blob(blob_name, download_option, request_timeout));
 
                     ARCTICDB_DEBUG(log::storage(), "Read key {}: {}", variant_key_type(k), variant_key_view(k));
                 }
-                catch (const Azure::Core::RequestFailedException& e){
+                catch (const Azure::Core::RequestFailedException& e) {
+                    raise_if_unexpected_error(e);
                     if (!opts.dont_warn_about_missing_key) {
                         log::storage().warn("Failed to read azure segment with key '{}' {} {}: {}",
                                                 k,
@@ -157,9 +208,7 @@ void do_remove_impl(Composite<VariantKey>&& ks,
                 azure_client.delete_blobs(to_delete, request_timeout);
             }
             catch (const Azure::Core::RequestFailedException& e) {
-                util::raise_rte("Failed to create azure segment batch remove request {}: {}",
-                                    static_cast<int>(e.StatusCode),
-                                    e.ReasonPhrase);
+                raise_azure_exception(e);
             }
             batch_counter = 0u;
         };
@@ -220,6 +269,7 @@ void do_iterate_type_impl(KeyType key_type,
             }
         }
         catch (const Azure::Core::RequestFailedException& e) {
+            raise_if_unexpected_error(e);
             log::storage().warn("Failed to iterate azure blobs '{}' {}: {}",
                                 key_type,
                                 static_cast<int>(e.StatusCode),
@@ -235,10 +285,11 @@ bool do_key_exists_impl(
     KeyBucketizer&& bucketizer) {
         auto key_type_dir = key_type_folder(root_folder, variant_key_type(key));
         auto blob_name = object_path(bucketizer.bucketize(key_type_dir, key), key);
-        try{
+        try {
             return azure_client.blob_exists(blob_name);
         }
-        catch (const Azure::Core::RequestFailedException& e){
+        catch (const Azure::Core::RequestFailedException& e) {
+            raise_if_unexpected_error(e);
             log::storage().debug("Failed to check azure key '{}' {} {}: {}",
                                 key,
                                 blob_name,

diff --git a/cpp/arcticdb/storage/s3/s3_mock_client.cpp b/cpp/arcticdb/storage/s3/s3_mock_client.cpp
@@ -48,7 +48,8 @@ std::optional<Aws::S3::S3Error> has_failure_trigger(const std::string& s3_object
     auto position = s3_object_name.rfind(failure_string_for_operation);
     if (position == std::string::npos) return std::nullopt;
     try {
-        auto failure_code_string = s3_object_name.substr(position + failure_string_for_operation.size(), s3_object_name.find_last_of('_'));
+        auto start = position + failure_string_for_operation.size();
+        auto failure_code_string = s3_object_name.substr(start, s3_object_name.find_last_of('_') - start);
         auto failure_code = Aws::S3::S3Errors(std::stoi(failure_code_string));
         bool retryable = std::stoi(s3_object_name.substr(s3_object_name.find_last_of('_') + 1));
         return Aws::S3::S3Error(Aws::Client::AWSError<Aws::S3::S3Errors>(failure_code, "Simulated error", "Simulated error message", retryable));

diff --git a/cpp/arcticdb/storage/test/common.hpp b/cpp/arcticdb/storage/test/common.hpp
@@ -30,6 +30,11 @@ inline void write_in_store(Storage &store, std::string symbol) {
     store.write(KeySegmentPair(std::move(variant_key), get_test_segment()));
 }
 
+inline void update_in_store(Storage &store, std::string symbol) {
+    auto variant_key = get_test_key(symbol);
+    store.update(KeySegmentPair(std::move(variant_key), get_test_segment()), arcticdb::storage::UpdateOpts{});
+}
+
 inline bool exists_in_store(Storage &store, std::string symbol) {
     auto variant_key = get_test_key(symbol);
     return store.key_exists(variant_key);

diff --git a/cpp/arcticdb/storage/test/test_azure_storage.cpp b/cpp/arcticdb/storage/test/test_azure_storage.cpp
@@ -48,8 +48,9 @@ TEST_F(AzureMockStorageFixture, test_read){
 TEST_F(AzureMockStorageFixture, test_write){
     write_in_store(store, "symbol");
     ASSERT_THROW(
-            write_in_store(store, MockAzureClient::get_failure_trigger("symbol", AzureOperation::WRITE, Azure::Core::Http::HttpStatusCode::Unauthorized)),
-            arcticdb::ArcticException);
+            write_in_store(store, MockAzureClient::get_failure_trigger("symbol",
+               AzureOperation::WRITE, AzureErrorCode_to_string(AzureErrorCode::UnauthorizedBlobOverwrite),
+               Azure::Core::Http::HttpStatusCode::Unauthorized)),arcticdb::ArcticException);
 }
 
 TEST_F(AzureMockStorageFixture, test_remove) {