Make LRU cache generic - #7202
Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the agent’s workload X.509 SVID caching to be based on a generic, type-parameterized LRU cache, enabling reuse of the same cache machinery for additional SVID types (e.g., WIT-SVIDs) while keeping existing X.509 workload behavior unchanged.
Changes:
- Introduces a generic
LRUCache[SVID, Update]with typed subscribers and update builders, plus anX509SVIDLRUCachewrapper implementing X.509-specific behaviors (e.g., taint rotation). - Updates manager and endpoint interfaces to use typed workload updates (
X509WorkloadUpdate,X509Identity) and typed subscribers. - Separates tainted authority state from cache entry update payloads and updates sync flow accordingly.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/agent/manager/sync.go | Switches sync/update flow to use the X.509-specific cache interface and separates tainted-authority handling. |
| pkg/agent/manager/storecache/cache.go | Updates store cache to accept X.509 SVID updates as a map keyed by entry ID. |
| pkg/agent/manager/storecache/cache_test.go | Adjusts store cache tests for the updated X.509 SVID update API. |
| pkg/agent/manager/manager.go | Updates manager APIs and fields to use typed subscriber/update types and the new X.509 cache. |
| pkg/agent/manager/manager_test.go | Updates manager tests to use typed X.509 identity/update types and the new cache field. |
| pkg/agent/manager/config.go | Constructs the new X.509 cache via generic LRU config and wires it into the manager. |
| pkg/agent/manager/cache/x509_lru_cache.go | Adds X.509 wrapper cache with X.509 workload update builder and taint rotation logic. |
| pkg/agent/manager/cache/workload.go | Removes legacy non-generic workload cache types in favor of X.509-specific typed ones. |
| pkg/agent/manager/cache/util.go | Moves Selectors and updates identity sorting for X.509-specific identity type. |
| pkg/agent/manager/cache/sets.go | Adjusts subscriber set to store the new base subscriber abstraction. |
| pkg/agent/manager/cache/lru_cache.go | Implements the new generic LRU cache, typed subscribers, and typed update building. |
| pkg/agent/manager/cache/lru_cache_test.go | Updates cache tests to use the X.509 wrapper and typed updates/subscribers. |
| pkg/agent/manager/cache/lru_cache_subscriber.go | Introduces generic subscriber type and base subscriber interface for indexing. |
| pkg/agent/endpoints/workload/handler.go | Updates Workload API handler plumbing to use X509WorkloadUpdate and typed subscribers. |
| pkg/agent/endpoints/workload/handler_test.go | Updates Workload API handler tests for typed identities/updates. |
| pkg/agent/endpoints/sdsv3/handler.go | Updates SDS handler to use X509WorkloadUpdate and typed identities. |
| pkg/agent/endpoints/sdsv3/handler_test.go | Updates SDS handler tests for typed identities/updates and typed subscribers. |
| pkg/agent/common/hintsfilter/hintsfilter.go | Updates hints filtering to operate on X509Identity. |
| pkg/agent/broker/api/service.go | Updates broker API X.509 SVID streaming helpers to use X509WorkloadUpdate. |
| pkg/agent/api/delegatedidentity/v1/service.go | Updates delegated identity streaming helpers to use X509WorkloadUpdate. |
| pkg/agent/api/delegatedidentity/v1/service_test.go | Updates delegated identity service tests for typed identities/updates. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| // UpdateSVIDs updates X509 SVIDs using the UpdateSVIDs struct (backward compat). | ||
| func (c *X509SVIDLRUCache) UpdateX509SVIDs(svids map[string]*X509SVID) { | ||
| c.LRUCache.UpdateSVIDs(svids) |
| var expiresAt time.Time | ||
| if cachedSvid, ok := c.svids[entryID]; ok { | ||
| expiresAt = cachedSvid.Chain[0].NotAfter | ||
| expiresAt = (*cachedSvid).ExpiresAt() | ||
| } |
Signed-off-by: Sorin Dumitru <sorin@returnze.ro>
cdd9262 to
3fc863e
Compare
| return w | ||
| // GetRecordsForSelectors returns the set of records matching the selector set. | ||
| // Exported for use by buildUpdate callbacks. | ||
| func (c *LRUCache[SVID, Update]) GetRecordsForSelectors(set selectorSet) (lruCacheRecordSet, func()) { |
There was a problem hiding this comment.
Since BuildUpdate takes the unexported selectorSet type, a buildUpdate callback can only be implemented inside this package, so I think GetRecordsForSelectors (buildX509WorkloadUpdate calls the unexported version directly), Bundles, TrustDomain, and Log could stay unexported or be dropped until an external consumer exists. Bundles in particular returns the internal map that UpdateEntries mutates in place, so a caller reading it after the read lock is released would race with a sync. Would it make sense to trim these until the WIT-SVID cache actually needs them?
| func (c *LRUCache[SVID, Update]) UpdateSVIDs(svids map[string]*SVID) { | ||
| c.mu.Lock() | ||
| defer func() { agentmetrics.SetSVIDMapSize(c.metrics, c.CountX509SVIDs()) }() | ||
| defer func() { agentmetrics.SetSVIDMapSize(c.metrics, c.CountSVIDs()) }() |
There was a problem hiding this comment.
I wonder if the gauges emitted here and in UpdateEntries (SVIDMapSize and RecordMapSize) should gain a cache-type dimension as part of making the cache reusable. They carry no label today, so once a WIT-SVID instance of the generic cache exists, both instances would write to the same gauge and overwrite each other's values.
|
|
||
| func (c *LRUCache) diffSelectors(existingEntry, newEntry *common.RegistrationEntry, added, removed selectorSet) { | ||
| // Make a set of all the selectors being added | ||
| // Make a set of all the selectors being added |
There was a problem hiding this comment.
nit: it looks like the refactor hoisted this step comment into doc-comment position; it describes only the first block of diffSelectors, and the same happened with the dedup comment above getRecordsForSelectors. Maybe we could move them back inside the function bodies so they don't read as doc comments for the whole function.
| taintedBatchProcessedCh chan struct{} | ||
| } | ||
|
|
||
| func NewX509LRUCache(config LRUCacheConfig[X509SVID, X509WorkloadUpdate]) *X509SVIDLRUCache { |
There was a problem hiding this comment.
nit: since BuildUpdate is unconditionally overwritten here, a caller-supplied value is silently discarded. It could be worth taking a narrower config, or an alias like type X509LRUCacheConfig = LRUCacheConfig[X509SVID, X509WorkloadUpdate], so call sites don't have to spell out the type parameters and can't pass a BuildUpdate that gets ignored.
Pull Request check list
Affected functionality
This should behave the same before and after the change
Description of change
Parameterise the LRU cache on SVID type to make it possible to reuse for WIT-SVIDs