list of subkeys with KvV2ReadMetadata is not working

[ha36d]

Expected Behavior

retreiving subkeys with KvV2ReadMetadata:

v := url.Values{}
v.Add("list", "true")
keys, err := client.Secrets.KvV2ReadMetadata(ctx, "", vault.WithMountPath("secret"), vault.WithCustomQueryParameters(v))

should work as

curl \                                                                                                                                                                      ─╯
    --header "X-Vault-Token: *******" \
    --request GET \
    http://127.0.0.1:8200/v1/kv-v2/metadata/\?list\=true

and showing the results as

{"request_id":"fff681f2-dd47-b4fc-6ca5-268a8a833828","lease_id":"","renewable":false,"lease_duration":0,"data":{"keys":["my-secret"]},"wrap_info":null,"warnings":null,"auth":null}

Current Behavior

It's showing:

{false 0001-01-01 00:00:00 +0000 UTC 0 map[] 0 0 0 0001-01-01 00:00:00 +0000 UTC map[]}

Failure Information

Vault v1.12.1 ('e34f8a14fb7a88af4640b09f3ddbb5646b946d9c+CHANGES'), built 2022-10-27T12:32:05Z
github.com/hashicorp/vault-client-go v0.3.2

Steps to Reproduce

Using this function:

keys, err := client.Secrets.KvV2ReadMetadata(ctx, "", vault.WithMountPath("secret"))

[averche]

Hi @ha36d, thanks for bringing this to our attention!

The root cause of the issue is that kv-v2/metadata/{path} endpoint supports both GET and LIST operations. They are unfortunately mapped to the same get operation in the generated OpenAPI schema (see
hashicorp/vault#12283 for more details). As a result, client.Secrets.KvV2ReadMetadata returns a "LIST" response which gets incorrectly marshaled into KvV2ReadMetadataResponse.

There doesn't seem to be a simple solution to this issue, but we'll try to consider different options. For now, a simple workaround is to use client.List instead:

keys, err := client.List(ctx, "secret/metadata/")
if err != nil {
	log.Fatal(err)
}
log.Println(keys.Data)

[maxb]

They are unfortunately mapped to the same get operation in the generated OpenAPI schema (see
hashicorp/vault#12283 for more details).

Hi @averche ! As a result of that Vault issue I filed, it's now possible to inspect the OpenAPI to split apart the GET and LIST operations.

An OpenAPI "get" that represents LIST only will have a parameter like this:

    {
        "name": "list",
        "description": "Must be set to `true`",
        "in": "query",
        "schema": {
            "type": "string",
            "enum": [
                "true"
                ]
            },
        "required": True
    }

whereas an endpoint that supports both GET and LIST will have a parameter like this:

    {
        "name": "list",
        "description": "Return a list if `true`",
        "in": "query",
        "schema": {
            "type": "string"
        }
    }

[averche]

Thanks @maxb,

This still leaves the library in a peculiar state. Ideally, we would like to have two separate methods, one for Read and one for List, each with the corresponding response structures. This would fit well with the existing pattern of methods.

Alternatively, we could go with a simpler approach of having a single method and returning a generic Response[map[string]interface{}]. This would work but would not be as convenient.

[maxb]

Ideally, we would like to have two separate methods, one for Read and one for List, each with the corresponding response structures.

I would go so far as to say that's a hard requirement, not an ideal that can be optionally implemented - because to do otherwise would be to create an interface that is bent out of shape from what users expect, to satisfy an internal implementation problem - which would contribute to a perception that auto-generated API clients are worse than hand-crafted ones.

My aim in my previous comment was just to show that there is sufficient information in the OpenAPI to reverse-engineer whether a Vault endpoint supports read, list, or both.

However I realise now you mention response structures, that the addition of response structures to the Vault OpenAPI has created a rather awkward situation, where the Vault read and list operations were previously sharing a single Vault endpoint.

I have an idea ... we could change the Vault OpenAPI so that instead of merging read and list operations into a single GET endpoint in the OpenAPI spec, we could have read rendered as it is now, but switch list to be rendered as a GET operation with an additional trailing slash in the URL. This would match how Vault canonicalises paths for list requests internally anyway, and would cure the overloading of two Vault operations into one OpenAPI endpoint. It would make the resulting OpenAPI document closer to the intentions of the specification, by removing one of Vault's special peculiarities that a human must know when reading it - which of course confuses automated tools such as the openapi-generator.

Ironically, this is what I originally thought of back when I opened that issue quoted above, back in 2021, but I discounted the option at the time, as response structures weren't populated, and I wanted to propose the easiest smallest viable change :-)

Let me know your thoughts, and if it seems viable to you, I'll put in a Vault PR for you to consider further.

[averche]

I have an idea ... we could change the Vault OpenAPI so that instead of merging read and list operations into a single GET endpoint in the OpenAPI spec, we could have read rendered as it is now, but switch list to be rendered as a GET operation with an additional trailing slash in the URL.

I really like this idea! I've put together a simple experiment (#181) to ensure that openapi-generator is OK with two paths that differ only by the trailing slash. It seems to work quite well!

[maxb]

I started playing with some code changes to implement this, and things got ... interesting.

My first thought was to remove the special hack we have in the code that expands Vault path regexes to OpenAPI paths, that avoids returning both foo and foo/, when the regex ends with a conditional slash - and then assign the ListOperation to the path with a slash, and the rest to the path without a slash.

So far, so good - but then I realised that this is failing to take account of paths that have a ListOperation as part of a 'match all' regex - such as kv-v2/metadata/{path}. I guess that means I need to split this into two OpenAPI paths:

• kv-v2/metadata/{path}
• kv-v2/metadata/{path}/

That's OK-ish I guess?

Around this time, I tested out some behaviour in the Swagger API Explorer, and ended up discovering that currently, if you try to manipulate a KV value with slashes in its path via the API Explorer, the slashes actually get URL-encoded! Doing some more reading, it turns out that formally, OpenAPI does not actually support expressing this kind of API at all and a related issue has been open since 2017, with prospects of a resolution not looking good: OAI/OpenAPI-Specification#892

Indeed, looking at the vault-client-go source code, it appears that it too will suffer from this issue.

That leaves us in the unpleasant position of having to deviate from the OpenAPI spec in order to represent the Vault API. I suppose we could probably get away with just declaring path parameters that are named 'path' as special, and deactivating conventional URL encoding in that case.

EDIT: In actual testing I was unable to reproduce the behaviour I thought I had seen. I must have misinterpreted somehow. In actual fact Vault accepts a request to http://localhost:8200/v1/kv-v2/metadata/foo%2Fbar and processes it against the secret at foo/bar. This surprises me, but it all works out OK.

Anyway, that was a digression. Back to how to trigger the Vault OpenAPI generator to render both kv-v2/metadata/{path}
and kv-v2/metadata/{path}/ into the output:

Option 1: Build some additional smarts into the regex analysis, so that it can know whether any given OpenAPI path generated from regex analysis will or will not match slash as the last character of a parameter. If it will, add an extra copy of the OpenAPI path to the generated paths, with an appended slash.

Option 2: Just make an assumption, that if a ListOperation is defined, that the regex author will have written it correctly. Don't generate the extra OpenAPI path during regex expansion - do it later, when looping over generated paths and generating the OpenAPI document.

And that is the design issue I was blocked on... but in the process of typing this up, I've had a realization:

If I make any alteration to the number of OpenAPI paths generated from existing regex expansions, I'm going to cause absolute chaos with all those pipe-separated strings being used to assign operation IDs.

So I think that locks in Option 2 as the design path to take.

[averche]

I slightly prefer Option 2 even if the Operation ID's were not of concern.

Just make an assumption, that if a ListOperation is defined, that the regex author will have written it correctly.

Would it be possible for us to fix the existing built-in plugins where this is not the case without breaking backwards compatibility?

[maxb]

Just make an assumption, that if a ListOperation is defined, that the regex author will have written it correctly.

Would it be possible for us to fix the existing built-in plugins where this is not the case without breaking backwards compatibility?

In the event the regex was supposed to accept a trailing slash but doesn't, or vice versa, the relevant operations would simply be entirely impossible to invoke, so we would be free of any compatibility worry.

I've now opened hashicorp/vault#21723 to implement, and #190 to showcase the result on the client library.