[datalake] More Authorization policies #673

roeap · 2022-03-02T21:57:41Z

This PR adds more authorization options to the datalake crate. Specifically using a token credential.

In many scenarios having auto refresh available for tokens is quite handy. Thus I added an auto refreshing token credential, which wraps around token credentials from the core crate. To get the refresh to work, I had to use the tokio Mutex - I believe this is required due to the refresh being an async operation and the regular mutex cannot be used across an await statement. But I may also just have missed the simpler solution. Due to that I put the implementation behind a feature gate, since I seem to remember reading pulling in a specific runtime as dependency was not desired.

I still need to add tests and clean up a bit, but would appreciate some feedback on the general approach - spcifically the use of tokio.

@thovoll @rylev @ctaggart

Update: usage of futures::lock::Mutext solves the Tokio issue (thanks @bmc-msft)

bmc-msft · 2022-03-03T04:14:40Z

Are you able to use futures::lock::Mutex for a runtime agnostic Mutex implementation?

roeap · 2022-03-03T06:30:39Z

Indeed, that works :) Thanks for the advice!

of course the next question follows suit... when returning an error inside the "refresh loop" I could not get it to work to return a Box'ed error due to lifetime constraints. The new errors using an ErrorKind and message would work fine, but this would require adjusting the return type of the TokenCredential.get_token function. Also I was hesitant to introduce a new variant to the "old" errors, and changing the signature would have a much bigger impact. Making the errors clone-able could also work I guess.

So again asking for some guidance.

Right now an HeaderNotFound erro is returned since it only needs a string as input, but that obviously is more misleading then helpful :).

yoshuawuyts · 2022-03-08T20:28:09Z

Also I was hesitant to introduce a new variant to the "old" errors

I discussed this with @thovoll today, and we think this would be the best option. There is no "right" answer here, but this would allow us to start incrementally migrating, the way we've been doing with other types as well.

Also we on reading the code we realized using Mutex might cause contention between readers; instead using an async RwLock might make reads cheaper.

Hope that's helpful!

roeap · 2022-03-09T05:39:32Z

Thanks @yoshuawuyts, @thovoll for the support. The code now uses RwLock from the newly introduced async_lock crate (did not find one in futures), and also added a new variant to the error enum.

The last remaining concerns was testing - locally I gave the credential a try and it seemed to work. Looked a bit around the identity crate and found that specific credentials are not tested there as well - not sure how to proceed with this.

Otherwise I hope we are approaching a merge-able state :).

yoshuawuyts · 2022-03-09T10:13:36Z

I'm not super familiar with how we're testing things; perhaps @cataggar can answer that question?

thovoll · 2022-03-09T14:17:59Z

sdk/core/src/errors.rs

@@ -42,6 +42,8 @@ pub enum Error {
    Json(#[source] serde_json::Error),
    #[error("Other error")]
    Other(#[source] Box<dyn std::error::Error + Send + Sync + 'static>),
+    #[error("Other error")]


Better description?

Better description need here

cataggar · 2022-03-09T17:31:04Z

sdk/storage_datalake/src/authorization_policies/auto_refreshing_token.rs

+    }
+}
+
+impl AutoRefreshingTokenCredential {


Shouldn't this go in azure_core?

It will eventually. I know @roeap wanted to do it in stages.

I completely agree :). If we can move this there now, great. Similarly, the pipeline authorization policies should likely end up at least in azure_storage, or if they are useful in other crates as well maybe also core?

If this is OK, I am happy to move this to core in this PR.

Moved the AutoRefreshingCredential into the identity crate to the other TokenCredentials. Was considering to also move the policies to the storage_core crate, but thought it better to do this when migrating blob to the pipeline architecture.

thovoll · 2022-03-11T12:58:24Z

I'm not super familiar with how we're testing things; perhaps @cataggar can answer that question?

@roeap and I discussed the testing plan and came up with these incremental steps:

Add an example (this PR)
Add an e2e record/playback test (maybe another PR)
Add a deterministic test for the loop with the RwLock (definitely another PR)

roeap · 2022-03-11T22:02:32Z

Moved the refreshing credential in the identity crate and updated the client constructors to also offer an option for using a token credential. With respect to client initialization I tried to keep changes minimal. Main reason being that from what I gather discussion is still ongoing on what the specifics of client configuration should look like.

cataggar · 2022-03-14T20:17:17Z

sdk/identity/src/token_credentials/auto_refreshing_credentials.rs

+                    *guard = Some(res);
+                }
+                Some(Err(err)) => {
+                    return Err(Error::AuthorizationPolicy(err.to_string()));


err.to_string does not feel right

I'm hoping @rylev has better advice for a solution.

agreed. my understanding was that this is sort of a stop gap solution, until we are migrating to the new error structure. Given that authorization is quite pervasive throughout all crates, maybe it a good idea to do that rather sooner then later?

I suggested above changing the error to take a boxed error trait object which would clean this up a bit.

cataggar · 2022-03-14T20:17:35Z

sdk/core/src/errors.rs

@@ -42,6 +42,8 @@ pub enum Error {
    Json(#[source] serde_json::Error),
    #[error("Other error")]
    Other(#[source] Box<dyn std::error::Error + Send + Sync + 'static>),
+    #[error("Other error")]


Better description need here

rylev

Just a few comments but largely this looks good.

rylev · 2022-03-16T11:09:49Z

sdk/core/src/errors.rs

+    #[error("Authorization policy error")]
+    AuthorizationPolicy(String),


Suggested change

#[error("Authorization policy error")]

AuthorizationPolicy(String),

#[error("authorization policy error: {0}")]

AuthorizationPolicy(Box<dyn std::error::Error + Send + Sync + 'static>),

Nit: can you also move this above the Other variant so that Other is last?

Switched the variants, but was not able to work with the boxed error. See comment below.

rylev · 2022-03-16T11:10:52Z

sdk/core/src/error/azure_core_errors.rs

@@ -19,6 +19,10 @@ impl From<crate::errors::Error> for Error {
            crate::errors::Error::GetToken(e) => Error::new(ErrorKind::Credential, e),
            crate::errors::Error::HttpPrepare(e) => e.into(),
            crate::errors::Error::Other(e) => Error::new(ErrorKind::Other, e),
+            crate::errors::Error::AuthorizationPolicy(msg) => Error::with_message(


If AuthorizationPolicy takes a boxed error trait object, you can just use Error::new instead of Error::with_message which doesn't really add much value.

It seems that when working with the guard from RwLock, we need to get a reference (guard.as_ref()) , which in turn ties the lifetime of the error to the TokenCredential itself, which then would need to be constrained by the 'static lifetime.

At least that's more or less what i gathered from the compiler errors, I get when trying to pass a box'ed error into the variant. Tried around different variations of de-referencing the error, but with no success. Somehow these lifetimes still manage to escape my grasp every so often :).

rylev · 2022-03-16T11:12:18Z

sdk/identity/src/token_credentials/auto_refreshing_credentials.rs

+                    *guard = Some(res);
+                }
+                Some(Err(err)) => {
+                    return Err(Error::AuthorizationPolicy(err.to_string()));


I suggested above changing the error to take a boxed error trait object which would clean this up a bit.

sdk/identity/src/token_credentials/auto_refreshing_credentials.rs

sdk/storage_datalake/src/lib.rs

rylev

Thanks for working hard on this! I still don't love the way that errors are being handled, but since we'll do a pass on error handling when we want to introduce the new error handling style, I think this is fine to merge.

Dismissing this review since it was asking for the same things I was asking for.

roeap · 2022-03-17T16:02:51Z

Agreed with the errors, but was not able to get the box thing to work..

roeap changed the title ~~[datalake] More Authrization policies~~ [datalake] More Authorization policies Mar 2, 2022

cataggar requested a review from yoshuawuyts March 3, 2022 16:16

thovoll requested a review from rylev March 8, 2022 16:18

roeap marked this pull request as ready for review March 9, 2022 05:39

thovoll approved these changes Mar 9, 2022

View reviewed changes

cataggar reviewed Mar 9, 2022

View reviewed changes

roeap added 8 commits March 11, 2022 18:32

stash some changes

46036af

auto refreshing credential

b013d8f

make new public

f7c1d0d

use futures::lock::Mutex

7497277

handle PR comments

9545bb7

clippy compliant version

97508ec

cleaner RwLock usage

2cc4d2b

move auto-refreshing credential to identity crate

1b22b60

roeap force-pushed the authorization branch from 65b9bcc to 1b22b60 Compare March 11, 2022 17:41

roeap added 2 commits March 11, 2022 22:35

refactor client constructors

6f5221b

fix test

0ae21e3

cataggar previously requested changes Mar 14, 2022

View reviewed changes

better erro description

2b8fa08

rylev suggested changes Mar 16, 2022

View reviewed changes

roeap added 2 commits March 16, 2022 21:26

avoid cloning token in exiry checks

22b648d

auth policy exports

22f0db4

rylev approved these changes Mar 17, 2022

View reviewed changes

rylev merged commit 0ed3e65 into Azure:main Mar 17, 2022

roeap deleted the authorization branch March 17, 2022 16:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[datalake] More Authorization policies #673

[datalake] More Authorization policies #673

roeap commented Mar 2, 2022 •

edited

Loading

bmc-msft commented Mar 3, 2022 •

edited

Loading

roeap commented Mar 3, 2022 •

edited

Loading

yoshuawuyts commented Mar 8, 2022

roeap commented Mar 9, 2022

yoshuawuyts commented Mar 9, 2022

thovoll Mar 9, 2022

cataggar Mar 14, 2022

cataggar Mar 9, 2022

thovoll Mar 9, 2022

roeap Mar 9, 2022

roeap Mar 11, 2022

thovoll commented Mar 11, 2022

roeap commented Mar 11, 2022

cataggar Mar 14, 2022

cataggar Mar 14, 2022

roeap Mar 15, 2022

rylev Mar 16, 2022

cataggar Mar 14, 2022

rylev left a comment

rylev Mar 16, 2022

roeap Mar 16, 2022

rylev Mar 16, 2022

roeap Mar 16, 2022

rylev Mar 16, 2022

rylev left a comment

roeap commented Mar 17, 2022

		#[error("Authorization policy error")]
		AuthorizationPolicy(String),

[datalake] More Authorization policies #673

[datalake] More Authorization policies #673

Conversation

roeap commented Mar 2, 2022 • edited Loading

bmc-msft commented Mar 3, 2022 • edited Loading

roeap commented Mar 3, 2022 • edited Loading

yoshuawuyts commented Mar 8, 2022

roeap commented Mar 9, 2022

yoshuawuyts commented Mar 9, 2022

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

Choose a reason for hiding this comment

thovoll commented Mar 11, 2022

roeap commented Mar 11, 2022

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

rylev 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

Choose a reason for hiding this comment

rylev left a comment

Choose a reason for hiding this comment

roeap commented Mar 17, 2022

roeap commented Mar 2, 2022 •

edited

Loading

bmc-msft commented Mar 3, 2022 •

edited

Loading

roeap commented Mar 3, 2022 •

edited

Loading