GH-39320: [C++][FS][Azure] Add managed identity auth configuration#39321

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
felipecrv merged 8 commits into from
Dec 23, 2023
Merged

GH-39320: [C++][FS][Azure] Add managed identity auth configuration #39321

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
4bfacdd
Add managed identity
Tom-Newton Dec 20, 2023
f6e40aa
Ensure filesystem initialisation fails without account name
Tom-Newton Dec 22, 2023
d4af840
Fix broken auths
Tom-Newton Dec 22, 2023
3a9b89f
style fix
Tom-Newton Dec 22, 2023
414f582
another code style fix
Tom-Newton Dec 22, 2023
de0a869
Move account_name to AzureOptions constructor
Tom-Newton Dec 22, 2023
8cdcf4b
Revert AzureOptions constructor. Just make account_name a public member.
Tom-Newton Dec 22, 2023
bfa2cc5
Add a test asserting initialisation fails without an account name
Tom-Newton Dec 22, 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
38 changes: 25 additions & 13 deletions cpp/src/arrow/filesystem/azurefs.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -58,7 +58,7 @@ bool AzureOptions::Equals(const AzureOptions& other) const {
blob_storage_scheme == other.blob_storage_scheme &&
dfs_storage_scheme == other.dfs_storage_scheme &&
default_metadata == other.default_metadata &&
account_name_ == other.account_name_ &&
account_name == other.account_name &&
credential_kind_ == other.credential_kind_;
if (!equals) {
return false;
Expand DownExpand Up@@ -104,17 +104,17 @@ std::string AzureOptions::AccountDfsUrl(const std::string& account_name) const {
return BuildBaseUrl(dfs_storage_scheme, dfs_storage_authority, account_name);
}

Status AzureOptions::ConfigureAccountKeyCredential(const std::string& account_name,
const std::string& account_key) {
Status AzureOptions::ConfigureAccountKeyCredential(const std::string& account_key) {
credential_kind_ = CredentialKind::kStorageSharedKeyCredential;
account_name_ = account_name;
if (account_name.empty()) {
return Status::Invalid("AzureOptions doesn't contain a valid account name");
}
storage_shared_key_credential_ =
std::make_shared<Storage::StorageSharedKeyCredential>(account_name, account_key);
return Status::OK();
}

Status AzureOptions::ConfigureClientSecretCredential(const std::string& account_name,
const std::string& tenant_id,
Status AzureOptions::ConfigureClientSecretCredential(const std::string& tenant_id,
const std::string& client_id,
const std::string& client_secret) {
credential_kind_ = CredentialKind::kTokenCredential;
Expand All@@ -123,45 +123,57 @@ Status AzureOptions::ConfigureClientSecretCredential(const std::string& account_
return Status::OK();
}

Status AzureOptions::ConfigureDefaultCredential(const std::string& account_name) {
Status AzureOptions::ConfigureDefaultCredential() {
credential_kind_ = CredentialKind::kTokenCredential;
token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();
return Status::OK();
}

Status AzureOptions::ConfigureWorkloadIdentityCredential(
const std::string& account_name) {
Status AzureOptions::ConfigureManagedIdentityCredential(const std::string& client_id) {
credential_kind_ = CredentialKind::kTokenCredential;
token_credential_ =
std::make_shared<Azure::Identity::ManagedIdentityCredential>(client_id);
return Status::OK();
}

Status AzureOptions::ConfigureWorkloadIdentityCredential() {
credential_kind_ = CredentialKind::kTokenCredential;
token_credential_ = std::make_shared<Azure::Identity::WorkloadIdentityCredential>();
return Status::OK();
}

Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceClient()
const {
if (account_name.empty()) {
return Status::Invalid("AzureOptions doesn't contain a valid account name");
}

@felipecrvfelipecrvDec 22, 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.

Another simplification we can make:

  • Rename kAnonymous to kDefaultCredential
  • On the kDefaultCredential case here, initialize token_credential_ to std::make_shared<Azure::Identity::DefaultAzureCredential>() if it's not initialized already.

In the kTokenCredential case you can add a DCHECK(token_credential_) as documentation that when credential_kind_ is set to kTokenCredential we expect it to be set already (something that is guaranteed by credential_kind_ being private and set only by functions that also initialize token_credential_.

What you think?

@Tom-NewtonTom-NewtonDec 22, 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 think its useful to support both anonymous and default credential as distinct things. Admittedly, its a niche use-case but storage accounts can be public, which is when anonymous is useful https://learn.microsoft.com/en-us/azure/storage/blobs/anonymous-read-access-configure?tabs=portal.

Its a good question though which should be the default. I know adlfs deafults to anonomous rather than default credential. In some respects it might be good to be consistent but also this choice of default has caused issues for me in the past e.g. flyteorg/flyte#3962

@felipecrvfelipecrvDec 22, 2023•
edited
Loading

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.

Ok. Makes sense to keep kAnonymous then. But what would you return in AzureOptions::MakeBlobServiceClient() when credential_kind_ is anonymous? Lazy-init token_credential_ with DefaultAzureCredential and create the client with it? can you create the client without any credentials?

@felipecrvfelipecrvDec 22, 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 looked and there is a constructor that doesn't take any credentials. I guess that's what we should call :-)

@Tom-NewtonTom-NewtonDec 22, 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.

👍 Do you mind if we do that in a separate PR to fix up anonymous and change the default to default credential. This was supposed to be a super simple PR until kou realised all the auths were broken 😅

@felipecrvfelipecrvDec 22, 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.

No problem.

switch (credential_kind_) {
case CredentialKind::kAnonymous:
break;
case CredentialKind::kTokenCredential:
return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name_),
return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name),
token_credential_);
case CredentialKind::kStorageSharedKeyCredential:
return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name_),
return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name),
storage_shared_key_credential_);
}
return Status::Invalid("AzureOptions doesn't contain a valid auth configuration");
}

Result<std::unique_ptr<DataLake::DataLakeServiceClient>>
AzureOptions::MakeDataLakeServiceClient() const {
if (account_name.empty()) {
return Status::Invalid("AzureOptions doesn't contain a valid account name");
}
switch (credential_kind_) {
case CredentialKind::kAnonymous:
break;
case CredentialKind::kTokenCredential:
return std::make_unique<DataLake::DataLakeServiceClient>(
AccountDfsUrl(account_name_), token_credential_);
AccountDfsUrl(account_name), token_credential_);
case CredentialKind::kStorageSharedKeyCredential:
return std::make_unique<DataLake::DataLakeServiceClient>(
AccountDfsUrl(account_name_), storage_shared_key_credential_);
AccountDfsUrl(account_name), storage_shared_key_credential_);
}
return Status::Invalid("AzureOptions doesn't contain a valid auth configuration");
}
Expand Down
16 changes: 9 additions & 7 deletions cpp/src/arrow/filesystem/azurefs.h
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -48,6 +48,9 @@ class TestAzureFileSystem;

/// Options for the AzureFileSystem implementation.
struct ARROW_EXPORT AzureOptions {
/// \brief account name of the Azure Storage account.
std::string account_name;

/// \brief hostname[:port] of the Azure Blob Storage Service.
///
/// If the hostname is a relative domain name (one that starts with a '.'), then storage
Expand DownExpand Up@@ -94,7 +97,6 @@ struct ARROW_EXPORT AzureOptions {
kStorageSharedKeyCredential,
} credential_kind_ = CredentialKind::kAnonymous;

std::string account_name_;
std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;
std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
storage_shared_key_credential_;
Expand All@@ -103,15 +105,15 @@ struct ARROW_EXPORT AzureOptions {
AzureOptions();
~AzureOptions();

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

Status ConfigureManagedIdentityCredential(const std::string& client_id = std::string());

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

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

Status ConfigureClientSecretCredential(const std::string& account_name,
const std::string& tenant_id,
Status ConfigureClientSecretCredential(const std::string& tenant_id,
const std::string& client_id,
const std::string& client_secret);

Expand Down
34 changes: 28 additions & 6 deletions cpp/src/arrow/filesystem/azurefs_test.cc
View file
Open in desktop
Original file line numberDiff line numberDiff line change
Expand Up@@ -271,22 +271,44 @@ class AzureHierarchicalNSEnv : public AzureEnvImpl<AzureHierarchicalNSEnv> {
bool WithHierarchicalNamespace() const final { return true; }
};

TEST(AzureFileSystem, InitializingFilesystemWithoutAccountNameFails) {
AzureOptions options;
ASSERT_RAISES(Invalid, options.ConfigureAccountKeyCredential("account_key"));

ARROW_EXPECT_OK(
options.ConfigureClientSecretCredential("tenant_id", "client_id", "client_secret"));
ASSERT_RAISES(Invalid, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithClientSecretCredential) {
AzureOptions options;
ARROW_EXPECT_OK(options.ConfigureClientSecretCredential(
"dummy-account-name", "tenant_id", "client_id", "client_secret"));
options.account_name = "dummy-account-name";
ARROW_EXPECT_OK(
options.ConfigureClientSecretCredential("tenant_id", "client_id", "client_secret"));
EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithDefaultCredential) {
AzureOptions options;
ARROW_EXPECT_OK(options.ConfigureDefaultCredential("dummy-account-name"));
options.account_name = "dummy-account-name";
ARROW_EXPECT_OK(options.ConfigureDefaultCredential());
EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithManagedIdentityCredential) {
AzureOptions options;
options.account_name = "dummy-account-name";
ARROW_EXPECT_OK(options.ConfigureManagedIdentityCredential());
EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));

ARROW_EXPECT_OK(options.ConfigureManagedIdentityCredential("specific-client-id"));
EXPECT_OK_AND_ASSIGN(fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithWorkloadIdentityCredential) {
AzureOptions options;
ARROW_EXPECT_OK(options.ConfigureWorkloadIdentityCredential("dummy-account-name"));
options.account_name = "dummy-account-name";
ARROW_EXPECT_OK(options.ConfigureWorkloadIdentityCredential());
EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

Expand DownExpand Up@@ -383,6 +405,7 @@ class TestAzureFileSystem : public ::testing::Test {

static Result<AzureOptions> MakeOptions(BaseAzureEnv* env) {
AzureOptions options;
options.account_name = env->account_name();
switch (env->backend()) {
case AzureBackend::kAzurite:
options.blob_storage_authority = "127.0.0.1:10000";
Expand All@@ -394,8 +417,7 @@ class TestAzureFileSystem : public ::testing::Test {
// Use the default values
break;
}
ARROW_EXPECT_OK(
options.ConfigureAccountKeyCredential(env->account_name(), env->account_key()));
ARROW_EXPECT_OK(options.ConfigureAccountKeyCredential(env->account_key()));
return options;
}

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-39320: [C++][FS][Azure] Add managed identity auth configuration#39321

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

felipecrv
merged 8 commits into

from

Dec 23, 2023
 Merged


GH-39320: [C++][FS][Azure] Add managed identity auth configuration
#39321
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


8 commits


Select commit
Hold shift + click to select a range
4bfacdd

Add managed identity


Tom-Newton Dec 20, 2023
f6e40aa

Ensure filesystem initialisation fails without account name


Tom-Newton Dec 22, 2023
d4af840

Fix broken auths


Tom-Newton Dec 22, 2023
3a9b89f

style fix


Tom-Newton Dec 22, 2023
414f582

another code style fix


Tom-Newton Dec 22, 2023
de0a869

Move account_name to AzureOptions constructor


Tom-Newton Dec 22, 2023
8cdcf4b

Revert AzureOptions constructor. Just make account_name a public member.


Tom-Newton Dec 22, 2023
bfa2cc5

Add a test asserting initialisation fails without an account name


Tom-Newton Dec 22, 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


38 changes: 25 additions & 13 deletions
cpp/src/arrow/filesystem/azurefs.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -58,7 +58,7 @@ bool AzureOptions::Equals(const AzureOptions& other) const {
 blob_storage_scheme == other.blob_storage_scheme &&
 dfs_storage_scheme == other.dfs_storage_scheme &&
 default_metadata == other.default_metadata &&
account_name_ == other.account_name_ &&
account_name == other.account_name &&
 credential_kind_ == other.credential_kind_;
 if (!equals) {
 return false;
Expand DownExpand Up@@ -104,17 +104,17 @@ std::string AzureOptions::AccountDfsUrl(const std::string& account_name) const {
 return BuildBaseUrl(dfs_storage_scheme, dfs_storage_authority, account_name);
}

Status AzureOptions::ConfigureAccountKeyCredential(const std::string& account_name,
 const std::string& account_key) {
Status AzureOptions::ConfigureAccountKeyCredential(const std::string& account_key) {
 credential_kind_ = CredentialKind::kStorageSharedKeyCredential;
 account_name_ = account_name;
 if (account_name.empty()) {
 return Status::Invalid("AzureOptions doesn't contain a valid account name");
 }
 storage_shared_key_credential_ =
 std::make_shared<Storage::StorageSharedKeyCredential>(account_name, account_key);
 return Status::OK();
}

Status AzureOptions::ConfigureClientSecretCredential(const std::string& account_name,
 const std::string& tenant_id,
Status AzureOptions::ConfigureClientSecretCredential(const std::string& tenant_id,
 const std::string& client_id,
 const std::string& client_secret) {
 credential_kind_ = CredentialKind::kTokenCredential;
Expand All@@ -123,45 +123,57 @@ Status AzureOptions::ConfigureClientSecretCredential(const std::string& account_
 return Status::OK();
}

Status AzureOptions::ConfigureDefaultCredential(const std::string& account_name) {
Status AzureOptions::ConfigureDefaultCredential() {
 credential_kind_ = CredentialKind::kTokenCredential;
 token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();
 return Status::OK();
}

Status AzureOptions::ConfigureWorkloadIdentityCredential(
 const std::string& account_name) {
Status AzureOptions::ConfigureManagedIdentityCredential(const std::string& client_id) {
 credential_kind_ = CredentialKind::kTokenCredential;
 token_credential_ =
 std::make_shared<Azure::Identity::ManagedIdentityCredential>(client_id);
 return Status::OK();
}

Status AzureOptions::ConfigureWorkloadIdentityCredential() {
 credential_kind_ = CredentialKind::kTokenCredential;
 token_credential_ = std::make_shared<Azure::Identity::WorkloadIdentityCredential>();
 return Status::OK();
}

Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceClient()
 const {
 if (account_name.empty()) {
 return Status::Invalid("AzureOptions doesn't contain a valid account name");
 }
@felipecrvfelipecrvDec 22, 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.

Another simplification we can make:
  • Rename kAnonymous to kDefaultCredential
  • On the kDefaultCredential case here, initialize token_credential_ to std::make_shared<Azure::Identity::DefaultAzureCredential>() if it's not initialized already.
In the kTokenCredential case you can add a DCHECK(token_credential_) as documentation that when credential_kind_ is set to kTokenCredential we expect it to be set already (something that is guaranteed by credential_kind_ being private and set only by functions that also initialize token_credential_.
What you think?
@Tom-NewtonTom-NewtonDec 22, 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 think its useful to support both anonymous and default credential as distinct things. Admittedly, its a niche use-case but storage accounts can be public, which is when anonymous is useful https://learn.microsoft.com/en-us/azure/storage/blobs/anonymous-read-access-configure?tabs=portal.
Its a good question though which should be the default. I know adlfs deafults to anonomous rather than default credential. In some respects it might be good to be consistent but also this choice of default has caused issues for me in the past e.g. flyteorg/flyte#3962
@felipecrvfelipecrvDec 22, 2023•

edited

Loading

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.

Ok. Makes sense to keep kAnonymous then. But what would you return in AzureOptions::MakeBlobServiceClient() when credential_kind_ is anonymous? Lazy-init token_credential_ with DefaultAzureCredential and create the client with it? can you create the client without any credentials?
@felipecrvfelipecrvDec 22, 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 looked and there is a constructor that doesn't take any credentials. I guess that's what we should call :-)
@Tom-NewtonTom-NewtonDec 22, 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.

👍 Do you mind if we do that in a separate PR to fix up anonymous and change the default to default credential. This was supposed to be a super simple PR until kou realised all the auths were broken 😅
@felipecrvfelipecrvDec 22, 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.

No problem.
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name_),
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name),
 token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name_),
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name),
 storage_shared_key_credential_);
 }
 return Status::Invalid("AzureOptions doesn't contain a valid auth configuration");
}

Result<std::unique_ptr<DataLake::DataLakeServiceClient>>
AzureOptions::MakeDataLakeServiceClient() const {
 if (account_name.empty()) {
 return Status::Invalid("AzureOptions doesn't contain a valid account name");
 }
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(
 AccountDfsUrl(account_name_), token_credential_);
 AccountDfsUrl(account_name), token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(
 AccountDfsUrl(account_name_), storage_shared_key_credential_);
 AccountDfsUrl(account_name), storage_shared_key_credential_);
 }
 return Status::Invalid("AzureOptions doesn't contain a valid auth configuration");
}
Expand Down

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

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -48,6 +48,9 @@ class TestAzureFileSystem;

/// Options for the AzureFileSystem implementation.
struct ARROW_EXPORT AzureOptions {
 /// \brief account name of the Azure Storage account.
 std::string account_name;

 /// \brief hostname[:port] of the Azure Blob Storage Service.
 ///
 /// If the hostname is a relative domain name (one that starts with a '.'), then storage
Expand DownExpand Up@@ -94,7 +97,6 @@ struct ARROW_EXPORT AzureOptions {
 kStorageSharedKeyCredential,
 } credential_kind_ = CredentialKind::kAnonymous;

 std::string account_name_;
 std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;
 std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
 storage_shared_key_credential_;
Expand All@@ -103,15 +105,15 @@ struct ARROW_EXPORT AzureOptions {
 AzureOptions();
 ~AzureOptions();

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

 Status ConfigureManagedIdentityCredential(const std::string& client_id = std::string());

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

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

 Status ConfigureClientSecretCredential(const std::string& account_name,
 const std::string& tenant_id,
 Status ConfigureClientSecretCredential(const std::string& tenant_id,
 const std::string& client_id,
 const std::string& client_secret);

Expand Down

34 changes: 28 additions & 6 deletions
cpp/src/arrow/filesystem/azurefs_test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -271,22 +271,44 @@ class AzureHierarchicalNSEnv : public AzureEnvImpl<AzureHierarchicalNSEnv> {
 bool WithHierarchicalNamespace() const final { return true; }
};

TEST(AzureFileSystem, InitializingFilesystemWithoutAccountNameFails) {
 AzureOptions options;
 ASSERT_RAISES(Invalid, options.ConfigureAccountKeyCredential("account_key"));

 ARROW_EXPECT_OK(
 options.ConfigureClientSecretCredential("tenant_id", "client_id", "client_secret"));
 ASSERT_RAISES(Invalid, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithClientSecretCredential) {
 AzureOptions options;
 ARROW_EXPECT_OK(options.ConfigureClientSecretCredential(
 "dummy-account-name", "tenant_id", "client_id", "client_secret"));
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(
 options.ConfigureClientSecretCredential("tenant_id", "client_id", "client_secret"));
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithDefaultCredential) {
 AzureOptions options;
 ARROW_EXPECT_OK(options.ConfigureDefaultCredential("dummy-account-name"));
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(options.ConfigureDefaultCredential());
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithManagedIdentityCredential) {
 AzureOptions options;
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(options.ConfigureManagedIdentityCredential());
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));

 ARROW_EXPECT_OK(options.ConfigureManagedIdentityCredential("specific-client-id"));
 EXPECT_OK_AND_ASSIGN(fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithWorkloadIdentityCredential) {
 AzureOptions options;
 ARROW_EXPECT_OK(options.ConfigureWorkloadIdentityCredential("dummy-account-name"));
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(options.ConfigureWorkloadIdentityCredential());
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

Expand DownExpand Up@@ -383,6 +405,7 @@ class TestAzureFileSystem : public ::testing::Test {

 static Result<AzureOptions> MakeOptions(BaseAzureEnv* env) {
 AzureOptions options;
 options.account_name = env->account_name();
 switch (env->backend()) {
 case AzureBackend::kAzurite:
 options.blob_storage_authority = "127.0.0.1:10000";
Expand All@@ -394,8 +417,7 @@ class TestAzureFileSystem : public ::testing::Test {
 // Use the default values
 break;
 }
 ARROW_EXPECT_OK(
 options.ConfigureAccountKeyCredential(env->account_name(), env->account_key()));
 ARROW_EXPECT_OK(options.ConfigureAccountKeyCredential(env->account_key()));
 return options;
 }

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-39320: [C++][FS][Azure] Add managed identity auth configuration#39321

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

felipecrv
merged 8 commits into

from

Dec 23, 2023
 Merged


GH-39320: [C++][FS][Azure] Add managed identity auth configuration
#39321
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


8 commits


Select commit
Hold shift + click to select a range
4bfacdd

Add managed identity


Tom-Newton Dec 20, 2023
f6e40aa

Ensure filesystem initialisation fails without account name


Tom-Newton Dec 22, 2023
d4af840

Fix broken auths


Tom-Newton Dec 22, 2023
3a9b89f

style fix


Tom-Newton Dec 22, 2023
414f582

another code style fix


Tom-Newton Dec 22, 2023
de0a869

Move account_name to AzureOptions constructor


Tom-Newton Dec 22, 2023
8cdcf4b

Revert AzureOptions constructor. Just make account_name a public member.


Tom-Newton Dec 22, 2023
bfa2cc5

Add a test asserting initialisation fails without an account name


Tom-Newton Dec 22, 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


38 changes: 25 additions & 13 deletions
cpp/src/arrow/filesystem/azurefs.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -58,7 +58,7 @@ bool AzureOptions::Equals(const AzureOptions& other) const {
 blob_storage_scheme == other.blob_storage_scheme &&
 dfs_storage_scheme == other.dfs_storage_scheme &&
 default_metadata == other.default_metadata &&
account_name_ == other.account_name_ &&
account_name == other.account_name &&
 credential_kind_ == other.credential_kind_;
 if (!equals) {
 return false;
Expand DownExpand Up@@ -104,17 +104,17 @@ std::string AzureOptions::AccountDfsUrl(const std::string& account_name) const {
 return BuildBaseUrl(dfs_storage_scheme, dfs_storage_authority, account_name);
}

Status AzureOptions::ConfigureAccountKeyCredential(const std::string& account_name,
 const std::string& account_key) {
Status AzureOptions::ConfigureAccountKeyCredential(const std::string& account_key) {
 credential_kind_ = CredentialKind::kStorageSharedKeyCredential;
 account_name_ = account_name;
 if (account_name.empty()) {
 return Status::Invalid("AzureOptions doesn't contain a valid account name");
 }
 storage_shared_key_credential_ =
 std::make_shared<Storage::StorageSharedKeyCredential>(account_name, account_key);
 return Status::OK();
}

Status AzureOptions::ConfigureClientSecretCredential(const std::string& account_name,
 const std::string& tenant_id,
Status AzureOptions::ConfigureClientSecretCredential(const std::string& tenant_id,
 const std::string& client_id,
 const std::string& client_secret) {
 credential_kind_ = CredentialKind::kTokenCredential;
Expand All@@ -123,45 +123,57 @@ Status AzureOptions::ConfigureClientSecretCredential(const std::string& account_
 return Status::OK();
}

Status AzureOptions::ConfigureDefaultCredential(const std::string& account_name) {
Status AzureOptions::ConfigureDefaultCredential() {
 credential_kind_ = CredentialKind::kTokenCredential;
 token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();
 return Status::OK();
}

Status AzureOptions::ConfigureWorkloadIdentityCredential(
 const std::string& account_name) {
Status AzureOptions::ConfigureManagedIdentityCredential(const std::string& client_id) {
 credential_kind_ = CredentialKind::kTokenCredential;
 token_credential_ =
 std::make_shared<Azure::Identity::ManagedIdentityCredential>(client_id);
 return Status::OK();
}

Status AzureOptions::ConfigureWorkloadIdentityCredential() {
 credential_kind_ = CredentialKind::kTokenCredential;
 token_credential_ = std::make_shared<Azure::Identity::WorkloadIdentityCredential>();
 return Status::OK();
}

Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceClient()
 const {
 if (account_name.empty()) {
 return Status::Invalid("AzureOptions doesn't contain a valid account name");
 }
@felipecrvfelipecrvDec 22, 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.

Another simplification we can make:
  • Rename kAnonymous to kDefaultCredential
  • On the kDefaultCredential case here, initialize token_credential_ to std::make_shared<Azure::Identity::DefaultAzureCredential>() if it's not initialized already.
In the kTokenCredential case you can add a DCHECK(token_credential_) as documentation that when credential_kind_ is set to kTokenCredential we expect it to be set already (something that is guaranteed by credential_kind_ being private and set only by functions that also initialize token_credential_.
What you think?
@Tom-NewtonTom-NewtonDec 22, 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 think its useful to support both anonymous and default credential as distinct things. Admittedly, its a niche use-case but storage accounts can be public, which is when anonymous is useful https://learn.microsoft.com/en-us/azure/storage/blobs/anonymous-read-access-configure?tabs=portal.
Its a good question though which should be the default. I know adlfs deafults to anonomous rather than default credential. In some respects it might be good to be consistent but also this choice of default has caused issues for me in the past e.g. flyteorg/flyte#3962
@felipecrvfelipecrvDec 22, 2023•

edited

Loading

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.

Ok. Makes sense to keep kAnonymous then. But what would you return in AzureOptions::MakeBlobServiceClient() when credential_kind_ is anonymous? Lazy-init token_credential_ with DefaultAzureCredential and create the client with it? can you create the client without any credentials?
@felipecrvfelipecrvDec 22, 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 looked and there is a constructor that doesn't take any credentials. I guess that's what we should call :-)
@Tom-NewtonTom-NewtonDec 22, 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.

👍 Do you mind if we do that in a separate PR to fix up anonymous and change the default to default credential. This was supposed to be a super simple PR until kou realised all the auths were broken 😅
@felipecrvfelipecrvDec 22, 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.

No problem.
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name_),
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name),
 token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name_),
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name),
 storage_shared_key_credential_);
 }
 return Status::Invalid("AzureOptions doesn't contain a valid auth configuration");
}

Result<std::unique_ptr<DataLake::DataLakeServiceClient>>
AzureOptions::MakeDataLakeServiceClient() const {
 if (account_name.empty()) {
 return Status::Invalid("AzureOptions doesn't contain a valid account name");
 }
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(
 AccountDfsUrl(account_name_), token_credential_);
 AccountDfsUrl(account_name), token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(
 AccountDfsUrl(account_name_), storage_shared_key_credential_);
 AccountDfsUrl(account_name), storage_shared_key_credential_);
 }
 return Status::Invalid("AzureOptions doesn't contain a valid auth configuration");
}
Expand Down

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

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -48,6 +48,9 @@ class TestAzureFileSystem;

/// Options for the AzureFileSystem implementation.
struct ARROW_EXPORT AzureOptions {
 /// \brief account name of the Azure Storage account.
 std::string account_name;

 /// \brief hostname[:port] of the Azure Blob Storage Service.
 ///
 /// If the hostname is a relative domain name (one that starts with a '.'), then storage
Expand DownExpand Up@@ -94,7 +97,6 @@ struct ARROW_EXPORT AzureOptions {
 kStorageSharedKeyCredential,
 } credential_kind_ = CredentialKind::kAnonymous;

 std::string account_name_;
 std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;
 std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
 storage_shared_key_credential_;
Expand All@@ -103,15 +105,15 @@ struct ARROW_EXPORT AzureOptions {
 AzureOptions();
 ~AzureOptions();

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

 Status ConfigureManagedIdentityCredential(const std::string& client_id = std::string());

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

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

 Status ConfigureClientSecretCredential(const std::string& account_name,
 const std::string& tenant_id,
 Status ConfigureClientSecretCredential(const std::string& tenant_id,
 const std::string& client_id,
 const std::string& client_secret);

Expand Down

34 changes: 28 additions & 6 deletions
cpp/src/arrow/filesystem/azurefs_test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -271,22 +271,44 @@ class AzureHierarchicalNSEnv : public AzureEnvImpl<AzureHierarchicalNSEnv> {
 bool WithHierarchicalNamespace() const final { return true; }
};

TEST(AzureFileSystem, InitializingFilesystemWithoutAccountNameFails) {
 AzureOptions options;
 ASSERT_RAISES(Invalid, options.ConfigureAccountKeyCredential("account_key"));

 ARROW_EXPECT_OK(
 options.ConfigureClientSecretCredential("tenant_id", "client_id", "client_secret"));
 ASSERT_RAISES(Invalid, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithClientSecretCredential) {
 AzureOptions options;
 ARROW_EXPECT_OK(options.ConfigureClientSecretCredential(
 "dummy-account-name", "tenant_id", "client_id", "client_secret"));
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(
 options.ConfigureClientSecretCredential("tenant_id", "client_id", "client_secret"));
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithDefaultCredential) {
 AzureOptions options;
 ARROW_EXPECT_OK(options.ConfigureDefaultCredential("dummy-account-name"));
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(options.ConfigureDefaultCredential());
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithManagedIdentityCredential) {
 AzureOptions options;
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(options.ConfigureManagedIdentityCredential());
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));

 ARROW_EXPECT_OK(options.ConfigureManagedIdentityCredential("specific-client-id"));
 EXPECT_OK_AND_ASSIGN(fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithWorkloadIdentityCredential) {
 AzureOptions options;
 ARROW_EXPECT_OK(options.ConfigureWorkloadIdentityCredential("dummy-account-name"));
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(options.ConfigureWorkloadIdentityCredential());
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

Expand DownExpand Up@@ -383,6 +405,7 @@ class TestAzureFileSystem : public ::testing::Test {

 static Result<AzureOptions> MakeOptions(BaseAzureEnv* env) {
 AzureOptions options;
 options.account_name = env->account_name();
 switch (env->backend()) {
 case AzureBackend::kAzurite:
 options.blob_storage_authority = "127.0.0.1:10000";
Expand All@@ -394,8 +417,7 @@ class TestAzureFileSystem : public ::testing::Test {
 // Use the default values
 break;
 }
 ARROW_EXPECT_OK(
 options.ConfigureAccountKeyCredential(env->account_name(), env->account_key()));
 ARROW_EXPECT_OK(options.ConfigureAccountKeyCredential(env->account_key()));
 return options;
 }

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-39320: [C++][FS][Azure] Add managed identity auth configuration#39321

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

felipecrv
merged 8 commits into

from

Dec 23, 2023
 Merged


GH-39320: [C++][FS][Azure] Add managed identity auth configuration
#39321
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


8 commits


Select commit
Hold shift + click to select a range
4bfacdd

Add managed identity


Tom-Newton Dec 20, 2023
f6e40aa

Ensure filesystem initialisation fails without account name


Tom-Newton Dec 22, 2023
d4af840

Fix broken auths


Tom-Newton Dec 22, 2023
3a9b89f

style fix


Tom-Newton Dec 22, 2023
414f582

another code style fix


Tom-Newton Dec 22, 2023
de0a869

Move account_name to AzureOptions constructor


Tom-Newton Dec 22, 2023
8cdcf4b

Revert AzureOptions constructor. Just make account_name a public member.


Tom-Newton Dec 22, 2023
bfa2cc5

Add a test asserting initialisation fails without an account name


Tom-Newton Dec 22, 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


38 changes: 25 additions & 13 deletions
cpp/src/arrow/filesystem/azurefs.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -58,7 +58,7 @@ bool AzureOptions::Equals(const AzureOptions& other) const {
 blob_storage_scheme == other.blob_storage_scheme &&
 dfs_storage_scheme == other.dfs_storage_scheme &&
 default_metadata == other.default_metadata &&
account_name_ == other.account_name_ &&
account_name == other.account_name &&
 credential_kind_ == other.credential_kind_;
 if (!equals) {
 return false;
Expand DownExpand Up@@ -104,17 +104,17 @@ std::string AzureOptions::AccountDfsUrl(const std::string& account_name) const {
 return BuildBaseUrl(dfs_storage_scheme, dfs_storage_authority, account_name);
}

Status AzureOptions::ConfigureAccountKeyCredential(const std::string& account_name,
 const std::string& account_key) {
Status AzureOptions::ConfigureAccountKeyCredential(const std::string& account_key) {
 credential_kind_ = CredentialKind::kStorageSharedKeyCredential;
 account_name_ = account_name;
 if (account_name.empty()) {
 return Status::Invalid("AzureOptions doesn't contain a valid account name");
 }
 storage_shared_key_credential_ =
 std::make_shared<Storage::StorageSharedKeyCredential>(account_name, account_key);
 return Status::OK();
}

Status AzureOptions::ConfigureClientSecretCredential(const std::string& account_name,
 const std::string& tenant_id,
Status AzureOptions::ConfigureClientSecretCredential(const std::string& tenant_id,
 const std::string& client_id,
 const std::string& client_secret) {
 credential_kind_ = CredentialKind::kTokenCredential;
Expand All@@ -123,45 +123,57 @@ Status AzureOptions::ConfigureClientSecretCredential(const std::string& account_
 return Status::OK();
}

Status AzureOptions::ConfigureDefaultCredential(const std::string& account_name) {
Status AzureOptions::ConfigureDefaultCredential() {
 credential_kind_ = CredentialKind::kTokenCredential;
 token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();
 return Status::OK();
}

Status AzureOptions::ConfigureWorkloadIdentityCredential(
 const std::string& account_name) {
Status AzureOptions::ConfigureManagedIdentityCredential(const std::string& client_id) {
 credential_kind_ = CredentialKind::kTokenCredential;
 token_credential_ =
 std::make_shared<Azure::Identity::ManagedIdentityCredential>(client_id);
 return Status::OK();
}

Status AzureOptions::ConfigureWorkloadIdentityCredential() {
 credential_kind_ = CredentialKind::kTokenCredential;
 token_credential_ = std::make_shared<Azure::Identity::WorkloadIdentityCredential>();
 return Status::OK();
}

Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceClient()
 const {
 if (account_name.empty()) {
 return Status::Invalid("AzureOptions doesn't contain a valid account name");
 }
@felipecrvfelipecrvDec 22, 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.

Another simplification we can make:
  • Rename kAnonymous to kDefaultCredential
  • On the kDefaultCredential case here, initialize token_credential_ to std::make_shared<Azure::Identity::DefaultAzureCredential>() if it's not initialized already.
In the kTokenCredential case you can add a DCHECK(token_credential_) as documentation that when credential_kind_ is set to kTokenCredential we expect it to be set already (something that is guaranteed by credential_kind_ being private and set only by functions that also initialize token_credential_.
What you think?
@Tom-NewtonTom-NewtonDec 22, 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 think its useful to support both anonymous and default credential as distinct things. Admittedly, its a niche use-case but storage accounts can be public, which is when anonymous is useful https://learn.microsoft.com/en-us/azure/storage/blobs/anonymous-read-access-configure?tabs=portal.
Its a good question though which should be the default. I know adlfs deafults to anonomous rather than default credential. In some respects it might be good to be consistent but also this choice of default has caused issues for me in the past e.g. flyteorg/flyte#3962
@felipecrvfelipecrvDec 22, 2023•

edited

Loading

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.

Ok. Makes sense to keep kAnonymous then. But what would you return in AzureOptions::MakeBlobServiceClient() when credential_kind_ is anonymous? Lazy-init token_credential_ with DefaultAzureCredential and create the client with it? can you create the client without any credentials?
@felipecrvfelipecrvDec 22, 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 looked and there is a constructor that doesn't take any credentials. I guess that's what we should call :-)
@Tom-NewtonTom-NewtonDec 22, 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.

👍 Do you mind if we do that in a separate PR to fix up anonymous and change the default to default credential. This was supposed to be a super simple PR until kou realised all the auths were broken 😅
@felipecrvfelipecrvDec 22, 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.

No problem.
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name_),
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name),
 token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name_),
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name),
 storage_shared_key_credential_);
 }
 return Status::Invalid("AzureOptions doesn't contain a valid auth configuration");
}

Result<std::unique_ptr<DataLake::DataLakeServiceClient>>
AzureOptions::MakeDataLakeServiceClient() const {
 if (account_name.empty()) {
 return Status::Invalid("AzureOptions doesn't contain a valid account name");
 }
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(
 AccountDfsUrl(account_name_), token_credential_);
 AccountDfsUrl(account_name), token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(
 AccountDfsUrl(account_name_), storage_shared_key_credential_);
 AccountDfsUrl(account_name), storage_shared_key_credential_);
 }
 return Status::Invalid("AzureOptions doesn't contain a valid auth configuration");
}
Expand Down

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

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -48,6 +48,9 @@ class TestAzureFileSystem;

/// Options for the AzureFileSystem implementation.
struct ARROW_EXPORT AzureOptions {
 /// \brief account name of the Azure Storage account.
 std::string account_name;

 /// \brief hostname[:port] of the Azure Blob Storage Service.
 ///
 /// If the hostname is a relative domain name (one that starts with a '.'), then storage
Expand DownExpand Up@@ -94,7 +97,6 @@ struct ARROW_EXPORT AzureOptions {
 kStorageSharedKeyCredential,
 } credential_kind_ = CredentialKind::kAnonymous;

 std::string account_name_;
 std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;
 std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
 storage_shared_key_credential_;
Expand All@@ -103,15 +105,15 @@ struct ARROW_EXPORT AzureOptions {
 AzureOptions();
 ~AzureOptions();

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

 Status ConfigureManagedIdentityCredential(const std::string& client_id = std::string());

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

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

 Status ConfigureClientSecretCredential(const std::string& account_name,
 const std::string& tenant_id,
 Status ConfigureClientSecretCredential(const std::string& tenant_id,
 const std::string& client_id,
 const std::string& client_secret);

Expand Down

34 changes: 28 additions & 6 deletions
cpp/src/arrow/filesystem/azurefs_test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -271,22 +271,44 @@ class AzureHierarchicalNSEnv : public AzureEnvImpl<AzureHierarchicalNSEnv> {
 bool WithHierarchicalNamespace() const final { return true; }
};

TEST(AzureFileSystem, InitializingFilesystemWithoutAccountNameFails) {
 AzureOptions options;
 ASSERT_RAISES(Invalid, options.ConfigureAccountKeyCredential("account_key"));

 ARROW_EXPECT_OK(
 options.ConfigureClientSecretCredential("tenant_id", "client_id", "client_secret"));
 ASSERT_RAISES(Invalid, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithClientSecretCredential) {
 AzureOptions options;
 ARROW_EXPECT_OK(options.ConfigureClientSecretCredential(
 "dummy-account-name", "tenant_id", "client_id", "client_secret"));
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(
 options.ConfigureClientSecretCredential("tenant_id", "client_id", "client_secret"));
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithDefaultCredential) {
 AzureOptions options;
 ARROW_EXPECT_OK(options.ConfigureDefaultCredential("dummy-account-name"));
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(options.ConfigureDefaultCredential());
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithManagedIdentityCredential) {
 AzureOptions options;
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(options.ConfigureManagedIdentityCredential());
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));

 ARROW_EXPECT_OK(options.ConfigureManagedIdentityCredential("specific-client-id"));
 EXPECT_OK_AND_ASSIGN(fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithWorkloadIdentityCredential) {
 AzureOptions options;
 ARROW_EXPECT_OK(options.ConfigureWorkloadIdentityCredential("dummy-account-name"));
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(options.ConfigureWorkloadIdentityCredential());
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

Expand DownExpand Up@@ -383,6 +405,7 @@ class TestAzureFileSystem : public ::testing::Test {

 static Result<AzureOptions> MakeOptions(BaseAzureEnv* env) {
 AzureOptions options;
 options.account_name = env->account_name();
 switch (env->backend()) {
 case AzureBackend::kAzurite:
 options.blob_storage_authority = "127.0.0.1:10000";
Expand All@@ -394,8 +417,7 @@ class TestAzureFileSystem : public ::testing::Test {
 // Use the default values
 break;
 }
 ARROW_EXPECT_OK(
 options.ConfigureAccountKeyCredential(env->account_name(), env->account_key()));
 ARROW_EXPECT_OK(options.ConfigureAccountKeyCredential(env->account_key()));
 return options;
 }

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-39320: [C++][FS][Azure] Add managed identity auth configuration#39321

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

felipecrv
merged 8 commits into

from

Dec 23, 2023
 Merged


GH-39320: [C++][FS][Azure] Add managed identity auth configuration
#39321
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


8 commits


Select commit
Hold shift + click to select a range
4bfacdd

Add managed identity


Tom-Newton Dec 20, 2023
f6e40aa

Ensure filesystem initialisation fails without account name


Tom-Newton Dec 22, 2023
d4af840

Fix broken auths


Tom-Newton Dec 22, 2023
3a9b89f

style fix


Tom-Newton Dec 22, 2023
414f582

another code style fix


Tom-Newton Dec 22, 2023
de0a869

Move account_name to AzureOptions constructor


Tom-Newton Dec 22, 2023
8cdcf4b

Revert AzureOptions constructor. Just make account_name a public member.


Tom-Newton Dec 22, 2023
bfa2cc5

Add a test asserting initialisation fails without an account name


Tom-Newton Dec 22, 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


38 changes: 25 additions & 13 deletions
cpp/src/arrow/filesystem/azurefs.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -58,7 +58,7 @@ bool AzureOptions::Equals(const AzureOptions& other) const {
 blob_storage_scheme == other.blob_storage_scheme &&
 dfs_storage_scheme == other.dfs_storage_scheme &&
 default_metadata == other.default_metadata &&
account_name_ == other.account_name_ &&
account_name == other.account_name &&
 credential_kind_ == other.credential_kind_;
 if (!equals) {
 return false;
Expand DownExpand Up@@ -104,17 +104,17 @@ std::string AzureOptions::AccountDfsUrl(const std::string& account_name) const {
 return BuildBaseUrl(dfs_storage_scheme, dfs_storage_authority, account_name);
}

Status AzureOptions::ConfigureAccountKeyCredential(const std::string& account_name,
 const std::string& account_key) {
Status AzureOptions::ConfigureAccountKeyCredential(const std::string& account_key) {
 credential_kind_ = CredentialKind::kStorageSharedKeyCredential;
 account_name_ = account_name;
 if (account_name.empty()) {
 return Status::Invalid("AzureOptions doesn't contain a valid account name");
 }
 storage_shared_key_credential_ =
 std::make_shared<Storage::StorageSharedKeyCredential>(account_name, account_key);
 return Status::OK();
}

Status AzureOptions::ConfigureClientSecretCredential(const std::string& account_name,
 const std::string& tenant_id,
Status AzureOptions::ConfigureClientSecretCredential(const std::string& tenant_id,
 const std::string& client_id,
 const std::string& client_secret) {
 credential_kind_ = CredentialKind::kTokenCredential;
Expand All@@ -123,45 +123,57 @@ Status AzureOptions::ConfigureClientSecretCredential(const std::string& account_
 return Status::OK();
}

Status AzureOptions::ConfigureDefaultCredential(const std::string& account_name) {
Status AzureOptions::ConfigureDefaultCredential() {
 credential_kind_ = CredentialKind::kTokenCredential;
 token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();
 return Status::OK();
}

Status AzureOptions::ConfigureWorkloadIdentityCredential(
 const std::string& account_name) {
Status AzureOptions::ConfigureManagedIdentityCredential(const std::string& client_id) {
 credential_kind_ = CredentialKind::kTokenCredential;
 token_credential_ =
 std::make_shared<Azure::Identity::ManagedIdentityCredential>(client_id);
 return Status::OK();
}

Status AzureOptions::ConfigureWorkloadIdentityCredential() {
 credential_kind_ = CredentialKind::kTokenCredential;
 token_credential_ = std::make_shared<Azure::Identity::WorkloadIdentityCredential>();
 return Status::OK();
}

Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceClient()
 const {
 if (account_name.empty()) {
 return Status::Invalid("AzureOptions doesn't contain a valid account name");
 }
@felipecrvfelipecrvDec 22, 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.

Another simplification we can make:
  • Rename kAnonymous to kDefaultCredential
  • On the kDefaultCredential case here, initialize token_credential_ to std::make_shared<Azure::Identity::DefaultAzureCredential>() if it's not initialized already.
In the kTokenCredential case you can add a DCHECK(token_credential_) as documentation that when credential_kind_ is set to kTokenCredential we expect it to be set already (something that is guaranteed by credential_kind_ being private and set only by functions that also initialize token_credential_.
What you think?
@Tom-NewtonTom-NewtonDec 22, 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 think its useful to support both anonymous and default credential as distinct things. Admittedly, its a niche use-case but storage accounts can be public, which is when anonymous is useful https://learn.microsoft.com/en-us/azure/storage/blobs/anonymous-read-access-configure?tabs=portal.
Its a good question though which should be the default. I know adlfs deafults to anonomous rather than default credential. In some respects it might be good to be consistent but also this choice of default has caused issues for me in the past e.g. flyteorg/flyte#3962
@felipecrvfelipecrvDec 22, 2023•

edited

Loading

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.

Ok. Makes sense to keep kAnonymous then. But what would you return in AzureOptions::MakeBlobServiceClient() when credential_kind_ is anonymous? Lazy-init token_credential_ with DefaultAzureCredential and create the client with it? can you create the client without any credentials?
@felipecrvfelipecrvDec 22, 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 looked and there is a constructor that doesn't take any credentials. I guess that's what we should call :-)
@Tom-NewtonTom-NewtonDec 22, 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.

👍 Do you mind if we do that in a separate PR to fix up anonymous and change the default to default credential. This was supposed to be a super simple PR until kou realised all the auths were broken 😅
@felipecrvfelipecrvDec 22, 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.

No problem.
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name_),
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name),
 token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name_),
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name),
 storage_shared_key_credential_);
 }
 return Status::Invalid("AzureOptions doesn't contain a valid auth configuration");
}

Result<std::unique_ptr<DataLake::DataLakeServiceClient>>
AzureOptions::MakeDataLakeServiceClient() const {
 if (account_name.empty()) {
 return Status::Invalid("AzureOptions doesn't contain a valid account name");
 }
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(
 AccountDfsUrl(account_name_), token_credential_);
 AccountDfsUrl(account_name), token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(
 AccountDfsUrl(account_name_), storage_shared_key_credential_);
 AccountDfsUrl(account_name), storage_shared_key_credential_);
 }
 return Status::Invalid("AzureOptions doesn't contain a valid auth configuration");
}
Expand Down

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

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -48,6 +48,9 @@ class TestAzureFileSystem;

/// Options for the AzureFileSystem implementation.
struct ARROW_EXPORT AzureOptions {
 /// \brief account name of the Azure Storage account.
 std::string account_name;

 /// \brief hostname[:port] of the Azure Blob Storage Service.
 ///
 /// If the hostname is a relative domain name (one that starts with a '.'), then storage
Expand DownExpand Up@@ -94,7 +97,6 @@ struct ARROW_EXPORT AzureOptions {
 kStorageSharedKeyCredential,
 } credential_kind_ = CredentialKind::kAnonymous;

 std::string account_name_;
 std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;
 std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
 storage_shared_key_credential_;
Expand All@@ -103,15 +105,15 @@ struct ARROW_EXPORT AzureOptions {
 AzureOptions();
 ~AzureOptions();

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

 Status ConfigureManagedIdentityCredential(const std::string& client_id = std::string());

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

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

 Status ConfigureClientSecretCredential(const std::string& account_name,
 const std::string& tenant_id,
 Status ConfigureClientSecretCredential(const std::string& tenant_id,
 const std::string& client_id,
 const std::string& client_secret);

Expand Down

34 changes: 28 additions & 6 deletions
cpp/src/arrow/filesystem/azurefs_test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -271,22 +271,44 @@ class AzureHierarchicalNSEnv : public AzureEnvImpl<AzureHierarchicalNSEnv> {
 bool WithHierarchicalNamespace() const final { return true; }
};

TEST(AzureFileSystem, InitializingFilesystemWithoutAccountNameFails) {
 AzureOptions options;
 ASSERT_RAISES(Invalid, options.ConfigureAccountKeyCredential("account_key"));

 ARROW_EXPECT_OK(
 options.ConfigureClientSecretCredential("tenant_id", "client_id", "client_secret"));
 ASSERT_RAISES(Invalid, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithClientSecretCredential) {
 AzureOptions options;
 ARROW_EXPECT_OK(options.ConfigureClientSecretCredential(
 "dummy-account-name", "tenant_id", "client_id", "client_secret"));
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(
 options.ConfigureClientSecretCredential("tenant_id", "client_id", "client_secret"));
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithDefaultCredential) {
 AzureOptions options;
 ARROW_EXPECT_OK(options.ConfigureDefaultCredential("dummy-account-name"));
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(options.ConfigureDefaultCredential());
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithManagedIdentityCredential) {
 AzureOptions options;
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(options.ConfigureManagedIdentityCredential());
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));

 ARROW_EXPECT_OK(options.ConfigureManagedIdentityCredential("specific-client-id"));
 EXPECT_OK_AND_ASSIGN(fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithWorkloadIdentityCredential) {
 AzureOptions options;
 ARROW_EXPECT_OK(options.ConfigureWorkloadIdentityCredential("dummy-account-name"));
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(options.ConfigureWorkloadIdentityCredential());
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

Expand DownExpand Up@@ -383,6 +405,7 @@ class TestAzureFileSystem : public ::testing::Test {

 static Result<AzureOptions> MakeOptions(BaseAzureEnv* env) {
 AzureOptions options;
 options.account_name = env->account_name();
 switch (env->backend()) {
 case AzureBackend::kAzurite:
 options.blob_storage_authority = "127.0.0.1:10000";
Expand All@@ -394,8 +417,7 @@ class TestAzureFileSystem : public ::testing::Test {
 // Use the default values
 break;
 }
 ARROW_EXPECT_OK(
 options.ConfigureAccountKeyCredential(env->account_name(), env->account_key()));
 ARROW_EXPECT_OK(options.ConfigureAccountKeyCredential(env->account_key()));
 return options;
 }

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-39320: [C++][FS][Azure] Add managed identity auth configuration#39321

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

felipecrv
merged 8 commits into

from

Dec 23, 2023
 Merged


GH-39320: [C++][FS][Azure] Add managed identity auth configuration
#39321
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


8 commits


Select commit
Hold shift + click to select a range
4bfacdd

Add managed identity


Tom-Newton Dec 20, 2023
f6e40aa

Ensure filesystem initialisation fails without account name


Tom-Newton Dec 22, 2023
d4af840

Fix broken auths


Tom-Newton Dec 22, 2023
3a9b89f

style fix


Tom-Newton Dec 22, 2023
414f582

another code style fix


Tom-Newton Dec 22, 2023
de0a869

Move account_name to AzureOptions constructor


Tom-Newton Dec 22, 2023
8cdcf4b

Revert AzureOptions constructor. Just make account_name a public member.


Tom-Newton Dec 22, 2023
bfa2cc5

Add a test asserting initialisation fails without an account name


Tom-Newton Dec 22, 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


38 changes: 25 additions & 13 deletions
cpp/src/arrow/filesystem/azurefs.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -58,7 +58,7 @@ bool AzureOptions::Equals(const AzureOptions& other) const {
 blob_storage_scheme == other.blob_storage_scheme &&
 dfs_storage_scheme == other.dfs_storage_scheme &&
 default_metadata == other.default_metadata &&
account_name_ == other.account_name_ &&
account_name == other.account_name &&
 credential_kind_ == other.credential_kind_;
 if (!equals) {
 return false;
Expand DownExpand Up@@ -104,17 +104,17 @@ std::string AzureOptions::AccountDfsUrl(const std::string& account_name) const {
 return BuildBaseUrl(dfs_storage_scheme, dfs_storage_authority, account_name);
}

Status AzureOptions::ConfigureAccountKeyCredential(const std::string& account_name,
 const std::string& account_key) {
Status AzureOptions::ConfigureAccountKeyCredential(const std::string& account_key) {
 credential_kind_ = CredentialKind::kStorageSharedKeyCredential;
 account_name_ = account_name;
 if (account_name.empty()) {
 return Status::Invalid("AzureOptions doesn't contain a valid account name");
 }
 storage_shared_key_credential_ =
 std::make_shared<Storage::StorageSharedKeyCredential>(account_name, account_key);
 return Status::OK();
}

Status AzureOptions::ConfigureClientSecretCredential(const std::string& account_name,
 const std::string& tenant_id,
Status AzureOptions::ConfigureClientSecretCredential(const std::string& tenant_id,
 const std::string& client_id,
 const std::string& client_secret) {
 credential_kind_ = CredentialKind::kTokenCredential;
Expand All@@ -123,45 +123,57 @@ Status AzureOptions::ConfigureClientSecretCredential(const std::string& account_
 return Status::OK();
}

Status AzureOptions::ConfigureDefaultCredential(const std::string& account_name) {
Status AzureOptions::ConfigureDefaultCredential() {
 credential_kind_ = CredentialKind::kTokenCredential;
 token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();
 return Status::OK();
}

Status AzureOptions::ConfigureWorkloadIdentityCredential(
 const std::string& account_name) {
Status AzureOptions::ConfigureManagedIdentityCredential(const std::string& client_id) {
 credential_kind_ = CredentialKind::kTokenCredential;
 token_credential_ =
 std::make_shared<Azure::Identity::ManagedIdentityCredential>(client_id);
 return Status::OK();
}

Status AzureOptions::ConfigureWorkloadIdentityCredential() {
 credential_kind_ = CredentialKind::kTokenCredential;
 token_credential_ = std::make_shared<Azure::Identity::WorkloadIdentityCredential>();
 return Status::OK();
}

Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceClient()
 const {
 if (account_name.empty()) {
 return Status::Invalid("AzureOptions doesn't contain a valid account name");
 }
@felipecrvfelipecrvDec 22, 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.

Another simplification we can make:
  • Rename kAnonymous to kDefaultCredential
  • On the kDefaultCredential case here, initialize token_credential_ to std::make_shared<Azure::Identity::DefaultAzureCredential>() if it's not initialized already.
In the kTokenCredential case you can add a DCHECK(token_credential_) as documentation that when credential_kind_ is set to kTokenCredential we expect it to be set already (something that is guaranteed by credential_kind_ being private and set only by functions that also initialize token_credential_.
What you think?
@Tom-NewtonTom-NewtonDec 22, 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 think its useful to support both anonymous and default credential as distinct things. Admittedly, its a niche use-case but storage accounts can be public, which is when anonymous is useful https://learn.microsoft.com/en-us/azure/storage/blobs/anonymous-read-access-configure?tabs=portal.
Its a good question though which should be the default. I know adlfs deafults to anonomous rather than default credential. In some respects it might be good to be consistent but also this choice of default has caused issues for me in the past e.g. flyteorg/flyte#3962
@felipecrvfelipecrvDec 22, 2023•

edited

Loading

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.

Ok. Makes sense to keep kAnonymous then. But what would you return in AzureOptions::MakeBlobServiceClient() when credential_kind_ is anonymous? Lazy-init token_credential_ with DefaultAzureCredential and create the client with it? can you create the client without any credentials?
@felipecrvfelipecrvDec 22, 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 looked and there is a constructor that doesn't take any credentials. I guess that's what we should call :-)
@Tom-NewtonTom-NewtonDec 22, 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.

👍 Do you mind if we do that in a separate PR to fix up anonymous and change the default to default credential. This was supposed to be a super simple PR until kou realised all the auths were broken 😅
@felipecrvfelipecrvDec 22, 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.

No problem.
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name_),
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name),
 token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name_),
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name),
 storage_shared_key_credential_);
 }
 return Status::Invalid("AzureOptions doesn't contain a valid auth configuration");
}

Result<std::unique_ptr<DataLake::DataLakeServiceClient>>
AzureOptions::MakeDataLakeServiceClient() const {
 if (account_name.empty()) {
 return Status::Invalid("AzureOptions doesn't contain a valid account name");
 }
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(
 AccountDfsUrl(account_name_), token_credential_);
 AccountDfsUrl(account_name), token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(
 AccountDfsUrl(account_name_), storage_shared_key_credential_);
 AccountDfsUrl(account_name), storage_shared_key_credential_);
 }
 return Status::Invalid("AzureOptions doesn't contain a valid auth configuration");
}
Expand Down

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

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -48,6 +48,9 @@ class TestAzureFileSystem;

/// Options for the AzureFileSystem implementation.
struct ARROW_EXPORT AzureOptions {
 /// \brief account name of the Azure Storage account.
 std::string account_name;

 /// \brief hostname[:port] of the Azure Blob Storage Service.
 ///
 /// If the hostname is a relative domain name (one that starts with a '.'), then storage
Expand DownExpand Up@@ -94,7 +97,6 @@ struct ARROW_EXPORT AzureOptions {
 kStorageSharedKeyCredential,
 } credential_kind_ = CredentialKind::kAnonymous;

 std::string account_name_;
 std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;
 std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
 storage_shared_key_credential_;
Expand All@@ -103,15 +105,15 @@ struct ARROW_EXPORT AzureOptions {
 AzureOptions();
 ~AzureOptions();

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

 Status ConfigureManagedIdentityCredential(const std::string& client_id = std::string());

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

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

 Status ConfigureClientSecretCredential(const std::string& account_name,
 const std::string& tenant_id,
 Status ConfigureClientSecretCredential(const std::string& tenant_id,
 const std::string& client_id,
 const std::string& client_secret);

Expand Down

34 changes: 28 additions & 6 deletions
cpp/src/arrow/filesystem/azurefs_test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -271,22 +271,44 @@ class AzureHierarchicalNSEnv : public AzureEnvImpl<AzureHierarchicalNSEnv> {
 bool WithHierarchicalNamespace() const final { return true; }
};

TEST(AzureFileSystem, InitializingFilesystemWithoutAccountNameFails) {
 AzureOptions options;
 ASSERT_RAISES(Invalid, options.ConfigureAccountKeyCredential("account_key"));

 ARROW_EXPECT_OK(
 options.ConfigureClientSecretCredential("tenant_id", "client_id", "client_secret"));
 ASSERT_RAISES(Invalid, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithClientSecretCredential) {
 AzureOptions options;
 ARROW_EXPECT_OK(options.ConfigureClientSecretCredential(
 "dummy-account-name", "tenant_id", "client_id", "client_secret"));
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(
 options.ConfigureClientSecretCredential("tenant_id", "client_id", "client_secret"));
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithDefaultCredential) {
 AzureOptions options;
 ARROW_EXPECT_OK(options.ConfigureDefaultCredential("dummy-account-name"));
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(options.ConfigureDefaultCredential());
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithManagedIdentityCredential) {
 AzureOptions options;
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(options.ConfigureManagedIdentityCredential());
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));

 ARROW_EXPECT_OK(options.ConfigureManagedIdentityCredential("specific-client-id"));
 EXPECT_OK_AND_ASSIGN(fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithWorkloadIdentityCredential) {
 AzureOptions options;
 ARROW_EXPECT_OK(options.ConfigureWorkloadIdentityCredential("dummy-account-name"));
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(options.ConfigureWorkloadIdentityCredential());
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

Expand DownExpand Up@@ -383,6 +405,7 @@ class TestAzureFileSystem : public ::testing::Test {

 static Result<AzureOptions> MakeOptions(BaseAzureEnv* env) {
 AzureOptions options;
 options.account_name = env->account_name();
 switch (env->backend()) {
 case AzureBackend::kAzurite:
 options.blob_storage_authority = "127.0.0.1:10000";
Expand All@@ -394,8 +417,7 @@ class TestAzureFileSystem : public ::testing::Test {
 // Use the default values
 break;
 }
 ARROW_EXPECT_OK(
 options.ConfigureAccountKeyCredential(env->account_name(), env->account_key()));
 ARROW_EXPECT_OK(options.ConfigureAccountKeyCredential(env->account_key()));
 return options;
 }

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-39320: [C++][FS][Azure] Add managed identity auth configuration#39321

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

felipecrv
merged 8 commits into

from

Dec 23, 2023
 Merged


GH-39320: [C++][FS][Azure] Add managed identity auth configuration
#39321
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


8 commits


Select commit
Hold shift + click to select a range
4bfacdd

Add managed identity


Tom-Newton Dec 20, 2023
f6e40aa

Ensure filesystem initialisation fails without account name


Tom-Newton Dec 22, 2023
d4af840

Fix broken auths


Tom-Newton Dec 22, 2023
3a9b89f

style fix


Tom-Newton Dec 22, 2023
414f582

another code style fix


Tom-Newton Dec 22, 2023
de0a869

Move account_name to AzureOptions constructor


Tom-Newton Dec 22, 2023
8cdcf4b

Revert AzureOptions constructor. Just make account_name a public member.


Tom-Newton Dec 22, 2023
bfa2cc5

Add a test asserting initialisation fails without an account name


Tom-Newton Dec 22, 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


38 changes: 25 additions & 13 deletions
cpp/src/arrow/filesystem/azurefs.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -58,7 +58,7 @@ bool AzureOptions::Equals(const AzureOptions& other) const {
 blob_storage_scheme == other.blob_storage_scheme &&
 dfs_storage_scheme == other.dfs_storage_scheme &&
 default_metadata == other.default_metadata &&
account_name_ == other.account_name_ &&
account_name == other.account_name &&
 credential_kind_ == other.credential_kind_;
 if (!equals) {
 return false;
Expand DownExpand Up@@ -104,17 +104,17 @@ std::string AzureOptions::AccountDfsUrl(const std::string& account_name) const {
 return BuildBaseUrl(dfs_storage_scheme, dfs_storage_authority, account_name);
}

Status AzureOptions::ConfigureAccountKeyCredential(const std::string& account_name,
 const std::string& account_key) {
Status AzureOptions::ConfigureAccountKeyCredential(const std::string& account_key) {
 credential_kind_ = CredentialKind::kStorageSharedKeyCredential;
 account_name_ = account_name;
 if (account_name.empty()) {
 return Status::Invalid("AzureOptions doesn't contain a valid account name");
 }
 storage_shared_key_credential_ =
 std::make_shared<Storage::StorageSharedKeyCredential>(account_name, account_key);
 return Status::OK();
}

Status AzureOptions::ConfigureClientSecretCredential(const std::string& account_name,
 const std::string& tenant_id,
Status AzureOptions::ConfigureClientSecretCredential(const std::string& tenant_id,
 const std::string& client_id,
 const std::string& client_secret) {
 credential_kind_ = CredentialKind::kTokenCredential;
Expand All@@ -123,45 +123,57 @@ Status AzureOptions::ConfigureClientSecretCredential(const std::string& account_
 return Status::OK();
}

Status AzureOptions::ConfigureDefaultCredential(const std::string& account_name) {
Status AzureOptions::ConfigureDefaultCredential() {
 credential_kind_ = CredentialKind::kTokenCredential;
 token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();
 return Status::OK();
}

Status AzureOptions::ConfigureWorkloadIdentityCredential(
 const std::string& account_name) {
Status AzureOptions::ConfigureManagedIdentityCredential(const std::string& client_id) {
 credential_kind_ = CredentialKind::kTokenCredential;
 token_credential_ =
 std::make_shared<Azure::Identity::ManagedIdentityCredential>(client_id);
 return Status::OK();
}

Status AzureOptions::ConfigureWorkloadIdentityCredential() {
 credential_kind_ = CredentialKind::kTokenCredential;
 token_credential_ = std::make_shared<Azure::Identity::WorkloadIdentityCredential>();
 return Status::OK();
}

Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceClient()
 const {
 if (account_name.empty()) {
 return Status::Invalid("AzureOptions doesn't contain a valid account name");
 }
@felipecrvfelipecrvDec 22, 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.

Another simplification we can make:
  • Rename kAnonymous to kDefaultCredential
  • On the kDefaultCredential case here, initialize token_credential_ to std::make_shared<Azure::Identity::DefaultAzureCredential>() if it's not initialized already.
In the kTokenCredential case you can add a DCHECK(token_credential_) as documentation that when credential_kind_ is set to kTokenCredential we expect it to be set already (something that is guaranteed by credential_kind_ being private and set only by functions that also initialize token_credential_.
What you think?
@Tom-NewtonTom-NewtonDec 22, 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 think its useful to support both anonymous and default credential as distinct things. Admittedly, its a niche use-case but storage accounts can be public, which is when anonymous is useful https://learn.microsoft.com/en-us/azure/storage/blobs/anonymous-read-access-configure?tabs=portal.
Its a good question though which should be the default. I know adlfs deafults to anonomous rather than default credential. In some respects it might be good to be consistent but also this choice of default has caused issues for me in the past e.g. flyteorg/flyte#3962
@felipecrvfelipecrvDec 22, 2023•

edited

Loading

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.

Ok. Makes sense to keep kAnonymous then. But what would you return in AzureOptions::MakeBlobServiceClient() when credential_kind_ is anonymous? Lazy-init token_credential_ with DefaultAzureCredential and create the client with it? can you create the client without any credentials?
@felipecrvfelipecrvDec 22, 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 looked and there is a constructor that doesn't take any credentials. I guess that's what we should call :-)
@Tom-NewtonTom-NewtonDec 22, 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.

👍 Do you mind if we do that in a separate PR to fix up anonymous and change the default to default credential. This was supposed to be a super simple PR until kou realised all the auths were broken 😅
@felipecrvfelipecrvDec 22, 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.

No problem.
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name_),
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name),
 token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name_),
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name),
 storage_shared_key_credential_);
 }
 return Status::Invalid("AzureOptions doesn't contain a valid auth configuration");
}

Result<std::unique_ptr<DataLake::DataLakeServiceClient>>
AzureOptions::MakeDataLakeServiceClient() const {
 if (account_name.empty()) {
 return Status::Invalid("AzureOptions doesn't contain a valid account name");
 }
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(
 AccountDfsUrl(account_name_), token_credential_);
 AccountDfsUrl(account_name), token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(
 AccountDfsUrl(account_name_), storage_shared_key_credential_);
 AccountDfsUrl(account_name), storage_shared_key_credential_);
 }
 return Status::Invalid("AzureOptions doesn't contain a valid auth configuration");
}
Expand Down

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

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -48,6 +48,9 @@ class TestAzureFileSystem;

/// Options for the AzureFileSystem implementation.
struct ARROW_EXPORT AzureOptions {
 /// \brief account name of the Azure Storage account.
 std::string account_name;

 /// \brief hostname[:port] of the Azure Blob Storage Service.
 ///
 /// If the hostname is a relative domain name (one that starts with a '.'), then storage
Expand DownExpand Up@@ -94,7 +97,6 @@ struct ARROW_EXPORT AzureOptions {
 kStorageSharedKeyCredential,
 } credential_kind_ = CredentialKind::kAnonymous;

 std::string account_name_;
 std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;
 std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
 storage_shared_key_credential_;
Expand All@@ -103,15 +105,15 @@ struct ARROW_EXPORT AzureOptions {
 AzureOptions();
 ~AzureOptions();

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

 Status ConfigureManagedIdentityCredential(const std::string& client_id = std::string());

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

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

 Status ConfigureClientSecretCredential(const std::string& account_name,
 const std::string& tenant_id,
 Status ConfigureClientSecretCredential(const std::string& tenant_id,
 const std::string& client_id,
 const std::string& client_secret);

Expand Down

34 changes: 28 additions & 6 deletions
cpp/src/arrow/filesystem/azurefs_test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -271,22 +271,44 @@ class AzureHierarchicalNSEnv : public AzureEnvImpl<AzureHierarchicalNSEnv> {
 bool WithHierarchicalNamespace() const final { return true; }
};

TEST(AzureFileSystem, InitializingFilesystemWithoutAccountNameFails) {
 AzureOptions options;
 ASSERT_RAISES(Invalid, options.ConfigureAccountKeyCredential("account_key"));

 ARROW_EXPECT_OK(
 options.ConfigureClientSecretCredential("tenant_id", "client_id", "client_secret"));
 ASSERT_RAISES(Invalid, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithClientSecretCredential) {
 AzureOptions options;
 ARROW_EXPECT_OK(options.ConfigureClientSecretCredential(
 "dummy-account-name", "tenant_id", "client_id", "client_secret"));
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(
 options.ConfigureClientSecretCredential("tenant_id", "client_id", "client_secret"));
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithDefaultCredential) {
 AzureOptions options;
 ARROW_EXPECT_OK(options.ConfigureDefaultCredential("dummy-account-name"));
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(options.ConfigureDefaultCredential());
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithManagedIdentityCredential) {
 AzureOptions options;
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(options.ConfigureManagedIdentityCredential());
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));

 ARROW_EXPECT_OK(options.ConfigureManagedIdentityCredential("specific-client-id"));
 EXPECT_OK_AND_ASSIGN(fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithWorkloadIdentityCredential) {
 AzureOptions options;
 ARROW_EXPECT_OK(options.ConfigureWorkloadIdentityCredential("dummy-account-name"));
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(options.ConfigureWorkloadIdentityCredential());
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

Expand DownExpand Up@@ -383,6 +405,7 @@ class TestAzureFileSystem : public ::testing::Test {

 static Result<AzureOptions> MakeOptions(BaseAzureEnv* env) {
 AzureOptions options;
 options.account_name = env->account_name();
 switch (env->backend()) {
 case AzureBackend::kAzurite:
 options.blob_storage_authority = "127.0.0.1:10000";
Expand All@@ -394,8 +417,7 @@ class TestAzureFileSystem : public ::testing::Test {
 // Use the default values
 break;
 }
 ARROW_EXPECT_OK(
 options.ConfigureAccountKeyCredential(env->account_name(), env->account_key()));
 ARROW_EXPECT_OK(options.ConfigureAccountKeyCredential(env->account_key()));
 return options;
 }

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-39320: [C++][FS][Azure] Add managed identity auth configuration#39321

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

felipecrv
merged 8 commits into

from

Dec 23, 2023
 Merged


GH-39320: [C++][FS][Azure] Add managed identity auth configuration
#39321
Show file tree
Hide file tree
 Changes from all commits
Commits

Show all changes


8 commits


Select commit
Hold shift + click to select a range
4bfacdd

Add managed identity


Tom-Newton Dec 20, 2023
f6e40aa

Ensure filesystem initialisation fails without account name


Tom-Newton Dec 22, 2023
d4af840

Fix broken auths


Tom-Newton Dec 22, 2023
3a9b89f

style fix


Tom-Newton Dec 22, 2023
414f582

another code style fix


Tom-Newton Dec 22, 2023
de0a869

Move account_name to AzureOptions constructor


Tom-Newton Dec 22, 2023
8cdcf4b

Revert AzureOptions constructor. Just make account_name a public member.


Tom-Newton Dec 22, 2023
bfa2cc5

Add a test asserting initialisation fails without an account name


Tom-Newton Dec 22, 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


38 changes: 25 additions & 13 deletions
cpp/src/arrow/filesystem/azurefs.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -58,7 +58,7 @@ bool AzureOptions::Equals(const AzureOptions& other) const {
 blob_storage_scheme == other.blob_storage_scheme &&
 dfs_storage_scheme == other.dfs_storage_scheme &&
 default_metadata == other.default_metadata &&
account_name_ == other.account_name_ &&
account_name == other.account_name &&
 credential_kind_ == other.credential_kind_;
 if (!equals) {
 return false;
Expand DownExpand Up@@ -104,17 +104,17 @@ std::string AzureOptions::AccountDfsUrl(const std::string& account_name) const {
 return BuildBaseUrl(dfs_storage_scheme, dfs_storage_authority, account_name);
}

Status AzureOptions::ConfigureAccountKeyCredential(const std::string& account_name,
 const std::string& account_key) {
Status AzureOptions::ConfigureAccountKeyCredential(const std::string& account_key) {
 credential_kind_ = CredentialKind::kStorageSharedKeyCredential;
 account_name_ = account_name;
 if (account_name.empty()) {
 return Status::Invalid("AzureOptions doesn't contain a valid account name");
 }
 storage_shared_key_credential_ =
 std::make_shared<Storage::StorageSharedKeyCredential>(account_name, account_key);
 return Status::OK();
}

Status AzureOptions::ConfigureClientSecretCredential(const std::string& account_name,
 const std::string& tenant_id,
Status AzureOptions::ConfigureClientSecretCredential(const std::string& tenant_id,
 const std::string& client_id,
 const std::string& client_secret) {
 credential_kind_ = CredentialKind::kTokenCredential;
Expand All@@ -123,45 +123,57 @@ Status AzureOptions::ConfigureClientSecretCredential(const std::string& account_
 return Status::OK();
}

Status AzureOptions::ConfigureDefaultCredential(const std::string& account_name) {
Status AzureOptions::ConfigureDefaultCredential() {
 credential_kind_ = CredentialKind::kTokenCredential;
 token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();
 return Status::OK();
}

Status AzureOptions::ConfigureWorkloadIdentityCredential(
 const std::string& account_name) {
Status AzureOptions::ConfigureManagedIdentityCredential(const std::string& client_id) {
 credential_kind_ = CredentialKind::kTokenCredential;
 token_credential_ =
 std::make_shared<Azure::Identity::ManagedIdentityCredential>(client_id);
 return Status::OK();
}

Status AzureOptions::ConfigureWorkloadIdentityCredential() {
 credential_kind_ = CredentialKind::kTokenCredential;
 token_credential_ = std::make_shared<Azure::Identity::WorkloadIdentityCredential>();
 return Status::OK();
}

Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceClient()
 const {
 if (account_name.empty()) {
 return Status::Invalid("AzureOptions doesn't contain a valid account name");
 }
@felipecrvfelipecrvDec 22, 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.

Another simplification we can make:
  • Rename kAnonymous to kDefaultCredential
  • On the kDefaultCredential case here, initialize token_credential_ to std::make_shared<Azure::Identity::DefaultAzureCredential>() if it's not initialized already.
In the kTokenCredential case you can add a DCHECK(token_credential_) as documentation that when credential_kind_ is set to kTokenCredential we expect it to be set already (something that is guaranteed by credential_kind_ being private and set only by functions that also initialize token_credential_.
What you think?
@Tom-NewtonTom-NewtonDec 22, 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 think its useful to support both anonymous and default credential as distinct things. Admittedly, its a niche use-case but storage accounts can be public, which is when anonymous is useful https://learn.microsoft.com/en-us/azure/storage/blobs/anonymous-read-access-configure?tabs=portal.
Its a good question though which should be the default. I know adlfs deafults to anonomous rather than default credential. In some respects it might be good to be consistent but also this choice of default has caused issues for me in the past e.g. flyteorg/flyte#3962
@felipecrvfelipecrvDec 22, 2023•

edited

Loading

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.

Ok. Makes sense to keep kAnonymous then. But what would you return in AzureOptions::MakeBlobServiceClient() when credential_kind_ is anonymous? Lazy-init token_credential_ with DefaultAzureCredential and create the client with it? can you create the client without any credentials?
@felipecrvfelipecrvDec 22, 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 looked and there is a constructor that doesn't take any credentials. I guess that's what we should call :-)
@Tom-NewtonTom-NewtonDec 22, 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.

👍 Do you mind if we do that in a separate PR to fix up anonymous and change the default to default credential. This was supposed to be a super simple PR until kou realised all the auths were broken 😅
@felipecrvfelipecrvDec 22, 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.

No problem.
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name_),
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name),
 token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name_),
 return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name),
 storage_shared_key_credential_);
 }
 return Status::Invalid("AzureOptions doesn't contain a valid auth configuration");
}

Result<std::unique_ptr<DataLake::DataLakeServiceClient>>
AzureOptions::MakeDataLakeServiceClient() const {
 if (account_name.empty()) {
 return Status::Invalid("AzureOptions doesn't contain a valid account name");
 }
 switch (credential_kind_) {
 case CredentialKind::kAnonymous:
 break;
 case CredentialKind::kTokenCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(
 AccountDfsUrl(account_name_), token_credential_);
 AccountDfsUrl(account_name), token_credential_);
 case CredentialKind::kStorageSharedKeyCredential:
 return std::make_unique<DataLake::DataLakeServiceClient>(
 AccountDfsUrl(account_name_), storage_shared_key_credential_);
 AccountDfsUrl(account_name), storage_shared_key_credential_);
 }
 return Status::Invalid("AzureOptions doesn't contain a valid auth configuration");
}
Expand Down

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

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -48,6 +48,9 @@ class TestAzureFileSystem;

/// Options for the AzureFileSystem implementation.
struct ARROW_EXPORT AzureOptions {
 /// \brief account name of the Azure Storage account.
 std::string account_name;

 /// \brief hostname[:port] of the Azure Blob Storage Service.
 ///
 /// If the hostname is a relative domain name (one that starts with a '.'), then storage
Expand DownExpand Up@@ -94,7 +97,6 @@ struct ARROW_EXPORT AzureOptions {
 kStorageSharedKeyCredential,
 } credential_kind_ = CredentialKind::kAnonymous;

 std::string account_name_;
 std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;
 std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
 storage_shared_key_credential_;
Expand All@@ -103,15 +105,15 @@ struct ARROW_EXPORT AzureOptions {
 AzureOptions();
 ~AzureOptions();

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

 Status ConfigureManagedIdentityCredential(const std::string& client_id = std::string());

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

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

 Status ConfigureClientSecretCredential(const std::string& account_name,
 const std::string& tenant_id,
 Status ConfigureClientSecretCredential(const std::string& tenant_id,
 const std::string& client_id,
 const std::string& client_secret);

Expand Down

34 changes: 28 additions & 6 deletions
cpp/src/arrow/filesystem/azurefs_test.cc

View file


Open in desktop

Original file line numberDiff line numberDiff line change
Expand Up@@ -271,22 +271,44 @@ class AzureHierarchicalNSEnv : public AzureEnvImpl<AzureHierarchicalNSEnv> {
 bool WithHierarchicalNamespace() const final { return true; }
};

TEST(AzureFileSystem, InitializingFilesystemWithoutAccountNameFails) {
 AzureOptions options;
 ASSERT_RAISES(Invalid, options.ConfigureAccountKeyCredential("account_key"));

 ARROW_EXPECT_OK(
 options.ConfigureClientSecretCredential("tenant_id", "client_id", "client_secret"));
 ASSERT_RAISES(Invalid, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithClientSecretCredential) {
 AzureOptions options;
 ARROW_EXPECT_OK(options.ConfigureClientSecretCredential(
 "dummy-account-name", "tenant_id", "client_id", "client_secret"));
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(
 options.ConfigureClientSecretCredential("tenant_id", "client_id", "client_secret"));
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithDefaultCredential) {
 AzureOptions options;
 ARROW_EXPECT_OK(options.ConfigureDefaultCredential("dummy-account-name"));
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(options.ConfigureDefaultCredential());
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithManagedIdentityCredential) {
 AzureOptions options;
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(options.ConfigureManagedIdentityCredential());
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));

 ARROW_EXPECT_OK(options.ConfigureManagedIdentityCredential("specific-client-id"));
 EXPECT_OK_AND_ASSIGN(fs, AzureFileSystem::Make(options));
}

TEST(AzureFileSystem, InitializeFilesystemWithWorkloadIdentityCredential) {
 AzureOptions options;
 ARROW_EXPECT_OK(options.ConfigureWorkloadIdentityCredential("dummy-account-name"));
 options.account_name = "dummy-account-name";
 ARROW_EXPECT_OK(options.ConfigureWorkloadIdentityCredential());
 EXPECT_OK_AND_ASSIGN(auto fs, AzureFileSystem::Make(options));
}

Expand DownExpand Up@@ -383,6 +405,7 @@ class TestAzureFileSystem : public ::testing::Test {

 static Result<AzureOptions> MakeOptions(BaseAzureEnv* env) {
 AzureOptions options;
 options.account_name = env->account_name();
 switch (env->backend()) {
 case AzureBackend::kAzurite:
 options.blob_storage_authority = "127.0.0.1:10000";
Expand All@@ -394,8 +417,7 @@ class TestAzureFileSystem : public ::testing::Test {
 // Use the default values
 break;
 }
 ARROW_EXPECT_OK(
 options.ConfigureAccountKeyCredential(env->account_name(), env->account_key()));
 ARROW_EXPECT_OK(options.ConfigureAccountKeyCredential(env->account_key()));
 return options;
 }

Expand Down