Improve OpenAPI request/response naming strategy

[averche]

Background

This PR changes the way we construct operationIds as well as request & response names in the generated openapi.json document. operationIds are translated directly into function or method names in the library code generated with OpenAPI generators. Therefore it's important to produce human-readable names.

Old naming strategy

The old naming strategy was to construct operationIds from the mount + path, removing any non-word characters, for example, for kv-v2 secrets:

• mount: "secret" + path: "data/{path}"
  • -> operationId: postSecretDataPath
    • -> func (a *Secrets) PostSecretDataPath(...) (in vault-client-go)

New naming strategy

The new naming strategy is to look up the name in a map for commonly used paths. If the name is not found in the map, we fall back to the old strategy. For the example above, the operationId is:

• mount: "secret" + path: "data/{path}"
  • -> operationId: WriteSecret
    • -> func (a *Secrets) WriteSecret(...)

[vercel[bot]]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated
vault	🔄 Building (Inspect)		Jan 5, 2023 at 1:10AM (UTC)

[maxb]

Hello,

I'm a user of the OpenAPI document (for ad-hoc tooling, and as a way to better comprehend the APIs), who just stumbled across this PR.

What is the motivation for making this change? From my perspective, I'm better served by consistency, and avoiding the creation of another set of identifiers, i.e. I think I'd be better off without this change.

I would also like to raise a concern about the renaming and changed semantics of requestResponsePrefix which, though poorly named, forms an API between Vault and versions of sdk/framework compiled in to separately distributed Vault plugins, so this change is a compatibility issue. (I have raised this in more detail in #18560, a large issue I have recently filed documenting experiences with existing 1.13-dev OpenAPI changes.)

This change also looks to be furthering the behaviour of sometimes calling KV v2 mounts secret, which is something I think tends to lead to confusion, and would be better for users to eliminate in favour of something clearer and more informative, like kv-v2.

[averche]

Hi @maxb!

Thank you for your detailed feedback! I'd love to hear more about how you use Vault's OpenAPI spec. If you are free for a quick zoom chat sometime this week, I'd be happy to connect!

What is the motivation for making this change?

We are working on adding a few new vault client libraries, which will be automatically generated from this OpenAPI spec. The operationIds generated in the spec are translated directly into method names. Unfortunately, since today the operationIds are constructed directly from paths, they are not very human-friendly in the resultant library code. Here are a couple examples:

// notice roleRoleRole stutter and the redundant 'Auth' in the method name
client.Auth.postAuthApproleRoleRoleName(...)

// this is the function generated for fetching KVv2 secrets (probably the most popular Vault usecase)
// but it sounds like it's getting a secret path instead!
client.Secrets.getSecretDataPath(...)

I would also like to raise a concern about the renaming and changed semantics of requestResponsePrefix which, though poorly named, forms an API between Vault and versions of sdk/framework compiled in to separately distributed Vault plugins, so this change is a compatibility issue. (I have raised this in more detail in #18560, a large issue I have recently filed documenting experiences with existing 1.13-dev OpenAPI changes.)

requestResponsePrefix is an undocumented internal parameter which is used strictly within Vault repo, as far as I can tell. I renamed it because of the changed semantics. If compatibility is a concern, I can keep the old parameter and introduce a new one, but it seems like it would just introduce a tech debt for no apparent gain. Do you know of an example where requestResponsePrefix is used outside of this repo?

This change also looks to be furthering the behaviour of sometimes calling KV v2 mounts secret, which is something I think tends to lead to confusion, and would be better for users to eliminate in favour of something clearer and more informative, like kv-v2.

I have updated the naming pattern for KVv2-related operation ID's / requests / responses (just updated the PR description as well). They now all have a KVv2 prefix (analogous to the KVv1 prefix for the KVv1 requests).

[maxb]

Thank you for your detailed feedback! I'd love to hear more about how you use Vault's OpenAPI spec. If you are free for a quick zoom chat sometime this week, I'd be happy to connect!

No problem!

My primary current use-case is parsing the OpenAPI document to produce a summary of API endpoints that exist, which operations each endpoint supports, and which are sudo-protected or anonymous-access, and whether they support create as a separate capability or not. I find this to often be easier to consume, and more reliable, than the verbose and sometimes incomplete website API docs.

I have a hopeful future use-case, where I am planning to consume the OpenAPI document to figure out when users are writing policies that don't match real endpoints, or are granting capabilities which do not apply to an endpoint.

I would be happy to have a chat - I'm on London time and would be able to accept most timeslots between 10am-6pm (maybe send me a couple of possibilities to choose from?). I've set my email address to be public on https://github.com/maxb .

postAuthApproleRoleRoleName

Yeah... point well made. That definitely needs to be improved. Hopefully there's a happy medium that achieves that, whilst also still serving up full information on all possible operations on each endpoint, and countermeasures to accidental naming clashes producing errors.

Do you know of an example where requestResponsePrefix is used outside of this repo?

The subtle detail is that it is only used in this repo, but each separately compiled Vault plugin ends up with its own copy of the the github.com/hashicorp/vault/sdk code inside it. That means there can be a compatibility issue where, for example, a Vault 1.13 server could be hosting a plugin built using an older sdk version that has expectations of requestResponsePrefix being passed to it.

It's the same reason why the addition of the <whatever>_mount_path parameter to the OpenAPI spec should be moved so it fully happens outside the sdk code, rather than being part in sdk and part in the main Vault code.

[averche]

Closing this in favour of https://github.com/hashicorp/vault/pull/19319, let's continue the conversation there.