VAULT-19233 First part of caching static secrets work #23054
Conversation
github-actions
bot
added
the
hashicorp-contributed-pr
|
CI Results: |
|
Build Results: |
| // which is used for static secret caching, and enabling multiple | ||
| // tokens to be able to access the same cache entry for static secrets. | ||
| // Required: false, Unique: false | ||
| Tokens []string |
There was a problem hiding this comment.
I updated this to be a map[string]struct{} so that values had to be unique. Coming in the next PR.
| @@ -337,6 +416,15 @@ func (c *LeaseCache) Send(ctx context.Context, req *SendRequest) (*SendResponse, | |||
| return resp, nil | |||
| } | |||
|
|
|||
| // TODO: if secret.MountType == "kvv1" || secret.MountType == "kvv2" | |||
There was a problem hiding this comment.
In addition to that condition, remember that we'll have to think more about method type: DELETE, PATCH, etc.
| } | ||
|
|
||
| // TODO: We need to also update the cache for the token's permission capabilities. | ||
| // TODO: for this we'll need: req.Token, req.URL.Path |
There was a problem hiding this comment.
Just in case it wasn't clear, this is also coming in the next PR, to better separate the kinds of caching and to make it easier to review. I thought I'd leave this here so it was obvious what was missing!
| // the X-Vault-Token header) to remain agnostic to which token is being | ||
| // used in the request. We care only about the path. | ||
| func computeStaticSecretCacheIndex(req *SendRequest) string { | ||
| return hex.EncodeToString(cryptoutil.Blake2b256Hash(req.Request.URL.Path)) |
There was a problem hiding this comment.
Shouldn't we care about the namespace header though?
There was a problem hiding this comment.
Hmm, that's a good point. We'll have to ensure that the namespace header and the path get added together, e.g. a request to ns1/secret should be the same as a request to secret with ns1 as the namespace header. This is actually a shortcoming/bug of the existing approach for dynamic secrets -- we don't treat the above the same
There was a problem hiding this comment.
I'll make sure to handle this and add a test for it. If you don't mind, I'd prefer to do this in the next PR, as I made a slight change to the static secret cache index in that PR
| @@ -337,6 +416,15 @@ func (c *LeaseCache) Send(ctx context.Context, req *SendRequest) (*SendResponse, | |||
| return resp, nil | |||
| } | |||
|
|
|||
| // TODO: if secret.MountType == "kvv1" || secret.MountType == "kvv2" | |||
There was a problem hiding this comment.
In addition to that condition, remember that we'll have to think more about method type: DELETE, PATCH, etc.
| @@ -263,19 +318,52 @@ func (c *LeaseCache) Send(ctx context.Context, req *SendRequest) (*SendResponse, | |||
| case <-inflight.ch: | |||
| } | |||
| } else { | |||
| inflight = newInflightRequest() | |||
| if inflight == nil { | |||
There was a problem hiding this comment.
What's the purpose of this if? When do we want to re-use an existing inflight?
There was a problem hiding this comment.
It's so that we have one inflight irrespective of if it's a dynamic or static request, essentially. It prevents double defer close(inflight.ch) and defer inflight.remaining.Dec() etc.
There was a problem hiding this comment.
How could we have two? How could the same request give rise to a cache hit for both keys?
There was a problem hiding this comment.
It won't result in an inflight cache hit for both keys, but we take the else branch if we get a cache miss. We also don't know at this stage if it's a cache hit for either the static secret ID or the dynamic secret ID
There was a problem hiding this comment.
To put it another way, it's likely for most requests to result in a cache miss for both of the inflight cache IDs
There was a problem hiding this comment.
This particular else could never see a non-nil inflight though, right? For the second case, the static if/else, might it be clearer to wrap the entire thing in an if inflight == nil instead? If we have already found the dynamic cache entry, there's no point in doing idLockStaticSecret.Lock and consulting the cache again, is there?
There was a problem hiding this comment.
We still need to set both of the IDs in the cache (we don't know what kind of request it is at this stage) and unlock the inflight cache lock, so that behaviour can't be conditional. I might be misunderstanding, though?
We only have one request inflight (the stuff protected by the if inflight == nil) but we need to lock the IDs as it could be dynamic or static (or neither)
There was a problem hiding this comment.
Ah, I'm the one who was missing something, I agree. Though I still feel the first if inflight == nil is a no-op, the one this comment is tied to.
What this does:
If a new config value is set, then we cache all secrets as static secrets. We check new requests coming in to see if they can share the path. If they have demonstrated they can access this secret before, they'll receive the cached value.
This won't affect any old functionality, as it relies on the new config value being set to true. It's far from a complete solution, but I'd rather have 'smaller' PRs than a couple of thousand line monsters, for reviewers' sanity and also code quality purposes.
What's missing:
I've set no changelog for this, as I'll do a large one as a
featurechangelog near the end.