msb-public
/
helm
mirror of https://github.com/helm/helm
1
0
Code Issues Projects Releases Wiki Activity

fix: refactor downloadAll

Browse Source 
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 <matt.fisher@microsoft.com>
pull/9889/head
Matthew Fisher 4 years ago
parent 92bd9558e5
commit 4b23d0a25b
No known key found for this signature in database
GPG Key ID: 92AA783CBAAE8E3B
2 changed files with 122 additions and 66 deletions
Show Stats Download Patch File Download Diff File

137
pkg/downloader/manager.go
Unescape View File

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

51
pkg/downloader/manager_test.go
Unescape View File

@ -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*")

Write Preview
Loading…
Cancel
Save