fix: two releases using the same chart in different version fails (#1685)

* fix: two releases using the same chart in different version fails Signed-off-by: yxxhero <aiopsclub@163.com>
2024-09-12 20:26:45 +08:00 · 2024-09-12 20:26:45 +08:00 · 18529ab7c5
parent 65f4e6122a
commit 18529ab7c5
4 changed files with 151 additions and 122 deletions
--- a/pkg/state/chart_dependency.go
+++ b/pkg/state/chart_dependency.go
@ -19,6 +19,10 @@ type ChartMeta struct {
 	Name string `yaml:"name"`
 }

+// unresolvedChartDependency represents a dependency that is to be resolved.
+//
+// Helmfile generates Helm Chart.yaml containing unresolved dependencies, and runs `helm dependency update` to produce Helm Chart.lock
+// which becomes helmfile.lock, which is then used to resolve the dependencies.
 type unresolvedChartDependency struct {
 	// ChartName identifies the dependant chart. In Helmfile, ChartName for `chart: stable/envoy` would be just `envoy`.
 	// It can't be collided with other charts referenced in the same helmfile spec.
@ -28,6 +32,10 @@ type unresolvedChartDependency struct {
 	Repository string `yaml:"repository"`
 	// VersionConstraint is the version constraint of the dependent chart. "*" means the latest version.
 	VersionConstraint string `yaml:"version"`
+	// Alias differentiates multiple dependencies with the same ChartName.
+	// Despite its name, and its optional in Helm's Chart.yaml, we use this as a unique identifier for the dependency.
+	// So, every dependency have an alias, even if it's not explicitly set in the helmfile.
+	Alias string `yaml:"alias"`
 }

 type ResolvedChartDependency struct {
@ -57,43 +65,13 @@ type ChartLockedRequirements struct {
 	Generated            string                    `yaml:"generated"`
 }

-func (d *UnresolvedDependencies) Add(chart, url, versionConstraint string) error {
-	dep := unresolvedChartDependency{
+func (d *UnresolvedDependencies) Add(chart, url, versionConstraint, alias string) {
+	d.deps[chart] = append(d.deps[chart], unresolvedChartDependency{
 		ChartName:         chart,
 		Repository:        url,
 		VersionConstraint: versionConstraint,
-	}
-	if !d.contains(dep) {
-		return d.add(dep)
-	}
-	return nil
-}
-
-func (d *UnresolvedDependencies) add(dep unresolvedChartDependency) error {
-	deps := d.deps[dep.ChartName]
-	if deps == nil {
-		deps = []unresolvedChartDependency{dep}
-	} else {
-		deps = append(deps, dep)
-	}
-	d.deps[dep.ChartName] = deps
-	return nil
-}
-
-// contains checks if the UnresolvedDependencies contains the specified unresolvedChartDependency.
-// It returns true if the dependency is found, otherwise it returns false.
-// fix 'more than one dependency with name or alias "raw"' error since helm v3.14.0
-func (d *UnresolvedDependencies) contains(dep unresolvedChartDependency) bool {
-	deps := d.deps[dep.ChartName]
-	if deps == nil {
-		return false
-	}
-	for _, existDep := range deps {
-		if existDep.ChartName == dep.ChartName {
-			return true
-		}
-	}
-	return false
+		Alias:             alias,
+	})
 }

 func (d *UnresolvedDependencies) ToChartRequirements() *ChartRequirements {
@ -152,10 +130,7 @@ func (d *ResolvedDependencies) Get(chart, versionConstraint string) (string, err
 }

 func (st *HelmState) mergeLockedDependencies() (*HelmState, error) {
-	filename, unresolved, err := getUnresolvedDependenciess(st)
-	if err != nil {
-		return nil, err
-	}
+	filename, unresolved := getUnresolvedDependenciess(st)

 	if len(unresolved.deps) == 0 {
 		return st, nil
@ -211,10 +186,7 @@ func resolveDependencies(st *HelmState, depMan *chartDependencyManager, unresolv
 }

 func (st *HelmState) updateDependenciesInTempDir(shell helmexec.DependencyUpdater, tempDir func(string, string) (string, error)) (*HelmState, error) {
-	filename, unresolved, err := getUnresolvedDependenciess(st)
-	if err != nil {
-		return nil, err
-	}
+	filename, unresolved := getUnresolvedDependenciess(st)

 	if len(unresolved.deps) == 0 {
 		st.logger.Warnf("There are no repositories defined in your helmfile.yaml.\nThis means helmfile cannot update your dependencies or create a lock file.\nSee https://github.com/roboll/helmfile/issues/878 for more information.")
@ -232,7 +204,12 @@ func (st *HelmState) updateDependenciesInTempDir(shell helmexec.DependencyUpdate
 	return updateDependencies(st, shell, unresolved, filename, d)
 }

-func getUnresolvedDependenciess(st *HelmState) (string, *UnresolvedDependencies, error) {
+// aliasNameFormat = regexp.MustCompile("^[a-zA-Z0-9_-]+$") from helm code
+func chartDependenciesAlias(namespace, releaseName string) string {
+	return fmt.Sprintf("%s-%s", namespace, releaseName)
+}
+
+func getUnresolvedDependenciess(st *HelmState) (string, *UnresolvedDependencies) {
 	repoToURL := map[string]RepositorySpec{}

 	for _, r := range st.Repositories {
@ -260,9 +237,7 @@ func getUnresolvedDependenciess(st *HelmState) (string, *UnresolvedDependencies,
 			url = fmt.Sprintf("oci://%s", url)
 		}

-		if err := unresolved.Add(chart, url, r.Version); err != nil {
-			return "", nil, err
-		}
+		unresolved.Add(chart, url, r.Version, chartDependenciesAlias(r.Namespace, r.Name))
 	}

 	filename := filepath.Base(st.FilePath)
@ -270,7 +245,7 @@ func getUnresolvedDependenciess(st *HelmState) (string, *UnresolvedDependencies,
 	filename = strings.TrimSuffix(filename, ".yaml")
 	filename = strings.TrimSuffix(filename, ".yml")

-	return filename, unresolved, nil
+	return filename, unresolved
 }

 func updateDependencies(st *HelmState, shell helmexec.DependencyUpdater, unresolved *UnresolvedDependencies, filename, wd string) (*HelmState, error) {
--- a/pkg/state/chart_dependency_test.go
+++ b/pkg/state/chart_dependency_test.go
@ -10,7 +10,6 @@ func TestGetUnresolvedDependenciess(t *testing.T) {
 	tests := []struct {
 		name       string
 		helmState  *HelmState
-		wantErr    bool
 		expectfile string
 		expectDeps *UnresolvedDependencies
 	}{
@ -21,21 +20,34 @@ func TestGetUnresolvedDependenciess(t *testing.T) {
 				ReleaseSetSpec: ReleaseSetSpec{
 					Releases: []ReleaseSpec{
 						{
-							Name:    "foo",
-							Chart:   "charts/abc",
-							Version: "0.1.0",
+							Name:      "foo",
+							Chart:     "chartsa/abc",
+							Version:   "0.1.0",
+							Namespace: "ns1",
+						},
+						{
+							Name:      "empty",
+							Chart:     "chartsb/empty",
+							Namespace: "ns2",
+						},
+						{
+							Name:  "empty",
+							Chart: "chartsb/empty",
 						},
 					},
 					Repositories: []RepositorySpec{
 						{
-							Name: "charts",
+							Name: "chartsa",
 							URL:  "localhost:5000/aaa",
 							OCI:  true,
 						},
+						{
+							Name: "chartsb",
+							URL:  "localhost:5000/bbb",
+						},
 					},
 				},
 			},
-			wantErr:    false,
 			expectfile: "helmfile",
 			expectDeps: &UnresolvedDependencies{
 				deps: map[string][]unresolvedChartDependency{
@ -44,6 +56,77 @@ func TestGetUnresolvedDependenciess(t *testing.T) {
 							ChartName:         "abc",
 							Repository:        "oci://localhost:5000/aaa",
 							VersionConstraint: "0.1.0",
+							Alias:             "ns1-foo",
+						},
+					},
+					"empty": {
+						{
+							ChartName:  "empty",
+							Repository: "localhost:5000/bbb",
+							Alias:      "ns2-empty",
+						},
+						{
+							ChartName:  "empty",
+							Repository: "localhost:5000/bbb",
+							Alias:      "-empty",
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "duplicate charts are differentiated by alias",
+			helmState: &HelmState{
+				FilePath: "helmfile.yaml",
+				ReleaseSetSpec: ReleaseSetSpec{
+					Releases: []ReleaseSpec{
+						{
+							Name:      "foo",
+							Chart:     "myrepo/abc",
+							Version:   "> 0.2.0",
+							Namespace: "ns1",
+						},
+						{
+							Name:      "bar",
+							Chart:     "myrepo/abc",
+							Version:   "0.1.0",
+							Namespace: "ns2",
+						},
+						{
+							Name:    "baz",
+							Chart:   "myrepo/abc",
+							Version: "0.3.0",
+						},
+					},
+					Repositories: []RepositorySpec{
+						{
+							Name: "myrepo",
+							URL:  "localhost:5000/aaa",
+						},
+					},
+				},
+			},
+			expectfile: "helmfile",
+			expectDeps: &UnresolvedDependencies{
+				deps: map[string][]unresolvedChartDependency{
+					"abc": {
+						{
+							ChartName:         "abc",
+							Repository:        "localhost:5000/aaa",
+							VersionConstraint: "> 0.2.0",
+							Alias:             "ns1-foo",
+						},
+						{
+							ChartName:         "abc",
+							Repository:        "localhost:5000/aaa",
+							VersionConstraint: "0.1.0",
+							Alias:             "ns2-bar",
+						},
+						{
+							ChartName:         "abc",
+							Repository:        "localhost:5000/aaa",
+							VersionConstraint: "0.3.0",
+							Alias:             "-baz",
 						},
 					},
 				},
@ -53,82 +136,30 @@ func TestGetUnresolvedDependenciess(t *testing.T) {

 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			f, ds, err := getUnresolvedDependenciess(tt.helmState)
-			if tt.wantErr {
-				require.Error(t, err, "getUnresolvedDependenciess() error = nil, wantErr")
-			} else {
-				require.NoErrorf(t, err, "getUnresolvedDependenciess() want no error, got %v", err)
-			}
+			f, ds := getUnresolvedDependenciess(tt.helmState)
 			require.Equalf(t, tt.expectfile, f, "getUnresolvedDependenciess() expect file %s, got %s", tt.expectfile, f)
 			require.Equalf(t, tt.expectDeps, ds, "getUnresolvedDependenciess() expect deps %v, got %v", tt.expectDeps, ds)
 		})
 	}
 }

-func TestContains(t *testing.T) {
-	tests := []struct {
-		name     string
-		dep      unresolvedChartDependency
-		deps     map[string][]unresolvedChartDependency
-		expected bool
-	}{
-		{
-			name: "existing dependency with right item",
-			dep: unresolvedChartDependency{
-				ChartName:         "abc",
-				Repository:        "oci://localhost:5000/aaa",
-				VersionConstraint: "0.1.0",
-			},
-			deps: map[string][]unresolvedChartDependency{
-				"abc": {
-					{
-						ChartName:         "abc",
-						Repository:        "oci://localhost:5000/aaa",
-						VersionConstraint: "0.1.0",
-					},
-				},
-			},
-			expected: true,
-		},
-		{
-			name: "existing dependency with empty item",
-			dep: unresolvedChartDependency{
-				ChartName:         "ghi",
-				Repository:        "oci://localhost:5000/aaa",
-				VersionConstraint: "0.1.0",
-			},
-			deps: map[string][]unresolvedChartDependency{
-				"ghi": {},
-			},
-			expected: false,
-		},
-		{
-			name: "non-existing dependency",
-			dep: unresolvedChartDependency{
-				ChartName:         "def",
-				Repository:        "oci://localhost:5000/bbb",
-				VersionConstraint: "0.2.0",
-			},
-			deps: map[string][]unresolvedChartDependency{
-				"abc": {
-					{
-						ChartName:         "abc",
-						Repository:        "oci://localhost:5000/aaa",
-						VersionConstraint: "0.1.0",
-					},
-				},
-			},
-			expected: false,
-		},
+func TestChartDependenciesAlias(t *testing.T) {
+	type testCase struct {
+		releaseName string
+		namespace   string
+		expected    string
 	}

-	for _, tt := range tests {
-		t.Run(tt.name, func(t *testing.T) {
-			d := &UnresolvedDependencies{
-				deps: tt.deps,
-			}
-			actual := d.contains(tt.dep)
-			require.Equal(t, tt.expected, actual)
-		})
+	testCases := []testCase{
+		{"release1", "n1", "n1-release1"},
+		{"release2", "n2", "n2-release2"},
+		{"empty", "", "-empty"},
+	}
+
+	for _, tc := range testCases {
+		result := chartDependenciesAlias(tc.namespace, tc.releaseName)
+		if result != tc.expected {
+			t.Errorf("Expected %s, but got %s", tc.expected, result)
+		}
 	}
 }
--- a/test/integration/test-cases/regression.sh
+++ b/test/integration/test-cases/regression.sh
@ -34,4 +34,14 @@ if [[ ${HELMFILE_V1MODE} = true ]]; then
 fi
 (${helmfile} -f ${regression_case_input_dir}/${config_file} template 1>/dev/null) || fail "\"helmfile template\" shouldn't fail"

+info "https://github.com/helmfile/helmfile/issues/1682"
+config_file="issue.1682.yaml"
+if [[ ${HELMFILE_V1MODE} = true ]]; then
+  pushd "${regression_case_input_dir}"
+  mv "${config_file}" "${config_file}.gotmpl"
+  config_file="${config_file}.gotmpl"
+  popd
+fi
+(${helmfile} -f ${regression_case_input_dir}/${config_file} deps 1>/dev/null) || fail "\"helmfile deps\" shouldn't fail"
+
 test_pass "regression tests"
--- a/test/integration/test-cases/regression/input/issue.1682.yaml
+++ b/test/integration/test-cases/regression/input/issue.1682.yaml
@ -0,0 +1,13 @@
+repositories:
+- name: elastic
+  url: https://helm.elastic.co/
+
+releases:
+- name: elasticsearch7
+  chart: elastic/elasticsearch
+  version: 7.17.3
+- name: elasticsearch8
+  chart: elastic/elasticsearch
+  version: 8.5.1
+- name: elasticsearchempty
+  chart: elastic/elasticsearch