Merge pull request #9176 from dastrobu/#9169/lint-dependency-shadowing

lint and validate dependency metadata to reference dependencies with …

pkg/chart/metadata.go

@@ -128,10 +128,19 @@ func (md *Metadata) Validate() error {
 
 	// Aliases need to be validated here to make sure that the alias name does
 	// not contain any illegal characters.
+	dependencies := map[string]*Dependency{}
 	for _, dependency := range md.Dependencies {
 		if err := dependency.Validate(); err != nil {
 			return err
 		}
+		key := dependency.Name
+		if dependency.Alias != "" {
+			key = dependency.Alias
+		}
+		if dependencies[key] != nil {
+			return ValidationErrorf("more than one dependency with name or alias %q", key)
+		}
+		dependencies[key] = dependency
 	}
 	return nil
 }

pkg/chart/metadata_test.go

@@ -21,34 +21,42 @@ import (
 
 func TestValidate(t *testing.T) {
 	tests := []struct {
+		name string
 		md   *Metadata
 		err  error
 	}{
 		{
+			"chart without metadata",
 			nil,
 			ValidationError("chart.metadata is required"),
 		},
 		{
+			"chart without apiVersion",
 			&Metadata{Name: "test", Version: "1.0"},
 			ValidationError("chart.metadata.apiVersion is required"),
 		},
 		{
+			"chart without name",
 			&Metadata{APIVersion: "v2", Version: "1.0"},
 			ValidationError("chart.metadata.name is required"),
 		},
 		{
+			"chart without version",
 			&Metadata{Name: "test", APIVersion: "v2"},
 			ValidationError("chart.metadata.version is required"),
 		},
 		{
+			"chart with bad type",
 			&Metadata{Name: "test", APIVersion: "v2", Version: "1.0", Type: "test"},
 			ValidationError("chart.metadata.type must be application or library"),
 		},
 		{
+			"chart without dependency",
 			&Metadata{Name: "test", APIVersion: "v2", Version: "1.0", Type: "application"},
 			nil,
 		},
 		{
+			"dependency with valid alias",
 			&Metadata{
 				Name:       "test",
 				APIVersion: "v2",
@@ -61,6 +69,7 @@ func TestValidate(t *testing.T) {
 			nil,
 		},
 		{
+			"dependency with bad characters in alias",
 			&Metadata{
 				Name:       "test",
 				APIVersion: "v2",
@@ -73,6 +82,67 @@ func TestValidate(t *testing.T) {
 			ValidationError("dependency \"bad\" has disallowed characters in the alias"),
 		},
 		{
+			"same dependency twice",
+			&Metadata{
+				Name:       "test",
+				APIVersion: "v2",
+				Version:    "1.0",
+				Type:       "application",
+				Dependencies: []*Dependency{
+					{Name: "foo", Alias: ""},
+					{Name: "foo", Alias: ""},
+				},
+			},
+			ValidationError("more than one dependency with name or alias \"foo\""),
+		},
+		{
+			"two dependencies with alias from second dependency shadowing first one",
+			&Metadata{
+				Name:       "test",
+				APIVersion: "v2",
+				Version:    "1.0",
+				Type:       "application",
+				Dependencies: []*Dependency{
+					{Name: "foo", Alias: ""},
+					{Name: "bar", Alias: "foo"},
+				},
+			},
+			ValidationError("more than one dependency with name or alias \"foo\""),
+		},
+		{
+			// this case would make sense and could work in future versions of Helm, currently template rendering would
+			// result in undefined behaviour
+			"same dependency twice with different version",
+			&Metadata{
+				Name:       "test",
+				APIVersion: "v2",
+				Version:    "1.0",
+				Type:       "application",
+				Dependencies: []*Dependency{
+					{Name: "foo", Alias: "", Version: "1.2.3"},
+					{Name: "foo", Alias: "", Version: "1.0.0"},
+				},
+			},
+			ValidationError("more than one dependency with name or alias \"foo\""),
+		},
+		{
+			// this case would make sense and could work in future versions of Helm, currently template rendering would
+			// result in undefined behaviour
+			"two dependencies with same name but different repos",
+			&Metadata{
+				Name:       "test",
+				APIVersion: "v2",
+				Version:    "1.0",
+				Type:       "application",
+				Dependencies: []*Dependency{
+					{Name: "foo", Repository: "repo-0"},
+					{Name: "foo", Repository: "repo-1"},
+				},
+			},
+			ValidationError("more than one dependency with name or alias \"foo\""),
+		},
+		{
+			"dependencies has nil",
 			&Metadata{
 				Name:       "test",
 				APIVersion: "v2",
@@ -85,6 +155,7 @@ func TestValidate(t *testing.T) {
 			ValidationError("dependencies must not contain empty or null nodes"),
 		},
 		{
+			"maintainer not empty",
 			&Metadata{
 				Name:       "test",
 				APIVersion: "v2",
@@ -97,6 +168,7 @@ func TestValidate(t *testing.T) {
 			ValidationError("maintainers must not contain empty or null nodes"),
 		},
 		{
+			"version invalid",
 			&Metadata{APIVersion: "v2", Name: "test", Version: "1.2.3.4"},
 			ValidationError("chart.metadata.version \"1.2.3.4\" is invalid"),
 		},
@@ -105,7 +177,7 @@ func TestValidate(t *testing.T) {
 	for _, tt := range tests {
 		result := tt.md.Validate()
 		if result != tt.err {
-			t.Errorf("expected '%s', got '%s'", tt.err, result)
+			t.Errorf("expected %q, got %q in test %q", tt.err, result, tt.name)
 		}
 	}
 }

pkg/lint/rules/dependencies.go

@@ -37,6 +37,7 @@ func Dependencies(linter *support.Linter) {
 	}
 
 	linter.RunLinterRule(support.ErrorSev, linter.ChartDir, validateDependencyInMetadata(c))
+	linter.RunLinterRule(support.ErrorSev, linter.ChartDir, validateDependenciesUnique(c))
 	linter.RunLinterRule(support.WarningSev, linter.ChartDir, validateDependencyInChartsDir(c))
 }
 
@@ -80,3 +81,23 @@ func validateDependencyInMetadata(c *chart.Chart) (err error) {
 	}
 	return err
 }
+
+func validateDependenciesUnique(c *chart.Chart) (err error) {
+	dependencies := map[string]*chart.Dependency{}
+	shadowing := []string{}
+
+	for _, dep := range c.Metadata.Dependencies {
+		key := dep.Name
+		if dep.Alias != "" {
+			key = dep.Alias
+		}
+		if dependencies[key] != nil {
+			shadowing = append(shadowing, key)
+		}
+		dependencies[key] = dep
+	}
+	if len(shadowing) > 0 {
+		err = fmt.Errorf("multiple dependencies with name or alias: %s", strings.Join(shadowing, ","))
+	}
+	return err
+}

pkg/lint/rules/dependencies_test.go

@@ -76,6 +76,67 @@ func TestValidateDependencyInMetadata(t *testing.T) {
 	}
 }
 
+func TestValidateDependenciesUnique(t *testing.T) {
+	tests := []struct {
+		chart chart.Chart
+	}{
+		{chart.Chart{
+			Metadata: &chart.Metadata{
+				Name:       "badchart",
+				Version:    "0.1.0",
+				APIVersion: "v2",
+				Dependencies: []*chart.Dependency{
+					{
+						Name: "foo",
+					},
+					{
+						Name: "foo",
+					},
+				},
+			},
+		}},
+		{chart.Chart{
+			Metadata: &chart.Metadata{
+				Name:       "badchart",
+				Version:    "0.1.0",
+				APIVersion: "v2",
+				Dependencies: []*chart.Dependency{
+					{
+						Name:  "foo",
+						Alias: "bar",
+					},
+					{
+						Name: "bar",
+					},
+				},
+			},
+		}},
+		{chart.Chart{
+			Metadata: &chart.Metadata{
+				Name:       "badchart",
+				Version:    "0.1.0",
+				APIVersion: "v2",
+				Dependencies: []*chart.Dependency{
+					{
+						Name:  "foo",
+						Alias: "baz",
+					},
+					{
+						Name:  "bar",
+						Alias: "baz",
+					},
+				},
+			},
+		}},
+	}
+
+	for _, tt := range tests {
+		if err := validateDependenciesUnique(&tt.chart); err == nil {
+			t.Errorf("chart should have been flagged for dependency shadowing")
+		}
+	}
+}
+
 func TestDependencies(t *testing.T) {
 	tmp := t.TempDir()