Add KeyVault Certificate Functions #334
Conversation
|
|
There was a problem hiding this comment.
Overall, the implementation looks good. Most of the requested changes are to align with our other SDKs, since we consistency after idiomacy (and naming - apart from casing - is rarely idiomatic). The biggest change, though, is to define a CertificateProperties and use that in various structs and methods, which actually makes calling the code much easier and tested well in UX studies. This is fairly consistent across our SDKs - even beyond Key Vault.
|
Thanks for your comments Heath, I (think) I've addressed all but the Let me know if anything else needs changing, potentially (probably) there are some cleaner ways to address some of the fixes I've implemented. Thanks again. |
Billy-Sheppard
force-pushed
the
add-kv-certificate
branch
from
20eb3c6 to
2e63df1
Compare
|
Apologies for the messy history, it took me a while to understand the ins and outs of the code, especially the tests, sorry, but I think I have resolved all the changes requested. However, I am unsure on how to mark them properly? Is marking comments as resolved incorrect? Cheers |
There was a problem hiding this comment.
So sorry. I started reviewing this a while ago, probably got distracted, and it fell off my radar.
I like the changes, but there's a few things I'd like to see changed first. I've noted some other places where it would great if you could clarify (all the auth in the client) or just open tracking issues, but only a couple likely small code changes to this PR. The others are more things I think we need to discuss further, so tracking issues are okay.
I'll keep a keen eye out for updates. Again, so sorry this fell off my radar. Feel free to ping if ever that happens.
sdk/key_vault/src/client.rs
Outdated
| /// let creds = DefaultCredential::default(); | ||
| /// let client = CertificateClient::new("test-key-vault.vault.azure.net", &creds).unwrap(); | ||
| /// ``` | ||
| pub fn new(vault_url: &str, token_credential: &'a T) -> Result<Self, Error> { |
sdk/key_vault/src/client.rs
Outdated
| Ok(client) | ||
| } | ||
|
|
||
| pub(crate) async fn refresh_token(&mut self) -> Result<(), Error> { |
There was a problem hiding this comment.
Note: Perhaps not now unless azure_identity is already doing this, but the TokenCredential and our challenge-based auth policy - which we may not yet have for KV - should handle this.
/cc @MindFlavor
There was a problem hiding this comment.
Hmm, looking further below, there's a lot of auth going on here that has me concerned about duplication. I wouldn't hold up the PR for this, but could you clarify or maybe open an issue if this is solving some deficiency in azure_identity? Apart from challenge-based auth with Key Vault and Managed HSM need (as well as Attestation and a couple other services we'll be onboarding for other languages), no auth - apart from attaching headers/etc - should be done in clients.
|
|
@Billy-Sheppard just a tip: when resolving comments (with or without change, though in the latter please explain why) it helps to resolve the conversation (comment) so I can tell what/how each PR comment is resolved. I'm in a bit of a crunch this week but will look at this soon and resolve with remaining open comments. |
|
Thanks Heath, I've marked the ones I fixed as resolved. I'll await further discussion on the remaining points. |
|
Could you resolve the conflicts and I'll take a look? I expect a few significant changes. |
Billy-Sheppard
force-pushed
the
add-kv-certificate
branch
from
2e399e3 to
2a9e87d
Compare
This reverts commit dc4c8bc.
Billy-Sheppard
force-pushed
the
add-kv-certificate
branch
from
2a9e87d to
1f6d133
Compare
Apologies if this was already worked on or out of spec, adapted directly from
secret.rs, welcome any improvements.enum RecoveryLevelto the key_vaultlib.rscertificate.rsbased directly offsecret.rsfn _update_certificate_policy()is not needed/required