From 21f34601eb4c62fc4fd52a26a6750dcd05cfea11 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Sat, 14 Jun 2025 09:16:06 +0200 Subject: [PATCH 1/2] artifact mount: improve single blob behavior If the artifact has a single blob then use the dst path directly as mount in case it does not exist. Signed-off-by: Paul Holzinger --- docs/source/markdown/options/mount.md | 13 +++++++++---- libpod/container_internal_common.go | 9 ++++++--- test/e2e/artifact_mount_test.go | 28 ++++++++++++++++++--------- 3 files changed, 34 insertions(+), 16 deletions(-) diff --git a/docs/source/markdown/options/mount.md b/docs/source/markdown/options/mount.md index 0d298c61b42..b70ab873cad 100644 --- a/docs/source/markdown/options/mount.md +++ b/docs/source/markdown/options/mount.md @@ -34,15 +34,20 @@ Options specific to type=**artifact**: The *src* argument contains the name of the artifact, which must already exist locally. The *dst* argument contains the target path, if the path in the container is a -directory or does not exist the blob title (`org.opencontainers.image.title` -annotation) will be used as filename and joined to the path. If the annotation -does not exist the digest will be used as filename instead. This results in all blobs -of the artifact mounted into the container at the given path. +directory the blob title (`org.opencontainers.image.title` annotation) will be used as +filename and joined to the path. If the annotation does not exist the digest will be +used as filename instead. This results in all blobs of the artifact mounted into the +container at the given path. However, if the *dst* path is an existing file in the container, then the blob will be mounted directly on it. This only works when the artifact contains a single blob or when either *digest* or *title* are specified. +If the *dst* path does not already exist in the container then if the artifact contains +a single blob it behaves like existing file case and mounts directly to that path. +If the artifact has more than one blob it works like the existing directory case and +mounts each blob as file within the *dst* path. + Options specific to type=**volume**: - *ro*, *readonly*: *true* or *false* (default if unspecified: *false*). diff --git a/libpod/container_internal_common.go b/libpod/container_internal_common.go index b6196dba7e4..f5222b7a933 100644 --- a/libpod/container_internal_common.go +++ b/libpod/container_internal_common.go @@ -554,9 +554,12 @@ func (c *Container) generateSpec(ctx context.Context) (s *spec.Spec, cleanupFunc return nil, nil, err } - // Ignore the error, destIsFile will return false with errors so if the file does not exist - // we treat it as dir, the oci runtime will always create the target bind mount path. - destIsFile, _ := containerPathIsFile(c.state.Mountpoint, artifactMount.Dest) + destIsFile, err := containerPathIsFile(c.state.Mountpoint, artifactMount.Dest) + // When the file does not exists and the artifact has only a single blob to mount + // assume it is a file so we use the dest path as direct mount. + if err != nil && len(paths) == 1 && errors.Is(err, fs.ErrNotExist) { + destIsFile = true + } if destIsFile && len(paths) > 1 { return nil, nil, fmt.Errorf("artifact %q contains more than one blob and container path %q is a file", artifactMount.Source, artifactMount.Dest) } diff --git a/test/e2e/artifact_mount_test.go b/test/e2e/artifact_mount_test.go index d158dc13312..e8b5a7ca2a2 100644 --- a/test/e2e/artifact_mount_test.go +++ b/test/e2e/artifact_mount_test.go @@ -29,7 +29,7 @@ var _ = Describe("Podman artifact mount", func() { { name: "single artifact mount", mountOpts: "dst=/test", - containerFile: "/test/testfile", + containerFile: "/test", }, { name: "single artifact mount on existing file", @@ -44,7 +44,7 @@ var _ = Describe("Podman artifact mount", func() { { name: "single artifact mount with digest", mountOpts: "dst=/data,digest=sha256:e9510923578af3632946ecf5ae479c1b5f08b47464e707b5cbab9819272a9752", - containerFile: "/data/sha256-e9510923578af3632946ecf5ae479c1b5f08b47464e707b5cbab9819272a9752", + containerFile: "/data", }, } @@ -104,21 +104,31 @@ var _ = Describe("Podman artifact mount", func() { }, }, { - name: "multi blob filter by title", + name: "multi blob filter by title on non existing file", mountOpts: "src=" + ARTIFACT_MULTI + ",dst=/test,title=test2", containerFiles: []expectedFiles{ { - file: "/test/test2", + file: "/test", + content: artifactContent2, + }, + }, + }, + { + name: "multi blob filter by title on existing file", + mountOpts: "src=" + ARTIFACT_MULTI + ",dst=/tmp,title=test2", + containerFiles: []expectedFiles{ + { + file: "/tmp/test2", content: artifactContent2, }, }, }, { name: "multi blob filter by digest", - mountOpts: "src=" + ARTIFACT_MULTI + ",dst=/test,digest=sha256:8257bba28b9d19ac353c4b713b470860278857767935ef7e139afd596cb1bb2d", + mountOpts: "src=" + ARTIFACT_MULTI + ",dst=/tmp,digest=sha256:8257bba28b9d19ac353c4b713b470860278857767935ef7e139afd596cb1bb2d", containerFiles: []expectedFiles{ { - file: "/test/sha256-8257bba28b9d19ac353c4b713b470860278857767935ef7e139afd596cb1bb2d", + file: "/tmp/sha256-8257bba28b9d19ac353c4b713b470860278857767935ef7e139afd596cb1bb2d", content: artifactContent1, }, }, @@ -152,12 +162,12 @@ var _ = Describe("Podman artifact mount", func() { podmanTest.PodmanExitCleanly("artifact", "add", artifactName, artifactFile) // FIXME: we need https://github.com/containers/container-selinux/pull/360 to fix the selinux access problem, until then disable it. - podmanTest.PodmanExitCleanly("run", "--security-opt=label=disable", "--name", ctrName, "-d", "--mount", "type=artifact,src="+artifactName+",dst=/test", ALPINE, "sleep", "100") + podmanTest.PodmanExitCleanly("run", "--security-opt=label=disable", "--name", ctrName, "-d", "--mount", "type=artifact,src="+artifactName+",dst=/tmp", ALPINE, "sleep", "100") podmanTest.PodmanExitCleanly("artifact", "rm", artifactName) // file must sill be readable after artifact removal - session := podmanTest.PodmanExitCleanly("exec", ctrName, "cat", "/test/"+artifactFileName) + session := podmanTest.PodmanExitCleanly("exec", ctrName, "cat", "/tmp/"+artifactFileName) Expect(session.OutputToString()).To(Equal("hello world")) // restart will fail if artifact does not exist @@ -174,7 +184,7 @@ var _ = Describe("Podman artifact mount", func() { podmanTest.PodmanExitCleanly("artifact", "add", artifactName, artifactFile, artifactFile2) podmanTest.PodmanExitCleanly("start", ctrName) - session = podmanTest.PodmanExitCleanly("exec", ctrName, "cat", "/test/"+artifactFileName, "/test/"+artifactFile2Name) + session = podmanTest.PodmanExitCleanly("exec", ctrName, "cat", "/tmp/"+artifactFileName, "/tmp/"+artifactFile2Name) Expect(session.OutputToString()).To(Equal("hello world second file")) }) From 0ab8a3c57697f17259f8ab11c3656429193e87fd Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Sat, 14 Jun 2025 10:03:41 +0200 Subject: [PATCH 2/2] artifact mount: add new name option to specify filename An artifact without the title annoation just gets the digest as name which is less than ideal. While it is a decent default to avoid conflicts users would like to configure the name. With the name=abc option we will call the file abc in case of a signle artifact and otherwise we use abc-x where x is the layer index starting at 0 to avoid conflicts. Signed-off-by: Paul Holzinger --- docs/source/markdown/options/mount.md | 6 ++++++ libpod/container.go | 5 +++++ libpod/container_internal_common.go | 13 +++++++++++-- pkg/specgen/generate/container_create.go | 1 + pkg/specgen/volumes.go | 5 +++++ pkg/specgenutil/volumes.go | 5 +++++ test/e2e/artifact_mount_test.go | 19 +++++++++++++++++++ 7 files changed, 52 insertions(+), 2 deletions(-) diff --git a/docs/source/markdown/options/mount.md b/docs/source/markdown/options/mount.md index b70ab873cad..ada1fb86704 100644 --- a/docs/source/markdown/options/mount.md +++ b/docs/source/markdown/options/mount.md @@ -32,6 +32,12 @@ Options specific to type=**artifact**: - *title*: If the artifact source contains multiple blobs a title can be set which is compared against `org.opencontainers.image.title` annotation. +- *name*: This can be used to overwrite the filename we use inside the container + for mounting. On a single blob artifact the name is used as is if *dst* is a + directory and otherwise ignored. With a multi blob artifact the name will be + used with an index suffix `-x` where x is the layer index in the artifact + starting with 0. + The *src* argument contains the name of the artifact, which must already exist locally. The *dst* argument contains the target path, if the path in the container is a directory the blob title (`org.opencontainers.image.title` annotation) will be used as diff --git a/libpod/container.go b/libpod/container.go index ff5839a549b..1f5d2298a6b 100644 --- a/libpod/container.go +++ b/libpod/container.go @@ -300,6 +300,11 @@ type ContainerArtifactVolume struct { // the title annotation exist. // Optional. Conflicts with Title. Digest string `json:"digest"` + // Name is the name that should be used for the path inside the container. When a single blob + // is mounted the name is used as is. If multiple blobs are mounted then mount them as + // "-x" where x is a 0 indexed integer based on the layer order. + // Optional. + Name string `json:"name,omitempty"` } // ContainerSecret is a secret that is mounted in a container diff --git a/libpod/container_internal_common.go b/libpod/container_internal_common.go index f5222b7a933..efb6074d726 100644 --- a/libpod/container_internal_common.go +++ b/libpod/container_internal_common.go @@ -564,12 +564,21 @@ func (c *Container) generateSpec(ctx context.Context) (s *spec.Spec, cleanupFunc return nil, nil, fmt.Errorf("artifact %q contains more than one blob and container path %q is a file", artifactMount.Source, artifactMount.Dest) } - for _, path := range paths { + for i, path := range paths { var dest string if destIsFile { dest = artifactMount.Dest } else { - dest = filepath.Join(artifactMount.Dest, path.Name) + var filename string + if artifactMount.Name != "" { + filename = artifactMount.Name + if len(paths) > 1 { + filename += "-" + strconv.Itoa(i) + } + } else { + filename = path.Name + } + dest = filepath.Join(artifactMount.Dest, filename) } logrus.Debugf("Mounting artifact %q in container %s, mount blob %q to %q", artifactMount.Source, c.ID(), path.SourcePath, dest) diff --git a/pkg/specgen/generate/container_create.go b/pkg/specgen/generate/container_create.go index 06e3ce5074e..119533d1b0f 100644 --- a/pkg/specgen/generate/container_create.go +++ b/pkg/specgen/generate/container_create.go @@ -515,6 +515,7 @@ func createContainerOptions(rt *libpod.Runtime, s *specgen.SpecGenerator, pod *l Source: v.Source, Digest: v.Digest, Title: v.Title, + Name: v.Name, }) } options = append(options, libpod.WithArtifactVolumes(vols)) diff --git a/pkg/specgen/volumes.go b/pkg/specgen/volumes.go index 786f271c833..3243333da51 100644 --- a/pkg/specgen/volumes.go +++ b/pkg/specgen/volumes.go @@ -78,6 +78,11 @@ type ArtifactVolume struct { // the title annotation exist. // Optional. Conflicts with Title. Digest string `json:"digest,omitempty"` + // Name is the name that should be used for the path inside the container. When a single blob + // is mounted the name is used as is. If multiple blobs are mounted then mount them as + // "-x" where x is a 0 indexed integer based on the layer order. + // Optional. + Name string `json:"name,omitempty"` } // GenVolumeMounts parses user input into mounts, volumes and overlay volumes diff --git a/pkg/specgenutil/volumes.go b/pkg/specgenutil/volumes.go index 02a7627a361..69366a1e902 100644 --- a/pkg/specgenutil/volumes.go +++ b/pkg/specgenutil/volumes.go @@ -747,6 +747,11 @@ func getArtifactVolume(args []string) (*specgen.ArtifactVolume, error) { return nil, fmt.Errorf("%v: %w", name, errOptionArg) } newVolume.Digest = value + case "name": + if !hasValue { + return nil, fmt.Errorf("%v: %w", name, errOptionArg) + } + newVolume.Name = value default: return nil, fmt.Errorf("%s: %w", name, util.ErrBadMntOption) } diff --git a/test/e2e/artifact_mount_test.go b/test/e2e/artifact_mount_test.go index e8b5a7ca2a2..4b71df43d63 100644 --- a/test/e2e/artifact_mount_test.go +++ b/test/e2e/artifact_mount_test.go @@ -46,6 +46,11 @@ var _ = Describe("Podman artifact mount", func() { mountOpts: "dst=/data,digest=sha256:e9510923578af3632946ecf5ae479c1b5f08b47464e707b5cbab9819272a9752", containerFile: "/data", }, + { + name: "single artifact mount with name", + mountOpts: "dst=/tmp,name=abcd", + containerFile: "/tmp/abcd", + }, } for _, tt := range tests { @@ -133,6 +138,20 @@ var _ = Describe("Podman artifact mount", func() { }, }, }, + { + name: "multi blob with name", + mountOpts: "src=" + ARTIFACT_MULTI + ",dst=/test,name=myname", + containerFiles: []expectedFiles{ + { + file: "/test/myname-0", + content: artifactContent1, + }, + { + file: "/test/myname-1", + content: artifactContent2, + }, + }, + }, } for _, tt := range tests { By(tt.name)