api: account for the HTTP Age header when renewing leases through a cache - #32040
api: account for the HTTP Age header when renewing leases through a cache#32040peteski22 wants to merge 5 commits into
Conversation
A caching proxy reports how stale its response is in the HTTP Age header, but lease_duration in the body still reads from issuance. LifetimeWatcher ignores Age, so the remaining lifetime it computes never falls and the threshold at which it gives up is never reached. Shows: for a 6s lease returned with Age 5s, DoneCh never fires, so the caller is never told to re-read the secret. Ref: hashicorp#19227
The same root cause has a second consequence: because the watcher sleeps on the reported lease_duration rather than what is left of it, it wakes and renews long after the credential is gone. Every such renewal is a request Vault and its secrets backend must serve for a lease that no longer exists. Shows: for a 6s lease returned with Age 5s, only 1s of life remains, but the watcher renews again at ~4.2s. Extends the caching proxy stub to time the renewals it receives. Ref: hashicorp#19227
ParseSecret sees only the response body, so the Age header was discarded and callers could not tell that the lease duration they were handed was already partly spent. Record it on Secret, for those responses carrying a lease or token TTL a caller does arithmetic on. Responses carrying only configuration are left alone, having no lifetime to reason about. Age is not serialized: it describes the delivery of a response rather than the secret that response carries, and cached responses are themselves serialized. LifetimeWatcher does not read the field yet, so both failing tests stay red. TestCache_Caching_LeaseResponse compared a proxied and a cached response for equality. A cache hit is now distinguishable from a fresh response by its age, so that comparison accounts for the field. Age is reported in whole seconds and rounds to zero on a fast enough hit, so it cannot be asserted on directly.
Anchor the clock at issuance rather than at arrival, so the existing duration arithmetic, including the error backoff window derived from it, accounts for the age of the response. remainingLease clamps at zero because a response can outlive the lease it describes once a cache has held it long enough, and a negative remaining duration would corrupt that backoff window. Fixes both failing tests: the watcher now signals a re-read once an aged lease is spent, and stops instead of renewing one that has expired.
GitHub Actions
Waiting
|
Deployment failed with the following error: Learn More: https://vercel.com/docs/concepts/projects/project-configuration |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
dosubot
Bot
added
size:M
VioletHynes
left a comment
VioletHynes
left a comment
There was a problem hiding this comment.
Hey Pete! Thanks for submitting this. I have a small comment but overall I like this PR and I'm really glad we get to fix this longstanding issue.
I'm still figuring out how outside contributions work in this new world so standby on that, but I'm going to try and get someone to own driving this to completion.
Appreciate the contribution!
| // | ||
| // It is never serialized, because it describes the delivery of a response | ||
| // rather than the secret that response carries. | ||
| Age time.Duration `json:"-"` |
There was a problem hiding this comment.
| Age time.Duration `json:"-"` | |
| Age time.Duration `json:"age,omitempty"` |
There was a problem hiding this comment.
Customers tend to be sensitive to additions here, so I want to be cautious here
There was a problem hiding this comment.
Thanks Violet.
omitempty only omits when zero, so it'd still serialize age on any cached response, which means the field would intermittently appear in the Secret JSON.
I believe json:"-" keeps it from being serialized, and matches other places in the code (like sys_plugins.go), so I do think we need to keep the "-" if we don't want to impact customers by changing the wire format.
There was a problem hiding this comment.
Right, makes sense. Thanks for thinking about this!
2 participants
Description
Vault Agent's cache stamps an
Ageheader on cache hits but replays the stored response body verbatim, so thelease_durationinside it still reads from the moment the lease was issued.ParseSecretonly ever sees that body, andapi.Secrethas no field for HTTP metadata, soAgewas read off the wire and discarded.LifetimeWatchertherefore treats every lease as though it began the instant the response arrived.For most of a lease's life this is harmless, which is what makes it easy to miss. While the lease is far from its
max_ttl, Vault returns the fulllease_durationon every renewal, so the cached body and the truth agree, and the watcher renews on schedule exactly as intended. This can run correctly for a very long time.The failure appears as the lease approaches
max_ttl. Vault then starts returning progressively smaller durations — whatever is left before the cap (CalculateTTL,sdk/framework/lease.go) — and that shrinking value is precisely howLifetimeWatcherknows to give up and tell its caller to re-read the secret. Served from cache, that signal never arrives: the replayed body still reports the original full duration, and the cache updates onlyLastRenewedon renewal, never the stored response. The remaining lifetime the watcher computes therefore never falls, the threshold at which it stops is never reached, and it goes on renewing and sleeping against a lease that has expired, while the template it feeds is never re-rendered. From the outside this looks like Agent running correctly for a long time and then quietly going to sleep.Separately, once a response has been held long enough that its age exceeds roughly a third of the reported duration, the watcher's ~2/3 sleep also overshoots the true expiry, so the renewals it does make are requests Vault and its secrets backend must serve for a lease that no longer exists.
The fix is in two parts:
Secretgains anAgefield, populated for responses that carry a lease or token TTL a caller does arithmetic on. It is additive and never serialized, andLeaseDurationis still reported exactly as the server sent it, so existing callers are unaffected. Responses carrying only configuration are left alone.LifetimeWatcheranchors its clock at issuance rather than at arrival, so the existing duration arithmetic — including the error backoff window derived from it — accounts for the age of the response. Because a cached entry's age grows without bound, the remaining lifetime now falls even when the shrinkinglease_durationnever reaches the client, which restores the give-up threshold.remainingLeaseclamps at zero, because a response can outlive the lease it describes once a cache has held it long enough, and a negative duration would corrupt that backoff window.The first two commits are failing tests, one per failure mode; the last two make them pass. They construct the end state directly — a renewal returned with
Age: 5on a 6s lease — rather than waiting out a realmax_ttl, so both are deterministic, use anhttpteststub caching proxy, and need no Vault server. Reproducing this has historically been the blocker on the issue.TestCache_Caching_LeaseResponsecompared a proxied and a cached response for equality. A cache hit is now distinguishable from a fresh response by its age, so that comparison accounts for the field.Rationale
Refs #19227 (closed as filed against an unsupported release, not on the merits), #16439 (closed as not reproducible for want of a failing test), and #19684 (open, same failure).
Prior art, all self-withdrawn or abandoned by the author rather than rejected on technical grounds: #16887, #16924, #17921. #17919 is merged and was explicitly framed as groundwork for making the sleep-duration calculation
Age-aware.Note #32009 targets the same symptom from the cache side by not caching renewal responses. That only covers the renewal path; a first
Logical().Read()served from cache still returns an unagedlease_duration, which is the gap #16887 was withdrawn over.PCI review checklist