Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Key Vault Secrets README #2047

Merged
merged 23 commits into from
Feb 5, 2025

Conversation

ronniegeraghty
Copy link
Member

Adding initial Key Vault Secrets create README.md.

Fixes #2040

@heaths heaths changed the title Ronniegeraghty/keyvault secrets readme update Update Key Vault Secrets README Feb 1, 2025
@ronniegeraghty ronniegeraghty marked this pull request as ready for review February 1, 2025 01:27
@heaths
Copy link
Member

heaths commented Feb 2, 2025

See https://github.com/Azure/azure-sdk-for-rust/pull/2051/files#diff-e68c23fe7f799586bbbcaf760ba0cd25ac804e07b08c0c4519192f0ae3d00a2a for some examples.

The pager needs work. Especially if we're making all fields Option<T>, which puts a burden on pageable responses where I think instead we should deserialize to an empty Vec instead of None. It also doesn't loop over all pages like it did in track 1 and I want to bring that back. Going through page by page necessarily is a pain and one I think will trip up some devs because, for Key Vault, listing resources could return an empty page but also a next_link. It happens. If they get no values they may terminate their loop early given how loops typically work i.e., Iterator::next() returns None for an item and they assume all items have been enumerated.

/cc @JeffreyRichter @RickWinter

@JeffreyRichter
Copy link
Member

I'm almost never a fan of SDK magic. The service offers a "get a page" operation and we need to expose this. The SDK can expose it with a pageable type that can optionally be used to get the nextLink page. In a single app, it is likely (but not mandatory; see example at bottom) that the app will loop over the page of items and then get the next page to loop of its results, and so on.

But knowing where in the code the high latency with many possible failures that might need recovery is happening (the get page operations) and knowing where in the code the low latency and no chance of failures (iterating over the page of items) is happening is critical especially in a language like Rust. Obfuscating this with some magical iterator that combines these 2 very different things is a mistake. If a customer wants to write code to obfuscate these 2 things to make their code "easier" (while hiding failure modes and latency issues), that is OK but our SDK should not encourage or force this obfuscation. I'll also add that a page with no items is actually how the service works, and we should not hide this either. We MUST document it, so customers write code properly to deal with an empty page, but we do not fix it for them (as ding so hide additional latency and failures).

Another reason not to magically conflate get-a-page & iterate-over-a-page is because it is also fairly common to have one PC initiate listing, process the page, and then another PC processes the next page. This is why our pager types have a way of getting a resume token and re-hydrating a pager from this resume token. I can give a real-life concrete example using 2 PCs and a resume token: Imagine an e-commerce website (like Amazon). The customer searches for some product, a server PC initiates the search, gets results and constructs an HTML page returning it to the browser. This webpage has the resume token embedded in it.

The customer now clicks a button to look at the next page of results. When they click this button, a new web request is sent back to the service with the resume token where it goes through a load balancer and reaches a different server PC. This different server PC needs to rehydrate the pager type from the resume token and get the next page in order to return a new HTML page with the 2nd page of results and the latest resume token embedded in it so the customer could click "next page" again if they desire.

For KeyVault, imagine a Portal (like the Azure Portal) showing 1 page of secrets/certificates at a time. My example above explains is exactly how this would be implemented.

@heaths
Copy link
Member

heaths commented Feb 3, 2025

@JeffreyRichter wrote,

Another reason not to magically conflate get-a-page & iterate-over-a-page is because it is also fairly common to have one PC initiate listing, process the page, and then another PC processes the next page.

Based on what? Even if we ignore client applications, many service schedulers I've seen documented seem to shuffle clients off to one machine or another for persistent connections. While I agree we should support rehydratable pagers, the fact we don't have them in all languages - and for a time didn't in any languages - and they still worked proves that they, well, work. While anecdotal, I didn't see any user feedback for .NET about wanting rehydratable pagers either.

As stated in another issue in which we discussed this, I agree we shouldn't hide getting page by page but iterating over a Stream for a single request the user made is also idiomatic in Rust as it is in .NET. Manually getting the next_link and making a different call is not idiomatic. Iteration is a major feature of Rust - as I often half-joke, std is pretty much there to support iteration and little else that people actually use - and making it harder to use is not idiomatic.

Still, part of my feedback above was also that returning an Option<Vec<T>> for value makes this API - even without a "magic" pager - much harder to use. I recommend we instead - for list results - return an empty Vec<T>. That would also reduce some of my worry about how people might interpret None here as well.

@heaths
Copy link
Member

heaths commented Feb 3, 2025

@ronniegeraghty I updated main with extension methods that will help clean things up. See #2052 and how I use them in #2051

@ronniegeraghty ronniegeraghty marked this pull request as draft February 3, 2025 23:43
@ronniegeraghty ronniegeraghty marked this pull request as ready for review February 4, 2025 01:33
Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few nits but otherwise LGTM

Copy link
Member

@heaths heaths left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best practice is to clone credentials, which is why we return a Result<Arc<T>>.

Unnecessary here, but we should show best practices regardless.

@ronniegeraghty ronniegeraghty merged commit 31dbecf into main Feb 5, 2025
16 checks passed
@ronniegeraghty ronniegeraghty deleted the ronniegeraghty/keyvault_secrets_readme_update branch February 5, 2025 01:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Key Vault Secrets README
5 participants