OIDC: Limit number of dynamic tenants #42804
Draft
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sberyozkin
reviewed
Aug 27, 2024
|
🎊 PR Preview 646c96b has been successfully built and deployed to https://quarkus-pr-main-42804-preview.surge.sh/version/main/guides/
|
snazy
force-pushed
the
oidc-context-limit
branch
2 times, most recently
from
905ff84
to
919d339
Compare
|
Marking this PR as draft, I'm reworking the changes. The current state is rather a preparation. |
sberyozkin
reviewed
Sep 2, 2024
| } | ||
| } | ||
|
|
||
| public TenantConfigContext getExistingTenantConfigContext(String tenantId) { |
There was a problem hiding this comment.
I'd consider Dynamic instead of Existing
|
Hi @snazy Thanks, I've had a quick look, and it looks good |
snazy
added a commit
to snazy/quarkus
that referenced
this pull request
Sep 4, 2024
…enants This is a prerequisite for quarkusio#42804 w/o any functional change. It moves the code to get/add tenants into the `TenantConfigBean`, so the limitation for the amount of dynamic tenants can be done there.
snazy
added a commit
to snazy/quarkus
that referenced
this pull request
Sep 5, 2024
…enants This is a prerequisite for quarkusio#42804 w/o any functional change. It moves the code to get/add tenants into the `TenantConfigBean`, so the limitation for the amount of dynamic tenants can be done there.
snazy
added a commit
to snazy/quarkus
that referenced
this pull request
Sep 5, 2024
…enants This is a prerequisite for quarkusio#42804 w/o any functional change. It moves the code to get/add tenants into the `TenantConfigBean`, so the limitation for the amount of dynamic tenants can be done there.
snazy
force-pushed
the
oidc-context-limit
branch
from
919d339
to
a41ea94
Compare
snazy
changed the title
snazy
force-pushed
the
oidc-context-limit
branch
from
a41ea94
to
d0257dc
Compare
snazy
changed the title
snazy
force-pushed
the
oidc-context-limit
branch
2 times, most recently
from
05f0d34
to
cdfb3d1
Compare
snazy
commented
Sep 7, 2024
| return new TenantConfigBean(staticTenantsConfig, defaultTenantContext, new LongSupplier() { | ||
| @Override | ||
| public long getAsLong() { | ||
| return System.nanoTime(); |
There was a problem hiding this comment.
Could change to currentTimeMillis() - however not sure whether all OSes (looking at you, Windows...) work fine then.
Lets `TenantConfigBean` be the sole "owner" of the static/dynamic tenants maps, adds/changes accessor methods for tenants. Also introduces a functional interface to create tenants. No functional change, only moving code around. Also some minor code cleanups (reducing the amount of warnings in IntelliJ).
…ved public functions, simplify some more code
Introduce a new configuration option `quarkus.oidc.dynamic-tenant-limit` as an opt-in to limit the number of active dynamic OIDC tenants. The default is no dynamic tenant limit. This change adds the field `volatile long TenantConfigContext.lastUsed`, and removes the field `TenantConfigContext.ready` to keep the heap footprint the same. The `lastUsed` field is initially set to "now" and updated when the dynamic tenant is being accessed. When a new dynamic tenant is about to be added to the dynamic tenants map, eviction runs, if there are more dynamic tenants than configured via `dynami-tenant-limit`. The eviction algo is built to iterate over the dynamic tenants only once - it may need to iterate more often, if one of the eviction candidates has been accessed in the mean time. There is no linked-list structure to form an LRU list/queue, as that likely causes more runtime overhead (pointer updates, synchronization) than the cost of iterating over the list once in a while. The assumption is that the map of dynamic tenants has not a lot of "churn".
snazy
force-pushed
the
oidc-context-limit
branch
from
cdfb3d1
to
91f6dee
Compare
|
Thanks @snazy, I'll have a closer look once the PRs it depends on are merged. For ex, look at the back channel logout cache and corresponding configuration properties in the Logout config group. Now that you have encapsulated the dynamic map nicely, I believe we should just have DynamicTenantsMap extending the abstract cache impl, instead of the dynamic tenants map, which should also terminate the optional Vertx timer on the shutdown |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Introduce a new configuration option
quarkus.oidc.dynamic-tenant-limitas an opt-in to limit the number of active dynamic OIDC tenants. The default is no dynamic tenant limit.This change adds the field
volatile long TenantConfigContext.lastUsed, and removes the fieldTenantConfigContext.readyto keep the heap footprint the same. ThelastUsedfield is initially set to "now" and updated when the dynamic tenant is being accessed.When a new dynamic tenant is about to be added to the dynamic tenants map, eviction runs, if there are more dynamic tenants than configured via
dynami-tenant-limit. The eviction algo is built to iterate over the dynamic tenants only once - it may need to iterate more often, if one of the eviction candidates has been accessed in the mean time. There is no linked-list structure to form an LRU list/queue, as that likely causes more runtime overhead (pointer updates, synchronization) than the cost of iterating over the list once in a while. The assumption is that the map of dynamic tenants has not a lot of "churn".Fixes #42803