From 3df996d7d3960b8e4bbb5055ddf95998bbe227c3 Mon Sep 17 00:00:00 2001 From: Matt Butcher Date: Fri, 11 Sep 2020 16:23:34 -0600 Subject: [PATCH] handle case where dependency name collisions break dependency resolution Signed-off-by: Matt Butcher (cherry picked from commit 40b78002873d525a31c5dec75c8607be67327360) --- pkg/action/dependency.go | 87 ++++++++++++++++++++-------- pkg/action/dependency_test.go | 103 ++++++++++++++++++++++++++++++++++ 2 files changed, 167 insertions(+), 23 deletions(-) diff --git a/pkg/action/dependency.go b/pkg/action/dependency.go index 4a4b8ebad..4c80d0159 100644 --- a/pkg/action/dependency.go +++ b/pkg/action/dependency.go @@ -21,6 +21,7 @@ import ( "io" "os" "path/filepath" + "strings" "github.com/Masterminds/semver/v3" "github.com/gosuri/uitable" @@ -61,6 +62,7 @@ func (d *Dependency) List(chartpath string, out io.Writer) error { return nil } +// dependecyStatus returns a string describing the status of a dependency viz a viz the parent chart. func (d *Dependency) dependencyStatus(chartpath string, dep *chart.Dependency, parent *chart.Chart) string { filename := fmt.Sprintf("%s-%s.tgz", dep.Name, "*") @@ -75,35 +77,40 @@ func (d *Dependency) dependencyStatus(chartpath string, dep *chart.Dependency, p case err != nil: return "bad pattern" case len(archives) > 1: - return "too many matches" - case len(archives) == 1: - archive := archives[0] - if _, err := os.Stat(archive); err == nil { - c, err := loader.Load(archive) - if err != nil { - return "corrupt" + // See if the second part is a SemVer + found := []string{} + for _, arc := range archives { + // we need to trip the prefix dirs and the extension off. + filename = strings.TrimSuffix(filepath.Base(arc), ".tgz") + maybeVersion := strings.TrimPrefix(filename, fmt.Sprintf("%s-", dep.Name)) + + if _, err := semver.StrictNewVersion(maybeVersion); err == nil { + // If the version parsed without an error, it is possibly a valid + // version. + found = append(found, arc) } - if c.Name() != dep.Name { - return "misnamed" + } + + if l := len(found); l == 1 { + // If we get here, we do the same thing as in len(archives) == 1. + if r := statArchiveForStatus(found[0], dep); r != "" { + return r } - if c.Metadata.Version != dep.Version { - constraint, err := semver.NewConstraint(dep.Version) - if err != nil { - return "invalid version" - } + // Fall through and look for directories + } else if l > 1 { + return "too many matches" + } - v, err := semver.NewVersion(c.Metadata.Version) - if err != nil { - return "invalid version" - } + // The sanest thing to do here is to fall through and see if we have any directory + // matches. - if !constraint.Check(v) { - return "wrong version" - } - } - return "ok" + case len(archives) == 1: + archive := archives[0] + if r := statArchiveForStatus(archive, dep); r != "" { + return r } + } // End unnecessary code. @@ -137,6 +144,40 @@ func (d *Dependency) dependencyStatus(chartpath string, dep *chart.Dependency, p return "unpacked" } +// stat an archive and return a message if the stat is successful +// +// This is a refactor of the code originally in dependencyStatus. It is here to +// support legacy behavior, and should be removed in Helm 4. +func statArchiveForStatus(archive string, dep *chart.Dependency) string { + if _, err := os.Stat(archive); err == nil { + c, err := loader.Load(archive) + if err != nil { + return "corrupt" + } + if c.Name() != dep.Name { + return "misnamed" + } + + if c.Metadata.Version != dep.Version { + constraint, err := semver.NewConstraint(dep.Version) + if err != nil { + return "invalid version" + } + + v, err := semver.NewVersion(c.Metadata.Version) + if err != nil { + return "invalid version" + } + + if !constraint.Check(v) { + return "wrong version" + } + } + return "ok" + } + return "" +} + // printDependencies prints all of the dependencies in the yaml file. func (d *Dependency) printDependencies(chartpath string, out io.Writer, c *chart.Chart) { table := uitable.New() diff --git a/pkg/action/dependency_test.go b/pkg/action/dependency_test.go index 4f3cb69a5..b5032a377 100644 --- a/pkg/action/dependency_test.go +++ b/pkg/action/dependency_test.go @@ -18,9 +18,16 @@ package action import ( "bytes" + "io/ioutil" + "os" + "path/filepath" "testing" + "github.com/stretchr/testify/assert" + "helm.sh/helm/v3/internal/test" + "helm.sh/helm/v3/pkg/chart" + "helm.sh/helm/v3/pkg/chartutil" ) func TestList(t *testing.T) { @@ -56,3 +63,99 @@ func TestList(t *testing.T) { test.AssertGoldenBytes(t, buf.Bytes(), tcase.golden) } } + +// TestDependencyStatus_Dashes is a regression test to make sure that dashes in +// chart names do not cause resolution problems. +func TestDependencyStatus_Dashes(t *testing.T) { + // Make a temp dir + dir, err := ioutil.TempDir("", "helmtest-") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + + chartpath := filepath.Join(dir, "charts") + if err := os.MkdirAll(chartpath, 0700); err != nil { + t.Fatal(err) + } + + // Add some fake charts + first := buildChart(withName("first-chart")) + _, err = chartutil.Save(first, chartpath) + if err != nil { + t.Fatal(err) + } + + second := buildChart(withName("first-chart-second-chart")) + _, err = chartutil.Save(second, chartpath) + if err != nil { + t.Fatal(err) + } + + dep := &chart.Dependency{ + Name: "first-chart", + Version: "0.1.0", + } + + // Now try to get the deps + stat := NewDependency().dependencyStatus(dir, dep, first) + if stat != "ok" { + t.Errorf("Unexpected status: %q", stat) + } +} + +func TestStatArchiveForStatus(t *testing.T) { + // Make a temp dir + dir, err := ioutil.TempDir("", "helmtest-") + if err != nil { + t.Fatal(err) + } + defer os.RemoveAll(dir) + + chartpath := filepath.Join(dir, "charts") + if err := os.MkdirAll(chartpath, 0700); err != nil { + t.Fatal(err) + } + + // unsaved chart + lilith := buildChart(withName("lilith")) + + // dep referring to chart + dep := &chart.Dependency{ + Name: "lilith", + Version: "1.2.3", + } + + is := assert.New(t) + + lilithpath := filepath.Join(chartpath, "lilith-1.2.3.tgz") + is.Empty(statArchiveForStatus(lilithpath, dep)) + + // save the chart (version 0.1.0, because that is the default) + where, err := chartutil.Save(lilith, chartpath) + is.NoError(err) + + // Should get "wrong version" because we asked for 1.2.3 and got 0.1.0 + is.Equal("wrong version", statArchiveForStatus(where, dep)) + + // Break version on dep + dep = &chart.Dependency{ + Name: "lilith", + Version: "1.2.3.4.5", + } + is.Equal("invalid version", statArchiveForStatus(where, dep)) + + // Break the name + dep = &chart.Dependency{ + Name: "lilith2", + Version: "1.2.3", + } + is.Equal("misnamed", statArchiveForStatus(where, dep)) + + // Now create the right version + dep = &chart.Dependency{ + Name: "lilith", + Version: "0.1.0", + } + is.Equal("ok", statArchiveForStatus(where, dep)) +}