KV helper methods for api package #15305
Conversation
api/kv.go
Outdated
| var metadata *KVMetadata | ||
|
|
||
| // deletion_time usually comes in as an empty string which can't be | ||
| // processed as time.RFC3339, so we reset it to a convertible value |
There was a problem hiding this comment.
If anyone knows how to get the DecodeHook to shut up and accept the empty string as time.RFC3339, lemme know. I tried the ,omitempty and ,string struct tags and still no good. So I'm just kinda hacking around this manually.
api/kv.go
Outdated
| OldestVersion int `mapstructure:"oldest_version"` | ||
| UpdatedTime time.Time `mapstructure:"updated_time"` | ||
| // Keys are stringified ints, e.g. "3" | ||
| Versions map[string]VersionMetadata `mapstructure:"versions"` |
There was a problem hiding this comment.
I think it would be nicer UX to have this as either map[int]VersionMetadata or []VersionMetadata (sorted by version, since Version is already in the struct)
There was a problem hiding this comment.
I couldn't find a way to make mapstructure happy if I had it as an int (since it comes in as a string), maybe there's some decode hook I could use but couldn't find it... 🤔 I did add a GetVersionsAsList method though so that folks have access to a sorted slice as well.
… it as part of helper method
* Add Read methods for KVClient * KV write helper * Add changelog * Add Delete method * Use extractVersionMetadata inside extractDataAndVersionMetadata * Return nil, nil for v1 writes * Add test for extracting version metadata * Split kv client into v1 and v2-specific clients * Add ability to set options on Put * Add test for KV helpers * Add custom metadata to top level and allow for getting versions as sorted slice * Update tests * Separate KV v1 and v2 into different files * Add test for GetVersionsAsList, rename Metadata key to VersionMetadata for clarity * Move structs and godoc comments to more appropriate files * Add more tests for extract methods * Rework custom metadata helper to be more consistent with other helpers * Remove KVSecret from custom metadata test now that we don't append to it as part of helper method * Return early for readability and make test value name less confusing
|
@digivava Great idea for this convenience functions - I'm struggling to find an equivalent for |
|
@universam1 Good question! At least at the time I implemented this, I chose not to do List just because it behaves pretty differently from the other KV commands and thus didn't really match the KVv2 method signature here. With the other KV commands, when you say That said, I'm sure its absence from these helpers sometimes trips people up! I'll let @hashicorp/vault-devex speak to whether they have plans for a List helper in the future, but in the meantime, you can of course always fall back on the old |
|
Thanks for the answer @digivava There the condition for v1 vs v2 is clearly handled. I'm not really understanding what you mean how List path is different than a Put/Get and so on, it is always the full path from root, right? |
Purpose
The Developer Experience team has observed many users struggling to figure out how to perform simple KV secret reads and writes in their application code. Not only are the Logical() methods hard to discover because of their naming, they also require users to know about the internal naming discrepancy between KV v1 and KV v2 (
secret/foovssecret/data/foo).Adding to this difficulty is the fact that in the case of KV v2, an additional layer called "data" must be parsed from the response, resulting in code that looks like
secret.Data["data"], which many users have had trouble discovering.These helper methods seek to eliminate those difficulties by providing an intuitive method that handles both the /data in the URL and the "data" object unwrapping for the user.
Usage
client.KVv1("secret").Read(ctx, "foo")andclient.KVv2("secret").Read(ctx, "foo"). No need for /data, hooray!Both will return a *KVSecret, the KVv2 one will take care of the "data" unwrapping for you.
Design Notes
Thanks @averche for the initial POC on this! One of the ideas that has stuck around from that POC is how the original
*api.Secretis returned as part of the KVSecret struct, so that users can easily pass it on to LifetimeWatcher.There was some discussion over whether to provide a separate method for each KV version (e.g.
client.KVv2("secret")) , or to specify the KV secrets engine version as a parameter (e.g.client.KV("secret", 2)or the enum version,client.KV("secret", api.KVv2Version)).After considering the various options, I decided to go with the
client.KVv2("secret")approach, mostly because it's simple. Having to specify an int every time looks awkward, and the enum might also be hard to discover. We could instead specify a NewKVClient struct and initialize the mount path and version as part of that, but that wouldn't be very consistent with theclient.ActionType().Action()style that we've been using in the rest of this package (client.Logical().Read,client.Sys().Renew, etc.) Some discussion was also had about automatically detecting the version, but this would require a hidden extra call to thesys/internal/ui/mountsendpoint every time the function is called, which doesn't feel like a great pattern.Other methods
Other KV-related methods like Destroy, DeleteMetadata, Rollback, etc. can come in subsequent PRs as needed. This is meant to be more of a stop-gap helper for just the basic CRUD functionality until the next major iteration of our client library.