From 662f0b36708c0d83bba2dc5625345f1421ea2887 Mon Sep 17 00:00:00 2001 From: ashvinctrl Date: Sat, 26 Sep 2026 15:02:24 +0530 Subject: [PATCH] fix(downloader): deduplicate dependency downloads by URL and digest downloadAll skipped a dependency whose archive URL had already been downloaded, keyed on the URL alone. When two repositories list the same archive URL and the first entry has no digest or a different one, the second entry's digest was never checked. Key the skip on the URL and the index digest together. Signed-off-by: ashvinctrl --- pkg/downloader/manager.go | 10 ++++++--- pkg/downloader/manager_test.go | 40 ++++++++++++++++++++++++++++++++++ 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index 13a7baeb2..131b7098f 100644 --- a/pkg/downloader/manager.go +++ b/pkg/downloader/manager.go @@ -275,7 +275,11 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { fmt.Fprintf(m.Out, "Saving %d charts\n", len(deps)) var saveError error - churls := make(map[string]struct{}) + // Downloads are deduplicated by digest as well as URL, so an archive that + // another repository listed under a different digest, or none, is still + // checked against this one. + type download struct{ url, digest string } + churls := make(map[download]struct{}) for _, dep := range deps { // No repository means the chart is in charts directory if dep.Repository == "" { @@ -324,7 +328,7 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { break } - if _, ok := churls[churl]; ok { + if _, ok := churls[download{churl, digest}]; ok { fmt.Fprintf(m.Out, "Already downloaded %s from repo %s\n", dep.Name, dep.Repository) continue } @@ -365,7 +369,7 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { break } - churls[churl] = struct{}{} + churls[download{churl, digest}] = struct{}{} } // TODO: this should probably be refactored to be a []error, so we can capture and provide more information rather than "last error wins". diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index 4681bb217..524b9c46c 100644 --- a/pkg/downloader/manager_test.go +++ b/pkg/downloader/manager_test.go @@ -18,6 +18,8 @@ package downloader import ( "bytes" "io/fs" + "net/http" + "net/http/httptest" "os" "path/filepath" "testing" @@ -699,3 +701,41 @@ func TestDownloadAll_RejectsDependencyNotMatchingIndexDigest(t *testing.T) { }) } } + +func TestDownloadAll_ChecksEachIndexDigestForSharedURL(t *testing.T) { + ensure.HelmHome(t) + contentCache := t.TempDir() + srv, _, _ := tamperedChartServer(t, contentCache) + + // A second repository lists the same archive URL without a digest. When + // its entry is downloaded first, the one that does carry a digest must + // still be checked rather than skipped as already downloaded. + idx, err := repo.LoadIndexFile(filepath.Join(srv.Root(), "index.yaml")) + require.NoError(t, err) + for _, cv := range idx.Entries["signtest"] { + cv.Digest = "" + } + noDigest, err := yaml.Marshal(idx) + require.NoError(t, err) + other := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + _, _ = w.Write(noDigest) + })) + t.Cleanup(other.Close) + + chartPath := t.TempDir() + m := &Manager{ + Out: new(bytes.Buffer), + ChartPath: chartPath, + RepositoryConfig: filepath.Join(t.TempDir(), "repositories.yaml"), + RepositoryCache: srv.Root(), + ContentCache: contentCache, + Getters: getter.All(&cli.EnvSettings{}), + } + deps := []*chart.Dependency{ + {Name: "signtest", Repository: other.URL, Version: "0.1.0"}, + {Name: "signtest", Repository: srv.URL(), Version: "0.1.0", Alias: "signtest-indexed"}, + } + + err = m.downloadAll(deps) + require.ErrorContains(t, err, "does not match the digest recorded for it in the repository index") +}