From 618e37794e504f70950e7a67412f3553f258a5e2 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 20 Feb 2025 18:42:14 +0100 Subject: [PATCH 01/11] libartifact: create FilterBlobOptions The main point of this is so that I can share the same lookup logic between Extract() and then the new blob path API I add next. Signed-off-by: Paul Holzinger --- pkg/domain/infra/abi/artifact.go | 6 ++++-- pkg/libartifact/store/store.go | 6 +++--- pkg/libartifact/types/config.go | 13 ++++++++++--- 3 files changed, 17 insertions(+), 8 deletions(-) diff --git a/pkg/domain/infra/abi/artifact.go b/pkg/domain/infra/abi/artifact.go index d66ec44561..fa70c28aac 100644 --- a/pkg/domain/infra/abi/artifact.go +++ b/pkg/domain/infra/abi/artifact.go @@ -215,8 +215,10 @@ func (ir *ImageEngine) ArtifactExtract(ctx context.Context, name string, target return err } extractOpt := &types.ExtractOptions{ - Digest: opts.Digest, - Title: opts.Title, + FilterBlobOptions: types.FilterBlobOptions{ + Digest: opts.Digest, + Title: opts.Title, + }, } return artStore.Extract(ctx, name, target, extractOpt) diff --git a/pkg/libartifact/store/store.go b/pkg/libartifact/store/store.go index 2224ce4066..eafeb87a67 100644 --- a/pkg/libartifact/store/store.go +++ b/pkg/libartifact/store/store.go @@ -364,7 +364,7 @@ func (as ArtifactStore) Extract(ctx context.Context, nameOrDigest string, target if len(options.Digest) == 0 && len(options.Title) == 0 { return fmt.Errorf("the artifact consists of several blobs and the target %q is not a directory and neither digest or title was specified to only copy a single blob", target) } - digest, err = findDigest(arty, options) + digest, err = findDigest(arty, &options.FilterBlobOptions) if err != nil { return err } @@ -376,7 +376,7 @@ func (as ArtifactStore) Extract(ctx context.Context, nameOrDigest string, target } if len(options.Digest) > 0 || len(options.Title) > 0 { - digest, err := findDigest(arty, options) + digest, err := findDigest(arty, &options.FilterBlobOptions) if err != nil { return err } @@ -427,7 +427,7 @@ func generateArtifactBlobName(title string, digest digest.Digest) (string, error return filename, nil } -func findDigest(arty *libartifact.Artifact, options *libartTypes.ExtractOptions) (digest.Digest, error) { +func findDigest(arty *libartifact.Artifact, options *libartTypes.FilterBlobOptions) (digest.Digest, error) { var digest digest.Digest for _, l := range arty.Manifest.Layers { if options.Digest == l.Digest.String() { diff --git a/pkg/libartifact/types/config.go b/pkg/libartifact/types/config.go index ab0a8eb3d6..2fb4fa79ab 100644 --- a/pkg/libartifact/types/config.go +++ b/pkg/libartifact/types/config.go @@ -12,9 +12,16 @@ type AddOptions struct { Append bool `json:",omitempty"` } -type ExtractOptions struct { - // Title annotation value to extract only a single blob matching that name. Optional. +// FilterBlobOptions options used to filter for a single blob in an artifact +type FilterBlobOptions struct { + // Title annotation value to extract only a single blob matching that name. + // Optional. Conflicts with Digest. Title string - // Digest of the blob to extract. Optional. + // Digest of the blob to extract. + // Optional. Conflicts with Title. Digest string } + +type ExtractOptions struct { + FilterBlobOptions +} From 4cd19b7f7aa401cf98f185e36a8d71ec15f4cafe Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 20 Feb 2025 18:45:03 +0100 Subject: [PATCH 02/11] libartifact: fix comment on Extract() Signed-off-by: Paul Holzinger --- pkg/libartifact/store/store.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/libartifact/store/store.go b/pkg/libartifact/store/store.go index eafeb87a67..f527d01227 100644 --- a/pkg/libartifact/store/store.go +++ b/pkg/libartifact/store/store.go @@ -312,7 +312,7 @@ func (as ArtifactStore) Add(ctx context.Context, dest string, paths []string, op return &artifactManifestDigest, nil } -// Inspect an artifact in a local store +// Extract an artifact to local file or directory func (as ArtifactStore) Extract(ctx context.Context, nameOrDigest string, target string, options *libartTypes.ExtractOptions) error { if len(options.Digest) > 0 && len(options.Title) > 0 { return errors.New("cannot specify both digest and title") From 86a6539b764df2da20cac33ce5c1067d39e00113 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 20 Feb 2025 19:04:30 +0100 Subject: [PATCH 03/11] libartifact: extract common code into helper Create a getArtifactAndImageSource() function so this one can be shared with the new mount blob API that is added next to avoid code duplication. Signed-off-by: Paul Holzinger --- pkg/libartifact/store/store.go | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/pkg/libartifact/store/store.go b/pkg/libartifact/store/store.go index f527d01227..62b0768c44 100644 --- a/pkg/libartifact/store/store.go +++ b/pkg/libartifact/store/store.go @@ -312,23 +312,22 @@ func (as ArtifactStore) Add(ctx context.Context, dest string, paths []string, op return &artifactManifestDigest, nil } -// Extract an artifact to local file or directory -func (as ArtifactStore) Extract(ctx context.Context, nameOrDigest string, target string, options *libartTypes.ExtractOptions) error { +func getArtifactAndImageSource(ctx context.Context, as ArtifactStore, nameOrDigest string, options *libartTypes.FilterBlobOptions) (*libartifact.Artifact, types.ImageSource, error) { if len(options.Digest) > 0 && len(options.Title) > 0 { - return errors.New("cannot specify both digest and title") + return nil, nil, errors.New("cannot specify both digest and title") } if len(nameOrDigest) == 0 { - return ErrEmptyArtifactName + return nil, nil, ErrEmptyArtifactName } artifacts, err := as.getArtifacts(ctx, nil) if err != nil { - return err + return nil, nil, err } arty, nameIsDigest, err := artifacts.GetByNameOrDigest(nameOrDigest) if err != nil { - return err + return nil, nil, err } name := nameOrDigest if nameIsDigest { @@ -336,14 +335,20 @@ func (as ArtifactStore) Extract(ctx context.Context, nameOrDigest string, target } if len(arty.Manifest.Layers) == 0 { - return fmt.Errorf("the artifact has no blobs, nothing to extract") + return nil, nil, fmt.Errorf("the artifact has no blobs, nothing to extract") } ir, err := layout.NewReference(as.storePath, name) if err != nil { - return err + return nil, nil, err } imgSrc, err := ir.NewImageSource(ctx, as.SystemContext) + return arty, imgSrc, err +} + +// Extract an artifact to local file or directory +func (as ArtifactStore) Extract(ctx context.Context, nameOrDigest string, target string, options *libartTypes.ExtractOptions) error { + arty, imgSrc, err := getArtifactAndImageSource(ctx, as, nameOrDigest, &options.FilterBlobOptions) if err != nil { return err } From 7c200a5f4cce5a4552b68f72a050478abe59f84b Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 20 Feb 2025 19:12:57 +0100 Subject: [PATCH 04/11] libartifact: add BlobMountPaths() The goal of this new interface is to expose the blob source path and the target file name for a bind mount into a container. libpod will call this and then take care of setting up the actual mounts based on the returned paths. Signed-off-by: Paul Holzinger --- pkg/libartifact/store/store.go | 50 +++++++++++++++++++++++++++++++++ pkg/libartifact/types/config.go | 12 ++++++++ 2 files changed, 62 insertions(+) diff --git a/pkg/libartifact/store/store.go b/pkg/libartifact/store/store.go index 62b0768c44..7d3cf9e8d4 100644 --- a/pkg/libartifact/store/store.go +++ b/pkg/libartifact/store/store.go @@ -346,6 +346,56 @@ func getArtifactAndImageSource(ctx context.Context, as ArtifactStore, nameOrDige return arty, imgSrc, err } +// BlobMountPaths allows the caller to access the file names from the store and how they should be mounted. +func (as ArtifactStore) BlobMountPaths(ctx context.Context, nameOrDigest string, options *libartTypes.BlobMountPathOptions) ([]libartTypes.BlobMountPath, error) { + arty, imgSrc, err := getArtifactAndImageSource(ctx, as, nameOrDigest, &options.FilterBlobOptions) + if err != nil { + return nil, err + } + defer imgSrc.Close() + + if len(options.Digest) > 0 || len(options.Title) > 0 { + digest, err := findDigest(arty, &options.FilterBlobOptions) + if err != nil { + return nil, err + } + // In case the digest is set we always use it as target name + // so we do not have to get the actual title annotation form the blob. + // Passing options.Title is enough because we know it is empty when digest + // is set as we only allow either one. + filename, err := generateArtifactBlobName(options.Title, digest) + if err != nil { + return nil, err + } + path, err := layout.GetLocalBlobPath(ctx, imgSrc, digest) + if err != nil { + return nil, err + } + return []libartTypes.BlobMountPath{{ + SourcePath: path, + Name: filename, + }}, nil + } + + mountPaths := make([]libartTypes.BlobMountPath, 0, len(arty.Manifest.Layers)) + for _, l := range arty.Manifest.Layers { + title := l.Annotations[specV1.AnnotationTitle] + filename, err := generateArtifactBlobName(title, l.Digest) + if err != nil { + return nil, err + } + path, err := layout.GetLocalBlobPath(ctx, imgSrc, l.Digest) + if err != nil { + return nil, err + } + mountPaths = append(mountPaths, libartTypes.BlobMountPath{ + SourcePath: path, + Name: filename, + }) + } + return mountPaths, nil +} + // Extract an artifact to local file or directory func (as ArtifactStore) Extract(ctx context.Context, nameOrDigest string, target string, options *libartTypes.ExtractOptions) error { arty, imgSrc, err := getArtifactAndImageSource(ctx, as, nameOrDigest, &options.FilterBlobOptions) diff --git a/pkg/libartifact/types/config.go b/pkg/libartifact/types/config.go index 2fb4fa79ab..cff62a3709 100644 --- a/pkg/libartifact/types/config.go +++ b/pkg/libartifact/types/config.go @@ -25,3 +25,15 @@ type FilterBlobOptions struct { type ExtractOptions struct { FilterBlobOptions } + +type BlobMountPathOptions struct { + FilterBlobOptions +} + +// BlobMountPath contains the info on how the artifact must be mounted +type BlobMountPath struct { + // Source path of the blob, i.e. full path in the blob dir. + SourcePath string + // Name of the file in the container. + Name string +} From b232ea3d18be38ebb0296fa43b655cce5bb21fb2 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Tue, 25 Feb 2025 11:49:31 +0100 Subject: [PATCH 05/11] create artifact store in the libpod runtime Instead of duplicating the NewArtifactStore() call in many places and having to make sure we always pass the same path to it define it as function on the runtime. This allows any caller with access to the libpod runtime to create the store easily. This is suing a sync.OnceValues() function so the store is initialized only once and only when actually needed. Signed-off-by: Paul Holzinger --- libpod/runtime.go | 9 +++++++++ pkg/domain/infra/abi/artifact.go | 20 +++++++------------- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/libpod/runtime.go b/libpod/runtime.go index fc358de026..993099cd58 100644 --- a/libpod/runtime.go +++ b/libpod/runtime.go @@ -34,6 +34,7 @@ import ( "github.com/containers/podman/v5/libpod/shutdown" "github.com/containers/podman/v5/pkg/domain/entities" "github.com/containers/podman/v5/pkg/domain/entities/reports" + artStore "github.com/containers/podman/v5/pkg/libartifact/store" "github.com/containers/podman/v5/pkg/rootless" "github.com/containers/podman/v5/pkg/systemd" "github.com/containers/podman/v5/pkg/util" @@ -83,6 +84,9 @@ type Runtime struct { libimageEventsShutdown chan bool lockManager lock.Manager + // ArtifactStore returns the artifact store created from the runtime. + ArtifactStore func() (*artStore.ArtifactStore, error) + // Worker workerChannel chan func() workerGroup sync.WaitGroup @@ -533,6 +537,11 @@ func makeRuntime(ctx context.Context, runtime *Runtime) (retErr error) { } runtime.config.Network.NetworkBackend = string(netBackend) runtime.network = netInterface + + // Using sync once value to only init the store exactly once and only when it will be actually be used. + runtime.ArtifactStore = sync.OnceValues(func() (*artStore.ArtifactStore, error) { + return artStore.NewArtifactStore(filepath.Join(runtime.storageConfig.GraphRoot, "artifacts"), runtime.SystemContext()) + }) } // We now need to see if the system has restarted diff --git a/pkg/domain/infra/abi/artifact.go b/pkg/domain/infra/abi/artifact.go index fa70c28aac..0fd3c09b7d 100644 --- a/pkg/domain/infra/abi/artifact.go +++ b/pkg/domain/infra/abi/artifact.go @@ -5,22 +5,16 @@ package abi import ( "context" "os" - "path/filepath" "time" "github.com/containers/common/libimage" "github.com/containers/podman/v5/pkg/domain/entities" - "github.com/containers/podman/v5/pkg/libartifact/store" "github.com/containers/podman/v5/pkg/libartifact/types" "github.com/opencontainers/go-digest" ) -func getDefaultArtifactStore(ir *ImageEngine) string { - return filepath.Join(ir.Libpod.StorageConfig().GraphRoot, "artifacts") -} - func (ir *ImageEngine) ArtifactInspect(ctx context.Context, name string, _ entities.ArtifactInspectOptions) (*entities.ArtifactInspectReport, error) { - artStore, err := store.NewArtifactStore(getDefaultArtifactStore(ir), ir.Libpod.SystemContext()) + artStore, err := ir.Libpod.ArtifactStore() if err != nil { return nil, err } @@ -41,7 +35,7 @@ func (ir *ImageEngine) ArtifactInspect(ctx context.Context, name string, _ entit func (ir *ImageEngine) ArtifactList(ctx context.Context, _ entities.ArtifactListOptions) ([]*entities.ArtifactListReport, error) { reports := make([]*entities.ArtifactListReport, 0) - artStore, err := store.NewArtifactStore(getDefaultArtifactStore(ir), ir.Libpod.SystemContext()) + artStore, err := ir.Libpod.ArtifactStore() if err != nil { return nil, err } @@ -80,7 +74,7 @@ func (ir *ImageEngine) ArtifactPull(ctx context.Context, name string, opts entit if !opts.Quiet && pullOptions.Writer == nil { pullOptions.Writer = os.Stderr } - artStore, err := store.NewArtifactStore(getDefaultArtifactStore(ir), ir.Libpod.SystemContext()) + artStore, err := ir.Libpod.ArtifactStore() if err != nil { return nil, err } @@ -92,7 +86,7 @@ func (ir *ImageEngine) ArtifactRm(ctx context.Context, name string, opts entitie namesOrDigests []string ) artifactDigests := make([]*digest.Digest, 0, len(namesOrDigests)) - artStore, err := store.NewArtifactStore(getDefaultArtifactStore(ir), ir.Libpod.SystemContext()) + artStore, err := ir.Libpod.ArtifactStore() if err != nil { return nil, err } @@ -133,7 +127,7 @@ func (ir *ImageEngine) ArtifactRm(ctx context.Context, name string, opts entitie func (ir *ImageEngine) ArtifactPush(ctx context.Context, name string, opts entities.ArtifactPushOptions) (*entities.ArtifactPushReport, error) { var retryDelay *time.Duration - artStore, err := store.NewArtifactStore(getDefaultArtifactStore(ir), ir.Libpod.SystemContext()) + artStore, err := ir.Libpod.ArtifactStore() if err != nil { return nil, err } @@ -189,7 +183,7 @@ func (ir *ImageEngine) ArtifactPush(ctx context.Context, name string, opts entit return &entities.ArtifactPushReport{}, err } func (ir *ImageEngine) ArtifactAdd(ctx context.Context, name string, paths []string, opts *entities.ArtifactAddOptions) (*entities.ArtifactAddReport, error) { - artStore, err := store.NewArtifactStore(getDefaultArtifactStore(ir), ir.Libpod.SystemContext()) + artStore, err := ir.Libpod.ArtifactStore() if err != nil { return nil, err } @@ -210,7 +204,7 @@ func (ir *ImageEngine) ArtifactAdd(ctx context.Context, name string, paths []str } func (ir *ImageEngine) ArtifactExtract(ctx context.Context, name string, target string, opts *entities.ArtifactExtractOptions) error { - artStore, err := store.NewArtifactStore(getDefaultArtifactStore(ir), ir.Libpod.SystemContext()) + artStore, err := ir.Libpod.ArtifactStore() if err != nil { return err } From 51cfcc65d5dd0337ff9333eacdd75bc565735caf Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Tue, 25 Feb 2025 19:09:29 +0100 Subject: [PATCH 06/11] correctly preallocate artifactDigests in ArtifactRm() Will safe a few memory copies, we must do that only after namesOrDigests was populated so the len() does not report zero. Signed-off-by: Paul Holzinger --- pkg/domain/infra/abi/artifact.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/domain/infra/abi/artifact.go b/pkg/domain/infra/abi/artifact.go index 0fd3c09b7d..a867a1d2ff 100644 --- a/pkg/domain/infra/abi/artifact.go +++ b/pkg/domain/infra/abi/artifact.go @@ -85,7 +85,6 @@ func (ir *ImageEngine) ArtifactRm(ctx context.Context, name string, opts entitie var ( namesOrDigests []string ) - artifactDigests := make([]*digest.Digest, 0, len(namesOrDigests)) artStore, err := ir.Libpod.ArtifactStore() if err != nil { return nil, err @@ -111,6 +110,7 @@ func (ir *ImageEngine) ArtifactRm(ctx context.Context, name string, opts entitie namesOrDigests = append(namesOrDigests, name) } + artifactDigests := make([]*digest.Digest, 0, len(namesOrDigests)) for _, namesOrDigest := range namesOrDigests { artifactDigest, err := artStore.Remove(ctx, namesOrDigest) if err != nil { From 590bf8b79d47c560e89849841c2c83fbaeea2ffb Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Tue, 25 Feb 2025 12:02:40 +0100 Subject: [PATCH 07/11] pkg/specgenutil: unexport Mounts() The function is never used elsewhere so do not export it. Signed-off-by: Paul Holzinger --- pkg/specgenutil/volumes.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/specgenutil/volumes.go b/pkg/specgenutil/volumes.go index fe45fd2f47..2e4e6efe0c 100644 --- a/pkg/specgenutil/volumes.go +++ b/pkg/specgenutil/volumes.go @@ -37,7 +37,7 @@ type universalMount struct { func parseVolumes(rtc *config.Config, volumeFlag, mountFlag, tmpfsFlag []string) ([]spec.Mount, []*specgen.NamedVolume, []*specgen.OverlayVolume, []*specgen.ImageVolume, error) { // Get mounts from the --mounts flag. // TODO: The runtime config part of this needs to move into pkg/specgen/generate to avoid querying containers.conf on the client. - unifiedMounts, unifiedVolumes, unifiedImageVolumes, err := Mounts(mountFlag, rtc.Mounts()) + unifiedMounts, unifiedVolumes, unifiedImageVolumes, err := mounts(mountFlag, rtc.Mounts()) if err != nil { return nil, nil, nil, nil, err } @@ -146,12 +146,12 @@ func parseVolumes(rtc *config.Config, volumeFlag, mountFlag, tmpfsFlag []string) return finalMounts, finalVolumes, finalOverlayVolume, finalImageVolumes, nil } -// Mounts takes user-provided input from the --mount flag as well as Mounts +// mounts takes user-provided input from the --mount flag as well as mounts // specified in containers.conf and creates OCI spec mounts and Libpod named volumes. // podman run --mount type=bind,src=/etc/resolv.conf,target=/etc/resolv.conf ... // podman run --mount type=tmpfs,target=/dev/shm ... // podman run --mount type=volume,source=test-volume, ... -func Mounts(mountFlag []string, configMounts []string) (map[string]spec.Mount, map[string]*specgen.NamedVolume, map[string]*specgen.ImageVolume, error) { +func mounts(mountFlag []string, configMounts []string) (map[string]spec.Mount, map[string]*specgen.NamedVolume, map[string]*specgen.ImageVolume, error) { finalMounts := make(map[string]spec.Mount) finalNamedVolumes := make(map[string]*specgen.NamedVolume) finalImageVolumes := make(map[string]*specgen.ImageVolume) From fe82fa85d2d2b7bebaab3845ea4511a120b0518c Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Tue, 25 Feb 2025 12:24:14 +0100 Subject: [PATCH 08/11] pkg/specgenutil: rework volume/mount parsing Use a helper struct to hold the mounts instead of returning 5+ return values from the functions. This allows use to easily add more volume types without having to update all return lines every time in the future. And 5+ return values are really not readable anymore so this should make it easier to follow the code. Signed-off-by: Paul Holzinger --- pkg/specgenutil/specgen.go | 10 ++--- pkg/specgenutil/volumes.go | 91 ++++++++++++++++++++++++-------------- 2 files changed, 62 insertions(+), 39 deletions(-) diff --git a/pkg/specgenutil/specgen.go b/pkg/specgenutil/specgen.go index f516a8ab14..f3ee9877cf 100644 --- a/pkg/specgenutil/specgen.go +++ b/pkg/specgenutil/specgen.go @@ -762,15 +762,15 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *entities.ContainerCreateOptions // Only add read-only tmpfs mounts in case that we are read-only and the // read-only tmpfs flag has been set. - mounts, volumes, overlayVolumes, imageVolumes, err := parseVolumes(rtc, c.Volume, c.Mount, c.TmpFS) + containerMounts, err := parseVolumes(rtc, c.Volume, c.Mount, c.TmpFS) if err != nil { return err } if len(s.Mounts) == 0 || len(c.Mount) != 0 { - s.Mounts = mounts + s.Mounts = containerMounts.mounts } if len(s.Volumes) == 0 || len(c.Volume) != 0 { - s.Volumes = volumes + s.Volumes = containerMounts.volumes } if s.LabelNested != nil && *s.LabelNested { @@ -785,10 +785,10 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *entities.ContainerCreateOptions } // TODO make sure these work in clone if len(s.OverlayVolumes) == 0 { - s.OverlayVolumes = overlayVolumes + s.OverlayVolumes = containerMounts.overlayVolumes } if len(s.ImageVolumes) == 0 { - s.ImageVolumes = imageVolumes + s.ImageVolumes = containerMounts.imageVolumes } devices := c.Devices diff --git a/pkg/specgenutil/volumes.go b/pkg/specgenutil/volumes.go index 2e4e6efe0c..1dea8fa2a4 100644 --- a/pkg/specgenutil/volumes.go +++ b/pkg/specgenutil/volumes.go @@ -22,6 +22,20 @@ var ( errNoDest = errors.New("must set volume destination") ) +type containerMountSlice struct { + mounts []spec.Mount + volumes []*specgen.NamedVolume + overlayVolumes []*specgen.OverlayVolume + imageVolumes []*specgen.ImageVolume +} + +// containerMountMap contains the container mounts with the destination path as map key +type containerMountMap struct { + mounts map[string]spec.Mount + volumes map[string]*specgen.NamedVolume + imageVolumes map[string]*specgen.ImageVolume +} + type universalMount struct { mount spec.Mount // Used only with Named Volume type mounts @@ -34,57 +48,57 @@ type universalMount struct { // Does not handle image volumes, init, and --volumes-from flags. // Can also add tmpfs mounts from read-only tmpfs. // TODO: handle options parsing/processing via containers/storage/pkg/mount -func parseVolumes(rtc *config.Config, volumeFlag, mountFlag, tmpfsFlag []string) ([]spec.Mount, []*specgen.NamedVolume, []*specgen.OverlayVolume, []*specgen.ImageVolume, error) { +func parseVolumes(rtc *config.Config, volumeFlag, mountFlag, tmpfsFlag []string) (*containerMountSlice, error) { // Get mounts from the --mounts flag. // TODO: The runtime config part of this needs to move into pkg/specgen/generate to avoid querying containers.conf on the client. - unifiedMounts, unifiedVolumes, unifiedImageVolumes, err := mounts(mountFlag, rtc.Mounts()) + unifiedContainerMounts, err := mounts(mountFlag, rtc.Mounts()) if err != nil { - return nil, nil, nil, nil, err + return nil, err } // Next --volumes flag. volumeMounts, volumeVolumes, overlayVolumes, err := specgen.GenVolumeMounts(volumeFlag) if err != nil { - return nil, nil, nil, nil, err + return nil, err } // Next --tmpfs flag. tmpfsMounts, err := getTmpfsMounts(tmpfsFlag) if err != nil { - return nil, nil, nil, nil, err + return nil, err } // Unify mounts from --mount, --volume, --tmpfs. // Start with --volume. for dest, mount := range volumeMounts { - if vol, ok := unifiedMounts[dest]; ok { + if vol, ok := unifiedContainerMounts.mounts[dest]; ok { if mount.Source == vol.Source && specgen.StringSlicesEqual(vol.Options, mount.Options) { continue } - return nil, nil, nil, nil, fmt.Errorf("%v: %w", dest, specgen.ErrDuplicateDest) + return nil, fmt.Errorf("%v: %w", dest, specgen.ErrDuplicateDest) } - unifiedMounts[dest] = mount + unifiedContainerMounts.mounts[dest] = mount } for dest, volume := range volumeVolumes { - if vol, ok := unifiedVolumes[dest]; ok { + if vol, ok := unifiedContainerMounts.volumes[dest]; ok { if volume.Name == vol.Name && specgen.StringSlicesEqual(vol.Options, volume.Options) { continue } - return nil, nil, nil, nil, fmt.Errorf("%v: %w", dest, specgen.ErrDuplicateDest) + return nil, fmt.Errorf("%v: %w", dest, specgen.ErrDuplicateDest) } - unifiedVolumes[dest] = volume + unifiedContainerMounts.volumes[dest] = volume } // Now --tmpfs for dest, tmpfs := range tmpfsMounts { - if vol, ok := unifiedMounts[dest]; ok { + if vol, ok := unifiedContainerMounts.mounts[dest]; ok { if vol.Type != define.TypeTmpfs { - return nil, nil, nil, nil, fmt.Errorf("%v: %w", dest, specgen.ErrDuplicateDest) + return nil, fmt.Errorf("%v: %w", dest, specgen.ErrDuplicateDest) } continue } - unifiedMounts[dest] = tmpfs + unifiedContainerMounts.mounts[dest] = tmpfs } // Check for conflicts between named volumes, overlay & image volumes, @@ -97,53 +111,58 @@ func parseVolumes(rtc *config.Config, volumeFlag, mountFlag, tmpfsFlag []string) allMounts[dest] = true return nil } - for dest := range unifiedMounts { + for dest := range unifiedContainerMounts.mounts { if err := testAndSet(dest); err != nil { - return nil, nil, nil, nil, err + return nil, err } } - for dest := range unifiedVolumes { + for dest := range unifiedContainerMounts.volumes { if err := testAndSet(dest); err != nil { - return nil, nil, nil, nil, err + return nil, err } } for dest := range overlayVolumes { if err := testAndSet(dest); err != nil { - return nil, nil, nil, nil, err + return nil, err } } - for dest := range unifiedImageVolumes { + for dest := range unifiedContainerMounts.imageVolumes { if err := testAndSet(dest); err != nil { - return nil, nil, nil, nil, err + return nil, err } } // Final step: maps to arrays - finalMounts := make([]spec.Mount, 0, len(unifiedMounts)) - for _, mount := range unifiedMounts { + finalMounts := make([]spec.Mount, 0, len(unifiedContainerMounts.mounts)) + for _, mount := range unifiedContainerMounts.mounts { if mount.Type == define.TypeBind { absSrc, err := specgen.ConvertWinMountPath(mount.Source) if err != nil { - return nil, nil, nil, nil, fmt.Errorf("getting absolute path of %s: %w", mount.Source, err) + return nil, fmt.Errorf("getting absolute path of %s: %w", mount.Source, err) } mount.Source = absSrc } finalMounts = append(finalMounts, mount) } - finalVolumes := make([]*specgen.NamedVolume, 0, len(unifiedVolumes)) - for _, volume := range unifiedVolumes { + finalVolumes := make([]*specgen.NamedVolume, 0, len(unifiedContainerMounts.volumes)) + for _, volume := range unifiedContainerMounts.volumes { finalVolumes = append(finalVolumes, volume) } - finalOverlayVolume := make([]*specgen.OverlayVolume, 0) + finalOverlayVolume := make([]*specgen.OverlayVolume, 0, len(overlayVolumes)) for _, volume := range overlayVolumes { finalOverlayVolume = append(finalOverlayVolume, volume) } - finalImageVolumes := make([]*specgen.ImageVolume, 0, len(unifiedImageVolumes)) - for _, volume := range unifiedImageVolumes { + finalImageVolumes := make([]*specgen.ImageVolume, 0, len(unifiedContainerMounts.imageVolumes)) + for _, volume := range unifiedContainerMounts.imageVolumes { finalImageVolumes = append(finalImageVolumes, volume) } - return finalMounts, finalVolumes, finalOverlayVolume, finalImageVolumes, nil + return &containerMountSlice{ + mounts: finalMounts, + volumes: finalVolumes, + overlayVolumes: finalOverlayVolume, + imageVolumes: finalImageVolumes, + }, nil } // mounts takes user-provided input from the --mount flag as well as mounts @@ -151,7 +170,7 @@ func parseVolumes(rtc *config.Config, volumeFlag, mountFlag, tmpfsFlag []string) // podman run --mount type=bind,src=/etc/resolv.conf,target=/etc/resolv.conf ... // podman run --mount type=tmpfs,target=/dev/shm ... // podman run --mount type=volume,source=test-volume, ... -func mounts(mountFlag []string, configMounts []string) (map[string]spec.Mount, map[string]*specgen.NamedVolume, map[string]*specgen.ImageVolume, error) { +func mounts(mountFlag []string, configMounts []string) (*containerMountMap, error) { finalMounts := make(map[string]spec.Mount) finalNamedVolumes := make(map[string]*specgen.NamedVolume) finalImageVolumes := make(map[string]*specgen.ImageVolume) @@ -246,17 +265,21 @@ func mounts(mountFlag []string, configMounts []string) (map[string]spec.Mount, m // Parse mounts passed in from the user if err := parseMounts(mountFlag, false); err != nil { - return nil, nil, nil, err + return nil, err } // If user specified a mount flag that conflicts with a containers.conf flag, then ignore // the duplicate. This means that the parsing of the containers.conf configMounts should always // happen second. if err := parseMounts(configMounts, true); err != nil { - return nil, nil, nil, fmt.Errorf("parsing containers.conf mounts: %w", err) + return nil, fmt.Errorf("parsing containers.conf mounts: %w", err) } - return finalMounts, finalNamedVolumes, finalImageVolumes, nil + return &containerMountMap{ + mounts: finalMounts, + volumes: finalNamedVolumes, + imageVolumes: finalImageVolumes, + }, nil } func parseMountOptions(mountType string, args []string) (*universalMount, error) { From f6e2d9440942865c835a624f521af4d8322c9bde Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Tue, 25 Feb 2025 18:49:51 +0100 Subject: [PATCH 09/11] test/e2e: improve createArtifactFile() There is no need whatsoever to run container to populate a random file, this is just much slower than just writing some random bytes directly without having to run a container and run dd in it. Also the function accepted the number of bytes, however because dd uses a minimum block size of 512 bytes it was actually numBytes * 1024 which where written. That makes no sense so fix the two tests that depended on the wrong number. Signed-off-by: Paul Holzinger --- test/e2e/artifact_test.go | 4 ++-- test/e2e/common_test.go | 16 ++++++++++++---- 2 files changed, 14 insertions(+), 6 deletions(-) diff --git a/test/e2e/artifact_test.go b/test/e2e/artifact_test.go index 001b142392..e5b34d64b3 100644 --- a/test/e2e/artifact_test.go +++ b/test/e2e/artifact_test.go @@ -475,7 +475,7 @@ var _ = Describe("Podman artifact", func() { a := podmanTest.InspectArtifact(artifact1Name) Expect(a.Manifest.Layers).To(HaveLen(1)) - Expect(a.TotalSizeBytes()).To(Equal(int64(524288))) + Expect(a.TotalSizeBytes()).To(Equal(int64(1024))) }) It("podman artifact add file already exists in artifact", func() { @@ -506,7 +506,7 @@ var _ = Describe("Podman artifact", func() { a := podmanTest.InspectArtifact(artifact1Name) Expect(a.Manifest.Layers).To(HaveLen(1)) - Expect(a.TotalSizeBytes()).To(Equal(int64(1048576))) + Expect(a.TotalSizeBytes()).To(Equal(int64(2048))) }) It("podman artifact add with --append and --type", func() { diff --git a/test/e2e/common_test.go b/test/e2e/common_test.go index 3773c1739b..518a8f028d 100644 --- a/test/e2e/common_test.go +++ b/test/e2e/common_test.go @@ -5,6 +5,7 @@ package integration import ( "bufio" "bytes" + crand "crypto/rand" "encoding/json" "errors" "fmt" @@ -1390,6 +1391,7 @@ func writeYaml(content string, fileName string) error { func GetPort() int { portMin := 5000 portMax := 5999 + rng := rand.New(rand.NewSource(time.Now().UnixNano())) // Avoid dup-allocation races between parallel ginkgo processes @@ -1617,16 +1619,22 @@ func setupRegistry(portOverride *int) (*lockfile.LockFile, string, error) { } func createArtifactFile(numBytes int64) (string, error) { + GinkgoHelper() artifactDir := filepath.Join(podmanTest.TempDir, "artifacts") if err := os.MkdirAll(artifactDir, 0755); err != nil { return "", err } filename := RandomString(8) outFile := filepath.Join(artifactDir, filename) - session := podmanTest.Podman([]string{"run", "-v", fmt.Sprintf("%s:/artifacts:z", artifactDir), ALPINE, "dd", "if=/dev/urandom", fmt.Sprintf("of=%s", filepath.Join("/artifacts", filename)), "bs=1b", fmt.Sprintf("count=%d", numBytes)}) - session.WaitWithDefaultTimeout() - if session.ExitCode() != 0 { - return "", errors.New("unable to generate artifact file") + + f, err := os.Create(filepath.Join(artifactDir, filename)) + if err != nil { + return "", err + } + defer f.Close() + _, err = io.CopyN(f, crand.Reader, numBytes) + if err != nil { + return "", err } return outFile, nil } From 9e94dc53b204f70903b997e121a68614ab6140ea Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 20 Feb 2025 18:37:34 +0100 Subject: [PATCH 10/11] add new artifact mount type Add a new option to allow for mounting artifacts in the container, the syntax is added to the existing --mount option: type=artifact,src=$artifactName,dest=/path[,digest=x][,title=x] This works very similar to image mounts. The name is passed down into the container config and then on each start we lookup the artifact and the figure out which blobs to mount. There is no protaction against a user removing the artifact while still being used in a container. When the container is running the bind mounted files will stay there (as the kernel keeps the mounts active even if the bind source was deleted). On the next start it will fail to start as if it does not find the artifact. The good thing is that this technically allows someone to update the artifact with the new file by creating a new artifact with the same name. Signed-off-by: Paul Holzinger --- docs/source/markdown/options/mount.md | 27 ++- libpod/container.go | 22 +++ libpod/container_config.go | 2 + libpod/container_internal_common.go | 47 +++++ libpod/container_internal_freebsd.go | 18 ++ libpod/container_internal_linux.go | 16 ++ libpod/options.go | 13 ++ pkg/specgen/generate/container_create.go | 13 ++ pkg/specgen/specgen.go | 2 + pkg/specgen/volumes.go | 22 +++ pkg/specgenutil/specgen.go | 3 + pkg/specgenutil/volumes.go | 99 ++++++++-- test/e2e/artifact_mount_test.go | 223 +++++++++++++++++++++++ 13 files changed, 490 insertions(+), 17 deletions(-) create mode 100644 test/e2e/artifact_mount_test.go diff --git a/docs/source/markdown/options/mount.md b/docs/source/markdown/options/mount.md index e0e6e50c93..8e05ab804c 100644 --- a/docs/source/markdown/options/mount.md +++ b/docs/source/markdown/options/mount.md @@ -6,12 +6,12 @@ Attach a filesystem mount to the container -Current supported mount TYPEs are **bind**, **devpts**, **glob**, **image**, **ramfs**, **tmpfs** and **volume**. +Current supported mount TYPEs are **artifact**, **bind**, **devpts**, **glob**, **image**, **ramfs**, **tmpfs** and **volume**. Options common to all mount types: - *src*, *source*: mount source spec for **bind**, **glob**, and **volume**. - Mandatory for **bind** and **glob**. + Mandatory for **artifact**, **bind**, **glob**, **image** and **volume**. - *dst*, *destination*, *target*: mount destination spec. @@ -24,6 +24,25 @@ on the destination directory are mounted. The option to mount host files matching /foo* to the /tmp/bar/ directory in the container. +Options specific to type=**artifact**: + +- *digest*: If the artifact source contains multiple blobs a digest can be + specified to only mount the one specific blob with the digest. + +- *title*: If the artifact source contains multiple blobs a title can be set + which is compared against `org.opencontainers.image.title` annotation. + +The *src* argument contains the name of the artifact, it 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. + +However if the *dst* path is a existing file in the container then the blob will be +mounted directly on it. This only works when the artifact contains of a single blob +or when either *digest* or *title* are specified. + Options specific to type=**volume**: - *ro*, *readonly*: *true* or *false* (default if unspecified: *false*). @@ -104,4 +123,6 @@ Examples: - `type=tmpfs,destination=/path/in/container,noswap` -- `type=volume,source=vol1,destination=/path/in/container,ro=true` +- `type=artifact,src=quay.io/libpod/testartifact:20250206-single,dst=/data` + +- `type=artifact,src=quay.io/libpod/testartifact:20250206-multi,dst=/data,title=test1` diff --git a/libpod/container.go b/libpod/container.go index fb9ca11800..479ef80829 100644 --- a/libpod/container.go +++ b/libpod/container.go @@ -280,6 +280,28 @@ type ContainerImageVolume struct { SubPath string `json:"subPath,omitempty"` } +// ContainerArtifactVolume is a volume based on a artifact. The artifact blobs will +// be bind mounted directly as files and must always be read only. +type ContainerArtifactVolume struct { + // Source is the name or digest of the artifact that should be mounted + Source string `json:"source"` + // Dest is the absolute path of the mount in the container. + // If path is a file in the container, then the artifact must consist of a single blob. + // Otherwise if it is a directory or does not exists all artifact blobs will be mounted + // into this path as files. As name the "org.opencontainers.image.title" will be used if + // available otherwise the digest is used as name. + Dest string `json:"dest"` + // Title can be used for multi blob artifacts to only mount the one specific blob that + // matches the "org.opencontainers.image.title" annotation. + // Optional. Conflicts with Digest. + Title string `json:"title"` + // Digest can be used to filter a single blob from a multi blob artifact by the given digest. + // When this option is set the file name in the container defaults to the digest even when + // the title annotation exist. + // Optional. Conflicts with Title. + Digest string `json:"digest"` +} + // ContainerSecret is a secret that is mounted in a container type ContainerSecret struct { // Secret is the secret diff --git a/libpod/container_config.go b/libpod/container_config.go index 3ccfad43c1..888b4636e7 100644 --- a/libpod/container_config.go +++ b/libpod/container_config.go @@ -162,6 +162,8 @@ type ContainerRootFSConfig struct { // moved out of Libpod into pkg/specgen). // Please DO NOT reuse the `imageVolumes` name in container JSON again. ImageVolumes []*ContainerImageVolume `json:"ctrImageVolumes,omitempty"` + // ArtifactVolumes lists the artifact volumes to mount into the container. + ArtifactVolumes []*ContainerArtifactVolume `json:"artifactVolumes,omitempty"` // CreateWorkingDir indicates that Libpod should create the container's // working directory if it does not exist. Some OCI runtimes do this by // default, but others do not. diff --git a/libpod/container_internal_common.go b/libpod/container_internal_common.go index 5b8d898247..c240ff225e 100644 --- a/libpod/container_internal_common.go +++ b/libpod/container_internal_common.go @@ -41,6 +41,7 @@ import ( "github.com/containers/podman/v5/pkg/annotations" "github.com/containers/podman/v5/pkg/checkpoint/crutils" "github.com/containers/podman/v5/pkg/criu" + libartTypes "github.com/containers/podman/v5/pkg/libartifact/types" "github.com/containers/podman/v5/pkg/lookup" "github.com/containers/podman/v5/pkg/rootless" "github.com/containers/podman/v5/pkg/util" @@ -483,6 +484,52 @@ func (c *Container) generateSpec(ctx context.Context) (s *spec.Spec, cleanupFunc g.AddMount(overlayMount) } + if len(c.config.ArtifactVolumes) > 0 { + artStore, err := c.runtime.ArtifactStore() + if err != nil { + return nil, nil, err + } + for _, artifactMount := range c.config.ArtifactVolumes { + paths, err := artStore.BlobMountPaths(ctx, artifactMount.Source, &libartTypes.BlobMountPathOptions{ + FilterBlobOptions: libartTypes.FilterBlobOptions{ + Title: artifactMount.Title, + Digest: artifactMount.Digest, + }, + }) + if err != nil { + 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) + 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 { + var dest string + if destIsFile { + dest = artifactMount.Dest + } else { + dest = filepath.Join(artifactMount.Dest, path.Name) + } + + logrus.Debugf("Mounting artifact %q in container %s, mount blob %q to %q", artifactMount.Source, c.ID(), path.SourcePath, dest) + + g.AddMount(spec.Mount{ + Destination: dest, + Source: path.SourcePath, + Type: define.TypeBind, + // Important: This must always be mounted read only here, we are using + // the source in the artifact store directly and because that is digest + // based a write will break the layout. + Options: []string{define.TypeBind, "ro"}, + }) + } + } + } + err = c.setHomeEnvIfNeeded() if err != nil { return nil, nil, err diff --git a/libpod/container_internal_freebsd.go b/libpod/container_internal_freebsd.go index a3e0c5d613..64d377b1fc 100644 --- a/libpod/container_internal_freebsd.go +++ b/libpod/container_internal_freebsd.go @@ -13,6 +13,7 @@ import ( "github.com/containers/common/libnetwork/types" "github.com/containers/podman/v5/pkg/rootless" + securejoin "github.com/cyphar/filepath-securejoin" spec "github.com/opencontainers/runtime-spec/specs-go" "github.com/opencontainers/runtime-tools/generate" "github.com/sirupsen/logrus" @@ -415,3 +416,20 @@ func (c *Container) hasPrivateUTS() bool { func hasCapSysResource() (bool, error) { return true, nil } + +// containerPathIsFile returns true if the given containerPath is a file +func containerPathIsFile(unsafeRoot string, containerPath string) (bool, error) { + // Note freebsd does not have support for OpenInRoot() so us the less safe way + // with the old SecureJoin(), but given this is only called before the container + // is started it is not subject to race conditions with the container process. + path, err := securejoin.SecureJoin(unsafeRoot, containerPath) + if err != nil { + return false, err + } + + st, err := os.Lstat(path) + if err == nil && !st.IsDir() { + return true, nil + } + return false, err +} diff --git a/libpod/container_internal_linux.go b/libpod/container_internal_linux.go index 0f6e5905d3..3a3704c2ff 100644 --- a/libpod/container_internal_linux.go +++ b/libpod/container_internal_linux.go @@ -21,6 +21,7 @@ import ( "github.com/containers/podman/v5/libpod/define" "github.com/containers/podman/v5/libpod/shutdown" "github.com/containers/podman/v5/pkg/rootless" + securejoin "github.com/cyphar/filepath-securejoin" "github.com/moby/sys/capability" spec "github.com/opencontainers/runtime-spec/specs-go" "github.com/opencontainers/runtime-tools/generate" @@ -848,3 +849,18 @@ var hasCapSysResource = sync.OnceValues(func() (bool, error) { } return currentCaps.Get(capability.EFFECTIVE, capability.CAP_SYS_RESOURCE), nil }) + +// containerPathIsFile returns true if the given containerPath is a file +func containerPathIsFile(unsafeRoot string, containerPath string) (bool, error) { + f, err := securejoin.OpenInRoot(unsafeRoot, containerPath) + if err != nil { + return false, err + } + defer f.Close() + + st, err := f.Stat() + if err == nil && !st.IsDir() { + return true, nil + } + return false, err +} diff --git a/libpod/options.go b/libpod/options.go index 4578a8ed26..980ce60b13 100644 --- a/libpod/options.go +++ b/libpod/options.go @@ -1515,6 +1515,19 @@ func WithImageVolumes(volumes []*ContainerImageVolume) CtrCreateOption { } } +// WithImageVolumes adds the given image volumes to the container. +func WithArtifactVolumes(volumes []*ContainerArtifactVolume) CtrCreateOption { + return func(ctr *Container) error { + if ctr.valid { + return define.ErrCtrFinalized + } + + ctr.config.ArtifactVolumes = volumes + + return nil + } +} + // WithHealthCheck adds the healthcheck to the container config func WithHealthCheck(healthCheck *manifest.Schema2HealthConfig) CtrCreateOption { return func(ctr *Container) error { diff --git a/pkg/specgen/generate/container_create.go b/pkg/specgen/generate/container_create.go index ca77218945..73ec4a5f95 100644 --- a/pkg/specgen/generate/container_create.go +++ b/pkg/specgen/generate/container_create.go @@ -507,6 +507,19 @@ func createContainerOptions(rt *libpod.Runtime, s *specgen.SpecGenerator, pod *l options = append(options, libpod.WithImageVolumes(vols)) } + if len(s.ArtifactVolumes) != 0 { + vols := make([]*libpod.ContainerArtifactVolume, 0, len(s.ArtifactVolumes)) + for _, v := range s.ArtifactVolumes { + vols = append(vols, &libpod.ContainerArtifactVolume{ + Dest: v.Destination, + Source: v.Source, + Digest: v.Digest, + Title: v.Title, + }) + } + options = append(options, libpod.WithArtifactVolumes(vols)) + } + if s.Command != nil { options = append(options, libpod.WithCommand(s.Command)) } diff --git a/pkg/specgen/specgen.go b/pkg/specgen/specgen.go index c5ba5fe17f..a62f4c7008 100644 --- a/pkg/specgen/specgen.go +++ b/pkg/specgen/specgen.go @@ -305,6 +305,8 @@ type ContainerStorageConfig struct { // Image volumes bind-mount a container-image mount into the container. // Optional. ImageVolumes []*ImageVolume `json:"image_volumes,omitempty"` + // ArtifactVolumes volumes based on an existing artifact. + ArtifactVolumes []*ArtifactVolume `json:"artifact_volumes,omitempty"` // Devices are devices that will be added to the container. // Optional. Devices []spec.LinuxDevice `json:"devices,omitempty"` diff --git a/pkg/specgen/volumes.go b/pkg/specgen/volumes.go index 77c89f8ce4..dbe987d439 100644 --- a/pkg/specgen/volumes.go +++ b/pkg/specgen/volumes.go @@ -58,6 +58,28 @@ type ImageVolume struct { SubPath string `json:"subPath,omitempty"` } +// ArtifactVolume is a volume based on a artifact. The artifact blobs will +// be bind mounted directly as files and must always be read only. +type ArtifactVolume struct { + // Source is the name or digest of the artifact that should be mounted + Source string `json:"source"` + // Destination is the absolute path of the mount in the container. + // If path is a file in the container, then the artifact must consist of a single blob. + // Otherwise if it is a directory or does not exists all artifact blobs will be mounted + // into this path as files. As name the "org.opencontainers.image.title" will be used if + // available otherwise the digest is used as name. + Destination string `json:"destination"` + // Title can be used for multi blob artifacts to only mount the one specific blob that + // matches the "org.opencontainers.image.title" annotation. + // Optional. Conflicts with Digest. + Title string `json:"title,omitempty"` + // Digest can be used to filter a single blob from a multi blob artifact by the given digest. + // When this option is set the file name in the container defaults to the digest even when + // the title annotation exist. + // Optional. Conflicts with Title. + Digest string `json:"digest,omitempty"` +} + // GenVolumeMounts parses user input into mounts, volumes and overlay volumes func GenVolumeMounts(volumeFlag []string) (map[string]spec.Mount, map[string]*NamedVolume, map[string]*OverlayVolume, error) { mounts := make(map[string]spec.Mount) diff --git a/pkg/specgenutil/specgen.go b/pkg/specgenutil/specgen.go index f3ee9877cf..6163f47a0e 100644 --- a/pkg/specgenutil/specgen.go +++ b/pkg/specgenutil/specgen.go @@ -790,6 +790,9 @@ func FillOutSpecGen(s *specgen.SpecGenerator, c *entities.ContainerCreateOptions if len(s.ImageVolumes) == 0 { s.ImageVolumes = containerMounts.imageVolumes } + if len(s.ArtifactVolumes) == 0 { + s.ArtifactVolumes = containerMounts.artifactVolumes + } devices := c.Devices for _, gpu := range c.GPUs { diff --git a/pkg/specgenutil/volumes.go b/pkg/specgenutil/volumes.go index 1dea8fa2a4..d8d190578c 100644 --- a/pkg/specgenutil/volumes.go +++ b/pkg/specgenutil/volumes.go @@ -23,17 +23,19 @@ var ( ) type containerMountSlice struct { - mounts []spec.Mount - volumes []*specgen.NamedVolume - overlayVolumes []*specgen.OverlayVolume - imageVolumes []*specgen.ImageVolume + mounts []spec.Mount + volumes []*specgen.NamedVolume + overlayVolumes []*specgen.OverlayVolume + imageVolumes []*specgen.ImageVolume + artifactVolumes []*specgen.ArtifactVolume } // containerMountMap contains the container mounts with the destination path as map key type containerMountMap struct { - mounts map[string]spec.Mount - volumes map[string]*specgen.NamedVolume - imageVolumes map[string]*specgen.ImageVolume + mounts map[string]spec.Mount + volumes map[string]*specgen.NamedVolume + imageVolumes map[string]*specgen.ImageVolume + artifactVolumes map[string]*specgen.ArtifactVolume } type universalMount struct { @@ -131,6 +133,11 @@ func parseVolumes(rtc *config.Config, volumeFlag, mountFlag, tmpfsFlag []string) return nil, err } } + for dest := range unifiedContainerMounts.artifactVolumes { + if err := testAndSet(dest); err != nil { + return nil, err + } + } // Final step: maps to arrays finalMounts := make([]spec.Mount, 0, len(unifiedContainerMounts.mounts)) @@ -156,12 +163,17 @@ func parseVolumes(rtc *config.Config, volumeFlag, mountFlag, tmpfsFlag []string) for _, volume := range unifiedContainerMounts.imageVolumes { finalImageVolumes = append(finalImageVolumes, volume) } + finalArtifactVolumes := make([]*specgen.ArtifactVolume, 0, len(unifiedContainerMounts.artifactVolumes)) + for _, volume := range unifiedContainerMounts.artifactVolumes { + finalArtifactVolumes = append(finalArtifactVolumes, volume) + } return &containerMountSlice{ - mounts: finalMounts, - volumes: finalVolumes, - overlayVolumes: finalOverlayVolume, - imageVolumes: finalImageVolumes, + mounts: finalMounts, + volumes: finalVolumes, + overlayVolumes: finalOverlayVolume, + imageVolumes: finalImageVolumes, + artifactVolumes: finalArtifactVolumes, }, nil } @@ -170,10 +182,12 @@ func parseVolumes(rtc *config.Config, volumeFlag, mountFlag, tmpfsFlag []string) // podman run --mount type=bind,src=/etc/resolv.conf,target=/etc/resolv.conf ... // podman run --mount type=tmpfs,target=/dev/shm ... // podman run --mount type=volume,source=test-volume, ... +// podman run --mount type=artifact,source=$artifact,dest=... func mounts(mountFlag []string, configMounts []string) (*containerMountMap, error) { finalMounts := make(map[string]spec.Mount) finalNamedVolumes := make(map[string]*specgen.NamedVolume) finalImageVolumes := make(map[string]*specgen.ImageVolume) + finalArtifactVolumes := make(map[string]*specgen.ArtifactVolume) parseMounts := func(mounts []string, ignoreDup bool) error { for _, mount := range mounts { // TODO: Docker defaults to "volume" if no mount type is specified. @@ -244,6 +258,18 @@ func mounts(mountFlag []string, configMounts []string) (*containerMountMap, erro return fmt.Errorf("%v: %w", volume.Destination, specgen.ErrDuplicateDest) } finalImageVolumes[volume.Destination] = volume + case "artifact": + volume, err := getArtifactVolume(tokens) + if err != nil { + return err + } + if _, ok := finalArtifactVolumes[volume.Destination]; ok { + if ignoreDup { + continue + } + return fmt.Errorf("%v: %w", volume.Destination, specgen.ErrDuplicateDest) + } + finalArtifactVolumes[volume.Destination] = volume case "volume": volume, err := getNamedVolume(tokens) if err != nil { @@ -276,9 +302,10 @@ func mounts(mountFlag []string, configMounts []string) (*containerMountMap, erro } return &containerMountMap{ - mounts: finalMounts, - volumes: finalNamedVolumes, - imageVolumes: finalImageVolumes, + mounts: finalMounts, + volumes: finalNamedVolumes, + imageVolumes: finalImageVolumes, + artifactVolumes: finalArtifactVolumes, }, nil } @@ -683,6 +710,50 @@ func getImageVolume(args []string) (*specgen.ImageVolume, error) { return newVolume, nil } +// Parse the arguments into an artifact volume. An artifact volume creates mounts +// based on an existing artifact in the store. +func getArtifactVolume(args []string) (*specgen.ArtifactVolume, error) { + newVolume := new(specgen.ArtifactVolume) + + for _, arg := range args { + name, value, hasValue := strings.Cut(arg, "=") + switch name { + case "src", "source": + if !hasValue { + return nil, fmt.Errorf("%v: %w", name, errOptionArg) + } + newVolume.Source = value + case "target", "dst", "destination": + if !hasValue { + return nil, fmt.Errorf("%v: %w", name, errOptionArg) + } + if err := parse.ValidateVolumeCtrDir(value); err != nil { + return nil, err + } + newVolume.Destination = unixPathClean(value) + case "title": + if !hasValue { + return nil, fmt.Errorf("%v: %w", name, errOptionArg) + } + newVolume.Title = value + + case "digest": + if !hasValue { + return nil, fmt.Errorf("%v: %w", name, errOptionArg) + } + newVolume.Digest = value + default: + return nil, fmt.Errorf("%s: %w", name, util.ErrBadMntOption) + } + } + + if len(newVolume.Source)*len(newVolume.Destination) == 0 { + return nil, errors.New("must set source and destination for artifact volume") + } + + return newVolume, nil +} + // GetTmpfsMounts creates spec.Mount structs for user-requested tmpfs mounts func getTmpfsMounts(tmpfsFlag []string) (map[string]spec.Mount, error) { m := make(map[string]spec.Mount) diff --git a/test/e2e/artifact_mount_test.go b/test/e2e/artifact_mount_test.go new file mode 100644 index 0000000000..d158dc1331 --- /dev/null +++ b/test/e2e/artifact_mount_test.go @@ -0,0 +1,223 @@ +//go:build linux || freebsd + +package integration + +import ( + "os" + "path/filepath" + + . "github.com/containers/podman/v5/test/utils" + . "github.com/onsi/ginkgo/v2" + . "github.com/onsi/gomega" +) + +var _ = Describe("Podman artifact mount", func() { + BeforeEach(func() { + SkipIfRemote("artifacts are not supported on the remote client yet due to being in development still") + }) + + It("podman artifact mount single blob", func() { + podmanTest.PodmanExitCleanly("artifact", "pull", ARTIFACT_SINGLE) + + const artifactContent = "mRuO9ykak1Q2j" + + tests := []struct { + name string + mountOpts string + containerFile string + }{ + { + name: "single artifact mount", + mountOpts: "dst=/test", + containerFile: "/test/testfile", + }, + { + name: "single artifact mount on existing file", + mountOpts: "dst=/etc/os-release", + containerFile: "/etc/os-release", + }, + { + name: "single artifact mount with title", + mountOpts: "dst=/tmp,title=testfile", + containerFile: "/tmp/testfile", + }, + { + name: "single artifact mount with digest", + mountOpts: "dst=/data,digest=sha256:e9510923578af3632946ecf5ae479c1b5f08b47464e707b5cbab9819272a9752", + containerFile: "/data/sha256-e9510923578af3632946ecf5ae479c1b5f08b47464e707b5cbab9819272a9752", + }, + } + + for _, tt := range tests { + By(tt.name) + // FIXME: we need https://github.com/containers/container-selinux/pull/360 to fix the selinux access problem, until then disable it. + session := podmanTest.PodmanExitCleanly("run", "--security-opt=label=disable", "--rm", "--mount", "type=artifact,src="+ARTIFACT_SINGLE+","+tt.mountOpts, ALPINE, "cat", tt.containerFile) + Expect(session.OutputToString()).To(Equal(artifactContent)) + } + }) + + It("podman artifact mount multi blob", func() { + podmanTest.PodmanExitCleanly("artifact", "pull", ARTIFACT_MULTI) + podmanTest.PodmanExitCleanly("artifact", "pull", ARTIFACT_MULTI_NO_TITLE) + + const ( + artifactContent1 = "xuHWedtC0ADST" + artifactContent2 = "tAyZczFlgFsi4" + ) + + type expectedFiles struct { + file string + content string + } + + tests := []struct { + name string + mountOpts string + containerFiles []expectedFiles + }{ + { + name: "multi blob with title", + mountOpts: "src=" + ARTIFACT_MULTI + ",dst=/test", + containerFiles: []expectedFiles{ + { + file: "/test/test1", + content: artifactContent1, + }, + { + file: "/test/test2", + content: artifactContent2, + }, + }, + }, + { + name: "multi blob without title", + mountOpts: "src=" + ARTIFACT_MULTI_NO_TITLE + ",dst=/test", + containerFiles: []expectedFiles{ + { + file: "/test/sha256-8257bba28b9d19ac353c4b713b470860278857767935ef7e139afd596cb1bb2d", + content: artifactContent1, + }, + { + file: "/test/sha256-63700c54129c6daaafe3a20850079f82d6d658d69de73d6158d81f920c6fbdd7", + content: artifactContent2, + }, + }, + }, + { + name: "multi blob filter by title", + mountOpts: "src=" + ARTIFACT_MULTI + ",dst=/test,title=test2", + containerFiles: []expectedFiles{ + { + file: "/test/test2", + content: artifactContent2, + }, + }, + }, + { + name: "multi blob filter by digest", + mountOpts: "src=" + ARTIFACT_MULTI + ",dst=/test,digest=sha256:8257bba28b9d19ac353c4b713b470860278857767935ef7e139afd596cb1bb2d", + containerFiles: []expectedFiles{ + { + file: "/test/sha256-8257bba28b9d19ac353c4b713b470860278857767935ef7e139afd596cb1bb2d", + content: artifactContent1, + }, + }, + }, + } + for _, tt := range tests { + By(tt.name) + // FIXME: we need https://github.com/containers/container-selinux/pull/360 to fix the selinux access problem, until then disable it. + args := []string{"run", "--security-opt=label=disable", "--rm", "--mount", "type=artifact," + tt.mountOpts, ALPINE, "cat"} + for _, f := range tt.containerFiles { + args = append(args, f.file) + } + session := podmanTest.PodmanExitCleanly(args...) + outs := session.OutputToStringArray() + Expect(outs).To(HaveLen(len(tt.containerFiles))) + for i, f := range tt.containerFiles { + Expect(outs[i]).To(Equal(f.content)) + } + } + }) + + It("podman artifact mount remove while in use", func() { + ctrName := "ctr1" + artifactName := "localhost/test" + artifactFileName := "somefile" + + artifactFile := filepath.Join(podmanTest.TempDir, artifactFileName) + err := os.WriteFile(artifactFile, []byte("hello world\n"), 0o644) + Expect(err).ToNot(HaveOccurred()) + + 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("artifact", "rm", artifactName) + + // file must sill be readable after artifact removal + session := podmanTest.PodmanExitCleanly("exec", ctrName, "cat", "/test/"+artifactFileName) + Expect(session.OutputToString()).To(Equal("hello world")) + + // restart will fail if artifact does not exist + session = podmanTest.Podman([]string{"restart", "-t0", ctrName}) + session.WaitWithDefaultTimeout() + Expect(session).To(ExitWithError(125, artifactName+": artifact does not exist")) + + // create a artifact with the same name again and add another file to ensure it picks up the changes + artifactFile2Name := "otherfile" + artifactFile2 := filepath.Join(podmanTest.TempDir, artifactFile2Name) + err = os.WriteFile(artifactFile2, []byte("second file"), 0o644) + Expect(err).ToNot(HaveOccurred()) + + podmanTest.PodmanExitCleanly("artifact", "add", artifactName, artifactFile, artifactFile2) + podmanTest.PodmanExitCleanly("start", ctrName) + + session = podmanTest.PodmanExitCleanly("exec", ctrName, "cat", "/test/"+artifactFileName, "/test/"+artifactFile2Name) + Expect(session.OutputToString()).To(Equal("hello world second file")) + }) + + It("podman artifact mount dest conflict", func() { + tests := []struct { + name string + mount string + }{ + { + name: "bind mount --volume", + mount: "--volume=/tmp:/test", + }, + { + name: "overlay mount", + mount: "--volume=/tmp:/test:O", + }, + { + name: "named volume", + mount: "--volume=abc:/test:O", + }, + { + name: "bind mount --mount type=bind", + mount: "--mount=type=bind,src=/tmp,dst=/test", + }, + { + name: "image mount", + mount: "--mount=type=bind,src=someimage,dst=/test", + }, + { + name: "tmpfs mount", + mount: "--tmpfs=/test", + }, + { + name: "artifact mount", + mount: "--mount=type=artifact,src=abc,dst=/test", + }, + } + + for _, tt := range tests { + By(tt.name) + session := podmanTest.Podman([]string{"run", "--rm", "--mount", "type=artifact,src=someartifact,dst=/test", tt.mount, ALPINE}) + session.WaitWithDefaultTimeout() + Expect(session).To(ExitWithError(125, "/test: duplicate mount destination")) + } + }) +}) From c05908a7f6c1eee4d90d95ad903c4827dffb23b4 Mon Sep 17 00:00:00 2001 From: Paul Holzinger Date: Thu, 27 Feb 2025 15:14:55 +0100 Subject: [PATCH 11/11] libartifact: NewArtifactStore() reject relative paths The oci layout code can handle a relative path find but all paths returned by the code then will alos be relative, this can be bad and result in bugs if something ever changes the cwd. The graphroot path we pass should already be always absolute, so just add a sanity check here given libartifact is planned to be moved as sperate lib and we cannot assume anything about the path we will be given there. Signed-off-by: Paul Holzinger --- pkg/libartifact/store/store.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/pkg/libartifact/store/store.go b/pkg/libartifact/store/store.go index 7d3cf9e8d4..904912c96d 100644 --- a/pkg/libartifact/store/store.go +++ b/pkg/libartifact/store/store.go @@ -47,6 +47,10 @@ func NewArtifactStore(storePath string, sc *types.SystemContext) (*ArtifactStore if storePath == "" { return nil, errors.New("store path cannot be empty") } + if !filepath.IsAbs(storePath) { + return nil, fmt.Errorf("store path %q must be absolute", storePath) + } + logrus.Debugf("Using artifact store path: %s", storePath) artifactStore := &ArtifactStore{