Vault 9800 Fix vault read handling for endpoints with no top-level data object #17913
Conversation
|
The changes look good to me. It might be prudent to add a short test to verify that |
| // to use this data in case there is no top-level data from | ||
| // api response | ||
| var teebuf bytes.Buffer | ||
| tee := io.TeeReader(r, &teebuf) |
There was a problem hiding this comment.
Do we have to read twice? It looks like we could possibly just make a copy of the original Decode response, rather than calling again. Or would that not work for some reason?
There was a problem hiding this comment.
I tried it, once the buffer is used to decode, the pointer reaches the end of the buffer. If we try to decode this again, the data in the buffer is nil.
| // If the data is null, add raw data to secret data if present | ||
| if secret.Data == nil { | ||
| data := make(map[string]interface{}) | ||
| if err := jsonutil.DecodeJSONFromReader(&teebuf, &data); err != nil { |
There was a problem hiding this comment.
I think you can do something like this:
| if err := jsonutil.DecodeJSONFromReader(&teebuf, &data); err != nil { | |
| if err := mapstructure.Decode(secret, &data); err != nil { |
and then we don't need a second copy of the stream above. I think this might be a good optimization because it prevents duplicating the stream every time we read, regardless of whether we have a Data field. In this case, we'll only need to make a copy of the data on-demand.
How do you feel about this?
There was a problem hiding this comment.
Except for the fact that it doesn't work 😆 . I'd have to think a bit more about how to get this correct, but it seems like it should be achievable...
There was a problem hiding this comment.
I don't think this would work as the secret would not have any data after decode in this case
There was a problem hiding this comment.
What we're looking for would be a way to be aware that some JSON keys didn't map to the Secret upon decode. I'm pretty sure that isn't possible. I think in order to achieve a single read would be to forego the decode to a Secret and instead use custom processing. The second decode isn't ideal for performance reasons but it's likely only going to happen in very few cases. It's probably a reasonable tradeoff to avoid introducing potential regressions by changing the vault read decode semantics completely. If there is a low risk way to prevent a subsequent decode then I'm all for it though!
There was a problem hiding this comment.
Looks like you can just decode to a map[string]interface{} and then decode that into secret/secret.Data.
What do you think about something like this instead?
// First decode the JSON into a map[string]interface{}
data := make(map[string]interface{})
if err := jsonutil.DecodeJSONFromReader(&buf, &data); err != nil {
return nil, err
}
var secret Secret
if err := mapstructure.Decode(data, &secret); err != nil {
return nil, err
}
// If the data is null, add raw data to secret data if present
if secret.Data == nil {
if err := mapstructure.Decode(data, &secret.Data); err != nil {
return nil, err
}
}There was a problem hiding this comment.
I'm not sure if this gains us anything, but we at least avoid creating two copies of the buffer for every read.
There was a problem hiding this comment.
Ah, sorry I think I conflated the conversation of a copy of the buffer and the requirement for 2 decodes. I think that the subsequent decode is necessary given my points above. So the tradeoff here is add a copy of the buffer versus adding a second decode for every read. Then in the off chance we don't have data, a third decode is required. I don't think that I feel that strongly about either approach.
There was a problem hiding this comment.
No worries, thanks for hopping in the thread! I'm actually sort of convinced that mapstructure.Decode with a decoder hook is the correct approach here: https://pkg.go.dev/github.com/mitchellh/mapstructure#DecodeHookFunc.
I think maybe we could use this to avoid making a copy of the stream and decode in one pass.
If we're not super concerned with optimizing this path, I'm cool with that. Just figured I'd raise the topic for conversation in case it was a concern.
There was a problem hiding this comment.
For full context, @akshya96 and I met synchronously about it last week so I had some prior context. It's totally worth exploring if there is a low risk of regression.
There was a problem hiding this comment.
Will create a follow up ticket to figure out which is the best approach.
https://hashicorp.atlassian.net/browse/VAULT-9800
The response bodies for the sys/health and sys/leader endpoints do not include a top-level data object. The vault read command expects this top-level field unless -format=raw (a recent addition) is used. It is entirely possible that issue exists for other endpoints and solving for these two likely solves for all.
Issue:
vault read sys/health -format=json
{
"request_id": "",
"lease_id": "",
"lease_duration": 0,
"renewable": false,
"data": null,
"warnings": null
}
vault read sys/health
(No response)
Sample response:
vault read sys/health
Key Value
cluster_id 9501c424-662b-01ef-c5f0-0f7a2e48575d
cluster_name vault-cluster-3d7e0d93
initialized true
performance_standby false
replication_dr_mode disabled
replication_performance_mode disabled
sealed false
server_time_utc 1668198854
standby false
version 1.13.0-dev1
vault read sys/health -format=json
{
"request_id": "",
"lease_id": "",
"lease_duration": 0,
"renewable": false,
"data": {
"cluster_id": "9501c424-662b-01ef-c5f0-0f7a2e48575d",
"cluster_name": "vault-cluster-3d7e0d93",
"initialized": true,
"performance_standby": false,
"replication_dr_mode": "disabled",
"replication_performance_mode": "disabled",
"sealed": false,
"server_time_utc": 1668198684,
"standby": false,
"version": "1.13.0-dev1"
},
"warnings": null
}