fix(foreman): reap audit ConfigMaps older than retention (#990)#1027
Conversation
…#990) Per-task audit records are written as ConfigMaps labeled foreman.llmkube.dev/audit=true (see writer.go) and intentionally owner-unbound so they outlive AgenticTask GC for compliance. Without retention they accumulate unbounded in the task namespace — five foreman-audit-dispatch-892-* CMs were observed from repeated dispatches of one issue after every AgenticTask had been deleted. Add a periodic reaper registered with the controller manager: pkg/foreman/audit/reaper.go Sweep + Reaper (leader-aware) pkg/foreman/audit/reaper_test.go covers prune/keep/label-filter/disable cmd/foreman-operator/main.go --audit-retention (default 7d, 0 disables) + interval flag; registers the runnable so leader election prevents HA double-sweep charts/foreman/values.yaml operator.audit.{retention,interval} charts/foreman/templates/operator-deployment.yaml flags plumbed charts/foreman/templates/operator-rbac.yaml configmaps/delete Retention-by-age via metadata.creationTimestamp (k8s-immutable, no need to stamp our own timestamp). The label filter keeps the blast radius to CMs the reaper actually owns. NeedLeaderElection makes the reaper a no-op on standby replicas under --leader-elect. Signed-off-by: Jory Irving <jory@jory.dev>
a1308df to
6706732
Compare
Defilan
left a comment
There was a problem hiding this comment.
Approve. This is clean, and it got the parts that usually bite exactly right.
The reaper logic is sound: age cut on the immutable CreationTimestamp against now - retention, label-scoped List so the blast radius is audit CMs only, IsNotFound-on-delete treated as benign for the HA race, and retention <= 0 a genuine no-op in both Sweep and Start so --audit-retention=0s disables cleanly. NeedLeaderElection() = true plus the immediate-first-sweep and log-and-continue loop are the right calls.
Nice catch on the duration handling too: DurationVar won't parse "7d", and you used 7*24*time.Hour for the default and 168h in the values, so the operator won't crash on a bad parse.
The 5 unit tests are real (old deleted / recent kept / non-audit ignored / retention-zero-and-negative no-op / empty cluster / ticker ticks and stops), and the body matches the diff.
Two non-blocking nits, take or leave:
Sweep'sListisn't paginated. Fine given the label filter and self-limiting deletes, but worth a comment if audit volume ever gets large.- The RBAC grant is full CRUD on configmaps while the reaper only needs list+delete. Standard (RBAC can't label-scope), but a one-line note that the code's label filter is the real guard would be nice.
The failing "Package and Install Test" is a kind-cluster setup flake (connection refused to localhost:8080, missing kind-registry container), not this change. I re-ran it.
…wing intent (defilantech#990) Reviewer nits on defilantech#1027: - pkg/foreman/audit/reaper.go: explain why the Sweep List is not paginated (label filter + self-bounded by deletes) and what to do if audit volume ever outgrows --max-objects-per-list (default 500). - charts/foreman/templates/operator-rbac.yaml: note that the audit reaper only needs list + delete but Kubernetes RBAC verbs are per-resource, not per-label-selector, so the full CRUD set is granted and the in-code label filter is the real blast guard. Both are comment-only. No behavior change, no new tests. Signed-off-by: Jory Irving <jory@jory.dev>
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
What
Adds a periodic reaper to
foreman-operatorthat deletes audit-recordConfigMaps older than a configurable retention window (default 7d). Label-scoped toforeman.llmkube.dev/audit=trueso non-audit CMs are untouched.Why
pkg/foreman/audit/writer.gostampsforeman-audit-<task>ConfigMaps WITHOUTownerReferencesby design — the audit record must outlive theAgenticTaskto remain a compliance trail after task GC. Reported in this issue: fiveforeman-audit-dispatch-892-*CMs from repeated dispatches of a single issue, all surviving every AgenticTask deletion and growing unbounded. Without a reaper there is no way to bound the count.AI assistance disclosure: drafted with opencode using MiniMax M3, per
CONTRIBUTING.md.Fixes #990
How
pkg/foreman/audit/reaper.go—Sweep(ctx, client, retention)(pure) andReaper(amanager.LeaderElectionRunnablethat ticksSweep). Retention 0 disables the reaper cleanly (Startblocks onctx.Done()and returns nil). Usesmetadata.creationTimestamp(k8s-immutable) as the age source. The first sweep runs immediately on start so a backlog of stale CMs is cleaned without waiting an hour.pkg/foreman/audit/reaper_test.go— 5 unit tests via fake client: prunes old CMs, keeps recent CMs, ignores non-audit CMs by label filter, retention≤0 is a no-op (negative also), empty cluster is a clean 0, the periodic runner ticks and stops on ctx cancellation,Retention=0runner never deletes.cmd/foreman-operator/main.go—--audit-retention(default7d,0disables) and--audit-retention-interval(default1h) flags;mgr.Addregisters the reaper unconditionally.NeedLeaderElectionmakes it a no-op on standby replicas under--leader-elect, so HA cannot double-sweep.charts/foreman/values.yaml—operator.audit.{retention,interval}keys.charts/foreman/templates/operator-deployment.yaml— flags plumbed through.charts/foreman/templates/operator-rbac.yaml— addeddeletetoconfigmaps(the reaper's only new RBAC need).Checklist
make testpasses locallymake lintpasses locallygit commit -s) per DCO