Skip to content

fix(foreman): reap audit ConfigMaps older than retention (#990)#1027

Merged
Defilan merged 2 commits into
defilantech:mainfrom
joryirving:fix/issue-990-audit-reaper
Jul 9, 2026
Merged

fix(foreman): reap audit ConfigMaps older than retention (#990)#1027
Defilan merged 2 commits into
defilantech:mainfrom
joryirving:fix/issue-990-audit-reaper

Conversation

@joryirving

@joryirving joryirving commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator

What

Adds a periodic reaper to foreman-operator that deletes audit-record ConfigMaps older than a configurable retention window (default 7d). Label-scoped to foreman.llmkube.dev/audit=true so non-audit CMs are untouched.

Why

pkg/foreman/audit/writer.go stamps foreman-audit-<task> ConfigMaps WITHOUT ownerReferences by design — the audit record must outlive the AgenticTask to remain a compliance trail after task GC. Reported in this issue: five foreman-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.goSweep(ctx, client, retention) (pure) and Reaper (a manager.LeaderElectionRunnable that ticks Sweep). Retention 0 disables the reaper cleanly (Start blocks on ctx.Done() and returns nil). Uses metadata.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=0 runner never deletes.
  • cmd/foreman-operator/main.go--audit-retention (default 7d, 0 disables) and --audit-retention-interval (default 1h) flags; mgr.Add registers the reaper unconditionally. NeedLeaderElection makes it a no-op on standby replicas under --leader-elect, so HA cannot double-sweep.
  • charts/foreman/values.yamloperator.audit.{retention,interval} keys.
  • charts/foreman/templates/operator-deployment.yaml — flags plumbed through.
  • charts/foreman/templates/operator-rbac.yaml — added delete to configmaps (the reaper's only new RBAC need).

Checklist

  • Tests added/updated
  • make test passes locally
  • make lint passes locally
  • Commit messages follow conventional commits
  • All commits are signed off (git commit -s) per DCO
  • AI assistance (if any) is disclosed above, per CONTRIBUTING.md
  • Documentation updated (if user-facing change)

@joryirving joryirving requested a review from Defilan as a code owner July 9, 2026 16:20
…#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>
@joryirving joryirving force-pushed the fix/issue-990-audit-reaper branch from a1308df to 6706732 Compare July 9, 2026 16:25

@Defilan Defilan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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's List isn'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

codecov Bot commented Jul 9, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 40.27778% with 43 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
cmd/foreman-operator/main.go 0.00% 25 Missing ⚠️
pkg/foreman/audit/reaper.go 61.70% 11 Missing and 7 partials ⚠️

📢 Thoughts on this report? Let us know!

@Defilan Defilan merged commit 08d06d6 into defilantech:main Jul 9, 2026
24 checks passed
@github-actions github-actions Bot mentioned this pull request Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] foreman audit ConfigMaps accumulate forever — no ownerReferences, no retention

2 participants