From 927ff6d8c59b7756046fd1aeef0ae492c0131144 Mon Sep 17 00:00:00 2001 From: Thomas Newton Date: Sun, 17 Dec 2023 16:09:32 +0000 Subject: [PATCH 1/6] Implement default credential --- cpp/src/arrow/filesystem/azurefs.cc | 24 ++++++++++++++++++++++-- cpp/src/arrow/filesystem/azurefs.h | 7 +++++++ 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/cpp/src/arrow/filesystem/azurefs.cc b/cpp/src/arrow/filesystem/azurefs.cc index 21788536408..dd267aac36d 100644 --- a/cpp/src/arrow/filesystem/azurefs.cc +++ b/cpp/src/arrow/filesystem/azurefs.cc @@ -18,6 +18,7 @@ #include "arrow/filesystem/azurefs.h" #include "arrow/filesystem/azurefs_internal.h" +#include #include #include @@ -61,6 +62,8 @@ bool AzureOptions::Equals(const AzureOptions& other) const { switch (credential_kind_) { case CredentialKind::kAnonymous: return true; + case CredentialKind::kTokenCredential: + return token_credential_ == other.token_credential_; case CredentialKind::kStorageSharedKeyCredential: return storage_shared_key_credential_->AccountName == other.storage_shared_key_credential_->AccountName; @@ -69,8 +72,7 @@ bool AzureOptions::Equals(const AzureOptions& other) const { return false; } -Status AzureOptions::ConfigureAccountKeyCredential(const std::string& account_name, - const std::string& account_key) { +void AzureOptions::SetUrlsForAccountName(const std::string& account_name) { if (this->backend == AzureBackend::kAzurite) { account_blob_url_ = "http://127.0.0.1:10000/" + account_name + "/"; account_dfs_url_ = "http://127.0.0.1:10000/" + account_name + "/"; @@ -78,6 +80,18 @@ Status AzureOptions::ConfigureAccountKeyCredential(const std::string& account_na account_dfs_url_ = "https://" + account_name + ".dfs.core.windows.net/"; account_blob_url_ = "https://" + account_name + ".blob.core.windows.net/"; } +} + +Status AzureOptions::ConfigureDefaultCredential(const std::string& account_name) { + AzureOptions::SetUrlsForAccountName(account_name); + credential_kind_ = CredentialKind::kTokenCredential; + token_credential_ = std::make_shared(); + return Status::OK(); +} + +Status AzureOptions::ConfigureAccountKeyCredential(const std::string& account_name, + const std::string& account_key) { + AzureOptions::SetUrlsForAccountName(account_name); credential_kind_ = CredentialKind::kStorageSharedKeyCredential; storage_shared_key_credential_ = std::make_shared(account_name, account_key); @@ -89,6 +103,9 @@ Result> AzureOptions::MakeBlobServiceC switch (credential_kind_) { case CredentialKind::kAnonymous: break; + case CredentialKind::kTokenCredential: + return std::make_unique(account_blob_url_, + token_credential_); case CredentialKind::kStorageSharedKeyCredential: return std::make_unique(account_blob_url_, storage_shared_key_credential_); @@ -101,6 +118,9 @@ AzureOptions::MakeDataLakeServiceClient() const { switch (credential_kind_) { case CredentialKind::kAnonymous: break; + case CredentialKind::kTokenCredential: + return std::make_unique(account_dfs_url_, + token_credential_); case CredentialKind::kStorageSharedKeyCredential: return std::make_unique( account_dfs_url_, storage_shared_key_credential_); diff --git a/cpp/src/arrow/filesystem/azurefs.h b/cpp/src/arrow/filesystem/azurefs.h index 1266aa2d02b..3953747fcf8 100644 --- a/cpp/src/arrow/filesystem/azurefs.h +++ b/cpp/src/arrow/filesystem/azurefs.h @@ -70,16 +70,23 @@ struct ARROW_EXPORT AzureOptions { enum class CredentialKind { kAnonymous, + kTokenCredential, kStorageSharedKeyCredential, } credential_kind_ = CredentialKind::kAnonymous; std::shared_ptr storage_shared_key_credential_; + std::shared_ptr token_credential_; + public: AzureOptions(); ~AzureOptions(); + void SetUrlsForAccountName(const std::string& account_name); + + Status ConfigureDefaultCredential(const std::string& account_name); + Status ConfigureAccountKeyCredential(const std::string& account_name, const std::string& account_key); From 56d796f6ec86a596980e76d0b179241848486bcd Mon Sep 17 00:00:00 2001 From: Thomas Newton Date: Sun, 17 Dec 2023 16:04:01 +0000 Subject: [PATCH 2/6] Simple test --- cpp/src/arrow/filesystem/azurefs_test.cc | 23 +++++++++-------------- 1 file changed, 9 insertions(+), 14 deletions(-) diff --git a/cpp/src/arrow/filesystem/azurefs_test.cc b/cpp/src/arrow/filesystem/azurefs_test.cc index 463ff4e8daf..57ef216fb84 100644 --- a/cpp/src/arrow/filesystem/azurefs_test.cc +++ b/cpp/src/arrow/filesystem/azurefs_test.cc @@ -43,9 +43,6 @@ #include #include #include -#include -#include -#include #include #include #include @@ -266,17 +263,6 @@ class AzureHierarchicalNSEnv : public AzureEnvImpl { bool WithHierarchicalNamespace() const final { return true; } }; -// Placeholder tests -// TODO: GH-18014 Remove once a proper test is added -TEST(AzureFileSystem, InitializeCredentials) { - auto default_credential = std::make_shared(); - auto managed_identity_credential = - std::make_shared(); - auto service_principal_credential = - std::make_shared("tenant_id", "client_id", - "client_secret"); -} - TEST(AzureFileSystem, OptionsCompare) { AzureOptions options; EXPECT_TRUE(options.Equals(options)); @@ -808,6 +794,15 @@ TEST_F(TestAzureHierarchicalNSFileSystem, DeleteDirContentsFailureNonexistent) { // Tests using Azurite (the local Azure emulator) +TEST_F(TestAzuriteFileSystem, InitialiseFilesystemWithDefaultCredential) { + auto data = SetUpPreexistingData(); + AzureOptions options; + EXPECT_OK_AND_ASSIGN(auto env, GetAzureEnv()); + options.backend = env->backend(); + ARROW_EXPECT_OK(options.ConfigureDefaultCredential(env->account_name())); + EXPECT_OK_AND_ASSIGN(auto default_credential_fs, AzureFileSystem::Make(options)); +} + TEST_F(TestAzuriteFileSystem, DetectHierarchicalNamespaceFailsWithMissingContainer) { auto hierarchical_namespace = internal::HierarchicalNamespaceDetector(); ASSERT_OK(hierarchical_namespace.Init(datalake_service_client_.get())); From 4dbe9478e421a2721dcef13a7ea8bb435114c3f9 Mon Sep 17 00:00:00 2001 From: Thomas Newton Date: Sun, 17 Dec 2023 16:25:36 +0000 Subject: [PATCH 3/6] Don't configure azurite for test --- cpp/src/arrow/filesystem/azurefs_test.cc | 17 ++++++++--------- 1 file changed, 8 insertions(+), 9 deletions(-) diff --git a/cpp/src/arrow/filesystem/azurefs_test.cc b/cpp/src/arrow/filesystem/azurefs_test.cc index 57ef216fb84..e5d642ebde0 100644 --- a/cpp/src/arrow/filesystem/azurefs_test.cc +++ b/cpp/src/arrow/filesystem/azurefs_test.cc @@ -263,6 +263,14 @@ class AzureHierarchicalNSEnv : public AzureEnvImpl { bool WithHierarchicalNamespace() const final { return true; } }; +TEST(AzureFileSystem, InitialiseFilesystemWithDefaultCredential) { + AzureOptions options; + options.backend = AzureBackend::kAzurite; // Irrelevant for this test because it + // doesn't connect to the server. + ARROW_EXPECT_OK(options.ConfigureDefaultCredential("dummy-account-name")); + EXPECT_OK_AND_ASSIGN(auto default_credential_fs, AzureFileSystem::Make(options)); +} + TEST(AzureFileSystem, OptionsCompare) { AzureOptions options; EXPECT_TRUE(options.Equals(options)); @@ -794,15 +802,6 @@ TEST_F(TestAzureHierarchicalNSFileSystem, DeleteDirContentsFailureNonexistent) { // Tests using Azurite (the local Azure emulator) -TEST_F(TestAzuriteFileSystem, InitialiseFilesystemWithDefaultCredential) { - auto data = SetUpPreexistingData(); - AzureOptions options; - EXPECT_OK_AND_ASSIGN(auto env, GetAzureEnv()); - options.backend = env->backend(); - ARROW_EXPECT_OK(options.ConfigureDefaultCredential(env->account_name())); - EXPECT_OK_AND_ASSIGN(auto default_credential_fs, AzureFileSystem::Make(options)); -} - TEST_F(TestAzuriteFileSystem, DetectHierarchicalNamespaceFailsWithMissingContainer) { auto hierarchical_namespace = internal::HierarchicalNamespaceDetector(); ASSERT_OK(hierarchical_namespace.Init(datalake_service_client_.get())); From 5e36f9073d2b88d379792828aa22f2305c4f36a5 Mon Sep 17 00:00:00 2001 From: Thomas Newton Date: Mon, 18 Dec 2023 13:20:43 +0000 Subject: [PATCH 4/6] Make SetUrlsForAccountName private --- cpp/src/arrow/filesystem/azurefs.h | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/cpp/src/arrow/filesystem/azurefs.h b/cpp/src/arrow/filesystem/azurefs.h index 3953747fcf8..5a8a59cc10d 100644 --- a/cpp/src/arrow/filesystem/azurefs.h +++ b/cpp/src/arrow/filesystem/azurefs.h @@ -79,12 +79,11 @@ struct ARROW_EXPORT AzureOptions { std::shared_ptr token_credential_; + void SetUrlsForAccountName(const std::string& account_name); public: AzureOptions(); ~AzureOptions(); - void SetUrlsForAccountName(const std::string& account_name); - Status ConfigureDefaultCredential(const std::string& account_name); Status ConfigureAccountKeyCredential(const std::string& account_name, From 0d9ccab25edace069b037b9a55da46e87f133c09 Mon Sep 17 00:00:00 2001 From: Thomas Newton Date: Mon, 18 Dec 2023 13:21:58 +0000 Subject: [PATCH 5/6] American spelling Co-authored-by: Sutou Kouhei --- cpp/src/arrow/filesystem/azurefs_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cpp/src/arrow/filesystem/azurefs_test.cc b/cpp/src/arrow/filesystem/azurefs_test.cc index e5d642ebde0..799f3992a22 100644 --- a/cpp/src/arrow/filesystem/azurefs_test.cc +++ b/cpp/src/arrow/filesystem/azurefs_test.cc @@ -263,7 +263,7 @@ class AzureHierarchicalNSEnv : public AzureEnvImpl { bool WithHierarchicalNamespace() const final { return true; } }; -TEST(AzureFileSystem, InitialiseFilesystemWithDefaultCredential) { +TEST(AzureFileSystem, InitializeFilesystemWithDefaultCredential) { AzureOptions options; options.backend = AzureBackend::kAzurite; // Irrelevant for this test because it // doesn't connect to the server. From c2a86257c5801a7f1d9b6d2221ee6c3aebb6417a Mon Sep 17 00:00:00 2001 From: Thomas Newton Date: Mon, 18 Dec 2023 13:35:10 +0000 Subject: [PATCH 6/6] Auto-format --- cpp/src/arrow/filesystem/azurefs.h | 1 + 1 file changed, 1 insertion(+) diff --git a/cpp/src/arrow/filesystem/azurefs.h b/cpp/src/arrow/filesystem/azurefs.h index 5a8a59cc10d..b2c7010ff37 100644 --- a/cpp/src/arrow/filesystem/azurefs.h +++ b/cpp/src/arrow/filesystem/azurefs.h @@ -80,6 +80,7 @@ struct ARROW_EXPORT AzureOptions { std::shared_ptr token_credential_; void SetUrlsForAccountName(const std::string& account_name); + public: AzureOptions(); ~AzureOptions();