sub-sub-charts of aliased sub-charts not evaluated correctly

[dastrobu]

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

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"}

[sbose78]

Hi @dastrobu, thank you for reporting this, I've been able to reproduce this issue!

In fact, I've tried aliasing further and I still see only one grandchild being returned.

foo:
  grandchild:
    from: foo

bar:
  grandchild:
    from: bar

another:
  grandchild:
    from: another

[mattfarina]

@dastrobu Thanks for providing an easy way to reproduce this situation.

[dastrobu]

Took a look at the code. I think the issue is in

helm/pkg/chart/chart.go

Line 85 in 59d5b94

charts[i].parent = ch

where the parent is set to the sub-chart. On loading the charts, the grandchild chart is loaded only once and the reference to the chart is shared between the two alias charts foo and bar, see

helm/pkg/chart/loader/load.go

Line 196 in a374fff

c.AddDependency(sc)

So the last loaded sub-chart overrides the parent of grandchild and only this one will be rendered in

helm/pkg/engine/engine.go

Line 358 in 3192408

newParentID := c.ChartFullPath()

There might be several ways to fix this. If I have a good idea how, I'll provide a PR.

[dastrobu]

Fixing this seems to be harder than I expected. My first thought was to copy dependencies on resolving aliases, see https://github.com/dastrobu/helm/blob/72fb27a4b0d146d5a14cc24b3c8df18da993deec/pkg/chartutil/dependencies.go#L112
However, this has the side effect that ChartFullPath of the dependencies will no longer return the file path of the referenced file (e.g. parent/charts/child/charts/grandchild/templates/dummy.yaml), instead it will contain the alias name (e.g. parent/charts/foo/charts/grandchild/templates/dummy.yaml), which is not a file path that can be resolved.

So I thought about keeping the chart data structures with the shared reference as is and don't rely on the ChartFullPath to create a parent id in

helm/pkg/engine/engine.go

Line 358 in 3192408

newParentID := c.ChartFullPath()

.
It is relatively easy to create an id which respects the aliases without relying on the ChartFullPath. The issue is, when returning a map with keys other than file names from

helm/pkg/engine/engine.go

Line 330 in 3192408

return templates

, this would also require to change keys of the rendered map in

helm/pkg/engine/engine.go

Line 260 in 3192408

return rendered, nil

(since there may be more than one rendering of a given template file).
There are multiple places relying on file paths as keys, see

helm/pkg/lint/rules/template.go

Line 118 in dfb5a5e

renderedContent := renderedContentMap[path.Join(chart.Name(), fileName)]

,

helm/pkg/action/action.go

Line 148 in ed5fba5

for k, v := range files {

ff.

In general I guess it would make sense to separate ids for a template instance and file paths pointing to the template definition. Getting this right seems to be a bit more work than I expected. I could try to move on with this approach if this is the right direction.

[dastrobu]

as it turns out there are a number of issue with aliases as subcharts: #2993, #3457, #3909, #4300, #4390, #7061, #7093, #7413, #7729. Not heaving read through all of them I assume some of them are related to the code analysis above.
Another idea to solve this would be to store charts under the alias name when downloaded, as proposed in #7413 (comment). That would make the dependency handling in Helm a bit easier. On the other side it would be a breaking change and many packed charts could break on implementing this change. Additionally, if someone maintains an umbrella chart with an unpacked subchart which he would like to instantiate several times via aliases, it would be required to copy the entire umpacked chart, which is not ideal.
So I guess we should keep the structure helm stores charts and handle aliases correctly.
For Helm 4, though, it should be considered to change the way charts are stored to handle name clashes from charts of different repos, references to the same chart but different versions and aliases to reference the same chart more than once.

[dastrobu]

after digging through the code I think using the alias names for ChartFullPath is the best we can do, currently. When looking at the # Source comments of the current Helm output we see that the situation is a bit inconsistent:

---
# Source: parent/charts/bar/templates/dummy.yaml
...
---
# Source: parent/charts/child/charts/grandchild/templates/dummy.yaml
...
---
# Source: parent/charts/foo/templates/dummy.yaml
...

While the child level paths are constructed from alias names .../foo/... and .../bar/..., on the grandchild level, no alias names are employed: .../child/.... In general I would prefer if source paths would show the actual paths where I could find the files on my file system (which is not the case for the foo and bar paths). Since this would require quite some changes to the code base, I suggest to use alias names consistently in all paths for now and document this behaviour.

On the long run it might be a good idea to have a clear separation between names and aliases in the code base, so that it is always clear what the path to a chart on the file system is, and what a path in the dependency tree ist. Mixing names and aliases by e.g. simply overriding the names of charts on processing dependencies does not seem to be a good idea from my point of view:

helm/pkg/chartutil/dependencies.go

Line 118 in 6a441a6

if dep.Alias != "" {

helm/pkg/chartutil/dependencies.go

Line 152 in 6a441a6

if req.Alias != "" {

[dastrobu]

@mattfarina any chance we get this on the 3.5.1 milestone?
Bug was described, code analysed and fix proposed. As you can see from the PR's comments I am not the only one affected by this issue. Without review from the maintainers, I think there is not much the community can do here anymore.

[dastrobu]

@sbose78 and @mattfarina there is an open PR (#9175) on this issue open for a few months. Anything I need to do, to get someone to review this PR?

[github-actions]

This issue has been marked as stale because it has been open for 90 days with no activity. This thread will be automatically closed in 30 days if no further activity occurs.

[dastrobu]

remove "Stale".

PR still waiting for review....

[github-actions]

This issue has been marked as stale because it has been open for 90 days with no activity. This thread will be automatically closed in 30 days if no further activity occurs.

[dastrobu]

remove "Stale".

PR still waiting for review....

[github-actions]

This issue has been marked as stale because it has been open for 90 days with no activity. This thread will be automatically closed in 30 days if no further activity occurs.

[dastrobu]

remove "Stale".

PR still waiting for review....

[github-actions]

This issue has been marked as stale because it has been open for 90 days with no activity. This thread will be automatically closed in 30 days if no further activity occurs.

[msadish]

Any chances this bug could get fixed with v3.10.0 ? its becoming headache with this issue. any one have workaround to avoid this ?

[brunocascio]

Is there any plan to include it soon?
I'm facing this issue too :(

[dastrobu]

would be great to see this in 3.11.0, as 3.10.3. was just released...

[bo0ts]

Any chance this is ever getting merged or is there a workaround?