Fix Cache.clean_expired purging still-valid entries (mtime != expiry)#69591
Open
cstrahan wants to merge 1 commit into
Open
Fix Cache.clean_expired purging still-valid entries (mtime != expiry)#69591cstrahan wants to merge 1 commit into
cstrahan wants to merge 1 commit into
Conversation
The generic Cache.clean_expired() fallback (used by the localfs driver,
which has no clean_expired() of its own) treated the driver's updated()
value -- the file mtime -- as the entry's expiry:
ts = updated(bank, key) # localfs: file mtime (always in the past)
if ts is not None and ts <= time.time():
flush(bank, key)
Because a file's mtime is always <= now, every entry was flushed on every
sweep. The master's periodic token maintenance (clean_expired_tokens ->
cache.clean_expired("tokens")) therefore deleted ALL eauth tokens on each
pass, regardless of their real expiry. A token thus lived only until the
next maintenance sweep (0-~60s), not token_expire (default 12h).
This broke long-running salt-api operations: a normal request uses its
token within ~1s, but a batch job runs gather + waits its per-chunk
timeout (default 60s) before issuing saltutil.find_job, by which point the
token had been swept -> "Authentication error occurred." -> HTTP 401
("Authentication denied" in pepper).
clean_expired() now consults each entry's stored _expires instead of the
mtime, removing only genuinely-expired entries and leaving entries without
an expiry envelope untouched.
Also fix LoadAuth.mk_token passing the absolute expiry timestamp where
Cache.store expects a duration (seconds); it placed _expires ~2x now in the
future so token entries were never cleaned even once the above is fixed.
Both defects were introduced together in 3008.0 by commit 78685e5
("Provide token storage using the salt.cache interface"), which moved
eauth token storage off the localfs token driver and onto the generic
salt.cache interface. That commit added the clean_expired() fallback (with
the mtime bug) and the mk_token cache.store(..., expires=...) call (with the
duration bug) in one go. Token eviction is therefore a regression: 3006.x
and 3007.x are unaffected (they stored tokens via the localfs token driver,
which never hit this path).
Introduced-by: 78685e5 ("Provide token storage using the salt.cache interface")
Fixes saltstack#69590
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
2 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 does this PR do?
Fixes eauth tokens being evicted within seconds of issuance on 3008, regardless of
token_expire.The generic
Cache.clean_expired()fallback (used by thelocalfsdriver, which has noclean_expired()of its own) treated the driver'supdated()value — the file mtime — as the entry's expiry. A file's mtime is always<= now, so every entry was flushed on every sweep. The master's periodic token maintenance (clean_expired_tokens→cache.clean_expired("tokens")) therefore deleted all eauth tokens on each pass.This PR makes
clean_expired()consult each entry's stored_expiresenvelope instead of the mtime (matching howCache.fetch()already detects that envelope), removing only genuinely-expired entries. It also fixesLoadAuth.mk_tokenpassing the absolute expiry timestamp whereCache.storeexpects a duration in seconds.Both defects were introduced together in 3008.0 by
78685e5c44("Provide token storage using the salt.cache interface"), which moved token storage onto the genericsalt.cacheinterface. 3006.x/3007.x are unaffected (they used the localfs token driver, which never hit this path).What issues does this PR fix or reference?
Fixes #69590
Previous Behavior
A freshly-minted token worked for the first request or two, then failed with
Authentication error occurred./ HTTP 401 (TokenExpiredErrorin the master log). Tokens lived only until the next maintenance sweep (0–~60s), nottoken_expire(default 12h). This broke long-running salt-api operations — notably batch jobs, whose mid-runsaltutil.find_jobre-auth happened after the token was already swept.New Behavior
Tokens remain valid until their real expiry (
token_expire); only genuinely-expired cache entries are purged, and entries with no expiry envelope are left untouched.Merge requirements satisfied?
[NOTICE] Bug fixes or features added to Salt require tests.
Commits signed with GPG?
No