Skip to content

Commit

Permalink
Update based on pr feedbacks
Browse files Browse the repository at this point in the history
  • Loading branch information
@sluongng
sluongng committed Dec 12, 2024
1 parent d9b6619 commit bfe7051
Show file tree
Hide file tree
Showing 3 changed files with 46 additions and 87 deletions.
67 changes: 1 addition & 66 deletions enterprise/server/remote_execution/containers/ociruntime/BUILD
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,7 @@ go_test(
"test.workload-isolation-type": "firecracker",
"test.container-image": "docker://gcr.io/flame-public/net-tools@sha256:ac701954d2c522d0d2b5296323127cacaaf77627e69db848a8d6ecb53149d344",
"test.EstimatedComputeUnits": "2",
"test.EstimatedFreeDiskBytes": "10GB",
},
# NOTE: if testing locally, use --test_sharding_strategy=disabled to work
# around the networking package not supporting cross-process locks.
Expand Down Expand Up @@ -105,72 +106,6 @@ 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({
Expand Down
55 changes: 36 additions & 19 deletions enterprise/server/remote_execution/containers/ociruntime/ociruntime.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1282,6 +1282,10 @@ func (s *ImageStore) pull(ctx context.Context, imageName string, creds oci.Crede

// downloadLayer downloads and extracts the given layer to the given destination
// dir. The extracted layer is suitable for use as an overlayfs lowerdir.
//
// For reference implementations, see:
// - Podman: https://github.com/containers/storage/blob/664fe5d9b95004e1be3eee004d56a1715c8ca790/pkg/archive/archive.go#L707-L729
// - Moby (Docker): https://github.com/moby/moby/blob/9633556bef3eb20dfe888903660c3df89a73605b/pkg/archive/archive.go#L726-L735
func downloadLayer(ctx context.Context, layer ctr.Layer, destDir string) error {
rc, err := layer.Uncompressed()
if err != nil {
Expand All @@ -1308,9 +1312,14 @@ func downloadLayer(ctx context.Context, layer ctr.Layer, destDir string) error {
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)

// filepath.Join applies filepath.Clean to all arguments
file := filepath.Join(tempUnpackDir, header.Name)
base := filepath.Base(file)
dir := filepath.Dir(file)
if !strings.HasPrefix(file, tempUnpackDir) {
return status.UnavailableErrorf("breakout attempt detected with tar entry: %q", header.Name)
}

const whiteoutPrefix = ".wh."
// Handle whiteout
Expand All @@ -1332,16 +1341,25 @@ func downloadLayer(ctx context.Context, layer ctr.Layer, destDir string) error {
continue
}

if header.Typeflag == tar.TypeDir ||
header.Typeflag == tar.TypeReg ||
header.Typeflag == tar.TypeSymlink ||
header.Typeflag == tar.TypeLink {
if err := os.MkdirAll(dir, os.ModePerm); err != nil {
return status.UnavailableErrorf("create directory: %s", err)
}
} else {
log.CtxInfof(ctx, "Ignoring unsupported tar header %q type %q in oci layer", header.Name, header.Typeflag)
continue
}

switch header.Typeflag {
case tar.TypeDir:
if err := os.MkdirAll(target, os.FileMode(header.Mode)); err != nil {
if err := os.MkdirAll(file, os.FileMode(header.Mode)); err != nil {
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))
f, err := os.OpenFile(file, os.O_CREATE|os.O_WRONLY, os.FileMode(header.Mode))
if err != nil {
return status.UnavailableErrorf("create file: %s", err)
}
Expand All @@ -1355,25 +1373,24 @@ func downloadLayer(ctx context.Context, layer ctr.Layer, destDir string) error {
}
f.Close()
case tar.TypeSymlink:
if err := os.Symlink(header.Linkname, target); err != nil {
// Symlink's target is only evaluated at runtime, inside the container context.
// So it's safe to have the symlink targeting paths outside unpackdir.
if err := os.Symlink(header.Linkname, file); err != nil {
return status.UnavailableErrorf("create symlink: %s", err)
}
if err := os.Lchown(target, header.Uid, header.Gid); err != nil {
if err := os.Lchown(file, 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)
target := filepath.Join(tempUnpackDir, header.Linkname)
if !strings.HasPrefix(target, tempUnpackDir) {
return status.UnavailableErrorf("breakout attempt detected with link: %q -> %q", header.Name, header.Linkname)
}
source := filepath.Join(tempUnpackDir, filepath.Clean(header.Linkname))
if err := os.Link(source, target); err != nil {
// Note that this will call linkat(2) without AT_SYMLINK_FOLLOW,
// so if target is a symlink, the hardlink will point to the symlink itself and not the symlink target.
if err := os.Link(target, file); 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:
log.CtxInfof(ctx, "Ignoring unsupported tar header %q type %q in oci layer", header.Name, header.Typeflag)
}
}

Expand Down
11 changes: 9 additions & 2 deletions enterprise/server/remote_execution/containers/ociruntime/ociruntime_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1411,9 +1411,16 @@ func hasMountPermissions(t *testing.T) bool {
//
// Example run way to run this test:
//
// bazel test --config=remote enterprise/server/remote_execution/containers/ociruntime:ociruntime_pull_test --test_output=all
// bazel test \
// --config=remote \
// --test_output=all \
// --test_sharding_strategy=disabled \
// --test_tag_filters=+docker \
// --test_filter=TestPullImage \
// --test_env=TEST_PULLIMAGE=1 \
// enterprise/server/remote_execution/containers/ociruntime:ociruntime_test
func TestPullImage(t *testing.T) {
if os.Getenv("BB_INTEGRATION") == "" {
if os.Getenv("TEST_PULLIMAGE") == "" {
t.Skip("Skipping integration test..")
}

Expand Down

0 comments on commit bfe7051

Please sign in to comment.