[ LFX 2026 ] operator: watch SecurityException/ClusterSecurityException CRDs and rescan on change or expiry#392
Conversation
…escan on change or expiry Signed-off-by: yugal07 <yashsadhwani544@gmail.com>
yugal07
changed the title
yugal07
changed the title
📝 WalkthroughWalkthroughAdds a capability-gated watcher for ChangesSecurity exception monitoring
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant KubernetesAPI
participant SecurityExceptionWatchHandler
participant RescanDispatcher
KubernetesAPI->>SecurityExceptionWatchHandler: Emit exception resource event
SecurityExceptionWatchHandler->>SecurityExceptionWatchHandler: Debounce and coalesce request
SecurityExceptionWatchHandler->>RescanDispatcher: Dispatch cluster posture rescan
Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@watcher/securityexceptionwatcher.go`:
- Around line 169-171: Separate the combined Bookmark/Error condition in the
watcher event loop: continue silently for watch.Bookmark, but for watch.Error
log the event’s diagnostic details before continuing. Update the logic around
the event-processing function to preserve existing skip behavior while making
watch errors observable.
- Around line 270-307: Update sweepExpired to avoid holding wh.mu during
Kubernetes List calls and expiry computation: collect and process all expired
keys outside the lock, then briefly lock only when checking and updating
wh.expiredSeen (and removing stale marks), preserving rescan behavior. Ensure
forgetExpired can acquire wh.mu while API calls are in progress.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 839cf4d0-59e3-41de-9637-634e8c297bfc
📒 Files selected for processing (3)
mainhandler/handlerequests.gowatcher/securityexceptionwatcher.gowatcher/securityexceptionwatcher_test.go
… calls Signed-off-by: yugal07 <yashsadhwani544@gmail.com>
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Thanks for this — the design writeup, the coalescing/throttle model, the -race tests and the E2E against a live kind cluster are all excellent, and closing the GitOps loop on the runtime side is genuinely useful. I don't see a hard merge blocker; the change is safe by construction (idempotent full-cluster rescan, no status writes, no new RBAC/transport). A few things I'd like addressed or at least explicitly acknowledged before merge, one of them more important than the rest.
1. Watch resilience — list-once + bare re-watch can silently miss changes (most important)
watchKind calls listExisting exactly once, then loops calling dynamicClient.Resource(gvr)...Watch(ctx, metav1.ListOptions{}) with no ResourceVersion and no relist on reconnect (watcher/securityexceptionwatcher.go, watchKind/consumeWatch).
A watch channel closing mid-run is normal (apiserver rotates watches every few minutes). Because you neither carry a resourceVersion across the reconnect nor relist, any create/update/delete that lands in the gap between the old watch closing and the new Watch() being established is lost — nothing else recovers it (the expiry sweep only reacts to expiresAt, not to general changes). The failure mode is subtle: an exception applied at just the wrong moment never triggers a rescan, and the GitOps loop this PR is meant to close quietly doesn't fire.
Rather than hand-rolling list+watch, please use client-go machinery that handles this: a dynamic informer (dynamicinformer.NewDynamicSharedInformerFactory) or cache.NewListWatchFromClient + watchtools.NewRetryWatcher, which track resourceVersion and relist on "resource version too old". That also removes the manual watch.Bookmark/watch.Error/retry handling. This is the one item I'd consider close-to-blocking, because the whole point of the feature is not missing changes.
2. Rescan is dropped on dispatch error
In rescanLoop, if dispatchRescan returns an error (e.g. the shared eventWorkerPool is overloaded and Invoke returns ErrPoolOverload) the error is logged and the pending rescan is discarded — the change isn't reflected until the next unrelated event or the scheduled scan. Consider re-arming the signal on failure (wh.requestRescan() in the error branch) so a transient pool-overload retries after the throttle rather than being silently lost.
3. Please confirm the shipped RBAC before merge
The description says no new RBAC is needed because the operator already has get/list/watch on these CRDs from helm-charts#875. Worth double-checking the deployed ClusterRole actually covers both securityexceptions and clustersecurityexceptions at kubescape.io — if it doesn't, watchKind logs an error and retries every 5s forever with zero rescans and no other signal, i.e. a silent no-op. A cheap way to make that non-silent would be to surface a clearer/rate-limited error after N consecutive watch failures.
Minor / nits (non-blocking)
- Shutdown leak:
eventQueue(CooldownQueue) is neverStop()ed, and its eviction callback sends on an unbuffered channel whilehandleEventshas already returned onctx.Done(), so the cache goroutine can block permanently. Harmless for a process-lifetime component, but adefer wh.eventQueue.Stop()inSecurityExceptionWatchwould be tidy. - List consistency:
listExistingpages viapager.New, butsweepExpireduses a plain unpaginatedList. Fine at expected cardinalities; worth matching for consistency if the exception count can grow. - The
v1beta1constant and the full-cluster-vs-targeted-rescan tradeoff are already called out in the description and I agree with both choices as-is — good follow-up material, not merge concerns.
Nice work overall; addressing #1 (and ideally #2) would make this solidly mergeable.
…scan on dispatch error, gate on riskAcceptance RBAC Signed-off-by: yugal07 <yashsadhwani544@gmail.com>
|
Thanks, Matthias, all three addressed. Replaced the hand-rolled list-once + bare re-watch with a dynamic informer per kind, so resourceVersion tracking and relist-on-reconnect are handled by client-go and no change is missed across watch rotations (this also removed the manual Bookmark/Error handling). |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
watcher/securityexceptionwatcher.go (1)
345-364: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider
defer wh.mu.Unlock()for panic-safety.The lock is released with an explicit
wh.mu.Unlock()rather thandefer. Currently the loop body only usescontinue, so this is safe today, butwh.mualso gatesresetWatchFailures, which runs on every single informer event. If a future change (or an unexpected panic fromlogger.L()/map access) panics inside this block, the lock would never release, permanently deadlocking all subsequent event processing.🔒 Proposed fix
newlyExpired := false wh.mu.Lock() + defer wh.mu.Unlock() for _, e := range entries { if !e.expired { delete(wh.expiredSeen, e.key) continue } if _, seen := wh.expiredSeen[e.key]; seen { continue } wh.expiredSeen[e.key] = struct{}{} newlyExpired = true logger.L().Info("security exception expired, requesting rescan", helpers.String("kind", e.kind), helpers.String("name", e.name), helpers.String("namespace", e.namespace)) } - wh.mu.Unlock() if newlyExpired { wh.requestRescan() }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@watcher/securityexceptionwatcher.go` around lines 345 - 364, Use defer wh.mu.Unlock() immediately after acquiring wh.mu.Lock() in the expiration-processing block, and remove the explicit unlock at the end. Preserve the existing loop, expiredSeen updates, and logging behavior while ensuring the lock is released during panics.watcher/securityexceptionwatcher_test.go (1)
197-219: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winFixed 50ms sleep before create is a timing-dependent synchronization point, not just a delay.
The informer factory is built with resync period
0(no periodic relist), so if theCreateraces ahead of the reflector'sWatch()call actually being established, the fake tracker's watch may never surface the event at all (there's no periodic resync to catch it later) — the test would then rely purely on the 3s timeout to fail, rather than flake intermittently under CI load where 50ms may not be enough for the goroutine scheduler to get the informer's watch running.Since informer readiness isn't otherwise observable from the test today, consider exposing a sync signal (e.g., returning/using
cache.WaitForCacheSyncfromstartInformers) rather than a fixed sleep, to make this deterministic.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@watcher/securityexceptionwatcher_test.go` around lines 197 - 219, The live-create test uses a fixed 50ms sleep instead of synchronizing with informer readiness. Update the informer startup flow around SecurityExceptionWatch and startInformers to expose or await cache synchronization via cache.WaitForCacheSync, then create the exception only after synchronization succeeds; remove the timing-based sleep while preserving the existing dispatch assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@watcher/securityexceptionwatcher_test.go`:
- Around line 197-219: The live-create test uses a fixed 50ms sleep instead of
synchronizing with informer readiness. Update the informer startup flow around
SecurityExceptionWatch and startInformers to expose or await cache
synchronization via cache.WaitForCacheSync, then create the exception only after
synchronization succeeds; remove the timing-based sleep while preserving the
existing dispatch assertion.
In `@watcher/securityexceptionwatcher.go`:
- Around line 345-364: Use defer wh.mu.Unlock() immediately after acquiring
wh.mu.Lock() in the expiration-processing block, and remove the explicit unlock
at the end. Preserve the existing loop, expiredSeen updates, and logging
behavior while ensuring the lock is released during panics.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d40d84ce-d993-4080-a34e-1483a5b6e0c8
📒 Files selected for processing (5)
config/config.goconfig/config_test.gomainhandler/handlerequests.gowatcher/securityexceptionwatcher.gowatcher/securityexceptionwatcher_test.go
matthyx
left a comment
matthyx
left a comment
There was a problem hiding this comment.
Re-reviewed after fdca18d — all three points from my previous review are properly addressed, and cleanly. Thanks for the quick turnaround.
- Watch resilience ✅ — replaced the hand-rolled list-once + bare re-watch with a
dynamicinformer.NewFilteredDynamicSharedInformerFactoryper kind. The shared reflector tracksresourceVersionand relists on watch errors, so changes across watch rotations are no longer leaked, and the initial cache sync replays pre-existing exceptions as Adds. Nice touch handlingDeletedFinalStateUnknowntombstones inenqueueInformerEvent. - Dropped rescan on dispatch error ✅ —
rescanLoopnow re-arms viarequestRescan()on dispatch failure, with the throttle sleep preventing a tight retry loop. - RBAC ✅ — gated on
RiskAcceptanceEnabled()(matching the Helm chart's conditional RBAC grant) andKubescape.Enabled, plus aWatchErrorHandlerthat escalates to an explicit "verify the ClusterRole grants get/list/watch…" log after 3 consecutive failures — so a misconfigured Role is no longer a silent no-op. The config accessor has its own unit test too.
Both minor nits are handled as well: defer wh.eventQueue.Stop() in SecurityExceptionWatch, and sweepExpired now paginates via pager.New.
One note on the two open CodeRabbit comments: the "sweepExpired holds wh.mu across Kube API list calls" one is already resolved in this revision — the paginated List builds entries with no lock held, and wh.mu is only taken around the in-memory map update afterward. The watch.Error logging one is now moot since the informer's WatchErrorHandler covers that path.
CI is green (build, vet, cross-platform, basic tests, CodeQL all pass) and the new tests cover the live-create informer path, the re-arm, and the RBAC accessor. LGTM — no remaining blockers from my side.
2 participants
Related issue: kubescape/kubescape#1982 — Implement SecurityException CRD: kubescape posture path, operator watcher, and Headlamp plugin
Design: kubevuln/docs/security-exception-design.md — §Architecture → operator — Watch & Rescan, §Observability → Expiry
Completes on the scanner side:
CRDExceptionsGetterposture path (merged) — reads the same CRDs at scan timeOverview
This PR adds the operator half of the SecurityException design (issue kubescape/kubescape/1982
checklist item "Operator
SecurityExceptionWatchHandlerwatching both kinds witha debounced
CooldownQueue, dispatching rescans on changes").The posture path already reads
SecurityException/ClusterSecurityExceptionCRDs at scan time (
CRDExceptionsGetter). But nothing triggers a scan when anexception changes: a team commits a new exception via GitOps, and the excepted
findings keep showing as failing until the next scheduled scan hours later — or
an exception passes its
expiresAtand the now-unsuppressed findings stay hiddenuntil the next schedule. The GitOps loop the design promises ("exceptions live
alongside app code and take effect when applied") was open on the runtime side.
This adds
SecurityExceptionWatchHandler: it watches both kinds, debounces burstsof changes, and dispatches a cluster posture rescan so results reflect the current
set of exceptions promptly — plus a periodic expiry sweep so lapsed exceptions
resurface their findings without waiting for a schedule.
It is wired into the operator's existing watcher startup, gated on the posture
scanner being enabled:
How it works
objects (so exceptions present before the operator came up are honored) then
follows changes on
securityexceptionsandclustersecurityexceptionsatkubescape.io/v1beta1via the dynamic client, re-establishing a dropped watchwith a bounded retry.
CooldownQueue). Every event goes through the operator'sexisting
CooldownQueue, which collapses repeated events for the same objectduring a cooldown window.
each is a distinct queue key, so distinct events would otherwise each trigger a
full rescan. A buffered-by-one
rescanSignalcollapses concurrent requests into asingle pending rescan, and a post-dispatch throttle absorbs the rest of the burst
— a set of exceptions applied together produces one rescan, not N.
buildRescanCommandemits exactlythe
apis.TypeRunKubescapecommand the operator already fires on startup(cluster
WildWlid, frameworksallcontrols/nsa/mitre), dispatched throughthe existing
utils.AddCommandToChannelworker pool. Re-evaluating the wholecluster is idempotent and re-applies every exception; no new transport or command
type is introduced.
and requests a rescan when an exception has newly passed its
spec.expiresAt(parsed as RFC3339). Consistent with the design, nothing writes status —
expiry is still evaluated by the scanners at scan time; the sweep only nudges a
rescan so previously-suppressed findings resurface. Each expiry triggers exactly
one rescan (tracked in
expiredSeen), and the mark is cleared ifexpiresAtislater pushed back out, so a re-expiry is caught again.
What changed
watcher/securityexceptionwatcher.go(new) —SecurityExceptionWatchHandler:the two watches (
watchKind/consumeWatch/listExisting), the debouncedevent handler (
handleEvents→requestRescan), the coalescing rescan dispatcher(
rescanLoop), the expiry loop (expiryLoop/sweepExpired), and the purehelpers
buildRescanCommandandparseExpiresAt. Tunables (cooldown, throttle,retry, sweep interval) are captured into the handler at construction so a running
handler is unaffected by later changes and tests are race-free.
mainhandler/handlerequests.go— start the handler inHandleWatchers, gatedon
Components().Kubescape.Enabled(the posture scanner an exception affects). Thedispatcher is wired to the existing
eventWorkerPool;dispatchRescanis aninjectable field so tests exercise the pipeline without a pool.
watcher/securityexceptionwatcher_test.go(new) — unit tests (below).No new RBAC: the operator already has
get/list/watchon the SecurityExceptionCRDs from helm-charts#875, and
dispatches through the same pool it already uses for scans.
Design decisions
"determines affected namespaces/workloads, dispatches rescan commands." Resolving
an exception's
resources/objectSelector/namespaceSelectorto a concreteworkload set duplicates matching logic that already lives in the scanner and
opa-utils. A cluster rescan is simpler, always correct, idempotent, and coalesced
so it isn't wasteful. Targeted rescans are a clean follow-up (see below).
v1beta1. The handler lists/watches atkubescape.io/v1beta1, matching theshipped CRDs and
CRDExceptionsGetter. A single named constant moves when the CRDgraduates to
v1.Kubescape.Enabled. An exception only affects posture results, sothe watcher runs exactly when the posture scanner does — no new config flag.
Testing
go build ./...,go vet ./..., andgofmtare clean.go test -race -count=1 ./watcher/passes for the new suite (9 tests). The onepackage failure,
watcher/TestRegistryCommandWatch, is a pre-existingtestcontainers integration test that times out booting its container in this
environment (confirmed failing identically on a clean
main); it is unrelated tothis change.
securityexceptionwatcher_test.go) cover:buildRescanCommandshape;
parseExpiresAt(present/absent/malformed/date-only); rescan coalescing(5 requests → 1 pending); the expiry sweep (one-shot per expiry, idempotent on
re-sweep, and re-arm when
expiresAtis pushed out);listExistingenqueue;event→rescan dispatch; and the full
SecurityExceptionWatchpath against a fakedynamic client. Two real concurrency issues surfaced by
-racewere fixed duringdevelopment: a shared
expiredSeenmap (now mutex-guarded) and loops readingmutable package globals (now captured into the handler).
End-to-end, against a live kind cluster
Run with the released operator replaced by this build's handler (driven via a small
go runharness that constructsNewSecurityExceptionWatchHandlerwith the realdynamic client and prints each dispatched command):
ClusterSecurityExceptionanddispatched a rescan.
SecurityExceptioneach produced a debounced rescanwith the correct cluster
WildWlid.kubescape scanagainst the same cluster, the dispatched rescanflips an excepted control (e.g.
C-0034onDeployment/nginx) fromfailedtopassed (w/exceptions)and back on delete — closing the GitOps loop end to end.Signed Commits
Summary by CodeRabbit
New Features
Bug Fixes
Tests
expiresAtparsing, event-driven rescan triggering, startup behavior, and idempotent expiration sweeps.