You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "Tom-Newton (via GitHub)" <gi...@apache.org> on 2023/12/20 13:11:53 UTC
[PR] GH-39320: Add Managed identity auth configuration [arrow]
Tom-Newton opened a new pull request, #39321:
URL: https://github.com/apache/arrow/pull/39321
<!--
Thanks for opening a pull request!
If this is your first pull request you can find detailed information on how
to contribute here:
* [New Contributor's Guide](https://arrow.apache.org/docs/dev/developers/guide/step_by_step/pr_lifecycle.html#reviews-and-merge-of-the-pull-request)
* [Contributing Overview](https://arrow.apache.org/docs/dev/developers/overview.html)
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
In the case of PARQUET issues on JIRA the title also supports:
PARQUET-${JIRA_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
-->
### Rationale for this change
Workload identity is a useful Azure authentication method.
### What changes are included in this PR?
Implement `AzureOptions::ConfigureManagedIdentityCredential`
### Are these changes tested?
Added a simple test initialising a filesystem using `ConfigureManagedIdentityCredential`. This is not the most comprehensive test but its the same as what we agreed on for https://github.com/apache/arrow/pull/39263.
### Are there any user-facing changes?
Workload identity authentication is now supported.
<!--
If there are any breaking changes to public APIs, please uncomment the line below and explain which changes are breaking.
-->
<!-- **This PR includes breaking changes to public APIs.** -->
<!--
Please uncomment the line below (and provide explanation) if the changes fix either (a) a security vulnerability, (b) a bug that caused incorrect or invalid data to be produced, or (c) a bug that causes a crash (even when the API contract is upheld). We use this to highlight fixes to issues that may affect users without their knowledge. For this reason, fixing bugs that cause errors don't count, since those are usually obvious.
-->
<!-- **This PR contains a "Critical Fix".** -->
* Closes: #39320
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #39321:
URL: https://github.com/apache/arrow/pull/39321#discussion_r1435314150
##########
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:
:+1: 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 :sweat_smile:
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #39321:
URL: https://github.com/apache/arrow/pull/39321#discussion_r1434619551
##########
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:
Good catch. Sorry, I have not been paying enough attention :man_facepalming: . I think @felipecrv refactored this. I need to check how the account name is configured now. I'll do it tomorrow.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #39321:
URL: https://github.com/apache/arrow/pull/39321#discussion_r1435261766
##########
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:
Oops... I did it again :disappointed:. Fixed now.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #39321:
URL: https://github.com/apache/arrow/pull/39321#discussion_r1435314150
##########
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:
:+1: Do you mind if we do that in a separate PR to fix up anonymous and change the default to default credential.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv merged PR #39321:
URL: https://github.com/apache/arrow/pull/39321
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #39321:
URL: https://github.com/apache/arrow/pull/39321#discussion_r1434623385
##########
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:
Thanks.
It seems that both of `ConfigureWorkloadIdentityCredential()` and `ConfigureDefaultCredential()` are the same situation. Could you also check them too?
##########
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:
Thanks.
It seems that both of `ConfigureWorkloadIdentityCredential()` and `ConfigureDefaultCredential()` are the same situation too. Could you also check them too?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #39321:
URL: https://github.com/apache/arrow/pull/39321#discussion_r1435278232
##########
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:
I looked and there is a constructor that doesn't take any credentials. I guess that's what we should call :-)
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #39321:
URL: https://github.com/apache/arrow/pull/39321#discussion_r1434615079
##########
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,
+ std::string const& clientId) {
Review Comment:
Could you use `const XXX` style?
Could you use `snake_case`?
```suggestion
const std::string& client_id) {
```
##########
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:
It seems that `account_name` isn't used. Is it needed?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #39321:
URL: https://github.com/apache/arrow/pull/39321#discussion_r1435273720
##########
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:
This would be a good idea in normal classes, but these `*Options` classes tend to be designed like simple C structs that can be constructed from any language and easily serialized. The only exception being the secret stuff that we don't want to leak and lazily initialized fields.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #39321:
URL: https://github.com/apache/arrow/pull/39321#discussion_r1435314150
##########
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:
:+1: 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 :sweat_smile:
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #39321:
URL: https://github.com/apache/arrow/pull/39321#discussion_r1435318550
##########
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:
Ok. I think its as you recommended now.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #39321:
URL: https://github.com/apache/arrow/pull/39321#discussion_r1435021105
##########
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:
Fixed now. I have added a check so that filesystem initialisation fails if `account_name_.empty()`.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #39321:
URL: https://github.com/apache/arrow/pull/39321#issuecomment-1868348032
After merging your PR, Conbench analyzed the 6 benchmarking runs that have been run so far on merge-commit d51954415882423584f2a95b0897aa4d073a4e1c.
There was 1 benchmark result indicating a performance regression:
- Commit Run on `ursa-i9-9960x` at [2023-12-23 15:45:55Z](https://conbench.ursa.dev/compare/runs/e71e86489a7f43ad97a98aa572cc72d3...10cf33505b084aea8c30f7ce3828d8b0/)
- [`tpch` (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-08, scale_factor=1](https://conbench.ursa.dev/compare/benchmarks/065861bc835e78fc80008fe4d13e16b7...0658715a69fb79cd8000b29c490de82b)
The [full Conbench report](https://github.com/apache/arrow/runs/19917914170) has more details. It also includes information about 1 possible false positive for unstable benchmarks that are known to sometimes produce them.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: Add Managed identity auth configuration [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on PR #39321:
URL: https://github.com/apache/arrow/pull/39321#issuecomment-1865315975
Looks like merging workload identity caused merge conflicts. I'll fix it tomorrow.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #39321:
URL: https://github.com/apache/arrow/pull/39321#discussion_r1435268313
##########
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:
Do we actually need to make `account_name_` public though? Personally I would just make `account_name` a required argument of the AzureOptions constructor.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on PR #39321:
URL: https://github.com/apache/arrow/pull/39321#issuecomment-1868311583
the MINGW failure is just `pacman` failing right away (re-run didn't fix it), so I merged the PR.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #39321:
URL: https://github.com/apache/arrow/pull/39321#discussion_r1435020556
##########
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,
+ std::string const& clientId) {
Review Comment:
Sorry, that's another case of not paying enough attention. Its fixed now.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #39321:
URL: https://github.com/apache/arrow/pull/39321#discussion_r1435276754
##########
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:
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?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #39321:
URL: https://github.com/apache/arrow/pull/39321#discussion_r1435276754
##########
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:
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?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #39321:
URL: https://github.com/apache/arrow/pull/39321#discussion_r1435264186
##########
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:
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. https://github.com/flyteorg/flyte/issues/3962
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #39321:
URL: https://github.com/apache/arrow/pull/39321#discussion_r1434619551
##########
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:
Good catch. Sorry, I have not been paying enough attention :man_facepalming: . I think @felipecrv refactored this slightly. I need to check how the account name is configured now. I'll do it tomorrow.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "kou (via GitHub)" <gi...@apache.org>.
kou commented on code in PR #39321:
URL: https://github.com/apache/arrow/pull/39321#discussion_r1434623385
##########
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:
Thanks.
It seems that both of `ConfigureWorkloadIdentityCredential()` and `ConfigureDefaultCredential()` are the same situation too. Could you check them too?
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #39321:
URL: https://github.com/apache/arrow/pull/39321#discussion_r1435261766
##########
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:
I did it again :disappointed:. Fixed now.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "Tom-Newton (via GitHub)" <gi...@apache.org>.
Tom-Newton commented on code in PR #39321:
URL: https://github.com/apache/arrow/pull/39321#discussion_r1435264840
##########
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:
That sounds good to me :+1:
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-39320: [C++][FS][Azure] Add managed identity auth configuration [arrow]
Posted by "felipecrv (via GitHub)" <gi...@apache.org>.
felipecrv commented on code in PR #39321:
URL: https://github.com/apache/arrow/pull/39321#discussion_r1435337585
##########
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:
No problem.
--
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: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org