feat: add token provider authorization to azure store #2374

roeap · 2022-08-08T17:20:23Z

Which issue does this PR close?

Rationale for this change

Allow a wider range of use by offering more ob the azure native auth methods. While this change might eventually becoem irrelevant, once we move away from SDKs, it is quite helpful in the meantime.

What changes are included in this PR?

~~Add an option to provide a TokenProvider credential via the MicrosoftAzureBuilder.~~
Add an options to provide client credentials via the MicrosoftAzureBuilder.

Are there any user-facing changes?

users now have more authorization options.

tustvold

Supporting token auth makes sense to me, even in a world where we move away from the SDK. I do wonder if we can hide the azure_core type from the interface though?

object_store/src/azure.rs

tustvold · 2022-08-08T17:24:31Z

object_store/src/azure.rs

+    /// Set a TokenCredential to be used for authentication (required - one of token_credential ot access key)
+    pub fn with_token_credential(
+        mut self,
+        token_credential: Arc<dyn TokenCredential>,


Is there some way we could avoid leaking the azure_core type into the public interface, this will not only make it easier to use, but also survive a move away from the SDK?

I haven't looked at the S§ implementation, but I seem to remember, that we do have some sense of a token credential as well. The problem I found with that, is that "get_token" accepts a client, while by now the client is somewhat hidden in the azure client. Also for gcs that was not a trait. Maybe thats the way to go?

An alternative might be to expose all (some) options required to initialize the token credentials. I guess we would always need that information regardless of the underlying implementation ..

I'm not familiar enough with the various authentication options available in Azure-land to know the implications of asking this, but if we can expose the information for the various options that would be ideal imo...

There are a few, but I can make that work ... also just realized I did this against the upcoming version, so I have to do some method renaming anyhow...

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

roeap · 2022-08-08T20:05:10Z

@tustvold - had to convert this back to draft, since the currently released version of the crates do not support identity based credentials just yet ... Maybe I'll wait until this gets released, and do the updates then, or just get it over with and migrate the implementation to a custom one in this crate ...

Which is also why there was not yet released APIs in use here :D

tustvold · 2022-08-10T16:10:30Z

object_store/Cargo.toml

-azure_core = { version = "0.2", optional = true, default-features = false, features = ["enable_reqwest_rustls"] }
-azure_storage = { version = "0.2", optional = true, default-features = false, features = ["account"] }
-azure_storage_blobs = { version = "0.2", optional = true, default-features = false, features = ["enable_reqwest_rustls"] }
+azure_core = { version = "0.4", optional = true, default-features = false, features = [


FYI, I'm not sure what is formatting these, but I think it makes the file harder to read...

I'll make my prettier less eager :)

object_store/src/azure.rs

roeap · 2022-08-10T17:03:28Z

The latest version of the Azure crates finally got released. Since there were many api changes between the last attempt (#2193) I did not update that PR but included it all here. The relevant update - authorizing via identity - is also part of the latest azure release.

Working on this made me realize once more that we really want to use our own, more focussed implementation of API interactions. I will take a look at that asap. in the meantime i was hoping to get the token auth in the next release, so while a bit finicky in some places, using the SDKs was more realistic to get doe in time (hopefully :)).

tustvold

This looks good to me, thank you 🎉

I'm going to hold off on merging this whilst the ongoing object_store 0.4 release process trundles along, just in case there are any bumps along the way (it is the first release following the Apache process). I'll then get this in

Working on this made me realize once more that we really want to use our own, more focussed implementation of API interactions. I will take a look at that asap.

Very exciting, btw if you haven't seen #2352 that moves away from rusuto for S3, so the Azure SDKs are the only ones that still remain.

object_store/src/azure.rs

tustvold · 2022-08-10T17:10:44Z

object_store/src/azure.rs

+                AzureErrorKind::HttpResponse {
+                    status: StatusCode::NotFound,
+                    ..
+                } => crate::Error::NotFound {


object_store/src/azure.rs

tustvold · 2022-08-10T17:19:09Z

There do appear to be failing lints and tests btw

roeap · 2022-08-10T21:47:41Z

object_store/src/azure.rs

+        let first = stream.next().await.transpose()?.unwrap_or_default();
+        Ok(GetResult::Stream(Box::pin(
+            futures::stream::once(async { Ok(first) }).chain(stream),
+        )))


Somehow this felt a bit strange to collect the first element just to add it to a stream right after, but I saw no other obvious way to have the method raise an error. Before this we are never actually touching any results...

roeap · 2022-08-10T21:55:50Z

There do appear to be failing lints and tests btw

All tests and lints should be passing now.

Very exciting, btw if you haven't seen #2352 that moves away from rusuto for S3, so the Azure SDKs are the only ones that still remain.

Very exciting indeed - one more reason to get this done and have everything nice and clean :)

ursabot · 2022-08-13T21:11:31Z

Benchmark runs are scheduled for baseline = f0d7d0b and contender = 12a9d84. 12a9d84 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

feat: add token provider authorizatiojn to azure store

891d412

github-actions bot added the object-store Object Store Interface label Aug 8, 2022

roeap changed the title ~~feat: add token provider authorizatiojn to azure store~~ feat: add token provider authorization to azure store Aug 8, 2022

tustvold reviewed Aug 8, 2022

View reviewed changes

Apply suggestions from code review

1c2fd68

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

roeap marked this pull request as draft August 8, 2022 20:03

feat: adpot latest APIs from altest version

f14bf1b

tustvold reviewed Aug 10, 2022

View reviewed changes

roeap added 2 commits August 10, 2022 18:32

chore: clippy

bf1505f

fix: lifetime issue

fe1d735

roeap commented Aug 10, 2022

View reviewed changes

object_store/src/azure.rs Show resolved Hide resolved

chore: better errors and docs

9bde022

roeap marked this pull request as ready for review August 10, 2022 16:56

roeap requested a review from tustvold August 10, 2022 17:05

chore: fmt whitespace

003799d

tustvold approved these changes Aug 10, 2022

View reviewed changes

roeap added 2 commits August 10, 2022 23:34

fix: firce first error in get method

eb2a298

chore: avoid unwrapping some options

3a340a0

roeap commented Aug 10, 2022

View reviewed changes

tustvold mentioned this pull request Aug 11, 2022

object_store: update azure dependencies #2193

Closed

tustvold merged commit 12a9d84 into apache:master Aug 13, 2022

roeap deleted the azure-auth branch August 13, 2022 22:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add token provider authorization to azure store #2374

feat: add token provider authorization to azure store #2374

roeap commented Aug 8, 2022 •

edited

Loading

tustvold left a comment

tustvold Aug 8, 2022

roeap Aug 8, 2022

tustvold Aug 8, 2022

roeap Aug 8, 2022

roeap commented Aug 8, 2022 •

edited

Loading

tustvold Aug 10, 2022

roeap Aug 10, 2022

roeap commented Aug 10, 2022

tustvold left a comment •

edited

Loading

tustvold Aug 10, 2022

tustvold commented Aug 10, 2022

roeap Aug 10, 2022

roeap commented Aug 10, 2022

ursabot commented Aug 13, 2022

feat: add token provider authorization to azure store #2374

feat: add token provider authorization to azure store #2374

Conversation

roeap commented Aug 8, 2022 • edited Loading

Which issue does this PR close?

Rationale for this change

What changes are included in this PR?

Are there any user-facing changes?

tustvold left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roeap commented Aug 8, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roeap commented Aug 10, 2022

tustvold left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tustvold commented Aug 10, 2022

Choose a reason for hiding this comment

roeap commented Aug 10, 2022

ursabot commented Aug 13, 2022

roeap commented Aug 8, 2022 •

edited

Loading

roeap commented Aug 8, 2022 •

edited

Loading

tustvold left a comment •

edited

Loading