Using TokenCredential trait in KeyVaultClient #10

gzp79 · 2020-10-09T14:43:05Z

resolve #5

ClientSecretCredential is used in examples to have the least change (the same env should result in the same response), but for the doc-examples I've used the DefaultCredentail.

As it is a breaking change the semver should be updated before releasing a new version of the crate !

ghost · 2020-10-09T14:43:19Z

All CLA requirements met.

sdks/keyvault/src/client.rs

sdks/keyvault/Cargo.toml

ctaggart · 2020-10-10T22:13:20Z

Can you merge in latest from master to turn CI green?

gzp79 · 2020-10-12T10:24:02Z

Can you merge in latest from master to turn CI green?

I think CI also depends on test, but I need to know what is the plan. If DeafaultCrendentials should be used or the ClientSecretCredential and preserve the dependency on the ENV as it is right now.

ctaggart

Looks great! Just moving async-trait is all I noticed.

sdks/keyvault/Cargo.toml

rylev

Looks good. I have some questions, but nothing that should prevent merging.

sdks/keyvault/Cargo.toml

rylev · 2020-10-14T11:00:15Z

sdks/keyvault/src/client.rs

-    pub(crate) aad_tenant_id: &'a str,
+pub struct KeyVaultClient<'a, T> {
+    pub(crate) token_credential: &'a T,
    pub(crate) keyvault_name: &'a str,


Why are we borrowing the keyvalue_name and token_credential instead of just taking ownership?

It was the original implementation and I left it as it was.

It would be ergonomic to leave the choice to the caller, for example using Cow. For example CosmosDB uses this pattern:

azure-sdk-for-rust/sdks/cosmos/src/clients/collection_struct.rs

Lines 24 to 37 in 8e5f133

impl<'a, C, D> CollectionStruct<'a, C, D>

where

C: CosmosClient + Clone,

D: DatabaseClient<C> + Clone,

{

#[inline]

pub(crate) fn new(database_client: Cow<'a, D>, collection_name: Cow<'a, str>) -> Self {

Self {

database_client,

collection_name,

p_c: PhantomData {},

}

}

}

The only limitation is the cloneability of the inner type, something like:

pub struct KeyVaultClient<'a, T: Clone> { pub(crate) token_credential: Cow<'a, T>,

Or we can try the MaybeOwned crate that relaxes this limitation. We should probably leave this as a separate PR.

I'm not sure we really need to care too deeply about the performance implications here. Taking a String and risky some additional heap allocations is fine IMO

rylev · 2020-10-14T11:01:43Z

sdks/keyvault/src/client.rs

-        aad_tenant_id: &'a str,
+        token_credential: &'a T,
        keyvault_name: &'a str,
        endpoint_suffix: String,


I know this was like this before, but since we're feeding this into a format! macro, it seems more wise to take endpoint_suffix as a &str.

Same as above, original code. I don't know about original intents.

I agree, an oversight on my end.

rylev · 2020-10-14T11:03:31Z

sdks/keyvault/src/client.rs


    pub(crate) async fn refresh_token(&mut self) -> Result<(), KeyVaultError> {
-        if matches!(self.token_expiration, Some(exp) if exp > chrono::Utc::now()) {
+        if matches!(&self.token, Some(token) if token.expires_on > chrono::Utc::now()) {


Just curious, should we provide some sort of buffer where we refresh the token even if its technically still valid? If the token is valid for 1ns more we won't refresh even though we arguably should.

I made no logic change here either. It was the original implementation.

@rylev I kept it simple, refreshing the token would be a great improvement.

gzp79 · 2020-10-14T12:50:20Z

Looks good. I have some questions, but nothing that should prevent merging.

@rylev if you have issues with the above and have suggestion how to fix/improve them, could you please create a ticket ?
If I have the time I may take a look at it. I wanted to change no logic only perform as much modification as required to allow managed identity and other type of authentication to work smoothly with the keyvault. You might also consider adding the removal of unwrap from the token secret usages and let refresh_token return it. Thus the number of unwrap greatly decreases in the crate.

rylev · 2020-10-14T14:06:36Z

@gzp-crey sorry, those comments weren't really meant for you since they aren't directly about your changes. I just wanted to mark them so I wouldn't figure them. I can make issues for them.

gzp added 4 commits October 9, 2020 16:26

Make TokenCredential a generic parameter of keyvault

60633a2

fix resource principal name

f57d0b0

fix resource principal name

2798cdb

bump version for the breaking change

ac6254d

gzp79 changed the title ~~Issue 5~~ Using TokenCredential trait in KeyVaultClient Oct 9, 2020

ctaggart reviewed Oct 10, 2020

View reviewed changes

sdks/keyvault/src/client.rs Outdated Show resolved Hide resolved

sdks/keyvault/Cargo.toml Outdated Show resolved Hide resolved

gzp added 3 commits October 12, 2020 08:42

Merge remote-tracking branch 'upstream/master' into issue_5

bc9450f

use .bearer_auth instead of direct header manipultion

a1aa2e7

revert version to 0.1.0

b5edecc

gzp79 requested a review from ctaggart October 12, 2020 07:13

gzp added 3 commits October 13, 2020 15:40

update examples and test (WIP)

a69fc9b

Merge remote-tracking branch 'upstream/master' into issue_5

2c2d754

fix examples and tests

536ed2c

gzp79 marked this pull request as ready for review October 13, 2020 14:10

ctaggart suggested changes Oct 14, 2020

View reviewed changes

sdks/keyvault/Cargo.toml Outdated Show resolved Hide resolved

move async-trait into dev-dep

60d7eeb

gzp79 requested a review from ctaggart October 14, 2020 06:36

Merge remote-tracking branch 'upstream/master' into issue_5

09968a5

rylev approved these changes Oct 14, 2020

View reviewed changes

rylev requested a review from guywaldman October 14, 2020 11:09

gzp79 requested a review from rylev October 14, 2020 12:42

rylev approved these changes Oct 14, 2020

View reviewed changes

rylev mentioned this pull request Oct 14, 2020

Rename Crates #34

Merged

ctaggart approved these changes Oct 14, 2020

View reviewed changes

rylev merged commit 9fc1dea into Azure:master Oct 14, 2020

snakehand mentioned this pull request May 13, 2021

Panic when reading blob with stream that has chunk size larger than file #245

Closed

	impl<'a, C, D> CollectionStruct<'a, C, D>
	where
	C: CosmosClient + Clone,
	D: DatabaseClient<C> + Clone,
	{
	#[inline]
	pub(crate) fn new(database_client: Cow<'a, D>, collection_name: Cow<'a, str>) -> Self {
	Self {
	database_client,
	collection_name,
	p_c: PhantomData {},
	}
	}
	}

Using TokenCredential trait in KeyVaultClient #10

Using TokenCredential trait in KeyVaultClient #10

Uh oh!

Conversation

gzp79 commented Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Oct 9, 2020 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ctaggart commented Oct 10, 2020

Uh oh!

gzp79 commented Oct 12, 2020

Uh oh!

ctaggart left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rylev left a comment

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!

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!

gzp79 commented Oct 14, 2020

Uh oh!

rylev commented Oct 14, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gzp79 commented Oct 9, 2020 •

edited

Loading

ghost commented Oct 9, 2020 •

edited by ghost

Loading