[Key Vault] TypeSpec for Security Domain library #31060

mccoyp · 2024-10-16T17:44:29Z

Data Plane API Specification Update Pull Request

Tip

Overwhelmed by all this guidance? See the Getting help section at the bottom of this PR description.

This converts the existing Security Domain specification from Swagger to TypeSpec. The goal is to generate a Python library from this specification, azure-keyvault-securitydomain, for CLI consumption and external customer use. There are currently no plans for shipping this library in other languages.

Initial preview timeline: Python public preview release by October 22nd, 2024.

Python library implementation: Azure/azure-sdk-for-python#37929

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.

Link to API Spec engagement record issue:

Is this review for (select one):

a private preview
a public preview
GA release

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.}

Design Document:
Previous API Spec Doc: https://github.com/Azure/azure-rest-api-specs/blob/main/specification/keyvault/data-plane/Microsoft.KeyVault/stable/7.5/securitydomain.json
Updated paths:

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 ApiView comment 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.

💬 Teams Channel
💌 email

Click here for links to tools, specs, guidelines & other good stuff

Tooling

Open API validation tools were run on this PR. Go here to see how to fix errors
Spectral Linting

Guidelines & Specifications

Helpful Links

Schedule a data plane REST API spec review

Getting help

First, please carefully read through this PR description, from top to bottom.
If you don't have permissions to remove or add labels to the PR, request write access per aka.ms/azsdk/access#request-access-to-rest-api-or-sdk-repositories
To understand what you must do next to merge this PR, see the Next Steps to Merge comment. It will appear within few minutes of submitting this PR and will continue to be up-to-date with current PR state.
For guidance on fixing this PR CI check failures, see the hyperlinks provided in given failure
and https://aka.ms/ci-fix.
If the PR CI checks appear to be stuck in queued state, please add a comment with contents /azp run.
This should result in a new comment denoting a PR validation pipeline has started and the checks should be updated after few minutes.
If the help provided by the previous points is not enough, post to https://aka.ms/azsdk/support/specreview-channel and link to this PR.

openapi-pipeline-app · 2024-10-16T17:44:32Z

Next Steps to Merge

Next steps that must be taken to merge this PR:

❌ This is the public specs repo main branch which is not intended for iterative development.
You must acknowledge that you understand that after this PR is merged, you won't be able to stop your changes from being published to Azure customers.
If this is what you intend, add PublishToCustomers label to your PR.
Otherwise, retarget this PR onto a feature branch, i.e. with prefix release- (see aka.ms/azsdk/api-versions#release--branches).
❌ This PR has at least one breaking change (label: BreakingChangeReviewRequired).
To unblock this PR, follow the process at aka.ms/brch.
❌ The required check named Swagger PrettierCheck has failed. Refer to the check in the PR's 'Checks' tab for details on how to fix it and consult the aka.ms/ci-fix guide
❌ The required check named Swagger SpellCheck has failed. Refer to the check in the PR's 'Checks' tab for details on how to fix it and consult the aka.ms/ci-fix guide

openapi-pipeline-app · 2024-10-16T17:44:33Z

Generated ApiView

Language	Package Name	ApiView Link
TypeSpec	Language.AnalyzeConversations-authoring	https://apiview.dev/Assemblies/Review/f6b03d30015e4369ae5cd06792925a75?revisionId=b04a7798d3d440ecba2f21ddf833f9f6
TypeSpec	Language.AnalyzeText-authoring	https://apiview.dev/Assemblies/Review/2a66ca7bd3464c3e85161319bb2c799c?revisionId=ad15a5ce884c42a386061dc86870ebe0
TypeSpec	Security.KeyVault.SecurityDomain	https://apiview.dev/Assemblies/Review/0529655b61bc404ebd37d2d5192ba45a?revisionId=5ac0b2c595064cfda791f50d9ad46d3e

heaths

Apart from a few non-blocking thoughts, as long as the emitted swagger matches their service definition this looks good to me. Best to have Key Vault check over the swagger, though, or did you already generate a Python SDK from it to test against their service (probably best anyway)?

heaths · 2024-10-16T23:03:34Z

specification/keyvault/data-plane/Security.KeyVault.SecurityDomain/tspconfig.yaml

+    emitter-output-dir: "{project-root}/.."
+    output-file: "{azure-resource-provider-folder}/{service-name}/{version-status}/{version}/securitydomain-sdk.json"
+  # Uncomment this line and add "@azure-tools/typespec-python" to your package.json to generate Python code
+  "@azure-tools/typespec-python":


Doesn't this need to be up in the emit array as well, or is that only for the python SDK repo?

heaths · 2024-10-16T23:04:53Z

specification/keyvault/data-plane/Microsoft.KeyVault/stable/7.5/securitydomain-sdk.json

Why are these files ending in "-sdk"? They are for the service which we don't consider an "SDK" typically. Can't it just be "securitydomain.json"? This also matches what service swaggers are now for Key Vault, and what all the other TSPs should be doing.

I added -sdk for this and the other conversions' swaggers because it's not totally aligned with the original swaggers. In this case, the Security Domain spec is constructed from common.json and securitydomain.json, so the swagger generated from it is the combination of those two

I think that's fine. common.json was also an implementation detail anyway and was just for humans to avoid duplication. Given we're emitting these swaggers, I don't see a need to differentiate. I worry that "sdk" will make orgs that want to generate their own clients (from swagger) think these aren't the service specs.

heaths · 2024-10-16T23:07:31Z

specification/keyvault/data-plane/Security.KeyVault.SecurityDomain/models.tsp

+  statusDetails?: string;
+}
+
+/**


These appear to be normal Azure Core errors. Can't we use those instead of redefining?

The Key Vault error is a little different in its definition, but I do think it's functionally equivalent to the Azure Core error. If I'm not mistaken, KV's custom error has an additional innerError property.

heaths · 2024-10-16T23:10:07Z

specification/keyvault/data-plane/Security.KeyVault.SecurityDomain/models.tsp

+/**
+ * A JSON Web Key (JWK) for use in a security domain operation.
+ */
+model SecurityDomainJsonWebKey {


Nit: seems we should be defining common types like JWKs in a common tsp further up. I won't block on this, but we should consider it. We don't want JWKs - an industry standard - diverging accidentally or incidentally.

heaths · 2024-10-16T23:15:45Z

specification/keyvault/data-plane/Security.KeyVault.SecurityDomain/models.tsp

+   * X509 certificate SHA1 thumbprint. This is optional.
+   */
+  @encodedName("application/json", "x5t")
+  x5T?: string;


Seems this should be x5t just as it is below (x5tS256). That's the industry standard anyway, and won't impact casing rules in client emitters anyway...or shouldn't, anyway.

specification/keyvault/data-plane/Security.KeyVault.SecurityDomain/routes.tsp

heaths · 2024-10-28T22:23:59Z

specification/keyvault/data-plane/Security.KeyVault.SecurityDomain/client.tsp

-  name: "SecurityDomainClient",
+  name: "KeyVaultClient",


Why? This should be renamed. We don't want other languages generating KeyVaultClient.

We have to patch the generated client to apply our challenge auth policy, provide functioning pollers (because of download having an unusual return type, as well as the "Success" terminal condition being non-standard), and fix operation return types (again because of download): https://github.com/Azure/azure-sdk-for-python/blob/56b3fd9d9ae75a6fc05c82b5caf5a2eb1f403ac1/sdk/keyvault/azure-keyvault-securitydomain/azure/keyvault/securitydomain/_patch.py

The patched client is the one that users see, and type resolution gets confused if the client we're patching has the same name. I just reused the "KeyVaultClient" name for consistency with generated clients in other Python libraries, but the public client is SecurityDomainClient.

We could make this client modification language-specific in the future if other languages start generating this library.

Can you just use the @clientName decorator for Python now? I worry down the road no one is going to remember the context. Seems you're the only one who needs a specialized name, so why make every other language opt into renaming it (since there's no language exclusion)?

mccoyp added 6 commits October 2, 2024 16:19

Initial conversion from swagger w/ tsp-client

4073a0e

Try using standard KV auth scheme

ffc8262

Add tspconfig.yaml

ef6c778

Add examples

41442f6

Refine specification

2b95414

Add generated OpenAPI spec

8195ed5

mccoyp added KeyVault TypeSpec Authored with TypeSpec labels Oct 16, 2024

mccoyp requested review from chlowell and johanste October 16, 2024 17:44

mccoyp requested review from heaths and jlichwa as code owners October 16, 2024 17:44

AzureRestAPISpecReview added data-plane VersioningReviewRequired <valid label in PR review process>add this label when versioning review is required labels Oct 16, 2024

heaths approved these changes Oct 16, 2024

View reviewed changes

mccoyp added 2 commits October 23, 2024 19:54

Use @finalOperation to resolve return type for download

6c5c06a

Add client.tsp to flatten operations

a87eb19

heaths approved these changes Oct 24, 2024

View reviewed changes

specification/keyvault/data-plane/Security.KeyVault.SecurityDomain/routes.tsp Outdated Show resolved Hide resolved

mccoyp mentioned this pull request Oct 24, 2024

[Key Vault] Create Security Domain library Azure/azure-sdk-for-python#37929

Draft

6 tasks

Fix opIDs; rename generated client; correct return types/model access

2263fe3

openapi-pipeline-app bot added PipelineBotTrigger and removed PipelineBotTrigger labels Oct 25, 2024

heaths reviewed Oct 28, 2024

View reviewed changes

mccoyp added 2 commits October 29, 2024 15:44

Scope client customizations to Python

638e8cd

Remove '-sdk' from generated swagger name

2ead8e3

Add operationId/title to examples, upload status example

828e75b

AzureRestAPISpecReview added BreakingChangeReviewRequired <valid label in PR review process>add this label when breaking change review is required and removed VersioningReviewRequired <valid label in PR review process>add this label when versioning review is required labels Oct 29, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Key Vault] TypeSpec for Security Domain library #31060

[Key Vault] TypeSpec for Security Domain library #31060

mccoyp commented Oct 16, 2024

openapi-pipeline-app bot commented Oct 16, 2024 •

edited

Loading

openapi-pipeline-app bot commented Oct 16, 2024 •

edited

Loading

heaths left a comment

heaths Oct 16, 2024

heaths Oct 16, 2024

mccoyp Oct 18, 2024

heaths Oct 18, 2024

heaths Oct 16, 2024

mccoyp Oct 18, 2024

heaths Oct 16, 2024

heaths Oct 16, 2024

heaths Oct 28, 2024

mccoyp Oct 28, 2024

mccoyp Oct 28, 2024

heaths Oct 28, 2024

[Key Vault] TypeSpec for Security Domain library #31060

Are you sure you want to change the base?

[Key Vault] TypeSpec for Security Domain library #31060

Conversation

mccoyp commented Oct 16, 2024

Data Plane API Specification Update Pull Request

PR review workflow diagram

API Info: The Basics

Change Scope

Viewing API changes

Suppressing failures

❔Got questions? Need additional info?? We are here to help!

Tooling

Guidelines & Specifications

Helpful Links

Getting help

openapi-pipeline-app bot commented Oct 16, 2024 • edited Loading

Next Steps to Merge

openapi-pipeline-app bot commented Oct 16, 2024 • edited Loading

Generated ApiView

heaths 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

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

Choose a reason for hiding this comment

openapi-pipeline-app bot commented Oct 16, 2024 •

edited

Loading

openapi-pipeline-app bot commented Oct 16, 2024 •

edited

Loading