GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | ||||||||||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | ||||||||||||||||||||||||||||||||||
GeezerVison is showing github.com — [pipeline-manager] Keep cached JWKS keys when a refresh fails by blp · Pull Request #7865 · feldera/feldera Conversation
|
Known kid, keys older than 120 s |
Before | After |
|---|---|---|
| Refresh succeeds | key | key |
| Refresh fails, or issuer cooling down | 401 | cached key, plus a warning in the log |
Refresh succeeds, but the kid was removed |
401 | 401 |
| Keys older than 1 h, refreshes failing | n/a | 401 |
Rationale
- A failed fetch says the issuer is unreachable, not that our keys are wrong. Issuers publish a new key before they sign with it, and they keep the old key published for a time after. So keys from a recent successful fetch are almost always still correct. Refusing every token because of a network error is the worse choice.
- The cooldown stays as it was. It limits an outage to one fetch per cooldown. A shorter cooldown would cause more fetches during an outage and would still refuse tokens for the shorter time.
- The fix is on the server, not in the CI client. A longer client retry would hide the problem in CI only. Other federated clients would still get 30 s of 401s.
- Risk: a key that the issuer removed, for example after a leak. We see a removal only on a successful refresh. A successful refresh replaces all keys of the issuer, so a removed key is refused at once. While the issuer stays unreachable, the cached keys stay in use for at most 1 h. That is long enough for a short outage, and it bounds how long a removed key is still accepted.
This touches auth.rs together with #7862. The two changes are independent, but one of them will need a rebase.
Describe Manual Test Plan
New unit tests. Each fails when its part of the fix is reverted:
| Test | Reverted change that makes it fail |
|---|---|
a_failed_refresh_keeps_the_cached_keys |
usable() limited to the fresh age |
a_refresh_drops_keys_the_issuer_removed |
refresh merges keys instead of replacing them |
concurrent_stale_hits_survive_a_failed_refresh (suggested in review) |
the cooldown branch refuses stale keys |
The last test covers the CI case: requests that wait on the gate while a refresh fails. It also checks both edges of the 1 h limit. All 28 auth:: tests pass, and clippy is clean.
Checklist
- Unit tests added/updated
- Integration tests added/updated
- Documentation updated
- Changelog updated
Breaking Changes?
- OpenAPI / REST HTTP API / feldera-types / manager
- Feldera SQL (Syntax, Semantics)
- feldera-sqllib
- Python SDK
- fda (CLI arguments)
- Adapters (including configuration)
- Storage Format / Checkpoints
- Others (specify)
Describe Incompatible Changes
None.
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
All reactions
|
Review: LGTM. What I ran:
The gates are met: there are unit tests for both halves of the fix, and they use paused tokio time, so they cannot flake. The changelog is updated, no dependencies change (
Missing (low-medium), test coverage: no test covers requests that queue on the gate while a refresh fails. Those requests reach the + /// Concurrent stale hits during an outage share one failed fetch, and the + /// request queued on the gate still gets the cached key. + #[tokio::test(start_paused = true)] + async fn concurrent_stale_hits_survive_a_failed_refresh() { + let cache = Arc::new(Mutex::new(super::IssuerJwkCache::new())); + let fetches = Arc::new(AtomicUsize::new(0)); + assert!(resolve_counting(&cache, &fetches, "real", true).await.is_ok()); + tokio::time::advance(super::JWK_FRESH_FOR).await; + let (first, second) = tokio::join!( + resolve_counting(&cache, &fetches, "real", false), + resolve_counting(&cache, &fetches, "real", false), + ); + assert!(first.is_ok() && second.is_ok()); + assert_eq!(2, fetches.load(Ordering::SeqCst)); + // Just inside the stale limit, after the cooldown: still usable. + tokio::time::advance(super::JWK_STALE_LIMIT - super::JWK_FRESH_FOR - Duration::from_millis(1)).await; + assert!(resolve_counting(&cache, &fetches, "real", false).await.is_ok()); + tokio::time::advance(Duration::from_millis(1) + COOLDOWN).await; + assert!(resolve_counting(&cache, &fetches, "real", false).await.is_err()); + } |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
happy to rebase #7862 on top of this / when this merges |
All reactions
-
❤️ 1 reaction
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
|
Added -- Claude Code, replying at @blp's request. |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Cached signing keys expired 120 s after a fetch. The next request then refreshed them, and every refresh, success or failure, starts a cooldown that rejects cache misses without a fetch. So one failed JWKS fetch refused every token of the issuer with a 401 for the whole cooldown, 30 s for federated tokens. CI authenticates to its instances with federated GitHub tokens, and its retry of a 401 comes too soon to outlast that. A failed fetch tells us that the issuer is unreachable, not that the keys we have are wrong. Issuers publish a new key before they sign with it and keep the old one published for a time after, so keys from a recent successful fetch are almost always still correct. Refusing every token because of a network error is the worse choice. So keys older than 120 s are still refreshed on use, but if the refresh fails, or the issuer is cooling down, the keys from the last successful refresh stay in use. The cooldown stays as it was, because it is what keeps an outage to one fetch per cooldown. Fixing only the CI client would leave other federated clients with the same 30 s of 401s. The risk is a key that the issuer removed, for example after a leak. We see the removal only on a successful refresh. A successful refresh replaces all keys of the issuer, so a removed key is refused at once. While the issuer stays unreachable, the cached keys stay in use for at most 1 hour, long enough for a short outage and short enough to bound the time a removed key is still accepted. Related to #7048. Signed-off-by: Ben Pfaff <[email protected]>
04de839 to
3003873
Compare
|
fixed the merge conflict in changelog, merging this |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Thank you! |
All reactions
Sorry, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Reviewers
Uh oh!
There was an error while loading. Please reload this page.
Assignees
Labels
Projects
Milestone
No milestoneDevelopment
Successfully merging this pull request may close these issues.
Uh oh!
There was an error while loading. Please reload this page.
2 participants
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.