Repository: helm/helm (go)

Issue:
when killing `helm dep up` the next `dep up` fails
when killing the process running `helm dep up` it sometimes leave a temporary folder named `tmpcharts` and this makes the next `helm dep up` fail with the following error.
```
Error: Unable to move current charts to tmp dir: rename /mychart/charts /mychart/tmpcharts: file exists
```
This is easily reproducable by creating a folder named `tmpcharts` inside an helm chart root folder and try to launch `helm dep up`

Of course a workaround is to remove the folder manually before running the `helm dep up` but that would be great that helm did that automatically somehow.

<!-- If you need help or think you have found a bug, please help us with your issue by entering the following information (otherwise you can delete this text): -->

Output of `helm version`:
```
$helm version
Client: &version.Version{SemVer:"v2.12.2", GitCommit:"7d2b0c73d734f6586ed222a567c5d103fed435be", GitTreeState:"clean"}
Server: &version.Version{SemVer:"v2.13.1", GitCommit:"618447cbf203d147601b4b9bd7f8c37a5d39fbb4", GitTreeState:"clean"}
```
Output of `kubectl version`:
```
kubectl version
Client Version: version.Info{Major:"1", Minor:"13", GitVersion:"v1.13.3", GitCommit:"721bfa751924da8d1680787490c54b9179b1fed0", GitTreeState:"clean", BuildDate:"2019-02-04T04:48:03Z", GoVersion:"go1.11.5", Compiler:"gc", Platform:"darwin/amd64"}
Server Version: version.Info{Major:"1", Minor:"13", GitVersion:"v1.13.0", GitCommit:"ddf47ac13c1a9483ea035a79cd7c10005ff21a6d", GitTreeState:"clean", BuildDate:"2018-12-03T20:56:12Z", GoVersion:"go1.11.2", Compiler:"gc", Platform:"linux/amd64"}
```
Cloud Provider/Platform (AKS, GKE, Minikube etc.): docker for mac


The code handling this is here : https://github.com/helm/helm/blob/53d432fa58748412ff3dc10bc27cbf996d96c3ed/pkg/downloader/manager.go#L190

Pull request:
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
4. Outdated charts in `charts` are removed (barring local dependencies)

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.
- the cyclomatic complexity of the code was simplified
- extra (and in some cases, dangerous) calls to `os.RemoveAll(destPath)` have been removed, cleaning the code and preventing certain failure cases.

A test has been provided to demonstrate the tmpCharts removal issue has been fixed. I also left a comment that further improvements can be made to this piece of code should someone feel the need to make improvements there.

closes #5567
closes #8846 
closes #9680

Signed-off-by: Matthew Fisher <matt.fisher@microsoft.com>

- [x] this PR contains unit tests
- [x] this PR has been tested for backwards compatibility


Patch:
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-"
 


Test patch:
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*")