Short lived cache for keycloak queries ?

[camunda-community-hub]

Short lived cache for keycloak queries ?

[VonDerBeck]

Hi @juja0,

thanks for your thoughts. Read this

https://medium.com/outbrain-engineering/oh-my-guava-we-are-moving-to-caffeine-99387819fdbb

Caffeine smells better than Guava... And you should be quickly familiar with it because
"Caffeine library is a rewrite of Guava’s cache that uses a Guava-inspired API that returns CompletableFutures, allowing asynchronous automatic loading of entries into a cache."

Dependencies seem to be ok - and much less than with guava cache:

We just have to add

	<dependency>
	    <groupId>com.github.ben-manes.caffeine</groupId>
	    <artifactId>caffeine</artifactId>
	</dependency>

to the pom, the version is already managed by existing BOMs. That's fine as well as I don't need to regularly update hundreds of different dependency versions but can concentrate mainly on Spring Boot and Camunda and that's it.

I think we should go for Caffeine. But to be honest: I had only a quick look at the caching topic. Why do you think, that "taking care of cases like race conditions and concurrent access might end up being more challenging than it seems"?

Thanks,
Gunnar

[juja0]

Great. I'll stick to caffeine then. I'll start working on the PR.

As to why I think a self written caching solution might be challenging to maintain, it's just a combination of paranoia due to prior bad experiences and a general aversion to re-implementing something that's been done already by other libraries which probably are much better tested and well maintained. For example, there was a case in an older codebase where the application always used to slow down towards the end of the week and since the project was huge, it was hard to pin down what the problem was. Turned out to be a simple cache implemented using ConcurrentHashMap but entries were strong references and were never evicted and hence never garbage collected. In another case, a simple cache was implemented using map.computeIfAbsent but the computation logic had to call some other functionality which was also modifying the same cache so it ended in a concurrentmodification exception and this was a very one off scenario and was never caught in unit testing and broke in production. And in keycloakUserService and keycloakGroupService we do have some methods which call other methods. Even though we are not planning to cache the nested method calls now, if we plan to do it later, we certainly may end up in a similar situation.

[VonDerBeck]

Hi @juja0,

that sounds like an excellent idea - and pull requests are welcome.

Some hints / thoughts: the cache should be a) optional as well as b) have a configurable timeout. Lastly, we should be careful to introduce new dependencies and keep the number low.

• What Cache library do you intend to use? Or do you plan to implement that on your own?

• Regarding point 3 "Implement caching on the RestTemplate used and not on KeycloakUserService and KeycloakGroupService": there is already a custom extended RestTemplate (see KeycloakRestTemplate.java) which includes a retry functionality in case of HTTP 401 responses. Honestly I'm not sure if this is the place to implement the cache - because you're already on an HTTP level here.

• It sounds more reasonable to have KeycloakUserQuery / KeycloakGroupQuery act as cache keys and move the cache mechanism to KeycloakIdentityProviderSession.findUserByQueryCriteria(...) / KeycloakIdentityProviderSession. findGroupByQueryCriteria(...). This would be minimal invasive.

What do you think?

Gunnar

[juja0]

Great.

• In our project we've used guava cache due to its simplicity (especially in handling timeout based eviction) and the fact that it was already on our classpath. I agree that adding the entire guava library as a dependancy might be a bit overkill. I came across this other library caffeine but I'm not sure if it will be a lighter dependency compared to guava. While it may be lighter to create our own caching solution, taking care of all possible cases like race conditions and concurrent access might end up being more challenging than it seems. One possible solution is to maybe create an abstraction for the cache in the main module and maybe provide a guava/caffeine based cache as a separate module which if present in the classpath will be used and if not, caching will be disabled. I don't have a preference or strong opinion on this so I'm open to thoughts.

• Now that I think about it, caching at the RestTemplate level may not be the best idea since we may have to store entire (or atleast most of) RequestEntity and ResponseEntity objects in memory and that may quickly add up. I agree that using the query objects as cache keys is probably better and I'll go ahead with that implementation.

Once we agree on the cache library/cache module approach, I'll start working on a PR.

Thanks,

[VonDerBeck]

One last hint:

• For the implementation I would concentrate on KeycloakIdentityProviderSession.findUserByQueryCriteria(...) / KeycloakIdentityProviderSession. findGroupByQueryCriteria(...) - that should be sufficient for what we want to achieve.
• There are some special methods not running over those 2 main query methods - I would never cache these special ones. For example when checking the login of a user caching will be contraproductive.

Looking forward to your PR :-)

[juja0]

Sure. Makes sense.

I've submitted a draft PR based on our discussion so far.

I'll update documentation and other housekeeping tasks after we discuss and agree on the changes that have been made.

[juja0]

Closing because #65 has been merged.