From 5340288ee0fb7f8924ef33b40b3771aac550e3fc Mon Sep 17 00:00:00 2001 From: Sohan Kunkerkar Date: Thu, 17 Apr 2025 12:55:17 -0400 Subject: [PATCH 1/4] store: extract artifact layers automatically Signed-off-by: Sohan Kunkerkar --- go.mod | 2 +- internal/ociartifact/store.go | 106 ++++++++++++++++++++++++++++++---- 2 files changed, 95 insertions(+), 13 deletions(-) diff --git a/go.mod b/go.mod index d0d69356dba..c4caced4057 100644 --- a/go.mod +++ b/go.mod @@ -43,6 +43,7 @@ require ( github.com/intel/goresctrl v0.8.0 github.com/json-iterator/go v1.1.12 github.com/kata-containers/kata-containers/src/runtime v0.0.0-20240208092920-b99f57452225 + github.com/klauspost/compress v1.18.0 github.com/moby/sys/mountinfo v0.7.2 github.com/moby/sys/user v0.3.0 github.com/moby/sys/userns v0.1.0 @@ -162,7 +163,6 @@ require ( github.com/jinzhu/copier v0.4.0 // indirect github.com/josharian/intern v1.0.0 // indirect github.com/kevinburke/ssh_config v1.2.0 // indirect - github.com/klauspost/compress v1.18.0 // indirect github.com/klauspost/pgzip v1.2.6 // indirect github.com/knqyf263/go-plugin v0.8.1-0.20241122101054-d8d42059d8f1 // indirect github.com/letsencrypt/boulder v0.0.0-20240620165639-de9c06129bec // indirect diff --git a/internal/ociartifact/store.go b/internal/ociartifact/store.go index 8be4ddb175f..edd99fe727b 100644 --- a/internal/ociartifact/store.go +++ b/internal/ociartifact/store.go @@ -1,11 +1,14 @@ package ociartifact import ( + "archive/tar" + "compress/gzip" "context" "encoding/hex" "errors" "fmt" "io" + "os" "path/filepath" "slices" "strings" @@ -17,6 +20,7 @@ import ( "github.com/containers/image/v5/oci/layout" "github.com/containers/image/v5/pkg/blobinfocache" "github.com/containers/image/v5/types" + "github.com/klauspost/compress/zstd" "github.com/opencontainers/go-digest" specs "github.com/opencontainers/image-spec/specs-go/v1" @@ -479,35 +483,113 @@ func (s *Store) BlobMountPaths(ctx context.Context, artifact *Artifact, sys *typ return nil, fmt.Errorf("failed to get an image reference: %w", err) } + extractDir := filepath.Join(s.rootPath, "extracted", artifact.Digest().Encoded()) + if err := os.MkdirAll(extractDir, 0o755); err != nil { + return nil, fmt.Errorf("failed to create artifact dir: %w", err) + } + src, err := ref.NewImageSource(ctx, sys) if err != nil { return nil, fmt.Errorf("failed to get an image source: %w", err) } - defer src.Close() mountPaths := make([]BlobMountPath, 0, len(artifact.Manifest().Layers)) + bic := blobinfocache.DefaultCache(s.systemContext) for _, l := range artifact.Manifest().Layers { - path, err := layout.GetLocalBlobPath(ctx, src, l.Digest) + func() { + blob, _, err := s.impl.GetBlob(ctx, src, types.BlobInfo{Digest: l.Digest}, bic) + if err != nil { + log.Errorf(ctx, "Failed to get blob: %v", err) + + return + } + defer blob.Close() + + content, err := s.processLayerContent(l.MediaType, blob) + if err != nil { + log.Errorf(ctx, "Failed to process layer: %v", err) + + return + } + + name := artifactName(l.Annotations) + if name == "" { + log.Warnf(ctx, "Unable to find name for artifact layer which makes it not mountable") + + return + } + + filePath := filepath.Join(extractDir, name) + if err := os.WriteFile(filePath, content, 0o644); err != nil { + log.Errorf(ctx, "Failed to write file: %v", err) + } + + mountPaths = append(mountPaths, BlobMountPath{ + SourcePath: filePath, + Name: name, + }) + }() + } + + return mountPaths, nil +} + +// processLayerContent decompresses and processes layer content based on media type. +// Supports gzip, zstd, and uncompressed tar formats. Returns raw bytes for non-tar content. +// Instead of relying on the exact media type like "application/vnd.oci.image.layer.v1.tar+zstd", +// it would be helpful to stick with more generic patterns like ".tar+gzip" or ".tar+zstd". +func (s *Store) processLayerContent(mediaType string, r io.Reader) ([]byte, error) { + switch { + case strings.HasSuffix(mediaType, ".tar+gzip"): + gr, err := gzip.NewReader(r) if err != nil { - return nil, fmt.Errorf("failed to get a local blob path: %w", err) + return nil, fmt.Errorf("gzip decompress failed: %w", err) } + defer gr.Close() - name := artifactName(l.Annotations) - if name == "" { - log.Warnf(ctx, "Unable to find name for artifact layer which makes it not mountable") + return s.extractSingleFileFromTar(gr) - continue + case strings.HasSuffix(mediaType, ".tar+zstd"): + zr, err := zstd.NewReader(r) + if err != nil { + return nil, fmt.Errorf("zstd decompress failed: %w", err) } + defer zr.Close() - mountPaths = append(mountPaths, BlobMountPath{ - SourcePath: path, - Name: name, - }) + return s.extractSingleFileFromTar(zr) + + case strings.HasSuffix(mediaType, ".tar"): + return s.extractSingleFileFromTar(r) + + default: + data, err := io.ReadAll(r) + if err != nil { + return nil, fmt.Errorf("reading blob content: %w", err) + } + + return data, nil } +} - return mountPaths, nil +func (s *Store) extractSingleFileFromTar(r io.Reader) ([]byte, error) { + tr := tar.NewReader(r) + + for { + hdr, err := tr.Next() + if err == io.EOF { + return nil, errors.New("no files found in tar archive") + } + + if err != nil { + return nil, fmt.Errorf("reading tar header: %w", err) + } + + if hdr.Typeflag == tar.TypeReg { + return io.ReadAll(tr) + } + } } func artifactName(annotations map[string]string) string { From b30695f61da4beca4e6a27848748d348cee6c177 Mon Sep 17 00:00:00 2001 From: Sohan Kunkerkar Date: Wed, 23 Apr 2025 16:09:51 -0400 Subject: [PATCH 2/4] test: add coverage for extracting artifact layers Signed-off-by: Sohan Kunkerkar --- internal/ociartifact/store.go | 52 ++++++++++++++++++++++++-------- server/container_create_linux.go | 9 ++++++ test/oci_artifacts.bats | 24 +++++++++++++++ 3 files changed, 73 insertions(+), 12 deletions(-) diff --git a/internal/ociartifact/store.go b/internal/ociartifact/store.go index edd99fe727b..51d0204876c 100644 --- a/internal/ociartifact/store.go +++ b/internal/ociartifact/store.go @@ -12,6 +12,7 @@ import ( "path/filepath" "slices" "strings" + "sync" modelSpec "github.com/CloudNativeAI/model-spec/specs-go/v1" "github.com/containers/common/libimage" @@ -473,7 +474,16 @@ type BlobMountPath struct { // Source path of the blob, i.e. full path in the blob dir. SourcePath string // Name of the file in the artifact. - Name string + Name string + cleanupFunc func() + cleanupOnce *sync.Once +} + +// Cleanup triggers the deletion of the extracted directory. +func (b *BlobMountPath) Cleanup() { + if b.cleanupFunc != nil && b.cleanupOnce != nil { + b.cleanupOnce.Do(b.cleanupFunc) + } } // BlobMountPaths retrieves the local file paths for all blobs in the provided artifact and returns them as BlobMountPath slices. @@ -488,6 +498,17 @@ func (s *Store) BlobMountPaths(ctx context.Context, artifact *Artifact, sys *typ return nil, fmt.Errorf("failed to create artifact dir: %w", err) } + cleanupFunc := func() { os.RemoveAll(extractDir) } + cleanupOnce := &sync.Once{} + + // Cleanup on function exit if we encounter an error + cleanup := true + defer func() { + if cleanup { + os.RemoveAll(extractDir) + } + }() + src, err := ref.NewImageSource(ctx, sys) if err != nil { return nil, fmt.Errorf("failed to get an image source: %w", err) @@ -498,41 +519,48 @@ func (s *Store) BlobMountPaths(ctx context.Context, artifact *Artifact, sys *typ bic := blobinfocache.DefaultCache(s.systemContext) for _, l := range artifact.Manifest().Layers { - func() { + err = func() error { blob, _, err := s.impl.GetBlob(ctx, src, types.BlobInfo{Digest: l.Digest}, bic) if err != nil { - log.Errorf(ctx, "Failed to get blob: %v", err) - - return + return fmt.Errorf("failed to get blob: %w", err) } defer blob.Close() content, err := s.processLayerContent(l.MediaType, blob) if err != nil { - log.Errorf(ctx, "Failed to process layer: %v", err) - - return + return fmt.Errorf("failed to process layer: %w", err) } name := artifactName(l.Annotations) if name == "" { log.Warnf(ctx, "Unable to find name for artifact layer which makes it not mountable") - return + return nil } filePath := filepath.Join(extractDir, name) if err := os.WriteFile(filePath, content, 0o644); err != nil { log.Errorf(ctx, "Failed to write file: %v", err) + os.Remove(filePath) + return err } mountPaths = append(mountPaths, BlobMountPath{ - SourcePath: filePath, - Name: name, + SourcePath: filePath, + Name: name, + cleanupFunc: cleanupFunc, + cleanupOnce: cleanupOnce, }) + return nil }() + // If any layer extraction fails, abort the entire operation. + if err != nil { + return nil, fmt.Errorf("failed to extract artifact layer: %w", err) + } } - + // Disable cleanup on success. + // mountArtifact() will handle the cleanup. + cleanup = false return mountPaths, nil } diff --git a/server/container_create_linux.go b/server/container_create_linux.go index c896967c00a..5f284122e3c 100644 --- a/server/container_create_linux.go +++ b/server/container_create_linux.go @@ -410,6 +410,15 @@ func (s *Server) mountArtifact(ctx context.Context, specgen *generate.Generator, return nil, fmt.Errorf("failed to get artifact blob mount paths: %w", err) } + // Cleanup extracted folder after use. + defer func() { + // All paths share the same cleanup function and sync.Once, so calling + // Cleanup() on any path will clean up the entire extraction directory. + for _, path := range paths { + path.Cleanup() + } + }() + options := []string{"bind", "ro"} volumes := make([]oci.ContainerVolume, 0, len(paths)) selinuxRelabel := true diff --git a/test/oci_artifacts.bats b/test/oci_artifacts.bats index cf7f9c6502e..a3ad3fb5608 100755 --- a/test/oci_artifacts.bats +++ b/test/oci_artifacts.bats @@ -204,3 +204,27 @@ ARTIFACT_IMAGE="$ARTIFACT_REPO:singlefile" crictl rm "$ctr_id" crictl rmi "$ARTIFACT_IMAGE" } + +@test "should extract mounted image artifact files correctly" { + start_crio + IMAGE="quay.io/sohankunkerkar/test-artifact:v1" + crictl pull "$IMAGE" + pod_id=$(crictl runp "$TESTDATA"/sandbox_config.json) + jq --arg IMAGE "$IMAGE" \ + '.mounts = [{ + container_path: "/root/artifacts", + image: { image: $IMAGE }, + readonly: true + }]' \ + "$TESTDATA"/container_sleep.json > "$TESTDIR/container.json" + + ctr_id=$(crictl run "$TESTDIR/container.json" "$TESTDATA/sandbox_config.json") + + run crictl exec --sync "$ctr_id" cat /root/artifacts/zstd.txt + [ "$output" == "This is a zstd-compressed file" ] + + # Cleanup + crictl stop "$ctr_id" + crictl rm "$ctr_id" + crictl rmi "$IMAGE" +} From 8234314b13a545e030ac1e3896f980247fa4900b Mon Sep 17 00:00:00 2001 From: Sohan Kunkerkar Date: Wed, 23 Apr 2025 23:52:55 -0400 Subject: [PATCH 3/4] test: fix artifacts Signed-off-by: Sohan Kunkerkar --- test/oci_artifacts.bats | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/oci_artifacts.bats b/test/oci_artifacts.bats index a3ad3fb5608..1e9517bbaec 100755 --- a/test/oci_artifacts.bats +++ b/test/oci_artifacts.bats @@ -11,7 +11,7 @@ function teardown() { cleanup_test } -ARTIFACT_REPO=quay.io/crio/artifact +ARTIFACT_REPO=quay.io/sohankunkerkar/artifact ARTIFACT_IMAGE="$ARTIFACT_REPO:singlefile" @test "should be able to pull and list an OCI artifact" { @@ -75,7 +75,7 @@ ARTIFACT_IMAGE="$ARTIFACT_REPO:singlefile" crictl inspecti "${imageId:0:12}" # shortname - crictl inspecti crio/artifact:singlefile + crictl inspecti sohankunkerkar/artifact:singlefile } @test "should be able to remove an OCI artifact" { From 3c63062658829d4e028b53c107c45dc38434e639 Mon Sep 17 00:00:00 2001 From: Sohan Kunkerkar Date: Fri, 25 Apr 2025 12:04:38 -0400 Subject: [PATCH 4/4] * : add cleanup function to remove extracted OCI artifacts dir Signed-off-by: Sohan Kunkerkar --- internal/oci/container.go | 17 +++++++ internal/oci/runtime_oci.go | 1 + internal/oci/runtime_vm.go | 1 + server/container_create.go | 6 ++- server/container_create_linux.go | 65 +++++++++++++-------------- server/container_create_linux_test.go | 16 +++---- 6 files changed, 62 insertions(+), 44 deletions(-) diff --git a/internal/oci/container.go b/internal/oci/container.go index 6ba460f4543..f329a012df6 100644 --- a/internal/oci/container.go +++ b/internal/oci/container.go @@ -77,6 +77,8 @@ type Container struct { runtimePath string // runtime path for a given platform execPIDs map[int]bool runtimeUser *types.ContainerUser + cleanups []func() + cleanupOnce *sync.Once } func (c *Container) CRIAttributes() *types.ContainerAttributes { @@ -877,3 +879,18 @@ func (c *Container) KillExecPIDs() { func (c *Container) RuntimeUser() *types.ContainerUser { return c.runtimeUser } + +func (c *Container) AddCleanup(cleanup func()) { + c.cleanups = append(c.cleanups, cleanup) +} + +func (c *Container) Cleanup() { + if c.cleanupOnce == nil { + c.cleanupOnce = &sync.Once{} + } + c.cleanupOnce.Do(func() { + for _, cleanup := range c.cleanups { + cleanup() + } + }) +} diff --git a/internal/oci/runtime_oci.go b/internal/oci/runtime_oci.go index 9b6ab42d4b2..355dfe50d78 100644 --- a/internal/oci/runtime_oci.go +++ b/internal/oci/runtime_oci.go @@ -919,6 +919,7 @@ func (r *runtimeOCI) StopLoopForContainer(c *Container, bm kwait.BackoffManager) c.state.Finished = time.Now() c.opLock.Unlock() c.SetAsDoneStopping() + c.Cleanup() }() if c.state.Status == ContainerStatePaused { diff --git a/internal/oci/runtime_vm.go b/internal/oci/runtime_vm.go index 1b522daad8a..a67fd513d97 100644 --- a/internal/oci/runtime_vm.go +++ b/internal/oci/runtime_vm.go @@ -689,6 +689,7 @@ func (r *runtimeVM) StopContainer(ctx context.Context, c *Container, timeout int return err } + c.Cleanup() c.state.Finished = time.Now() return nil diff --git a/server/container_create.go b/server/container_create.go index 6ad26048b5e..29d245c62de 100644 --- a/server/container_create.go +++ b/server/container_create.go @@ -837,7 +837,7 @@ func (s *Server) createSandboxContainer(ctx context.Context, ctr container.Conta } }() - containerVolumes, ociMounts, safeMounts, err := s.addOCIBindMounts(ctx, ctr, mountLabel, s.config.BindMountPrefix, s.config.AbsentMountSourcesToReject, maybeRelabel, skipRelabel, cgroup2RW, idMapSupport, rroSupport, s.ContainerServer.Config().Root, containerInfo.RunDir) + containerVolumes, ociMounts, safeMounts, cleanups, err := s.addOCIBindMounts(ctx, ctr, mountLabel, s.config.BindMountPrefix, s.config.AbsentMountSourcesToReject, maybeRelabel, skipRelabel, cgroup2RW, idMapSupport, rroSupport, s.ContainerServer.Config().Root, containerInfo.RunDir) if err != nil { return nil, err } @@ -1380,6 +1380,10 @@ func (s *Server) createSandboxContainer(ctx context.Context, ctr container.Conta ociContainer.AddVolume(cv) } + for _, cleanup := range cleanups { + ociContainer.AddCleanup(cleanup) + } + return ociContainer, nil } diff --git a/server/container_create_linux.go b/server/container_create_linux.go index 5f284122e3c..7e083283dd9 100644 --- a/server/container_create_linux.go +++ b/server/container_create_linux.go @@ -145,9 +145,10 @@ func clearReadOnly(m *rspec.Mount) { m.Options = append(m.Options, "rw") } -func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, mountLabel, bindMountPrefix string, absentMountSourcesToReject []string, maybeRelabel, skipRelabel, cgroup2RW, idMapSupport, rroSupport bool, storageRoot, runDir string) ([]oci.ContainerVolume, []rspec.Mount, []*safeMountInfo, error) { +func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, mountLabel, bindMountPrefix string, absentMountSourcesToReject []string, maybeRelabel, skipRelabel, cgroup2RW, idMapSupport, rroSupport bool, storageRoot, runDir string) ([]oci.ContainerVolume, []rspec.Mount, []*safeMountInfo, []func(), error) { ctx, span := log.StartSpan(ctx) defer span.End() + var cleanups []func() volumes := []oci.ContainerVolume{} ociMounts := []rspec.Mount{} @@ -192,12 +193,12 @@ func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, mountInfos, err := mount.GetMounts() if err != nil { - return nil, nil, nil, err + return nil, nil, nil, nil, err } imageVolumesPath, err := s.ensureImageVolumesPath(ctx, mounts) if err != nil { - return nil, nil, nil, fmt.Errorf("ensure image volumes path: %w", err) + return nil, nil, nil, nil, fmt.Errorf("ensure image volumes path: %w", err) } var safeMounts []*safeMountInfo @@ -205,14 +206,17 @@ func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, for _, m := range mounts { dest := m.ContainerPath if dest == "" { - return nil, nil, nil, errors.New("mount.ContainerPath is empty") + return nil, nil, nil, nil, errors.New("mount.ContainerPath is empty") } if m.GetImage().GetImage() != "" { // Try mountArtifact first, and fall back to mountImage if it fails with ErrNotFound - artifactVolumes, err := s.mountArtifact(ctx, specgen, m, mountLabel, skipRelabel, maybeRelabel) + artifactVolumes, cleanup, err := s.mountArtifact(ctx, specgen, m, mountLabel, skipRelabel, maybeRelabel) if err == nil { volumes = append(volumes, artifactVolumes...) + if cleanup != nil { + cleanups = append(cleanups, cleanup) + } continue } @@ -221,7 +225,7 @@ func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, volume, safeMount, err := s.mountImage(ctx, specgen, imageVolumesPath, m, runDir) if err != nil { - return nil, nil, nil, fmt.Errorf("%w: %w", crierrors.ErrImageVolumeMountFailed, err) + return nil, nil, nil, nil, fmt.Errorf("%w: %w", crierrors.ErrImageVolumeMountFailed, err) } volumes = append(volumes, *volume) @@ -231,7 +235,7 @@ func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, } if m.HostPath == "" { - return nil, nil, nil, errors.New("mount.HostPath is empty") + return nil, nil, nil, nil, errors.New("mount.HostPath is empty") } if m.HostPath == "/" && dest == "/" { @@ -250,14 +254,14 @@ func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, src = resolvedSrc } else { if !os.IsNotExist(err) { - return nil, nil, nil, fmt.Errorf("failed to resolve symlink %q: %w", src, err) + return nil, nil, nil, nil, fmt.Errorf("failed to resolve symlink %q: %w", src, err) } for _, toReject := range absentMountSourcesToReject { if filepath.Clean(src) == toReject { // special-case /etc/hostname, as we don't want it to be created as a directory // This can cause issues with node reboot. - return nil, nil, nil, fmt.Errorf("cannot mount %s: path does not exist and will cause issues as a directory", toReject) + return nil, nil, nil, nil, fmt.Errorf("cannot mount %s: path does not exist and will cause issues as a directory", toReject) } } @@ -270,7 +274,7 @@ func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, // create the missing bind mount source for restore and return an // error to the user. if err = os.MkdirAll(src, 0o755); err != nil { - return nil, nil, nil, fmt.Errorf("failed to mkdir %s: %w", src, err) + return nil, nil, nil, nil, fmt.Errorf("failed to mkdir %s: %w", src, err) } } } @@ -285,17 +289,17 @@ func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, // setting the root propagation case types.MountPropagation_PROPAGATION_BIDIRECTIONAL: if err := ensureShared(src, mountInfos); err != nil { - return nil, nil, nil, err + return nil, nil, nil, nil, err } options = append(options, "rshared") if err := specgen.SetLinuxRootPropagation("rshared"); err != nil { - return nil, nil, nil, err + return nil, nil, nil, nil, err } case types.MountPropagation_PROPAGATION_HOST_TO_CONTAINER: if err := ensureSharedOrSlave(src, mountInfos); err != nil { - return nil, nil, nil, err + return nil, nil, nil, nil, err } options = append(options, "rslave") @@ -303,7 +307,7 @@ func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, if specgen.Config.Linux.RootfsPropagation != "rshared" && specgen.Config.Linux.RootfsPropagation != "rslave" { if err := specgen.SetLinuxRootPropagation("rslave"); err != nil { - return nil, nil, nil, err + return nil, nil, nil, nil, err } } default: @@ -317,14 +321,14 @@ func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, switch { case m.RecursiveReadOnly && m.Readonly: if !rroSupport { - return nil, nil, nil, fmt.Errorf( + return nil, nil, nil, nil, fmt.Errorf( "recursive read-only mount support is not available for hostPath %q", m.HostPath, ) } if m.Propagation != types.MountPropagation_PROPAGATION_PRIVATE { - return nil, nil, nil, fmt.Errorf( + return nil, nil, nil, nil, fmt.Errorf( "recursive read-only mount requires private propagation for hostPath %q, got: %s", m.HostPath, m.Propagation, ) @@ -332,7 +336,7 @@ func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, options = append(options, "rro") case m.RecursiveReadOnly: - return nil, nil, nil, fmt.Errorf( + return nil, nil, nil, nil, fmt.Errorf( "recursive read-only mount conflicts with read-write mount for hostPath %q", m.HostPath, ) @@ -346,7 +350,7 @@ func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, if skipRelabel { log.Debugf(ctx, "Skipping relabel for %s because of super privileged container (type: spc_t)", src) } else if err := securityLabel(src, mountLabel, false, maybeRelabel); err != nil { - return nil, nil, nil, err + return nil, nil, nil, nil, err } } else { log.Debugf(ctx, "Skipping relabel for %s because kubelet did not request it", src) @@ -366,7 +370,7 @@ func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, gidMappings := getOCIMappings(m.GidMappings) if (uidMappings != nil || gidMappings != nil) && !idMapSupport { - return nil, nil, nil, errors.New("idmap mounts specified but OCI runtime does not support them. Perhaps the OCI runtime is too old") + return nil, nil, nil, nil, errors.New("idmap mounts specified but OCI runtime does not support them. Perhaps the OCI runtime is too old") } ociMounts = append(ociMounts, rspec.Mount{ @@ -395,30 +399,21 @@ func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, specgen.AddMount(m) } - return volumes, ociMounts, safeMounts, nil + return volumes, ociMounts, safeMounts, cleanups, nil } // mountArtifact binds artifact blobs to the container filesystem based on the provided mount configuration. -func (s *Server) mountArtifact(ctx context.Context, specgen *generate.Generator, m *types.Mount, mountLabel string, isSPC, maybeRelabel bool) ([]oci.ContainerVolume, error) { +func (s *Server) mountArtifact(ctx context.Context, specgen *generate.Generator, m *types.Mount, mountLabel string, isSPC, maybeRelabel bool) ([]oci.ContainerVolume, func(), error) { artifact, err := s.ArtifactStore().Status(ctx, m.Image.Image) if err != nil { - return nil, fmt.Errorf("failed to get artifact status: %w", err) + return nil, nil, fmt.Errorf("failed to get artifact status: %w", err) } paths, err := s.ArtifactStore().BlobMountPaths(ctx, artifact, s.config.SystemContext) if err != nil { - return nil, fmt.Errorf("failed to get artifact blob mount paths: %w", err) + return nil, nil, fmt.Errorf("failed to get artifact blob mount paths: %w", err) } - // Cleanup extracted folder after use. - defer func() { - // All paths share the same cleanup function and sync.Once, so calling - // Cleanup() on any path will clean up the entire extraction directory. - for _, path := range paths { - path.Cleanup() - } - }() - options := []string{"bind", "ro"} volumes := make([]oci.ContainerVolume, 0, len(paths)) selinuxRelabel := true @@ -436,12 +431,12 @@ func (s *Server) mountArtifact(ctx context.Context, specgen *generate.Generator, for _, path := range paths { dest, err := securejoin.SecureJoin(m.ContainerPath, path.Name) if err != nil { - return nil, fmt.Errorf("failed to join container path %q and artifact blob path %q: %w", m.ContainerPath, path.Name, err) + return nil, nil, fmt.Errorf("failed to join container path %q and artifact blob path %q: %w", m.ContainerPath, path.Name, err) } if selinuxRelabel { if err := securityLabel(path.SourcePath, mountLabel, false, maybeRelabel); err != nil { - return nil, err + return nil, nil, err } } @@ -465,7 +460,7 @@ func (s *Server) mountArtifact(ctx context.Context, specgen *generate.Generator, }) } - return volumes, nil + return volumes, paths[0].Cleanup, nil } // mountImage adds required image mounts to the provided spec generator and returns a corresponding ContainerVolume. diff --git a/server/container_create_linux_test.go b/server/container_create_linux_test.go index 9bf7134829f..b507e68be86 100644 --- a/server/container_create_linux_test.go +++ b/server/container_create_linux_test.go @@ -34,7 +34,7 @@ func TestAddOCIBindsForDev(t *testing.T) { sut := &Server{} - _, binds, _, err := sut.addOCIBindMounts(t.Context(), ctr, "", "", nil, false, false, false, false, false, "", "") + _, binds, _, _, err := sut.addOCIBindMounts(t.Context(), ctr, "", "", nil, false, false, false, false, false, "", "") if err != nil { t.Error(err) } @@ -86,7 +86,7 @@ func TestAddOCIBindsForSys(t *testing.T) { sut := &Server{} - _, binds, _, err := sut.addOCIBindMounts(t.Context(), ctr, "", "", nil, false, false, false, false, false, "", "") + _, binds, _, _, err := sut.addOCIBindMounts(t.Context(), ctr, "", "", nil, false, false, false, false, false, "", "") if err != nil { t.Error(err) } @@ -140,7 +140,7 @@ func TestAddOCIBindsRROMounts(t *testing.T) { sut := &Server{} - _, binds, _, err := sut.addOCIBindMounts(ctx, ctr, "", "", nil, false, false, false, false, true, "", "") + _, binds, _, _, err := sut.addOCIBindMounts(ctx, ctr, "", "", nil, false, false, false, false, true, "", "") if err != nil { t.Errorf("Should not fail to create RRO mount, got: %v", err) } @@ -238,7 +238,7 @@ func TestAddOCIBindsRROMountsError(t *testing.T) { sut := &Server{} - _, _, _, err = sut.addOCIBindMounts(ctx, ctr, "", "", nil, false, false, false, false, tc.rroSupport, "", "") + _, _, _, _, err = sut.addOCIBindMounts(ctx, ctr, "", "", nil, false, false, false, false, tc.rroSupport, "", "") if err == nil { t.Error("Should fail to add an RRO mount with a specific error") } @@ -271,7 +271,7 @@ func TestAddOCIBindsCGroupRW(t *testing.T) { sut := &Server{} //nolint: dogsled - _, _, _, err = sut.addOCIBindMounts(t.Context(), ctr, "", "", nil, false, false, true, false, false, "", "") + _, _, _, _, err = sut.addOCIBindMounts(t.Context(), ctr, "", "", nil, false, false, true, false, false, "", "") if err != nil { t.Error(err) } @@ -312,7 +312,7 @@ func TestAddOCIBindsCGroupRW(t *testing.T) { var hasCgroupRO bool //nolint: dogsled - _, _, _, err = sut.addOCIBindMounts(t.Context(), ctr, "", "", nil, false, false, false, false, false, "", "") + _, _, _, _, err = sut.addOCIBindMounts(t.Context(), ctr, "", "", nil, false, false, false, false, false, "", "") if err != nil { t.Error(err) } @@ -366,13 +366,13 @@ func TestAddOCIBindsErrorWithoutIDMap(t *testing.T) { sut := &Server{} //nolint: dogsled - _, _, _, err = sut.addOCIBindMounts(t.Context(), ctr, "", "", nil, false, false, false, false, false, "", "") + _, _, _, _, err = sut.addOCIBindMounts(t.Context(), ctr, "", "", nil, false, false, false, false, false, "", "") if err == nil { t.Errorf("Should have failed to create id mapped mount with no id map support") } //nolint: dogsled - _, _, _, err = sut.addOCIBindMounts(t.Context(), ctr, "", "", nil, false, false, false, true, false, "", "") + _, _, _, _, err = sut.addOCIBindMounts(t.Context(), ctr, "", "", nil, false, false, false, true, false, "", "") if err != nil { t.Errorf("%v", err) }