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: KVv2Write
    • -> func (a *Secrets) KVv2Write(...)

Additionally, this PR should resolve #18578 isssue. The request and response names are no longer "flattened" and should therefore be unique within the schema. For example:

POST /auth/{approle_mount_path}/role/{role_name}/secret-id/lookup
    OperationID: AppRoleWriteSecretIDLookup
   Request name: AppRoleWriteSecretIDLookupRequest
  Response name: AppRoleWriteSecretIDLookupResponse

[AnPucel]

I'm thinking I'll regen C# with this updated spec to make sure the naming works out in the code generation

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

[maxb]

Some further comments, as I look through the code in a bit more detail

[maxb]

I have doubts about this being a good direction to move in - individual backends are currently on a journey towards being less coupled with the Vault core, in their own repositories, but this would be adding a new way in which coupling is increased again.

[averche]

An alternative solution we have considered was to add an OperationIDHint in the Path object or OperationIDOverride in the PathOperation object. Unfortunately, neither of these solutions would work since a single Path + Operation can sometimes result into multiple OpenAPI paths. For example, the following 3 paths come from a single path definition for /sys/tools/random, the {source} and {urlbytes} are optional parameters:

• /sys/tools/random
• /sys/tools/random/{source}
• /sys/tools/random/{source}/{urlbytes}

Arguably, even if the above solution worked, it would have introduced even more coupling since now each plugin would have to have some awareness of the OpenAPI logic.

The good thing about the map solution is that when a new path or a new plugin are introduced, they don't necessarily need to be added to this map. Any plugins/paths not mentioned in this map will simply default to the old naming logic.

[dhuckins]

not sure I like this either.... would rather have plugins to have the option to override vs keeping a big table

is this only the ones that need to be changed or all of the endpoints?

---

it would have introduced even more coupling since now each plugin would have to have some awareness of the OpenAPI logic.

disagree slightly here: the plugin wouldn't have to, as you mention, can use the default factory

[averche]

Yes, I agree, if the plugin approach worked, I would have probably preferred it as well, but, alas, the OpenAPI paths don't have a 1-to-1 mapping with the plugin Path definitions so we'd end up with a number of duplicate Operation ID's.

it would have introduced even more coupling since now each plugin would have to have some awareness of the OpenAPI logic.

disagree slightly here: the plugin wouldn't have to, as you mention, can use the default factory

What I meant is that the plugin's Path or PathOperation objects would have a reference to something specific to the implementation of OpenAPI, which seems a bit weird. So far the relationship between OpenAPI and the plugins has been in the opposite direction (OpenAPI knows about the plugins, not vice-versa).

[maxb]

Well... there are already several fields in the framework.Path and framework.PathOperation structs that exist solely for the OpenAPI generator, so I think it's a bit of an oversimplification to say the plugins don't know about OpenAPI - to be sure, many plugins won't currently make use of those fields, but they are there to be used.

I have recently been working on the exact behaviour of how one Path generates multiple OpenID operations - indeed, the PKI secrets engine's OpenAPI is currently very broken, with smaller impact on AWS and GCP. My fix for this is in #18554.

Through this work, I was able to fairly easily construct a report of all of the (OSS) instances where a builtin plugin generates multiple OpenAPI paths from one regexp path:

auth/radius/:
n=2 regexp="^login(/(?P<urlusername>.+))?$" openapi=["login", "login/{urlusername}"]

aws/:
n=2 regexp="^(creds|sts)/(?P<name>\w(([\w-.@]+)?\w)?)$" openapi=["creds/{name}", "sts/{name}"]

gcp/:
n=2 regexp="^rolesets?/?$" openapi=["roleset", "rolesets"]
n=2 regexp="^static-accounts?/?$" openapi=["static-account", "static-accounts"]

pki/:
n=2 regexp="^sign-verbatim(/(?P<role>.+))?$" openapi=["sign-verbatim", "sign-verbatim/{role}"]
n=4 regexp="^issuer/(?P<issuer_ref>\w(([\w-.]+)?\w)?)(/der|/pem|/json)?$" openapi=["issuer/{issuer_ref}", "issuer/{issuer_ref}/der", "issuer/{issuer_ref}/pem", "issuer/{issuer_ref}/json"]
n=6 regexp="^issuer/(?P<issuer_ref>\w(([\w-.]+)?\w)?)/crl(/pem|/der|/delta(/pem|/der)?)?$" openapi=["issuer/{issuer_ref}/crl", "issuer/{issuer_ref}/crl/pem", "issuer/{issuer_ref}/crl/der", "issuer/{issuer_ref}/crl/delta", "issuer/{issuer_ref}/crl/delta/pem", "issuer/{issuer_ref}/crl/delta/der"]
n=2 regexp="^issuers/import/(cert|bundle)$" openapi=["issuers/import/cert", "issuers/import/bundle"]
n=2 regexp="^issuer/(?P<issuer_ref>\w(([\w-.]+)?\w)?)/sign-verbatim(/(?P<role>.+))?$" openapi=["issuer/{issuer_ref}/sign-verbatim", "issuer/{issuer_ref}/sign-verbatim/{role}"]
n=3 regexp="^keys/generate/(internal|exported|kms)$" openapi=["keys/generate/internal", "keys/generate/exported", "keys/generate/kms"]
n=2 regexp="^ca(/pem)?$" openapi=["ca", "ca/pem"]
n=2 regexp="^(cert/)?ca_chain$" openapi=["ca_chain", "cert/ca_chain"]
n=4 regexp="^crl(/pem|/delta(/pem)?)?$" openapi=["crl", "crl/pem", "crl/delta", "crl/delta/pem"]
n=2 regexp="^cert/(crl|delta-crl)$" openapi=["cert/crl", "cert/delta-crl"]
n=2 regexp="^cert/(?P<serial>[0-9A-Fa-f-:]+)/raw(/pem)?$" openapi=["cert/{serial}/raw", "cert/{serial}/raw/pem"]

transit/:
2023-01-06T08:14:28.940Z [INFO]  core: successful mount: namespace="" path=transit/ type=transit version=""
n=2 regexp="^export/(?P<type>\w(([\w-.]+)?\w)?)/(?P<name>\w(([\w-.]+)?\w)?)(/(?P<version>.+))?$" openapi=["export/{type}/{name}", "export/{type}/{name}/{version}"]
n=4 regexp="^random(/(?P<source>\w(([\w-.]+)?\w)?))?(/(?P<urlbytes>.+))?$" openapi=["random", "random/{source}", "random/{urlbytes}", "random/{source}/{urlbytes}"]
n=2 regexp="^hash(/(?P<urlalgorithm>.+))?$" openapi=["hash", "hash/{urlalgorithm}"]
n=2 regexp="^hmac/(?P<name>\w(([\w-.]+)?\w)?)(/(?P<urlalgorithm>.+))?$" openapi=["hmac/{name}", "hmac/{name}/{urlalgorithm}"]
n=2 regexp="^sign/(?P<name>\w(([\w-.]+)?\w)?)(/(?P<urlalgorithm>.+))?$" openapi=["sign/{name}", "sign/{name}/{urlalgorithm}"]
n=2 regexp="^verify/(?P<name>\w(([\w-.]+)?\w)?)(/(?P<urlalgorithm>.+))?$" openapi=["verify/{name}", "verify/{name}/{urlalgorithm}"]
n=2 regexp="^restore(/(?P<name>.+))?$" openapi=["restore", "restore/{name}"]

sys/:
n=2 regexp="^mfa/method(/(?P<method_id>[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}))?$" openapi=["mfa/method", "mfa/method/{method_id}"]
n=2 regexp="^mfa/method/totp(/(?P<method_id>[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}))?$" openapi=["mfa/method/totp", "mfa/method/totp/{method_id}"]
n=2 regexp="^mfa/method/okta(/(?P<method_id>[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}))?$" openapi=["mfa/method/okta", "mfa/method/okta/{method_id}"]
n=2 regexp="^mfa/method/duo(/(?P<method_id>[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}))?$" openapi=["mfa/method/duo", "mfa/method/duo/{method_id}"]
n=2 regexp="^mfa/method/pingid(/(?P<method_id>[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}))?$" openapi=["mfa/method/pingid", "mfa/method/pingid/{method_id}"]
n=2 regexp="^generate-root(/attempt)?$" openapi=["generate-root", "generate-root/attempt"]
n=2 regexp="^plugins/catalog(/(?P<type>auth|database|secret))?/(?P<name>.+)$" openapi=["plugins/catalog/{name}", "plugins/catalog/{type}/{name}"]
n=2 regexp="^leases/lookup/(?P<prefix>.+?)?$" openapi=["leases/lookup/", "leases/lookup/{prefix}"]
n=4 regexp="^(leases/)?renew(/(?P<url_lease_id>.+))?$" openapi=["renew", "leases/renew", "renew/{url_lease_id}", "leases/renew/{url_lease_id}"]
n=4 regexp="^(leases/)?revoke(/(?P<url_lease_id>.+))?$" openapi=["revoke", "leases/revoke", "revoke/{url_lease_id}", "leases/revoke/{url_lease_id}"]
n=2 regexp="^(leases/)?revoke-force/(?P<prefix>.+)$" openapi=["revoke-force/{prefix}", "leases/revoke-force/{prefix}"]
n=2 regexp="^(leases/)?revoke-prefix/(?P<prefix>.+)$" openapi=["revoke-prefix/{prefix}", "leases/revoke-prefix/{prefix}"]
n=2 regexp="^tools/hash(/(?P<urlalgorithm>.+))?$" openapi=["tools/hash", "tools/hash/{urlalgorithm}"]
n=4 regexp="^tools/random(/(?P<source>\w(([\w-.]+)?\w)?))?(/(?P<urlbytes>.+))?$" openapi=["tools/random", "tools/random/{source}", "tools/random/{urlbytes}", "tools/random/{source}/{urlbytes}"]

Though there are a lot, they are concentrated in only 6 backends, and I wonder if some of them could be reduced by refactoring.

For example, in the sys/ backend, all the variants with an optional leases/ prefix could usefully be split apart into multiple framework.Path structs anyway, so the deprecated versions of the APIs can be more clearly deprecated.

In the PKI backend, there are a few that are being split, because the author used an unnamed (foo|bar|baz) for what really ought to be a named parameter.

And, in the relatively common case of optional additional URL segments, we might be able to a standard convention of automatically appending a generated suffix which makes adequate human-readable sense.

[averche]

If the consensus is that we should have hints defined at framework.Path / framework.PathOperation level, I suppose it is not impossible to do that, though I can foresee that the logic to translate the hints into OperationID's might be more complicated.

I would prefer to keep the map implementation for now at least until #18554 is merged. Adding the name hints to each plugin may take some time, especially for the plugins defined in other repos.

P.S. Thank you for generating the list above, it will be very helpful when testing the new approach!

[maxb]

I would prefer to keep the map implementation for now at least until #18554 is merged.

If it helps, I can make the time to move fast on that one, so we can reduce the need for an interim solution. I've done the changelog for #18663, which should be ready to merge - once that one's in, I'll merge main into #18554 and address the pending feedback.

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

[dhuckins]

just noticed we dont have this in the doc, should we?

[maxb]

It is likely this parameter will get removed as part of the outcome of discussion in #18560

[averche]

just noticed we dont have this in the doc, should we?

The doc you linked is for the system/internal-specs-openapi endpoint. The parameter here is for generic helpCallback, which, I think, applies to every endpoint. I'd rather keep it as an internal/undocumented parameter since it's here purely for OpenAPI generation purposes.

It is likely this parameter will get removed as part of the outcome of discussion in #18560

I think this parameter or some variation of it will still be needed to pass the mount information into the plugins. The only alternatives I can think of are:

• Somehow determine the plugin's mount info from within vault/sdk (I don't believe it is possible)
• Extract all mount-specific code out of vault/sdk into logical_system.go. I suppose this is possible but will not be easy - we will need to iterate all request/response names & references and append mount-specific info there.

[dhuckins]

do we want to emit a warning or something?

[maxb]

I think the nature of the compatibility accomodation is potentially subject to change via the discussion in #18560

[averche]

Yes, still not sure which approach we are going with 😄

[dhuckins]

not sure I like this either.... would rather have plugins to have the option to override vs keeping a big table

is this only the ones that need to be changed or all of the endpoints?

---

it would have introduced even more coupling since now each plugin would have to have some awareness of the OpenAPI logic.

disagree slightly here: the plugin wouldn't have to, as you mention, can use the default factory

[maxb]

I lack the permissions to unresolve the previous conversation, so I need to copy/paste my comment into a new one instead:

Sorry, but the new solution is still very problematic for my usecase, as I rely on the OpenAPI to determine the complete set of operations that a given path supports, and this would erase that information.

Additionally, it is likely to prove confusing to people referring to the OpenAPI or Swagger UI to guide their understanding of already-written code, and find it is calling an operation which is now erased, and so seems mysterious and undocumented.

[averche]

Fair enough, I suppose I can disambiguate the names in some other way. It's just a shame that the libraries will have multiple methods generated which will result in exactly the same behaviour.

[maxb]

The more I think about it, the more I think the library code generation is going to need a way to apply tweaks to the canonical OpenAPI document, to vary the generation process. Let me explain further:

Today, as you have identified, there are various APIs in Vault which are deprecated and replaced by newer APIs. It would be nice if we could exclude the historical cruft from the generated client libraries, whilst it remains important to still acknowledge the APIs exist, to help people understand existing code, and understand the impact of wildcards in policies.

We could (and IMO should) add a simple "deprecated" flag to deprecated APIs, and use that to render them as crossed out in the API explorer UI (something Swagger UI already supports). Then, at the birth of a new autogenerated client library, it would make sense to exclude all these.

But wait! This only works out nice and simply, for the first version of the autogenerated client library. Users expect API stability from their libraries. If we simply exclude all deprecated APIs, then the moment any future API deprecation in Vault would result in the API being removed from the next version of the autogenerated client library, breaking compatibility.

The problem is actually deeper still. There may be a bugfix to how an API is represented in Vault, but that may trigger an API removal in an autogenerated client library if we're not careful. Here is an example:

There is currently this path regexp with the PKI secrets engine: "keys/generate/(internal|exported|kms)" and this currently generates three separate APIs. However, the same three values are represented in multiple other PKI secrets engine APIs in this other style: "issuers/generate/intermediate/"+framework.GenericNameRegex("exported") where the parameter captured by the GenericNameRegex must have one of the three values, internal, exported or kms. This other pattern only generates one OpenAPI operation. I intend to submit a PR to make the PKI secrets engine internally consistent in how it handles this parameter. When I do, what is currently three operations in the OpenAPI will change to one. If this was after version 1 of an autogenerated client library, there would be a need for the library to maintain deprecated compatibility implementations of the old operations!

There are lots more examples to be found in my #18554 PR - various incorrect parameters being fixed or removed, for example.

In summary, then: the OpenAPI straight from Vault is a great starting point for client library generation, but if the library's API is to meet the stability guarantees a developer would hope for, it will be an important requirement to be able to support library specific overrides.

[averche]

Yes, I definitely agree that we will want to support some sort of library-specific overrides. As I mentioned before we have been entertaining an idea of a dedicated repository for generating and hosting openapi.json files. The repository will likely contain two flavours: one for library generation and one for documentation purposes. It is likely that the repository will use sdk/openapi.go directly (and without loading the actual plugins) rather than going though the API endpoint.

You bring up very good points regarding deprecation & backwards compatibility issues. Since our conversation last week, I had some time to think about it. In the case of an API endpoint being deprecated and removed from the library-specific API, it should definitely result in a new major version of OpenAPI-for-libraries spec (and consequently a new major version of the libraries). This seems to be a fairly established pattern for OpenAPI-generated libraries. For examples Stripe's go library is currently on major version 74 using OpenAPI version v217. The client code does not need to upgrade right away but if an when they do, they could expect some breaking code changes due to the volatile nature of code generation.

Regarding bug fixes resulting in removal of certain paths, hopefully we can fix all such issues before the 1.0 release. If not, it is still not catastrophic as we can simply bump up the major version for both OpenAPI spec and the generated libraries.

[maxb]

Fast-paced new major versions of libraries does work particularly well in the Go world, since with Go modules, a final compiled application can depend upon multiple major versions if it needs to.

In other programming language ecosystems, such as .NET, each compiled application can only depend on one version of each library, so if application A depends on libraries B and C, and B and C need incompatible versions of D, this is a problem. (The "diamond dependency" problem, as if you draw out the dependency graph between A, B, C and D it forms a diamond shape.)

Because of this, what's considered as perfectly reasonable for Go may be seen as less so for .NET. Interestingly, stripe-dotnet has had 30 major versions in the same time stripe-go has gone through 46!

In Vault and Stripe's cases, this might be OK, as both tend to be mostly used for custom business logic for a specific use-case, and unlikely to be required from multiple places deep in a dependency tree, as a common utility library would be.

One thing that might be worth considering, is making sure the library generation system has the capability to apply tweaks on top of the canonical OpenAPI document fetched from the Vault project. As an example, let's imagine that Vault 1.14 hypothetically cleans up the specification of some APIs, in a way which is perfectly compatible at the HTTP layer, but results in incompatible changes to generated code - whilst at the same time adding new APIs that you want to release to users ASAP. You might approach this by keeping a manually curated "overlay" or "patch" in the library generation repo, which temporarily preserves the older forms alongside the newer forms, until such time you need to declare a library major version increment in the future.

[averche]

Closing this in favour of #19319, let's continue the conversation there.