From 55886f71199bc5b96d5c74ae8489b4c2a622e551 Mon Sep 17 00:00:00 2001 From: Sohan Kunkerkar Date: Thu, 17 Apr 2025 12:55:17 -0400 Subject: [PATCH 1/5] 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 146e97afa79..bc5d8419f10 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.4.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 a4db21f9a92..a0ce583af51 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" @@ -515,35 +519,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 30e9a8bd619ec2ef152ab1e8c8893d9aaf6a8cc1 Mon Sep 17 00:00:00 2001 From: Sohan Kunkerkar Date: Wed, 23 Apr 2025 16:09:51 -0400 Subject: [PATCH 2/5] 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 | 23 ++++++++++++++ 3 files changed, 72 insertions(+), 12 deletions(-) diff --git a/internal/ociartifact/store.go b/internal/ociartifact/store.go index a0ce583af51..f6d1b418682 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" @@ -509,7 +510,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. @@ -524,6 +534,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) @@ -534,41 +555,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 bb1f0e136f2..ae85add6883 100644 --- a/server/container_create_linux.go +++ b/server/container_create_linux.go @@ -422,6 +422,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 3213d76e6af..2f57196cc36 100755 --- a/test/oci_artifacts.bats +++ b/test/oci_artifacts.bats @@ -285,4 +285,27 @@ EOF ctr_id=$(crictl run "$TESTDIR/container_config.json" "$TESTDATA/sandbox_config.json") run crictl exec "$ctr_id" sha256sum /root/artifact/cri-o/bin/crio [[ "$output" == *"ae5d192303e5f9a357c6ea39308338956b62b8830fd05f0460796db2215c2b35"* ]] + +@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 e7d90f8acf52187d4ade4c0030abfc94d534da75 Mon Sep 17 00:00:00 2001 From: Sohan Kunkerkar Date: Wed, 23 Apr 2025 23:52:55 -0400 Subject: [PATCH 3/5] 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 2f57196cc36..af045f06c55 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" ARTIFACT_IMAGE_SUBPATH="$ARTIFACT_REPO:subpath" @@ -76,7 +76,7 @@ ARTIFACT_IMAGE_SUBPATH="$ARTIFACT_REPO:subpath" 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 8492b0c324e62a73ac4ef39759fe682e86479d93 Mon Sep 17 00:00:00 2001 From: Sohan Kunkerkar Date: Fri, 25 Apr 2025 12:04:38 -0400 Subject: [PATCH 4/5] * : 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 | 70 +++++++++++++-------------- server/container_create_linux_test.go | 16 +++--- 6 files changed, 65 insertions(+), 46 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 83717dae922..9f49453d977 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 d5e463f0cfe..0c3ddf5aed3 100644 --- a/server/container_create.go +++ b/server/container_create.go @@ -808,7 +808,7 @@ func (s *Server) createSandboxContainer(ctx context.Context, ctr container.Conta } }() - containerVolumes, ociMounts, safeMounts, err := s.addOCIBindMounts(ctx, ctr, &containerInfo, maybeRelabel, skipRelabel, cgroup2RW, idMapSupport, rroSupport) + containerVolumes, ociMounts, safeMounts, cleanups, err := s.addOCIBindMounts(ctx, ctr, &containerInfo, maybeRelabel, skipRelabel, cgroup2RW, idMapSupport, rroSupport) if err != nil { return nil, err } @@ -1361,6 +1361,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 ae85add6883..95050821eb6 100644 --- a/server/container_create_linux.go +++ b/server/container_create_linux.go @@ -147,10 +147,12 @@ func clearReadOnly(m *rspec.Mount) { m.Options = append(m.Options, "rw") } -func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, ctrInfo *storage.ContainerInfo, maybeRelabel, skipRelabel, cgroup2RW, idMapSupport, rroSupport bool) ([]oci.ContainerVolume, []rspec.Mount, []*safeMountInfo, error) { +func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, ctrInfo *storage.ContainerInfo, maybeRelabel, skipRelabel, cgroup2RW, idMapSupport, rroSupport bool) ([]oci.ContainerVolume, []rspec.Mount, []*safeMountInfo, []func(), error) { ctx, span := log.StartSpan(ctx) defer span.End() + var cleanups []func() + volumes := []oci.ContainerVolume{} ociMounts := []rspec.Mount{} containerConfig := ctr.Config() @@ -195,12 +197,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 @@ -208,22 +210,25 @@ 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() != "" { if s.config.OCIArtifactMountSupport { // Try mountArtifact first, and fall back to mountImage if it fails with ErrNotFound - artifactVolumes, err := s.mountArtifact(ctx, specgen, m, ctrInfo.MountLabel, skipRelabel, maybeRelabel) + artifactVolumes, cleanup, err := s.mountArtifact(ctx, specgen, m, ctrInfo.MountLabel, skipRelabel, maybeRelabel) if err == nil { volumes = append(volumes, artifactVolumes...) + if cleanup != nil { + cleanups = append(cleanups, cleanup) + } continue } // Don't fallback to an image mount if we already encounter an ErrImageVolumeMountFailed error if errors.Is(err, crierrors.ErrImageVolumeMountFailed) { - return nil, nil, nil, err + return nil, nil, nil, nil, err } log.Warnf(ctx, "Artifact mount failed, falling back to image mount: %v", err) @@ -233,7 +238,7 @@ func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, volume, safeMount, err := s.mountImage(ctx, specgen, imageVolumesPath, m, ctrInfo.RunDir, namespace) 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) @@ -243,7 +248,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 == "/" { @@ -262,14 +267,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 s.config.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) } } @@ -282,7 +287,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) } } } @@ -297,17 +302,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") @@ -315,7 +320,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: @@ -329,14 +334,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, ) @@ -344,7 +349,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, ) @@ -358,7 +363,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, ctrInfo.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) @@ -378,7 +383,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{ @@ -407,30 +412,21 @@ func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, specgen.AddMount(m) } - return volumes, ociMounts, safeMounts, nil + return volumes, ociMounts, safeMounts, nil, 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 @@ -448,18 +444,18 @@ func (s *Server) mountArtifact(ctx context.Context, specgen *generate.Generator, paths, err = FilterMountPathsBySubPath(ctx, m.Image.Image, m.ImageSubPath, paths) if err != nil { // This error will get reported directly to the end user - return nil, err + return nil, nil, err } 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 } } @@ -483,7 +479,7 @@ func (s *Server) mountArtifact(ctx context.Context, specgen *generate.Generator, }) } - return volumes, nil + return volumes, paths[0].Cleanup, nil } func FilterMountPathsBySubPath(ctx context.Context, artifact, subPath string, paths []ociartifact.BlobMountPath) (filteredPaths []ociartifact.BlobMountPath, err error) { diff --git a/server/container_create_linux_test.go b/server/container_create_linux_test.go index 938e6e4504c..1443a3e960f 100644 --- a/server/container_create_linux_test.go +++ b/server/container_create_linux_test.go @@ -38,7 +38,7 @@ func TestAddOCIBindsForDev(t *testing.T) { MountLabel: "", } - _, binds, _, err := sut.addOCIBindMounts(t.Context(), ctr, ctrInfo, false, false, false, false, false) + _, binds, _, _, err := sut.addOCIBindMounts(t.Context(), ctr, "", "", nil, false, false, false, false, false, "", "") if err != nil { t.Error(err) } @@ -93,7 +93,7 @@ func TestAddOCIBindsForSys(t *testing.T) { MountLabel: "", } - _, binds, _, err := sut.addOCIBindMounts(t.Context(), ctr, ctrInfo, false, false, false, false, false) + _, binds, _, _, err := sut.addOCIBindMounts(t.Context(), ctr, "", "", nil, false, false, false, false, false, "", "") if err != nil { t.Error(err) } @@ -150,7 +150,7 @@ func TestAddOCIBindsRROMounts(t *testing.T) { MountLabel: "", } - _, binds, _, err := sut.addOCIBindMounts(ctx, ctr, ctrInfo, 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) } @@ -251,7 +251,7 @@ func TestAddOCIBindsRROMountsError(t *testing.T) { MountLabel: "", } - _, _, _, err = sut.addOCIBindMounts(ctx, ctr, ctrInfo, 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") } @@ -287,7 +287,7 @@ func TestAddOCIBindsCGroupRW(t *testing.T) { } //nolint: dogsled - _, _, _, err = sut.addOCIBindMounts(t.Context(), ctr, ctrInfo, false, false, true, false, false) + _, _, _, _, err = sut.addOCIBindMounts(t.Context(), ctr, "", "", nil, false, false, true, false, false, "", "") if err != nil { t.Error(err) } @@ -328,7 +328,7 @@ func TestAddOCIBindsCGroupRW(t *testing.T) { var hasCgroupRO bool //nolint: dogsled - _, _, _, err = sut.addOCIBindMounts(t.Context(), ctr, ctrInfo, false, false, false, false, false) + _, _, _, _, err = sut.addOCIBindMounts(t.Context(), ctr, "", "", nil, false, false, false, false, false, "", "") if err != nil { t.Error(err) } @@ -385,13 +385,13 @@ func TestAddOCIBindsErrorWithoutIDMap(t *testing.T) { } //nolint: dogsled - _, _, _, err = sut.addOCIBindMounts(t.Context(), ctr, ctrInfo, 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, ctrInfo, false, false, false, true, false) + _, _, _, _, err = sut.addOCIBindMounts(t.Context(), ctr, "", "", nil, false, false, false, true, false, "", "") if err != nil { t.Errorf("%v", err) } From 6dfd6f1c71a42da2c671e7cce45a835d9a91567d Mon Sep 17 00:00:00 2001 From: Sohan Kunkerkar Date: Sat, 7 Jun 2025 13:33:45 -0400 Subject: [PATCH 5/5] test Signed-off-by: Sohan Kunkerkar --- internal/oci/container.go | 1 + internal/ociartifact/store.go | 240 +++++++++++++++++++++----- server/container_create_freebsd.go | 14 +- server/container_create_linux.go | 13 +- server/container_create_linux_test.go | 19 +- test/oci_artifacts.bats | 7 +- 6 files changed, 231 insertions(+), 63 deletions(-) diff --git a/internal/oci/container.go b/internal/oci/container.go index f329a012df6..d59a1dd1cc5 100644 --- a/internal/oci/container.go +++ b/internal/oci/container.go @@ -888,6 +888,7 @@ 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/ociartifact/store.go b/internal/ociartifact/store.go index f6d1b418682..d081bc78457 100644 --- a/internal/ociartifact/store.go +++ b/internal/ociartifact/store.go @@ -5,6 +5,7 @@ import ( "compress/gzip" "context" "encoding/hex" + "encoding/json" "errors" "fmt" "io" @@ -41,17 +42,19 @@ var ( // Store is the main structure to build an artifact storage. type Store struct { - rootPath string - systemContext *types.SystemContext - impl Impl + rootPath string + systemContext *types.SystemContext + impl Impl + extractArtifactDir string } // NewStore creates a new OCI artifact store. func NewStore(rootPath string, systemContext *types.SystemContext) *Store { return &Store{ - rootPath: filepath.Join(rootPath, "artifacts"), - systemContext: systemContext, - impl: &defaultImpl{}, + rootPath: filepath.Join(rootPath, "artifacts"), + systemContext: systemContext, + impl: &defaultImpl{}, + extractArtifactDir: filepath.Join(rootPath, "extracted-artifacts"), } } @@ -65,6 +68,26 @@ func (u unknownRef) Name() string { return u.String() } +func (s *Store) getArtifactExtractDir(artifact *Artifact) (string, error) { + // Parse manifest from artifact + manifestBytes, err := s.impl.ToJSON(artifact.Manifest()) + if err != nil { + return "", fmt.Errorf("marshal manifest: %w", err) + } + + parsedManifest, err := s.impl.ManifestFromBlob(manifestBytes, specs.MediaTypeImageManifest) + if err != nil { + return "", fmt.Errorf("parse manifest: %w", err) + } + + configInfo := parsedManifest.ConfigInfo() + if configInfo.Digest == "" { + return "", errors.New("artifact manifest has no config digest") + } + + return filepath.Join(s.extractArtifactDir, configInfo.Digest.Encoded()), nil +} + // PullOptions can be used to customize the pull behavior. type PullOptions struct { // EnforceConfigMediaType can be set to enforce a specific manifest config @@ -217,6 +240,16 @@ func (s *Store) PullManifest( return nil, fmt.Errorf("close copier: %w", err) } + configInfo := parsedManifest.ConfigInfo() + if configInfo.Digest == "" { + return nil, errors.New("manifest has no config digest") + } + + extractDir := filepath.Join(s.extractArtifactDir, configInfo.Digest.Encoded()) + if err := s.extractArtifactLayers(ctx, strRef, parsedManifest, dst, extractDir); err != nil { + return nil, fmt.Errorf("extract artifact layers: %w", err) + } + return manifestBytes, nil } @@ -529,15 +562,17 @@ 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) + extractDir, err := s.getArtifactExtractDir(artifact) + if err != nil { + return nil, fmt.Errorf("get artifact extract dir: %w", err) } - cleanupFunc := func() { os.RemoveAll(extractDir) } + cleanupFunc := func() { + os.RemoveAll(extractDir) + } cleanupOnce := &sync.Once{} - // Cleanup on function exit if we encounter an error + // Cleanup on function exit if we encounter an error. cleanup := true defer func() { if cleanup { @@ -552,106 +587,205 @@ func (s *Store) BlobMountPaths(ctx context.Context, artifact *Artifact, sys *typ defer src.Close() mountPaths := make([]BlobMountPath, 0, len(artifact.Manifest().Layers)) - bic := blobinfocache.DefaultCache(s.systemContext) + data, err := os.ReadFile(filepath.Join(s.extractArtifactDir, "layer-map.json")) + if err != nil { + return nil, fmt.Errorf("failed to read layer map: %w", err) + } + + layerMap := make(map[string]string) + if err := json.Unmarshal(data, &layerMap); err != nil { + return nil, fmt.Errorf("failed to parse layer map: %w", err) + } for _, l := range artifact.Manifest().Layers { + name, exists := layerMap[l.Digest.String()] + if !exists { + log.Warnf(ctx, "No mapping found for layer %s", l.Digest) + + name = artifactName(l.Annotations) + if name == "" { + log.Warnf(ctx, "Unable to find name for artifact layer which makes it not mountable") + + continue + } + } + + filePath := filepath.Join(extractDir, name) + + mountPaths = append(mountPaths, BlobMountPath{ + SourcePath: filePath, + Name: name, + cleanupFunc: cleanupFunc, + cleanupOnce: cleanupOnce, + }) + } + // Disable cleanup on success. + // mountArtifact() will handle the cleanup. + cleanup = false + + return mountPaths, nil +} + +// extractArtifactLayers extracts tar/tar.gz layers from the pulled OCI artifact. +func (s *Store) extractArtifactLayers(ctx context.Context, strRef string, parsedManifest manifest.Manifest, ref types.ImageReference, extractDir string) error { + // Creates top-level extraction directory. + if err := os.MkdirAll(extractDir, 0o755); err != nil { + return fmt.Errorf("failed to create artifact dir: %w", err) + } + + // Cleanup directory if extraction fails + var success bool + defer func() { + if !success { + os.RemoveAll(extractDir) + } + }() + + src, err := ref.NewImageSource(ctx, s.systemContext) + if err != nil { + return fmt.Errorf("failed to get an image source: %w", err) + } + defer src.Close() + + bic := blobinfocache.DefaultCache(s.systemContext) + layerMap := make(map[string]string) + + for _, layer := range s.impl.LayerInfos(parsedManifest) { err = func() error { - blob, _, err := s.impl.GetBlob(ctx, src, types.BlobInfo{Digest: l.Digest}, bic) + blob, _, err := s.impl.GetBlob(ctx, src, types.BlobInfo{Digest: layer.Digest}, bic) if err != nil { return fmt.Errorf("failed to get blob: %w", err) } defer blob.Close() - content, err := s.processLayerContent(l.MediaType, blob) + content, extractedFilename, err := s.processLayerContent(ctx, layer.MediaType, blob) if err != nil { return fmt.Errorf("failed to process layer: %w", err) } - name := artifactName(l.Annotations) + name := extractedFilename if name == "" { - log.Warnf(ctx, "Unable to find name for artifact layer which makes it not mountable") + name = artifactName(layer.Annotations) + } + + if name == "" { + log.Warnf(ctx, "Unable to find name for artifact layer") return nil } + layerMap[layer.Digest.String()] = name filePath := filepath.Join(extractDir, name) + + // Create parent directories for this specific file path. + if err := os.MkdirAll(filepath.Dir(filePath), 0o755); err != nil { + return fmt.Errorf("failed to create parent directories for %s: %w", filePath, err) + } + 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, - 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) + return fmt.Errorf("failed to extract artifact layer: %w", err) } } - // Disable cleanup on success. - // mountArtifact() will handle the cleanup. - cleanup = false - return mountPaths, nil + // Save layer mapping + mapPath := filepath.Join(s.extractArtifactDir, "layer-map.json") + data, err := json.Marshal(layerMap) + if err != nil { + return fmt.Errorf("failed to marshal layer map: %w", err) + } + + if err := os.WriteFile(mapPath, data, 0o644); err != nil { + log.Errorf(ctx, "Failed to write file: %v", err) + } + + log.Debugf(ctx, "Successfully extracted artifact layers for %s", strRef) + + success = true + + return 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) { +func (s *Store) processLayerContent(ctx context.Context, mediaType string, r io.Reader) (content []byte, filename string, err error) { switch { - case strings.HasSuffix(mediaType, ".tar+gzip"): + case strings.Contains(mediaType, ".tar+gzip"): gr, err := gzip.NewReader(r) if err != nil { - return nil, fmt.Errorf("gzip decompress failed: %w", err) + return nil, "", fmt.Errorf("gzip decompress failed: %w", err) } defer gr.Close() - return s.extractSingleFileFromTar(gr) + content, filename, err := s.extractSingleFileFromTar(ctx, gr) + + return content, filename, err - case strings.HasSuffix(mediaType, ".tar+zstd"): + case strings.Contains(mediaType, ".tar+zstd"): zr, err := zstd.NewReader(r) if err != nil { - return nil, fmt.Errorf("zstd decompress failed: %w", err) + return nil, "", fmt.Errorf("zstd decompress failed: %w", err) } defer zr.Close() - return s.extractSingleFileFromTar(zr) + content, filename, err := s.extractSingleFileFromTar(ctx, zr) - case strings.HasSuffix(mediaType, ".tar"): - return s.extractSingleFileFromTar(r) + return content, filename, err + + case strings.Contains(mediaType, ".tar"): + content, filename, err := s.extractSingleFileFromTar(ctx, r) + + return content, filename, err default: data, err := io.ReadAll(r) if err != nil { - return nil, fmt.Errorf("reading blob content: %w", err) + return nil, "", fmt.Errorf("reading blob content: %w", err) } - return data, nil + return data, "", nil } } -func (s *Store) extractSingleFileFromTar(r io.Reader) ([]byte, error) { +func (s *Store) extractSingleFileFromTar(ctx context.Context, r io.Reader) (content []byte, baseName string, err error) { tr := tar.NewReader(r) for { hdr, err := tr.Next() if err == io.EOF { - return nil, errors.New("no files found in tar archive") + return nil, "", errors.New("no files found in tar archive") } if err != nil { - return nil, fmt.Errorf("reading tar header: %w", err) + return nil, "", fmt.Errorf("reading tar header: %w", err) } if hdr.Typeflag == tar.TypeReg { - return io.ReadAll(tr) + // Validate path is safe + safePath, err := sanitizePath("", hdr.Name) + if err != nil { + // Log warning and continue to next file instead of failing immediately + log.Warnf(ctx, "Skipping file with invalid path %s: %v", hdr.Name, err) + + continue + } + + baseName := filepath.Base(safePath) + content, err := io.ReadAll(tr) + if err != nil { + return nil, "", fmt.Errorf("reading file content from tar: %w", err) + } + return content, baseName, nil } } } @@ -667,3 +801,25 @@ func artifactName(annotations map[string]string) string { return "" } + +// sanitizePath prevents path traversal attacks by resolving relative paths +// and ensuring the final path is within the target directory. +func sanitizePath(base, path string) (string, error) { + cleanedPath := filepath.Clean(path) + + if strings.Contains(cleanedPath, "..") || strings.HasPrefix(cleanedPath, "/") { + return "", fmt.Errorf("illegal path traversal: %s", path) + } + + // Join with base directory if provided + if base != "" { + target := filepath.Join(base, cleanedPath) + // Verify the path stays within the base directory + if !strings.HasPrefix(target, base) { + return "", fmt.Errorf("path escapes base directory: %s", path) + } + return target, nil + } + + return cleanedPath, nil +} diff --git a/server/container_create_freebsd.go b/server/container_create_freebsd.go index 621fd5c5d4c..ac95006a534 100644 --- a/server/container_create_freebsd.go +++ b/server/container_create_freebsd.go @@ -45,7 +45,7 @@ func addSysfsMounts(ctr ctrfactory.Container, containerConfig *types.ContainerCo func setOCIBindMountsPrivileged(g *generate.Generator) { } -func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, ctrInfo *storage.ContainerInfo, maybeRelabel, skipRelabel, cgroup2RW, idMapSupport, rroSupport bool) ([]oci.ContainerVolume, []rspec.Mount, []*safeMountInfo, error) { +func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, ctrInfo *storage.ContainerInfo, maybeRelabel, skipRelabel, cgroup2RW, idMapSupport, rroSupport bool) ([]oci.ContainerVolume, []rspec.Mount, []*safeMountInfo, []func(), error) { ctx, span := log.StartSpan(ctx) defer span.End() @@ -88,11 +88,11 @@ func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, for _, m := range mounts { dest := m.ContainerPath if dest == "" { - return nil, nil, nil, fmt.Errorf("mount.ContainerPath is empty") + return nil, nil, nil, nil, fmt.Errorf("mount.ContainerPath is empty") } // TODO: add support for image mounts here if m.HostPath == "" { - return nil, nil, nil, fmt.Errorf("mount.HostPath is empty") + return nil, nil, nil, nil, fmt.Errorf("mount.HostPath is empty") } if m.HostPath == "/" && dest == "/" { log.Warnf(ctx, "Configuration specifies mounting host root to the container root. This is dangerous (especially with privileged containers) and should be avoided.") @@ -104,13 +104,13 @@ 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 s.config.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) } } if !ctr.Restore() { @@ -122,7 +122,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: %s", src, err) + return nil, nil, nil, nil, fmt.Errorf("failed to mkdir %s: %s", src, err) } } } @@ -147,7 +147,7 @@ func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, }) } - return volumes, ociMounts, nil, nil + return volumes, ociMounts, nil, nil, nil } func addShmMount(ctr ctrfactory.Container, sb *sandbox.Sandbox) { diff --git a/server/container_create_linux.go b/server/container_create_linux.go index 95050821eb6..d29efb629c9 100644 --- a/server/container_create_linux.go +++ b/server/container_create_linux.go @@ -221,8 +221,10 @@ func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, volumes = append(volumes, artifactVolumes...) if cleanup != nil { - cleanups = append(cleanups, cleanup) + //nolint:staticcheck + cleanups = append(cleanups, cleanup...) } + continue } @@ -416,7 +418,7 @@ func (s *Server) addOCIBindMounts(ctx context.Context, ctr ctrfactory.Container, } // 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, func(), 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, nil, fmt.Errorf("failed to get artifact status: %w", err) @@ -429,6 +431,7 @@ func (s *Server) mountArtifact(ctx context.Context, specgen *generate.Generator, options := []string{"bind", "ro"} volumes := make([]oci.ContainerVolume, 0, len(paths)) + cleanups := []func(){} selinuxRelabel := true if !m.SelinuxRelabel { @@ -477,9 +480,13 @@ func (s *Server) mountArtifact(ctx context.Context, specgen *generate.Generator, UIDMappings: getOCIMappings(m.UidMappings), GIDMappings: getOCIMappings(m.GidMappings), }) + // Add cleanup function to call when this specific mount is no longer needed + cleanups = append(cleanups, func() { + path.Cleanup() + }) } - return volumes, paths[0].Cleanup, nil + return volumes, cleanups, nil } func FilterMountPathsBySubPath(ctx context.Context, artifact, subPath string, paths []ociartifact.BlobMountPath) (filteredPaths []ociartifact.BlobMountPath, err error) { diff --git a/server/container_create_linux_test.go b/server/container_create_linux_test.go index 1443a3e960f..258ad50d220 100644 --- a/server/container_create_linux_test.go +++ b/server/container_create_linux_test.go @@ -38,7 +38,8 @@ func TestAddOCIBindsForDev(t *testing.T) { MountLabel: "", } - _, binds, _, _, err := sut.addOCIBindMounts(t.Context(), ctr, "", "", nil, false, false, false, false, false, "", "") + //nolint:dogsled + _, binds, _, _, err := sut.addOCIBindMounts(t.Context(), ctr, ctrInfo, false, false, false, false, false) if err != nil { t.Error(err) } @@ -93,7 +94,8 @@ func TestAddOCIBindsForSys(t *testing.T) { MountLabel: "", } - _, binds, _, _, err := sut.addOCIBindMounts(t.Context(), ctr, "", "", nil, false, false, false, false, false, "", "") + //nolint:dogsled + _, binds, _, _, err := sut.addOCIBindMounts(t.Context(), ctr, ctrInfo, false, false, false, false, false) if err != nil { t.Error(err) } @@ -150,7 +152,8 @@ func TestAddOCIBindsRROMounts(t *testing.T) { MountLabel: "", } - _, binds, _, _, err := sut.addOCIBindMounts(ctx, ctr, "", "", nil, false, false, false, false, true, "", "") + //nolint:dogsled + _, binds, _, _, err := sut.addOCIBindMounts(ctx, ctr, ctrInfo, false, false, false, false, true) if err != nil { t.Errorf("Should not fail to create RRO mount, got: %v", err) } @@ -251,7 +254,7 @@ func TestAddOCIBindsRROMountsError(t *testing.T) { MountLabel: "", } - _, _, _, _, err = sut.addOCIBindMounts(ctx, ctr, "", "", nil, false, false, false, false, tc.rroSupport, "", "") + _, _, _, _, err = sut.addOCIBindMounts(ctx, ctr, ctrInfo, false, false, false, false, tc.rroSupport) if err == nil { t.Error("Should fail to add an RRO mount with a specific error") } @@ -287,7 +290,7 @@ func TestAddOCIBindsCGroupRW(t *testing.T) { } //nolint: dogsled - _, _, _, _, err = sut.addOCIBindMounts(t.Context(), ctr, "", "", nil, false, false, true, false, false, "", "") + _, _, _, _, err = sut.addOCIBindMounts(t.Context(), ctr, ctrInfo, false, false, true, false, false) if err != nil { t.Error(err) } @@ -328,7 +331,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, ctrInfo, false, false, false, false, false) if err != nil { t.Error(err) } @@ -385,13 +388,13 @@ func TestAddOCIBindsErrorWithoutIDMap(t *testing.T) { } //nolint: dogsled - _, _, _, _, err = sut.addOCIBindMounts(t.Context(), ctr, "", "", nil, false, false, false, false, false, "", "") + _, _, _, _, err = sut.addOCIBindMounts(t.Context(), ctr, ctrInfo, 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, ctrInfo, false, false, false, true, false) if err != nil { t.Errorf("%v", err) } diff --git a/test/oci_artifacts.bats b/test/oci_artifacts.bats index af045f06c55..b13a7350264 100755 --- a/test/oci_artifacts.bats +++ b/test/oci_artifacts.bats @@ -33,7 +33,7 @@ ARTIFACT_IMAGE_SUBPATH="$ARTIFACT_REPO:subpath" mkdir -p "$CONTAINER_REGISTRIES_CONF_DIR" printf 'unqualified-search-registries = ["quay.io"]' >> "$CONTAINER_REGISTRIES_CONF_DIR/99-registry.conf" - IMAGE=crio/artifact:singlefile + IMAGE=sohankunkerkar/artifact:singlefile CONTAINER_REGISTRIES_CONF_DIR=$CONTAINER_REGISTRIES_CONF_DIR start_crio cleanup_images crictl pull $IMAGE @@ -43,7 +43,7 @@ ARTIFACT_IMAGE_SUBPATH="$ARTIFACT_REPO:subpath" [ "$output" != "" ] # Should be available on the whole list - crictl images | grep -qE "quay.io/crio/artifact.*singlefile" + crictl images | grep -qE "quay.io/sohankunkerkar/artifact.*singlefile" } @test "should be able to inspect an OCI artifact" { @@ -285,6 +285,7 @@ EOF ctr_id=$(crictl run "$TESTDIR/container_config.json" "$TESTDATA/sandbox_config.json") run crictl exec "$ctr_id" sha256sum /root/artifact/cri-o/bin/crio [[ "$output" == *"ae5d192303e5f9a357c6ea39308338956b62b8830fd05f0460796db2215c2b35"* ]] +} @test "should extract mounted image artifact files correctly" { start_crio @@ -299,7 +300,7 @@ EOF }]' \ "$TESTDATA"/container_sleep.json > "$TESTDIR/container.json" - ctr_id=$(crictl run "$TESTDIR/container.json" "$TESTDATA/sandbox_config.json") + ctr_id=$(crictl create "$pod_id" "$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" ]