Skip to content

Make KeycloakAuthManager cache configurable - #70839

Open
stephen-bracken wants to merge 1 commit into
apache:mainfrom
stephen-bracken:configure-cache
Open

Make KeycloakAuthManager cache configurable#70839
stephen-bracken wants to merge 1 commit into
apache:mainfrom
stephen-bracken:configure-cache

Conversation

@stephen-bracken

@stephen-bracken stephen-bracken commented Jul 31, 2026

Copy link
Copy Markdown
Contributor

Add configuration settings for KeycloakAuthManager single_flight() cache

Allow deployment managers to configure the cache timeout on KeycloakAuthManager bulk authorisation methods.

  • Add cache_ttl_seconds option to configure _CACHE_TTL_SECONDS in KeycloakAuthManager
  • Add cache_timeout_seconds option to configure _SINGLE_FLIGHT_TIMEOUT_SECONDS in KeycloakAuthManager
Was generative AI tooling used to co-author this PR?
  • Yes (please specify the tool below)
  • No

@stephen-bracken
stephen-bracken force-pushed the configure-cache branch 6 times, most recently from b71e2e0 to 27348aa Compare July 31, 2026 18:26
@stephen-bracken
stephen-bracken marked this pull request as ready for review July 31, 2026 22:51
@stephen-bracken
stephen-bracken force-pushed the configure-cache branch 2 times, most recently from 1b432a9 to d777fc9 Compare August 1, 2026 00:11
@potiuk

potiuk commented Aug 1, 2026

Copy link
Copy Markdown
Member

Sensible thing to make configurable — a hardcoded 30 second auth-decision cache is exactly the sort of thing deployments need to tune. Two things before merge.

version_added needs a real version. Every other option in provider.yaml carries one (0.0.1, 0.4.0), and the equivalent FAB option uses version_added: 3.2.0. The provider is currently at 0.8.2, so this wants 0.9.0:

      cache_ttl_seconds:
        description: |
          Time (in seconds) to cache auth decisions in ``bulk is_authorized_{resource}`` methods.
        type: integer
        version_added: 0.9.0
        example: ~
        default: "30"

The import-time read is what forces reload() in the tests, and that's worth avoiding.

_CACHE_TTL_SECONDS = conf.getint(CONF_SECTION_NAME, CONF_CACHE_TTL_SECONDS_KEY, fallback=30)

Because this is evaluated once at import, the tests need an autouse fixture that reloads the module to change it. Reloading mid-suite is fragile — it rebinds _cache and _pending_requests to fresh dicts while the _clear_cache fixture is holding references to the previous ones, so the two fixtures are operating on different objects depending on ordering. The autouse fixture also pins the TTL to 60 for every pre-existing test in the file, quietly changing their baseline.

I know FabAuthManager does read its cache_ttl at import, so there is precedent. But it's precedent worth breaking rather than following, because FAB shows the cost: its test decorates with

@conf_vars({("fab", "cache_ttl"): "1"})
def test_deserialize_user(...):

and that has no effect whatsoever — CACHE_TTL was already baked into TTLCache(maxsize=1024, ttl=CACHE_TTL) when the class body was evaluated at import. The test passes because caching happens at all, not because the configured TTL was honoured. You clearly spotted this hazard, which is why the reload is there at all.

Reading it lazily makes the whole problem go away:

def _cache_ttl_seconds() -> int:
    return conf.getint(CONF_SECTION_NAME, CONF_CACHE_TTL_SECONDS_KEY, fallback=30)

with the two call sites using _cache_ttl_seconds() instead of the constant. Then conf_vars works normally, the reload fixture disappears, test_configure_ttl becomes a plain assertion, and — the part that matters for operators — the setting can be changed without restarting the API server, which is most of the reason to make it configurable in the first place.


Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting

@stephen-bracken stephen-bracken changed the title Make KeycloakAuthManager _CACHE_TTL_SECONDS configurable Make KeycloakAuthManager _CACHE_TTL_SECONDS and _SINGLE_FLIGHT_TIMEOUT_SECONDS configurable Aug 1, 2026
@stephen-bracken

Copy link
Copy Markdown
Contributor Author

updated docs, added lazy init to _CACHE_TTL_SECONDS, also made _SINGLE_FLIGHT_TIMEOUT_SECONDS configurable in the same way

@stephen-bracken stephen-bracken changed the title Make KeycloakAuthManager _CACHE_TTL_SECONDS and _SINGLE_FLIGHT_TIMEOUT_SECONDS configurable Make KeycloakAuthManager cache configurable Aug 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants