From 9731d95a564fa8fe8369d676cf8850e9661f2a0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jakob=20M=C3=B6ller?= Date: Fri, 23 May 2025 14:45:07 +0200 Subject: [PATCH] fix: make sure that helm downloader does not overwrite chart with provenance data (#1481) #### What this PR does / why we need it in the helm downloader, there was a case when the chart data was overwritten with the provenance data. if the target path passed to the downloader was a path such as `path.tgz` then this path was reused for writing the provenance file. This was now fixed and there will be: 1. A download path pointing to the .tgz => chart.tgz 2. A provenance file (if it exists) => chart.prov #### Which issue(s) this PR fixes Note that before, due to buggy behavior the output provenance file was just called `chart` because the .prov file extension was not properly added. this has been fixed as well. additionally if the file path was `tar.gz` instead of `tgz`, the file ending removal was incorrect. this has been fixed. This was also the last reason why the ocm download resource command returned incorrect data in the tgz when downloading a resource with accessType helm. fix https://github.com/open-component-model/ocm/issues/1469 (cherry picked from commit c2e1bd2cfb20d7fe97415580013bad004858778e) --- .../download/handlers/helm/download_test.go | 47 +++++++++++++++---- .../download/handlers/helm/handler.go | 19 ++++++-- .../download/handlers/helm/handler_test.go | 45 ++++++++++++++++++ 3 files changed, 100 insertions(+), 11 deletions(-) create mode 100644 api/ocm/extensions/download/handlers/helm/handler_test.go diff --git a/api/ocm/extensions/download/handlers/helm/download_test.go b/api/ocm/extensions/download/handlers/helm/download_test.go index 7925d4c77d..324949a9d2 100644 --- a/api/ocm/extensions/download/handlers/helm/download_test.go +++ b/api/ocm/extensions/download/handlers/helm/download_test.go @@ -4,12 +4,12 @@ import ( . "github.com/mandelsoft/goutils/testutils" . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - - . "ocm.software/ocm/api/helper/builder" + "helm.sh/helm/v3/pkg/registry" "github.com/mandelsoft/filepath/pkg/filepath" "github.com/mandelsoft/vfs/pkg/vfs" + . "ocm.software/ocm/api/helper/builder" envhelper "ocm.software/ocm/api/helper/env" "ocm.software/ocm/api/oci/artdesc" "ocm.software/ocm/api/oci/extensions/repositories/artifactset" @@ -21,6 +21,7 @@ import ( "ocm.software/ocm/api/ocm/selectors" "ocm.software/ocm/api/utils/accessio" "ocm.software/ocm/api/utils/accessobj" + "ocm.software/ocm/api/utils/blobaccess/dirtree" "ocm.software/ocm/api/utils/tarutils" ) @@ -30,10 +31,11 @@ const ( Version = "v1.0.0" OCIResource = "helm" - ArtifactType = "NotHelmChart" - SpecialOCIResource = "specialhelm" - UnusualOCIResource = "unusualhelm" - LegacyOCIResource = "legacyhelm" + ArtifactType = "NotHelmChart" + SpecialOCIResource = "specialhelm" + UnusualOCIResource = "unusualhelm" + LegacyOCIResource = "legacyhelm" + HelmViaChartMediaType = "helmviaaccess" ) var _ = Describe("upload", func() { @@ -57,6 +59,9 @@ var _ = Describe("upload", func() { env.Resource(LegacyOCIResource, Version, resourcetypes.HELM_CHART, v1.LocalRelation, func() { env.BlobFromFile(artifactset.MediaType(artdesc.MediaTypeImageManifest), filepath.Join("/testdata/legacy-pre-hip-helm-chart.tgz")) }) + env.Resource(HelmViaChartMediaType, Version, resourcetypes.HELM_CHART, v1.LocalRelation, func() { + env.BlobFromDirTree("/testdata/test-chart", dirtree.WithMimeType(registry.ChartLayerMediaType), dirtree.WithPreserveDir(true), dirtree.WithCompressWithGzip(true)) + }) }) }) }) @@ -85,7 +90,6 @@ var _ = Describe("upload", func() { It("successfully download unusual artifacts with non-defacto helm chart order", func() { MustBeSuccessful(download.RegisterHandlerByName(env, helm.PATH, nil, download.ForArtifactType(ArtifactType))) - repo := Must(ctf.Open(env.OCMContext(), accessobj.ACC_READONLY, CTFPath, 0o777, env)) cv := Must(repo.LookupComponentVersion(Component, Version)) path := Must(download.DownloadResource(env.OCMContext(), Must(cv.SelectResources(selectors.Identity(v1.Identity{"name": UnusualOCIResource})))[0], "/resource", download.WithFileSystem(env.FileSystem()))) @@ -93,13 +97,40 @@ var _ = Describe("upload", func() { Expect(Must(vfs.FileExists(env.FileSystem(), "/postgresql/Chart.yaml"))).To(BeTrue()) }) - It("successfully download artifacts with helm chart content with legacy content type", func() { + It("successfully download unusual artifacts with non-defacto helm chart order and path given to tgz", func() { MustBeSuccessful(download.RegisterHandlerByName(env, helm.PATH, nil, download.ForArtifactType(ArtifactType))) + repo := Must(ctf.Open(env.OCMContext(), accessobj.ACC_READONLY, CTFPath, 0o777, env)) + cv := Must(repo.LookupComponentVersion(Component, Version)) + path := Must(download.DownloadResource(env.OCMContext(), Must(cv.SelectResources(selectors.Identity(v1.Identity{"name": UnusualOCIResource})))[0], "/path.tgz", download.WithFileSystem(env.FileSystem()))) + MustBeSuccessful(tarutils.ExtractArchiveToFs(env.FileSystem(), path, env.FileSystem())) + Expect(Must(vfs.FileExists(env.FileSystem(), "/postgresql/Chart.yaml"))).To(BeTrue()) + }) + It("successfully download unusual artifacts with non-defacto helm chart order and path given to tar.gz", func() { + MustBeSuccessful(download.RegisterHandlerByName(env, helm.PATH, nil, download.ForArtifactType(ArtifactType))) + repo := Must(ctf.Open(env.OCMContext(), accessobj.ACC_READONLY, CTFPath, 0o777, env)) + cv := Must(repo.LookupComponentVersion(Component, Version)) + path := Must(download.DownloadResource(env.OCMContext(), Must(cv.SelectResources(selectors.Identity(v1.Identity{"name": UnusualOCIResource})))[0], "/path.tar.gz", download.WithFileSystem(env.FileSystem()))) + MustBeSuccessful(tarutils.ExtractArchiveToFs(env.FileSystem(), path, env.FileSystem())) + Expect(Must(vfs.FileExists(env.FileSystem(), "/postgresql/Chart.yaml"))).To(BeTrue()) + }) + + It("successfully download unusual artifacts with helm media type", func() { + MustBeSuccessful(download.RegisterHandlerByName(env, helm.PATH, nil, download.ForArtifactType(ArtifactType))) + repo := Must(ctf.Open(env.OCMContext(), accessobj.ACC_READONLY, CTFPath, 0o777, env)) + cv := Must(repo.LookupComponentVersion(Component, Version)) + path := Must(download.DownloadResource(env.OCMContext(), Must(cv.SelectResources(selectors.Identity(v1.Identity{"name": HelmViaChartMediaType})))[0], "/resource", download.WithFileSystem(env.FileSystem()))) + MustBeSuccessful(tarutils.ExtractArchiveToFs(env.FileSystem(), path, env.FileSystem())) + Expect(Must(vfs.FileExists(env.FileSystem(), "test-chart/Chart.yaml"))).To(BeTrue()) + }) + + It("successfully download artifacts with helm chart content with legacy content type", func() { + MustBeSuccessful(download.RegisterHandlerByName(env, helm.PATH, nil, download.ForArtifactType(ArtifactType))) repo := Must(ctf.Open(env.OCMContext(), accessobj.ACC_READONLY, CTFPath, 0o777, env)) cv := Must(repo.LookupComponentVersion(Component, Version)) path := Must(download.DownloadResource(env.OCMContext(), Must(cv.SelectResources(selectors.Identity(v1.Identity{"name": LegacyOCIResource})))[0], "/resource", download.WithFileSystem(env.FileSystem()))) MustBeSuccessful(tarutils.ExtractArchiveToFs(env.FileSystem(), path, env.FileSystem())) Expect(Must(vfs.FileExists(env.FileSystem(), "/ingress-nginx/Chart.yaml"))).To(BeTrue()) }) + }) diff --git a/api/ocm/extensions/download/handlers/helm/handler.go b/api/ocm/extensions/download/handlers/helm/handler.go index 74481cbdfa..e2601a8811 100644 --- a/api/ocm/extensions/download/handlers/helm/handler.go +++ b/api/ocm/extensions/download/handlers/helm/handler.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "io" + "path/filepath" "strings" "github.com/mandelsoft/goutils/finalizer" @@ -155,9 +156,8 @@ func download(p common.Printer, art oci.ArtifactAccess, path string, fs vfs.File return "", "", err } finalize.Close(provBlob) - - prov = chart[:len(chart)-3] + "prov" - if err := write(p, provBlob, path, fs); err != nil { + prov = trimExtN(chart, 2) + ".prov" + if err := write(p, provBlob, prov, fs); err != nil { return "", "", err } } else if !errors.Is(err, ErrNoMatchingLayer) { // Ignore if no provenance layer is found, because its optional. @@ -167,6 +167,19 @@ func download(p common.Printer, art oci.ArtifactAccess, path string, fs vfs.File return chart, prov, nil } +// trimExtN trims the file extension from the path, at most n times. +// This is useful for removing the ".tgz" or ".tar.gz" suffix from a file name. +func trimExtN(path string, n int) string { + for i := 0; i < n; i++ { + trimmed := strings.TrimSuffix(path, filepath.Ext(path)) + if trimmed == path { + break + } + path = trimmed + } + return path +} + func findLayer(layers []ocispec.Descriptor, mediaType string) (*ocispec.Descriptor, error) { var candidates []*ocispec.Descriptor diff --git a/api/ocm/extensions/download/handlers/helm/handler_test.go b/api/ocm/extensions/download/handlers/helm/handler_test.go new file mode 100644 index 0000000000..4562171a66 --- /dev/null +++ b/api/ocm/extensions/download/handlers/helm/handler_test.go @@ -0,0 +1,45 @@ +package helm + +import ( + "testing" +) + +func TestAssureArchiveSuffix(t *testing.T) { + tests := []struct { + input string + expected string + }{ + {"chart", "chart.tgz"}, + {"chart.tgz", "chart.tgz"}, + {"chart.tar.gz", "chart.tar.gz"}, + {"archive.zip", "archive.zip.tgz"}, + } + + for _, tt := range tests { + result := AssureArchiveSuffix(tt.input) + if result != tt.expected { + t.Errorf("AssureArchiveSuffix(%q) = %q; want %q", tt.input, result, tt.expected) + } + } +} + +func TestTrimExtN(t *testing.T) { + tests := []struct { + input string + n int + expected string + }{ + {"chart.tgz", 1, "chart"}, + {"chart.tar.gz", 1, "chart.tar"}, + {"chart.tar.gz", 2, "chart"}, + {"archive.zip", 1, "archive"}, + {"archive", 1, "archive"}, + } + + for _, tt := range tests { + result := trimExtN(tt.input, tt.n) + if result != tt.expected { + t.Errorf("trimExtN(%q, %d) = %q; want %q", tt.input, tt.n, result, tt.expected) + } + } +}