From 577d2a21f0746b0f1976fb32994d897b9b611350 Mon Sep 17 00:00:00 2001 From: Sebastien Tardif Date: Fri, 23 Oct 2026 03:46:26 -0700 Subject: [PATCH] Remove leaked Helm index cache on error CacheIndex writes chart-index-*.yaml under the controller temp directory and leaves removal to the caller. reconcileArtifact deletes that file, but reconcile returns on the first error, so a failed index load, revision calculation, or status patch never reaches it. Each failed reconcile keeps the file until the pod restarts. Remove the cached index on those three returns. Leave it in place when reconcileSource succeeds, including the unchanged-revision short circuit, because reconcileArtifact still needs the file. Assisted-by: Grok/grok-4.7 Signed-off-by: Sebastien Tardif --- .../controller/helmrepository_controller.go | 10 ++ .../helmrepository_controller_test.go | 131 ++++++++++++++++++ 2 files changed, 141 insertions(+) diff --git a/internal/controller/helmrepository_controller.go b/internal/controller/helmrepository_controller.go index 7266e6e89..5e8d84d30 100644 --- a/internal/controller/helmrepository_controller.go +++ b/internal/controller/helmrepository_controller.go @@ -461,6 +461,13 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc } *chartRepo = *newChartRepo + // reconcileArtifact removes this file. Error returns never reach it. + removeCachedIndex := func() { + if err := chartRepo.Clear(); err != nil { + ctrl.LoggerFrom(ctx).Error(err, "failed to remove temporary cached index file") + } + } + // Early comparison to current Artifact. if curArtifact := obj.GetArtifact(); curArtifact != nil { curRev := digest.Digest(curArtifact.Revision) @@ -477,6 +484,7 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc // Load the cached repository index to ensure it passes validation. if err := chartRepo.LoadFromPath(); err != nil { + removeCachedIndex() e := serror.NewGeneric( fmt.Errorf("failed to load Helm repository from index YAML: %w", err), sourcev1.IndexationFailedReason, @@ -490,6 +498,7 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc // Calculate revision. revision := chartRepo.Digest(intdigest.Canonical) if revision.Validate() != nil { + removeCachedIndex() e := serror.NewGeneric( fmt.Errorf("failed to calculate revision: %w", err), sourcev1.IndexationFailedReason, @@ -505,6 +514,7 @@ func (r *HelmRepositoryReconciler) reconcileSource(ctx context.Context, sp *patc } rreconcile.ProgressiveStatus(true, obj, meta.ProgressingReason, "building artifact: %s", message) if err := sp.Patch(ctx, obj, r.patchOptions...); err != nil { + removeCachedIndex() return sreconcile.ResultEmpty, serror.NewGeneric(err, sourcev1.PatchOperationFailedReason) } diff --git a/internal/controller/helmrepository_controller_test.go b/internal/controller/helmrepository_controller_test.go index c34c49d27..f67db9c92 100644 --- a/internal/controller/helmrepository_controller_test.go +++ b/internal/controller/helmrepository_controller_test.go @@ -38,6 +38,7 @@ import ( ctrl "sigs.k8s.io/controller-runtime" "sigs.k8s.io/controller-runtime/pkg/client" fakeclient "sigs.k8s.io/controller-runtime/pkg/client/fake" + "sigs.k8s.io/controller-runtime/pkg/client/interceptor" "sigs.k8s.io/controller-runtime/pkg/controller/controllerutil" kstatus "github.com/fluxcd/cli-utils/pkg/kstatus/status" @@ -1089,6 +1090,136 @@ func TestHelmRepositoryReconciler_reconcileSource(t *testing.T) { } } +func TestHelmRepositoryReconciler_reconcileSource_cachedIndexFile(t *testing.T) { + newReconciler := func(g *WithT, c client.Client) *HelmRepositoryReconciler { + return &HelmRepositoryReconciler{ + Recorder: events.NewFakeRecorder(32, false), + Client: c, + Storage: testStorage, + Getters: testGetters, + patchOptions: getPatchOptions(helmRepositoryReadyCondition.Owned, "sc"), + } + } + + startServer := func(g *WithT, index []byte) *helmtestserver.HelmServer { + server, err := helmtestserver.NewTempHelmServer() + g.Expect(err).NotTo(HaveOccurred()) + if index == nil { + g.Expect(server.PackageChart("testdata/charts/helmchart")).To(Succeed()) + g.Expect(server.GenerateIndex()).To(Succeed()) + } else { + g.Expect(os.WriteFile(filepath.Join(server.Root(), "index.yaml"), index, 0o600)).To(Succeed()) + } + server.Start() + return server + } + + newObject := func(url string) *sourcev1.HelmRepository { + return &sourcev1.HelmRepository{ + ObjectMeta: metav1.ObjectMeta{ + Name: "index-cache", + Namespace: "default", + Generation: 1, + }, + Spec: sourcev1.HelmRepositorySpec{ + Interval: metav1.Duration{Duration: interval}, + Timeout: &metav1.Duration{Duration: timeout}, + URL: url, + }, + } + } + + chartIndexFiles := func(g *WithT, dir string) []string { + matches, err := filepath.Glob(filepath.Join(dir, "chart-index-*.yaml")) + g.Expect(err).NotTo(HaveOccurred()) + return matches + } + + t.Run("invalid index removes cached file", func(t *testing.T) { + g := NewWithT(t) + dir := t.TempDir() + t.Setenv("TMPDIR", dir) + + // HTTP 200 with a body that is not a Helm index. Download succeeds + // and LoadFromPath fails. + server := startServer(g, []byte("{}\n")) + defer server.Stop() + defer os.RemoveAll(server.Root()) + + c := fakeclient.NewClientBuilder(). + WithScheme(testEnv.GetScheme()). + WithStatusSubresource(&sourcev1.HelmRepository{}). + Build() + obj := newObject(server.URL()) + g.Expect(c.Create(context.TODO(), obj)).To(Succeed()) + + var chartRepo repository.ChartRepository + got, err := newReconciler(g, c).reconcileSource(context.TODO(), patch.NewSerialPatcher(obj, c), obj, &meta.Artifact{}, &chartRepo) + + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("failed to load Helm repository from index YAML")) + g.Expect(got).To(Equal(sreconcile.ResultEmpty)) + g.Expect(conditions.IsTrue(obj, sourcev1.FetchFailedCondition)).To(BeTrue()) + g.Expect(chartIndexFiles(g, dir)).To(BeEmpty()) + }) + + t.Run("status patch failure removes cached file", func(t *testing.T) { + g := NewWithT(t) + dir := t.TempDir() + t.Setenv("TMPDIR", dir) + + server := startServer(g, nil) + defer server.Stop() + defer os.RemoveAll(server.Root()) + + base := fakeclient.NewClientBuilder(). + WithScheme(testEnv.GetScheme()). + WithStatusSubresource(&sourcev1.HelmRepository{}). + Build() + c := interceptor.NewClient(base, interceptor.Funcs{ + SubResourcePatch: func(context.Context, client.Client, string, client.Object, client.Patch, ...client.SubResourcePatchOption) error { + return errors.New("injected patch failure") + }, + }) + obj := newObject(server.URL()) + g.Expect(c.Create(context.TODO(), obj)).To(Succeed()) + + var chartRepo repository.ChartRepository + got, err := newReconciler(g, c).reconcileSource(context.TODO(), patch.NewSerialPatcher(obj, c), obj, &meta.Artifact{}, &chartRepo) + + g.Expect(err).To(HaveOccurred()) + g.Expect(err.Error()).To(ContainSubstring("injected patch failure")) + g.Expect(got).To(Equal(sreconcile.ResultEmpty)) + g.Expect(chartIndexFiles(g, dir)).To(BeEmpty()) + }) + + t.Run("successful fetch keeps cached file", func(t *testing.T) { + g := NewWithT(t) + dir := t.TempDir() + t.Setenv("TMPDIR", dir) + + server := startServer(g, nil) + defer server.Stop() + defer os.RemoveAll(server.Root()) + + c := fakeclient.NewClientBuilder(). + WithScheme(testEnv.GetScheme()). + WithStatusSubresource(&sourcev1.HelmRepository{}). + Build() + obj := newObject(server.URL()) + g.Expect(c.Create(context.TODO(), obj)).To(Succeed()) + + var chartRepo repository.ChartRepository + got, err := newReconciler(g, c).reconcileSource(context.TODO(), patch.NewSerialPatcher(obj, c), obj, &meta.Artifact{}, &chartRepo) + defer os.Remove(chartRepo.Path) + + g.Expect(err).NotTo(HaveOccurred()) + g.Expect(got).To(Equal(sreconcile.ResultSuccess)) + matches := chartIndexFiles(g, dir) + g.Expect(matches).To(Equal([]string{chartRepo.Path})) + }) +} + func TestHelmRepositoryReconciler_reconcileArtifact(t *testing.T) { // The cache is at capacity with the previous revision and an index of // another repository, the new revision can only be added after eviction.