10:36 p.m.
We have still open issue https://issues.jboss.org/browse/KEYCLOAK-838 .
In shortcut, during authentication of user into LDAP there are 9 same
queries to LDAP to load this single user. Similar situation might be for
other federation providers. Few ideas to improve:
1) Simple per-request caching at UserFederationManager level - currently
each call to "session.users().getUserByXXX()" always call "isValid" on
federation provider causing that validation/loading of same user might
be done more time per request. With very simple enhancement at
UserFederationManager level, we ensure that loading of user will be done
just once per request (per KeycloakSession), which is not an issue IMO
as KeycloakSession is really short-living object. Note that this is not
specific to LDAP, but should improve performance of UserFederation in
general. I have very simple patch here
https://github.com/mposolda/keycloak/commit/f8302dac0e927756ca0d49a67500f...
. With this, number of loads to LDAP per authentication is reduced from
9 calls to 3. Any objections against merging it?
2) Global caching - this is obviously much more tricky, but I think that
earlier or later we need to address it as (1) might not be sufficient.
Few ideas:
2.a) Allow to switch the user cache on top of delegation. Currently we
have UserProvider delegation chain like:
User Federation => User Cache Provider => JPA/Mongo UserProvider
The alternative I mean is, that people will have possibility to
configure the chain like:
User Cache Provider => User Federation => JPA/Mongo UserProvider
By default, the order will be still the same like it's now, however I
can imagine that for many LDAP (and other federation) deployments, the
admins use them just as read-only store and don't do any direct changes
here. Hence there is no reason to always validate/proxy users and ask
federation storage if user still exists etc but instead suffer from our
already existing cache impl and enjoy improved performance.
I propose we will just add on/off switch to admin console, which will
allow to specify when cache should be put on top of federation or after
(like it's now). We may also need to add the button to clear the cache.
WDYT?
2.b) Another alternative is to put cache layer on top of
LDAPIdentityStore. TBH I am not the fan of it as it's specific to LDAP
and it requires another implementation of caching with all the pitfalls
(Figure which items should be invalidated during writes, abstraction to
support both "mem" and cluster/infinispan deployments etc...)
my proposal is to go with (1) for this release and then look at (2.a)
if I have time as it may be another few days of work (so maybe postpone
to next release)
WDYT, other better ideas to improve?
Marek