Repository: helm/helm (go)

Issue:
dependency name (and alias) clashes should be reported as errors
While trying to fix #9150 I found another class of issues with aliases and name clashes of dependencies. 

I am proposing to report the following situation as errors on linting (and template/installing/upgrading) a chart: 

```
dependencies:
- name: foo
  alias: baz # ← baz used twice
  version: 1.0.0
- name: bar
  alias: baz # ← baz used twice
  version: 1.0.0
```
```
dependencies:
- name: foo
  alias: bar # ← shadows chart below
  version: 1.0.0
- name: bar
  version: 1.0.0
```
```
dependencies:
- name: foo
  version: 1.0.0
- name: foo # ← chart with same name as above (although version or repo will be different, this will not work currently)
  version: 1.2.3
```
While the last example may be working in the future (see #7413), I can't see any valid use case for the first two examples. 
Currently Helm renders templates, resulting in more or less undefined behaviour. Some templates will override each other, others won't, depending on file names in the actual charts and on the chart names itself. 

Output of `helm version`:
`version.BuildInfo{Version:"v3.4.2", GitCommit:"23dd3af5e19a02d4f4baa5b2f242645a1a3af629", GitTreeState:"clean", GoVersion:"go1.14.13"}`



Pull request:
lint and validate dependency metadata to reference dependencies with …
…a unique key (name or alias)

Report charts with the following bad dependency specifications as bad charts:

    dependencies:
    - name: foo
      alias: baz # ← baz used twice
      version: 1.0.0
    - name: bar
      alias: baz # ← baz used twice
      version: 1.0.0

    dependencies:
    - name: foo
      alias: bar # ← shadows chart below
      version: 1.0.0
    - name: bar
      version: 1.0.0

    dependencies:
    - name: foo
      version: 1.0.0
    - name: foo # ← chart with same name as above (although version or repo will be different, this will not work currently)
      version: 1.2.3

Closes #9169

Signed-off-by: Daniel Strobusch <1847260+dastrobu@users.noreply.github.com>

<!--  Thanks for sending a pull request!  Here are some tips for you:
1. Make sure to read the Contributing Guide before submitting your PR: https://github.com/helm/helm/blob/master/CONTRIBUTING.md
2. If this PR closes another issue, add 'closes #<issue number>' somewhere in the PR summary. GitHub will automatically close that issue when this PR gets merged. Alternatively, adding 'refs #<issue number>' will not close the issue, but help provide the reviewer more context.-->

**What this PR does / why we need it**:

**Special notes for your reviewer**:

**If applicable**:
- [ ] this PR contains documentation
- [x] this PR contains unit tests
- [x] this PR has been tested for backwards compatibility


Patch:
diff --git a/pkg/chart/metadata.go b/pkg/chart/metadata.go
index ae572abb7..97bfc2c0c 100644
--- a/pkg/chart/metadata.go
+++ b/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
 }
diff --git a/pkg/lint/rules/dependencies.go b/pkg/lint/rules/dependencies.go
index abecd1feb..f1ab1dcad 100644
--- a/pkg/lint/rules/dependencies.go
+++ b/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
+}


Test patch:
diff --git a/pkg/chart/metadata_test.go b/pkg/chart/metadata_test.go
index cc04f095b..c8b89ae55 100644
--- a/pkg/chart/metadata_test.go
+++ b/pkg/chart/metadata_test.go
@@ -21,34 +21,42 @@ import (
 
 func TestValidate(t *testing.T) {
 	tests := []struct {
-		md  *Metadata
-		err error
+		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)
 		}
 	}
 }
diff --git a/pkg/lint/rules/dependencies_test.go b/pkg/lint/rules/dependencies_test.go
index 67b160936..0da996bf6 100644
--- a/pkg/lint/rules/dependencies_test.go
+++ b/pkg/lint/rules/dependencies_test.go
@@ -78,6 +78,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 := ensure.TempDir(t)
 	defer os.RemoveAll(tmp)