From 54ced19b932e2f83bbbf702145ce944a4bc90dea Mon Sep 17 00:00:00 2001 From: Sourav21jain Date: Thu, 4 Jun 2026 16:03:56 +0530 Subject: [PATCH 1/3] fix(helm): tolerate v-prefix version mismatch in disk-to-mirror During the disk-to-mirror phase oc-mirror reconstructed chart tarball paths as {name}-{version}.tgz, but the Helm downloader saves files using the URL basename from the repository index.yaml, which may embed a "v" prefix (e.g. csi-driver-nfs-v4.9.0.tgz). This caused d2m to fail with "file not found" even though the m2d download succeeded. Introduce resolveChartPath which tries the primary path first and falls back to toggling the "v" prefix on the version component: - Path traversal is prevented by confining both candidates to the charts directory via filepath.Rel. - Non-ENOENT os.Stat errors (permission/I/O) are surfaced immediately rather than being silently treated as "file missing". - A descriptive error naming both attempted paths is returned when neither candidate exists. Add two new test functions: - TestResolveChartPath: unit tests for the helper covering all four cases (exact match, config omits v / disk has v, config has v / disk has no v, both missing). The not-found case asserts both candidate path fragments appear in the error message. - TestHelmImageCollectorVPrefixDiskToMirror: end-to-end d2m collector test that places a v-prefixed chart on disk while the config specifies the bare version, and the reverse. Co-authored-by: Cursor --- internal/pkg/helm/local_stored_collector.go | 62 ++++++- .../pkg/helm/local_stored_collector_test.go | 163 ++++++++++++++++++ 2 files changed, 223 insertions(+), 2 deletions(-) diff --git a/internal/pkg/helm/local_stored_collector.go b/internal/pkg/helm/local_stored_collector.go index da09d8575..73e179313 100644 --- a/internal/pkg/helm/local_stored_collector.go +++ b/internal/pkg/helm/local_stored_collector.go @@ -168,8 +168,12 @@ func (o *LocalStorageCollector) HelmImageCollector(ctx context.Context) ([]v2alp } for _, chart := range charts { - src := filepath.Join(lsc.Opts.Global.WorkingDir, helmDir, helmChartDir) - path := filepath.Join(src, fmt.Sprintf("%s-%s.tgz", chart.Name, chart.Version)) + chartDir := filepath.Join(lsc.Opts.Global.WorkingDir, helmDir, helmChartDir) + path, err := resolveChartPath(chartDir, chart.Name, chart.Version) + if err != nil { + errs = append(errs, err) + continue + } imgs, err := getImages(path, chart.ImagePaths...) if err != nil { @@ -349,6 +353,60 @@ func getChartsFromIndex(indexURL string, indexFile helmrepo.IndexFile) ([]v2alph return charts, nil } +// resolveChartPath returns the filesystem path to a Helm chart tarball stored +// under dir. It first tries "{name}-{version}.tgz". If that file is absent +// it retries with the version's "v" prefix toggled (added when missing, or +// stripped when present), because some Helm repositories embed the prefix in +// the tarball URL while others omit it. A descriptive error is returned when +// neither candidate exists. +func resolveChartPath(dir, name, version string) (string, error) { + baseDir := filepath.Clean(dir) + + // buildCandidate constructs a candidate path and ensures it stays inside + // baseDir, preventing path traversal via crafted name or version values. + buildCandidate := func(ver string) (string, error) { + p := filepath.Join(baseDir, fmt.Sprintf("%s-%s.tgz", name, ver)) + rel, err := filepath.Rel(baseDir, p) + if err != nil { + return "", fmt.Errorf("resolve chart path: %w", err) + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return "", fmt.Errorf("invalid chart reference %q:%q escapes chart directory", name, ver) + } + return p, nil + } + + primary, err := buildCandidate(version) + if err != nil { + return "", err + } + if _, err := os.Stat(primary); err == nil { + return primary, nil + } else if !errors.Is(err, os.ErrNotExist) { + return "", fmt.Errorf("stat %s: %w", primary, err) + } + + var altVersion string + if strings.HasPrefix(version, "v") { + altVersion = strings.TrimPrefix(version, "v") + } else { + altVersion = "v" + version + } + + alt, err := buildCandidate(altVersion) + if err != nil { + return "", err + } + if _, err := os.Stat(alt); err == nil { + lsc.Log.Debug("chart %s: %s not found, using %s (v-prefix variant)", name, filepath.Base(primary), filepath.Base(alt)) + return alt, nil + } else if !errors.Is(err, os.ErrNotExist) { + return "", fmt.Errorf("stat %s: %w", alt, err) + } + + return "", fmt.Errorf("chart file not found for %s version %s: tried %s and %s", name, version, primary, alt) +} + func getImages(path string, imagePaths ...string) (images []v2alpha1.RelatedImage, err error) { lsc.Log.Debug("Reading from path %s", path) diff --git a/internal/pkg/helm/local_stored_collector_test.go b/internal/pkg/helm/local_stored_collector_test.go index da1719f75..5d813432d 100644 --- a/internal/pkg/helm/local_stored_collector_test.go +++ b/internal/pkg/helm/local_stored_collector_test.go @@ -9,6 +9,7 @@ import ( "os" "path" "path/filepath" + "strings" "testing" "github.com/otiai10/copy" @@ -715,6 +716,168 @@ func TestHelmImageCollector(t *testing.T) { } } +// TestResolveChartPath verifies that resolveChartPath tolerates mismatches +// between the version string in the ImageSetConfiguration and the "v" prefix +// that a Helm repository may embed in its tarball filenames. +func TestResolveChartPath(t *testing.T) { + dir := t.TempDir() + + // chartA-1.0.0.tgz – version stored without "v" + // chartB-v2.0.0.tgz – version stored with "v" + chartAFile := filepath.Join(dir, "chartA-1.0.0.tgz") + chartBFile := filepath.Join(dir, "chartB-v2.0.0.tgz") + assert.NoError(t, os.WriteFile(chartAFile, []byte("placeholder"), 0600)) + assert.NoError(t, os.WriteFile(chartBFile, []byte("placeholder"), 0600)) + + // resolveChartPath calls lsc.Log.Debug; initialise the global so the + // helper does not panic. + lsc = &LocalStorageCollector{Log: clog.New("trace")} + + tests := []struct { + name string + chartName string + version string + wantPath string + wantErr bool + }{ + { + name: "exact match – no v prefix", + chartName: "chartA", + version: "1.0.0", + wantPath: chartAFile, + }, + { + name: "config omits v; file on disk has v prefix", + chartName: "chartB", + version: "2.0.0", + wantPath: chartBFile, + }, + { + name: "config has v; file on disk has no v prefix", + chartName: "chartA", + version: "v1.0.0", + wantPath: chartAFile, + }, + { + name: "neither candidate exists returns error", + chartName: "missing", + version: "9.9.9", + wantErr: true, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got, err := resolveChartPath(dir, tc.chartName, tc.version) + if tc.wantErr { + assert.Error(t, err) + // Both attempted candidate paths must appear in the error so + // callers can diagnose which filenames were probed. + assert.Contains(t, err.Error(), fmt.Sprintf("%s-%s.tgz", tc.chartName, tc.version)) + altVersion := "v" + tc.version + if strings.HasPrefix(tc.version, "v") { + altVersion = strings.TrimPrefix(tc.version, "v") + } + assert.Contains(t, err.Error(), fmt.Sprintf("%s-%s.tgz", tc.chartName, altVersion)) + return + } + assert.NoError(t, err) + assert.Equal(t, tc.wantPath, got) + }) + } +} + +// TestHelmImageCollectorVPrefixDiskToMirror ensures that the disk-to-mirror +// collector succeeds even when the chart tarball on disk was saved by the Helm +// downloader using a "v"-prefixed version (e.g. podinfo-v5.0.0.tgz) while the +// ImageSetConfiguration specifies the version without the prefix (5.0.0), or +// vice-versa. +func TestHelmImageCollectorVPrefixDiskToMirror(t *testing.T) { + log := clog.New("trace") + ctx := context.Background() + + tests := []struct { + name string + configVersion string + diskVersion string + expectedImages []v2alpha1.CopyImageSchema + }{ + { + name: "config without v; disk file has v prefix", + configVersion: "5.0.0", + diskVersion: "v5.0.0", + expectedImages: []v2alpha1.CopyImageSchema{ + { + Source: consts.DockerProtocol + testLocalStorageFQDN + "/stefanprodan/podinfo:5.0.0", + Destination: testDest + "/stefanprodan/podinfo:5.0.0", + Origin: "ghcr.io/stefanprodan/podinfo:5.0.0", + Type: v2alpha1.TypeHelmImage, + }, + }, + }, + { + name: "config with v; disk file has no v prefix", + configVersion: "v5.0.0", + diskVersion: "5.0.0", + expectedImages: []v2alpha1.CopyImageSchema{ + { + Source: consts.DockerProtocol + testLocalStorageFQDN + "/stefanprodan/podinfo:5.0.0", + Destination: testDest + "/stefanprodan/podinfo:5.0.0", + Origin: "ghcr.io/stefanprodan/podinfo:5.0.0", + Type: v2alpha1.TypeHelmImage, + }, + }, + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + workingDir, err := prepareFolder(t.TempDir()) + assert.NoError(t, err) + + helmCfg := v2alpha1.Helm{ + Repositories: []v2alpha1.Repository{ + { + Name: "podinfo", + URL: "https://stefanprodan.github.io/podinfo", + Charts: []v2alpha1.Chart{ + {Name: "podinfo", Version: tc.configVersion}, + }, + }, + }, + } + + testCfg := v2alpha1.ImageSetConfiguration{ + ImageSetConfigurationSpec: v2alpha1.ImageSetConfigurationSpec{ + Mirror: v2alpha1.Mirror{Helm: helmCfg}, + }, + } + + _, srcOpts := mirror.ImageSrcFlags(nil, nil, nil, "src-", "screds") + opts := mirror.CopyOptions{ + Mode: mirror.DiskToMirror, + Global: &mirror.GlobalOptions{WorkingDir: workingDir}, + LocalStorageFQDN: testLocalStorageFQDN, + Destination: testDest, + SrcImage: srcOpts, + } + + New(log, testCfg, opts, nil, nil, nil) + + // Place the chart on disk using the downloader-supplied version + // string (which may differ from what the config specifies). + diskFile := filepath.Join(tempChartDir, fmt.Sprintf("podinfo-%s.tgz", tc.diskVersion)) + assert.NoError(t, copy.Copy(filepath.Join(testChartsDataPath, "podinfo-5.0.0.tgz"), diskFile)) + + helmCollector := lsc + imgs, err := helmCollector.HelmImageCollector(ctx) + + assert.NoError(t, err) + assert.ElementsMatch(t, tc.expectedImages, imgs) + }) + } +} + func prepareDiskToMirror(testCase testCase) error { for _, repo := range testCase.helmConfig.Repositories { var err error From 842be5346988beee518b7efb88383442a1bf7cab Mon Sep 17 00:00:00 2001 From: Sourav21jain Date: Thu, 11 Jun 2026 21:41:38 +0530 Subject: [PATCH 2/3] fix(helm): reduce cyclomatic complexity of resolveChartPath Extract chartPathExists helper to handle os.Stat + error classification logic, dropping resolveChartPath's cyclomatic complexity from 11 to 6 to satisfy the cyclop linter's max-complexity limit of 10. Also promote buildChartCandidatePath and toggleVersionPrefix from inline closures to package-level functions for clarity. Co-authored-by: Cursor --- internal/pkg/helm/local_stored_collector.go | 73 ++++++++++++++------- 1 file changed, 48 insertions(+), 25 deletions(-) diff --git a/internal/pkg/helm/local_stored_collector.go b/internal/pkg/helm/local_stored_collector.go index 73e179313..f0a28aa9d 100644 --- a/internal/pkg/helm/local_stored_collector.go +++ b/internal/pkg/helm/local_stored_collector.go @@ -362,51 +362,74 @@ func getChartsFromIndex(indexURL string, indexFile helmrepo.IndexFile) ([]v2alph func resolveChartPath(dir, name, version string) (string, error) { baseDir := filepath.Clean(dir) - // buildCandidate constructs a candidate path and ensures it stays inside - // baseDir, preventing path traversal via crafted name or version values. - buildCandidate := func(ver string) (string, error) { - p := filepath.Join(baseDir, fmt.Sprintf("%s-%s.tgz", name, ver)) - rel, err := filepath.Rel(baseDir, p) - if err != nil { - return "", fmt.Errorf("resolve chart path: %w", err) - } - if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { - return "", fmt.Errorf("invalid chart reference %q:%q escapes chart directory", name, ver) - } - return p, nil + primary, err := buildChartCandidatePath(baseDir, name, version) + if err != nil { + return "", err } - primary, err := buildCandidate(version) + primaryFound, err := chartPathExists(primary) if err != nil { return "", err } - if _, err := os.Stat(primary); err == nil { + if primaryFound { return primary, nil - } else if !errors.Is(err, os.ErrNotExist) { - return "", fmt.Errorf("stat %s: %w", primary, err) } - var altVersion string - if strings.HasPrefix(version, "v") { - altVersion = strings.TrimPrefix(version, "v") - } else { - altVersion = "v" + version + alt, err := buildChartCandidatePath(baseDir, name, toggleVersionPrefix(version)) + if err != nil { + return "", err } - alt, err := buildCandidate(altVersion) + altFound, err := chartPathExists(alt) if err != nil { return "", err } - if _, err := os.Stat(alt); err == nil { + if altFound { lsc.Log.Debug("chart %s: %s not found, using %s (v-prefix variant)", name, filepath.Base(primary), filepath.Base(alt)) return alt, nil - } else if !errors.Is(err, os.ErrNotExist) { - return "", fmt.Errorf("stat %s: %w", alt, err) } return "", fmt.Errorf("chart file not found for %s version %s: tried %s and %s", name, version, primary, alt) } +// chartPathExists reports whether the given path exists on disk. +// It returns false (without error) for missing files and propagates +// real I/O or permission errors so they are not silently swallowed. +func chartPathExists(path string) (bool, error) { + _, err := os.Stat(path) + if err == nil { + return true, nil + } + if errors.Is(err, os.ErrNotExist) { + return false, nil + } + return false, fmt.Errorf("stat %s: %w", path, err) +} + +// buildChartCandidatePath constructs the expected tarball path for a chart +// version and validates it stays within baseDir to prevent path traversal. +func buildChartCandidatePath(baseDir, name, ver string) (string, error) { + p := filepath.Join(baseDir, fmt.Sprintf("%s-%s.tgz", name, ver)) + rel, err := filepath.Rel(baseDir, p) + if err != nil { + return "", fmt.Errorf("resolve chart path: %w", err) + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) { + return "", fmt.Errorf("invalid chart reference %q:%q escapes chart directory", name, ver) + } + return p, nil +} + +// toggleVersionPrefix adds a "v" prefix to ver when absent, or removes it when +// present. This normalises between Helm repositories that differ on whether +// chart tarball filenames carry the prefix. +func toggleVersionPrefix(ver string) string { + if strings.HasPrefix(ver, "v") { + return strings.TrimPrefix(ver, "v") + } + return "v" + ver +} + func getImages(path string, imagePaths ...string) (images []v2alpha1.RelatedImage, err error) { lsc.Log.Debug("Reading from path %s", path) From 4d86534a06df0352a832cac83fea45bc629dfd69 Mon Sep 17 00:00:00 2001 From: Sourav21jain Date: Fri, 12 Jun 2026 14:24:06 +0530 Subject: [PATCH 3/3] refactor(helm): use semver to normalize version in resolveChartPath Replace manual v-prefix toggling with semver.NewVersion() which already exists in the codebase. semver.String() always returns the canonical form without a "v" prefix, so we can deterministically probe both "{name}-{canonical}.tgz" and "{name}-v{canonical}.tgz" in a loop, removing the need for the toggleVersionPrefix() helper. Co-authored-by: Cursor --- internal/pkg/helm/local_stored_collector.go | 68 +++++++++------------ 1 file changed, 30 insertions(+), 38 deletions(-) diff --git a/internal/pkg/helm/local_stored_collector.go b/internal/pkg/helm/local_stored_collector.go index f0a28aa9d..4fff91415 100644 --- a/internal/pkg/helm/local_stored_collector.go +++ b/internal/pkg/helm/local_stored_collector.go @@ -11,6 +11,7 @@ import ( "path/filepath" "strings" + "github.com/Masterminds/semver/v3" helmchart "helm.sh/helm/v3/pkg/chart" "helm.sh/helm/v3/pkg/chart/loader" "helm.sh/helm/v3/pkg/chartutil" @@ -354,42 +355,43 @@ func getChartsFromIndex(indexURL string, indexFile helmrepo.IndexFile) ([]v2alph } // resolveChartPath returns the filesystem path to a Helm chart tarball stored -// under dir. It first tries "{name}-{version}.tgz". If that file is absent -// it retries with the version's "v" prefix toggled (added when missing, or -// stripped when present), because some Helm repositories embed the prefix in -// the tarball URL while others omit it. A descriptive error is returned when -// neither candidate exists. +// under dir. It normalises the version with semver (which strips any leading +// "v") and then probes two candidate filenames — "{name}-{canonical}.tgz" and +// "{name}-v{canonical}.tgz" — because Helm repositories differ on whether they +// embed the "v" prefix in tarball URLs. A descriptive error naming both +// attempted paths is returned when neither file exists. func resolveChartPath(dir, name, version string) (string, error) { - baseDir := filepath.Clean(dir) - - primary, err := buildChartCandidatePath(baseDir, name, version) + ver, err := semver.NewVersion(version) if err != nil { - return "", err + return "", fmt.Errorf("invalid chart version %q: %w", version, err) } + canonical := ver.String() // always without "v" prefix, regardless of how version was specified - primaryFound, err := chartPathExists(primary) - if err != nil { - return "", err - } - if primaryFound { - return primary, nil - } + baseDir := filepath.Clean(dir) + candidateVersions := []string{canonical, "v" + canonical} + var candidatePaths []string - alt, err := buildChartCandidatePath(baseDir, name, toggleVersionPrefix(version)) - if err != nil { - return "", err - } + for _, candidateVer := range candidateVersions { + path, err := buildChartCandidatePath(baseDir, name, candidateVer) + if err != nil { + return "", err + } + candidatePaths = append(candidatePaths, path) - altFound, err := chartPathExists(alt) - if err != nil { - return "", err - } - if altFound { - lsc.Log.Debug("chart %s: %s not found, using %s (v-prefix variant)", name, filepath.Base(primary), filepath.Base(alt)) - return alt, nil + found, err := chartPathExists(path) + if err != nil { + return "", err + } + if found { + if candidateVer != candidateVersions[0] { + lsc.Log.Debug("chart %s: %s not found, using %s (v-prefix variant)", name, filepath.Base(candidatePaths[0]), filepath.Base(path)) + } + return path, nil + } } - return "", fmt.Errorf("chart file not found for %s version %s: tried %s and %s", name, version, primary, alt) + return "", fmt.Errorf("chart file not found for %s version %s: tried %s and %s", + name, version, filepath.Base(candidatePaths[0]), filepath.Base(candidatePaths[1])) } // chartPathExists reports whether the given path exists on disk. @@ -420,16 +422,6 @@ func buildChartCandidatePath(baseDir, name, ver string) (string, error) { return p, nil } -// toggleVersionPrefix adds a "v" prefix to ver when absent, or removes it when -// present. This normalises between Helm repositories that differ on whether -// chart tarball filenames carry the prefix. -func toggleVersionPrefix(ver string) string { - if strings.HasPrefix(ver, "v") { - return strings.TrimPrefix(ver, "v") - } - return "v" + ver -} - func getImages(path string, imagePaths ...string) (images []v2alpha1.RelatedImage, err error) { lsc.Log.Debug("Reading from path %s", path)