From 66d3331fc631841c0e50a63353c94c9d0f8341c9 Mon Sep 17 00:00:00 2001 From: MichaelMorris Date: Thu, 6 Mar 2025 15:21:14 +0000 Subject: [PATCH] Set chartURL in dependency instead of passing around a cache Signed-off-by: MichaelMorris --- internal/chart/v3/dependency.go | 2 + internal/resolver/resolver.go | 32 +++++++------ internal/resolver/resolver_test.go | 4 +- pkg/chart/v2/dependency.go | 2 + pkg/downloader/manager.go | 44 ++++++++---------- pkg/downloader/manager_test.go | 74 +++++++++++------------------- 6 files changed, 70 insertions(+), 88 deletions(-) diff --git a/internal/chart/v3/dependency.go b/internal/chart/v3/dependency.go index 2d956b548..68b90131e 100644 --- a/internal/chart/v3/dependency.go +++ b/internal/chart/v3/dependency.go @@ -31,6 +31,8 @@ type Dependency struct { // A lock file will always produce a single version, while a dependency // may contain a semantic version range. Version string `json:"version,omitempty" yaml:"version,omitempty"` + // The URL to the dependency. + ChartURL string `json:"charturl,omitempty" yaml:"charturl,omitempty"` // The URL to the repository. // // Appending `index.yaml` to this string should result in a URL that can be diff --git a/internal/resolver/resolver.go b/internal/resolver/resolver.go index 5d4d75f89..e2a7c929f 100644 --- a/internal/resolver/resolver.go +++ b/internal/resolver/resolver.go @@ -52,25 +52,23 @@ func New(chartpath, cachepath string, registryClient *registry.Client) *Resolver } } -// Resolve resolves dependencies and returns a lock file with the resolution and a map containaing the chart URLs for the dependencies. -// The key to the map is generated by concatenating the repo URL, name and version for the dependency -func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string) (*chart.Lock, map[string]string, error) { +// Resolve resolves dependencies and returns a lock file with the resolution. +func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string) (*chart.Lock, error) { // Now we clone the dependencies, locking as we go. locked := make([]*chart.Dependency, len(reqs)) missing := []string{} chartRepoIndexURLs := make(map[string]*repo.IndexFile) - urls := make(map[string]string) for i, d := range reqs { constraint, err := semver.NewConstraint(d.Version) if err != nil { - return nil, nil, fmt.Errorf("dependency %q has an invalid version/constraint format: %w", d.Name, err) + return nil, fmt.Errorf("dependency %q has an invalid version/constraint format: %w", d.Name, err) } if d.Repository == "" { // Local chart subfolder if _, err := GetLocalPath(filepath.Join("charts", d.Name), r.chartpath); err != nil { - return nil, nil, err + return nil, err } locked[i] = &chart.Dependency{ @@ -83,12 +81,12 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string if strings.HasPrefix(d.Repository, "file://") { chartpath, err := GetLocalPath(d.Repository, r.chartpath) if err != nil { - return nil, nil, err + return nil, err } ch, err := loader.LoadDir(chartpath) if err != nil { - return nil, nil, err + return nil, err } v, err := semver.NewVersion(ch.Metadata.Version) @@ -137,7 +135,7 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string repoIndex, err = repo.LoadIndexFile(repoFilepath) chartRepoIndexURLs[repoFilepath] = repoIndex if err != nil { - return nil, nil, fmt.Errorf("no cached repository for %s found. (try 'helm repo update'): %w", repoName, err) + return nil, fmt.Errorf("no cached repository for %s found. (try 'helm repo update'): %w", repoName, err) } } else { repoIndex = indexFile @@ -145,7 +143,7 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string vs, ok = repoIndex.Entries[d.Name] if !ok { - return nil, nil, fmt.Errorf("%s chart not found in repo %s", d.Name, d.Repository) + return nil, fmt.Errorf("%s chart not found in repo %s", d.Name, d.Repository) } found = false } else { @@ -167,7 +165,7 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string ref := fmt.Sprintf("%s/%s", strings.TrimPrefix(d.Repository, fmt.Sprintf("%s://", registry.OCIScheme)), d.Name) tags, err := r.registryClient.Tags(ref) if err != nil { - return nil, nil, fmt.Errorf("could not retrieve list of tags for repository %s: %w", d.Repository, err) + return nil, fmt.Errorf("could not retrieve list of tags for repository %s: %w", d.Repository, err) } vs = make(repo.ChartVersions, len(tags)) @@ -199,9 +197,13 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string if constraint.Check(v) { found = true if len(ver.URLs) > 0 { - urls[d.Repository+ver.Name+ver.Version] = ver.URLs[0] + locked[i].ChartURL, err = repo.ResolveReferenceURL(d.Repository, ver.URLs[0]) + if err != nil { + return nil, err + } } locked[i].Version = v.Original() + break } } @@ -211,19 +213,19 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string } } if len(missing) > 0 { - return nil, nil, fmt.Errorf("can't get a valid version for %d subchart(s): %s. Make sure a matching chart version exists in the repo, or change the version constraint in Chart.yaml", len(missing), strings.Join(missing, ", ")) + return nil, fmt.Errorf("can't get a valid version for %d subchart(s): %s. Make sure a matching chart version exists in the repo, or change the version constraint in Chart.yaml", len(missing), strings.Join(missing, ", ")) } digest, err := HashReq(reqs, locked) if err != nil { - return nil, nil, err + return nil, err } return &chart.Lock{ Generated: time.Now(), Digest: digest, Dependencies: locked, - }, urls, nil + }, nil } // HashReq generates a hash of the dependencies. diff --git a/internal/resolver/resolver_test.go b/internal/resolver/resolver_test.go index 766443a7c..8e504dbda 100644 --- a/internal/resolver/resolver_test.go +++ b/internal/resolver/resolver_test.go @@ -81,7 +81,7 @@ func TestResolve(t *testing.T) { }, expect: &chart.Lock{ Dependencies: []*chart.Dependency{ - {Name: "alpine", Repository: "http://example.com", Version: "0.2.0"}, + {Name: "alpine", Repository: "http://example.com", Version: "0.2.0", ChartURL: "https://charts.helm.sh/stable/alpine-0.1.0.tgz"}, }, }, }, @@ -144,7 +144,7 @@ func TestResolve(t *testing.T) { r := New("testdata/chartpath", "testdata/repository", registryClient) for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - l, _, err := r.Resolve(tt.req, repoNames) + l, err := r.Resolve(tt.req, repoNames) if err != nil { if tt.err { return diff --git a/pkg/chart/v2/dependency.go b/pkg/chart/v2/dependency.go index 8a590a036..73435599b 100644 --- a/pkg/chart/v2/dependency.go +++ b/pkg/chart/v2/dependency.go @@ -31,6 +31,8 @@ type Dependency struct { // A lock file will always produce a single version, while a dependency // may contain a semantic version range. Version string `json:"version,omitempty" yaml:"version,omitempty"` + // The URL to the dependency. + ChartURL string `json:"charturl,omitempty" yaml:"charturl,omitempty"` // The URL to the repository. // // Appending `index.yaml` to this string should result in a URL that can be diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index 68b6c5f6e..c752acf55 100644 --- a/pkg/downloader/manager.go +++ b/pkg/downloader/manager.go @@ -141,7 +141,7 @@ func (m *Manager) Build() error { } // Now we need to fetch every package here into charts/ - return m.downloadAll(lock.Dependencies, nil) + return m.downloadAll(lock.Dependencies) } // Update updates a local charts directory. @@ -191,13 +191,13 @@ func (m *Manager) Update() error { // Now we need to find out which version of a chart best satisfies the // dependencies in the Chart.yaml - lock, urls, err := m.resolve(req, repoNames) + lock, err := m.resolve(req, repoNames) if err != nil { return err } // Now we need to fetch every package here into charts/ - if err := m.downloadAll(lock.Dependencies, urls); err != nil { + if err := m.downloadAll(lock.Dependencies); err != nil { return err } @@ -230,8 +230,7 @@ func (m *Manager) loadChartDir() (*chart.Chart, error) { // resolve takes a list of dependencies and translates them into an exact version to download. // // This returns a lock file, which has all of the dependencies normalized to a specific version -// and a map containaing the URLs for the dependencies. The key to the map is generated by concatenating the repo URL, name and version for the dependency. -func (m *Manager) resolve(req []*chart.Dependency, repoNames map[string]string) (*chart.Lock, map[string]string, error) { +func (m *Manager) resolve(req []*chart.Dependency, repoNames map[string]string) (*chart.Lock, error) { res := resolver.New(m.ChartPath, m.RepositoryCache, m.RegistryClient) return res.Resolve(req, repoNames) } @@ -240,7 +239,7 @@ func (m *Manager) resolve(req []*chart.Dependency, repoNames map[string]string) // // It will delete versions of the chart that exist on disk and might cause // a conflict. -func (m *Manager) downloadAll(deps []*chart.Dependency, urls map[string]string) error { +func (m *Manager) downloadAll(deps []*chart.Dependency) error { repos, err := m.loadChartRepositories() if err != nil { return err @@ -313,7 +312,7 @@ func (m *Manager) downloadAll(deps []*chart.Dependency, urls map[string]string) // Any failure to resolve/download a chart should fail: // https://github.com/helm/helm/issues/1439 - churl, username, password, insecureskiptlsverify, passcredentialsall, caFile, certFile, keyFile, err := m.findChartURL(dep.Name, dep.Version, dep.Repository, repos, urls) + churl, username, password, insecureskiptlsverify, passcredentialsall, caFile, certFile, keyFile, err := m.findChartURL(dep, repos) if err != nil { saveError = fmt.Errorf("could not find %s: %w", churl, err) break @@ -723,25 +722,21 @@ func (m *Manager) parallelRepoUpdate(repos []*repo.Entry) error { return nil } -// findChartURL searches the cache of repo data for a chart that has the name and the repoURL specified. +// findChartURL searches the cache of repo data for the specified dependency. // -// 'name' is the name of the chart. Version is an exact semver, or an empty string. If empty, the +// The version in the given dependency is an exact semver, or an empty string. If empty, the // newest version will be returned. // -// repoURL is the repository to search -// -// resolvedChartUrls is a map of already resolved chart URLs keyed by a concatenation of the the repo URL, chart name and version. The URL shall be read from this map if present, rather than downloading and reading the index file -// // If it finds a URL that is "relative", it will prepend the repoURL. -func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]*repo.ChartRepository, resolvedChartUrls map[string]string) (url, username, password string, insecureskiptlsverify, passcredentialsall bool, caFile, certFile, keyFile string, err error) { - if registry.IsOCI(repoURL) { - return fmt.Sprintf("%s/%s:%s", repoURL, name, version), "", "", false, false, "", "", "", nil +func (m *Manager) findChartURL(dep *chart.Dependency, repos map[string]*repo.ChartRepository) (url, username, password string, insecureskiptlsverify, passcredentialsall bool, caFile, certFile, keyFile string, err error) { + if registry.IsOCI(dep.Repository) { + return fmt.Sprintf("%s/%s:%s", dep.Repository, dep.Name, dep.Version), "", "", false, false, "", "", "", nil } for _, cr := range repos { - if urlutil.Equal(repoURL, cr.Config.URL) { + if urlutil.Equal(dep.Repository, cr.Config.URL) { var entry repo.ChartVersions - entry, err = findEntryByName(name, cr) + entry, err = findEntryByName(dep.Name, cr) if err != nil { // TODO: Where linting is skipped in this function we should // refactor to remove naked returns while ensuring the same @@ -750,12 +745,12 @@ func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]* return } var ve *repo.ChartVersion - ve, err = findVersionedEntry(version, entry) + ve, err = findVersionedEntry(dep.Version, entry) if err != nil { //nolint:nakedret return } - url, err = repo.ResolveReferenceURL(repoURL, ve.URLs[0]) + url, err = repo.ResolveReferenceURL(dep.Repository, ve.URLs[0]) if err != nil { //nolint:nakedret return @@ -772,17 +767,16 @@ func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]* } } - urlsKey := repoURL + name + version - if chartURL, ok := resolvedChartUrls[urlsKey]; ok { - url, err = repo.ResolveReferenceURL(repoURL, chartURL) + if dep.ChartURL != "" { + url = dep.ChartURL } else { - url, err = repo.FindChartInRepoURL(repoURL, name, m.Getters, repo.WithChartVersion(version), repo.WithClientTLS(certFile, keyFile, caFile)) + url, err = repo.FindChartInRepoURL(dep.Repository, dep.Name, m.Getters, repo.WithChartVersion(dep.Version), repo.WithClientTLS(certFile, keyFile, caFile)) } if err == nil { return url, username, password, false, false, "", "", "", err } - err = fmt.Errorf("chart %s not found in %s: %w", name, repoURL, err) + err = fmt.Errorf("chart %s not found in %s: %w", dep.Name, dep.Repository, err) return url, username, password, false, false, "", "", "", err } diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index a5b1397ad..d18319391 100644 --- a/pkg/downloader/manager_test.go +++ b/pkg/downloader/manager_test.go @@ -67,11 +67,13 @@ func TestFindChartURL(t *testing.T) { t.Fatal(err) } - name := "alpine" - version := "0.1.0" - repoURL := "http://example.com/charts" + dep := &chart.Dependency{ + Name: "alpine", + Version: "0.1.0", + Repository: "http://example.com/charts", + } - churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err := m.findChartURL(name, version, repoURL, repos, make(map[string]string)) + churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err := m.findChartURL(dep, repos) if err != nil { t.Fatal(err) } @@ -92,11 +94,13 @@ func TestFindChartURL(t *testing.T) { t.Errorf("Unexpected insecureSkipTLSVerify %t", insecureSkipTLSVerify) } - name = "tlsfoo" - version = "1.2.3" - repoURL = "https://example-https-insecureskiptlsverify.com" + dep = &chart.Dependency{ + Name: "tlsfoo", + Version: "1.2.3", + Repository: "https://example-https-insecureskiptlsverify.com", + } - churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err = m.findChartURL(name, version, repoURL, repos, make(map[string]string)) + churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err = m.findChartURL(dep, repos) if err != nil { t.Fatal(err) } @@ -117,11 +121,13 @@ func TestFindChartURL(t *testing.T) { t.Errorf("Unexpected passcredentialsall %t", passcredentialsall) } - name = "foo" - version = "1.2.3" - repoURL = "http://example.com/helm" + dep = &chart.Dependency{ + Name: "foo", + Version: "1.2.3", + Repository: "http://example.com/helm", + } - churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err = m.findChartURL(name, version, repoURL, repos) + churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err = m.findChartURL(dep, repos) if err != nil { t.Fatal(err) } @@ -143,7 +149,7 @@ func TestFindChartURL(t *testing.T) { } } -func TestFindChartURLBasedOnUrlMap(t *testing.T) { +func TestFindChartURLAlreadyKnownUrl(t *testing.T) { var b bytes.Buffer m := &Manager{ Out: &b, @@ -152,43 +158,19 @@ func TestFindChartURLBasedOnUrlMap(t *testing.T) { } repos := make(map[string]*repo.ChartRepository) - name := "alpine" - version := "0.1.0" - repoURL := "http://example.com/charts" - - urlMap := make(map[string]string) - urlKey := repoURL + name + version - urlMap[urlKey] = "https://charts.helm.sh/stable/alpine-0.1.0.tgzFromMap" - - churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err := m.findChartURL(name, version, repoURL, repos, urlMap) - if err != nil { - t.Fatal(err) - } - - if churl != "https://charts.helm.sh/stable/alpine-0.1.0.tgzFromMap" { - t.Errorf("Unexpected URL %q", churl) - } - if username != "" { - t.Errorf("Unexpected username %q", username) - } - if password != "" { - t.Errorf("Unexpected password %q", password) - } - if passcredentialsall != false { - t.Errorf("Unexpected passcredentialsall %t", passcredentialsall) - } - if insecureSkipTLSVerify { - t.Errorf("Unexpected insecureSkipTLSVerify %t", insecureSkipTLSVerify) + dep := &chart.Dependency{ + Name: "alpine", + Version: "0.1.0", + Repository: "http://example.com/charts", + ChartURL: "https://charts.helm.sh/stable/alpine-0.1.0.tgzFromMap", } - urlMap[urlKey] = "alpine-0.1.0.tgz" - - churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err = m.findChartURL(name, version, repoURL, repos, urlMap) + churl, username, password, insecureSkipTLSVerify, passcredentialsall, _, _, _, err := m.findChartURL(dep, repos) if err != nil { t.Fatal(err) } - if churl != "http://example.com/charts/alpine-0.1.0.tgz" { + if churl != "https://charts.helm.sh/stable/alpine-0.1.0.tgzFromMap" { t.Errorf("Unexpected URL %q", churl) } if username != "" { @@ -329,7 +311,7 @@ func TestDownloadAll(t *testing.T) { if err := os.MkdirAll(filepath.Join(chartPath, "tmpcharts"), 0755); err != nil { t.Fatal(err) } - if err := m.downloadAll([]*chart.Dependency{signDep, localDep}, make(map[string]string)); err != nil { + if err := m.downloadAll([]*chart.Dependency{signDep, localDep}); err != nil { t.Error(err) } @@ -358,7 +340,7 @@ version: 0.1.0` Version: "0.1.0", } - err = m.downloadAll([]*chart.Dependency{badLocalDep}, make(map[string]string)) + err = m.downloadAll([]*chart.Dependency{badLocalDep}) if err == nil { t.Fatal("Expected error for bad dependency name") }