diff --git a/internal/chart/v3/util/save.go b/internal/chart/v3/util/save.go index f886c6175..e08386e6d 100644 --- a/internal/chart/v3/util/save.go +++ b/internal/chart/v3/util/save.go @@ -147,19 +147,21 @@ func Save(c *chart.Chart, outDir string) (string, error) { } }() - if err := writeTarContents(twriter, c, ""); err != nil { + if err := writeTarContents(twriter, c, "", c.Name()); err != nil { rollback = true return filename, err } return filename, nil } -func writeTarContents(out *tar.Writer, c *chart.Chart, prefix string) error { - err := validateName(c.Name()) - if err != nil { +func writeTarContents(out *tar.Writer, c *chart.Chart, prefix, baseName string) error { + if err := validateName(c.Name()); err != nil { + return err + } + if err := validateName(baseName); err != nil { return err } - base := filepath.Join(prefix, c.Name()) + base := filepath.Join(prefix, baseName) // Save Chart.yaml cdata, err := yaml.Marshal(c.Metadata) @@ -217,14 +219,82 @@ func writeTarContents(out *tar.Writer, c *chart.Chart, prefix string) error { } // Save dependencies - for _, dep := range c.Dependencies() { - if err := writeTarContents(out, dep, filepath.Join(base, ChartsDir)); err != nil { + deps := c.Dependencies() + archiveNames := dependencyArchiveNames(c, deps) + for i, dep := range deps { + if err := writeTarContents(out, dep, filepath.Join(base, ChartsDir), archiveNames[i]); err != nil { return err } } return nil } +func dependencyArchiveNames(c *chart.Chart, deps []*chart.Chart) []string { + names := make([]string, len(deps)) + chartNameCounts := map[string]int{} + for _, dep := range deps { + chartNameCounts[dep.Name()]++ + } + + used := map[string]bool{} + for i, dep := range deps { + name := dep.Name() + if chartNameCounts[dep.Name()] > 1 { + if alias := dependencyAlias(c, dep, used); alias != "" { + name = alias + } else { + name = dependencyVersionedName(dep) + } + } + if used[name] { + name = uniqueDependencyArchiveName(dep, used) + } + used[name] = true + names[i] = name + } + return names +} + +// Use Chart.yaml aliases to avoid clobbering sibling dependencies with the same chart name. +func dependencyAlias(c *chart.Chart, dep *chart.Chart, used map[string]bool) string { + if c.Metadata == nil { + return "" + } + for _, req := range c.Metadata.Dependencies { + if req == nil || req.Alias == "" || used[req.Alias] { + continue + } + if dependencyMatches(dep, req) { + return req.Alias + } + } + return "" +} + +func dependencyMatches(dep *chart.Chart, req *chart.Dependency) bool { + // Dependency processing may rename the loaded chart to its alias. + if req.Name != dep.Name() && req.Alias != dep.Name() { + return false + } + return dep.Metadata == nil || req.Version == "" || IsCompatibleRange(req.Version, dep.Metadata.Version) +} + +func dependencyVersionedName(dep *chart.Chart) string { + if dep.Metadata == nil || dep.Metadata.Version == "" { + return dep.Name() + } + return fmt.Sprintf("%s-%s", dep.Name(), dep.Metadata.Version) +} + +func uniqueDependencyArchiveName(dep *chart.Chart, used map[string]bool) string { + base := dependencyVersionedName(dep) + name := base + for i := 2; used[name]; i++ { + name = fmt.Sprintf("%s-%d", base, i) + } + return name +} + // writeToTar writes a single file to a tar archive. func writeToTar(out *tar.Writer, name string, body []byte, modTime time.Time) error { // TODO: Do we need to create dummy parent directory names if none exist? diff --git a/internal/chart/v3/util/save_test.go b/internal/chart/v3/util/save_test.go index 34e7d898e..06a3e214a 100644 --- a/internal/chart/v3/util/save_test.go +++ b/internal/chart/v3/util/save_test.go @@ -32,6 +32,8 @@ import ( "testing" "time" + "github.com/stretchr/testify/require" + chart "helm.sh/helm/v4/internal/chart/v3" "helm.sh/helm/v4/internal/chart/v3/loader" "helm.sh/helm/v4/pkg/chart/common" @@ -127,6 +129,148 @@ func TestSave(t *testing.T) { } } +func TestSaveDependencyArchiveNames(t *testing.T) { + tests := []struct { + name string + requirements []*chart.Dependency + dependencies []*chart.Chart + wantHeaders []string + notWantHeaders []string + wantLoadedDeps int + wantVersions []string + }{ + { + name: "duplicate chart names use matching aliases", + requirements: []*chart.Dependency{ + {Name: "worker", Version: "1.0.0", Alias: "blue"}, + {Name: "worker", Version: "2.0.0", Alias: "green"}, + }, + dependencies: []*chart.Chart{ + newSaveTestChart("worker", "1.0.0"), + newSaveTestChart("worker", "2.0.0"), + }, + wantHeaders: []string{ + "parent/charts/blue/Chart.yaml", + "parent/charts/green/Chart.yaml", + }, + notWantHeaders: []string{ + "parent/charts/worker/Chart.yaml", + }, + wantLoadedDeps: 2, + wantVersions: []string{"1.0.0", "2.0.0"}, + }, + { + name: "single shared chart keeps original name", + requirements: []*chart.Dependency{ + {Name: "worker", Version: "1.0.0", Alias: "blue"}, + {Name: "worker", Version: "1.0.0", Alias: "green"}, + }, + dependencies: []*chart.Chart{ + newSaveTestChart("worker", "1.0.0"), + }, + wantHeaders: []string{ + "parent/charts/worker/Chart.yaml", + }, + notWantHeaders: []string{ + "parent/charts/blue/Chart.yaml", + "parent/charts/green/Chart.yaml", + }, + wantLoadedDeps: 1, + wantVersions: []string{"1.0.0"}, + }, + { + name: "duplicate chart names without matching aliases use versions", + requirements: []*chart.Dependency{ + {Name: "worker", Version: "3.0.0", Alias: "canary"}, + }, + dependencies: []*chart.Chart{ + newSaveTestChart("worker", "1.0.0"), + newSaveTestChart("worker", "2.0.0"), + }, + wantHeaders: []string{ + "parent/charts/worker-1.0.0/Chart.yaml", + "parent/charts/worker-2.0.0/Chart.yaml", + }, + notWantHeaders: []string{ + "parent/charts/canary/Chart.yaml", + "parent/charts/worker/Chart.yaml", + }, + wantLoadedDeps: 2, + wantVersions: []string{"1.0.0", "2.0.0"}, + }, + { + name: "duplicate archive names get unique suffixes", + dependencies: []*chart.Chart{ + newSaveTestChart("worker", "1.0.0"), + newSaveTestChart("worker", "1.0.0"), + }, + wantHeaders: []string{ + "parent/charts/worker-1.0.0/Chart.yaml", + "parent/charts/worker-1.0.0-2/Chart.yaml", + }, + notWantHeaders: []string{ + "parent/charts/worker/Chart.yaml", + }, + wantLoadedDeps: 2, + wantVersions: []string{"1.0.0", "1.0.0"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + parent := newSaveTestChart("parent", "0.1.0") + parent.Metadata.Dependencies = tt.requirements + parent.SetDependencies(tt.dependencies...) + + archivePath, err := Save(parent, t.TempDir()) + require.NoError(t, err) + + headerNames := retrieveHeaderNames(t, archivePath) + for _, want := range tt.wantHeaders { + require.True(t, headerNames[want], "expected archive to contain %s", want) + } + for _, notWant := range tt.notWantHeaders { + require.False(t, headerNames[notWant], "expected archive not to contain %s", notWant) + } + + loaded, err := loader.LoadFile(archivePath) + require.NoError(t, err) + require.Len(t, loaded.Dependencies(), tt.wantLoadedDeps) + require.ElementsMatch(t, tt.wantVersions, dependencyVersions(loaded.Dependencies())) + }) + } +} + +func newSaveTestChart(name, version string) *chart.Chart { + return &chart.Chart{ + Metadata: &chart.Metadata{ + APIVersion: chart.APIVersionV3, + Name: name, + Version: version, + }, + } +} + +func retrieveHeaderNames(t *testing.T, archivePath string) map[string]bool { + t.Helper() + headers, err := retrieveAllHeadersFromTar(archivePath) + require.NoError(t, err) + + headerNames := map[string]bool{} + for _, header := range headers { + headerNames[header.Name] = true + } + return headerNames +} + +func dependencyVersions(deps []*chart.Chart) []string { + versions := make([]string, len(deps)) + for i, dep := range deps { + versions[i] = dep.Metadata.Version + } + return versions +} + // Creates a copy with a different schema; does not modify anything. func withSchema(chart chart.Chart, schema []byte) chart.Chart { chart.Schema = schema diff --git a/pkg/chart/v2/util/save.go b/pkg/chart/v2/util/save.go index e66d86991..077082bab 100644 --- a/pkg/chart/v2/util/save.go +++ b/pkg/chart/v2/util/save.go @@ -147,19 +147,21 @@ func Save(c *chart.Chart, outDir string) (string, error) { } }() - if err := writeTarContents(twriter, c, ""); err != nil { + if err := writeTarContents(twriter, c, "", c.Name()); err != nil { rollback = true return filename, err } return filename, nil } -func writeTarContents(out *tar.Writer, c *chart.Chart, prefix string) error { - err := validateName(c.Name()) - if err != nil { +func writeTarContents(out *tar.Writer, c *chart.Chart, prefix, baseName string) error { + if err := validateName(c.Name()); err != nil { + return err + } + if err := validateName(baseName); err != nil { return err } - base := filepath.Join(prefix, c.Name()) + base := filepath.Join(prefix, baseName) // Pull out the dependencies of a v1 Chart, since there's no way // to tell the serializer to skip a field for just this use case @@ -229,14 +231,82 @@ func writeTarContents(out *tar.Writer, c *chart.Chart, prefix string) error { } // Save dependencies - for _, dep := range c.Dependencies() { - if err := writeTarContents(out, dep, filepath.Join(base, ChartsDir)); err != nil { + deps := c.Dependencies() + archiveNames := dependencyArchiveNames(c, deps) + for i, dep := range deps { + if err := writeTarContents(out, dep, filepath.Join(base, ChartsDir), archiveNames[i]); err != nil { return err } } return nil } +func dependencyArchiveNames(c *chart.Chart, deps []*chart.Chart) []string { + names := make([]string, len(deps)) + chartNameCounts := map[string]int{} + for _, dep := range deps { + chartNameCounts[dep.Name()]++ + } + + used := map[string]bool{} + for i, dep := range deps { + name := dep.Name() + if chartNameCounts[dep.Name()] > 1 { + if alias := dependencyAlias(c, dep, used); alias != "" { + name = alias + } else { + name = dependencyVersionedName(dep) + } + } + if used[name] { + name = uniqueDependencyArchiveName(dep, used) + } + used[name] = true + names[i] = name + } + return names +} + +// Use Chart.yaml aliases to avoid clobbering sibling dependencies with the same chart name. +func dependencyAlias(c *chart.Chart, dep *chart.Chart, used map[string]bool) string { + if c.Metadata == nil { + return "" + } + for _, req := range c.Metadata.Dependencies { + if req == nil || req.Alias == "" || used[req.Alias] { + continue + } + if dependencyMatches(dep, req) { + return req.Alias + } + } + return "" +} + +func dependencyMatches(dep *chart.Chart, req *chart.Dependency) bool { + // Dependency processing may rename the loaded chart to its alias. + if req.Name != dep.Name() && req.Alias != dep.Name() { + return false + } + return dep.Metadata == nil || req.Version == "" || IsCompatibleRange(req.Version, dep.Metadata.Version) +} + +func dependencyVersionedName(dep *chart.Chart) string { + if dep.Metadata == nil || dep.Metadata.Version == "" { + return dep.Name() + } + return fmt.Sprintf("%s-%s", dep.Name(), dep.Metadata.Version) +} + +func uniqueDependencyArchiveName(dep *chart.Chart, used map[string]bool) string { + base := dependencyVersionedName(dep) + name := base + for i := 2; used[name]; i++ { + name = fmt.Sprintf("%s-%d", base, i) + } + return name +} + // writeToTar writes a single file to a tar archive. func writeToTar(out *tar.Writer, name string, body []byte, modTime time.Time) error { // TODO: Do we need to create dummy parent directory names if none exist? diff --git a/pkg/chart/v2/util/save_test.go b/pkg/chart/v2/util/save_test.go index 2f2b73efd..40ac0c010 100644 --- a/pkg/chart/v2/util/save_test.go +++ b/pkg/chart/v2/util/save_test.go @@ -32,6 +32,8 @@ import ( "testing" "time" + "github.com/stretchr/testify/require" + "helm.sh/helm/v4/pkg/chart/common" chart "helm.sh/helm/v4/pkg/chart/v2" "helm.sh/helm/v4/pkg/chart/v2/loader" @@ -130,6 +132,148 @@ func TestSave(t *testing.T) { } } +func TestSaveDependencyArchiveNames(t *testing.T) { + tests := []struct { + name string + requirements []*chart.Dependency + dependencies []*chart.Chart + wantHeaders []string + notWantHeaders []string + wantLoadedDeps int + wantVersions []string + }{ + { + name: "duplicate chart names use matching aliases", + requirements: []*chart.Dependency{ + {Name: "worker", Version: "1.0.0", Alias: "blue"}, + {Name: "worker", Version: "2.0.0", Alias: "green"}, + }, + dependencies: []*chart.Chart{ + newSaveTestChart("worker", "1.0.0"), + newSaveTestChart("worker", "2.0.0"), + }, + wantHeaders: []string{ + "parent/charts/blue/Chart.yaml", + "parent/charts/green/Chart.yaml", + }, + notWantHeaders: []string{ + "parent/charts/worker/Chart.yaml", + }, + wantLoadedDeps: 2, + wantVersions: []string{"1.0.0", "2.0.0"}, + }, + { + name: "single shared chart keeps original name", + requirements: []*chart.Dependency{ + {Name: "worker", Version: "1.0.0", Alias: "blue"}, + {Name: "worker", Version: "1.0.0", Alias: "green"}, + }, + dependencies: []*chart.Chart{ + newSaveTestChart("worker", "1.0.0"), + }, + wantHeaders: []string{ + "parent/charts/worker/Chart.yaml", + }, + notWantHeaders: []string{ + "parent/charts/blue/Chart.yaml", + "parent/charts/green/Chart.yaml", + }, + wantLoadedDeps: 1, + wantVersions: []string{"1.0.0"}, + }, + { + name: "duplicate chart names without matching aliases use versions", + requirements: []*chart.Dependency{ + {Name: "worker", Version: "3.0.0", Alias: "canary"}, + }, + dependencies: []*chart.Chart{ + newSaveTestChart("worker", "1.0.0"), + newSaveTestChart("worker", "2.0.0"), + }, + wantHeaders: []string{ + "parent/charts/worker-1.0.0/Chart.yaml", + "parent/charts/worker-2.0.0/Chart.yaml", + }, + notWantHeaders: []string{ + "parent/charts/canary/Chart.yaml", + "parent/charts/worker/Chart.yaml", + }, + wantLoadedDeps: 2, + wantVersions: []string{"1.0.0", "2.0.0"}, + }, + { + name: "duplicate archive names get unique suffixes", + dependencies: []*chart.Chart{ + newSaveTestChart("worker", "1.0.0"), + newSaveTestChart("worker", "1.0.0"), + }, + wantHeaders: []string{ + "parent/charts/worker-1.0.0/Chart.yaml", + "parent/charts/worker-1.0.0-2/Chart.yaml", + }, + notWantHeaders: []string{ + "parent/charts/worker/Chart.yaml", + }, + wantLoadedDeps: 2, + wantVersions: []string{"1.0.0", "1.0.0"}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + parent := newSaveTestChart("parent", "0.1.0") + parent.Metadata.Dependencies = tt.requirements + parent.SetDependencies(tt.dependencies...) + + archivePath, err := Save(parent, t.TempDir()) + require.NoError(t, err) + + headerNames := retrieveHeaderNames(t, archivePath) + for _, want := range tt.wantHeaders { + require.True(t, headerNames[want], "expected archive to contain %s", want) + } + for _, notWant := range tt.notWantHeaders { + require.False(t, headerNames[notWant], "expected archive not to contain %s", notWant) + } + + loaded, err := loader.LoadFile(archivePath) + require.NoError(t, err) + require.Len(t, loaded.Dependencies(), tt.wantLoadedDeps) + require.ElementsMatch(t, tt.wantVersions, dependencyVersions(loaded.Dependencies())) + }) + } +} + +func newSaveTestChart(name, version string) *chart.Chart { + return &chart.Chart{ + Metadata: &chart.Metadata{ + APIVersion: chart.APIVersionV2, + Name: name, + Version: version, + }, + } +} + +func retrieveHeaderNames(t *testing.T, archivePath string) map[string]bool { + t.Helper() + headers, err := retrieveAllHeadersFromTar(archivePath) + require.NoError(t, err) + + headerNames := map[string]bool{} + for _, header := range headers { + headerNames[header.Name] = true + } + return headerNames +} + +func dependencyVersions(deps []*chart.Chart) []string { + versions := make([]string, len(deps)) + for i, dep := range deps { + versions[i] = dep.Metadata.Version + } + return versions +} + // Creates a copy with a different schema; does not modify anything. func withSchema(chart chart.Chart, schema []byte) chart.Chart { chart.Schema = schema