diff --git a/internal/pkg/helm/local_stored_collector.go b/internal/pkg/helm/local_stored_collector.go index da09d8575..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" @@ -168,8 +169,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 +354,74 @@ 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 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) { + ver, err := semver.NewVersion(version) + if err != nil { + return "", fmt.Errorf("invalid chart version %q: %w", version, err) + } + canonical := ver.String() // always without "v" prefix, regardless of how version was specified + + baseDir := filepath.Clean(dir) + candidateVersions := []string{canonical, "v" + canonical} + var candidatePaths []string + + for _, candidateVer := range candidateVersions { + path, err := buildChartCandidatePath(baseDir, name, candidateVer) + if err != nil { + return "", err + } + candidatePaths = append(candidatePaths, path) + + 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, filepath.Base(candidatePaths[0]), filepath.Base(candidatePaths[1])) +} + +// 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 +} + 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