OIDC: Limit number of active dynamic tenants

[snazy]

Description

Currently the number of OIDC tenants is unlimited. A global, static map of "tenant ID" to TenantConfigContext is held in i.q.oidc.runtime.OidcRecorder (propagated to i.q.oidc.runtime.TenantConfigBean).

Some scenarios may create new OIDC tenant configs at runtime. Either by having some functionality to change the configuration (and generate a new tenant-ID) or by adding new OIDC tenant configs. Both use cases lead to new TenantConfigContexts being created, but never cleaned up.

Proposal:

• Add a configuration option to limit the number of dynamic OIDC tenants
• Replace the static CHM in OidcRecorder with a bounded Caffeine cache
• Use the Caffeine cache to limit the number of active OIDC tenant contexts and automatically close old ones
• The Caffeine cache could also be used to automatically close inactive OIDC tenant contexts after some period of time to safe resources (at the cost of a somewhat higher latency when inactive contexts are used again).

New Configuration properties

int quarkus.oidc.dynamic-tenants.limit (default to Integer.MAX_VALUE to retain the current behavior) Optional<Duration> quarkus.oidc.dynamic-tenants.close-inactive-after (empty = don't close inactive tenant contexts, other value = configure via Caffeine's expireAfterAccess)

Implementation ideas

No response

[quarkus-bot[bot]]

/cc @pedroigor (oidc), @sberyozkin (oidc)

[sberyozkin]

Hi @snazy Adding the limit is good, but I'm not too keen to add caffeine dependency...My preference would be to add a timestamp and get rid of the oldest ones when the limit is reached as opposed to having a timer probing the map all the time. IMHO we are talking about a very edge case here where a high limit is reached...

The other reason I'm not too keen on quarkus.oidc.dynamic-tenants.close-inactive-after, as it will be hard to implement right, and can be confusing. Static tenants can also be inactive. There is also a concept of the session refreshment, users will be confused what value it should be set to, we have a session extension property, etc

[sberyozkin]

Hi @snazy So we have DefaultTenantConfigResolver keeping hold of static, dynamic tenants and I guess we kind of meant with Pedro, that there could be custom managers. So may be (though I'm not proposing to do it right now), we can eventually make that pluggable, where users could apply whatever dynamic tenant management policy they want.

Thinking more about it, we can probably clean up dynamic tenants proactively as you suggest, but I'd just avoid any specialized cache implementations, and have properties similar to quarkus.oidc.token-cache properties, max time to live, which is used to clean oldest ones when the limit is reached, and there is a property there to clean up periodically using a Vert.x timer

[snazy]

Oh, static tenants can "leak" into the dynamic tenant map?

BTW: there's no timer in Caffeine or so. It's rather that Caffeine expires those when it "comes along" those.

[snazy]

Let me see whether that works with the approach you suggested.

[sberyozkin]

Hi @snazy When the static tenant is not ready at the start up, it is initialized at the first request using the same function which is dealing with the dynamic tenant registration, so indeed, in such cases, the statically configured tenants may end up recorded in the dynamic tenants map, since the function is updating the dynamic tenants map.

Perhaps the function should be modified to distinguish between the actual dynamic tenants and the static tenants which are initialized dynamically. I can handle it independently.

My comment was related to the fact that the tenant activity is also relevant to the static tenants, but we don't want to remove them, and the whole inactivity concept is confusing enough with the session management alone, which is why I was not too keen on it.

Hopefully, we can start with the basic strategy of getting rid of the oldest dynamic tenants once the map reaches the limit, and then I can investigate how to get that pluggable