GH-39262: [C++][Azure][FS] Add default credential auth configuration#39263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
kou merged 6 commits into from
Dec 19, 2023
Merged

GH-39262: [C++][Azure][FS] Add default credential auth configuration #39263

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
927ff6d
Implement default credential
Tom-Newton Dec 17, 2023
56d796f
Simple test
Tom-Newton Dec 17, 2023
4dbe947
Don't configure azurite for test
Tom-Newton Dec 17, 2023
5e36f90
Make SetUrlsForAccountName private
Tom-Newton Dec 18, 2023
0d9ccab
American spelling
Tom-Newton Dec 18, 2023
c2a8625
Auto-format
Tom-Newton Dec 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 22 additions & 2 deletions cpp/src/arrow/filesystem/azurefs.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -18,6 +18,7 @@
#include "arrow/filesystem/azurefs.h"
#include "arrow/filesystem/azurefs_internal.h"

#include <azure/identity.hpp>
#include <azure/storage/blobs.hpp>
#include <azure/storage/files/datalake.hpp>

Expand DownExpand Up@@ -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;
Expand All@@ -69,15 +72,26 @@ 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 + "/";
} else {
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<Azure::Identity::DefaultAzureCredential>();
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<Storage::StorageSharedKeyCredential>(account_name, account_key);
Expand All@@ -89,6 +103,9 @@ Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceC
switch (credential_kind_) {
case CredentialKind::kAnonymous:
break;
case CredentialKind::kTokenCredential:
return std::make_unique<Blobs::BlobServiceClient>(account_blob_url_,
token_credential_);
case CredentialKind::kStorageSharedKeyCredential:
return std::make_unique<Blobs::BlobServiceClient>(account_blob_url_,
storage_shared_key_credential_);
Expand All@@ -101,6 +118,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
switch (credential_kind_) {
case CredentialKind::kAnonymous:
break;
case CredentialKind::kTokenCredential:
return std::make_unique<DataLake::DataLakeServiceClient>(account_dfs_url_,
token_credential_);
case CredentialKind::kStorageSharedKeyCredential:
return std::make_unique<DataLake::DataLakeServiceClient>(
account_dfs_url_, storage_shared_key_credential_);
Expand Down
7 changes: 7 additions & 0 deletions cpp/src/arrow/filesystem/azurefs.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -70,16 +70,23 @@ struct ARROW_EXPORT AzureOptions {

enum class CredentialKind {
kAnonymous,
kTokenCredential,
kStorageSharedKeyCredential,
} credential_kind_ = CredentialKind::kAnonymous;

std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
storage_shared_key_credential_;

std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;

void SetUrlsForAccountName(const std::string& account_name);

public:
AzureOptions();
~AzureOptions();

Status ConfigureDefaultCredential(const std::string& account_name);

Status ConfigureAccountKeyCredential(const std::string& account_name,
const std::string& account_key);

Expand Down
18 changes: 6 additions & 12 deletions cpp/src/arrow/filesystem/azurefs_test.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -43,9 +43,6 @@
#include <gmock/gmock-matchers.h>
#include <gmock/gmock-more-matchers.h>
#include <gtest/gtest.h>
#include <azure/identity/client_secret_credential.hpp>
#include <azure/identity/default_azure_credential.hpp>
#include <azure/identity/managed_identity_credential.hpp>
#include <azure/storage/blobs.hpp>
#include <azure/storage/common/storage_credential.hpp>
#include <azure/storage/files/datalake.hpp>
Expand DownExpand Up@@ -266,15 +263,12 @@ class AzureHierarchicalNSEnv : public AzureEnvImpl<AzureHierarchicalNSEnv> {
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<Azure::Identity::DefaultAzureCredential>();
auto managed_identity_credential =
std::make_shared<Azure::Identity::ManagedIdentityCredential>();
auto service_principal_credential =
std::make_shared<Azure::Identity::ClientSecretCredential>("tenant_id", "client_id",
"client_secret");
TEST(AzureFileSystem, InitializeFilesystemWithDefaultCredential) {
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));

@koukouDec 17, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that we can use Azurite with the DefaultAzureCredential: https://learn.microsoft.com/en-us/azure/storage/common/storage-use-azurite?tabs=visual-studio%2Cblob-storage#azure-sdks

Can we do an operation (CreateDir()?) and check the result to verify whether this filesystem is valid or not?

@Tom-NewtonTom-NewtonDec 17, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a bit of a try but I couldn't get the SSL setup working. I'll give it another go but I don't have great hope for making it work in CI even if I get it working locally.

@koukouDec 18, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I'll also try it.

@koukouDec 18, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried and understand why you said SSL.
Azure::Identity::DefaultAzureCredential uses a Bearer token and Azure SDK for C++ rejects it with http: Bearer token authentication is not permitted for non TLS protected (https) endpoints.
If we want to use DefaultAzureCredential with Azurite, we need to generate a key and certificate pair and use it.

I looked at how to set it to Azure SDK for C++. It seems that we need to BlobClientOptions::Transport::Transport:

If we set BlobClientOptions::Transport::Transport, we need to specify curl based HTTP transport implementation or WinHTTP based HTTP transport implementation. They have different configurations for TLS...

How about using TestAzureHierarchicalNSFileSystem to test DefaultAzureCredential? If we use the real Azure service, we don't need to custom TLS configuration.

@Tom-NewtonTom-NewtonDec 18, 2023•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would definitely be possible to test against real blob storage but there will be a significant amount of manual configuration for all the identities to test all the different authentications.

Then we need to provide details of these identities to TestAzureHierarchicalNSFileSystem either they are required always or we need to add new versions for each auth e.g. TestAzureHierarchicalNSFileSystemWithServicePrincipal.

Also there are some Auth methods that are not feasible to test. For example managed identity can only work on Azure VMs and workload identity can only work in kubernetes.

Personally I don't think this is worthwhile to make a more comprehensive test because of how little complexity there is outside the Azure SDK.

@felipecrvfelipecrvDec 19, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test is enough, because as @pitrou said the other day: "we are not re-implementing the Azure SDK".

@koukouDec 19, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there are some Auth methods that are not feasible to test. For example managed identity can only work on Azure VMs and workload identity can only work in kubernetes.

Oh...

OK. I see.

}

TEST(AzureFileSystem, OptionsCompare) {
Expand Down
, 'i'); if (__m === '*' || __re.test(location.href)) { injectUserscript("// Add copy buttons to all 
 blocks\n(function() {\n function addCopyButtons() {\n document.querySelectorAll('pre code').forEach(function(codeBlock) {\n if (codeBlock.parentElement.hasAttribute('data-copy-added')) return;\n codeBlock.parentElement.setAttribute('data-copy-added', 'true');\n \n var btn = document.createElement('button');\n btn.textContent = 'Copy';\n btn.style.cssText = 'position:absolute;top:4px;right:4px;padding:2px 8px;font-size:11px;background:#4ecdc4;border:none;border-radius:4px;color:#1a1a2e;cursor:pointer;opacity:0.7;transition:opacity 0.2s;';\n btn.onmouseover = function() { this.style.opacity = '1'; };\n btn.onmouseout = function() { this.style.opacity = '0.7'; };\n btn.onclick = function() {\n navigator.clipboard.writeText(codeBlock.textContent).then(function() {\n btn.textContent = 'Copied!';\n setTimeout(function() { btn.textContent = 'Copy'; }, 1500);\n });\n };\n codeBlock.parentElement.style.position = 'relative';\n codeBlock.parentElement.appendChild(btn);\n });\n }\n \n addCopyButtons();\n \n // Re-run on dynamic content\n var observer = new MutationObserver(addCopyButtons);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Add Copy Buttons to Code Blocks");
}
} catch(__e) { console.warn('[Userscript:Add Copy Buttons to Code Blocks]', __e); }
})();
(function(){
try {
var __m = "github.com";
var __re = new RegExp('^' + "github\\.com" + '

Skip to content
GH-39262: [C++][Azure][FS] Add default credential auth configuration#39263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

kou
merged 6 commits into

from

Dec 19, 2023
 Merged


GH-39262: [C++][Azure][FS] Add default credential auth configuration
#39263
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


6 commits


Select commit
Hold shift + click to select a range
927ff6d

Implement default credential


Tom-Newton Dec 17, 2023
56d796f

Simple test


Tom-Newton Dec 17, 2023
4dbe947

Don't configure azurite for test


Tom-Newton Dec 17, 2023
5e36f90

Make SetUrlsForAccountName private


Tom-Newton Dec 18, 2023
0d9ccab

American spelling


Tom-Newton Dec 18, 2023
c2a8625

Auto-format


Tom-Newton Dec 18, 2023

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


24 changes: 22 additions & 2 deletions
cpp/src/arrow/filesystem/azurefs.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -18,6 +18,7 @@
#include "arrow/filesystem/azurefs.h"
#include "arrow/filesystem/azurefs_internal.h"

#include <azure/identity.hpp>
#include <azure/storage/blobs.hpp>
#include <azure/storage/files/datalake.hpp>

Expand DownExpand Up@@ -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;
Expand All@@ -69,15 +72,26 @@ 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 + "/";
 } else {
 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<Azure::Identity::DefaultAzureCredential>();
 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<Storage::StorageSharedKeyCredential>(account_name, account_key);
Expand All@@ -89,6 +103,9 @@ Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceC
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<Blobs::BlobServiceClient>(account_blob_url_,
 token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<Blobs::BlobServiceClient>(account_blob_url_,
 storage_shared_key_credential_);
Expand All@@ -101,6 +118,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(account_dfs_url_,
 token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(
 account_dfs_url_, storage_shared_key_credential_);
Expand Down

7 changes: 7 additions & 0 deletions
cpp/src/arrow/filesystem/azurefs.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -70,16 +70,23 @@ struct ARROW_EXPORT AzureOptions {

 enum class CredentialKind {
 kAnonymous,
 kTokenCredential,
 kStorageSharedKeyCredential,
 } credential_kind_ = CredentialKind::kAnonymous;

 std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
 storage_shared_key_credential_;

 std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;

 void SetUrlsForAccountName(const std::string& account_name);

 public:
 AzureOptions();
 ~AzureOptions();

 Status ConfigureDefaultCredential(const std::string& account_name);

 Status ConfigureAccountKeyCredential(const std::string& account_name,
 const std::string& account_key);

Expand Down

18 changes: 6 additions & 12 deletions
cpp/src/arrow/filesystem/azurefs_test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -43,9 +43,6 @@
#include <gmock/gmock-matchers.h>
#include <gmock/gmock-more-matchers.h>
#include <gtest/gtest.h>
#include <azure/identity/client_secret_credential.hpp>
#include <azure/identity/default_azure_credential.hpp>
#include <azure/identity/managed_identity_credential.hpp>
#include <azure/storage/blobs.hpp>
#include <azure/storage/common/storage_credential.hpp>
#include <azure/storage/files/datalake.hpp>
Expand DownExpand Up@@ -266,15 +263,12 @@ class AzureHierarchicalNSEnv : public AzureEnvImpl<AzureHierarchicalNSEnv> {
 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<Azure::Identity::DefaultAzureCredential>();
 auto managed_identity_credential =
 std::make_shared<Azure::Identity::ManagedIdentityCredential>();
 auto service_principal_credential =
 std::make_shared<Azure::Identity::ClientSecretCredential>("tenant_id", "client_id",
 "client_secret");
TEST(AzureFileSystem, InitializeFilesystemWithDefaultCredential) {
 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));
@koukouDec 17, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that we can use Azurite with the DefaultAzureCredential: https://learn.microsoft.com/en-us/azure/storage/common/storage-use-azurite?tabs=visual-studio%2Cblob-storage#azure-sdks
Can we do an operation (CreateDir()?) and check the result to verify whether this filesystem is valid or not?
@Tom-NewtonTom-NewtonDec 17, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a bit of a try but I couldn't get the SSL setup working. I'll give it another go but I don't have great hope for making it work in CI even if I get it working locally.
@koukouDec 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I'll also try it.
@koukouDec 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried and understand why you said SSL.
Azure::Identity::DefaultAzureCredential uses a Bearer token and Azure SDK for C++ rejects it with http: Bearer token authentication is not permitted for non TLS protected (https) endpoints.

If we want to use DefaultAzureCredential with Azurite, we need to generate a key and certificate pair and use it.
I looked at how to set it to Azure SDK for C++. It seems that we need to BlobClientOptions::Transport::Transport:
If we set BlobClientOptions::Transport::Transport, we need to specify curl based HTTP transport implementation or WinHTTP based HTTP transport implementation. They have different configurations for TLS...
How about using TestAzureHierarchicalNSFileSystem to test DefaultAzureCredential? If we use the real Azure service, we don't need to custom TLS configuration.
@Tom-NewtonTom-NewtonDec 18, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would definitely be possible to test against real blob storage but there will be a significant amount of manual configuration for all the identities to test all the different authentications.
Then we need to provide details of these identities to TestAzureHierarchicalNSFileSystem either they are required always or we need to add new versions for each auth e.g. TestAzureHierarchicalNSFileSystemWithServicePrincipal.
Also there are some Auth methods that are not feasible to test. For example managed identity can only work on Azure VMs and workload identity can only work in kubernetes.
Personally I don't think this is worthwhile to make a more comprehensive test because of how little complexity there is outside the Azure SDK.
@felipecrvfelipecrvDec 19, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test is enough, because as @pitrou said the other day: "we are not re-implementing the Azure SDK".
@koukouDec 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there are some Auth methods that are not feasible to test. For example managed identity can only work on Azure VMs and workload identity can only work in kubernetes.
Oh...
OK. I see.
}

TEST(AzureFileSystem, OptionsCompare) {
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Force GitHub README to respect dark mode\n(function() {\n var style = document.createElement('style');\n style.textContent = '\n .markdown-body {\n color-scheme: dark light;\n }\n .markdown-body pre { background: #161b22 !important; }\n .markdown-body code { background: rgba(110, 118, 129, 0.4) !important; }\n .markdown-body table th, .markdown-body table td { border-color: #30363d !important; }\n .markdown-body img { background: #0d1117; }\n .markdown-body blockquote { border-left-color: #8b949e; }\n .markdown-body hr { border-color: #30363d; }\n ';\n document.head.appendChild(style);\n})();", "GitHub Dark Mode README Fix");
}
} catch(__e) { console.warn('[Userscript:GitHub Dark Mode README Fix]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
GH-39262: [C++][Azure][FS] Add default credential auth configuration#39263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

kou
merged 6 commits into

from

Dec 19, 2023
 Merged


GH-39262: [C++][Azure][FS] Add default credential auth configuration
#39263
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


6 commits


Select commit
Hold shift + click to select a range
927ff6d

Implement default credential


Tom-Newton Dec 17, 2023
56d796f

Simple test


Tom-Newton Dec 17, 2023
4dbe947

Don't configure azurite for test


Tom-Newton Dec 17, 2023
5e36f90

Make SetUrlsForAccountName private


Tom-Newton Dec 18, 2023
0d9ccab

American spelling


Tom-Newton Dec 18, 2023
c2a8625

Auto-format


Tom-Newton Dec 18, 2023

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


24 changes: 22 additions & 2 deletions
cpp/src/arrow/filesystem/azurefs.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -18,6 +18,7 @@
#include "arrow/filesystem/azurefs.h"
#include "arrow/filesystem/azurefs_internal.h"

#include <azure/identity.hpp>
#include <azure/storage/blobs.hpp>
#include <azure/storage/files/datalake.hpp>

Expand DownExpand Up@@ -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;
Expand All@@ -69,15 +72,26 @@ 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 + "/";
 } else {
 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<Azure::Identity::DefaultAzureCredential>();
 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<Storage::StorageSharedKeyCredential>(account_name, account_key);
Expand All@@ -89,6 +103,9 @@ Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceC
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<Blobs::BlobServiceClient>(account_blob_url_,
 token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<Blobs::BlobServiceClient>(account_blob_url_,
 storage_shared_key_credential_);
Expand All@@ -101,6 +118,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(account_dfs_url_,
 token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(
 account_dfs_url_, storage_shared_key_credential_);
Expand Down

7 changes: 7 additions & 0 deletions
cpp/src/arrow/filesystem/azurefs.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -70,16 +70,23 @@ struct ARROW_EXPORT AzureOptions {

 enum class CredentialKind {
 kAnonymous,
 kTokenCredential,
 kStorageSharedKeyCredential,
 } credential_kind_ = CredentialKind::kAnonymous;

 std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
 storage_shared_key_credential_;

 std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;

 void SetUrlsForAccountName(const std::string& account_name);

 public:
 AzureOptions();
 ~AzureOptions();

 Status ConfigureDefaultCredential(const std::string& account_name);

 Status ConfigureAccountKeyCredential(const std::string& account_name,
 const std::string& account_key);

Expand Down

18 changes: 6 additions & 12 deletions
cpp/src/arrow/filesystem/azurefs_test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -43,9 +43,6 @@
#include <gmock/gmock-matchers.h>
#include <gmock/gmock-more-matchers.h>
#include <gtest/gtest.h>
#include <azure/identity/client_secret_credential.hpp>
#include <azure/identity/default_azure_credential.hpp>
#include <azure/identity/managed_identity_credential.hpp>
#include <azure/storage/blobs.hpp>
#include <azure/storage/common/storage_credential.hpp>
#include <azure/storage/files/datalake.hpp>
Expand DownExpand Up@@ -266,15 +263,12 @@ class AzureHierarchicalNSEnv : public AzureEnvImpl<AzureHierarchicalNSEnv> {
 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<Azure::Identity::DefaultAzureCredential>();
 auto managed_identity_credential =
 std::make_shared<Azure::Identity::ManagedIdentityCredential>();
 auto service_principal_credential =
 std::make_shared<Azure::Identity::ClientSecretCredential>("tenant_id", "client_id",
 "client_secret");
TEST(AzureFileSystem, InitializeFilesystemWithDefaultCredential) {
 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));
@koukouDec 17, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that we can use Azurite with the DefaultAzureCredential: https://learn.microsoft.com/en-us/azure/storage/common/storage-use-azurite?tabs=visual-studio%2Cblob-storage#azure-sdks
Can we do an operation (CreateDir()?) and check the result to verify whether this filesystem is valid or not?
@Tom-NewtonTom-NewtonDec 17, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a bit of a try but I couldn't get the SSL setup working. I'll give it another go but I don't have great hope for making it work in CI even if I get it working locally.
@koukouDec 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I'll also try it.
@koukouDec 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried and understand why you said SSL.
Azure::Identity::DefaultAzureCredential uses a Bearer token and Azure SDK for C++ rejects it with http: Bearer token authentication is not permitted for non TLS protected (https) endpoints.

If we want to use DefaultAzureCredential with Azurite, we need to generate a key and certificate pair and use it.
I looked at how to set it to Azure SDK for C++. It seems that we need to BlobClientOptions::Transport::Transport:
If we set BlobClientOptions::Transport::Transport, we need to specify curl based HTTP transport implementation or WinHTTP based HTTP transport implementation. They have different configurations for TLS...
How about using TestAzureHierarchicalNSFileSystem to test DefaultAzureCredential? If we use the real Azure service, we don't need to custom TLS configuration.
@Tom-NewtonTom-NewtonDec 18, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would definitely be possible to test against real blob storage but there will be a significant amount of manual configuration for all the identities to test all the different authentications.
Then we need to provide details of these identities to TestAzureHierarchicalNSFileSystem either they are required always or we need to add new versions for each auth e.g. TestAzureHierarchicalNSFileSystemWithServicePrincipal.
Also there are some Auth methods that are not feasible to test. For example managed identity can only work on Azure VMs and workload identity can only work in kubernetes.
Personally I don't think this is worthwhile to make a more comprehensive test because of how little complexity there is outside the Azure SDK.
@felipecrvfelipecrvDec 19, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test is enough, because as @pitrou said the other day: "we are not re-implementing the Azure SDK".
@koukouDec 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there are some Auth methods that are not feasible to test. For example managed identity can only work on Azure VMs and workload identity can only work in kubernetes.
Oh...
OK. I see.
}

TEST(AzureFileSystem, OptionsCompare) {
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Highlight search terms from Google/DuckDuckGo/Bing referrer\n(function() {\n var ref = document.referrer;\n var terms = [];\n \n if (ref.includes('google.com') || ref.includes('duckduckgo.com') || ref.includes('bing.com')) {\n var url = new URL(ref);\n var q = url.searchParams.get('q') || url.searchParams.get('p');\n if (q) {\n terms = q.split(/\\s+/).filter(function(t) { return t.length > 2; });\n }\n }\n \n if (terms.length === 0) return;\n \n var style = document.createElement('style');\n style.textContent = '.userscript-highlight { background: #fbbf24; color: #1a1a2e; padding: 1px 3px; border-radius: 2px; }';\n document.head.appendChild(style);\n \n function highlight(node) {\n if (node.nodeType === 3) { // text node\n var text = node.textContent;\n var found = false;\n terms.forEach(function(term) {\n var regex = new RegExp('(' + term.replace(/[.*+?^${}()|[\\]\\\\]/g, '\\\\') + ')', 'gi');\n if (regex.test(text)) {\n found = true;\n var frag = document.createDocumentFragment();\n var parts = text.split(regex);\n parts.forEach(function(part, i) {\n if (i % 2 === 0) {\n frag.appendChild(document.createTextNode(part));\n } else {\n var span = document.createElement('span');\n span.className = 'userscript-highlight';\n span.textContent = part;\n frag.appendChild(span);\n }\n });\n node.parentNode.replaceChild(frag, node);\n }\n });\n } else if (node.nodeType === 1 && node.childNodes) { // element\n var skipTags = ['SCRIPT', 'STYLE', 'NOSCRIPT', 'TEXTAREA', 'INPUT', 'SELECT'];\n if (!skipTags.includes(node.tagName)) {\n Array.from(node.childNodes).forEach(highlight);\n }\n }\n }\n \n highlight(document.body);\n \n // Re-highlight on dynamic content\n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1 || node.nodeType === 3) highlight(node);\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Highlight Search Terms");
}
} catch(__e) { console.warn('[Userscript:Highlight Search Terms]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
GH-39262: [C++][Azure][FS] Add default credential auth configuration#39263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

kou
merged 6 commits into

from

Dec 19, 2023
 Merged


GH-39262: [C++][Azure][FS] Add default credential auth configuration
#39263
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


6 commits


Select commit
Hold shift + click to select a range
927ff6d

Implement default credential


Tom-Newton Dec 17, 2023
56d796f

Simple test


Tom-Newton Dec 17, 2023
4dbe947

Don't configure azurite for test


Tom-Newton Dec 17, 2023
5e36f90

Make SetUrlsForAccountName private


Tom-Newton Dec 18, 2023
0d9ccab

American spelling


Tom-Newton Dec 18, 2023
c2a8625

Auto-format


Tom-Newton Dec 18, 2023

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


24 changes: 22 additions & 2 deletions
cpp/src/arrow/filesystem/azurefs.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -18,6 +18,7 @@
#include "arrow/filesystem/azurefs.h"
#include "arrow/filesystem/azurefs_internal.h"

#include <azure/identity.hpp>
#include <azure/storage/blobs.hpp>
#include <azure/storage/files/datalake.hpp>

Expand DownExpand Up@@ -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;
Expand All@@ -69,15 +72,26 @@ 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 + "/";
 } else {
 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<Azure::Identity::DefaultAzureCredential>();
 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<Storage::StorageSharedKeyCredential>(account_name, account_key);
Expand All@@ -89,6 +103,9 @@ Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceC
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<Blobs::BlobServiceClient>(account_blob_url_,
 token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<Blobs::BlobServiceClient>(account_blob_url_,
 storage_shared_key_credential_);
Expand All@@ -101,6 +118,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(account_dfs_url_,
 token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(
 account_dfs_url_, storage_shared_key_credential_);
Expand Down

7 changes: 7 additions & 0 deletions
cpp/src/arrow/filesystem/azurefs.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -70,16 +70,23 @@ struct ARROW_EXPORT AzureOptions {

 enum class CredentialKind {
 kAnonymous,
 kTokenCredential,
 kStorageSharedKeyCredential,
 } credential_kind_ = CredentialKind::kAnonymous;

 std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
 storage_shared_key_credential_;

 std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;

 void SetUrlsForAccountName(const std::string& account_name);

 public:
 AzureOptions();
 ~AzureOptions();

 Status ConfigureDefaultCredential(const std::string& account_name);

 Status ConfigureAccountKeyCredential(const std::string& account_name,
 const std::string& account_key);

Expand Down

18 changes: 6 additions & 12 deletions
cpp/src/arrow/filesystem/azurefs_test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -43,9 +43,6 @@
#include <gmock/gmock-matchers.h>
#include <gmock/gmock-more-matchers.h>
#include <gtest/gtest.h>
#include <azure/identity/client_secret_credential.hpp>
#include <azure/identity/default_azure_credential.hpp>
#include <azure/identity/managed_identity_credential.hpp>
#include <azure/storage/blobs.hpp>
#include <azure/storage/common/storage_credential.hpp>
#include <azure/storage/files/datalake.hpp>
Expand DownExpand Up@@ -266,15 +263,12 @@ class AzureHierarchicalNSEnv : public AzureEnvImpl<AzureHierarchicalNSEnv> {
 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<Azure::Identity::DefaultAzureCredential>();
 auto managed_identity_credential =
 std::make_shared<Azure::Identity::ManagedIdentityCredential>();
 auto service_principal_credential =
 std::make_shared<Azure::Identity::ClientSecretCredential>("tenant_id", "client_id",
 "client_secret");
TEST(AzureFileSystem, InitializeFilesystemWithDefaultCredential) {
 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));
@koukouDec 17, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that we can use Azurite with the DefaultAzureCredential: https://learn.microsoft.com/en-us/azure/storage/common/storage-use-azurite?tabs=visual-studio%2Cblob-storage#azure-sdks
Can we do an operation (CreateDir()?) and check the result to verify whether this filesystem is valid or not?
@Tom-NewtonTom-NewtonDec 17, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a bit of a try but I couldn't get the SSL setup working. I'll give it another go but I don't have great hope for making it work in CI even if I get it working locally.
@koukouDec 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I'll also try it.
@koukouDec 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried and understand why you said SSL.
Azure::Identity::DefaultAzureCredential uses a Bearer token and Azure SDK for C++ rejects it with http: Bearer token authentication is not permitted for non TLS protected (https) endpoints.

If we want to use DefaultAzureCredential with Azurite, we need to generate a key and certificate pair and use it.
I looked at how to set it to Azure SDK for C++. It seems that we need to BlobClientOptions::Transport::Transport:
If we set BlobClientOptions::Transport::Transport, we need to specify curl based HTTP transport implementation or WinHTTP based HTTP transport implementation. They have different configurations for TLS...
How about using TestAzureHierarchicalNSFileSystem to test DefaultAzureCredential? If we use the real Azure service, we don't need to custom TLS configuration.
@Tom-NewtonTom-NewtonDec 18, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would definitely be possible to test against real blob storage but there will be a significant amount of manual configuration for all the identities to test all the different authentications.
Then we need to provide details of these identities to TestAzureHierarchicalNSFileSystem either they are required always or we need to add new versions for each auth e.g. TestAzureHierarchicalNSFileSystemWithServicePrincipal.
Also there are some Auth methods that are not feasible to test. For example managed identity can only work on Azure VMs and workload identity can only work in kubernetes.
Personally I don't think this is worthwhile to make a more comprehensive test because of how little complexity there is outside the Azure SDK.
@felipecrvfelipecrvDec 19, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test is enough, because as @pitrou said the other day: "we are not re-implementing the Azure SDK".
@koukouDec 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there are some Auth methods that are not feasible to test. For example managed identity can only work on Azure VMs and workload identity can only work in kubernetes.
Oh...
OK. I see.
}

TEST(AzureFileSystem, OptionsCompare) {
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Strip utm_, fbclid, gclid, etc. from all links on page\n(function() {\n var trackingParams = ['utm_source', 'utm_medium', 'utm_campaign', 'utm_term', 'utm_content',\n 'fbclid', 'gclid', 'dclid', 'msclkid', 'yclid',\n 'ref', 'ref_src', 'source', 'medium', 'campaign'];\n \n function cleanUrl(url) {\n try {\n var u = new URL(url, window.location.origin);\n var changed = false;\n trackingParams.forEach(function(p) {\n if (u.searchParams.has(p)) {\n u.searchParams.delete(p);\n changed = true;\n }\n });\n return changed ? u.toString() : url;\n } catch (e) {\n return url;\n }\n }\n \n function cleanLinks() {\n document.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n \n cleanLinks();\n \n var observer = new MutationObserver(function(mutations) {\n mutations.forEach(function(m) {\n m.addedNodes.forEach(function(node) {\n if (node.nodeType === 1) {\n if (node.tagName === 'A') cleanLinks();\n node.querySelectorAll('a[href]').forEach(function(a) {\n var clean = cleanUrl(a.href);\n if (clean !== a.href) a.href = clean;\n });\n }\n });\n });\n });\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "Remove Tracking Parameters from Links");
}
} catch(__e) { console.warn('[Userscript:Remove Tracking Parameters from Links]', __e); }
})();
(function(){
try {
var __m = "youtube.com";
var __re = new RegExp('^' + "youtube\\.com" + '

Skip to content
GH-39262: [C++][Azure][FS] Add default credential auth configuration#39263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

kou
merged 6 commits into

from

Dec 19, 2023
 Merged


GH-39262: [C++][Azure][FS] Add default credential auth configuration
#39263
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


6 commits


Select commit
Hold shift + click to select a range
927ff6d

Implement default credential


Tom-Newton Dec 17, 2023
56d796f

Simple test


Tom-Newton Dec 17, 2023
4dbe947

Don't configure azurite for test


Tom-Newton Dec 17, 2023
5e36f90

Make SetUrlsForAccountName private


Tom-Newton Dec 18, 2023
0d9ccab

American spelling


Tom-Newton Dec 18, 2023
c2a8625

Auto-format


Tom-Newton Dec 18, 2023

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


24 changes: 22 additions & 2 deletions
cpp/src/arrow/filesystem/azurefs.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -18,6 +18,7 @@
#include "arrow/filesystem/azurefs.h"
#include "arrow/filesystem/azurefs_internal.h"

#include <azure/identity.hpp>
#include <azure/storage/blobs.hpp>
#include <azure/storage/files/datalake.hpp>

Expand DownExpand Up@@ -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;
Expand All@@ -69,15 +72,26 @@ 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 + "/";
 } else {
 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<Azure::Identity::DefaultAzureCredential>();
 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<Storage::StorageSharedKeyCredential>(account_name, account_key);
Expand All@@ -89,6 +103,9 @@ Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceC
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<Blobs::BlobServiceClient>(account_blob_url_,
 token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<Blobs::BlobServiceClient>(account_blob_url_,
 storage_shared_key_credential_);
Expand All@@ -101,6 +118,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(account_dfs_url_,
 token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(
 account_dfs_url_, storage_shared_key_credential_);
Expand Down

7 changes: 7 additions & 0 deletions
cpp/src/arrow/filesystem/azurefs.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -70,16 +70,23 @@ struct ARROW_EXPORT AzureOptions {

 enum class CredentialKind {
 kAnonymous,
 kTokenCredential,
 kStorageSharedKeyCredential,
 } credential_kind_ = CredentialKind::kAnonymous;

 std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
 storage_shared_key_credential_;

 std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;

 void SetUrlsForAccountName(const std::string& account_name);

 public:
 AzureOptions();
 ~AzureOptions();

 Status ConfigureDefaultCredential(const std::string& account_name);

 Status ConfigureAccountKeyCredential(const std::string& account_name,
 const std::string& account_key);

Expand Down

18 changes: 6 additions & 12 deletions
cpp/src/arrow/filesystem/azurefs_test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -43,9 +43,6 @@
#include <gmock/gmock-matchers.h>
#include <gmock/gmock-more-matchers.h>
#include <gtest/gtest.h>
#include <azure/identity/client_secret_credential.hpp>
#include <azure/identity/default_azure_credential.hpp>
#include <azure/identity/managed_identity_credential.hpp>
#include <azure/storage/blobs.hpp>
#include <azure/storage/common/storage_credential.hpp>
#include <azure/storage/files/datalake.hpp>
Expand DownExpand Up@@ -266,15 +263,12 @@ class AzureHierarchicalNSEnv : public AzureEnvImpl<AzureHierarchicalNSEnv> {
 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<Azure::Identity::DefaultAzureCredential>();
 auto managed_identity_credential =
 std::make_shared<Azure::Identity::ManagedIdentityCredential>();
 auto service_principal_credential =
 std::make_shared<Azure::Identity::ClientSecretCredential>("tenant_id", "client_id",
 "client_secret");
TEST(AzureFileSystem, InitializeFilesystemWithDefaultCredential) {
 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));
@koukouDec 17, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that we can use Azurite with the DefaultAzureCredential: https://learn.microsoft.com/en-us/azure/storage/common/storage-use-azurite?tabs=visual-studio%2Cblob-storage#azure-sdks
Can we do an operation (CreateDir()?) and check the result to verify whether this filesystem is valid or not?
@Tom-NewtonTom-NewtonDec 17, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a bit of a try but I couldn't get the SSL setup working. I'll give it another go but I don't have great hope for making it work in CI even if I get it working locally.
@koukouDec 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I'll also try it.
@koukouDec 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried and understand why you said SSL.
Azure::Identity::DefaultAzureCredential uses a Bearer token and Azure SDK for C++ rejects it with http: Bearer token authentication is not permitted for non TLS protected (https) endpoints.

If we want to use DefaultAzureCredential with Azurite, we need to generate a key and certificate pair and use it.
I looked at how to set it to Azure SDK for C++. It seems that we need to BlobClientOptions::Transport::Transport:
If we set BlobClientOptions::Transport::Transport, we need to specify curl based HTTP transport implementation or WinHTTP based HTTP transport implementation. They have different configurations for TLS...
How about using TestAzureHierarchicalNSFileSystem to test DefaultAzureCredential? If we use the real Azure service, we don't need to custom TLS configuration.
@Tom-NewtonTom-NewtonDec 18, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would definitely be possible to test against real blob storage but there will be a significant amount of manual configuration for all the identities to test all the different authentications.
Then we need to provide details of these identities to TestAzureHierarchicalNSFileSystem either they are required always or we need to add new versions for each auth e.g. TestAzureHierarchicalNSFileSystemWithServicePrincipal.
Also there are some Auth methods that are not feasible to test. For example managed identity can only work on Azure VMs and workload identity can only work in kubernetes.
Personally I don't think this is worthwhile to make a more comprehensive test because of how little complexity there is outside the Azure SDK.
@felipecrvfelipecrvDec 19, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test is enough, because as @pitrou said the other day: "we are not re-implementing the Azure SDK".
@koukouDec 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there are some Auth methods that are not feasible to test. For example managed identity can only work on Azure VMs and workload identity can only work in kubernetes.
Oh...
OK. I see.
}

TEST(AzureFileSystem, OptionsCompare) {
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Auto-enable theater mode on YouTube\n(function() {\n function tryTheater() {\n var btn = document.querySelector('button[aria-label=\"Theater mode\"], ytd-player #player button[title=\"Theater mode\"]');\n if (btn && !btn.classList.contains('activated')) {\n btn.click();\n }\n }\n \n // Try immediately\n tryTheater();\n \n // Try after navigation (SPA)\n var lastUrl = location.href;\n setInterval(function() {\n if (location.href !== lastUrl) {\n lastUrl = location.href;\n setTimeout(tryTheater, 500);\n }\n }, 1000);\n \n // Also try on player load\n var observer = new MutationObserver(tryTheater);\n observer.observe(document.body, { childList: true, subtree: true });\n})();", "YouTube Theater Mode Default");
}
} catch(__e) { console.warn('[Userscript:YouTube Theater Mode Default]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
GH-39262: [C++][Azure][FS] Add default credential auth configuration#39263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

kou
merged 6 commits into

from

Dec 19, 2023
 Merged


GH-39262: [C++][Azure][FS] Add default credential auth configuration
#39263
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


6 commits


Select commit
Hold shift + click to select a range
927ff6d

Implement default credential


Tom-Newton Dec 17, 2023
56d796f

Simple test


Tom-Newton Dec 17, 2023
4dbe947

Don't configure azurite for test


Tom-Newton Dec 17, 2023
5e36f90

Make SetUrlsForAccountName private


Tom-Newton Dec 18, 2023
0d9ccab

American spelling


Tom-Newton Dec 18, 2023
c2a8625

Auto-format


Tom-Newton Dec 18, 2023

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


24 changes: 22 additions & 2 deletions
cpp/src/arrow/filesystem/azurefs.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -18,6 +18,7 @@
#include "arrow/filesystem/azurefs.h"
#include "arrow/filesystem/azurefs_internal.h"

#include <azure/identity.hpp>
#include <azure/storage/blobs.hpp>
#include <azure/storage/files/datalake.hpp>

Expand DownExpand Up@@ -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;
Expand All@@ -69,15 +72,26 @@ 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 + "/";
 } else {
 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<Azure::Identity::DefaultAzureCredential>();
 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<Storage::StorageSharedKeyCredential>(account_name, account_key);
Expand All@@ -89,6 +103,9 @@ Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceC
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<Blobs::BlobServiceClient>(account_blob_url_,
 token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<Blobs::BlobServiceClient>(account_blob_url_,
 storage_shared_key_credential_);
Expand All@@ -101,6 +118,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(account_dfs_url_,
 token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(
 account_dfs_url_, storage_shared_key_credential_);
Expand Down

7 changes: 7 additions & 0 deletions
cpp/src/arrow/filesystem/azurefs.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -70,16 +70,23 @@ struct ARROW_EXPORT AzureOptions {

 enum class CredentialKind {
 kAnonymous,
 kTokenCredential,
 kStorageSharedKeyCredential,
 } credential_kind_ = CredentialKind::kAnonymous;

 std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
 storage_shared_key_credential_;

 std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;

 void SetUrlsForAccountName(const std::string& account_name);

 public:
 AzureOptions();
 ~AzureOptions();

 Status ConfigureDefaultCredential(const std::string& account_name);

 Status ConfigureAccountKeyCredential(const std::string& account_name,
 const std::string& account_key);

Expand Down

18 changes: 6 additions & 12 deletions
cpp/src/arrow/filesystem/azurefs_test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -43,9 +43,6 @@
#include <gmock/gmock-matchers.h>
#include <gmock/gmock-more-matchers.h>
#include <gtest/gtest.h>
#include <azure/identity/client_secret_credential.hpp>
#include <azure/identity/default_azure_credential.hpp>
#include <azure/identity/managed_identity_credential.hpp>
#include <azure/storage/blobs.hpp>
#include <azure/storage/common/storage_credential.hpp>
#include <azure/storage/files/datalake.hpp>
Expand DownExpand Up@@ -266,15 +263,12 @@ class AzureHierarchicalNSEnv : public AzureEnvImpl<AzureHierarchicalNSEnv> {
 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<Azure::Identity::DefaultAzureCredential>();
 auto managed_identity_credential =
 std::make_shared<Azure::Identity::ManagedIdentityCredential>();
 auto service_principal_credential =
 std::make_shared<Azure::Identity::ClientSecretCredential>("tenant_id", "client_id",
 "client_secret");
TEST(AzureFileSystem, InitializeFilesystemWithDefaultCredential) {
 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));
@koukouDec 17, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that we can use Azurite with the DefaultAzureCredential: https://learn.microsoft.com/en-us/azure/storage/common/storage-use-azurite?tabs=visual-studio%2Cblob-storage#azure-sdks
Can we do an operation (CreateDir()?) and check the result to verify whether this filesystem is valid or not?
@Tom-NewtonTom-NewtonDec 17, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a bit of a try but I couldn't get the SSL setup working. I'll give it another go but I don't have great hope for making it work in CI even if I get it working locally.
@koukouDec 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I'll also try it.
@koukouDec 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried and understand why you said SSL.
Azure::Identity::DefaultAzureCredential uses a Bearer token and Azure SDK for C++ rejects it with http: Bearer token authentication is not permitted for non TLS protected (https) endpoints.

If we want to use DefaultAzureCredential with Azurite, we need to generate a key and certificate pair and use it.
I looked at how to set it to Azure SDK for C++. It seems that we need to BlobClientOptions::Transport::Transport:
If we set BlobClientOptions::Transport::Transport, we need to specify curl based HTTP transport implementation or WinHTTP based HTTP transport implementation. They have different configurations for TLS...
How about using TestAzureHierarchicalNSFileSystem to test DefaultAzureCredential? If we use the real Azure service, we don't need to custom TLS configuration.
@Tom-NewtonTom-NewtonDec 18, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would definitely be possible to test against real blob storage but there will be a significant amount of manual configuration for all the identities to test all the different authentications.
Then we need to provide details of these identities to TestAzureHierarchicalNSFileSystem either they are required always or we need to add new versions for each auth e.g. TestAzureHierarchicalNSFileSystemWithServicePrincipal.
Also there are some Auth methods that are not feasible to test. For example managed identity can only work on Azure VMs and workload identity can only work in kubernetes.
Personally I don't think this is worthwhile to make a more comprehensive test because of how little complexity there is outside the Azure SDK.
@felipecrvfelipecrvDec 19, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test is enough, because as @pitrou said the other day: "we are not re-implementing the Azure SDK".
@koukouDec 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there are some Auth methods that are not feasible to test. For example managed identity can only work on Azure VMs and workload identity can only work in kubernetes.
Oh...
OK. I see.
}

TEST(AzureFileSystem, OptionsCompare) {
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Remove or un-stick sticky/fixed headers that block content\n(function() {\n function unstick() {\n document.querySelectorAll('header, nav, [role=\"banner\"], .header, .navbar, .sticky, .fixed-top, [style*=\"position: fixed\"], [style*=\"position:sticky\"]').forEach(function(el) {\n if (el.style.position === 'fixed' || el.style.position === 'sticky' || \n getComputedStyle(el).position === 'fixed' || getComputedStyle(el).position === 'sticky') {\n el.style.position = 'static';\n el.style.top = 'auto';\n el.style.zIndex = 'auto';\n }\n });\n }\n \n unstick();\n \n var observer = new MutationObserver(unstick);\n observer.observe(document.body, { childList: true, subtree: true, attributes: true, attributeFilter: ['style', 'class'] });\n})();", "Kill Sticky Headers");
}
} catch(__e) { console.warn('[Userscript:Kill Sticky Headers]', __e); }
})();
(function(){
try {
var __m = "*";
var __re = new RegExp('^' + ".*" + '

Skip to content
GH-39262: [C++][Azure][FS] Add default credential auth configuration#39263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

kou
merged 6 commits into

from

Dec 19, 2023
 Merged


GH-39262: [C++][Azure][FS] Add default credential auth configuration
#39263
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


6 commits


Select commit
Hold shift + click to select a range
927ff6d

Implement default credential


Tom-Newton Dec 17, 2023
56d796f

Simple test


Tom-Newton Dec 17, 2023
4dbe947

Don't configure azurite for test


Tom-Newton Dec 17, 2023
5e36f90

Make SetUrlsForAccountName private


Tom-Newton Dec 18, 2023
0d9ccab

American spelling


Tom-Newton Dec 18, 2023
c2a8625

Auto-format


Tom-Newton Dec 18, 2023

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


24 changes: 22 additions & 2 deletions
cpp/src/arrow/filesystem/azurefs.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -18,6 +18,7 @@
#include "arrow/filesystem/azurefs.h"
#include "arrow/filesystem/azurefs_internal.h"

#include <azure/identity.hpp>
#include <azure/storage/blobs.hpp>
#include <azure/storage/files/datalake.hpp>

Expand DownExpand Up@@ -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;
Expand All@@ -69,15 +72,26 @@ 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 + "/";
 } else {
 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<Azure::Identity::DefaultAzureCredential>();
 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<Storage::StorageSharedKeyCredential>(account_name, account_key);
Expand All@@ -89,6 +103,9 @@ Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceC
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<Blobs::BlobServiceClient>(account_blob_url_,
 token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<Blobs::BlobServiceClient>(account_blob_url_,
 storage_shared_key_credential_);
Expand All@@ -101,6 +118,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(account_dfs_url_,
 token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(
 account_dfs_url_, storage_shared_key_credential_);
Expand Down

7 changes: 7 additions & 0 deletions
cpp/src/arrow/filesystem/azurefs.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -70,16 +70,23 @@ struct ARROW_EXPORT AzureOptions {

 enum class CredentialKind {
 kAnonymous,
 kTokenCredential,
 kStorageSharedKeyCredential,
 } credential_kind_ = CredentialKind::kAnonymous;

 std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
 storage_shared_key_credential_;

 std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;

 void SetUrlsForAccountName(const std::string& account_name);

 public:
 AzureOptions();
 ~AzureOptions();

 Status ConfigureDefaultCredential(const std::string& account_name);

 Status ConfigureAccountKeyCredential(const std::string& account_name,
 const std::string& account_key);

Expand Down

18 changes: 6 additions & 12 deletions
cpp/src/arrow/filesystem/azurefs_test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -43,9 +43,6 @@
#include <gmock/gmock-matchers.h>
#include <gmock/gmock-more-matchers.h>
#include <gtest/gtest.h>
#include <azure/identity/client_secret_credential.hpp>
#include <azure/identity/default_azure_credential.hpp>
#include <azure/identity/managed_identity_credential.hpp>
#include <azure/storage/blobs.hpp>
#include <azure/storage/common/storage_credential.hpp>
#include <azure/storage/files/datalake.hpp>
Expand DownExpand Up@@ -266,15 +263,12 @@ class AzureHierarchicalNSEnv : public AzureEnvImpl<AzureHierarchicalNSEnv> {
 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<Azure::Identity::DefaultAzureCredential>();
 auto managed_identity_credential =
 std::make_shared<Azure::Identity::ManagedIdentityCredential>();
 auto service_principal_credential =
 std::make_shared<Azure::Identity::ClientSecretCredential>("tenant_id", "client_id",
 "client_secret");
TEST(AzureFileSystem, InitializeFilesystemWithDefaultCredential) {
 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));
@koukouDec 17, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that we can use Azurite with the DefaultAzureCredential: https://learn.microsoft.com/en-us/azure/storage/common/storage-use-azurite?tabs=visual-studio%2Cblob-storage#azure-sdks
Can we do an operation (CreateDir()?) and check the result to verify whether this filesystem is valid or not?
@Tom-NewtonTom-NewtonDec 17, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a bit of a try but I couldn't get the SSL setup working. I'll give it another go but I don't have great hope for making it work in CI even if I get it working locally.
@koukouDec 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I'll also try it.
@koukouDec 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried and understand why you said SSL.
Azure::Identity::DefaultAzureCredential uses a Bearer token and Azure SDK for C++ rejects it with http: Bearer token authentication is not permitted for non TLS protected (https) endpoints.

If we want to use DefaultAzureCredential with Azurite, we need to generate a key and certificate pair and use it.
I looked at how to set it to Azure SDK for C++. It seems that we need to BlobClientOptions::Transport::Transport:
If we set BlobClientOptions::Transport::Transport, we need to specify curl based HTTP transport implementation or WinHTTP based HTTP transport implementation. They have different configurations for TLS...
How about using TestAzureHierarchicalNSFileSystem to test DefaultAzureCredential? If we use the real Azure service, we don't need to custom TLS configuration.
@Tom-NewtonTom-NewtonDec 18, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would definitely be possible to test against real blob storage but there will be a significant amount of manual configuration for all the identities to test all the different authentications.
Then we need to provide details of these identities to TestAzureHierarchicalNSFileSystem either they are required always or we need to add new versions for each auth e.g. TestAzureHierarchicalNSFileSystemWithServicePrincipal.
Also there are some Auth methods that are not feasible to test. For example managed identity can only work on Azure VMs and workload identity can only work in kubernetes.
Personally I don't think this is worthwhile to make a more comprehensive test because of how little complexity there is outside the Azure SDK.
@felipecrvfelipecrvDec 19, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test is enough, because as @pitrou said the other day: "we are not re-implementing the Azure SDK".
@koukouDec 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there are some Auth methods that are not feasible to test. For example managed identity can only work on Azure VMs and workload identity can only work in kubernetes.
Oh...
OK. I see.
}

TEST(AzureFileSystem, OptionsCompare) {
Expand Down

, 'i');
if (__m === '*' || __re.test(location.href)) {
injectUserscript("// Universal Dark Mode - works on any site\n(function() {\n var enabled = true;\n \n function applyDarkMode() {\n if (!enabled) return;\n \n // Create style element if it doesn't exist\n var style = document.getElementById('universal-dark-mode-style');\n if (!style) {\n style = document.createElement('style');\n style.id = 'universal-dark-mode-style';\n document.head.appendChild(style);\n }\n \n // Dark mode CSS - inverts colors but preserves images/video\n style.textContent = '\n /* Invert everything except media */\n html {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #1a1a2e !important;\n }\n \n /* Restore images, videos, iframes, canvas */\n img, video, iframe, canvas, svg, picture, [style*=\"background-image\"] {\n filter: invert(1) hue-rotate(180deg) !important;\n }\n \n /* Preserve specific elements that should not be inverted */\n .no-dark-mode, .no-dark-mode *,\n [data-theme=\"light\"], [data-theme=\"light\"],\n .ace_editor, .ace_editor *,\n .CodeMirror, .CodeMirror *,\n .monaco-editor, .monaco-editor *,\n .markdown-body pre, .markdown-body pre *,\n .highlight, .highlight *,\n pre code, pre code * {\n filter: none !important;\n }\n \n /* Fix common UI elements */\n .modal, .popup, .dropdown-menu, .tooltip, .popover {\n filter: invert(1) hue-rotate(180deg) !important;\n background: #2d2d44 !important;\n border-color: #444 !important;\n }\n \n /* Scrollbars */\n ::-webkit-scrollbar { background: #1a1a2e !important; }\n ::-webkit-scrollbar-thumb { background: #444 !important; }\n ::-webkit-scrollbar-thumb:hover { background: #555 !important; }\n \n /* Selection */\n ::selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ::-moz-selection { background: #4ecdc4 !important; color: #1a1a2e !important; }\n ';\n }\n \n function removeDarkMode() {\n var style = document.getElementById('universal-dark-mode-style');\n if (style) style.remove();\n }\n \n // Toggle with Alt+Shift+D\n document.addEventListener('keydown', function(e) {\n if (e.altKey && e.shiftKey && e.key === 'D') {\n e.preventDefault();\n enabled = !enabled;\n if (enabled) {\n applyDarkMode();\n console.log('[Universal Dark Mode] Enabled');\n } else {\n removeDarkMode();\n console.log('[Universal Dark Mode] Disabled');\n }\n }\n });\n \n // Apply on load\n applyDarkMode();\n \n // Re-apply on dynamic content\n var observer = new MutationObserver(function(mutations) {\n if (enabled && !document.getElementById('universal-dark-mode-style')) {\n applyDarkMode();\n }\n });\n observer.observe(document.head, { childList: true });\n \n console.log('[Universal Dark Mode] Loaded - Press Alt+Shift+D to toggle');\n})();", "Universal Dark Mode");
}
} catch(__e) { console.warn('[Userscript:Universal Dark Mode]', __e); }
})();
})();

Skip to content
GH-39262: [C++][Azure][FS] Add default credential auth configuration#39263

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

 Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and
privacy statement. We’ll occasionally send you account related emails.

Already on GitHub?
Sign in
to your account

 Merged

kou
merged 6 commits into

from

Dec 19, 2023
 Merged


GH-39262: [C++][Azure][FS] Add default credential auth configuration
#39263
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


6 commits


Select commit
Hold shift + click to select a range
927ff6d

Implement default credential


Tom-Newton Dec 17, 2023
56d796f

Simple test


Tom-Newton Dec 17, 2023
4dbe947

Don't configure azurite for test


Tom-Newton Dec 17, 2023
5e36f90

Make SetUrlsForAccountName private


Tom-Newton Dec 18, 2023
0d9ccab

American spelling


Tom-Newton Dec 18, 2023
c2a8625

Auto-format


Tom-Newton Dec 18, 2023

File filter

Filter by extension
Filter by extension
Conversations

Failed to load comments. 
Loading
Jump to

Jump to file


Failed to load files. 
Loading
 Diff view

 Diff view


24 changes: 22 additions & 2 deletions
cpp/src/arrow/filesystem/azurefs.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -18,6 +18,7 @@
#include "arrow/filesystem/azurefs.h"
#include "arrow/filesystem/azurefs_internal.h"

#include <azure/identity.hpp>
#include <azure/storage/blobs.hpp>
#include <azure/storage/files/datalake.hpp>

Expand DownExpand Up@@ -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;
Expand All@@ -69,15 +72,26 @@ 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 + "/";
 } else {
 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<Azure::Identity::DefaultAzureCredential>();
 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<Storage::StorageSharedKeyCredential>(account_name, account_key);
Expand All@@ -89,6 +103,9 @@ Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceC
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<Blobs::BlobServiceClient>(account_blob_url_,
 token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<Blobs::BlobServiceClient>(account_blob_url_,
 storage_shared_key_credential_);
Expand All@@ -101,6 +118,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(account_dfs_url_,
 token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(
 account_dfs_url_, storage_shared_key_credential_);
Expand Down

7 changes: 7 additions & 0 deletions
cpp/src/arrow/filesystem/azurefs.h

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -70,16 +70,23 @@ struct ARROW_EXPORT AzureOptions {

 enum class CredentialKind {
 kAnonymous,
 kTokenCredential,
 kStorageSharedKeyCredential,
 } credential_kind_ = CredentialKind::kAnonymous;

 std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
 storage_shared_key_credential_;

 std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;

 void SetUrlsForAccountName(const std::string& account_name);

 public:
 AzureOptions();
 ~AzureOptions();

 Status ConfigureDefaultCredential(const std::string& account_name);

 Status ConfigureAccountKeyCredential(const std::string& account_name,
 const std::string& account_key);

Expand Down

18 changes: 6 additions & 12 deletions
cpp/src/arrow/filesystem/azurefs_test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -43,9 +43,6 @@
#include <gmock/gmock-matchers.h>
#include <gmock/gmock-more-matchers.h>
#include <gtest/gtest.h>
#include <azure/identity/client_secret_credential.hpp>
#include <azure/identity/default_azure_credential.hpp>
#include <azure/identity/managed_identity_credential.hpp>
#include <azure/storage/blobs.hpp>
#include <azure/storage/common/storage_credential.hpp>
#include <azure/storage/files/datalake.hpp>
Expand DownExpand Up@@ -266,15 +263,12 @@ class AzureHierarchicalNSEnv : public AzureEnvImpl<AzureHierarchicalNSEnv> {
 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<Azure::Identity::DefaultAzureCredential>();
 auto managed_identity_credential =
 std::make_shared<Azure::Identity::ManagedIdentityCredential>();
 auto service_principal_credential =
 std::make_shared<Azure::Identity::ClientSecretCredential>("tenant_id", "client_id",
 "client_secret");
TEST(AzureFileSystem, InitializeFilesystemWithDefaultCredential) {
 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));
@koukouDec 17, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that we can use Azurite with the DefaultAzureCredential: https://learn.microsoft.com/en-us/azure/storage/common/storage-use-azurite?tabs=visual-studio%2Cblob-storage#azure-sdks
Can we do an operation (CreateDir()?) and check the result to verify whether this filesystem is valid or not?
@Tom-NewtonTom-NewtonDec 17, 2023

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had a bit of a try but I couldn't get the SSL setup working. I'll give it another go but I don't have great hope for making it work in CI even if I get it working locally.
@koukouDec 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I'll also try it.
@koukouDec 18, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried and understand why you said SSL.
Azure::Identity::DefaultAzureCredential uses a Bearer token and Azure SDK for C++ rejects it with http: Bearer token authentication is not permitted for non TLS protected (https) endpoints.

If we want to use DefaultAzureCredential with Azurite, we need to generate a key and certificate pair and use it.
I looked at how to set it to Azure SDK for C++. It seems that we need to BlobClientOptions::Transport::Transport:
If we set BlobClientOptions::Transport::Transport, we need to specify curl based HTTP transport implementation or WinHTTP based HTTP transport implementation. They have different configurations for TLS...
How about using TestAzureHierarchicalNSFileSystem to test DefaultAzureCredential? If we use the real Azure service, we don't need to custom TLS configuration.
@Tom-NewtonTom-NewtonDec 18, 2023•

edited

Loading

Copy link


Copy Markdown

ContributorAuthor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would definitely be possible to test against real blob storage but there will be a significant amount of manual configuration for all the identities to test all the different authentications.
Then we need to provide details of these identities to TestAzureHierarchicalNSFileSystem either they are required always or we need to add new versions for each auth e.g. TestAzureHierarchicalNSFileSystemWithServicePrincipal.
Also there are some Auth methods that are not feasible to test. For example managed identity can only work on Azure VMs and workload identity can only work in kubernetes.
Personally I don't think this is worthwhile to make a more comprehensive test because of how little complexity there is outside the Azure SDK.
@felipecrvfelipecrvDec 19, 2023

Copy link


Copy Markdown

Contributor
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this test is enough, because as @pitrou said the other day: "we are not re-implementing the Azure SDK".
@koukouDec 19, 2023

Copy link


Copy Markdown

Member
Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also there are some Auth methods that are not feasible to test. For example managed identity can only work on Azure VMs and workload identity can only work in kubernetes.
Oh...
OK. I see.
}

TEST(AzureFileSystem, OptionsCompare) {
Expand Down