feat: resync OPA state after container restart. Fixes #296#295
Conversation
|
Sorry for delay, missed it completely. |
eshepelyuk
left a comment
There was a problem hiding this comment.
Please adjust commit message accordingly, currently it's just init
There was a problem hiding this comment.
Pull request overview
Adds an OPA health poller to detect OPA container restarts and proactively re-populate OPA state from kube-mgmt’s caches, preventing silent policy/data loss after OPA restarts.
Changes:
- Introduces
--opa-resync-intervaland a background/healthpoller that triggers resync after OPA recovers. - Adds
GenericSync.ForceResync()to force a fullsyncAll()via the existing error-recovery path. - Adds
configmap.Sync.Resync()to re-push matching ConfigMaps from informer stores into OPA.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
pkg/data/generic.go |
Stores the workqueue on the sync struct and adds a sentinel-driven ForceResync() path to trigger full reload. |
pkg/configmap/configmap.go |
Captures informer stores per namespace and adds Resync() to re-push cached ConfigMaps into OPA. |
cmd/kube-mgmt/main.go |
Adds --opa-resync-interval and a goroutine polling OPA health to trigger configmap + generic sync resync on recovery. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Resync re-pushes all matching ConfigMaps from the local informer cache into | ||
| // OPA. Intended for use when OPA has restarted and lost its in-memory state. | ||
| func (s *Sync) Resync() { | ||
| s.mu.Lock() | ||
| stores := make([]cache.Store, len(s.stores)) | ||
| copy(stores, s.stores) | ||
| s.mu.Unlock() | ||
|
|
||
| logrus.Infof("Resyncing all ConfigMaps to OPA") | ||
| for _, store := range stores { | ||
| for _, obj := range store.List() { | ||
| cm := obj.(*v1.ConfigMap) | ||
| if match, isPolicy := s.matcher(cm); match { | ||
| s.syncAdd(cm, isPolicy) | ||
| } |
There was a problem hiding this comment.
Resync() reuses syncAdd(), which always patches status/retry annotations back to the ConfigMap and logs an Info line per entry. On a full resync this can generate a large number of Kubernetes API patch calls and noisy logs even when nothing changed. Consider adding a resync-specific path that pushes to OPA without writing annotations (or only patches when the desired annotation values differ) and logs at debug-level or aggregates results.
| resp, err := http.Get(healthURL) //nolint:noctx // intentional fire-and-forget health probe | ||
| up := err == nil && resp != nil && resp.StatusCode == http.StatusOK |
There was a problem hiding this comment.
watchOPAHealth uses http.Get(healthURL) without adding the configured OPA bearer token. When kube-mgmt is configured with --opa-auth-token/--opa-auth-token-file and OPA enforces authentication, /health will typically return 401/403, causing the poller to treat OPA as permanently down and never trigger a resync. Build the request manually and set the Authorization header (or reuse the existing opa client) so the health check works in authenticated deployments.
| case <-ticker.C: | ||
| resp, err := http.Get(healthURL) //nolint:noctx // intentional fire-and-forget health probe | ||
| up := err == nil && resp != nil && resp.StatusCode == http.StatusOK | ||
| if resp != nil { | ||
| resp.Body.Close() | ||
| } |
There was a problem hiding this comment.
watchOPAHealth uses http.Get with the default client and no timeout/context, so a stalled TCP connect or hung response can block the poll loop indefinitely (and ctx cancellation won’t interrupt it). Use an http.Client with a reasonable Timeout and create the request with the provided ctx so shutdown and recovery detection are reliable.
| if params.opaResyncInterval > 0 { | ||
| healthURL := strings.TrimSuffix(params.opaURL, "/v1") + "/health" | ||
| go watchOPAHealth(ctx, healthURL, params.opaResyncInterval, cmSync, genericSyncs) |
There was a problem hiding this comment.
healthURL is derived via strings.TrimSuffix(params.opaURL, "/v1") + "/health" which produces malformed URLs when --opa-url has a trailing slash (e.g. .../v1/ -> .../v1//health) and can be incorrect for non-standard base paths. Consider normalizing with TrimRight on "/" first (and/or parsing with net/url and setting Path) before appending /health.
| } | ||
| if !up { | ||
| if !wasDown { | ||
| logrus.Warnf("OPA health check failed (%v), will resync on recovery", healthURL) |
There was a problem hiding this comment.
On health check failure, the log message doesn’t include the underlying error or HTTP status code, which makes diagnosing auth/TLS/connection issues harder. Include err (when non-nil) and resp.StatusCode (when resp != nil) in the warning.
| logrus.Warnf("OPA health check failed (%v), will resync on recovery", healthURL) | |
| details := "" | |
| if err != nil { | |
| details += fmt.Sprintf(", err=%v", err) | |
| } | |
| if resp != nil { | |
| details += fmt.Sprintf(", status=%d", resp.StatusCode) | |
| } | |
| logrus.Warnf("OPA health check failed (%v%s), will resync on recovery", healthURL, details) |
| wasDown := false | ||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return | ||
| case <-ticker.C: | ||
| resp, err := http.Get(healthURL) //nolint:noctx // intentional fire-and-forget health probe | ||
| up := err == nil && resp != nil && resp.StatusCode == http.StatusOK | ||
| if resp != nil { | ||
| resp.Body.Close() | ||
| } | ||
| if !up { | ||
| if !wasDown { | ||
| logrus.Warnf("OPA health check failed (%v), will resync on recovery", healthURL) | ||
| } | ||
| wasDown = true | ||
| } else if wasDown { | ||
| logrus.Infof("OPA recovered, triggering full resync") | ||
| if cmSync != nil { | ||
| cmSync.Resync() | ||
| } | ||
| for _, s := range genericSyncs { | ||
| s.ForceResync() | ||
| } | ||
| wasDown = false |
There was a problem hiding this comment.
The poller only triggers resync on a transition from “down” to “up”. If OPA restarts and becomes healthy between polling ticks (downtime shorter than the interval), wasDown may never be set and kube-mgmt won’t resync. If the goal is to reliably detect restarts, consider also checking a restart indicator (e.g., /v1/status uptime/started timestamp, or a marker policy/data key) rather than relying solely on reachability transitions.
| // A force-resync was requested (e.g. OPA restarted). Reset state so the | ||
| // outer loop will perform a full syncAll on its next iteration. | ||
| if path == resetPath { | ||
| *syncDone = false | ||
| return fmt.Errorf("resync requested for %v", s.ns) | ||
| } |
There was a problem hiding this comment.
ForceResync triggers the outer error-recovery path by returning an error from processNext when it sees resetPath. This will be logged as a sync failure and can apply the rate-limiter backoff, even though the resync request is an expected control-flow event. Consider using a dedicated sentinel error that the loop treats as a normal resync signal (log at info/debug and resync immediately without backoff), or handle resetPath by re-queuing initPath and continuing without surfacing an error.
|
Hello @JerT33 , thanks for the contribution. I'm not Go dev, so I can't properly review the changes - I've asked for adding e2e and fixing commit message, although if Copilot suggestions makes sense you can address them. Also, if possible, would be great to have some more pure go tests added. |
ee9b34b to
5e36df0
Compare
|
Please note, I've implemented a lot of changes affecting local development and e2e testing. Please make sure you rebase your branch and carefully read README. |
|
👀 |
Problem
When the OPA container restarts (OOM or otherwise), it loses all in-memory state. kube-mgmt keeps running but only pushes data reactively. If nothing changes in Kubernetes after OPA restarts, OPA stays empty. ConfigMap-based policies are never recovered at all. This can cause mutation and validation policies to silently stop being enforced
Changes
Adds a background goroutine that polls OPA's
/healthendpoint on a configurableinterval (
--opa-resync-interval, default 15s). When it detects OPA has recoveredafter being down, it triggers a full resync across both sync paths:
GenericSync.ForceResync()— injects a reset signal into the existing work queue,which causes the loop to run a full
syncAll()via its existing error-recovery pathconfigmap.Sync.Resync()— re-pushes all matching ConfigMaps from the localinformer cache back into OPA
Set
--opa-resync-interval=0to disable the poller entirelyCloses #296, #211. Related to #189, #206.
Note: this recovers in-place rather than restarting kube-mgmt. Happy to discuss
if a different approach is preferred