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 <sharmaashvin27@gmail.com>

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".

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")
+}