feat: add cache to hydrator #418
Conversation
There was a problem hiding this comment.
Sweet! I think we will be able to use this in a couple of places!
Could you please add some docs to the hydrator so that this feature is documented?
pipeline/mutate/mutator_hydrator.go
Outdated
| @@ -104,6 +143,13 @@ func (a *MutatorHydrator) Mutate(r *http.Request, session *authn.AuthenticationS | |||
| return errors.WithStack(err) | |||
| } | |||
|
|
|||
| if cfg.Cache.Ttl != "" { | |||
There was a problem hiding this comment.
Should we not also respsect Cache-Control headers? Maybe the server doesn't want us to cache the requests and sends Cache-Control: no-cache or some other cache value?
There was a problem hiding this comment.
There's actually a http.NetTransport that does that: https://github.com/gregjones/httpcache
There was a problem hiding this comment.
That library will only work for GET requests. I imagine the hydrator is mostly used where someone is controlling both Oathkeeper and the hydrate service.
There was a problem hiding this comment.
Ah, I see - yeah I think it makes sense. We can also add this at a later stage anyways as it won't be a breaking change.
Co-Authored-By: hackerman <[email protected]>
…thkeeper into feature/hydrateCache
|
Linter complains because of format: |
…in id token mutator" This reverts commit c8e6b3a.
|
Thank you, this looks perfect :) |
|
I found an issue where the downstream id token mutator adds header information that gets put into the hydrate cache, and with a large volume of calls will sometimes cause a concurrent write error in the id token mutator. |
|
Do you know how to fix this? |
|
I was able to fix it by using a deep copy library in and out of the cache (https://github.com/mitchellh/copystructure). The other option might be to just cache the json response instead of the de-serialized struct. |
Related issue
#417
Proposed changes
Add ability to configure a cache for hydrate calls
Checklist
vulnerability. If this pull request addresses a security. vulnerability, I
confirm that I got green light (please contact
[email protected]) from the maintainers to push
the changes.
works.
Further comments
Works similar to the id token cache, but uses a
cache_ttlconfig to determine if it should cache calls