feat: make id_token mutator cache configurable#1177
Conversation
6146085 to
6ad3106
Compare
There was a problem hiding this comment.
To be discussed if this is really the best strategy for id_token.
I'm less familiar with other use cases of generated id_token and also how ristretto computes the "cost".
For now, this is copied from the AuthN OAuth2 introspection handler.
But perhaps saying that each id_token = 1 cost would cache too many id_tokens.
I'd like @aeneasr's opinion here :)
There was a problem hiding this comment.
In the current state it does not work, but we could at least do
NumCounters: 10000,
BufferItems: 64,
MaxCost: cost,
to keep the same behaviour as previously.
There was a problem hiding this comment.
cost = 1 << 25
as the default is definitely too many now that the cost function is 1
There was a problem hiding this comment.
Since JWTs can be quite long it may make more sense to calculate the cost based on the string length to have an approximation of storage use
There was a problem hiding this comment.
That could make sense indeed 👍
Then the default limit of 1 << 25 (~33.5 million), if we estimate an average length of JWT of a few thousands char, we should be able to store some tens of thousands of keys - which sounds reasonable.
6ad3106 to
7a46fd0
Compare
7a46fd0 to
2b89d1e
Compare
2f91f5e to
e78b048
Compare
There was a problem hiding this comment.
We have a bunch of other caches in the config schema already. Can you make your change so that it is more similar (identical) to those other cache configurations? The max_cost parameter in particular is really opaque and its impossible to come up with a reasonable value without knowing the implementation.
There was a problem hiding this comment.
I agree with the remark on max_cost 👍
A value with similar semantics is used today for the OAuth2 introspection authenticator, so I mainly mimicked this one.
Else, there is also max_tokens, used by the OAuth2 client credentials authenticator.
I think the reason for the cost instead comes from the fact that the cached objects have variable lengths, so storing a certain number of objects will result in a different cache memory usage depending on your config.
However, one can also make the decision to let the user make this decision anyway :)
Let me know what makes most sense to you, and Ory's strategy around these questions (should the product have the same defaults for everyone, or is the user trusted to configure this accordingly).
For the enabled/disabled value, I didn't re-use the existing
"handlerSwitch": {
"title": "Enabled",
"type": "boolean",
"default": false,
"examples": [true],
"description": "En-/disables this component."
},
to avoid introducing a breaking change.
Currently, the id_token cache is enabled by default, and didn't wanna change the default value to false - so I couldn't re-use this configuration.
And finally, this cache config has no TTL, because the id_token mutator already has a TTL config value for the JWT expiration date.
It make sense to me to re-use the same value => cache for 15 min if the JWT is valid 15min.
There was a problem hiding this comment.
Ideally, we would probably have one namespaced cache for all of oathkeeper and then use that everywhere, but this is good for now!
There was a problem hiding this comment.
In other caches, I believe we set the TTL to min(TTL, time.Until(expiresAt). That would make sense here too IMO.
There was a problem hiding this comment.
They should be same normally. Since calling this method does:
now := time.Now().UTC()
exp := now.Add(ttl)
[...]
a.tokenToCache(c, session, templateClaims, ttl, exp, signed)
Or what am I missing? :)
aeneasr
left a comment
aeneasr
left a comment
There was a problem hiding this comment.
This only changes cache sizes and not the actual caching function itself, right? If so I think we’re very close!
I'm glad to read this 😁 At the time of writing this patch does:
|
8c5806e to
d48aa10
Compare
d48aa10 to
4229ee6
Compare
|
Hey @aeneasr I hope you're well :) Did you get a chance to have a look again? 😇 Perhaps this can make it into the next version? |
9c749a7 to
5ffc3e7
Compare
5ffc3e7 to
a24b0a2
Compare
95d5a75 to
d2b9074
Compare
There was a problem hiding this comment.
The a.tokenCache.MaxCost() != cost condition is mainly for unit tests, in order to be able to test other max_cost values on the cache config.
On prod envs, this should not happen, and we'll keep skipping this branch.
There was a problem hiding this comment.
So basically this does hot reloading if the cost changes?
There was a problem hiding this comment.
So counters should actually be 10 * number of items. Since the cost no longer == number of items, it's not trivial to set this. But this is probably too high?
There was a problem hiding this comment.
Yeah, being able to estimate the number of JWTs that we can fit in the given max size relates to the average size of JWTs. But it's gonna be tough to get an "average JWT size" when the payload is configurable 😅
What would be a better guess in your opinion?
I pushed this for now, which would leave a bit less extra space through the number of counters.
| NumCounters: cost * 10, | |
| NumCounters: cost * 4, |
aeneasr
left a comment
aeneasr
left a comment
There was a problem hiding this comment.
Generally LGTM, a few comments
d2b9074 to
e390ea5
Compare
124120c to
30c02fa
Compare
|
Looks like we're now failing some cache tests: https://github.com/ory/oathkeeper/actions/runs/12588707987/job/35087289005?pr=1177 |
30c02fa to
d1a6359
Compare
I think we're good again 🙂 |
d1a6359 to
9075a49
Compare
9075a49 to
461e6a0
Compare
461e6a0 to
786fd8e
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughID token mutator caching transitions from a global ChangesPer-Rule ID Token Caching Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
♻️ Duplicate comments (3)
.schemas/mutators.id_token.schema.json (1)
47-51:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSpell out what
cache.max_costmeasures in this schema copy.The other schema copies now explain that the cost is the JWT string length, but this one still says only
Max cost to cache.. If docs or tooling read this file, users still get the opaque version.Suggested diff
"max_cost": { "type": "integer", "default": 33554432, "title": "Maximum Cached Cost", - "description": "Max cost to cache." + "description": "The cost of one cached JSON Web Token is the length of its string form." }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.schemas/mutators.id_token.schema.json around lines 47 - 51, Update the "max_cost" JSON schema property description so it explicitly states what is being measured (the JWT string length) and how the value is applied; locate the "max_cost" property in the schema (the integer property titled "Maximum Cached Cost") and replace the opaque description "Max cost to cache." with a clear sentence such as "Maximum cost to cache, measured as the length (in characters) of the JWT string." to match the other schema copies.pipeline/mutate/mutator_id_token.go (2)
215-223:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
NumCountersis being sized from bytes instead of expected entry count.With the default
max_cost, this setsNumCountersto134217728. Since cost is now JWT length, the cache can hold only on the order of thousands-to-tens-of-thousands of tokens, so the frequency-tracking metadata becomes vastly oversized relative to the actual cache contents.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pipeline/mutate/mutator_id_token.go` around lines 215 - 223, The NumCounters is currently computed from byte-based `cost` (MaxCost) which makes it far too large; change the logic in the ristretto.NewCache config in mutator_id_token.go so NumCounters is sized from an estimated number of entries rather than bytes: compute an estimatedEntries value (e.g., estimatedEntries := max(1, cost / expectedTokenSize) or a sensible default like 1024) and set NumCounters = estimatedEntries * 4 (or another small multiplier). Keep MaxCost as `cost` and keep the Cost func on `idTokenCacheContainer` returning token byte length; only adjust how NumCounters is derived to use estimated entry count instead of raw bytes.
41-42:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftA single shared
tokenCachecan't honor per-rule cache settings safely.
MutatorIDTokenis shared across requests, butConfig()mutates one global cache from the effective rule config. That means a rule withcache.enabled=falsestill pays the cache allocation cost, and two rules with differentcache.max_costvalues will keep replacing the cache and dropping each other’s entries. Because this swap happens on the request path without synchronization, it also introduces a data race under concurrent traffic.Also applies to: 195-231
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@pipeline/mutate/mutator_id_token.go` around lines 41 - 42, MutatorIDToken currently uses a single shared tokenCache field which is mutated in Config(), causing cross-rule interference and races; change the design so cache state is per-rule instead of a single global: in Config() allocate a per-rule cache (e.g., store a *ristretto.Cache in the rule-specific config or in a map keyed by rule ID) rather than replacing MutatorIDToken.tokenCache, and guard the map with a sync.RWMutex (or keep cache pointer on the rule config object returned by Config()); ensure when cache.enabled is false you leave the per-rule entry nil (no allocation) and when updating the cache you only set the per-rule entry under a Lock to avoid races, and update all uses to look up the per-rule cache (or nil) instead of using the single tokenCache field referenced elsewhere (references: MutatorIDToken, tokenCache, Config(), idTokenCacheContainer).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In @.schemas/mutators.id_token.schema.json:
- Around line 47-51: Update the "max_cost" JSON schema property description so
it explicitly states what is being measured (the JWT string length) and how the
value is applied; locate the "max_cost" property in the schema (the integer
property titled "Maximum Cached Cost") and replace the opaque description "Max
cost to cache." with a clear sentence such as "Maximum cost to cache, measured
as the length (in characters) of the JWT string." to match the other schema
copies.
In `@pipeline/mutate/mutator_id_token.go`:
- Around line 215-223: The NumCounters is currently computed from byte-based
`cost` (MaxCost) which makes it far too large; change the logic in the
ristretto.NewCache config in mutator_id_token.go so NumCounters is sized from an
estimated number of entries rather than bytes: compute an estimatedEntries value
(e.g., estimatedEntries := max(1, cost / expectedTokenSize) or a sensible
default like 1024) and set NumCounters = estimatedEntries * 4 (or another small
multiplier). Keep MaxCost as `cost` and keep the Cost func on
`idTokenCacheContainer` returning token byte length; only adjust how NumCounters
is derived to use estimated entry count instead of raw bytes.
- Around line 41-42: MutatorIDToken currently uses a single shared tokenCache
field which is mutated in Config(), causing cross-rule interference and races;
change the design so cache state is per-rule instead of a single global: in
Config() allocate a per-rule cache (e.g., store a *ristretto.Cache in the
rule-specific config or in a map keyed by rule ID) rather than replacing
MutatorIDToken.tokenCache, and guard the map with a sync.RWMutex (or keep cache
pointer on the rule config object returned by Config()); ensure when
cache.enabled is false you leave the per-rule entry nil (no allocation) and when
updating the cache you only set the per-rule entry under a Lock to avoid races,
and update all uses to look up the per-rule cache (or nil) instead of using the
single tokenCache field referenced elsewhere (references: MutatorIDToken,
tokenCache, Config(), idTokenCacheContainer).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f9000af2-0fd5-486f-a3bf-7decc90d1c7b
📒 Files selected for processing (5)
.schema/config.schema.json.schemas/mutators.id_token.schema.jsonpipeline/mutate/mutator_id_token.gopipeline/mutate/mutator_id_token_test.gospec/config.schema.json
786fd8e to
5327566
Compare
3 participants
Make the
id_tokenmutator cache configurable:max_costChanges to default configuration:
Previous:
New:
Related issue(s)
Follow up of #1171 and #1209 (and #1210 too a bit).
Related docs PR: ory/docs#1820
Checklist
introduces a new feature.
contributing code guidelines.
vulnerability. If this pull request addresses a security vulnerability, I
confirm that I got the approval (please contact
security@ory.sh) from the maintainers to push
the changes.
works.
Further Comments
Could probably be subject to a minor version bump, since there's a behaviour change.
Summary by CodeRabbit