Fix HelmChart local dependency resolution for name-based path

[matheuscscp]

Fixes fluxcd/flux2#4368

[souleb]

I believe, it was intended to be used with file:// to define the local repository location, see https://helm.sh/docs/helm/helm_dependency/

e.g.

dependencies:
  - name: helmchart
    alias: aliased
    version: "0.1.0"
    repository: "file://../helmchart"

Now, it's true that documentation is missing, but should we introduce an arbitrary default location?

[matheuscscp]

I believe, it was intended to be used with file:// to define the local repository location, see https://helm.sh/docs/helm/helm_dependency/

e.g.

dependencies:
  - name: helmchart
    alias: aliased
    version: "0.1.0"
    repository: "file://../helmchart"

Now, it's true that documentation is missing, but should we introduce an arbitrary default location?

@souleb it's not arbitrary, it's exactly what Helm itself does:

https://github.com/helm/helm/blob/588041f6a55a8f23113f3c080d1733e65176fa0a/pkg/downloader/manager.go#L275-L278

[stefanprodan]

LGTM

Thanks @matheuscscp 🥇

[souleb]

LGTM! Thanks for fixing this @matheuscscp

[darkowlzz]

Thanks for the findings and the fix.

I would like to add some context based on my testing and understanding of the bug and the observations from fluxcd/flux2#4368 (comment) for reasoning behind the duplicate resources. And also suggest adding more test coverage for this special case of empty dependency repository field.

https://helm.sh/docs/helm/helm_dependency/ doesn't talk about the expected behavior when the dependency repository is left empty but the best practices docs https://helm.sh/docs/chart_best_practices/dependencies/#repository-urls mentions about it

Helm cannot perform dependency management operations on the dependency when the repository field is left blank. In that case Helm will assume the dependency is in a sub-directory of the charts folder with the name being the same as the name property for the dependency.

I think we need to cover this scenario for local dependencies in our tests. I'll share a diff of what I tried below which can be adapted to include in this change.

The source for duplicate resources seems to be due to parsing of empty repository as a URL in https://github.com/fluxcd/source-controller/blob/v1.3.0/internal/helm/chart/dependency_manager.go#L299. Empty URL parsing doesn't return any error, due to which, secureLocalChartPath() continues to execute and return the main chart path itself as the local chart path.

	return securejoin.SecureJoin(ref.WorkDir, filepath.Join(ref.Path, localUrl.Host, localUrl.Path))

In the above, localUrl being empty, ref.WorkDir and ref.Path are joined and returned.

addLocalDependency() takes the path and loads the main chart itself.

In the test chart repository chart at https://github.com/kgreczka-iteo/flux-fail/blob/main/chart-not-working/Chart.yaml, the main chart is loaded first, then for every dependency, the main chart is loaded again because the secureLocalChartPath() returned the main chart's path to load due to empty repository value, resulting in 3 copies of the same chart with duplicate resources.

I don't see upstream helm preventing charts from importing themselves as their dependency. So it may be fine to not prevent this behavior.

I made the following changes while testing. The diff doesn't include the testdata changes as they are a lot. Since we don't have testdata for such charts with charts/ subdirectory, I added new testdata chart based on internal/helm/testdata/charts/helmchartwithdeps, renamed it to helmchartwithdepsnorepo and copied the simple chart internal/helm/testdata/charts/helmchart into the charts/ directory. Deleted the repository entries from Chart.yaml and also grafana which is not relevant here.
The two new test cases check for success and failure scenarios for such empty repository dependencies. I believe this covers the core issue and would help prevent any regression in the future.

Click to see test changes

diff --git a/internal/helm/chart/dependency_manager_test.go b/internal/helm/chart/dependency_manager_test.go
index 457d6c6..241959f 100644
--- a/internal/helm/chart/dependency_manager_test.go
+++ b/internal/helm/chart/dependency_manager_test.go
@@ -290,13 +290,15 @@ func TestDependencyManager_build(t *testing.T) {
 
 func TestDependencyManager_addLocalDependency(t *testing.T) {
        tests := []struct {
-               name     string
-               dep      *helmchart.Dependency
-               wantErr  string
-               wantFunc func(g *WithT, c *helmchart.Chart)
+               name      string
+               chartName string
+               dep       *helmchart.Dependency
+               wantErr   string
+               wantFunc  func(g *WithT, c *helmchart.Chart)
        }{
                {
-                       name: "local dependency",
+                       name:      "local dependency",
+                       chartName: "helmchartwithdeps",
                        dep: &helmchart.Dependency{
                                Name:       chartName,
                                Version:    chartVersion,
@@ -307,7 +309,8 @@ func TestDependencyManager_addLocalDependency(t *testing.T) {
                        },
                },
                {
-                       name: "version not matching constraint",
+                       name:      "version not matching constraint",
+                       chartName: "helmchartwithdeps",
                        dep: &helmchart.Dependency{
                                Name:       chartName,
                                Version:    "0.2.0",
@@ -316,7 +319,8 @@ func TestDependencyManager_addLocalDependency(t *testing.T) {
                        wantErr: "can't get a valid version for constraint '0.2.0'",
                },
                {
-                       name: "invalid local reference",
+                       name:      "invalid local reference",
+                       chartName: "helmchartwithdeps",
                        dep: &helmchart.Dependency{
                                Name:       chartName,
                                Version:    chartVersion,
@@ -325,7 +329,8 @@ func TestDependencyManager_addLocalDependency(t *testing.T) {
                        wantErr: "no chart found at '/absolutely/invalid'",
                },
                {
-                       name: "invalid chart archive",
+                       name:      "invalid chart archive",
+                       chartName: "helmchartwithdeps",
                        dep: &helmchart.Dependency{
                                Name:       chartName,
                                Version:    chartVersion,
@@ -334,7 +339,8 @@ func TestDependencyManager_addLocalDependency(t *testing.T) {
                        wantErr: "failed to load chart from '/empty.tgz'",
                },
                {
-                       name: "invalid constraint",
+                       name:      "invalid constraint",
+                       chartName: "helmchartwithdeps",
                        dep: &helmchart.Dependency{
                                Name:       chartName,
                                Version:    "invalid",
@@ -342,6 +348,26 @@ func TestDependencyManager_addLocalDependency(t *testing.T) {
                        },
                        wantErr: "invalid version/constraint format 'invalid'",
                },
+               {
+                       name:      "no repository",
+                       chartName: "helmchartwithdepsnorepo",
+                       dep: &helmchart.Dependency{
+                               Name:    chartName,
+                               Version: chartVersion,
+                       },
+                       wantFunc: func(g *WithT, c *helmchart.Chart) {
+                               g.Expect(c.Dependencies()).To(HaveLen(1))
+                       },
+               },
+               {
+                       name:      "no repository invalid reference",
+                       chartName: "helmchartwithdepsnorepo",
+                       dep: &helmchart.Dependency{
+                               Name:    "nonexistingchart",
+                               Version: chartVersion,
+                       },
+                       wantErr: "no chart found at '/helmchartwithdepsnorepo/charts/nonexistingchart'",
+               },
        }
        for _, tt := range tests {
                t.Run(tt.name, func(t *testing.T) {
@@ -353,7 +379,7 @@ func TestDependencyManager_addLocalDependency(t *testing.T) {
                        absWorkDir, err := filepath.Abs("../testdata/charts")
                        g.Expect(err).ToNot(HaveOccurred())
 
-                       err = dm.addLocalDependency(LocalReference{WorkDir: absWorkDir, Path: "helmchartwithdeps"},
+                       err = dm.addLocalDependency(LocalReference{WorkDir: absWorkDir, Path: tt.chartName},
                                &chartWithLock{Chart: chart}, tt.dep)
                        if tt.wantErr != "" {
                                g.Expect(err).To(HaveOccurred())

[matheuscscp]

Thanks for the thorough explanation of the bug @darkowlzz 👍

I will add the test cases tomorrow 👌

[darkowlzz]

LGTM!
Thanks.

[fluxcdbot]

Successfully created backport PR for release/v1.3.x:

• [release/v1.3.x] Fix HelmChart local dependency resolution for name-based path #1542