Merge pull request #8753 from technosophos/fix/dependency-status

handle case where dependency name collisions break dependency resolution

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()

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))
+}