GH-39449: [C++] Use default Azure credentials implicitly and support anonymous credentials explicitly

[felipecrv]

Rationale for this change

• Default credentials should be used by default.
• There should be a way to connect to public containers without credentials (aka "anonymous credential").

What changes are included in this PR?

• Sync ordering of declarations and definitions in the AzureOptions classs
• Use default credentials even when ConfigureDefaultCredential() isn't explicitly called
• Create clients when credential_kind_ is "anonymous" instead of returning an error

Are these changes tested?

By new and existing tests.

• Closes: [C++][FS] Use default Azure credentials implicitly and support anonymous credentials explictly #39449

[github-actions]

⚠️ GitHub issue #39449has been automatically assigned in GitHub to PR creator.

[felipecrv]

@Tom-Newton

[Tom-Newton]

I don't think we really need to change this. I would rather just keep kTokenCredential to cover all the credentials that are based on https://github.com/Azure/azure-sdk-for-cpp/blob/e5e675440b44ace7d7a9e7bc303f877c06b59ea5/sdk/core/azure-core/inc/azure/core/credentials/credentials.hpp#L68

[felipecrv]

We need this to support Equals. Think of it as runtime type-information that describes which concrete implementation of TokenCredential is being used.

[felipecrv]

Besides that, we would need to make a distinction between kDefault and all the others kToken at least (to support the implicit default behavior). It's clearer if we then make a distinction on all of them.

[Tom-Newton]

I think I would have done it differently but I don't feel strongly.

[kou]

How about always creating Azure::Identity::DefaultAzureCredential instead of using mutable?

diff --git a/cpp/src/arrow/filesystem/azurefs.cc b/cpp/src/arrow/filesystem/azurefs.cc
index e98d56c73..5099cd25d 100644
--- a/cpp/src/arrow/filesystem/azurefs.cc+++ b/cpp/src/arrow/filesystem/azurefs.cc@@ -110,7 +110,6 @@ std::string AzureOptions::AccountDfsUrl(const std::string& account_name) const {
Status AzureOptions::ConfigureDefaultCredential() {
credential_kind_ = CredentialKind::kDefault;
- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();
return Status::OK();
}
@@ -160,10 +159,9 @@ Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceC
case CredentialKind::kAnonymous:
return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name));
case CredentialKind::kDefault:
- if (!token_credential_) {- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();- }- [[fallthrough]];+ return std::make_unique<Blobs::BlobServiceClient>(+ AccountBlobUrl(account_name),+ std::make_shared<Azure::Identity::DefaultAzureCredential>());
case CredentialKind::kClientSecret:
case CredentialKind::kManagedIdentity:
case CredentialKind::kWorkloadIdentity:
@@ -186,10 +184,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
return std::make_unique<DataLake::DataLakeServiceClient>(
AccountDfsUrl(account_name));
case CredentialKind::kDefault:
- if (!token_credential_) {- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();- }- [[fallthrough]];+ return std::make_unique<DataLake::DataLakeServiceClient>(+ AccountDfsUrl(account_name),+ std::make_shared<Azure::Identity::DefaultAzureCredential>());
case CredentialKind::kClientSecret:
case CredentialKind::kManagedIdentity:
case CredentialKind::kWorkloadIdentity:
diff --git a/cpp/src/arrow/filesystem/azurefs.h b/cpp/src/arrow/filesystem/azurefs.h
index 55f89ba47..ba612f799 100644
--- a/cpp/src/arrow/filesystem/azurefs.h+++ b/cpp/src/arrow/filesystem/azurefs.h@@ -117,7 +117,7 @@ struct ARROW_EXPORT AzureOptions {
std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
storage_shared_key_credential_;
- mutable std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;+ std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;
public:
AzureOptions();

[felipecrv]

Creating DefaultAzureCredential is a lot of non-trivial work: env vars, multiple allocations and probes. We can't assume it to be a cheap operation. mutable is a common solution to lazily-initialized class members and this one has a very well-defined behavior: it's set once, then it doesn't change at all anymore.

[felipecrv]

And it's important that we do mutate it in ConfigureDefaultCredential because the user might want to explicitly initialize the credential immediately after doing something with the env variables used by the Azure SDK or right before unsetting the variables.

[felipecrv]

Rebased and force-pushed after the merge of my bigger PR related to directory semantics.

[Tom-Newton]

👍

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 33c64ed.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.

[felipecrv]

Rationale for this change

• Default credentials should be used by default.
• There should be a way to connect to public containers without credentials (aka "anonymous credential").

What changes are included in this PR?

• Sync ordering of declarations and definitions in the AzureOptions classs
• Use default credentials even when ConfigureDefaultCredential() isn't explicitly called
• Create clients when credential_kind_ is "anonymous" instead of returning an error

Are these changes tested?

By new and existing tests.

• Closes: [C++][FS] Use default Azure credentials implicitly and support anonymous credentials explictly #39449

[github-actions]

⚠️ GitHub issue #39449has been automatically assigned in GitHub to PR creator.

[felipecrv]

@Tom-Newton

[Tom-Newton]

I don't think we really need to change this. I would rather just keep kTokenCredential to cover all the credentials that are based on https://github.com/Azure/azure-sdk-for-cpp/blob/e5e675440b44ace7d7a9e7bc303f877c06b59ea5/sdk/core/azure-core/inc/azure/core/credentials/credentials.hpp#L68

[felipecrv]

We need this to support Equals. Think of it as runtime type-information that describes which concrete implementation of TokenCredential is being used.

[felipecrv]

Besides that, we would need to make a distinction between kDefault and all the others kToken at least (to support the implicit default behavior). It's clearer if we then make a distinction on all of them.

[Tom-Newton]

I think I would have done it differently but I don't feel strongly.

[kou]

How about always creating Azure::Identity::DefaultAzureCredential instead of using mutable?

diff --git a/cpp/src/arrow/filesystem/azurefs.cc b/cpp/src/arrow/filesystem/azurefs.cc
index e98d56c73..5099cd25d 100644
--- a/cpp/src/arrow/filesystem/azurefs.cc+++ b/cpp/src/arrow/filesystem/azurefs.cc@@ -110,7 +110,6 @@ std::string AzureOptions::AccountDfsUrl(const std::string& account_name) const {
Status AzureOptions::ConfigureDefaultCredential() {
credential_kind_ = CredentialKind::kDefault;
- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();
return Status::OK();
}
@@ -160,10 +159,9 @@ Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceC
case CredentialKind::kAnonymous:
return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name));
case CredentialKind::kDefault:
- if (!token_credential_) {- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();- }- [[fallthrough]];+ return std::make_unique<Blobs::BlobServiceClient>(+ AccountBlobUrl(account_name),+ std::make_shared<Azure::Identity::DefaultAzureCredential>());
case CredentialKind::kClientSecret:
case CredentialKind::kManagedIdentity:
case CredentialKind::kWorkloadIdentity:
@@ -186,10 +184,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
return std::make_unique<DataLake::DataLakeServiceClient>(
AccountDfsUrl(account_name));
case CredentialKind::kDefault:
- if (!token_credential_) {- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();- }- [[fallthrough]];+ return std::make_unique<DataLake::DataLakeServiceClient>(+ AccountDfsUrl(account_name),+ std::make_shared<Azure::Identity::DefaultAzureCredential>());
case CredentialKind::kClientSecret:
case CredentialKind::kManagedIdentity:
case CredentialKind::kWorkloadIdentity:
diff --git a/cpp/src/arrow/filesystem/azurefs.h b/cpp/src/arrow/filesystem/azurefs.h
index 55f89ba47..ba612f799 100644
--- a/cpp/src/arrow/filesystem/azurefs.h+++ b/cpp/src/arrow/filesystem/azurefs.h@@ -117,7 +117,7 @@ struct ARROW_EXPORT AzureOptions {
std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
storage_shared_key_credential_;
- mutable std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;+ std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;
public:
AzureOptions();

[felipecrv]

Creating DefaultAzureCredential is a lot of non-trivial work: env vars, multiple allocations and probes. We can't assume it to be a cheap operation. mutable is a common solution to lazily-initialized class members and this one has a very well-defined behavior: it's set once, then it doesn't change at all anymore.

[felipecrv]

And it's important that we do mutate it in ConfigureDefaultCredential because the user might want to explicitly initialize the credential immediately after doing something with the env variables used by the Azure SDK or right before unsetting the variables.

[felipecrv]

Rebased and force-pushed after the merge of my bigger PR related to directory semantics.

[Tom-Newton]

👍

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 33c64ed.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.

[felipecrv]

Rationale for this change

• Default credentials should be used by default.
• There should be a way to connect to public containers without credentials (aka "anonymous credential").

What changes are included in this PR?

• Sync ordering of declarations and definitions in the AzureOptions classs
• Use default credentials even when ConfigureDefaultCredential() isn't explicitly called
• Create clients when credential_kind_ is "anonymous" instead of returning an error

Are these changes tested?

By new and existing tests.

• Closes: [C++][FS] Use default Azure credentials implicitly and support anonymous credentials explictly #39449

[github-actions]

⚠️ GitHub issue #39449has been automatically assigned in GitHub to PR creator.

[felipecrv]

@Tom-Newton

[Tom-Newton]

I don't think we really need to change this. I would rather just keep kTokenCredential to cover all the credentials that are based on https://github.com/Azure/azure-sdk-for-cpp/blob/e5e675440b44ace7d7a9e7bc303f877c06b59ea5/sdk/core/azure-core/inc/azure/core/credentials/credentials.hpp#L68

[felipecrv]

We need this to support Equals. Think of it as runtime type-information that describes which concrete implementation of TokenCredential is being used.

[felipecrv]

Besides that, we would need to make a distinction between kDefault and all the others kToken at least (to support the implicit default behavior). It's clearer if we then make a distinction on all of them.

[Tom-Newton]

I think I would have done it differently but I don't feel strongly.

[kou]

How about always creating Azure::Identity::DefaultAzureCredential instead of using mutable?

diff --git a/cpp/src/arrow/filesystem/azurefs.cc b/cpp/src/arrow/filesystem/azurefs.cc
index e98d56c73..5099cd25d 100644
--- a/cpp/src/arrow/filesystem/azurefs.cc+++ b/cpp/src/arrow/filesystem/azurefs.cc@@ -110,7 +110,6 @@ std::string AzureOptions::AccountDfsUrl(const std::string& account_name) const {
Status AzureOptions::ConfigureDefaultCredential() {
credential_kind_ = CredentialKind::kDefault;
- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();
return Status::OK();
}
@@ -160,10 +159,9 @@ Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceC
case CredentialKind::kAnonymous:
return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name));
case CredentialKind::kDefault:
- if (!token_credential_) {- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();- }- [[fallthrough]];+ return std::make_unique<Blobs::BlobServiceClient>(+ AccountBlobUrl(account_name),+ std::make_shared<Azure::Identity::DefaultAzureCredential>());
case CredentialKind::kClientSecret:
case CredentialKind::kManagedIdentity:
case CredentialKind::kWorkloadIdentity:
@@ -186,10 +184,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
return std::make_unique<DataLake::DataLakeServiceClient>(
AccountDfsUrl(account_name));
case CredentialKind::kDefault:
- if (!token_credential_) {- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();- }- [[fallthrough]];+ return std::make_unique<DataLake::DataLakeServiceClient>(+ AccountDfsUrl(account_name),+ std::make_shared<Azure::Identity::DefaultAzureCredential>());
case CredentialKind::kClientSecret:
case CredentialKind::kManagedIdentity:
case CredentialKind::kWorkloadIdentity:
diff --git a/cpp/src/arrow/filesystem/azurefs.h b/cpp/src/arrow/filesystem/azurefs.h
index 55f89ba47..ba612f799 100644
--- a/cpp/src/arrow/filesystem/azurefs.h+++ b/cpp/src/arrow/filesystem/azurefs.h@@ -117,7 +117,7 @@ struct ARROW_EXPORT AzureOptions {
std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
storage_shared_key_credential_;
- mutable std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;+ std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;
public:
AzureOptions();

[felipecrv]

Creating DefaultAzureCredential is a lot of non-trivial work: env vars, multiple allocations and probes. We can't assume it to be a cheap operation. mutable is a common solution to lazily-initialized class members and this one has a very well-defined behavior: it's set once, then it doesn't change at all anymore.

[felipecrv]

And it's important that we do mutate it in ConfigureDefaultCredential because the user might want to explicitly initialize the credential immediately after doing something with the env variables used by the Azure SDK or right before unsetting the variables.

[felipecrv]

Rebased and force-pushed after the merge of my bigger PR related to directory semantics.

[Tom-Newton]

👍

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 33c64ed.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.

[felipecrv]

Rationale for this change

• Default credentials should be used by default.
• There should be a way to connect to public containers without credentials (aka "anonymous credential").

What changes are included in this PR?

• Sync ordering of declarations and definitions in the AzureOptions classs
• Use default credentials even when ConfigureDefaultCredential() isn't explicitly called
• Create clients when credential_kind_ is "anonymous" instead of returning an error

Are these changes tested?

By new and existing tests.

• Closes: [C++][FS] Use default Azure credentials implicitly and support anonymous credentials explictly #39449

[github-actions]

⚠️ GitHub issue #39449has been automatically assigned in GitHub to PR creator.

[felipecrv]

@Tom-Newton

[Tom-Newton]

I don't think we really need to change this. I would rather just keep kTokenCredential to cover all the credentials that are based on https://github.com/Azure/azure-sdk-for-cpp/blob/e5e675440b44ace7d7a9e7bc303f877c06b59ea5/sdk/core/azure-core/inc/azure/core/credentials/credentials.hpp#L68

[felipecrv]

We need this to support Equals. Think of it as runtime type-information that describes which concrete implementation of TokenCredential is being used.

[felipecrv]

Besides that, we would need to make a distinction between kDefault and all the others kToken at least (to support the implicit default behavior). It's clearer if we then make a distinction on all of them.

[Tom-Newton]

I think I would have done it differently but I don't feel strongly.

[kou]

How about always creating Azure::Identity::DefaultAzureCredential instead of using mutable?

diff --git a/cpp/src/arrow/filesystem/azurefs.cc b/cpp/src/arrow/filesystem/azurefs.cc
index e98d56c73..5099cd25d 100644
--- a/cpp/src/arrow/filesystem/azurefs.cc+++ b/cpp/src/arrow/filesystem/azurefs.cc@@ -110,7 +110,6 @@ std::string AzureOptions::AccountDfsUrl(const std::string& account_name) const {
Status AzureOptions::ConfigureDefaultCredential() {
credential_kind_ = CredentialKind::kDefault;
- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();
return Status::OK();
}
@@ -160,10 +159,9 @@ Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceC
case CredentialKind::kAnonymous:
return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name));
case CredentialKind::kDefault:
- if (!token_credential_) {- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();- }- [[fallthrough]];+ return std::make_unique<Blobs::BlobServiceClient>(+ AccountBlobUrl(account_name),+ std::make_shared<Azure::Identity::DefaultAzureCredential>());
case CredentialKind::kClientSecret:
case CredentialKind::kManagedIdentity:
case CredentialKind::kWorkloadIdentity:
@@ -186,10 +184,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
return std::make_unique<DataLake::DataLakeServiceClient>(
AccountDfsUrl(account_name));
case CredentialKind::kDefault:
- if (!token_credential_) {- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();- }- [[fallthrough]];+ return std::make_unique<DataLake::DataLakeServiceClient>(+ AccountDfsUrl(account_name),+ std::make_shared<Azure::Identity::DefaultAzureCredential>());
case CredentialKind::kClientSecret:
case CredentialKind::kManagedIdentity:
case CredentialKind::kWorkloadIdentity:
diff --git a/cpp/src/arrow/filesystem/azurefs.h b/cpp/src/arrow/filesystem/azurefs.h
index 55f89ba47..ba612f799 100644
--- a/cpp/src/arrow/filesystem/azurefs.h+++ b/cpp/src/arrow/filesystem/azurefs.h@@ -117,7 +117,7 @@ struct ARROW_EXPORT AzureOptions {
std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
storage_shared_key_credential_;
- mutable std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;+ std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;
public:
AzureOptions();

[felipecrv]

Creating DefaultAzureCredential is a lot of non-trivial work: env vars, multiple allocations and probes. We can't assume it to be a cheap operation. mutable is a common solution to lazily-initialized class members and this one has a very well-defined behavior: it's set once, then it doesn't change at all anymore.

[felipecrv]

And it's important that we do mutate it in ConfigureDefaultCredential because the user might want to explicitly initialize the credential immediately after doing something with the env variables used by the Azure SDK or right before unsetting the variables.

[felipecrv]

Rebased and force-pushed after the merge of my bigger PR related to directory semantics.

[Tom-Newton]

👍

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 33c64ed.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.

[felipecrv]

Rationale for this change

• Default credentials should be used by default.
• There should be a way to connect to public containers without credentials (aka "anonymous credential").

What changes are included in this PR?

• Sync ordering of declarations and definitions in the AzureOptions classs
• Use default credentials even when ConfigureDefaultCredential() isn't explicitly called
• Create clients when credential_kind_ is "anonymous" instead of returning an error

Are these changes tested?

By new and existing tests.

• Closes: [C++][FS] Use default Azure credentials implicitly and support anonymous credentials explictly #39449

[github-actions]

⚠️ GitHub issue #39449has been automatically assigned in GitHub to PR creator.

[felipecrv]

@Tom-Newton

[Tom-Newton]

I don't think we really need to change this. I would rather just keep kTokenCredential to cover all the credentials that are based on https://github.com/Azure/azure-sdk-for-cpp/blob/e5e675440b44ace7d7a9e7bc303f877c06b59ea5/sdk/core/azure-core/inc/azure/core/credentials/credentials.hpp#L68

[felipecrv]

We need this to support Equals. Think of it as runtime type-information that describes which concrete implementation of TokenCredential is being used.

[felipecrv]

Besides that, we would need to make a distinction between kDefault and all the others kToken at least (to support the implicit default behavior). It's clearer if we then make a distinction on all of them.

[Tom-Newton]

I think I would have done it differently but I don't feel strongly.

[kou]

How about always creating Azure::Identity::DefaultAzureCredential instead of using mutable?

diff --git a/cpp/src/arrow/filesystem/azurefs.cc b/cpp/src/arrow/filesystem/azurefs.cc
index e98d56c73..5099cd25d 100644
--- a/cpp/src/arrow/filesystem/azurefs.cc+++ b/cpp/src/arrow/filesystem/azurefs.cc@@ -110,7 +110,6 @@ std::string AzureOptions::AccountDfsUrl(const std::string& account_name) const {
Status AzureOptions::ConfigureDefaultCredential() {
credential_kind_ = CredentialKind::kDefault;
- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();
return Status::OK();
}
@@ -160,10 +159,9 @@ Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceC
case CredentialKind::kAnonymous:
return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name));
case CredentialKind::kDefault:
- if (!token_credential_) {- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();- }- [[fallthrough]];+ return std::make_unique<Blobs::BlobServiceClient>(+ AccountBlobUrl(account_name),+ std::make_shared<Azure::Identity::DefaultAzureCredential>());
case CredentialKind::kClientSecret:
case CredentialKind::kManagedIdentity:
case CredentialKind::kWorkloadIdentity:
@@ -186,10 +184,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
return std::make_unique<DataLake::DataLakeServiceClient>(
AccountDfsUrl(account_name));
case CredentialKind::kDefault:
- if (!token_credential_) {- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();- }- [[fallthrough]];+ return std::make_unique<DataLake::DataLakeServiceClient>(+ AccountDfsUrl(account_name),+ std::make_shared<Azure::Identity::DefaultAzureCredential>());
case CredentialKind::kClientSecret:
case CredentialKind::kManagedIdentity:
case CredentialKind::kWorkloadIdentity:
diff --git a/cpp/src/arrow/filesystem/azurefs.h b/cpp/src/arrow/filesystem/azurefs.h
index 55f89ba47..ba612f799 100644
--- a/cpp/src/arrow/filesystem/azurefs.h+++ b/cpp/src/arrow/filesystem/azurefs.h@@ -117,7 +117,7 @@ struct ARROW_EXPORT AzureOptions {
std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
storage_shared_key_credential_;
- mutable std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;+ std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;
public:
AzureOptions();

[felipecrv]

Creating DefaultAzureCredential is a lot of non-trivial work: env vars, multiple allocations and probes. We can't assume it to be a cheap operation. mutable is a common solution to lazily-initialized class members and this one has a very well-defined behavior: it's set once, then it doesn't change at all anymore.

[felipecrv]

And it's important that we do mutate it in ConfigureDefaultCredential because the user might want to explicitly initialize the credential immediately after doing something with the env variables used by the Azure SDK or right before unsetting the variables.

[felipecrv]

Rebased and force-pushed after the merge of my bigger PR related to directory semantics.

[Tom-Newton]

👍

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 33c64ed.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.

[felipecrv]

Rationale for this change

• Default credentials should be used by default.
• There should be a way to connect to public containers without credentials (aka "anonymous credential").

What changes are included in this PR?

• Sync ordering of declarations and definitions in the AzureOptions classs
• Use default credentials even when ConfigureDefaultCredential() isn't explicitly called
• Create clients when credential_kind_ is "anonymous" instead of returning an error

Are these changes tested?

By new and existing tests.

• Closes: [C++][FS] Use default Azure credentials implicitly and support anonymous credentials explictly #39449

[github-actions]

⚠️ GitHub issue #39449has been automatically assigned in GitHub to PR creator.

[felipecrv]

@Tom-Newton

[Tom-Newton]

I don't think we really need to change this. I would rather just keep kTokenCredential to cover all the credentials that are based on https://github.com/Azure/azure-sdk-for-cpp/blob/e5e675440b44ace7d7a9e7bc303f877c06b59ea5/sdk/core/azure-core/inc/azure/core/credentials/credentials.hpp#L68

[felipecrv]

We need this to support Equals. Think of it as runtime type-information that describes which concrete implementation of TokenCredential is being used.

[felipecrv]

Besides that, we would need to make a distinction between kDefault and all the others kToken at least (to support the implicit default behavior). It's clearer if we then make a distinction on all of them.

[Tom-Newton]

I think I would have done it differently but I don't feel strongly.

[kou]

How about always creating Azure::Identity::DefaultAzureCredential instead of using mutable?

diff --git a/cpp/src/arrow/filesystem/azurefs.cc b/cpp/src/arrow/filesystem/azurefs.cc
index e98d56c73..5099cd25d 100644
--- a/cpp/src/arrow/filesystem/azurefs.cc+++ b/cpp/src/arrow/filesystem/azurefs.cc@@ -110,7 +110,6 @@ std::string AzureOptions::AccountDfsUrl(const std::string& account_name) const {
Status AzureOptions::ConfigureDefaultCredential() {
credential_kind_ = CredentialKind::kDefault;
- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();
return Status::OK();
}
@@ -160,10 +159,9 @@ Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceC
case CredentialKind::kAnonymous:
return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name));
case CredentialKind::kDefault:
- if (!token_credential_) {- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();- }- [[fallthrough]];+ return std::make_unique<Blobs::BlobServiceClient>(+ AccountBlobUrl(account_name),+ std::make_shared<Azure::Identity::DefaultAzureCredential>());
case CredentialKind::kClientSecret:
case CredentialKind::kManagedIdentity:
case CredentialKind::kWorkloadIdentity:
@@ -186,10 +184,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
return std::make_unique<DataLake::DataLakeServiceClient>(
AccountDfsUrl(account_name));
case CredentialKind::kDefault:
- if (!token_credential_) {- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();- }- [[fallthrough]];+ return std::make_unique<DataLake::DataLakeServiceClient>(+ AccountDfsUrl(account_name),+ std::make_shared<Azure::Identity::DefaultAzureCredential>());
case CredentialKind::kClientSecret:
case CredentialKind::kManagedIdentity:
case CredentialKind::kWorkloadIdentity:
diff --git a/cpp/src/arrow/filesystem/azurefs.h b/cpp/src/arrow/filesystem/azurefs.h
index 55f89ba47..ba612f799 100644
--- a/cpp/src/arrow/filesystem/azurefs.h+++ b/cpp/src/arrow/filesystem/azurefs.h@@ -117,7 +117,7 @@ struct ARROW_EXPORT AzureOptions {
std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
storage_shared_key_credential_;
- mutable std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;+ std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;
public:
AzureOptions();

[felipecrv]

Creating DefaultAzureCredential is a lot of non-trivial work: env vars, multiple allocations and probes. We can't assume it to be a cheap operation. mutable is a common solution to lazily-initialized class members and this one has a very well-defined behavior: it's set once, then it doesn't change at all anymore.

[felipecrv]

And it's important that we do mutate it in ConfigureDefaultCredential because the user might want to explicitly initialize the credential immediately after doing something with the env variables used by the Azure SDK or right before unsetting the variables.

[felipecrv]

Rebased and force-pushed after the merge of my bigger PR related to directory semantics.

[Tom-Newton]

👍

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 33c64ed.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.

[felipecrv]

Rationale for this change

• Default credentials should be used by default.
• There should be a way to connect to public containers without credentials (aka "anonymous credential").

What changes are included in this PR?

• Sync ordering of declarations and definitions in the AzureOptions classs
• Use default credentials even when ConfigureDefaultCredential() isn't explicitly called
• Create clients when credential_kind_ is "anonymous" instead of returning an error

Are these changes tested?

By new and existing tests.

• Closes: [C++][FS] Use default Azure credentials implicitly and support anonymous credentials explictly #39449

[github-actions]

⚠️ GitHub issue #39449has been automatically assigned in GitHub to PR creator.

[felipecrv]

@Tom-Newton

[Tom-Newton]

I don't think we really need to change this. I would rather just keep kTokenCredential to cover all the credentials that are based on https://github.com/Azure/azure-sdk-for-cpp/blob/e5e675440b44ace7d7a9e7bc303f877c06b59ea5/sdk/core/azure-core/inc/azure/core/credentials/credentials.hpp#L68

[felipecrv]

We need this to support Equals. Think of it as runtime type-information that describes which concrete implementation of TokenCredential is being used.

[felipecrv]

Besides that, we would need to make a distinction between kDefault and all the others kToken at least (to support the implicit default behavior). It's clearer if we then make a distinction on all of them.

[Tom-Newton]

I think I would have done it differently but I don't feel strongly.

[kou]

How about always creating Azure::Identity::DefaultAzureCredential instead of using mutable?

diff --git a/cpp/src/arrow/filesystem/azurefs.cc b/cpp/src/arrow/filesystem/azurefs.cc
index e98d56c73..5099cd25d 100644
--- a/cpp/src/arrow/filesystem/azurefs.cc+++ b/cpp/src/arrow/filesystem/azurefs.cc@@ -110,7 +110,6 @@ std::string AzureOptions::AccountDfsUrl(const std::string& account_name) const {
Status AzureOptions::ConfigureDefaultCredential() {
credential_kind_ = CredentialKind::kDefault;
- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();
return Status::OK();
}
@@ -160,10 +159,9 @@ Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceC
case CredentialKind::kAnonymous:
return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name));
case CredentialKind::kDefault:
- if (!token_credential_) {- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();- }- [[fallthrough]];+ return std::make_unique<Blobs::BlobServiceClient>(+ AccountBlobUrl(account_name),+ std::make_shared<Azure::Identity::DefaultAzureCredential>());
case CredentialKind::kClientSecret:
case CredentialKind::kManagedIdentity:
case CredentialKind::kWorkloadIdentity:
@@ -186,10 +184,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
return std::make_unique<DataLake::DataLakeServiceClient>(
AccountDfsUrl(account_name));
case CredentialKind::kDefault:
- if (!token_credential_) {- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();- }- [[fallthrough]];+ return std::make_unique<DataLake::DataLakeServiceClient>(+ AccountDfsUrl(account_name),+ std::make_shared<Azure::Identity::DefaultAzureCredential>());
case CredentialKind::kClientSecret:
case CredentialKind::kManagedIdentity:
case CredentialKind::kWorkloadIdentity:
diff --git a/cpp/src/arrow/filesystem/azurefs.h b/cpp/src/arrow/filesystem/azurefs.h
index 55f89ba47..ba612f799 100644
--- a/cpp/src/arrow/filesystem/azurefs.h+++ b/cpp/src/arrow/filesystem/azurefs.h@@ -117,7 +117,7 @@ struct ARROW_EXPORT AzureOptions {
std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
storage_shared_key_credential_;
- mutable std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;+ std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;
public:
AzureOptions();

[felipecrv]

Creating DefaultAzureCredential is a lot of non-trivial work: env vars, multiple allocations and probes. We can't assume it to be a cheap operation. mutable is a common solution to lazily-initialized class members and this one has a very well-defined behavior: it's set once, then it doesn't change at all anymore.

[felipecrv]

And it's important that we do mutate it in ConfigureDefaultCredential because the user might want to explicitly initialize the credential immediately after doing something with the env variables used by the Azure SDK or right before unsetting the variables.

[felipecrv]

Rebased and force-pushed after the merge of my bigger PR related to directory semantics.

[Tom-Newton]

👍

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 33c64ed.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.

[felipecrv]

Rationale for this change

• Default credentials should be used by default.
• There should be a way to connect to public containers without credentials (aka "anonymous credential").

What changes are included in this PR?

• Sync ordering of declarations and definitions in the AzureOptions classs
• Use default credentials even when ConfigureDefaultCredential() isn't explicitly called
• Create clients when credential_kind_ is "anonymous" instead of returning an error

Are these changes tested?

By new and existing tests.

• Closes: [C++][FS] Use default Azure credentials implicitly and support anonymous credentials explictly #39449

[github-actions]

⚠️ GitHub issue #39449has been automatically assigned in GitHub to PR creator.

[felipecrv]

@Tom-Newton

[Tom-Newton]

I don't think we really need to change this. I would rather just keep kTokenCredential to cover all the credentials that are based on https://github.com/Azure/azure-sdk-for-cpp/blob/e5e675440b44ace7d7a9e7bc303f877c06b59ea5/sdk/core/azure-core/inc/azure/core/credentials/credentials.hpp#L68

[felipecrv]

We need this to support Equals. Think of it as runtime type-information that describes which concrete implementation of TokenCredential is being used.

[felipecrv]

Besides that, we would need to make a distinction between kDefault and all the others kToken at least (to support the implicit default behavior). It's clearer if we then make a distinction on all of them.

[Tom-Newton]

I think I would have done it differently but I don't feel strongly.

[kou]

How about always creating Azure::Identity::DefaultAzureCredential instead of using mutable?

diff --git a/cpp/src/arrow/filesystem/azurefs.cc b/cpp/src/arrow/filesystem/azurefs.cc
index e98d56c73..5099cd25d 100644
--- a/cpp/src/arrow/filesystem/azurefs.cc+++ b/cpp/src/arrow/filesystem/azurefs.cc@@ -110,7 +110,6 @@ std::string AzureOptions::AccountDfsUrl(const std::string& account_name) const {
Status AzureOptions::ConfigureDefaultCredential() {
credential_kind_ = CredentialKind::kDefault;
- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();
return Status::OK();
}
@@ -160,10 +159,9 @@ Result<std::unique_ptr<Blobs::BlobServiceClient>> AzureOptions::MakeBlobServiceC
case CredentialKind::kAnonymous:
return std::make_unique<Blobs::BlobServiceClient>(AccountBlobUrl(account_name));
case CredentialKind::kDefault:
- if (!token_credential_) {- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();- }- [[fallthrough]];+ return std::make_unique<Blobs::BlobServiceClient>(+ AccountBlobUrl(account_name),+ std::make_shared<Azure::Identity::DefaultAzureCredential>());
case CredentialKind::kClientSecret:
case CredentialKind::kManagedIdentity:
case CredentialKind::kWorkloadIdentity:
@@ -186,10 +184,9 @@ AzureOptions::MakeDataLakeServiceClient() const {
return std::make_unique<DataLake::DataLakeServiceClient>(
AccountDfsUrl(account_name));
case CredentialKind::kDefault:
- if (!token_credential_) {- token_credential_ = std::make_shared<Azure::Identity::DefaultAzureCredential>();- }- [[fallthrough]];+ return std::make_unique<DataLake::DataLakeServiceClient>(+ AccountDfsUrl(account_name),+ std::make_shared<Azure::Identity::DefaultAzureCredential>());
case CredentialKind::kClientSecret:
case CredentialKind::kManagedIdentity:
case CredentialKind::kWorkloadIdentity:
diff --git a/cpp/src/arrow/filesystem/azurefs.h b/cpp/src/arrow/filesystem/azurefs.h
index 55f89ba47..ba612f799 100644
--- a/cpp/src/arrow/filesystem/azurefs.h+++ b/cpp/src/arrow/filesystem/azurefs.h@@ -117,7 +117,7 @@ struct ARROW_EXPORT AzureOptions {
std::shared_ptr<Azure::Storage::StorageSharedKeyCredential>
storage_shared_key_credential_;
- mutable std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;+ std::shared_ptr<Azure::Core::Credentials::TokenCredential> token_credential_;
public:
AzureOptions();

[felipecrv]

Creating DefaultAzureCredential is a lot of non-trivial work: env vars, multiple allocations and probes. We can't assume it to be a cheap operation. mutable is a common solution to lazily-initialized class members and this one has a very well-defined behavior: it's set once, then it doesn't change at all anymore.

[felipecrv]

And it's important that we do mutate it in ConfigureDefaultCredential because the user might want to explicitly initialize the credential immediately after doing something with the env variables used by the Azure SDK or right before unsetting the variables.

[felipecrv]

Rebased and force-pushed after the merge of my bigger PR related to directory semantics.

[Tom-Newton]

👍

[conbench-apache-arrow]

After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit 33c64ed.

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 4 possible false positives for unstable benchmarks that are known to sometimes produce them.