Fix subcharts dependencies processing by vflaux · Pull Request #10174 · helm/helm

vflaux · 2021-09-22T14:32:45Z

What this PR does / why we need it:

There is an issue when you use a subchart more than once with alias and that this subchart also have a subchart.

In pkg/chartutil/dependencies.go, getAliasDependency() return a partial copy of the passed chart keeping the same slice for metadata.Dependencies. This results in metadata.Dependencies been shared by multiple Charts objects.
At some point processDependencyEnabled() changes the name of each dependencies by their alias. This impact others dependencies generated by getAliasDependency() and leads to processDependencyEnabled() to not correctly handles their dependencies (as it use the name to do so and the name is now the alias).

This patch make a copy of all the dependencies in a new slice to avoid this issue.

fix #10160

Special notes for your reviewer:

If applicable:

this PR contains unit tests
this PR has been tested for backwards compatibility

bacongobbler · 2021-09-22T14:41:31Z

Please provide unit tests to ensure your change fixes the issue. Thanks.

bacongobbler · 2021-09-22T14:44:52Z

pkg/chartutil/dependencies.go

 		md := *c.Metadata
+		deps := make([]*chart.Dependency, 0, len(md.Dependencies))
+		for _, dep := range md.Dependencies {
+			dep := *dep


Why is this de-referencing the address, then re-assigning a new pointer? Wouldn't a copy be safer?

What happens when you provide a list of nil dependencies? Wouldn't that result in a stack trace when you attempt to de-reference nil? How should this code handle nil dependencies?

A copy would not give us new Dependency structs.
We need new ones because their name are modified here which leaks on other Charts:

helm/pkg/chartutil/dependencies.go

Lines 139 to 146 in 9fafb4a

for _, req := range c.Metadata.Dependencies {

if chartDependency := getAliasDependency(c.Dependencies(), req); chartDependency != nil {

chartDependencies = append(chartDependencies, chartDependency)

}

if req.Alias != "" {

req.Name = req.Alias

}

}

When you call getAliasDependency(c.Dependencies(), req) multiple time with the same req.Name, you get new Charts structs each time but their metadata.Dependencies are the same slice which come from the Chart in c.Dependencies() that match the Dependency (req).
Then we iterate over these dependencies (*chart.Chart) and call processDependencyEnabled()

helm/pkg/chartutil/dependencies.go

Lines 186 to 191 in 9fafb4a

for _, t := range cd {

subpath := path + t.Metadata.Name + "."

if err := processDependencyEnabled(t, cvals, subpath); err != nil {

return err

}

}

At this point the dependencies generated by getAliasDependency are now the pointer c.
Then we iterate over c.Metadata.Dependencies (and call again getAliasDependency) then replace req.Name with req.Alias. But c.Metadata.Dependencies is the slice return by getAliasDependency in the parent, which may be shared with another Chart as seen before.
When the processDependencyEnabled is called with such a Chart (same name but different alias), Dependency structs in c.Metadata.Dependencies have already their name changed to the alias and the following match do not work as expected:

helm/pkg/chartutil/dependencies.go

Lines 130 to 133 in 9fafb4a

for _, existing := range c.Dependencies() {

for _, req := range c.Metadata.Dependencies {

if existing.Name() == req.Name && IsCompatibleRange(req.Version, existing.Metadata.Version) {

continue Loop

The only Chart dependencies that are kept are the one in c.Dependencies, not in c.Metadata.Dependencies.

With a new slice and new structs for .metadata.Dependencies, the req.Name = req.Alias do not leaks on other Charts.

Not sure if I'm clear enough here, sorry 😕.

Charts.Dependencies are apparently not supposed to contains nil pointers as this line would panic on the req.Name in this case:

helm/pkg/chartutil/dependencies.go

Lines 130 to 132 in 9fafb4a

for _, existing := range c.Dependencies() {

for _, req := range c.Metadata.Dependencies {

if existing.Name() == req.Name && IsCompatibleRange(req.Version, existing.Metadata.Version) {

But I can add checks for nil if you prefer.

Signed-off-by: Valentin Flaux <vflaux@oui.sncf>

vflaux · 2021-09-22T23:55:18Z

Just find that there is another PR for the same issue and with the same fix: #9175

imported values are stored in dependency objects, which breaks if a chart dependency is shared among multiple aliases. By copying the dependency objects in the metadata values can be imported correctly. Supersedes helm#10174 Signed-off-by: Daniel Strobusch <1847260+dastrobu@users.noreply.github.com>

github-actions · 2025-08-26T00:23:03Z

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

imported values are stored in dependency objects, which breaks if a chart dependency is shared among multiple aliases. By copying the dependency objects in the metadata values can be imported correctly. Supersedes helm#10174 Signed-off-by: Daniel Strobusch <1847260+dastrobu@users.noreply.github.com>

helm-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 22, 2021

vflaux force-pushed the fix/10160 branch from 4bee23f to 7e21009 Compare September 22, 2021 14:35

bacongobbler reviewed Sep 22, 2021

View reviewed changes

Fix subcharts dependencies processing

437a08b

Signed-off-by: Valentin Flaux <vflaux@oui.sncf>

vflaux force-pushed the fix/10160 branch from 7e21009 to 437a08b Compare September 22, 2021 23:10

helm-bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 22, 2021

mederel mentioned this pull request May 2, 2022

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

Closed

github-actions bot added the Stale label Aug 26, 2025

github-actions bot closed this Sep 27, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix subcharts dependencies processing#10174

Fix subcharts dependencies processing#10174
vflaux wants to merge 1 commit intohelm:mainfrom
vflaux:fix/10160

vflaux commented Sep 22, 2021 •

edited

Loading

Uh oh!

bacongobbler commented Sep 22, 2021

Uh oh!

bacongobbler Sep 22, 2021

Uh oh!

vflaux Sep 22, 2021

Uh oh!

vflaux commented Sep 22, 2021

Uh oh!

github-actions bot commented Aug 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	for _, req := range c.Metadata.Dependencies {
	if chartDependency := getAliasDependency(c.Dependencies(), req); chartDependency != nil {
	chartDependencies = append(chartDependencies, chartDependency)
	}
	if req.Alias != "" {
	req.Name = req.Alias
	}
	}

	for _, t := range cd {
	subpath := path + t.Metadata.Name + "."
	if err := processDependencyEnabled(t, cvals, subpath); err != nil {
	return err
	}
	}

	for _, existing := range c.Dependencies() {
	for _, req := range c.Metadata.Dependencies {
	if existing.Name() == req.Name && IsCompatibleRange(req.Version, existing.Metadata.Version) {
	continue Loop

Conversation

vflaux commented Sep 22, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bacongobbler commented Sep 22, 2021

Uh oh!

bacongobbler Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

vflaux Sep 22, 2021

Choose a reason for hiding this comment

Uh oh!

vflaux commented Sep 22, 2021

Uh oh!

github-actions bot commented Aug 26, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

vflaux commented Sep 22, 2021 •

edited

Loading