fix: Reuse the OIDC JWKS client across requests - #6683
Merged
franciscojavierarceo merged 5 commits intoJul 31, 2026
Merged
Conversation
_decode_token constructed a fresh PyJWKClient on every call. The JWK-set cache lives on the instance, so every authenticated request started cold and paid a full HTTPS fetch of the JWKS document (TLS handshake included) before signature verification. That blocking I/O also runs on the event loop, so concurrent requests serialized behind it. Build the client lazily once per parser instead (the parser is constructed once per process by init_auth_manager), letting PyJWT's built-in JWK-set cache (default lifespan 300s) do its job. IdP key rotation stays safe: PyJWKClient.get_signing_key refreshes the set and retries once whenever it sees an unknown kid. Local benchmark, 25 authenticated decodes against a mock JWKS endpoint: 25 JWKS fetches before, 1 after. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
Floor pyjwt at 2.13.0: the reuse makes PyJWT's JWK-set cache semantics load-bearing, and only >=2.13 keeps the cache through transient fetch failures (2.5-2.12 wipe it; pre-2.5 has no cache, silently reverting the fix). Pass lifespan and timeout explicitly so upgrades cannot change the documented staleness window and a hung IdP bounds how long a fetch can block the serving path. Scope the rotation docstring honestly: new-kid rotations recover immediately, but a removed key keeps validating and a reused-kid rotation keeps failing for up to the cache lifespan. Test hardening from mutation results: pin the SSL context per verify_ssl config (both dropping ssl_context and inverting the guard previously left all tests green), pin laziness (no construction before the first request), pin per-parser scoping (second parser builds its own client), and pin construction-failure recovery (a failed first build must not wedge the process-singleton parser). Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6683 +/- ##
==========================================
+ Coverage 46.76% 46.77% +0.01%
==========================================
Files 414 414
Lines 50186 50191 +5
Branches 7180 7181 +1
==========================================
+ Hits 23467 23475 +8
+ Misses 25079 25077 -2
+ Partials 1640 1639 -1
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
Contributor
Author
|
Adding a deployment-shaped before/after to complement the mock benchmark in the description. Setup: this branch built from source (Dockerfile.dev at 602feb0) versus the pinned 0.65.0 release image, both serving the same feature repository against the same Redis online store on the same machine, authenticating real client-credentials tokens from a live Entra ID tenant. The only variable is this change.
The 250 ms baseline matches what we measured server-side on a real deployment (ALB TargetResponseTime p50 247 ms), where the JWKS endpoint fetch cost 124-196 ms per request. One expected behavior worth noting: a cold-started process pays its single JWKS fetch on the first request, and a burst arriving before that completes queues briefly behind it; after that it is steady state, with a refresh at most once per lifespan (300 s). We saw no failures at 10 concurrent readers on a warm process. |
ntkathole
reviewed
Jul 31, 2026
Review feedback on feast-dev#6683: expose the two PyJWKClient tunables as OidcAuthConfig options rather than hardcoding them. jwks_cache_lifespan_seconds (default 300) also bounds how long a key the IdP has revoked keeps validating tokens, so operators whose provider rotates or revokes aggressively can tighten it at the cost of more JWKS fetches. jwks_request_timeout_seconds (default 10) bounds how long an unresponsive IdP blocks the serving path. Both reject non-positive values at config load: a zero or negative lifespan would expire the cache immediately and silently restore a JWKS fetch per request, undoing this PR. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
Member
|
@larrysingleton007 looks good, want to handle operator support in same PR or different one? |
Contributor
Author
|
@ntkathole These two are tuning knobs rather than IdP-coupled values, so they don't really fit the Secret whitelist that audience and issuer went through in #6677. They belong next to verifySSL as CR fields on OidcAuthz, which means a CRD change and regenerated manifests, so a different review surface than this Python diff. There's also a sequencing argument: operator support is only useful once this lands in a release the operator can point at, same as #6670 and #6677 last week. I'll open an issue and follow with the PR. Happy to fold it in here instead if you'd rather have it as one changeset. |
Contributor
Author
|
Filed as #6686 with the CR-field plan spelled out. |
ntkathole
approved these changes
Jul 31, 2026
CI runs pixi install --locked, which fails at setup when pyproject.toml and pixi.lock disagree, so adding the pyjwt>=2.13.0 bound took out all five pixi-based integration jobs before any test ran. Regenerate the lock; the only change is the corresponding requires_dist entry. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
3 tasks
ntkathole
pushed a commit
to larrysingleton007/feast
that referenced
this pull request
Aug 14, 2026
Follow-up to feast-dev#6683, requested in its review. Add jwksCacheLifespanSeconds and jwksRequestTimeoutSeconds to OidcAuthz as CR fields rather than OIDC Secret keys: these are non-secret operational knobs, so they belong with verifySSL and caCertConfigMap rather than in the Secret bag that carries IdP-coupled credentials. Both are optional pointers with a Minimum=1 constraint mirroring the SDK's validation, and are omitted from the generated feature_store.yaml when unset so the SDK defaults apply rather than the operator asserting its own. Regenerates deepcopy, CRD bases, dist/install.yaml, and the API reference. Documents that the cache lifespan is not purely a performance setting: it also bounds how long a key the provider revoked keeps validating tokens. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
ntkathole
pushed a commit
that referenced
this pull request
Aug 14, 2026
* feat: Expose the OIDC JWKS tunables through the operator Follow-up to #6683, requested in its review. Add jwksCacheLifespanSeconds and jwksRequestTimeoutSeconds to OidcAuthz as CR fields rather than OIDC Secret keys: these are non-secret operational knobs, so they belong with verifySSL and caCertConfigMap rather than in the Secret bag that carries IdP-coupled credentials. Both are optional pointers with a Minimum=1 constraint mirroring the SDK's validation, and are omitted from the generated feature_store.yaml when unset so the SDK defaults apply rather than the operator asserting its own. Regenerates deepcopy, CRD bases, dist/install.yaml, and the API reference. Documents that the cache lifespan is not purely a performance setting: it also bounds how long a key the provider revoked keeps validating tokens. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> * fix: Regenerate the OLM bundle and close review gaps Code review findings on the JWKS tunables change. The OLM bundle CRD was not regenerated, so it lacked both new fields while config/crd/bases and dist/install.yaml carried them. Every other OidcAuthz field is present in all three copies, and no PR workflow runs make bundle, so CI would not have caught it: an OLM install would have pruned the settings silently rather than failing. Regenerated with make bundle; operator-sdk bundle validate passes. Assert the client repo config omits both keys. OidcClientAuthConfig inherits the same strict validation, so mirroring the forwarding into the client path would break every client pod, and nothing tested it. Also restore the neighbouring blocks' length assertion so a leaked parameter fails. Add CRD validation tests for the Minimum=1 constraints the docs promise. Docs: caCertConfigMap was documented as a bare string but the CRD requires an object with a name key, so the whole snippet failed to apply, including the lines added here. Name the required Feast version instead of implying any newer image works. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> * docs: State that the OIDC authz options require apiVersion v1 The CRD serves v1alpha1 alongside v1 with no conversion webhook, so a resource submitted as v1alpha1 is validated against the v1alpha1 schema and any field outside it is pruned without error. Under v1alpha1, authz.oidc accepts only secretRef, so every other option is silently dropped. This predates the JWKS settings: v1alpha1 has never carried issuerUrl, secretKeyName, tokenEnvVar, verifySSL or caCertConfigMap either, all of which landed v1-only in 7c04026. Documenting the whole section rather than the two new fields keeps the guidance consistent with that. The v1alpha1 schema already enforces this - the fields cannot be expressed there - so no validation change is needed, only the missing warning. Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com> --------- Signed-off-by: Larry Singleton <166439969+larrysingleton007@users.noreply.github.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
What this PR does / why we need it
OidcTokenParser._decode_tokenbuilds a freshPyJWKClienton every call. The JWK-set cache lives on the instance, so every authenticated request starts cold and pays a full HTTPS fetch of the JWKS document (TLS handshake included) before signature verification. On a production-shaped deployment that fetch measured 124-196 ms against the IdP, dominating single-entity online reads (server-side p50 247 ms while the online store idled), and because it's blocking I/O on the event loop, concurrent requests serialize behind it: 3 parallel readers measured an exact 3x of single-request latency, and 10 produced load-balancer 502s from a healthy process.The fix builds the client lazily once per parser (the parser is a process singleton created by
init_auth_manager), letting PyJWT's JWK-set cache work. Reproducible benchmark against a counting mock JWKS endpoint with real RS256 tokens: 25 authenticated decodes performed 25 JWKS fetches before, 1 after.Details worth reviewer attention:
pyjwtnow has a>=2.13.0floor: the reuse makes PyJWT's cache semantics load-bearing, and only 2.13+ keeps the cache through transient fetch failures (2.5-2.12 wipe it on one failed fetch; pre-2.5 has no cache at all, silently reverting the fix). The lockfiles already resolve 2.13.0, so no lock changes.lifespan=300andtimeout=10are passed explicitly so upgrades can't silently change behavior. The lifespan bounds two staleness windows that were zero with per-request clients and are documented in the code: a key the IdP removed keeps validating, and a rotation that reuses akidkeeps failing, each for at most 300 s. Rotations that mint a newkid(the common case) recover immediately becausePyJWKClient.get_signing_keyrefreshes and retries on a cache miss.verify_ssl(mutation testing showed the SSL branch previously had zero coverage), laziness (no construction before the first request, keeping the blocking discovery fetch out of server boot), per-parser scoping, reuse across requests, and recovery after a failed first construction (the parser is a process singleton, so a wedged client would require a restart).Known accepted residuals, for transparency: on the threaded Arrow Flight server a cold-start burst can still build the client more than once (benign, self-heals, and PyJWT's own cold-fetch path has the same property); PyJWT's JWK-set cache is unlocked so simultaneous expiry can trigger redundant refetches (bounded by concurrency). Both are strictly better than the guaranteed fetch-per-request they replace. The client-side token fetch (
client_secretflow) has the same per-request pattern and is a natural follow-up.To be precise about the concurrency claim: this removes the accidental serialization behind per-request network I/O; it does not add parallelism to the server.
Testing: 311 permissions unit tests pass (including the new lifecycle tests); full unit suite green (2488 passed); ruff and mypy clean.
Which issue(s) this PR fixes
Fixes #6682