From 18529ab7c550bfa6d6eefddeb47418f1685f4a7f Mon Sep 17 00:00:00 2001 From: yxxhero <11087727+yxxhero@users.noreply.github.com> Date: Thu, 12 Sep 2024 20:26:45 +0800 Subject: [PATCH] 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 --- pkg/state/chart_dependency.go | 69 +++---- pkg/state/chart_dependency_test.go | 181 ++++++++++-------- test/integration/test-cases/regression.sh | 10 + .../regression/input/issue.1682.yaml | 13 ++ 4 files changed, 151 insertions(+), 122 deletions(-) create mode 100644 test/integration/test-cases/regression/input/issue.1682.yaml diff --git a/pkg/state/chart_dependency.go b/pkg/state/chart_dependency.go index 5ed5a668..82cb7846 100644 --- 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) { diff --git a/pkg/state/chart_dependency_test.go b/pkg/state/chart_dependency_test.go index 9ad33715..23c97c5c 100644 --- 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) + } } } diff --git a/test/integration/test-cases/regression.sh b/test/integration/test-cases/regression.sh index 5028c9c4..8a68d491 100644 --- 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" \ No newline at end of file diff --git a/test/integration/test-cases/regression/input/issue.1682.yaml b/test/integration/test-cases/regression/input/issue.1682.yaml new file mode 100644 index 00000000..3ae6ec01 --- /dev/null +++ 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 \ No newline at end of file