From 4b23d0a25b15f990e5de9bc2e1e8cbd80de9243c Mon Sep 17 00:00:00 2001 From: Matthew Fisher Date: Tue, 29 Jun 2021 11:46:58 -0700 Subject: [PATCH] fix: refactor downloadAll This refactor cleans up downloadAll's validation, download, and save logic: 1. A temporary directory is created, and removed after all references to the struct have been dropped via `defer` 2. Any local dependencies in the `charts` directory are kept intact and validated 3. Charts that have been updated are moved to the `charts` directory This refactor has a number of improvements, including: - tmpCharts is removed after execution - no remote charts are downloaded to destPath: they are all pulled into tmpPath, validated, then moved to destPath - lots of code cleanup/improvements, like the `if` block checking whether the `charts` directory was actually not a directory. In some cases it could be checking a `nil` object, causing a runtime panic. - the cyclomatic complexity of the code was simplified - extra (and in some cases, dangerous) calls to `os.RemoveAll` have been refactored, cleaning the code and preventing certain failure cases. A test has been provided to demonstrate the tmpCharts removal issue has been fixed. Signed-off-by: Matthew Fisher --- pkg/downloader/manager.go | 137 +++++++++++++++++---------------- pkg/downloader/manager_test.go | 51 ++++++++++++ 2 files changed, 122 insertions(+), 66 deletions(-) diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index 9491f41ad..8d210ea70 100644 --- a/pkg/downloader/manager.go +++ b/pkg/downloader/manager.go @@ -249,22 +249,24 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { destPath := filepath.Join(m.ChartPath, "charts") tmpPath := filepath.Join(m.ChartPath, "tmpcharts") - // Create 'charts' directory if it doesn't already exist. - if fi, err := os.Stat(destPath); err != nil { + // Check if 'charts' directory is not actally a directory. If it does not exist, create it. + if fi, err := os.Stat(destPath); err == nil { + if !fi.IsDir() { + return errors.Errorf("%q is not a directory", destPath) + } + } else if os.IsNotExist(err) { if err := os.MkdirAll(destPath, 0755); err != nil { return err } - } else if !fi.IsDir() { - return errors.Errorf("%q is not a directory", destPath) - } - - if err := fs.RenameWithFallback(destPath, tmpPath); err != nil { - return errors.Wrap(err, "unable to move current charts to tmp dir") + } else { + return fmt.Errorf("unable to retrieve file info for '%s': %v", destPath, err) } - if err := os.MkdirAll(destPath, 0755); err != nil { + // Prepare tmpPath + if err := os.MkdirAll(tmpPath, 0755); err != nil { return err } + defer os.RemoveAll(tmpPath) fmt.Fprintf(m.Out, "Saving %d charts\n", len(deps)) var saveError error @@ -273,10 +275,11 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { // No repository means the chart is in charts directory if dep.Repository == "" { fmt.Fprintf(m.Out, "Dependency %s did not declare a repository. Assuming it exists in the charts directory\n", dep.Name) - chartPath := filepath.Join(tmpPath, dep.Name) + // NOTE: we are only validating the local dependency conforms to the constraints. No copying to tmpPath is necessary. + chartPath := filepath.Join(destPath, dep.Name) ch, err := loader.LoadDir(chartPath) if err != nil { - return fmt.Errorf("unable to load chart: %v", err) + return fmt.Errorf("unable to load chart '%s': %v", chartPath, err) } constraint, err := semver.NewConstraint(dep.Version) @@ -354,8 +357,7 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { getter.WithTagName(version)) } - _, _, err = dl.DownloadTo(churl, version, destPath) - if err != nil { + if _, _, err = dl.DownloadTo(churl, version, tmpPath); err != nil { saveError = errors.Wrapf(err, "could not download %s", churl) break } @@ -363,36 +365,14 @@ func (m *Manager) downloadAll(deps []*chart.Dependency) error { churls[churl] = struct{}{} } + // TODO: this should probably be refactored to be a []error, so we can capture and provide more information rather than "last error wins". if saveError == nil { - fmt.Fprintln(m.Out, "Deleting outdated charts") - for _, dep := range deps { - // Chart from local charts directory stays in place - if dep.Repository != "" { - if err := m.safeDeleteDep(dep.Name, tmpPath); err != nil { - return err - } - } - } - if err := move(tmpPath, destPath); err != nil { + // now we can move all downloaded charts to destPath and delete outdated dependencies + if err := m.safeMoveDeps(deps, tmpPath, destPath); err != nil { return err } - if err := os.RemoveAll(tmpPath); err != nil { - return errors.Wrapf(err, "failed to remove %v", tmpPath) - } } else { fmt.Fprintln(m.Out, "Save error occurred: ", saveError) - fmt.Fprintln(m.Out, "Deleting newly downloaded charts, restoring pre-update state") - for _, dep := range deps { - if err := m.safeDeleteDep(dep.Name, destPath); err != nil { - return err - } - } - if err := os.RemoveAll(destPath); err != nil { - return errors.Wrapf(err, "failed to remove %v", destPath) - } - if err := fs.RenameWithFallback(tmpPath, destPath); err != nil { - return errors.Wrap(err, "unable to move current charts to tmp dir") - } return saveError } return nil @@ -410,36 +390,75 @@ func parseOCIRef(chartRef string) (string, string, error) { return chartRef, tag, nil } -// safeDeleteDep deletes any versions of the given dependency in the given directory. +// safeMoveDep moves all dependencies in the source and moves them into dest. // // It does this by first matching the file name to an expected pattern, then loading -// the file to verify that it is a chart with the same name as the given name. +// the file to verify that it is a chart. // -// Because it requires tar file introspection, it is more intensive than a basic delete. +// Any charts in dest that do not exist in source are removed (barring local dependencies) +// +// Because it requires tar file introspection, it is more intensive than a basic move. // // This will only return errors that should stop processing entirely. Other errors // will emit log messages or be ignored. -func (m *Manager) safeDeleteDep(name, dir string) error { - files, err := filepath.Glob(filepath.Join(dir, name+"-*.tgz")) +func (m *Manager) safeMoveDeps(deps []*chart.Dependency, source, dest string) error { + existsInSourceDirectory := map[string]bool{} + isLocalDependency := map[string]bool{} + sourceFiles, err := ioutil.ReadDir(source) if err != nil { - // Only for ErrBadPattern return err } - for _, fname := range files { - ch, err := loader.LoadFile(fname) - if err != nil { - fmt.Fprintf(m.Out, "Could not verify %s for deletion: %s (Skipping)", fname, err) + // attempt to read destFiles; fail fast if we can't + destFiles, err := ioutil.ReadDir(dest) + if err != nil { + return err + } + + for _, dep := range deps { + if dep.Repository == "" { + isLocalDependency[dep.Name] = true + } + } + + for _, file := range sourceFiles { + if file.IsDir() { continue } - if ch.Name() != name { - // This is not the file you are looking for. + filename := file.Name() + sourcefile := filepath.Join(source, filename) + destfile := filepath.Join(dest, filename) + existsInSourceDirectory[filename] = true + if _, err := loader.LoadFile(sourcefile); err != nil { + fmt.Fprintf(m.Out, "Could not verify %s for moving: %s (Skipping)", sourcefile, err) continue } - if err := os.Remove(fname); err != nil { - fmt.Fprintf(m.Out, "Could not delete %s: %s (Skipping)", fname, err) + // NOTE: no need to delete the dest; os.Rename replaces it. + if err := fs.RenameWithFallback(sourcefile, destfile); err != nil { + fmt.Fprintf(m.Out, "Unable to move %s to charts dir %s (Skipping)", sourcefile, err) continue } } + + fmt.Fprintln(m.Out, "Deleting outdated charts") + // find all files that exist in dest that do not exist in source; delete them (outdated dependendencies) + for _, file := range destFiles { + if !file.IsDir() && !existsInSourceDirectory[file.Name()] { + fname := filepath.Join(dest, file.Name()) + ch, err := loader.LoadFile(fname) + if err != nil { + fmt.Fprintf(m.Out, "Could not verify %s for deletion: %s (Skipping)", fname, err) + } + // local dependency - skip + if isLocalDependency[ch.Name()] { + continue + } + if err := os.Remove(fname); err != nil { + fmt.Fprintf(m.Out, "Could not delete %s: %s (Skipping)", fname, err) + continue + } + } + } + return nil } @@ -868,20 +887,6 @@ func tarFromLocalDir(chartpath, name, repo, version string) (string, error) { return "", errors.Errorf("can't get a valid version for dependency %s", name) } -// move files from tmppath to destpath -func move(tmpPath, destPath string) error { - files, _ := os.ReadDir(tmpPath) - for _, file := range files { - filename := file.Name() - tmpfile := filepath.Join(tmpPath, filename) - destfile := filepath.Join(destPath, filename) - if err := fs.RenameWithFallback(tmpfile, destfile); err != nil { - return errors.Wrap(err, "unable to move local charts to charts dir") - } - } - return nil -} - // The prefix to use for cache keys created by the manager for repo names const managerKeyPrefix = "helm-manager-" diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index b8da42cd2..b2b32bf74 100644 --- a/pkg/downloader/manager_test.go +++ b/pkg/downloader/manager_test.go @@ -17,11 +17,14 @@ package downloader import ( "bytes" + "io/ioutil" + "os" "path/filepath" "reflect" "testing" "helm.sh/helm/v3/pkg/chart" + "helm.sh/helm/v3/pkg/chart/loader" "helm.sh/helm/v3/pkg/chartutil" "helm.sh/helm/v3/pkg/getter" "helm.sh/helm/v3/pkg/repo/repotest" @@ -213,6 +216,54 @@ func TestGetRepoNames(t *testing.T) { } } +func TestDownloadAll(t *testing.T) { + chartPath, err := ioutil.TempDir("", "test-downloadall") + if err != nil { + t.Fatalf("could not create tempdir: %v", err) + } + defer os.RemoveAll(chartPath) + m := &Manager{ + Out: new(bytes.Buffer), + RepositoryConfig: repoConfig, + RepositoryCache: repoCache, + ChartPath: chartPath, + } + signtest, err := loader.LoadDir(filepath.Join("testdata", "signtest")) + if err != nil { + t.Fatal(err) + } + if err := chartutil.SaveDir(signtest, filepath.Join(chartPath, "testdata")); err != nil { + t.Fatal(err) + } + + local, err := loader.LoadDir(filepath.Join("testdata", "local-subchart")) + if err != nil { + t.Fatal(err) + } + if err := chartutil.SaveDir(local, filepath.Join(chartPath, "charts")); err != nil { + t.Fatal(err) + } + + signDep := &chart.Dependency{ + Name: signtest.Name(), + Repository: "file://./testdata/signtest", + Version: signtest.Metadata.Version, + } + localDep := &chart.Dependency{ + Name: local.Name(), + Repository: "", + Version: local.Metadata.Version, + } + + // create a 'tmpcharts' directory to test #5567 + if err := os.MkdirAll(filepath.Join(chartPath, "tmpcharts"), 0755); err != nil { + t.Fatal(err) + } + if err := m.downloadAll([]*chart.Dependency{signDep, localDep}); err != nil { + t.Error(err) + } +} + func TestUpdateBeforeBuild(t *testing.T) { // Set up a fake repo srv, err := repotest.NewTempServerWithCleanup(t, "testdata/*.tgz*")