diff --git a/charts/foreman/templates/operator-deployment.yaml b/charts/foreman/templates/operator-deployment.yaml index fd740306..6a8bcc60 100644 --- a/charts/foreman/templates/operator-deployment.yaml +++ b/charts/foreman/templates/operator-deployment.yaml @@ -59,6 +59,12 @@ spec: {{- if .Values.webhook.enabled }} - --webhook-port={{ .Values.webhook.port }} {{- end }} + # Audit reaper (#990): the writer stamps audit-record CMs + # owner-unbound by design, so they accumulate unless a + # reaper trims them. --audit-retention=0s disables the + # reaper entirely without redeploying. + - --audit-retention={{ .Values.operator.audit.retention }} + - --audit-retention-interval={{ .Values.operator.audit.interval }} ports: - name: health containerPort: {{ .Values.operator.health.port }} diff --git a/charts/foreman/templates/operator-rbac.yaml b/charts/foreman/templates/operator-rbac.yaml index 33783f6c..303ce35e 100644 --- a/charts/foreman/templates/operator-rbac.yaml +++ b/charts/foreman/templates/operator-rbac.yaml @@ -77,8 +77,14 @@ rules: resources: ["events"] verbs: ["create", "patch"] - apiGroups: [""] + # The audit reaper (#990) only needs list + delete, but Kubernetes + # RBAC verbs are per-resource, not per-label-selector, so the full + # CRUD set is granted and the blast radius is enforced in code by + # client.MatchingLabels{AuditLabel: "true"} inside the reaper's + # Sweep (pkg/foreman/audit/reaper.go). get/list/watch are also used + # by the live audit writer and the transcript helpers. resources: ["configmaps"] - verbs: ["get", "list", "watch", "create", "update", "patch"] + verbs: ["get", "list", "watch", "create", "update", "patch", "delete"] --- apiVersion: rbac.authorization.k8s.io/v1 kind: ClusterRoleBinding diff --git a/charts/foreman/values.yaml b/charts/foreman/values.yaml index 553d2833..03f0e4fd 100644 --- a/charts/foreman/values.yaml +++ b/charts/foreman/values.yaml @@ -65,6 +65,31 @@ operator: # debug only when debugging a bad reconcile cycle. logLevel: info + # Audit reaper (#990). The audit writer stamps audit-record + # ConfigMaps WITHOUT ownerReferences by design — the record must + # outlive the AgenticTask so it remains a compliance trail after + # task GC. That design choice leaves the records to accumulate + # unbounded in the task namespace over time, so the operator runs + # a periodic reaper that deletes any audit CM older than + # `audit.retention`. Set retention to a zero-duration (e.g. "0s", + # "0m", "0") to disable the reaper entirely. + # + # With leader election enabled (the default), the reaper only runs + # on the elected leader — so a HA deployment (replicaCount > 1) + # cannot double-sweep, and a standalone (replicaCount=1) deployment + # runs it on the only replica. + audit: + # Maximum age before an audit-record ConfigMap is deleted. + # Default: 7 days. Override shorter for tighter retention + # (e.g. "24h"); override longer for compliance-driven horizons. + # Set to "0s" / "0m" / "0" to disable the reaper. + retention: 168h + # How often the reaper sweeps. Default 1h; only useful to lower + # for tests. The first sweep runs immediately on operator start, + # so a fresh install with a backlog of stale audit CMs does not + # wait for the first tick. + interval: 1h + # Metrics + health endpoints on the operator pod. Match the # llmkube core operator's defaults for symmetry. metrics: diff --git a/cmd/foreman-operator/main.go b/cmd/foreman-operator/main.go index c2e1fc6b..cd982ada 100644 --- a/cmd/foreman-operator/main.go +++ b/cmd/foreman-operator/main.go @@ -25,6 +25,7 @@ import ( "flag" "os" "path/filepath" + "time" // Import all Kubernetes client auth plugins (e.g. Azure, GCP, OIDC) so // exec-entrypoint and run can make use of them. @@ -42,6 +43,7 @@ import ( foremanv1alpha1 "github.com/defilantech/llmkube/api/foreman/v1alpha1" foremancontroller "github.com/defilantech/llmkube/internal/foreman/controller" foremanwebhook "github.com/defilantech/llmkube/internal/foreman/webhook" + "github.com/defilantech/llmkube/pkg/foreman/audit" ) // defaultWebhookCertDir is the controller-runtime default serving-cert @@ -67,6 +69,8 @@ func main() { var enableWebhooks bool var webhookCertDir string var webhookPort int + var auditRetention time.Duration + var auditRetentionInterval time.Duration flag.StringVar(&metricsAddr, "metrics-bind-address", ":8081", "The address the metrics endpoint binds to.") flag.StringVar(&probeAddr, "health-probe-bind-address", ":8082", @@ -92,7 +96,15 @@ func main() { "chart mounts the self-signed serving Secret here.") flag.IntVar(&webhookPort, "webhook-port", 9443, "Port the webhook server listens on.") - + flag.DurationVar(&auditRetention, "audit-retention", 7*24*time.Hour, + "Maximum age before an audit-record ConfigMap is reaped. "+ + "Audit CMs are intentionally owner-unbound so they outlive the "+ + "AgenticTask for compliance; without this reaper they accumulate "+ + "unbounded (#990). Set to 0 to disable the reaper.") + flag.DurationVar(&auditRetentionInterval, "audit-retention-interval", time.Hour, + "How often the audit reaper sweeps. Defaults to 1h. Lower values are "+ + "only useful in tests; raising it past a few hours delays the "+ + "first cleanup pass proportionally.") opts := zap.Options{Development: true} opts.BindFlags(flag.CommandLine) flag.Parse() @@ -187,6 +199,31 @@ func main() { os.Exit(1) } + // Audit reaper (#990): the writer stamps audit ConfigMaps without + // ownerReferences by design (they must outlive the AgenticTask for + // compliance) — see pkg/foreman/audit/writer.go. Without a periodic + // reaper they accumulate unbounded, so we register one here. The + // reaper's NeedLeaderElection makes it a no-op on standby replicas + // when --leader-elect is enabled; with retention=0 it just waits + // on ctx.Done() and does nothing, so the registration is always + // safe. + if err := mgr.Add(&audit.Reaper{ + Client: mgr.GetClient(), + Retention: auditRetention, + Interval: auditRetentionInterval, + Log: ctrl.Log.WithName("audit-reaper"), + }); err != nil { + setupLog.Error(err, "unable to register audit reaper") + os.Exit(1) + } + if auditRetention > 0 { + setupLog.Info("audit reaper enabled", + "retention", auditRetention.String(), + "interval", auditRetentionInterval.String()) + } else { + setupLog.Info("audit reaper disabled (retention=0)") + } + setupLog.Info("starting foreman-operator") if err := mgr.Start(ctrl.SetupSignalHandler()); err != nil { setupLog.Error(err, "problem running manager") diff --git a/pkg/foreman/audit/reaper.go b/pkg/foreman/audit/reaper.go new file mode 100644 index 00000000..24ff234a --- /dev/null +++ b/pkg/foreman/audit/reaper.go @@ -0,0 +1,154 @@ +/* +Copyright 2025. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package audit + +import ( + "context" + "fmt" + "time" + + "github.com/go-logr/logr" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "sigs.k8s.io/controller-runtime/pkg/client" +) + +// defaultReaperInterval is the sweep cadence used when Reaper.Interval +// is left at its zero value. Once an hour is plenty for a record-count +// bounded by CreationTimestamp — the reaper runs through the cache and +// is purely cosmetic in the hot path. +const defaultReaperInterval = time.Hour + +// Sweep deletes audit ConfigMaps whose CreationTimestamp is older than +// retention. Returns the count deleted. retention <= 0 is a no-op +// (returns 0, nil) so operators can disable the reaper via +// --audit-retention=0 without redeploying with the feature stripped. +// +// Audit CMs are intentionally owner-unbound — see writer.go. The +// record must outlive the AgenticTask to remain a compliance trail +// after task GC. That design choice leaves them to accumulate +// unbounded in the task namespace over time without this sweep; see +// defilantech/LLMKube#990. +func Sweep(ctx context.Context, c client.Client, retention time.Duration) (int, error) { + if retention <= 0 { + return 0, nil + } + var list corev1.ConfigMapList + // Cluster-wide: audit CMs land in the task's own namespace (or in + // AuditNamespace, if the operator was configured with one), which + // the reaper has no upfront knowledge of. The audit-label selector + // keeps the blast radius to CMs the reaper actually owns. + // + // The List is intentionally NOT paginated: the label filter scopes + // to audit CMs only and the reaper itself self-bounds the count + // by deleting the aged-out ones each tick, so the un-paginated + // result stays small in practice. If audit volume ever grows past + // the apiserver's --max-objects-per-list (default 500) this single + // call would start to truncate; if that ever happens, switch to + // client.List with a Continue loop and a per-page list limit. + if err := c.List(ctx, &list, client.MatchingLabels{AuditLabel: "true"}); err != nil { + return 0, fmt.Errorf("audit reaper: list: %w", err) + } + cutoff := time.Now().Add(-retention) + deleted := 0 + for i := range list.Items { + cm := &list.Items[i] + if cm.CreationTimestamp.Time.After(cutoff) { + continue + } + if err := c.Delete(ctx, cm); err != nil { + if apierrors.IsNotFound(err) { + // Race with another replica or a manual delete; benign. + continue + } + return deleted, fmt.Errorf("audit reaper: delete %s/%s: %w", + cm.Namespace, cm.Name, err) + } + deleted++ + } + return deleted, nil +} + +// Reaper runs Sweep on a periodic ticker. Wire via manager.Add so it +// obeys leader election when the operator is deployed with replicas>1: +// +// mgr.Add(&audit.Reaper{ +// Client: mgr.GetClient(), +// Retention: 7 * 24 * time.Hour, // 7 days; 0 disables +// Interval: time.Hour, // optional; defaults to 1h +// }) +// +// With --leader-elect enabled, controller-runtime only starts the +// reaper on the elected leader, so a HA deployment cannot double-sweep. +// Retention 0 disables the reaper entirely (Start blocks on ctx.Done() +// and returns nil without running Sweep). +type Reaper struct { + Client client.Client + Retention time.Duration + Interval time.Duration + Log logr.Logger +} + +// NeedLeaderElection makes the reaper a leader-election-aware runnable +// (manager.LeaderElectionRunnable): when the operator is deployed with +// --leader-elect=true the reaper only runs on the elected replica. A +// standalone (single-replica) deployment has no leader; the reaper +// runs there regardless. Either way, this matches the intent: only +// one reconciler per cluster should be pruning audit records. +func (r *Reaper) NeedLeaderElection() bool { return true } + +// Start runs the periodic sweep until ctx is cancelled, then returns +// nil. A failed sweep is logged and the loop continues (one transient +// apiserver blip does not stop the reaper permanently). +func (r *Reaper) Start(ctx context.Context) error { + if r.Retention <= 0 { + <-ctx.Done() + return nil + } + interval := r.Interval + if interval <= 0 { + interval = defaultReaperInterval + } + log := r.Log + if log.IsZero() { + log = logr.Discard() + } + // Tick immediately on start so a fresh install with a backlog of + // stale audit CMs does not wait an hour for the first cleanup + // pass. + if deleted, err := Sweep(ctx, r.Client, r.Retention); err != nil { + log.Error(err, "audit reaper sweep failed (initial)") + } else if deleted > 0 { + log.Info("audit reaper swept old records (initial)", + "deleted", deleted, "retention", r.Retention.String()) + } + ticker := time.NewTicker(interval) + defer ticker.Stop() + for { + select { + case <-ctx.Done(): + return nil + case <-ticker.C: + if deleted, err := Sweep(ctx, r.Client, r.Retention); err != nil { + log.Error(err, "audit reaper sweep failed") + } else if deleted > 0 { + log.Info("audit reaper swept old records", + "deleted", deleted, "retention", r.Retention.String()) + } + } + } +} diff --git a/pkg/foreman/audit/reaper_test.go b/pkg/foreman/audit/reaper_test.go new file mode 100644 index 00000000..3feb570f --- /dev/null +++ b/pkg/foreman/audit/reaper_test.go @@ -0,0 +1,206 @@ +/* +Copyright 2025. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +*/ + +package audit + +import ( + "context" + "testing" + "time" + + "github.com/go-logr/logr" + corev1 "k8s.io/api/core/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/types" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/client/fake" +) + +// makeAuditCM builds a synthetic audit ConfigMap with the labels the +// real writer stamps onto it, and a caller-controlled CreationTimestamp. +func makeAuditCM(namespace, name string, age time.Duration) *corev1.ConfigMap { + return &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + CreationTimestamp: metav1.NewTime(time.Now().Add(-age)), + Labels: map[string]string{ + AuditLabel: "true", + AuditTaskLabel: name, + }, + }, + Data: map[string]string{auditDataKey: "{}"}, + } +} + +// TestSweepPrunesOldAuditConfigMaps pins the core reaper behavior: audit +// ConfigMaps older than `retention` are deleted, recent ones are kept, +// and ConfigMaps that do NOT carry the audit label are left untouched +// regardless of age. Without this reaper, audit CMs accumulate forever +// in the task namespace because the writer is deliberately owner-ref +// free (see writer.go) — they must outlive the AgenticTask for audit +// purposes. See defilantech/LLMKube#990. +func TestSweepPrunesOldAuditConfigMaps(t *testing.T) { + oldAuditNS := makeAuditCM("ns-old", "old", 10*24*time.Hour) // 10d old + oldAuditDefault := makeAuditCM("default", "default-old", 30*24*time.Hour) // 30d old + recentAudit := makeAuditCM("default", "recent", 1*time.Hour) // 1h old + oldNonAudit := &corev1.ConfigMap{ + ObjectMeta: metav1.ObjectMeta{ + Name: "important-config", + Namespace: "default", + CreationTimestamp: metav1.NewTime(time.Now().Add(-30 * 24 * time.Hour)), + }, + Data: map[string]string{"k": "v"}, + } + + c := fake.NewClientBuilder().WithObjects(oldAuditNS, oldAuditDefault, recentAudit, oldNonAudit).Build() + + deleted, err := Sweep(context.Background(), c, 7*24*time.Hour) + if err != nil { + t.Fatalf("Sweep: %v", err) + } + if deleted != 2 { + t.Errorf("expected 2 deletions (old audit CMs), got %d", deleted) + } + + // Old audit CMs must be gone. + for _, gone := range []types.NamespacedName{ + {Namespace: "ns-old", Name: "old"}, + {Namespace: "default", Name: "default-old"}, + } { + var cm corev1.ConfigMap + err := c.Get(context.Background(), gone, &cm) + if !apierrors.IsNotFound(err) { + t.Errorf("expected %v to be deleted, got err=%v", gone, err) + } + } + // Recent audit CM must survive (within window). + var cm corev1.ConfigMap + if err := c.Get(context.Background(), types.NamespacedName{Namespace: "default", Name: "recent"}, &cm); err != nil { + t.Errorf("recent audit CM should be preserved: %v", err) + } + // Non-audit CM must NOT be touched (label filter is the contract). + if err := c.Get(context.Background(), + types.NamespacedName{Namespace: "default", Name: "important-config"}, &cm); err != nil { + t.Errorf("non-audit CM was incorrectly modified/deleted: %v", err) + } +} + +// TestSweepRetentionZeroIsNoop pins the disable switch: a retention of 0 +// (or negative) means "feature off" and must be a no-op that returns 0 +// deletions with no error. Operators can flip --audit-retention=0 to +// disable the reaper without removing the binary. +func TestSweepRetentionZeroIsNoop(t *testing.T) { + oldAudit := makeAuditCM("default", "old", 365*24*time.Hour) // 1 year old + c := fake.NewClientBuilder().WithObjects(oldAudit).Build() + + deleted, err := Sweep(context.Background(), c, 0) + if err != nil { + t.Fatalf("Sweep with retention=0: %v", err) + } + if deleted != 0 { + t.Errorf("retention=0 must delete nothing, got %d", deleted) + } + var cm corev1.ConfigMap + if err := c.Get(context.Background(), types.NamespacedName{Namespace: "default", Name: "old"}, &cm); err != nil { + t.Errorf("retention=0 must not delete; got %v", err) + } + + // Negative retention is also disabled (defensive). + deleted, err = Sweep(context.Background(), c, -time.Hour) + if err != nil { + t.Fatalf("Sweep with negative retention: %v", err) + } + if deleted != 0 { + t.Errorf("negative retention must delete nothing, got %d", deleted) + } +} + +// TestSweepHandlesEmptyCluster pins the no-input case: no audit CMs is +// a clean pass with 0 deletions and no error (guards against division- +// by-zero / nil-list edge cases). +func TestSweepHandlesEmptyCluster(t *testing.T) { + c := fake.NewClientBuilder().Build() + deleted, err := Sweep(context.Background(), c, 24*time.Hour) + if err != nil { + t.Fatalf("Sweep on empty cluster: %v", err) + } + if deleted != 0 { + t.Errorf("expected 0 deletions on empty cluster, got %d", deleted) + } +} + +// TestReaperRunTicksAndStops pins the periodic-runner behavior: Start +// runs Sweep on the configured interval, returns when ctx is cancelled, +// and never errors on a clean shutdown. We use a short interval and +// inject a fresh-old audit CM to assert at least one tick observed and +// deleted it. +func TestReaperRunTicksAndStops(t *testing.T) { + old := makeAuditCM("default", "stale", 2*time.Hour) + c := fake.NewClientBuilder().WithObjects(old).Build() + + r := &Reaper{ + Client: c, + Retention: time.Hour, + Interval: 10 * time.Millisecond, // tight loop for the test + Log: logr.Discard(), + } + + ctx, cancel := context.WithTimeout(context.Background(), 80*time.Millisecond) + defer cancel() + + done := make(chan error, 1) + go func() { done <- r.Start(ctx) }() + + select { + case err := <-done: + if err != nil { + t.Fatalf("Reaper.Start: %v", err) + } + case <-time.After(2 * time.Second): + t.Fatal("Reaper did not stop within 2s after ctx cancel") + } + + // The stale audit CM should be gone after at least one tick. + var cm corev1.ConfigMap + err := c.Get(context.Background(), types.NamespacedName{Namespace: "default", Name: "stale"}, &cm) + if !apierrors.IsNotFound(err) { + t.Errorf("stale audit CM should have been reaped; got err=%v", err) + } +} + +// TestReaperRunDisabledRetentionNeverSweeps pins that with Retention=0 +// the periodic runner is a no-op: no deletions happen on its tick, and +// cancelling the context still returns cleanly. This is the contract +// for --audit-retention=0 disabling the feature. +func TestReaperRunDisabledRetentionNeverSweeps(t *testing.T) { + old := makeAuditCM("default", "stale", 365*24*time.Hour) + c := fake.NewClientBuilder().WithObjects(old).Build() + + r := &Reaper{ + Client: c, + Retention: 0, + Interval: 10 * time.Millisecond, + Log: logr.Discard(), + } + + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + + if err := r.Start(ctx); err != nil { + t.Fatalf("Reaper.Start with Retention=0: %v", err) + } + + var cm corev1.ConfigMap + if err := c.Get(context.Background(), types.NamespacedName{Namespace: "default", Name: "stale"}, &cm); err != nil { + t.Errorf("disabled reaper must not delete; got %v", err) + } +} + +// silence unused-import linter when the test list above shrinks. +var _ = client.IgnoreNotFound