From 7b91f7c6c3f2252fb83313932bb643f2cc40a436 Mon Sep 17 00:00:00 2001 From: Lemon-miaow Date: Sun, 27 Sep 2026 12:08:03 +0800 Subject: [PATCH] =?UTF-8?q?fix(reaper):=20=E5=8F=AA=E6=B8=85=E7=90=86?= =?UTF-8?q?=E5=A4=87=E4=BB=BD=E5=BA=93=E7=9A=84=20CronJob=20=E7=94=A8?= =?UTF-8?q?=E5=91=BD=E5=90=8D=E7=A9=BA=E9=97=B4=E9=BB=98=E8=AE=A4=20SA?= =?UTF-8?q?=EF=BC=8Ce2e=20=E5=AE=9E=E8=B7=91=E4=B8=80=E6=AC=A1=20reaper?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- deploy/e2e_check.sh | 14 ++++++++ internal/platform/bundle_test.go | 55 +++++++++++++++++++++++++++++ internal/platform/workloads.go | 29 ++++++++++----- internal/platform/workloads_test.go | 3 ++ 4 files changed, 92 insertions(+), 9 deletions(-) diff --git a/deploy/e2e_check.sh b/deploy/e2e_check.sh index f930163..7720f0b 100644 --- a/deploy/e2e_check.sh +++ b/deploy/e2e_check.sh @@ -119,6 +119,20 @@ if [ "$phase" != release ]; then fi rm -rf "$bundle_dir" fi +# The daily reaper is what deletes expired archives. Run it once from its CronJob: the API +# accepts a pod template naming a ServiceAccount that does not exist, and only the Job's +# pod creation fails, so rendering it proves nothing. A release may carry exactly that bug. +if [ "$phase" != release ]; then + reaper_job="felis-e2e-reaper-${phase}" + "${KUBECTL[@]}" -n minecraft delete job "$reaper_job" --ignore-not-found >/dev/null + if "${KUBECTL[@]}" -n minecraft create job --from=cronjob/felis-reaper "$reaper_job" >/dev/null; then + check "the reaper CronJob runs to completion" \ + "${KUBECTL[@]}" -n minecraft wait --for=condition=complete "job/${reaper_job}" --timeout=180s + "${KUBECTL[@]}" -n minecraft delete job "$reaper_job" --ignore-not-found >/dev/null + else + fail "a Job can be created from the reaper CronJob" + fi +fi # A release may predate a timer; what this commit installs has them all. if [ "$phase" != release ]; then for timer in felis-db-backup.timer felis-watchdog.timer felis-update-check.timer; do diff --git a/internal/platform/bundle_test.go b/internal/platform/bundle_test.go index e89c861..b01e25d 100644 --- a/internal/platform/bundle_test.go +++ b/internal/platform/bundle_test.go @@ -226,3 +226,58 @@ func TestRenderYAML_Deterministic(t *testing.T) { t.Error("RenderYAML must be deterministic across calls") } } + +// TestObjects_EveryPodRunsAsARenderedServiceAccount keeps every pod template in the +// bundle on a ServiceAccount the bundle renders in the same namespace, or on the +// namespace's default one. A pod naming a missing ServiceAccount is never created: +// the retention-only reaper once named felis-reaper, which renders only with the +// reaping shape, so on every stock install its Jobs timed out without a pod. +func TestObjects_EveryPodRunsAsARenderedServiceAccount(t *testing.T) { + retention := testParams() + retention.BackupPVC, retention.ArchiveLocalPath = "felis-backups", "/backups" + reaping := retention + reaping.WorldsHostPath = "/var/lib/felis/worlds" + for _, c := range []struct { + name string + p Params + pods int + }{ + {"no archive store", testParams(), 4}, + {"retention only", retention, 5}, + {"reaping", reaping, 5}, + } { + objs := Objects(c.p) + sas := map[string]bool{} + for _, o := range objs { + if sa, ok := o.(*corev1.ServiceAccount); ok { + sas[sa.Namespace+"/"+sa.Name] = true + } + } + pods := 0 + for _, o := range objs { + var spec *corev1.PodSpec + switch w := o.(type) { + case *appsv1.Deployment: + spec = &w.Spec.Template.Spec + case *appsv1.StatefulSet: + spec = &w.Spec.Template.Spec + case *appsv1.DaemonSet: + spec = &w.Spec.Template.Spec + case *batchv1.Job: + spec = &w.Spec.Template.Spec + case *batchv1.CronJob: + spec = &w.Spec.JobTemplate.Spec.Template.Spec + default: + continue + } + pods++ + if sa := spec.ServiceAccountName; sa != "" && sa != "default" && !sas[o.GetNamespace()+"/"+sa] { + t.Errorf("%s: %T %s/%s runs as ServiceAccount %q, which the bundle does not render there", + c.name, o, o.GetNamespace(), o.GetName(), sa) + } + } + if pods != c.pods { + t.Errorf("%s: %d pod templates checked, want %d", c.name, pods, c.pods) + } + } +} diff --git a/internal/platform/workloads.go b/internal/platform/workloads.go index a3f686e..2c29cfb 100644 --- a/internal/platform/workloads.go +++ b/internal/platform/workloads.go @@ -825,21 +825,32 @@ func worldsRootPVC(p Params) *corev1.PersistentVolumeClaim { // reaperPodSpec is the reaper Job's pod template. It lives apart from the CronJob // literal only so the optional node pin is one visible branch: with ReaperNode // set the pod carries a kubernetes.io/hostname selector, keeping the reaper on -// the node that actually holds the worlds hostPath on a multi-node cluster. The -// retention-only shape gets no service account token: it never calls the API. +// the node that actually holds the worlds hostPath on a multi-node cluster. +// +// Only the reaping shape runs as SAReaper. The retention-only shape never calls the +// API, so it gets no token and runs as the namespace's default ServiceAccount: +// felis-reaper is rendered only alongside its Role (rbac.go), and a pod naming a +// ServiceAccount that does not exist is never created. Every stock install renders +// this shape, and its Jobs used to hit their deadline without a single pod. +// +// "default" is spelled out. An install upgraded from the broken shape still carries +// the deprecated serviceAccount: felis-reaper the API server copied into the +// template, and an empty serviceAccountName is filled back in from it. func reaperPodSpec(p Params, container corev1.Container, volumes []corev1.Volume) corev1.PodSpec { spec := corev1.PodSpec{ - ServiceAccountName: SAReaper, - PriorityClassName: controlPlanePriorityName, - RestartPolicy: corev1.RestartPolicyNever, - SecurityContext: reaperPodSecurityContext(), - Containers: []corev1.Container{container}, - Volumes: volumes, + PriorityClassName: controlPlanePriorityName, + RestartPolicy: corev1.RestartPolicyNever, + SecurityContext: reaperPodSecurityContext(), + Containers: []corev1.Container{container}, + Volumes: volumes, } if p.ReaperNode != "" { spec.NodeSelector = map[string]string{"kubernetes.io/hostname": p.ReaperNode} } - if p.WorldsHostPath == "" { + if p.WorldsHostPath != "" { + spec.ServiceAccountName = SAReaper + } else { + spec.ServiceAccountName = "default" spec.AutomountServiceAccountToken = boolPtr(false) } return spec diff --git a/internal/platform/workloads_test.go b/internal/platform/workloads_test.go index ad6ed38..704025c 100644 --- a/internal/platform/workloads_test.go +++ b/internal/platform/workloads_test.go @@ -873,6 +873,9 @@ func TestReaperCronJob_RetentionOnlyShape(t *testing.T) { if ps.AutomountServiceAccountToken == nil || *ps.AutomountServiceAccountToken { t.Error("a retention-only run never calls the API and must not mount a service account token") } + if ps.ServiceAccountName != "default" { + t.Errorf("serviceAccountName = %q, want the namespace default spelled out: felis-reaper renders only with the reaping shape, and an empty name is filled back in from an upgraded install's deprecated serviceAccount", ps.ServiceAccountName) + } if c.SecurityContext == nil || c.SecurityContext.Capabilities == nil || len(c.SecurityContext.Capabilities.Add) != 1 || c.SecurityContext.Capabilities.Add[0] != "DAC_OVERRIDE" { t.Error("it reads back and deletes archives other identities wrote, so it keeps DAC_OVERRIDE") }