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

Tom-Newton · 2023-12-20T13:11:50Z

Rationale for this change

Workload identity is a useful Azure authentication method. Also I failed to set the account_name correctly for a bunch of auths (I think this got lost in a rebase then I copy pasted the broken code).

What changes are included in this PR?

Make filesystem initialisation fail if account_name_.empty(). This prevents the account name configuration bug we had. Also added a test asserting that filesystem initialization fails in this case.
Remove account name configuration on all auth configs, in favour of setting in separately from the auth configuration.
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 #39263.

Are there any user-facing changes?

Managed identity authentication is now supported.

Closes: [C++][FS][Azure] Managed Identity authentication #39320

Tom-Newton · 2023-12-21T00:33:55Z

Looks like merging workload identity caused merge conflicts. I'll fix it tomorrow.

cpp/src/arrow/filesystem/azurefs.cc

kou · 2023-12-22T00:42:02Z

cpp/src/arrow/filesystem/azurefs.cc

It seems that account_name isn't used. Is it needed?

Good catch. Sorry, I have not been paying enough attention 🤦‍♂️ . I think @felipecrv refactored this slightly. I need to check how the account name is configured now. I'll do it tomorrow.

Thanks.
It seems that both of ConfigureWorkloadIdentityCredential() and ConfigureDefaultCredential() are the same situation too. Could you check them too?

Fixed now. I have added a check so that filesystem initialisation fails if account_name_.empty().

Sorry. I messed up and forgot to set account_name_ on the Configure... functions.

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?

That sounds good to me 👍

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 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.

Ok. I think its as you recommended now.

felipecrv · 2023-12-22T17:34:34Z

cpp/src/arrow/filesystem/azurefs.cc

Sorry. I messed up and forgot to set account_name_ on the Configure... functions.

felipecrv · 2023-12-22T17:39:32Z

cpp/src/arrow/filesystem/azurefs.cc

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?

felipecrv · 2023-12-22T17:49:22Z

cpp/src/arrow/filesystem/azurefs.cc

    const {
+  if (account_name_.empty()) {
+    return Status::Invalid("AzureOptions doesn't contain a valid account name");
+  }


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?

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

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?

I looked and there is a constructor that doesn't take any credentials. I guess that's what we should call :-)

👍 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 😅

No problem.

cpp/src/arrow/filesystem/azurefs.h

felipecrv · 2023-12-23T15:04:22Z

the MINGW failure is just pacman failing right away (re-run didn't fix it), so I merged the PR.

conbench-apache-arrow · 2023-12-23T18:34:11Z

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

There was 1 benchmark result indicating a performance regression:

Commit Run on ursa-i9-9960x at 2023-12-23 15:45:55Z
- tpch (R) with engine=arrow, format=native, language=R, memory_map=False, query_id=TPCH-08, scale_factor=1

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

…ion (apache#39321) ### Rationale for this change Workload identity is a useful Azure authentication method. Also I failed to set the account_name correctly for a bunch of auths (I think this got lost in a rebase then I copy pasted the broken code). ### What changes are included in this PR? - Make filesystem initialisation fail if `account_name_.empty()`. This prevents the account name configuration bug we had. Also added a test asserting that filesystem initialization fails in this case. - Remove account name configuration on all auth configs, in favour of setting in separately from the auth configuration. - 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 apache#39263. ### Are there any user-facing changes? Managed identity authentication is now supported. * Closes: apache#39320 Authored-by: Thomas Newton <[email protected]> Signed-off-by: Felipe Oliveira Carvalho <[email protected]>

github-actions bot added Component: C++ awaiting review Awaiting review labels Dec 20, 2023

kou changed the title ~~GH-39320: Add Managed identity auth configuration~~ GH-39320: [C++][FS][Azure] Add managed identity auth configuration Dec 21, 2023

Tom-Newton force-pushed the tomnewton/azure_managed_identity/GH-39320 branch from e71b386 to bdb0a00 Compare December 21, 2023 10:30

kou reviewed Dec 22, 2023

View reviewed changes

github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Dec 22, 2023

Tom-Newton added 3 commits December 22, 2023 12:22

Add managed identity

4bfacdd

Ensure filesystem initialisation fails without account name

f6e40aa

Fix broken auths

d4af840

Tom-Newton force-pushed the tomnewton/azure_managed_identity/GH-39320 branch from bdb0a00 to d4af840 Compare December 22, 2023 12:34

style fix

3a9b89f

github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Dec 22, 2023

Tom-Newton requested a review from kou December 22, 2023 12:41

felipecrv requested changes Dec 22, 2023

View reviewed changes

Tom-Newton added 4 commits December 22, 2023 18:06

another code style fix

414f582

Move account_name to AzureOptions constructor

de0a869

Revert AzureOptions constructor. Just make account_name a public member.

8cdcf4b

Add a test asserting initialisation fails without an account name

bfa2cc5

Tom-Newton requested a review from felipecrv December 22, 2023 19:59

felipecrv approved these changes Dec 22, 2023

View reviewed changes

felipecrv merged commit d519544 into apache:main Dec 23, 2023

felipecrv removed the awaiting change review Awaiting change review label Dec 23, 2023

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

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

Uh oh!

Conversation

Tom-Newton commented Dec 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Rationale for this change

What changes are included in this PR?

Are these changes tested?

Are there any user-facing changes?

Uh oh!

Tom-Newton commented Dec 21, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tom-Newton Dec 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kou Dec 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

felipecrv Dec 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tom-Newton Dec 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

felipecrv commented Dec 23, 2023

Uh oh!

conbench-apache-arrow bot commented Dec 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Tom-Newton commented Dec 20, 2023 •

edited

Loading

Tom-Newton Dec 22, 2023 •

edited

Loading

kou Dec 22, 2023 •

edited

Loading

felipecrv Dec 22, 2023 •

edited

Loading

Tom-Newton Dec 22, 2023 •

edited

Loading