[object_store] support azure managed and workload identities by roeap · Pull Request #3581 · apache/arrow-rs

roeap · 2023-01-21T14:01:48Z

Which issue does this PR close?

Rationale for this change

see linked issue

What changes are included in this PR?

added implementations for a managed identity and federated workload credential for azure along with corresponding configuration on the azure builder.

Are there any user-facing changes?

users can now use managed identities and workload credentials to authenticate object store requests.

tustvold · 2023-01-22T20:30:16Z

I intend to review this first thing tomorrow

roeap · 2023-01-23T09:29:10Z

object_store/src/azure/credential.rs

+            assert_eq!(req.uri().path(), format!("/{}/oauth2/v2.0/token", tenant));
+            assert_eq!(req.method(), &Method::POST);


One thing I really wanted to check here is if the right content from the federated token file is included in the client_assertion. However getting the body bytes is an async operation. We could probably get a tokio runtime internally, but I was wondering if there is a better way to achieve this.

You could use futures::block_on

tustvold

Mostly just some minor nits, looks good thank you 👍

tustvold · 2023-01-22T17:54:45Z

object_store/src/azure/credential.rs

        client_id: String,
        client_secret: String,
-        tenant_id: String,
+        tenant_id: impl AsRef<str>,


tustvold · 2023-01-23T14:18:59Z

object_store/src/azure/credential.rs

    /// Fetch a token
-    pub async fn fetch_token(
+    async fn fetch_token(&self, client: &Client, retry: &RetryConfig) -> Result<String> {
+        self.cache


What do you think about moving the TokenCache onto CredentialProvider::TokenProvider?

makes absolute sense - done.

tustvold · 2023-01-23T14:22:28Z

object_store/src/azure/credential.rs

-#[derive(serde::Deserialize, Debug)]
+#[async_trait::async_trait]
+pub trait TokenCredential: std::fmt::Debug + Send + Sync + 'static {
+    async fn fetch_token(&self, client: &Client, retry: &RetryConfig) -> Result<String>;


I think the client should be passed at credential construction, this avoids issues with clients not permitting https

I passed in a client for the managed identity, but I was unsure about the other credentials as they always use ssl. So we just ignore the client for the managed identity. If however the "savings" of not having a redundant instance of the client are not relevant, I'm happy to move it for all credentials.

tustvold · 2023-01-23T14:25:23Z

object_store/src/azure/credential.rs

+            (None, None, Some(msi_res_id)) => {
+                query_items.push(("msi_res_id", msi_res_id))
+            }
+            _ => (),


I think this silently ignores parameters if multiple are specified, I can't see why this would be the case. If this is intentional I think we should return an error for this case

True - I went for just having a hierarchy. Since object id and msi_res_id are more specific (only used by managed identity) and less likely to just "end up" in the environment, I thought they would get precedence.

tustvold · 2023-01-23T14:26:02Z

object_store/src/azure/credential.rs

+            .header("metadata", "true")
+            .query(&query_items);
+
+        if let Ok(val) = std::env::var(MSI_SECRET_ENV_KEY) {


I think it would be better to provide this in the constructor, and delegate to the config key system

I was not sure about this value. From what I understood, its rotated at runtime.

Unless I'm mistaken, environment variables can't reliably be altered at runtime by an external process. So I think it should be fine to move it outside

tustvold · 2023-01-23T14:34:43Z

object_store/src/azure/credential.rs

+        let fs = LocalFileSystem::new_with_prefix(root.path()).unwrap();
+        let tenant = "tenant";
+        let tokenfile = root.path().join("tokenfile");
+        fs.put(
+            &crate::path::Path::from("tokenfile"),
+            bytes::Bytes::from("federated-token"),
+        )
+        .await
+        .unwrap();


Why not just use NamedTempFile here?

good point :)

tustvold · 2023-01-23T14:36:34Z

object_store/src/azure/credential.rs

+            assert_eq!(req.uri().path(), format!("/{}/oauth2/v2.0/token", tenant));
+            assert_eq!(req.method(), &Method::POST);


You could use futures::block_on

object_store/src/azure/mod.rs

tustvold · 2023-01-23T14:41:14Z

object_store/src/azure/credential.rs

+        msi_endpoint: Option<String>,
+    ) -> Self {
+        let msi_endpoint = msi_endpoint.unwrap_or_else(|| {
+            "http://169.254.169.254/metadata/identity/oauth2/token".to_owned()


A problem we had to workaround with the AWS provider, is ensuring that the client used for this is not https only. In practice this meant creating a separate client for it

completely missed that - using a dedicated client here as well now.

tustvold · 2023-01-23T14:43:24Z

object_store/src/azure/mod.rs

                Ok(credential::CredentialProvider::AccessKey(bearer_token))
            } else if let Some(access_key) = self.access_key {
                Ok(credential::CredentialProvider::AccessKey(access_key))
+            } else if self.use_managed_identity {


I'm not familiar with Azure SDKs, but the AWS and Google SDKs default to using managed identity if nothing else is specified. I'm not sure if this is something we want to do here?

The default azure credential does a lot of opinionated things that I think we do not want to replicate here :). That said, it makes sense to use the metadata as default since it may work without any configuration.

Co-authored-by: Raphael Taylor-Davies <[email protected]>

tustvold · 2023-01-25T14:50:53Z

I don't have the means to test this, so I am going to take it on faith that this works as advertised 😄

ursabot · 2023-01-25T15:01:34Z

Benchmark runs are scheduled for baseline = f0be9da and contender = 73ce076. 73ce076 is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ec2-t3-xlarge-us-east-2] ec2-t3-xlarge-us-east-2
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on test-mac-arm] test-mac-arm
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-i9-9960x] ursa-i9-9960x
[Skipped ⚠️ Benchmarking of arrow-rs-commits is not supported on ursa-thinkcentre-m75q] ursa-thinkcentre-m75q
Buildkite builds:
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java

roeap · 2023-01-25T15:34:11Z

@tustvold - I also have to set up a real-world test and am in the process of doing this right now. Will open up a follow up PR if I see that changes need to be made. Also wanted to move that last variable :). Sorry for not mentioning that sooner!

…3581) * feat: add azure managed identity credential * test: azure managed identity credential * feat: add azure federated token credential * test: add workload identity test * refactor: PR feedback * Update object_store/src/azure/mod.rs Co-authored-by: Raphael Taylor-Davies <[email protected]> * refactor: id priorities * refactor: use managed identity as default credential * chore: remove usused parameter Co-authored-by: Raphael Taylor-Davies <[email protected]>

roeap added 3 commits January 21, 2023 12:45

feat: add azure managed identity credential

ca4f9fa

test: azure managed identity credential

249fde5

feat: add azure federated token credential

6b7b34e

github-actions bot added the object-store label Jan 21, 2023

test: add workload identity test

ee204a6

roeap commented Jan 23, 2023

View reviewed changes

tustvold reviewed Jan 23, 2023

View reviewed changes

roeap and others added 5 commits January 23, 2023 19:58

refactor: PR feedback

70ee67f

Update object_store/src/azure/mod.rs

9f2dffb

Co-authored-by: Raphael Taylor-Davies <[email protected]>

refactor: id priorities

5a437d1

refactor: use managed identity as default credential

8630d21

chore: remove usused parameter

70fca1a

tustvold approved these changes Jan 25, 2023

View reviewed changes

tustvold merged commit 73ce076 into apache:master Jan 25, 2023

rbushri mentioned this pull request Jan 26, 2023

write_deltalake - Support azure managed identities delta-io/delta-rs#1105

Closed

tustvold mentioned this pull request Jan 27, 2023

[object_store] support more identity based auth flows for azure apache/arrow-rs-object-store#198

Closed

roeap deleted the more-azure-credentials branch February 11, 2023 10:48

jychen7 mentioned this pull request Feb 26, 2023

Upgrade object_store to 0.5.4 for GCP/Azure workload identity roapi/roapi#262

Merged

tustvold mentioned this pull request May 10, 2023

Fix ImdsManagedIdentityProvider (#4096) #4193

Merged

		assert_eq!(req.uri().path(), format!("/{}/oauth2/v2.0/token", tenant));
		assert_eq!(req.method(), &Method::POST);

Conversation

roeap commented Jan 21, 2023

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

Uh oh!

tustvold commented Jan 22, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tustvold left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

tustvold commented Jan 25, 2023

Uh oh!

ursabot commented Jan 25, 2023

Uh oh!

roeap commented Jan 25, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants