From addf08545cb098d37b75dbd02594981272f667de Mon Sep 17 00:00:00 2001 From: Son Luong Ngoc Date: Fri, 6 Dec 2024 16:39:58 +0100 Subject: [PATCH 1/3] Reapply "ociruntime: replace tar extraction subprocess with in-process (#7589)" (#8019) This reverts commit f45d620928dde3e1f6c486a704dd22240c5e81eb. --- .../containers/ociruntime/ociruntime.go | 104 +++++++++++------- 1 file changed, 65 insertions(+), 39 deletions(-) diff --git a/enterprise/server/remote_execution/containers/ociruntime/ociruntime.go b/enterprise/server/remote_execution/containers/ociruntime/ociruntime.go index 58305ed35f2..98aef8db83b 100644 --- a/enterprise/server/remote_execution/containers/ociruntime/ociruntime.go +++ b/enterprise/server/remote_execution/containers/ociruntime/ociruntime.go @@ -1,6 +1,7 @@ package ociruntime import ( + "archive/tar" "bytes" "context" "crypto/rand" @@ -8,7 +9,7 @@ import ( "errors" "flag" "fmt" - "io/fs" + "io" "os" "os/exec" "path/filepath" @@ -1293,58 +1294,83 @@ func downloadLayer(ctx context.Context, layer ctr.Layer, destDir string) error { } defer os.RemoveAll(tempUnpackDir) - // TODO: avoid tar command. - cmd := exec.CommandContext(ctx, "tar", "--extract", "--directory", tempUnpackDir) - var stderr bytes.Buffer - cmd.Stdin = rc - cmd.Stderr = &stderr - cmd.SysProcAttr = &syscall.SysProcAttr{Setpgid: true} - if err := cmd.Run(); err != nil { - return status.UnavailableErrorf("download and extract layer tarball: %s: %q", err, stderr.String()) - } - - // Convert whiteout files to overlayfs format. - err = filepath.WalkDir(tempUnpackDir, func(path string, entry fs.DirEntry, err error) error { + tr := tar.NewReader(rc) + for { + header, err := tr.Next() + if err == io.EOF { + break + } if err != nil { - return err + return status.UnavailableErrorf("download and extract layer tarball: %s", err) } - if !entry.Type().IsRegular() { - return nil + if slices.Contains(strings.Split(header.Name, string(os.PathSeparator)), "..") { + return status.UnavailableErrorf("tar entry is not clean: %q", header.Name) } + target := filepath.Join(tempUnpackDir, filepath.Clean(header.Name)) + base := filepath.Base(target) + dir := filepath.Dir(target) - base := filepath.Base(path) - dir := filepath.Dir(path) const whiteoutPrefix = ".wh." - - // Directory whiteouts - if base == whiteoutPrefix+whiteoutPrefix+".opq" { - if err := unix.Setxattr(dir, "trusted.overlay.opaque", []byte{'y'}, 0); err != nil { - return fmt.Errorf("setxattr on deleted dir: %w", err) - } - if err := os.Remove(path); err != nil { - return fmt.Errorf("remove directory whiteout marker: %w", err) + // Handle whiteout + if strings.HasPrefix(base, whiteoutPrefix) { + // Directory whiteout + if base == whiteoutPrefix+whiteoutPrefix+".opq" { + if err := unix.Setxattr(dir, "trusted.overlay.opaque", []byte{'y'}, 0); err != nil { + return status.UnavailableErrorf("setxattr on deleted dir: %s", err) + } + continue } - return nil - } - // File whiteouts - if strings.HasPrefix(base, whiteoutPrefix) { + // File whiteout: Mark the file for deletion in overlayfs. originalBase := base[len(whiteoutPrefix):] originalPath := filepath.Join(dir, originalBase) if err := unix.Mknod(originalPath, unix.S_IFCHR, 0); err != nil { - return fmt.Errorf("mknod for whiteout marker: %w", err) - } - if err := os.Remove(path); err != nil { - return fmt.Errorf("remove directory whiteout marker: %w", err) + return status.UnavailableErrorf("mknod for whiteout marker: %s", err) } - return nil + continue } - return nil - }) - if err != nil { - return status.UnavailableErrorf("walk layer dir: %s", err) + switch header.Typeflag { + case tar.TypeDir: + if err := os.MkdirAll(target, os.FileMode(header.Mode)); err != nil { + return status.UnavailableErrorf("create directory: %s", err) + } + case tar.TypeReg: + f, err := os.OpenFile(target, os.O_CREATE|os.O_WRONLY, os.FileMode(header.Mode)) + if err != nil { + return status.UnavailableErrorf("create file: %s", err) + } + if _, err := io.Copy(f, tr); err != nil { + f.Close() + return status.UnavailableErrorf("copy file content: %s", err) + } + if err := f.Chown(header.Uid, header.Gid); err != nil { + f.Close() + return status.UnavailableErrorf("chown file: %s", err) + } + f.Close() + case tar.TypeSymlink: + if err := os.Symlink(header.Linkname, target); err != nil { + return status.UnavailableErrorf("create symlink: %s", err) + } + if err := os.Lchown(target, header.Uid, header.Gid); err != nil { + return status.UnavailableErrorf("chown link: %s", err) + } + case tar.TypeLink: + if slices.Contains(strings.Split(header.Linkname, string(os.PathSeparator)), "..") { + return status.UnavailableErrorf("tar entry is not clean: %q", header.Name) + } + source := filepath.Join(tempUnpackDir, filepath.Clean(header.Linkname)) + if err := os.Link(source, target); err != nil { + return status.UnavailableErrorf("create hard link: %s", err) + } + if err := os.Chown(target, header.Uid, header.Gid); err != nil { + return status.UnavailableErrorf("chown file: %s", err) + } + default: + return status.UnavailableErrorf("unsupported tar entry type %q", header.Typeflag) + } } if err := os.Rename(tempUnpackDir, destDir); err != nil { From d9b119da3b31b8c7ef314a6aad30c0c938dd0655 Mon Sep 17 00:00:00 2001 From: Son Luong Ngoc Date: Fri, 6 Dec 2024 16:52:56 +0100 Subject: [PATCH 2/3] Add a pull test --- .../containers/ociruntime/BUILD | 66 +++++++++++++++++++ .../containers/ociruntime/ociruntime_test.go | 53 +++++++++++++++ 2 files changed, 119 insertions(+) diff --git a/enterprise/server/remote_execution/containers/ociruntime/BUILD b/enterprise/server/remote_execution/containers/ociruntime/BUILD index e160453e01c..6c283186e1c 100644 --- a/enterprise/server/remote_execution/containers/ociruntime/BUILD +++ b/enterprise/server/remote_execution/containers/ociruntime/BUILD @@ -102,6 +102,72 @@ go_test( ], ) +# keep +go_test( + name = "ociruntime_pull_test", + srcs = ["ociruntime_test.go"], + args = ["-test.run=TestPullImage"], + data = [ + ":busybox", + ":crun", + "//enterprise/server/remote_execution/runner/testworker", + "@busybox", + ], + env = { + "BB_INTEGRATION": "true", + }, + exec_properties = { + "test.workload-isolation-type": "firecracker", + "test.container-image": "docker://gcr.io/flame-public/net-tools@sha256:ac701954d2c522d0d2b5296323127cacaaf77627e69db848a8d6ecb53149d344", + "test.EstimatedComputeUnits": "2", + "test.EstimatedFreeDiskBytes": "10GB", + }, + tags = [ + "manual", + ], + target_compatible_with = [ + "@platforms//os:linux", + # TODO: GitHub Actions arm runners are running into cgroup-related issues; + # fix and re-enable on arm64. + "@platforms//cpu:x86_64", + ], + x_defs = { + "crunRlocationpath": "$(rlocationpath :crun)", + "busyboxRlocationpath": "$(rlocationpath :busybox)", + "ociBusyboxRlocationpath": "$(rlocationpath @busybox)", + "testworkerRlocationpath": "$(rlocationpath //enterprise/server/remote_execution/runner/testworker)", + }, + deps = [ + ":ociruntime", + "//enterprise/server/remote_execution/container", + "//enterprise/server/remote_execution/persistentworker", + "//enterprise/server/remote_execution/platform", + "//enterprise/server/remote_execution/workspace", + "//enterprise/server/util/oci", + "//proto:remote_execution_go_proto", + "//proto:scheduler_go_proto", + "//proto:worker_go_proto", + "//server/interfaces", + "//server/testutil/testenv", + "//server/testutil/testfs", + "//server/testutil/testnetworking", + "//server/testutil/testregistry", + "//server/testutil/testshell", + "//server/testutil/testtar", + "//server/util/disk", + "//server/util/proto", + "//server/util/status", + "//server/util/testing/flags", + "//server/util/uuid", + "@com_github_google_go_containerregistry//pkg/crane", + "@com_github_google_go_containerregistry//pkg/v1:pkg", + "@com_github_google_go_containerregistry//pkg/v1/mutate", + "@com_github_stretchr_testify//assert", + "@com_github_stretchr_testify//require", + "@io_bazel_rules_go//go/runfiles:go_default_library", + ], +) + alias( name = "crun", actual = select({ diff --git a/enterprise/server/remote_execution/containers/ociruntime/ociruntime_test.go b/enterprise/server/remote_execution/containers/ociruntime/ociruntime_test.go index ce24078395e..a2d80f9591d 100644 --- a/enterprise/server/remote_execution/containers/ociruntime/ociruntime_test.go +++ b/enterprise/server/remote_execution/containers/ociruntime/ociruntime_test.go @@ -1405,3 +1405,56 @@ func ociCPUSharesToCgroup2Weight(shares int64) int64 { // See https://github.com/containers/crun/blob/main/crun.1.md#cpu-controller return (1 + ((shares-2)*9999)/262142) } + +// TestPullImage is a simple integration test that pulls images from a public repository. +// Can be used as a smoke test to verify that the image pulling functionality works. +// This is setup as a separate, manual test target in BUILD file to help aid development. +// +// Example run way to run this test: +// +// bazel test --config=remote enterprise/server/remote_execution/containers/ociruntime:ociruntime_pull_test --test_output=all +func TestPullImage(t *testing.T) { + if os.Getenv("BB_INTEGRATION") == "" { + t.Skip("Skipping integration test. Set env var 'integration' to run this test.") + } + + for _, tc := range []struct { + name string + image string + }{ + { + name: "dockerhub_busybox", + image: "busybox:latest", + }, + { + name: "ghcr_nix", + image: "ghcr.io/avdv/nix-build@sha256:5f731adacf7290352fed6c1960dfb56ec3fdb31a376d0f2170961fbc96944d50", + }, + { + name: "executor_image", + image: "gcr.io/flame-public/buildbuddy-executor-enterprise:latest", + }, + { + name: "executor_docker", + image: "gcr.io/flame-public/executor-docker-default:enterprise-v1.6.0", + }, + { + name: "workflow_2004", + image: "gcr.io/flame-public/rbe-ubuntu20-04:latest", + }, + { + name: "workflow_2204", + image: "gcr.io/flame-public/rbe-ubuntu20-04:latest", + }, + } { + t.Run(tc.name, func(t *testing.T) { + layerDir := t.TempDir() + imgStore := ociruntime.NewImageStore(layerDir) + + ctx := context.Background() + img, err := imgStore.Pull(ctx, tc.image, oci.Credentials{}) + require.NoError(t, err) + require.NotNil(t, img) + }) + } +} From 9f4b3cd4610a7bbb3f2a7c66f24ee95ce7d9e38c Mon Sep 17 00:00:00 2001 From: Son Luong Ngoc Date: Fri, 6 Dec 2024 16:53:11 +0100 Subject: [PATCH 3/3] ociruntime: fix error from pulling our ubuntu1604 image --- .../remote_execution/containers/ociruntime/ociruntime.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/enterprise/server/remote_execution/containers/ociruntime/ociruntime.go b/enterprise/server/remote_execution/containers/ociruntime/ociruntime.go index 98aef8db83b..31ca88d59cf 100644 --- a/enterprise/server/remote_execution/containers/ociruntime/ociruntime.go +++ b/enterprise/server/remote_execution/containers/ociruntime/ociruntime.go @@ -1337,6 +1337,9 @@ func downloadLayer(ctx context.Context, layer ctr.Layer, destDir string) error { return status.UnavailableErrorf("create directory: %s", err) } case tar.TypeReg: + if err := os.MkdirAll(dir, os.ModePerm); err != nil { + return status.UnavailableErrorf("create directory: %s", err) + } f, err := os.OpenFile(target, os.O_CREATE|os.O_WRONLY, os.FileMode(header.Mode)) if err != nil { return status.UnavailableErrorf("create file: %s", err) @@ -1369,7 +1372,7 @@ func downloadLayer(ctx context.Context, layer ctr.Layer, destDir string) error { return status.UnavailableErrorf("chown file: %s", err) } default: - return status.UnavailableErrorf("unsupported tar entry type %q", header.Typeflag) + log.CtxInfof(ctx, "Ignoring unsupported tar header %q type %q in oci layer", header.Name, header.Typeflag) } }