-
Notifications
You must be signed in to change notification settings - Fork 616
Use a controller to sync remote jwks store to ConfigMaps #13011
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Use a controller to sync remote jwks store to ConfigMaps #13011
Conversation
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR moves JWKS persistence from an ad-hoc write path inside the store to dedicated controllers: one watching policy changes to drive JWKS fetches, and one syncing the JWKS cache to ConfigMaps with retries and reconciliation. This aligns persistence with controller patterns, enabling retries, self-healing on external CM changes, and clearer separation of concerns.
- Introduces a Policy controller that emits JwksSource changes to the store.
- Adds a ConfigMap controller that reconciles JWKS state into labeled ConfigMaps and handles deletes.
- Refactors JWKS cache/fetcher/store for concurrency and controller-driven updates; replaces controller-runtime logging with the internal logging package.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/kgateway/jwks/jwks_store.go | Refactors store to consume JwksSource events, track CM name-to-URI mapping, expose SubscribeToUpdates for CM controller, and use internal logging. |
| internal/kgateway/jwks/jwks_fetcher.go | Replaces bulk UpdateJwksSources with AddOrUpdateKeyset/RemoveKeyset; changes cache update semantics and subscriber notifications. |
| internal/kgateway/jwks/jwks_cache.go | Adds locking to cache and a GetJwks helper; changes add semantics to always write. |
| internal/kgateway/jwks/config_map_syncer.go | Simplifies to direct client use; defines label constants/helpers, loader, and serialization helper. |
| internal/kgateway/controller/start.go | Wires up new Policy and ConfigMap controllers; constructs the store with channels. |
| internal/kgateway/agentjwksstore/policy_controller.go | New controller emitting JwksSource changes derived from AgentgatewayPolicy. |
| internal/kgateway/agentjwksstore/cm_controller.go | New controller reconciling JWKS to ConfigMaps and reacting to store updates with retries. |
| internal/kgateway/agentjwksstore/jwks_store_controller.go | Removes old singleton-based JWKS store controller. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review for a chance to win a $100 gift card. Take the survey.
…-controller Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
| go func() { | ||
| for { | ||
| select { | ||
| case u := <-jcm.jwksUpdates: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if this decoupling makes much sense; instead of using a channel, I could add a function to add events directly to the event queue and call it from JwksStore?
…-controller Signed-off-by: Dmitri Dolguikh <[email protected]>
…-controller Signed-off-by: Dmitri Dolguikh <[email protected]>
|
Is there an overall design doc on this feature? No problem if not, just wondering if there's a logical entrypoint for a new reviewer |
| wellknown.AgentgatewayPolicyGVR, | ||
| kclient.Filter{ObjectFilter: j.agw.Client.ObjectFilter()}, | ||
| ), j.agw.KrtOpts.ToOptions("AgentgatewayPolicy")...) | ||
| j.jwks = krt.NewManyCollection(policyCol, func(kctx krt.HandlerContext, p *v1alpha1.AgentgatewayPolicy) []jwks.JwksSource { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if multiple policies or backends reference the same JWKS URI, do we skip re-creating the config map or do we create a configmap per policy?
…-controller Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Signed-off-by: Dmitri Dolguikh <[email protected]>
Description
Use a controller to sync jwks store internal state to ConfigMaps. The reasons for this change are to allow for retries on failed ConfigMap udpates/deletes, and to reset changes to ConfigMaps made outside of our controller loop (e.g. inadvertent ConfigMap removal by a user, etc).
ConfigMap syncer is still being used to initialize jwks store on launch; it's value (at least atm) is limited though -- it allows us to recover from inadvertent changes to ConfigMaps used to store jwks' when the keystore isn't available in the store (e.g. after controller restart, with remote jwks source not responding).
Change Type
/kind cleanup
Changelog
Additional Notes