In my recent #21942, I overlooked the need to sort another part of the
OpenAPI document to ensure stable output.
I've also removed `strings.ToLower()` from the code I copied from, as
this code is sorting Vault API parameter names, which are all lowercase
anyway!
* OpenAPI: Define default response structure for ListOperations
Almost all Vault ListOperation responses have an identical response
schema. Update the OpenAPI generator to know this, and remove a few
instances where that standard response schema had been manually
copy/pasted into place in individual endpoints.
* changelog
* Only render StandardListResponse schema, if an operation uses it
* Teach the response schema validation test helper about the default list schema too
---------
Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com>
* OpenAPI: Fix generation of correct fields
Currently, the OpenAPI generator logic is wrong about how it maps from
Vault framework fields to OpenAPI. This manifests most obviously with
endpoints making use of `framework.OptionalParamRegex` or similar
regex-level optional path parameters, and results in various incorrect
fields showing up in the generated request structures.
The fix is a bit complicated, but in essence is just rewriting the
OpenAPI logic to properly parallel the real request processing logic.
With these changes:
* A path parameter in an optional part of the regex, no longer gets
erroneously treated as a body parameter when creating OpenAPI
endpoints that do not include the optional parameter.
* A field marked as `Query: true` no longer gets incorrectly skipped
when creating OpenAPI `POST` operations.
* changelog
* OpenAPI: Separate ListOperation from ReadOperation
Historically, since Vault's ReadOperation and ListOperation both map to
the HTTP GET method, their representation in the generated OpenAPI has
been a bit confusing.
This was partially mitigated some time ago, by making the `list` query
parameter express whether it was required or optional - but only in
a way useful to human readers - the human had to know, for example, that
the schema of the response body would change depending on whether `list`
was selected.
Now that there is an effort underway to automatically generate API
clients from the OpenAPI spec, we have a need to fix this more
comprehensively. Fortunately, we do have a means to do so - since Vault
has opinionated treatment of trailing slashes, linked to operations
being list or not, we can use an added trailing slash on the URL path to
separate list operations in the OpenAPI spec.
This PR implements that, and then fixes an operation ID which becomes
duplicated, with this change applied.
See also hashicorp/vault-client-go#174, a bug which will be fixed by
this work.
* Set further DisplayAttrs in auth/github plugin
To mask out more duplicate read/list functionality, now being separately
generated to OpenAPI client libraries as a result of this change.
* Apply requested changes to operation IDs
I'm not totally convinced its worth the extra lines of code, but
equally, I don't have strong feelings about it, so I'll just make the
change.
* Adjust logic to prevent any possibility of generating OpenAPI paths with doubled final slashes
Even in the edge case of improper use of regex patterns and operations.
* changelog
* Fix TestSudoPaths to pass again... which snowballed a bit...
Once I looked hard at it, I found it was missing several sudo paths,
which led to additional bug fixing elsewhere.
I might need to pull some parts of this change out into a separate PR
for ease of review...
* Fix other tests
* More test fixing
* Undo scope creep - back away from fixing sudo paths not shown as such in OpenAPI, at least within this PR
Just add TODO comments for now.
Vault API endpoints are defined using regexes in instances of the SDK's
framework.Path structure. However, OpenAPI does not use regexes, so a
translation is performed. It is technically possible that this
translation produces colliding OpenAPI paths from multiple
framework.Path structures. When this happens, there has formerly been no
diagnostic, and one result silently overwrites the other in a map.
As a result of this, several operations are currently accidentally
missing from the Vault OpenAPI, which is also the trigger for
https://github.com/hashicorp/vault-client-go/issues/180.
This PR adds a log message, to help catch such accidents so that they
can be fixed. Much of the PR is propagating a logger to the point where
it is needed, and adjusting tests for the API change.
With current Vault, this will result in the following being logged each
time a request is made which triggers OpenAPI generation:
```
[WARN] secrets.identity.identity_0cd35e4d: OpenAPI spec generation: multiple framework.Path instances generated the same path; last processed wins: path=/mfa/method
[WARN] secrets.identity.identity_0cd35e4d: OpenAPI spec generation: multiple framework.Path instances generated the same path; last processed wins: path=/mfa/method/totp
[WARN] secrets.identity.identity_0cd35e4d: OpenAPI spec generation: multiple framework.Path instances generated the same path; last processed wins: path=/mfa/method/okta
[WARN] secrets.identity.identity_0cd35e4d: OpenAPI spec generation: multiple framework.Path instances generated the same path; last processed wins: path=/mfa/method/duo
[WARN] secrets.identity.identity_0cd35e4d: OpenAPI spec generation: multiple framework.Path instances generated the same path; last processed wins: path=/mfa/method/pingid
```
I will submit a further PR to fix the issue - this one is just to add
the diagnostic.
* Fix non-deterministic ordering of 'required' field in OpenAPI spec
Fixes a minor annoyance I discovered whilst comparing before and after
OpenAPI specs whilst working on hashicorp/vault-client-go#180.
Sort the entries in a JSON array which has set semantics, after we
construct it by iterating a map (non-deterministic ordering).
* changelog
* Regexp metacharacter `.` should be escaped when used literally
The paths including `/.well-known/` in the Vault API could currently
technically be invoked with any random character in place of the dot.
* Replace implementation of OpenAPI path translator with regexp AST-based one
* Add changelog
* Typo fix from PR review - thanks!
Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com>
* Add comment based on review feedback
* Change style of error handling as suggested in code review
* Make a further tweak to the handling of the error case
* Add more tests, testing cases which fail with the previous implementation
* Resolve issue with a test, and improve comment
---------
Co-authored-by: Anton Averchenkov <84287187+averche@users.noreply.github.com>
* OpenAPI `generic_mount_paths` follow-up
An incremental improvement within larger context discussed in #18560.
* Following the revert in #18617, re-introduce the change from
`{mountPath}` to `{<path-of-mount>_mount_path}`; this is needed, as
otherwise paths from multiple plugins would clash - e.g. almost every
auth method would provide a conflicting definition for
`auth/{mountPath}/login`, and the last one written into the map would
win.
* Move the half of the functionality that was in `sdk/framework/` to
`vault/logical_system.go` with the rest; this is needed, as
`sdk/framework/` gets compiled in to externally built plugins, and
therefore there may be version skew between it and the Vault main
code. Implementing the `generic_mount_paths` feature entirely on one
side of this boundary frees us from problems caused by this.
* Update the special exception that recognizes `system` and `identity`
as singleton mounts to also include the other two singleton mounts,
`cubbyhole` and `auth/token`.
* Include a comment that documents to restricted circumstances in which
the `generic_mount_paths` option makes sense to use:
// Note that for this to actually be useful, you have to be using it with
// a Vault instance in which you have mounted one of each secrets engine
// and auth method of types you are interested in, at paths which identify
// their type, and for the KV secrets engine you will probably want to
// mount separate kv-v1 and kv-v2 mounts to include the documentation for
// each of those APIs.
* Fix tests
Also remove comment "// TODO update after kv repo update" which was
added 4 years ago in #5687 - the implied update has not happened.
* Add changelog
* Update 18663.txt
* Revert "Add mount path into the default generated openapi.json spec (UI) (#17926)"
This reverts commit db8efac708.
* Revert "Remove `generic_mount_paths` field (#18558)"
This reverts commit 79c8f626c5.
Move version out of SDK. For now it's a copy rather than move: the part not addressed by this change is sdk/helper/useragent.String, which we'll want to remove in favour of PluginString. That will have to wait until we've removed uses of useragent.String from all builtins.
The current behaviour is to only add mount paths into the generated `opeanpi.json` spec if a `generic_mount_paths` flag is added to the request. This means that we would have to maintain two different `openapi.json` files, which is not ideal. The new solution in this PR is to add `{mount_path}` into every path with a default value specified:
```diff
-- "/auth/token/accessors/": {
++ "/auth/{mount_path}/accessors/": {
"parameters": [
{
"name": "mount_path",
"description": "....",
"in": "path",
"schema": {
"type": "string",
++ "default": "token"
}
}
],
```
Additionally, fixed the logic to generate the `operationId` (used to generate method names in the code generated from OpenAPI spec). It had a bug where the ID had `mountPath` in it. The new ID will look like this:
```diff
-- "operationId": "listAuthMountpathAccessors",
++ "operationId": "listTokenAccessors",
```
strings.ReplaceAll(s, old, new) is a wrapper function for
strings.Replace(s, old, new, -1). But strings.ReplaceAll is more
readable and removes the hardcoded -1.
Signed-off-by: Eng Zer Jun <engzerjun@gmail.com>
* Refactor table driven tests to use subtests
* sdk/framework: add TypeSignedDurationSecond FieldType
Adds the TypeSignedDurationSecond FieldType which accepts positive and
negative durations. The existing TypeDurationSecond FieldType does not
accept negative durations.
* Add tests for 0 for TypeDurationSecond and TypeSignedDurationSecond
