[Key Vault] TypeSpec for Secrets library #29249
Conversation
Next Steps to MergeNext steps that must be taken to merge this PR:
|
Swagger Validation Report
|
| Compared specs (v2.2.2) | new version | base version |
|---|---|---|
| default | default(56e1117) | default(main) |
️️✔️Avocado succeeded [Detail] [Expand]
Validation passes for Avocado.
️️✔️SwaggerAPIView succeeded [Detail] [Expand]
️️✔️TypeSpecAPIView succeeded [Detail] [Expand]
️️✔️ModelValidation succeeded [Detail] [Expand]
Validation passes for ModelValidation.
️️✔️SemanticValidation succeeded [Detail] [Expand]
Validation passes for SemanticValidation.
️️✔️PoliCheck succeeded [Detail] [Expand]
Validation passed for PoliCheck.
️️✔️SpellCheck succeeded [Detail] [Expand]
Validation passes for SpellCheck.
️️✔️Lint(RPaaS) succeeded [Detail] [Expand]
Validation passes for Lint(RPaaS).
️️✔️PR Summary succeeded [Detail] [Expand]
Validation passes for Summary.
️️✔️Automated merging requirements met succeeded [Detail] [Expand]
Swagger Generation Artifacts
|
Generated ApiView
|
mccoyp
force-pushed
the
mccoyp/kv-secrets-tsp
branch
from
6c80a51
to
fd66435
Compare
|
I'm going to wait for the Breaking Change validation to run. It'll be easier than having to manually diff. |
There was a problem hiding this comment.
Breaking change validation didn't really find anything, but, overall, I'm concerned this doesn't use much of Azure Core's abstractions and instead uses individual ops using Foundations. I appreciate Key Vault doesn't support much of the standard traits, but we can define our own and likely use more resource collections; otherwise, I don't think these TypeSpecs really "sell" the idea and likely are more authoring than the existing swaggers. We want the service team to be able to take these over, and resource collection abstractions should reduce the onboarding and maintenance effort.
We can talk offline more but should get some others e.g., @johanste involved in the discussions. I suspect my feedback will be the same as for Keys and Certificates.
BTW: Missing Administration.
Also BTW: All these need to be in separate project directories. We need to split them based on the SDK splits now.
|
|
||
| namespace KeyVault; | ||
|
|
||
| alias KeyVaultOperations = StandardResourceOperations; |
There was a problem hiding this comment.
I can't find the definition of StandardResourceOperations, but I highly doubt this is correct. Key Vault does not, for example, support etags for resources which, I'm betting, is probably standard for Azure.
There was a problem hiding this comment.
That's a good point; I didn't consider that Azure-standard traits could be carried over by this which wouldn't apply to Key Vault. This was done for defining resource operations (e.g. KeyVaultOperations.ResourceList).
There was a problem hiding this comment.
Key Vault doesn't really support standard resource operations, though. They are close, but not standard.
| */ | ||
| @pattern("^[0-9a-zA-Z-]+$") | ||
| @path | ||
| secretName: string; |
There was a problem hiding this comment.
I believe it's just "name" for all SDKs. Should that use a decorator instead? The old swagger uses "secret-name". Same for "secret-version" below.
| @summary("Get a specified secret from a given key vault.") | ||
| @route("/secrets/{secretName}/{secretVersion}") | ||
| @get | ||
| op getSecret is Azure.Core.Foundations.Operation< |
There was a problem hiding this comment.
This will create problems for SDKs but I'm not sure how to solve it. We went with making "version" an optional parameter across languages, which means we basically collapsed the routes into one. @lmazuel or @johanste any suggestions? Can client TSPs "fix" this, or is this a case where we're going to generate an incompatible SDK and have to either wrap it or ship a new major version?
| * requires the secrets/list permission. | ||
| */ | ||
| @summary("List secrets in a specified key vault.") | ||
| op getSecrets is KeyVaultOperations.ResourceList< |
There was a problem hiding this comment.
This uses the right pattern. Why don't the others? Do they not work? Do we need to declare our own traits so they do align? Key Vault doesn't really support many of the (now-) standard params.
There was a problem hiding this comment.
Answered in #29249 (comment)
| /** | ||
| * The object attributes managed by the KeyVault service. | ||
| */ | ||
| model Attributes { |
There was a problem hiding this comment.
Where is this used other than as a base class?
There was a problem hiding this comment.
This is just used as a base class that comes from the original swagger.
|
|
||
| using Azure.Core; | ||
| using Azure.Core.Traits; | ||
| using Azure.ClientGenerator.Core; |
There was a problem hiding this comment.
We do not want this in the service description. Please create a client.tsp for client codegen customizations.
| /** | ||
| * The secret set parameters. | ||
| */ | ||
| model SecretSetParameters { |
There was a problem hiding this comment.
Isn't this just the subset of properties of a SecretBundle that are writeable? If so, why do we need two models and not one with the appropriate visibility?
| /** | ||
| * The secret management attributes. | ||
| */ | ||
| model SecretAttributes extends Attributes { |
There was a problem hiding this comment.
Why does this extend Attributes? Can I ever use one in place of another?
| @summary("Sets a secret in a specified key vault.") | ||
| @route("/secrets/{secretName}") | ||
| @put | ||
| op setSecret is Azure.Core.Foundations.Operation< |
There was a problem hiding this comment.
This looks like a resource create or replace?
There was a problem hiding this comment.
See #29249 (comment). Many of these operations could be standard resource operations if the service returned a resource name instead of a resource ID in the form of a full URL. I couldn't get operations to conform to using the latter as a resource key, and I'm not aware of a way to override the route otherwise.
There was a problem hiding this comment.
So the secret name is not returned? It is only in the path of the request?
There was a problem hiding this comment.
It's not returned as a standalone property, no. It's part of the id but needs to be parsed manually -- here's an example from a recording.
"id": "https://vaultname.vault.azure.net/secrets/{secretName}/{secretVersion}"
| @summary("Deletes a secret from a specified key vault.") | ||
| @route("/secrets/{secretName}") | ||
| @delete | ||
| op deleteSecret is Azure.Core.Foundations.Operation< |
There was a problem hiding this comment.
This looks like a resource delete?
specification/keyvault/data-plane/Security.KeyVault.Secrets/main.tsp
Outdated
Show resolved
Hide resolved
microsoft-github-policy-service
bot
added
no-recent-activity
microsoft-github-policy-service
bot
added
no-recent-activity
| using TypeSpec.Rest; | ||
| using TypeSpec.Http; | ||
|
|
||
| namespace KeyVault; |
There was a problem hiding this comment.
Either here or in a client TSP, this or something else should be changed so we generate a SecretClient and not a KeyVaultClient.
See Azure/autorest.rust#86 for context.
AzureRestAPISpecReview
added
BreakingChangeReviewRequired
|
Lots of breaking changes reported: https://github.com/Azure/azure-rest-api-specs/pull/29249/checks?check_run_id=32562321385 |
The majority of these are from path formatting changes (e.g. |
gracewilcox
force-pushed
the
mccoyp/kv-secrets-tsp
branch
2 times, most recently
from
aa125b3
to
710421c
Compare
gracewilcox
force-pushed
the
mccoyp/kv-secrets-tsp
branch
from
710421c
to
0d12c74
Compare
| * at the end of the retention interval. | ||
| */ | ||
| @visibility("read") | ||
| recoveryLevel?: string; // DeletionRecoveryLevel, but this should be treated as an opaque string. |
There was a problem hiding this comment.
👋 I think this should be DeletionRecoveryLevel and not a string IIUC - because DeletionRecoveryLevel is not actually used in routes it is not generated (at least by the typespec-ts codegen) - can we change that?
When I change the type to DeletionRecoveryLevel I see the correct code being generated (including extensible enum patterns)
There was a problem hiding this comment.
We should be able to get DeletionRecoveryLevel generated by using @@access(DeletionRecoveryLevel, Access.public) in our client.tsp. For example, from another library:
EDIT: @@access didn't actually seem to work, but local testing showed that @@usage is what we'd want -- PR has been updated!
There was a problem hiding this comment.
But why not use DeletionRecoveryLevel here instead of adding a client.tsp customization - does it not generate the correct thing? Just trying to understand the reasoning behind not using recoveryLevel?: DeletionRecoveryLevel when DeletionRecoveryLevel is defined as a string union already
There was a problem hiding this comment.
Using a plain string instead of the enum was a change request from Heath; "[i]t's response-only and should not be compared, which is why none of our client libraries should ship it as an enum".
There was a problem hiding this comment.
Ah ok, got it! Thanks for the context.
So @heaths do you consider it a bug that JS shipped KnownDeletionRecoveryLevel as part of our public API? I think what you said makes sense re: response-only just want to triple-check - if an extensible enum value can only show up in a response, never as an input, we should not be shipping the Known enum values as part of our public API is that accurate? (I know the ship has sailed on this one for JS, but mostly asking for my future self)
Data Plane API Specification Update Pull Request
Tip
Overwhelmed by all this guidance? See the
Getting helpsection at the bottom of this PR description.Based on feedback from #28708. This spec is for the KV Secrets library. This has been successfully tested end-to-end with Python (Azure/azure-sdk-for-python#36901).
PR review workflow diagram
Please understand this diagram before proceeding. It explains how to get your PR approved & merged.
API Info: The Basics
Most of the information about your service should be captured in the issue that serves as your API Spec engagement record.
Is this review for (select one):
Change Scope
This section will help us focus on the specific parts of your API that are new or have been modified.
Please share a link to the design document for the new APIs, a link to the previous API Spec document (if applicable), and the root paths that have been updated.
Viewing API changes
For convenient view of the API changes made by this PR, refer to the URLs provided in the table
in the
Generated ApiViewcomment added to this PR. You can use ApiView to show API versions diff.Suppressing failures
If one or multiple validation error/warning suppression(s) is detected in your PR, please follow the
Swagger-Suppression-Process
to get approval.
❔Got questions? Need additional info?? We are here to help!
Contact us!
The Azure API Review Board is dedicated to helping you create amazing APIs. You can read about our mission and learn more about our process on our wiki.
Click here for links to tools, specs, guidelines & other good stuff
Tooling
Guidelines & Specifications
Helpful Links
Getting help
write accessper aka.ms/azsdk/access#request-access-to-rest-api-or-sdk-repositoriesNext Steps to Mergecomment. It will appear within few minutes of submitting this PR and will continue to be up-to-date with current PR state.and https://aka.ms/ci-fix.
queuedstate, please add a comment with contents/azp run.This should result in a new comment denoting a
PR validation pipelinehas started and the checks should be updated after few minutes.