Repository: helm/helm (go)

Issue:
sub-sub-charts of aliased sub-charts not evaluated correctly
# Summary

Given a chart with a subchart `child` and a sub-sub-chart `grandchild`
```
charts/
 + child
   + grandchild
```
where the sub chart is referenced twice with an alias from the `Chart.yaml`:
```
 dependencies:
 - name: child
   alias: foo
   version: 1.0.0
 - name: child
   alias: bar
   version: 1.0.0
```

It is expected that the resources of the `grandchild` chart are rendered twice with different values specified for the two aliases `foo` and `bar`. 

# [SSCCE](http://www.sscce.org)
To reproduce it fully, run: 

```
mkdir -p parent/charts/child/charts/grandchild/templates
mkdir -p parent/charts/child/templates

echo '
apiVersion: v2
appVersion: 1.0.0
name: grandchild
type: application
version: 1.0.0
' > parent/charts/child/charts/grandchild/Chart.yaml

echo '
apiVersion: v2
appVersion: 1.0.0
name: child
type: application
version: 1.0.0
' > parent/charts/child/Chart.yaml

echo '
apiVersion: v2
appVersion: 1.0.0
name: parent
type: application
version: 1.0.0

dependencies:
- name: child
  alias: foo
  version: 1.0.0
- name: child
  alias: bar
  version: 1.0.0
' > parent/Chart.yaml

echo '
apiVersion: v1
kind: ConfigMap
metadata:
  name: {{ .Chart.Name }}
data:
{{ toYaml .Values | indent 2}}
' > parent/charts/child/templates/dummy.yaml

echo '
apiVersion: v1
kind: ConfigMap
metadata:
  name: {{ .Chart.Name }}-{{ .Values.from }}
data:
{{ toYaml .Values | indent 2}}
' > parent/charts/child/charts/grandchild/templates/dummy.yaml


echo '
foo:
  grandchild:
    from: foo
bar:
  grandchild:
    from: bar
' > parent/values.yaml

helm template parent
```

which prints out: 
```
---
# Source: parent/charts/bar/templates/dummy.yaml
apiVersion: v1
kind: ConfigMap
metadata:
  name: bar
data:
  global: {}
  grandchild:
    from: bar
    global: {}
---
# Source: parent/charts/child/charts/grandchild/templates/dummy.yaml
apiVersion: v1
kind: ConfigMap
metadata:
  name: grandchild-bar
data:
  from: bar
  global: {}
---
# Source: parent/charts/foo/templates/dummy.yaml
apiVersion: v1
kind: ConfigMap
metadata:
  name: foo
data:
  global: {}
  grandchild:
    from: foo
    global: {}
```

What is missing, is the following: 
```
# Source: parent/charts/child/charts/grandchild/templates/dummy.yaml
apiVersion: v1
kind: ConfigMap
metadata:
  name: grandchild-foo
data:
  from: foo
  global: {}
```

From my understanding of sub-charts and aliases, this is a bug.

Output of `helm version`:
`version.BuildInfo{Version:"v3.4.1", GitCommit:"c4e74854886b2efe3321e185578e6db9be0a6e29", GitTreeState:"clean", GoVersion:"go1.14.11"}
`





Pull request:
fix: copy dependencies on aliasing to avoid sharing chart references on multiply aliased dependencies
Dependencies keep a reference on their parent chart, which breaks if a chart reference is shared among multiple aliases.
By copying the dependencies, parent information can be set correctly to render the templates as expected later on.

Note that this change will make ChartFullPath return a different path for sub-subcharts. It will contain the alias names instead of the path to the chart files which makes it consistent with paths to templates on the subchart level.

Closes #9150

Signed-off-by: Daniel Strobusch <1847260+dastrobu@users.noreply.github.com>

**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/v2/chart.go b/pkg/chart/v2/chart.go
index dcc2a43eb..66ddf98a5 100644
--- a/pkg/chart/v2/chart.go
+++ b/pkg/chart/v2/chart.go
@@ -113,6 +113,8 @@ func (ch *Chart) ChartPath() string {
 }
 
 // ChartFullPath returns the full path to this chart.
+// Note that the path may not correspond to the path where the file can be found on the file system if the path
+// points to an aliased subchart.
 func (ch *Chart) ChartFullPath() string {
 	if !ch.IsRoot() {
 		return ch.Parent().ChartFullPath() + "/charts/" + ch.Name()
diff --git a/pkg/chart/v2/util/dependencies.go b/pkg/chart/v2/util/dependencies.go
index b7f78010b..e2cce6f2f 100644
--- a/pkg/chart/v2/util/dependencies.go
+++ b/pkg/chart/v2/util/dependencies.go
@@ -91,6 +91,7 @@ func processDependencyTags(reqs []*chart.Dependency, cvals Values) {
 	}
 }
 
+// getAliasDependency finds the chart for an alias dependency and copies parts that will be modified
 func getAliasDependency(charts []*chart.Chart, dep *chart.Dependency) *chart.Chart {
 	for _, c := range charts {
 		if c == nil {
@@ -104,17 +105,38 @@ func getAliasDependency(charts []*chart.Chart, dep *chart.Dependency) *chart.Cha
 		}
 
 		out := *c
-		md := *c.Metadata
-		out.Metadata = &md
+		out.Metadata = copyMetadata(c.Metadata)
+
+		// empty dependencies and shallow copy all dependencies, otherwise parent info may be corrupted if
+		// there is more than one dependency aliasing this chart
+		out.SetDependencies()
+		for _, dependency := range c.Dependencies() {
+			cpy := *dependency
+			out.AddDependency(&cpy)
+		}
 
 		if dep.Alias != "" {
-			md.Name = dep.Alias
+			out.Metadata.Name = dep.Alias
 		}
 		return &out
 	}
 	return nil
 }
 
+func copyMetadata(metadata *chart.Metadata) *chart.Metadata {
+	md := *metadata
+
+	if md.Dependencies != nil {
+		dependencies := make([]*chart.Dependency, len(md.Dependencies))
+		for i := range md.Dependencies {
+			dependency := *md.Dependencies[i]
+			dependencies[i] = &dependency
+		}
+		md.Dependencies = dependencies
+	}
+	return &md
+}
+
 // processDependencyEnabled removes disabled charts from dependencies
 func processDependencyEnabled(c *chart.Chart, v map[string]interface{}, path string) error {
 	if c.Metadata.Dependencies == nil {


Test patch:
diff --git a/pkg/chart/v2/util/dependencies_test.go b/pkg/chart/v2/util/dependencies_test.go
index 5bd332990..9b7fe3bef 100644
--- a/pkg/chart/v2/util/dependencies_test.go
+++ b/pkg/chart/v2/util/dependencies_test.go
@@ -286,6 +286,38 @@ func TestProcessDependencyImportValues(t *testing.T) {
 	}
 }
 
+func TestProcessDependencyImportValuesFromSharedDependencyToAliases(t *testing.T) {
+	c := loadChart(t, "testdata/chart-with-import-from-aliased-dependencies")
+
+	if err := processDependencyEnabled(c, c.Values, ""); err != nil {
+		t.Fatalf("expected no errors but got %q", err)
+	}
+	if err := processDependencyImportValues(c, true); err != nil {
+		t.Fatalf("processing import values dependencies %v", err)
+	}
+	e := make(map[string]string)
+
+	e["foo-defaults.defaultValue"] = "42"
+	e["bar-defaults.defaultValue"] = "42"
+
+	e["foo.defaults.defaultValue"] = "42"
+	e["bar.defaults.defaultValue"] = "42"
+
+	e["foo.grandchild.defaults.defaultValue"] = "42"
+	e["bar.grandchild.defaults.defaultValue"] = "42"
+
+	cValues := Values(c.Values)
+	for kk, vv := range e {
+		pv, err := cValues.PathValue(kk)
+		if err != nil {
+			t.Fatalf("retrieving import values table %v %v", kk, err)
+		}
+		if pv != vv {
+			t.Errorf("failed to match imported value %v with expected %v", pv, vv)
+		}
+	}
+}
+
 func TestProcessDependencyImportValuesMultiLevelPrecedence(t *testing.T) {
 	c := loadChart(t, "testdata/three-level-dependent-chart/umbrella")
 
@@ -430,6 +462,9 @@ func TestDependentChartAliases(t *testing.T) {
 	if aliasChart == nil {
 		t.Fatalf("failed to get dependency chart for alias %s", req[2].Name)
 	}
+	if aliasChart.Parent() != c {
+		t.Fatalf("dependency chart has wrong parent, expected %s but got %s", c.Name(), aliasChart.Parent().Name())
+	}
 	if req[2].Alias != "" {
 		if aliasChart.Name() != req[2].Alias {
 			t.Fatalf("dependency chart name should be %s but got %s", req[2].Alias, aliasChart.Name())
@@ -521,3 +556,32 @@ func TestDependentChartsWithSomeSubchartsSpecifiedInDependency(t *testing.T) {
 		t.Fatalf("expected 1 dependency specified in Chart.yaml, got %d", len(c.Metadata.Dependencies))
 	}
 }
+
+func validateDependencyTree(t *testing.T, c *chart.Chart) {
+	for _, dependency := range c.Dependencies() {
+		if dependency.Parent() != c {
+			if dependency.Parent() != c {
+				t.Fatalf("dependency chart %s has wrong parent, expected %s but got %s", dependency.Name(), c.Name(), dependency.Parent().Name())
+			}
+		}
+		// recurse entire tree
+		validateDependencyTree(t, dependency)
+	}
+}
+
+func TestChartWithDependencyAliasedTwiceAndDoublyReferencedSubDependency(t *testing.T) {
+	c := loadChart(t, "testdata/chart-with-dependency-aliased-twice")
+
+	if len(c.Dependencies()) != 1 {
+		t.Fatalf("expected one dependency for this chart, but got %d", len(c.Dependencies()))
+	}
+
+	if err := processDependencyEnabled(c, c.Values, ""); err != nil {
+		t.Fatalf("expected no errors but got %q", err)
+	}
+
+	if len(c.Dependencies()) != 2 {
+		t.Fatal("expected two dependencies after processing aliases")
+	}
+	validateDependencyTree(t, c)
+}
diff --git a/pkg/chart/v2/util/testdata/chart-with-dependency-aliased-twice/Chart.yaml b/pkg/chart/v2/util/testdata/chart-with-dependency-aliased-twice/Chart.yaml
new file mode 100644
index 000000000..d778f8fe9
--- /dev/null
+++ b/pkg/chart/v2/util/testdata/chart-with-dependency-aliased-twice/Chart.yaml
@@ -0,0 +1,14 @@
+apiVersion: v2
+appVersion: 1.0.0
+name: chart-with-dependency-aliased-twice
+type: application
+version: 1.0.0
+
+dependencies:
+  - name: child
+    alias: foo
+    version: 1.0.0
+  - name: child
+    alias: bar
+    version: 1.0.0
+
diff --git a/pkg/chart/v2/util/testdata/chart-with-dependency-aliased-twice/charts/child/Chart.yaml b/pkg/chart/v2/util/testdata/chart-with-dependency-aliased-twice/charts/child/Chart.yaml
new file mode 100644
index 000000000..220fda663
--- /dev/null
+++ b/pkg/chart/v2/util/testdata/chart-with-dependency-aliased-twice/charts/child/Chart.yaml
@@ -0,0 +1,6 @@
+apiVersion: v2
+appVersion: 1.0.0
+name: child
+type: application
+version: 1.0.0
+
diff --git a/pkg/chart/v2/util/testdata/chart-with-dependency-aliased-twice/charts/child/charts/grandchild/Chart.yaml b/pkg/chart/v2/util/testdata/chart-with-dependency-aliased-twice/charts/child/charts/grandchild/Chart.yaml
new file mode 100644
index 000000000..50e620a8d
--- /dev/null
+++ b/pkg/chart/v2/util/testdata/chart-with-dependency-aliased-twice/charts/child/charts/grandchild/Chart.yaml
@@ -0,0 +1,6 @@
+apiVersion: v2
+appVersion: 1.0.0
+name: grandchild
+type: application
+version: 1.0.0
+
diff --git a/pkg/chart/v2/util/testdata/chart-with-dependency-aliased-twice/charts/child/charts/grandchild/templates/dummy.yaml b/pkg/chart/v2/util/testdata/chart-with-dependency-aliased-twice/charts/child/charts/grandchild/templates/dummy.yaml
new file mode 100644
index 000000000..1830492ef
--- /dev/null
+++ b/pkg/chart/v2/util/testdata/chart-with-dependency-aliased-twice/charts/child/charts/grandchild/templates/dummy.yaml
@@ -0,0 +1,7 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: {{ .Chart.Name }}-{{ .Values.from }}
+data:
+  {{- toYaml .Values | nindent 2 }}
+
diff --git a/pkg/chart/v2/util/testdata/chart-with-dependency-aliased-twice/charts/child/templates/dummy.yaml b/pkg/chart/v2/util/testdata/chart-with-dependency-aliased-twice/charts/child/templates/dummy.yaml
new file mode 100644
index 000000000..b5d55af7c
--- /dev/null
+++ b/pkg/chart/v2/util/testdata/chart-with-dependency-aliased-twice/charts/child/templates/dummy.yaml
@@ -0,0 +1,7 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: {{ .Chart.Name }}
+data:
+  {{- toYaml .Values | nindent 2 }}
+
diff --git a/pkg/chart/v2/util/testdata/chart-with-dependency-aliased-twice/values.yaml b/pkg/chart/v2/util/testdata/chart-with-dependency-aliased-twice/values.yaml
new file mode 100644
index 000000000..695521a4a
--- /dev/null
+++ b/pkg/chart/v2/util/testdata/chart-with-dependency-aliased-twice/values.yaml
@@ -0,0 +1,7 @@
+foo:
+  grandchild:
+    from: foo
+bar:
+  grandchild:
+    from: bar
+
diff --git a/pkg/chart/v2/util/testdata/chart-with-import-from-aliased-dependencies/Chart.yaml b/pkg/chart/v2/util/testdata/chart-with-import-from-aliased-dependencies/Chart.yaml
new file mode 100644
index 000000000..c408f0ca8
--- /dev/null
+++ b/pkg/chart/v2/util/testdata/chart-with-import-from-aliased-dependencies/Chart.yaml
@@ -0,0 +1,20 @@
+apiVersion: v2
+appVersion: 1.0.0
+name: chart-with-dependency-aliased-twice
+type: application
+version: 1.0.0
+
+dependencies:
+  - name: child
+    alias: foo
+    version: 1.0.0
+    import-values:
+      - parent: foo-defaults
+        child: defaults
+  - name: child
+    alias: bar
+    version: 1.0.0
+    import-values:
+      - parent: bar-defaults
+        child: defaults
+
diff --git a/pkg/chart/v2/util/testdata/chart-with-import-from-aliased-dependencies/charts/child/Chart.yaml b/pkg/chart/v2/util/testdata/chart-with-import-from-aliased-dependencies/charts/child/Chart.yaml
new file mode 100644
index 000000000..ecdaf04dc
--- /dev/null
+++ b/pkg/chart/v2/util/testdata/chart-with-import-from-aliased-dependencies/charts/child/Chart.yaml
@@ -0,0 +1,12 @@
+apiVersion: v2
+appVersion: 1.0.0
+name: child
+type: application
+version: 1.0.0
+
+dependencies:
+  - name: grandchild
+    version: 1.0.0
+    import-values:
+      - parent: defaults
+        child: defaults
diff --git a/pkg/chart/v2/util/testdata/chart-with-import-from-aliased-dependencies/charts/child/charts/grandchild/Chart.yaml b/pkg/chart/v2/util/testdata/chart-with-import-from-aliased-dependencies/charts/child/charts/grandchild/Chart.yaml
new file mode 100644
index 000000000..50e620a8d
--- /dev/null
+++ b/pkg/chart/v2/util/testdata/chart-with-import-from-aliased-dependencies/charts/child/charts/grandchild/Chart.yaml
@@ -0,0 +1,6 @@
+apiVersion: v2
+appVersion: 1.0.0
+name: grandchild
+type: application
+version: 1.0.0
+
diff --git a/pkg/chart/v2/util/testdata/chart-with-import-from-aliased-dependencies/charts/child/charts/grandchild/values.yaml b/pkg/chart/v2/util/testdata/chart-with-import-from-aliased-dependencies/charts/child/charts/grandchild/values.yaml
new file mode 100644
index 000000000..f51c594f4
--- /dev/null
+++ b/pkg/chart/v2/util/testdata/chart-with-import-from-aliased-dependencies/charts/child/charts/grandchild/values.yaml
@@ -0,0 +1,2 @@
+defaults:
+  defaultValue: "42"
\ No newline at end of file
diff --git a/pkg/chart/v2/util/testdata/chart-with-import-from-aliased-dependencies/charts/child/templates/dummy.yaml b/pkg/chart/v2/util/testdata/chart-with-import-from-aliased-dependencies/charts/child/templates/dummy.yaml
new file mode 100644
index 000000000..3140f53dd
--- /dev/null
+++ b/pkg/chart/v2/util/testdata/chart-with-import-from-aliased-dependencies/charts/child/templates/dummy.yaml
@@ -0,0 +1,7 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: {{ .Chart.Name }}
+data:
+  {{ .Values.defaults | toYaml }}
+
diff --git a/pkg/chart/v2/util/testdata/chart-with-import-from-aliased-dependencies/templates/dummy.yaml b/pkg/chart/v2/util/testdata/chart-with-import-from-aliased-dependencies/templates/dummy.yaml
new file mode 100644
index 000000000..a2b62c95a
--- /dev/null
+++ b/pkg/chart/v2/util/testdata/chart-with-import-from-aliased-dependencies/templates/dummy.yaml
@@ -0,0 +1,7 @@
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: {{ .Chart.Name }}
+data:
+  {{ toYaml .Values.defaults | indent 2 }}
+