From d04b43179bfd815855ed6ec25fbd0cce5d988e80 Mon Sep 17 00:00:00 2001 From: Sebastien Gandon Date: Fri, 27 Apr 2018 18:49:59 +0200 Subject: [PATCH] fix(helm): fix failing helm dep up on subchar requirements Closes #3742 Signed-off-by: Sebastien Gandon --- pkg/downloader/manager.go | 72 +++++++++++++++++++++------------- pkg/downloader/manager_test.go | 61 ++++++++++++++++++++++++++++ pkg/resolver/resolver.go | 42 +++++++++++++++++--- pkg/resolver/resolver_test.go | 55 ++++++++++++++++++++++++++ 4 files changed, 196 insertions(+), 34 deletions(-) diff --git a/pkg/downloader/manager.go b/pkg/downloader/manager.go index 372940880..98d66a20f 100644 --- a/pkg/downloader/manager.go +++ b/pkg/downloader/manager.go @@ -216,42 +216,58 @@ func (m *Manager) downloadAll(deps []*chartutil.Dependency) error { fmt.Fprintf(m.Out, "Saving %d charts\n", len(deps)) var saveError error for _, dep := range deps { + var isSubChart bool if strings.HasPrefix(dep.Repository, "file://") { - if m.Debug { - fmt.Fprintf(m.Out, "Archiving %s from repo %s\n", dep.Name, dep.Repository) - } - ver, err := tarFromLocalDir(m.ChartPath, dep.Name, dep.Repository, dep.Version) + //check if file repo path does belong to the current chart "/charts" folder + isSubChart, err = resolver.IsLocalSubChart(dep.Repository, m.ChartPath) if err != nil { saveError = err - break - } - dep.Version = ver - continue + } //else everythig is fine. } - fmt.Fprintf(m.Out, "Downloading %s from repo %s\n", dep.Name, dep.Repository) + //Only download or package dependencies that are not sub chart in the "/charts" folder + if !isSubChart { + if strings.HasPrefix(dep.Repository, "file://") { + if m.Debug { + fmt.Fprintf(m.Out, "Archiving %s from repo %s\n", dep.Name, dep.Repository) + } + ver, err := tarFromLocalDir(m.ChartPath, dep.Name, dep.Repository, dep.Version) + if err != nil { + saveError = err + break + } + dep.Version = ver + continue + } - // Any failure to resolve/download a chart should fail: - // https://github.com/kubernetes/helm/issues/1439 - churl, username, password, err := findChartURL(dep.Name, dep.Version, dep.Repository, repos) - if err != nil { - saveError = fmt.Errorf("could not find %s: %s", churl, err) - break - } + fmt.Fprintf(m.Out, "Downloading %s from repo %s\n", dep.Name, dep.Repository) - dl := ChartDownloader{ - Out: m.Out, - Verify: m.Verify, - Keyring: m.Keyring, - HelmHome: m.HelmHome, - Getters: m.Getters, - Username: username, - Password: password, - } + // Any failure to resolve/download a chart should fail: + // https://github.com/kubernetes/helm/issues/1439 + churl, username, password, err := findChartURL(dep.Name, dep.Version, dep.Repository, repos) + if err != nil { + saveError = fmt.Errorf("could not find %s: %s", churl, err) + break + } + + dl := ChartDownloader{ + Out: m.Out, + Verify: m.Verify, + Keyring: m.Keyring, + HelmHome: m.HelmHome, + Getters: m.Getters, + Username: username, + Password: password, + } - if _, _, err := dl.DownloadTo(churl, "", destPath); err != nil { - saveError = fmt.Errorf("could not download %s: %s", churl, err) - break + if _, _, err := dl.DownloadTo(churl, "", destPath); err != nil { + saveError = fmt.Errorf("could not download %s: %s", churl, err) + break + } + } else { + if m.Debug { + fmt.Fprintf(m.Out, "did not package local chart %s: %s\n", dep.Name, dep.Repository) + } } } diff --git a/pkg/downloader/manager_test.go b/pkg/downloader/manager_test.go index cb588394a..ed806ccae 100644 --- a/pkg/downloader/manager_test.go +++ b/pkg/downloader/manager_test.go @@ -17,6 +17,9 @@ package downloader import ( "bytes" + "io/ioutil" + "os" + "path/filepath" "reflect" "strings" "testing" @@ -95,6 +98,7 @@ func TestFindChartURL(t *testing.T) { func TestGetRepoNames(t *testing.T) { b := bytes.NewBuffer(nil) + m := &Manager{ Out: b, HelmHome: helmpath.Home("testdata/helmhome"), @@ -181,3 +185,60 @@ func TestGetRepoNames(t *testing.T) { } } } + +func TestDownloadAllSubChartsOnly(t *testing.T) { + b := bytes.NewBuffer(nil) + + //create a chart dir + tmp, err := ioutil.TempDir("", "helm-chart") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(tmp) + + m := &Manager{ + Out: b, + HelmHome: helmpath.Home("testdata/helmhome"), + ChartPath: tmp, + } + //create the /charts subdir + chartsFolder := filepath.Join(tmp, "charts") + if err != nil { + t.Fatal(err) + } + os.Mkdir(chartsFolder, 0755) + + tests := []struct { + name string + req []*chartutil.Dependency + expect map[string]string + err bool + }{ + { + name: "sub char relative no dot", + req: []*chartutil.Dependency{ + {Name: "foo", Repository: "file://charts/foo"}, + }, + }, + { + name: "sub chart relative with dot", + req: []*chartutil.Dependency{ + {Name: "foo", Repository: "file://./charts/foo"}, + }, + }, + { + name: "sub chart absolute", + req: []*chartutil.Dependency{ + {Name: "foo", Repository: "file://" + filepath.Join(chartsFolder, "foo")}, + }, + }, + } + + for _, tt := range tests { + //we only check the downloadAll does not return any error. + err := m.downloadAll(tt.req) + if err != nil { + t.Fatal(err) + } + } +} diff --git a/pkg/resolver/resolver.go b/pkg/resolver/resolver.go index 8177df2d3..669de8d63 100644 --- a/pkg/resolver/resolver.go +++ b/pkg/resolver/resolver.go @@ -128,8 +128,24 @@ func HashReq(req *chartutil.Requirements) (string, error) { } // GetLocalPath generates absolute local path when use -// "file://" in repository of requirements +// "file://" in repository of requirements and check if it exists func GetLocalPath(repo string, chartpath string) (string, error) { + localPath, err := GenerateLocalPath(repo, chartpath) + if err != nil { + return "", err + } + if _, err := os.Stat(localPath); os.IsNotExist(err) { + return "", fmt.Errorf("directory %s not found", localPath) + } else if err != nil { + return "", err + } + + return localPath, nil +} + +// GenerateLocalPath generates absolute local path when use +// "file://" in repository of requirements +func GenerateLocalPath(repo string, chartpath string) (string, error) { var depPath string var err error p := strings.TrimPrefix(repo, "file://") @@ -143,11 +159,25 @@ func GetLocalPath(repo string, chartpath string) (string, error) { depPath = filepath.Join(chartpath, p) } - if _, err = os.Stat(depPath); os.IsNotExist(err) { - return "", fmt.Errorf("directory %s not found", depPath) - } else if err != nil { - return "", err + return depPath, nil +} + +//IsLocalSubChart checks if file repo path does belong to the chartpath "/charts" folder +func IsLocalSubChart(repo string, chartpath string) (bool, error) { + if !strings.HasPrefix(repo, "file://") { + return false, nil } + var isSubChart bool - return depPath, nil + destPath := filepath.Join(chartpath, "charts") + origPath, err := GenerateLocalPath(repo, chartpath) + if err != nil { + return false, err + } + relPath, err := filepath.Rel(destPath, origPath) + if err != nil { + return false, err + } + isSubChart = !strings.HasPrefix(relPath, ".") + return isSubChart, nil } diff --git a/pkg/resolver/resolver_test.go b/pkg/resolver/resolver_test.go index 689ffbc32..41fbd3009 100644 --- a/pkg/resolver/resolver_test.go +++ b/pkg/resolver/resolver_test.go @@ -169,3 +169,58 @@ func TestHashReq(t *testing.T) { t.Errorf("Expected %q != %q", expect, h) } } + +func TestIsSubChart(t *testing.T) { + tests := []struct { + name string + repo string + chartPath string + res bool + }{ + { + name: "direct subchart", + chartPath: "/foo", + repo: "file://charts/bar", + res: true, + }, + { + name: "direct subchart with a dot", + chartPath: "/foo", + repo: "file://./charts/bar", + res: true, + }, + { + name: "absolute subchart ", + chartPath: "/foo", + repo: "file:///foo/charts/bar", + res: true, + }, + { + name: "absolute not subchart ", + chartPath: "/foo", + repo: "file:///bar/charts/bar", + res: false, + }, + { + name: "relative not subchart ", + chartPath: "/foo", + repo: "file:///./bar", + res: false, + }, + { + name: "not file:// ", + chartPath: "/foo", + repo: "XXX", + res: false, + }, + } + for _, tt := range tests { + result, err := IsLocalSubChart(tt.repo, tt.chartPath) + if err != nil { + t.Fatal(err) + } + if result != tt.res { + t.Errorf("Expected %v != %v", tt.res, result) + } + } +}