From c81bd8912e67177f7941714098e762a88f04c430 Mon Sep 17 00:00:00 2001 From: Matt Farina Date: Thu, 12 Sep 2024 10:21:14 -0400 Subject: [PATCH] Revert "Improve helm dependency update performance" The change in #11726 caused a regression where `helm dependency udpate` stopped working. The format of the internal representation of the data changed causing errors of "non-absolute URLs should be in form of repo_name/path_to_chart". See #13324 for more details. Since this change is in released Helm and it's a regression, reverting the original change was the fastest and safest route to deliver a fix as quickly as possible. Closes #13324 Signed-off-by: Matt Farina --- internal/resolver/resolver.go | 44 +++++++++--------------------- internal/resolver/resolver_test.go | 2 +- pkg/downloader/manager.go | 32 ++++++---------------- pkg/downloader/manager_test.go | 8 +++--- 4 files changed, 26 insertions(+), 60 deletions(-) diff --git a/internal/resolver/resolver.go b/internal/resolver/resolver.go index 3d2878ea9..c5fc63643 100644 --- a/internal/resolver/resolver.go +++ b/internal/resolver/resolver.go @@ -52,23 +52,21 @@ func New(chartpath, cachepath string, registryClient *registry.Client) *Resolver } // Resolve resolves dependencies and returns a lock file with the resolution. -func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string) (*chart.Lock, map[string]string, error) { +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{} - loadedIndexFiles := 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, errors.Wrapf(err, "dependency %q has an invalid version/constraint format", d.Name) + return nil, errors.Wrapf(err, "dependency %q has an invalid version/constraint format", d.Name) } 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{ @@ -81,12 +79,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) @@ -124,26 +122,14 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string var ok bool found := true if !registry.IsOCI(d.Repository) { - filepath := filepath.Join(r.cachepath, helmpath.CacheIndexFile(repoName)) - var repoIndex *repo.IndexFile - - // Store previously loaded index files in a map. If repositories share the - // same index file there is no need to reload the same file again. This - // improves performance. - if indexFile, loaded := loadedIndexFiles[filepath]; !loaded { - var err error - repoIndex, err = repo.LoadIndexFile(filepath) - loadedIndexFiles[filepath] = repoIndex - if err != nil { - return nil, nil, errors.Wrapf(err, "no cached repository for %s found. (try 'helm repo update')", repoName) - } - } else { - repoIndex = indexFile + repoIndex, err := repo.LoadIndexFile(filepath.Join(r.cachepath, helmpath.CacheIndexFile(repoName))) + if err != nil { + return nil, errors.Wrapf(err, "no cached repository for %s found. (try 'helm repo update')", repoName) } vs, ok = repoIndex.Entries[d.Name] if !ok { - return nil, nil, errors.Errorf("%s chart not found in repo %s", d.Name, d.Repository) + return nil, errors.Errorf("%s chart not found in repo %s", d.Name, d.Repository) } found = false } else { @@ -165,7 +151,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, errors.Wrapf(err, "could not retrieve list of tags for repository %s", d.Repository) + return nil, errors.Wrapf(err, "could not retrieve list of tags for repository %s", d.Repository) } vs = make(repo.ChartVersions, len(tags)) @@ -186,7 +172,6 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string Repository: d.Repository, Version: version, } - // The version are already sorted and hence the first one to satisfy the constraint is used for _, ver := range vs { v, err := semver.NewVersion(ver.Version) @@ -197,9 +182,6 @@ 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].Version = v.Original() break } @@ -210,19 +192,19 @@ func (r *Resolver) Resolve(reqs []*chart.Dependency, repoNames map[string]string } } if len(missing) > 0 { - return nil, nil, errors.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, errors.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 a4c4ef351..a79852175 100644 --- a/internal/resolver/resolver_test.go +++ b/internal/resolver/resolver_test.go @@ -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/downloader/manager.go b/pkg/downloader/manager.go index 03c13bcf1..d5340575d 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,7 +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. -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) } @@ -239,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 @@ -312,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.Name, dep.Version, dep.Repository, repos) if err != nil { saveError = errors.Wrapf(err, "could not find %s", churl) break @@ -501,7 +501,6 @@ func (m *Manager) ensureMissingRepos(repoNames map[string]string, deps []*chart. var ru []*repo.Entry -Outer: for _, dd := range deps { // If the chart is in the local charts directory no repository needs @@ -529,14 +528,6 @@ Outer: repoNames[dd.Name] = rn - // If repository is already present don't add to array. This will skip - // unnecessary index file downloading improving performance. - for _, item := range ru { - if item.URL == dd.Repository { - continue Outer - } - } - // Assuming the repository is generally available. For Helm managed // access controls the repository needs to be added through the user // managed system. This path will work for public charts, like those @@ -712,7 +703,7 @@ func (m *Manager) parallelRepoUpdate(repos []*repo.Entry) error { // repoURL is the repository to search // // 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, urls map[string]string) (url, username, password string, insecureskiptlsverify, passcredentialsall bool, caFile, certFile, keyFile string, err error) { +func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]*repo.ChartRepository) (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 } @@ -751,14 +742,7 @@ func (m *Manager) findChartURL(name, version, repoURL string, repos map[string]* return } } - - urlsKey := repoURL + name + version - if _, ok := urls[urlsKey]; ok { - url = urls[urlsKey] - } else { - url, err = repo.FindChartInRepoURL(repoURL, name, version, certFile, keyFile, caFile, m.Getters) - } - + url, err = repo.FindChartInRepoURL(repoURL, name, version, certFile, keyFile, caFile, m.Getters) if err == nil { return url, username, password, false, false, "", "", "", err } diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index a2ced2e5d..db2487d16 100644 --- a/pkg/downloader/manager_test.go +++ b/pkg/downloader/manager_test.go @@ -84,7 +84,7 @@ func TestFindChartURL(t *testing.T) { version := "0.1.0" repoURL := "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(name, version, repoURL, repos) if err != nil { t.Fatal(err) } @@ -109,7 +109,7 @@ func TestFindChartURL(t *testing.T) { version = "1.2.3" repoURL = "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(name, version, repoURL, repos) if err != nil { t.Fatal(err) } @@ -255,7 +255,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) } @@ -284,7 +284,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") }