Skip to content

Commit

Permalink
OpenAPI: Separate ListOperation from ReadOperation
Browse files Browse the repository at this point in the history 
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.
  • Loading branch information
@maxb
maxb committed Jul 10, 2023
1 parent e29842e commit e6718ff
Show file tree
Hide file tree
Showing 2 changed files with 63 additions and 28 deletions.
89 changes: 62 additions & 27 deletions sdk/framework/openapi.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -107,13 +107,12 @@ type OASLicense struct {
}

type OASPathItem struct {
Description string `json:"description,omitempty"`
Parameters []OASParameter `json:"parameters,omitempty"`
Sudo bool `json:"x-vault-sudo,omitempty" mapstructure:"x-vault-sudo"`
Unauthenticated bool `json:"x-vault-unauthenticated,omitempty" mapstructure:"x-vault-unauthenticated"`
CreateSupported bool `json:"x-vault-createSupported,omitempty" mapstructure:"x-vault-createSupported"`
DisplayNavigation bool `json:"x-vault-displayNavigation,omitempty" mapstructure:"x-vault-displayNavigation"`
DisplayAttrs *DisplayAttributes `json:"x-vault-displayAttrs,omitempty" mapstructure:"x-vault-displayAttrs"`
Description string `json:"description,omitempty"`
Parameters []OASParameter `json:"parameters,omitempty"`
Sudo bool `json:"x-vault-sudo,omitempty" mapstructure:"x-vault-sudo"`
Unauthenticated bool `json:"x-vault-unauthenticated,omitempty" mapstructure:"x-vault-unauthenticated"`
CreateSupported bool `json:"x-vault-createSupported,omitempty" mapstructure:"x-vault-createSupported"`
DisplayAttrs *DisplayAttributes `json:"x-vault-displayAttrs,omitempty" mapstructure:"x-vault-displayAttrs"`

Get *OASOperation `json:"get,omitempty"`
Post *OASOperation `json:"post,omitempty"`
Expand Down Expand Up @@ -309,6 +308,7 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc *

// Process each supported operation by building up an Operation object
// with descriptions, properties and examples from the framework.Path data.
var listOperation *OASOperation
for opType, opHandler := range operations {
props := opHandler.Properties()
if props.Unpublished || forceUnpublished {
Expand All @@ -324,11 +324,6 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc *
}
}

// If both List and Read are defined, only process Read.
if opType == logical.ListOperation && operations[logical.ReadOperation] != nil {
continue
}

op := NewOASOperation()

operationID := constructOperationID(
Expand Down Expand Up @@ -408,24 +403,17 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc *
}
}

// LIST is represented as GET with a `list` query parameter.
// LIST is represented as GET with a `list` query parameter. Code later on in this function will assign
// list operations to a path with an extra trailing slash, ensuring they do not collide with read
// operations.
if opType == logical.ListOperation {
// Only accepts List (due to the above skipping of ListOperations that also have ReadOperations)
op.Parameters = append(op.Parameters, OASParameter{
Name: "list",
Description: "Must be set to `true`",
Required: true,
In: "query",
Schema: &OASSchema{Type: "string", Enum: []interface{}{"true"}},
})
} else if opType == logical.ReadOperation && operations[logical.ListOperation] != nil {
// Accepts both Read and List
op.Parameters = append(op.Parameters, OASParameter{
Name: "list",
Description: "Return a list if `true`",
In: "query",
Schema: &OASSchema{Type: "string"},
})
}

// Add tags based on backend type
Expand Down Expand Up @@ -521,18 +509,65 @@ func documentPath(p *Path, backend *Backend, requestResponsePrefix string, doc *
switch opType {
case logical.CreateOperation, logical.UpdateOperation:
pi.Post = op
case logical.ReadOperation, logical.ListOperation:
case logical.ReadOperation:
pi.Get = op
case logical.DeleteOperation:
pi.Delete = op
case logical.ListOperation:
listOperation = op
}
}

openAPIPath := "/" + path
if doc.Paths[openAPIPath] != nil {
backend.Logger().Warn("OpenAPI spec generation: multiple framework.Path instances generated the same path; last processed wins", "path", openAPIPath)
// Write the regular, non-list, OpenAPI path to the OpenAPI document, UNLESS we generated a ListOperation, and
// NO OTHER operation types. In that fairly common case (there are lots of list-only endpoints), we avoid
// writing a redundant OpenAPI path for (e.g.) "auth/token/accessors" with no operations, only to then write
// one for "auth/token/accessors/" immediately below.
//
// On the other hand, we do still write the OpenAPI path here if we generated ZERO operation types - this serves
// to provide documentation to a human that an endpoint exists, even if it has no invokable OpenAPI operations.
// Examples of this include kv-v2's ".*" endpoint (regex cannot be translated to OpenAPI parameters), and the
// auth/oci/login endpoint (implements ResolveRoleOperation only, only callable from inside Vault).
generateNonListOpenAPIPath := listOperation == nil || pi.Get != nil || pi.Post != nil || pi.Delete != nil
if generateNonListOpenAPIPath {
openAPIPath := "/" + path
if doc.Paths[openAPIPath] != nil {
backend.Logger().Warn(
"OpenAPI spec generation: multiple framework.Path instances generated the same path; "+
"last processed wins", "path", openAPIPath)
}
doc.Paths[openAPIPath] = &pi
}

// If there is a ListOperation, write it to a separate OpenAPI path in the document.
if listOperation != nil {
// Typically, we will append a slash here to disambiguate from the path written immediately above.
// However, if we skipped writing the path above (and so don't need to worry about disambiguating from it),
// AND the path already contains a trailing slash, we want to avoid doubling it.
if generateNonListOpenAPIPath || !strings.HasSuffix(path, "/") {
path += "/"
}

listPathItem := OASPathItem{
Description: pi.Description,
Parameters: pi.Parameters,
DisplayAttrs: pi.DisplayAttrs,

// Since the path may now have an extra slash on the end, we need to recalculate the special path
// matches, as the sudo or unauthenticated status may be changed as a result!
Sudo: specialPathMatch(path, sudoPaths),
Unauthenticated: specialPathMatch(path, unauthPaths),

Get: listOperation,
}

openAPIPath := "/" + path
if doc.Paths[openAPIPath] != nil {
backend.Logger().Warn(
"OpenAPI spec generation: multiple framework.Path instances generated the same path; "+
"last processed wins", "path", openAPIPath)
}
doc.Paths[openAPIPath] = &listPathItem
}
doc.Paths[openAPIPath] = &pi
}

return nil
Expand Down
2 changes: 1 addition & 1 deletion vault/logical_system_paths.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3658,7 +3658,7 @@ func (b *SystemBackend) policyPaths() []*framework.Path {

DisplayAttrs: &framework.DisplayAttributes{
OperationPrefix: "policies",
OperationVerb: "list",
OperationSuffix: "acl-policy-list2", // this endpoint duplicates /sys/policies/acl
},

Operations: map[logical.Operation]framework.OperationHandler{
Expand Down

0 comments on commit e6718ff

Please sign in to comment.