Use most parameters from Credentials Cache key#3339
Conversation
The Credentials Cache produces cache hits based on the cache key, but the cached data is generated from parameters that are not always reflected in the key (e.g. `PolaricPrincipal`). As a result, it is not evident that positive cache hits always represent exactly the same credentials that would be produced from scratch. This change takes care of propagating all parameter through the cache key, except the entity, which is a bit complicated to handle and probably needs a separate PR.
|
CC: @tokoko |
|
|
||
| @Value.Parameter(order = 8) | ||
| Optional<String> principalName(); | ||
| Optional<PolarisPrincipal> principal(); |
There was a problem hiding this comment.
I thought of doing something like this before. (using key to get the value feels right). Having said that, I convinced myself that it wasn't worth it because now you're using whole PolarisPrincipal object as part of the cache key which contains more than just the name. I doubt using roles and properties will cause cache misses all that much, but you're also relying on the fact that someone in the future won't go ahead and write something more dynamic in there (like a token which was discussed before iirc).
Or maybe I'm completely misunderstanding how this immutables library works...
There was a problem hiding this comment.
right... the background for this is #3327 where, indeed, roles are used in STS calls
There was a problem hiding this comment.
okay, got it. I still think it's a ticking time bomb 😆 especially that it will affect cache even if only includePrincipalNameInSubscopedCredential is activated.
I guess the only actual solution I see for these sorts of issues is to delegate key generation to Integration classes themselves:
- Manager always loads Integration class (might have to introduce some sort of a cache for Integration classes, maybe..)
- passes all the parameters and asks for a key
- checks the cache and returns if non-empty
- passes all the parameters again now asking for a value.
But that will need a major refactor that I'm still gonna get back to soon 😆 Until then, I guess it's okay to merge this not to block the feature.
There was a problem hiding this comment.
I guess the only actual solution I see for these sorts of issues is to delegate key generation to Integration classes themselves
Yes, I actually thought about that exact approach too. I agree that it's ultimately the better solution.
I'm going to close this PR. I think there is no bug in current cache behaviour. Let's go for the proper solution.
github-project-automation
Bot
moved this from PRs In Progress
to Done
in Basic Kanban Board
2 participants
The Credentials Cache produces cache hits based on the cache key, but the cached data is generated from parameters that are not always reflected in the key (e.g.
PolaricPrincipal). As a result, it is not evident that positive cache hits always represent exactly the same credentials that would be produced from scratch.This change takes care of propagating all parameter through the cache key, except the entity, which is a bit complicated to handle and probably needs a separate PR.
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)