Preserve paths under hermetic `/tmp` in the sandbox by fmeum · Pull Request #22001 · bazelbuild/bazel

fmeum · 2024-04-15T14:30:11Z

The bind mounting scheme used with the Linux sandbox' hermetic /tmp feature is modified to preserve all paths as they are outside the sandbox, which removes the need to rewrite paths when staging inputs into and, crucially, moving outputs out of the sandbox.

Source roots and output base paths under /tmp are now treated just like any user-specified bind mount under /tmp: They are mounted under the hermetic tmp directory with their path relativized against /tmp before the hermetic tmp directory is mounted as /tmp as the final step.

There is one caveat compared to user-specified mounts: Source roots, which may themselves not lie under /tmp, can be symlinks to directories under /tmp (e.g., when they arise from a local_repository). To handle this situation in the common case, all parent directories of package path entries (up to direct children of /tmp) are mounted into the sandbox. If users use local_repositorys with fixed target paths under /tmp, they will need to specify --sandbox_add_mount_pair.

Overlayfs has been considered as an alternative to this approach, but ultimately doesn't seem to work for this use case since its lowerpath, which would be /tmp, is not allowed to have child mounts from a different user namespace (see https://unix.stackexchange.com/questions/776030/mounting-overlayfs-in-a-user-namespace-with-child-mounts). However, this is exactly the situation created by a Bazel-in-Bazel test and can also arise if the user has existing mounts under /tmp when using Bazel (e.g. the JetBrains toolbox on Linux uses such mounts).

This replaces and mostly reverts the following commits, but keeps their tests:

bf6ebe9
fb6658c
bc1d9d3
1829883
70691f2
a556969
8e32f44 (had its test lost in an incorrect merge conflict resolution, this PR adds it back)

Fixes #20533
Work towards #20753
Fixes #21215
Fixes #22117
Fixes #22226
Fixes #22290

RELNOTES: Paths in the Linux sandbox are now again identical to those outside the sandbox, even with --incompatible_sandbox_hermetic_tmp.

fmeum · 2024-05-08T17:03:16Z

@bazel-io fork 7.2.0

fmeum · 2024-05-08T17:04:16Z

src/test/shell/bazel/bazel_sandboxing_test.sh

  bazel build //pkg:a &>$TEST_log || fail "expected build to succeed"
 }

+# Sets up targets under //test that, when building //test:all, verify that the


I introduced this check instead of checking for whether /tmp is shared with the host directly. We don't care so much about the latter, so I felt that encoding it in tests would make them more brittle as we explore alternative options.

oquenchil · 2024-05-10T15:38:58Z

Thanks a lot for the PR Fabian.

#22226 was broken because of hermetic tmp.

The error used to be:

external/protobuf~/src/google/protobuf/port.cc:12:10: fatal error: google/protobuf/port_def.inc: No such file or directory
   12 | #include "google/protobuf/port_def.inc"

Now the error is:

Could not make proto path relative: bazel-out/k8-fastbuild/bin/external/protobuf~/src/google/protobuf/_virtual_imports/descriptor_proto/google/protobuf/descriptor.proto: No such file or directory

Do you think this is fixable in this PR?

fmeum · 2024-05-12T09:08:00Z

I extended an existing integration test to cover what I think is the root cause of the issue you linked: Neither the logic on `master` nor the current PR track external roots that only arise as targets of resolved symlink artifacts. We probably need similar logic to that in fb6658c to handle this. That commit by itself doesn't suffice since it only covers source roots on the package path.

…correctly." This reverts commit bf6ebe9.

…t manifested themselves when the output base was under `/tmp`:" This reverts commit fb6658c.

…root." This reverts commit bc1d9d3, but keeps the sorting of writable dirs.

This reverts commit 5e68afd.

This reverts commit 620d617.

fmeum · 2024-05-14T14:08:13Z

Tests are passing 🎉

oquenchil

Amazing contribution Fabian. A lot of bug fixes with simpler code than before 🥳

LGTM

tjgq · 2024-05-16T12:42:56Z

This is amazing work, Fabian! Thank you.

fmeum · 2024-05-16T12:47:26Z

Just noticed that the PR description was out of date, I updated it.

The bind mounting scheme used with the Linux sandbox' hermetic `/tmp` feature is modified to preserve all paths as they are outside the sandbox, which removes the need to rewrite paths when staging inputs into and, crucially, moving outputs out of the sandbox. Source roots and output base paths under `/tmp` are now treated just like any user-specified bind mount under `/tmp`: They are mounted under the hermetic tmp directory with their path relativized against `/tmp` before the hermetic tmp directory is mounted as `/tmp` as the final step. There is one caveat compared to user-specified mounts: Source roots, which may themselves not lie under `/tmp`, can be symlinks to directories under `/tmp` (e.g., when they arise from a `local_repository`). To handle this situation in the common case, all parent directories of package path entries (up to direct children of `/tmp`) are mounted into the sandbox. If users use `local_repository`s with fixed target paths under `/tmp`, they will need to specify `--sandbox_add_mount_pair`. Overlayfs has been considered as an alternative to this approach, but ultimately doesn't seem to work for this use case since its `lowerpath`, which would be `/tmp`, is not allowed to have child mounts from a different user namespace (see https://unix.stackexchange.com/questions/776030/mounting-overlayfs-in-a-user-namespace-with-child-mounts). However, this is exactly the situation created by a Bazel-in-Bazel test and can also arise if the user has existing mounts under `/tmp` when using Bazel (e.g. the JetBrains toolbox on Linux uses such mounts). This replaces and mostly reverts the following commits, but keeps their tests: * bazelbuild@bf6ebe9 * bazelbuild@fb6658c * bazelbuild@bc1d9d3 * bazelbuild@1829883 * bazelbuild@70691f2 * bazelbuild@a556969 * bazelbuild@8e32f44 (had its test lost in an incorrect merge conflict resolution, this PR adds it back) Fixes bazelbuild#20533 Work towards bazelbuild#20753 Fixes bazelbuild#21215 Fixes bazelbuild#22117 Fixes bazelbuild#22226 Fixes bazelbuild#22290 RELNOTES: Paths in the Linux sandbox are now again identical to those outside the sandbox, even with `--incompatible_sandbox_hermetic_tmp`. Closes bazelbuild#22001. PiperOrigin-RevId: 634381503 Change-Id: I9f7f3948c705be120c55c9b0c51204e5bea45f61

The bind mounting scheme used with the Linux sandbox' hermetic `/tmp` feature is modified to preserve all paths as they are outside the sandbox, which removes the need to rewrite paths when staging inputs into and, crucially, moving outputs out of the sandbox. Source roots and output base paths under `/tmp` are now treated just like any user-specified bind mount under `/tmp`: They are mounted under the hermetic tmp directory with their path relativized against `/tmp` before the hermetic tmp directory is mounted as `/tmp` as the final step. There is one caveat compared to user-specified mounts: Source roots, which may themselves not lie under `/tmp`, can be symlinks to directories under `/tmp` (e.g., when they arise from a `local_repository`). To handle this situation in the common case, all parent directories of package path entries (up to direct children of `/tmp`) are mounted into the sandbox. If users use `local_repository`s with fixed target paths under `/tmp`, they will need to specify `--sandbox_add_mount_pair`. Overlayfs has been considered as an alternative to this approach, but ultimately doesn't seem to work for this use case since its `lowerpath`, which would be `/tmp`, is not allowed to have child mounts from a different user namespace (see https://unix.stackexchange.com/questions/776030/mounting-overlayfs-in-a-user-namespace-with-child-mounts). However, this is exactly the situation created by a Bazel-in-Bazel test and can also arise if the user has existing mounts under `/tmp` when using Bazel (e.g. the JetBrains toolbox on Linux uses such mounts). This replaces and mostly reverts the following commits, but keeps their tests: * bf6ebe9 * fb6658c * bc1d9d3 * 1829883 * 70691f2 * a556969 * 8e32f44 (had its test lost in an incorrect merge conflict resolution, this PR adds it back) Fixes #20533 Work towards #20753 Fixes #21215 Fixes #22117 Fixes #22226 Fixes #22290 RELNOTES: Paths in the Linux sandbox are now again identical to those outside the sandbox, even with `--incompatible_sandbox_hermetic_tmp`. Closes #22001. PiperOrigin-RevId: 634381503 Change-Id: I9f7f3948c705be120c55c9b0c51204e5bea45f61 Fixes #22291

This was apparently broken with the Bazel 7 upgrade and bazelbuild/bazel#22001 specifically. If `--test_tmpdir` is set to some directory under `/tmp`, we need to add `/tmp` as a mount pair as well. This cannot be done in remote mode so `doctor` needs to be aware of this. Closes: cockroachdb#128204 Epic: None Release note: None Release justification: Build-only code changes

128207: dev: in `doctor`, add `--sandbox_add_mount_pair` if relevant r=rail a=rickystewart This was apparently broken with the Bazel 7 upgrade and bazelbuild/bazel#22001 specifically. If `--test_tmpdir` is set to some directory under `/tmp`, we need to add `/tmp` as a mount pair as well. This cannot be done in remote mode so `doctor` needs to be aware of this. Closes: #128204 Epic: None Release note: None Release justification: Build-only code changes 128211: dev: refactor prompts for autofix permission r=rail a=rickystewart This code is duplicated in many places, so this refactor saves us some LOC. Epic: none Release note: None Release justification: Build-only code changes Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com>

This was apparently broken with the Bazel 7 upgrade and bazelbuild/bazel#22001 specifically. If `--test_tmpdir` is set to some directory under `/tmp`, we need to add `/tmp` as a mount pair as well. This cannot be done in remote mode so `doctor` needs to be aware of this. Closes: #128204 Epic: None Release note: None Release justification: Build-only code changes

fmeum changed the title ~~Replace hermetic /tmp/ implementation with overlayfs~~ Replace hermetic /tmp implementation with overlayfs Apr 15, 2024

fmeum force-pushed the 21215-revert-hermetic-tmp-impl branch 2 times, most recently from e5dbc28 to aec09a2 Compare April 18, 2024 20:13

fmeum force-pushed the 21215-revert-hermetic-tmp-impl branch 8 times, most recently from 4945777 to 21fcad5 Compare May 8, 2024 16:12

fmeum changed the title ~~Replace hermetic /tmp implementation with overlayfs~~ Preserve paths under hermetic /tmp in the sandbox May 8, 2024

fmeum force-pushed the 21215-revert-hermetic-tmp-impl branch from 3a641ff to 9055f00 Compare May 8, 2024 17:02

fmeum requested review from lberki and oquenchil May 8, 2024 17:02

fmeum marked this pull request as ready for review May 8, 2024 17:03

github-actions bot added team-Performance Issues for Performance teams team-Local-Exec Issues and PRs for the Execution (Local) team awaiting-review PR is awaiting review from an assigned reviewer labels May 8, 2024

bazel-io mentioned this pull request May 8, 2024

[7.2.0] Preserve paths under hermetic /tmp in the sandbox #22291

Closed

fmeum commented May 8, 2024

View reviewed changes

fmeum mentioned this pull request May 8, 2024

Should --sandbox_debug imply --incompatible_sandbox_hermetic_tmp=false? #22290

Closed

fmeum added 5 commits May 14, 2024 15:33

Revert "Make tree artifacts that are symlinks to absolute paths work …

e772a12

…correctly." This reverts commit bf6ebe9.

Revert "Fix two issues with --incompatible_sandbox_hermetic_tmp tha…

5014297

…t manifested themselves when the output base was under `/tmp`:" This reverts commit fb6658c.

Revert "Rewrite paths of writable directories that are under the exec…

86e40c6

…root." This reverts commit bc1d9d3, but keeps the sorting of writable dirs.

Revert "Add support for bind mounts under /tmp with hermetic tmp"

19296b1

This reverts commit 5e68afd.

Revert "Add support for tmpfs mounts under /tmp with hermetic tmp"

d9f1cdd

This reverts commit 620d617.

fmeum added 4 commits May 14, 2024 15:35

Modify sandboxing test to move workspace into subdirectory

b6888d4

WIp

612da61

Simplify sandbox setup and fix integration test

9c0c738

Re-revert the switch to RootedPath

9ebdcc2

fmeum force-pushed the 21215-revert-hermetic-tmp-impl branch from 5204156 to 9ebdcc2 Compare May 14, 2024 13:35

oquenchil approved these changes May 15, 2024

View reviewed changes

oquenchil added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels May 15, 2024

copybara-service bot closed this in 3fddc7f May 16, 2024

github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label May 16, 2024

fmeum mentioned this pull request May 16, 2024

[7.2.0] Preserve paths under hermetic /tmp in the sandbox #22407

Merged

fmeum deleted the 21215-revert-hermetic-tmp-impl branch May 16, 2024 14:55

rickystewart mentioned this pull request Aug 2, 2024

bazel: --test_tmpdir's under /tmp are broken on Linux since Bazel 7 upgrade cockroachdb/cockroach#128204

Closed

rickystewart mentioned this pull request Aug 2, 2024

dev: in doctor, add --sandbox_add_mount_pair if relevant cockroachdb/cockroach#128207

Merged

rickystewart mentioned this pull request Aug 2, 2024

dev: refactor prompts for autofix permission cockroachdb/cockroach#128211

Merged

blathers-crl bot mentioned this pull request Aug 2, 2024

release-24.1: dev: in doctor, add --sandbox_add_mount_pair if relevant cockroachdb/cockroach#128223

Merged

blathers-crl bot mentioned this pull request Aug 2, 2024

release-24.2: dev: in doctor, add --sandbox_add_mount_pair if relevant cockroachdb/cockroach#128224

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preserve paths under hermetic `/tmp` in the sandbox#22001

Preserve paths under hermetic `/tmp` in the sandbox#22001
fmeum wants to merge 32 commits intobazelbuild:masterfrom
fmeum:21215-revert-hermetic-tmp-impl

fmeum commented Apr 15, 2024 •

edited

Loading

Uh oh!

fmeum commented May 8, 2024

Uh oh!

fmeum May 8, 2024

Uh oh!

oquenchil commented May 10, 2024

Uh oh!

fmeum commented May 12, 2024 via email •

edited

Loading

Uh oh!

fmeum commented May 14, 2024

Uh oh!

oquenchil left a comment

Uh oh!

tjgq commented May 16, 2024

Uh oh!

fmeum commented May 16, 2024 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

fmeum commented Apr 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmeum commented May 8, 2024

Uh oh!

fmeum May 8, 2024

Choose a reason for hiding this comment

Uh oh!

oquenchil commented May 10, 2024

Uh oh!

fmeum commented May 12, 2024 via email • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fmeum commented May 14, 2024

Uh oh!

oquenchil left a comment

Choose a reason for hiding this comment

Uh oh!

tjgq commented May 16, 2024

Uh oh!

fmeum commented May 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

fmeum commented Apr 15, 2024 •

edited

Loading

fmeum commented May 12, 2024 via email •

edited

Loading

fmeum commented May 16, 2024 •

edited

Loading