felipecrv commented on code in PR #39321:
URL: https://github.com/apache/arrow/pull/39321#discussion_r1435250919
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -117,27 +117,42 @@ Status
AzureOptions::ConfigureClientSecretCredential(const std::string& account_
const std::string&
tenant_id,
const std::string&
client_id,
const std::string&
client_secret) {
+ account_name_ = account_name;
credential_kind_ = CredentialKind::kTokenCredential;
token_credential_ =
std::make_shared<Azure::Identity::ClientSecretCredential>(
tenant_id, client_id, client_secret);
return Status::OK();
}
Status AzureOptions::ConfigureDefaultCredential(const std::string&
account_name) {
+ account_name_ = account_name;
credential_kind_ = CredentialKind::kTokenCredential;
token_credential_ =
std::make_shared<Azure::Identity::DefaultAzureCredential>();
return Status::OK();
}
+Status AzureOptions::ConfigureManagedIdentityCredential(const std::string&
account_name,
+ const std::string&
client_id) {
+ account_name_ = account_name;
+ credential_kind_ = CredentialKind::kTokenCredential;
+ token_credential_ =
+ std::make_shared<Azure::Identity::ManagedIdentityCredential>(client_id);
+ return Status::OK();
+}
+
Status AzureOptions::ConfigureWorkloadIdentityCredential(
const std::string& account_name) {
+ account_name_ = account_name;
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");
+ }
Review Comment:
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?
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -119,6 +119,14 @@ Status AzureOptions::ConfigureDefaultCredential(const
std::string& account_name)
return Status::OK();
}
+Status AzureOptions::ConfigureManagedIdentityCredential(const std::string&
account_name,
Review Comment:
Sorry. I messed up and forgot to set `account_name_` on the `Configure...`
functions.
##########
cpp/src/arrow/filesystem/azurefs.h:
##########
@@ -105,6 +105,9 @@ struct ARROW_EXPORT AzureOptions {
Status ConfigureDefaultCredential(const std::string& account_name);
+ Status ConfigureManagedIdentityCredential(const std::string& account_name,
+ std::string const& clientId =
std::string());
Review Comment:
Code Style: should be `client_id` and you can put the `const` before
`std::string` like you did in the implementation. The MS C++ style is so weird
:D
##########
cpp/src/arrow/filesystem/azurefs.cc:
##########
@@ -119,6 +119,14 @@ Status AzureOptions::ConfigureDefaultCredential(const
std::string& account_name)
return Status::OK();
}
+Status AzureOptions::ConfigureManagedIdentityCredential(const std::string&
account_name,
Review Comment:
I think we should undo something I did and move `account_name_` back to a
public `account_name` field (the first in `AzureOptions`, but remove the
`account_name` parameter from all the `Configure...` functions. That allows us
to avoid the repetition of the `account_name` parameter and is more in line to
how every credential constructor doesn't really need an `account_name`.
@Tom-Newton what you think and can you do this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
