Repository navigation
fix(op,minimald): fetch a box build's web sources in its classifier leaf (0.7 backport) - #3101
norrietaylor wants to merge 1 commit into
Conversation
A web-source fetch for a build placed for a box opens each connection from a socket created in that box's classifier leaf, through a loopback relay, and asks its name query from the same leaf. The box's egress verdict then decides the fetch (NET-155). On a VM host the frames carry the box mark, so the host gate decides them by the box's rules and not by the node-plane baseline. The cache lookup and the content-hash check stay as they are. A fetch that the box's rules refuse fails the build with the source host. The node-plane fetch record no longer lists a web source fetched in a box's leaf. A gs:// source, and a web source of a box with no leaf, still leave from the daemon and are recorded. Backports [#2795](#2795) to the 0.7 line. The change is T124 from main with these names adapted, because the line predates two moves on main: - sandbox::config is sandbox2::config, in op and in minimald's tests. The line predates the crate rename in [#2106](#2106). - box_ops::{BuildPlacement, BuildTap} is session_sop::{BuildPlacement, BuildTap}, in the new test in minimald's session/tests.rs. main's box_actor/tests.rs is that file on the line. (cherry picked from commit c7f6ddd)
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the
 |
|
Reviewed: 2 findings, none blocking at Address the comments on the diff. Clobber: not checked in depth. If that was not intentional, restore the earlier change. |
| pub(crate) fn strip_urls(text: &str) -> String { | ||
| let mut out = String::with_capacity(text.len()); | ||
| let mut rest = text; | ||
| while let Some(at) = rest.find("://") { | ||
| let start = rest[..at] | ||
| .rfind(|c: char| !(c.is_ascii_alphanumeric() || matches!(c, '+' | '-' | '.'))) | ||
| .map_or(0, |i| i + 1); | ||
| let end = rest[at..] | ||
| .find(|c: char| c.is_whitespace() || matches!(c, '"' | '\'' | '<' | '>' | '(' | ')')) | ||
| .map_or(rest.len(), |i| at + i); | ||
| out.push_str(&rest[..start]); | ||
| if start == at { | ||
| // No scheme before the `://`: not a URL. | ||
| out.push_str(&rest[start..end]); | ||
| } else { | ||
| match stripped_url(&rest[start..end]) { | ||
| 8000 | Some(url) => out.push_str(&url), | |
| None => out.push_str("<url>"), |
There was a problem hiding this comment.
HIGH · correctness · confidence 0.50 · in strip_urls
strip_urls ends a URL at the first ( or ). A URL with parentheses in its path, such as https://h/a_(1).tar.gz?sig=abc, is cut at the (. The text after it, including the signed query, stays in the output. The output goes to the build stderr and to a daemon log line. Parse the URL up to whitespace or quote, and drop only a trailing ) that wraps it.
What the reviewer checked
specs.rs:383 .find(|c: char| c.is_whitespace() || matches!(c, '"' | ''' | '<' | '>' | '(' | ')'))
Hand this to an agent
Act on a code-review finding.
File: crates/op/src/specs.rs
Lines: 375-392
Category: correctness
Severity: high (reviewer confidence 0.50)
Finding:
`strip_urls` ends a URL at the first `(` or `)`. A URL with parentheses in its path, such as `https://h/a_(1).tar.gz?sig=abc`, is cut at the `(`. The text after it, including the signed query, stays in the output. The output goes to the build stderr and to a daemon log line. Parse the URL up to whitespace or quote, and drop only a trailing `)` that wraps it.
First verify this against the code yourself. It is model output and
may be wrong. If it is wrong, or the tradeoff was deliberate, say so
and change nothing.
👍 useful · 👎 noise — react to tell the reviewer. Replying works too, and says more.
| if !addrs.is_empty() { | ||
| Lookup::Found(addrs) | ||
| } else if answered { | ||
| Lookup::Answered(no_such_name.unwrap_or_else(|| { | ||
| io::Error::new(io::ErrorKind::NotFound, format!("{host} has no address")) | ||
| })) | ||
| } else { | ||
| Lookup::Unanswered(last.unwrap_or_else(|| { | ||
| io::Error::other(format!("the name server {server} did not answer")) |
There was a problem hiding this comment.
LOW · correctness · confidence 0.50 · in lookup_at
In lookup_at, a lost or failed A query is hidden when the AAAA query answers with no records. answered becomes true, so the result is Lookup::Answered and lookup_in_cgroup does not ask the next name server. The download then fails with "has no address" for a host that has an A record. Treat a lookup as answered only when no query failed, or fall through to the next server when a query failed.
What the reviewer checked
A query error sets
lastand continues. AAAA NoError setsanswered = true. With emptyaddrs,else if answeredreturns Lookup::Answered.
Hand this to an agent
Act on a code-review finding.
File: crates/common/src/remote_storage.rs
Lines: 632-640
Category: correctness
Severity: low (reviewer confidence 0.50)
Finding:
In `lookup_at`, a lost or failed A query is hidden when the AAAA query answers with no records. `answered` becomes true, so the result is `Lookup::Answered` and `lookup_in_cgroup` does not ask the next name server. The download then fails with "has no address" for a host that has an A record. Treat a lookup as answered only when no query failed, or fall through to the next server when a query failed.
First verify this against the code yourself. It is model output and
may be wrong. If it is wrong, or the tradeoff was deliberate, say so
and change nothing.
👍 useful · 👎 noise — react to tell the reviewer. Replying works too, and says more.
|
This agrees with the arch as written. Rule 11 of the owner ruling on gominimal/arch#167 (https://github.com/gominimal/arch/issues/167#issuecomment-6695894260) keeps #2795 and this backport as the temporary solution until build boxes are available. The two minimal-foundry findings ( |
Backports #2795 (
c7f6ddd67) torelease/0.7.x, as onecherry-pick -xcommit.Refs #3058. This fixes that issue for host-address boxes, the default network mode, but does not close it.
On
v0.7.3-rc1every web source leaves from the daemon, so on a VM host the node-plane baseline drops the frames and the first launch of a box with such a package waits about four minutes and then fails. With this change, a box with a classifier leaf fetches its web sources in that leaf, and the box's egress rules decide the fetch.Adaptation
The picked code did not compile on this line. Two names changed, and nothing else:
sandbox::configsandbox2::configcrate::box_ops::{BuildPlacement, BuildTap}crate::session_sop::{BuildPlacement, BuildTap}box_opsdoes not exist hereNo other commit is needed. #2720 and #2723 are already on the line.
Still open
own_ipbox has no classifier leaf, so its web sources still leave from the daemon and are dropped, even when the box allows the host. This is #2802.gs://sources still leave from the daemon, also #2802.Testing
Real microVM on macOS arm64, comparing a build of this branch with a build of
81f7ce2c. The branch was rebased after testing over161a18cf, which changes only.github/workflows/release.yml.81f7ce2cstorage.googleapis.comsourceclaude-codeat pkgs08b41ede--deny-all-egress--network own_ip --allow-dns-hosts storage.googleapis.com"Worked" means
execexited 0 and the log showed the fetch in the box's leaf. The extracted file was not confirmed in the box, andclaudewas not launched.just fmt-check,just check-version,just clippy,just deny,just test,just doctestandjust zoned-link-checkpass, at the macOS scope.just cistops atclippy-strict, which compares againstmain. Againstorigin/release/0.7.xit reports 4 hits, all in T124'sremote_storage.rs, the code asmainhas it. They are left as they are.just test-cross: clippy passes. Tests were run with--no-fail-fast, because the recipe stops at the first failing target. Failures incheckouts,common(shell_harnesses),minimal,minimaldandsandbox2also occur on81f7ce2c, and none is unique to this branch. All of T124's tests pass.just e2e,just test-vm, and the root lane.After the release,
git cherrylists this commit, and its fix is onmainas #2795.