Merge pull request #26430 from Luap99/artifact-mount-name

artifact mount: improve single file behavior and add name option to specify a custom container name

docs/source/markdown/options/mount.md

@@ -32,17 +32,28 @@ 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 `<name>-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 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*).

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
+	// "<name>-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

libpod/container_internal_common.go

@@ -554,19 +554,31 @@ 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)
 			}
 
-			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)

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))

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
+	// "<name>-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

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)
 		}

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,12 @@ var _ = Describe("Podman artifact mount", func() {
 			{
 				name:          "single artifact mount with digest",
 				mountOpts:     "dst=/data,digest=sha256:e9510923578af3632946ecf5ae479c1b5f08b47464e707b5cbab9819272a9752",
-				containerFile: "/data/sha256-e9510923578af3632946ecf5ae479c1b5f08b47464e707b5cbab9819272a9752",
+				containerFile: "/data",
+			},
+			{
+				name:          "single artifact mount with name",
+				mountOpts:     "dst=/tmp,name=abcd",
+				containerFile: "/tmp/abcd",
 			},
 		}
 
@@ -104,25 +109,49 @@ 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,
 					},
 				},
 			},
+			{
+				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)
@@ -152,12 +181,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 +203,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"))
 	})