Conversation
There was a problem hiding this comment.
Pull request overview
Refactors the integration test suite to keep Rust’s default test harness while still running each test body inside a bubblewrap-based sandbox. The sandboxing is reworked into a small module + component system (with DNS as the first component), and the SRV lookup tests are updated to run generically over any SrvResolver implementation.
Changes:
- Replace the custom integration-test runner/harness with per-
#[test]sandbox execution viaSandbox::run(...)/run_with_tokio(...). - Introduce
tests/sandboxwith a component trait and a mock DNS component built onhickory-proto. - Update dev dependencies (notably Tokio) and adjust benches accordingly.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| Cargo.toml | Updates dev-deps (Tokio 1.x) and removes deps only needed by the deleted custom harness. |
| benches/client.rs | Minor cleanup to align with Tokio runtime usage (no longer mutable). |
| benches/resolver.rs | Minor cleanup to align with Tokio runtime usage (no longer mutable). |
| tests/run_integration_tests.rs | Removes the custom harness = false test runner binary. |
| tests/harness/mod.rs | Removes the bespoke harness implementation (parallel runner + sandboxing). |
| tests/tests/mod.rs | Removes old “test registry” module used by the custom harness. |
| tests/tests/test_trivial_with_default_config.rs | Deletes old harness-driven test definition. |
| tests/tests/test_simple_lookup_srv_single.rs | Deletes old harness-driven SRV lookup test definition. |
| tests/tests/test_simple_lookup_srv_multiple.rs | Deletes old harness-driven SRV lookup test definition. |
| tests/test_trivial_with_default_config.rs | Adds a standard #[test] that runs its body inside the sandbox. |
| tests/test_simple_lookup_srv_single.rs | Adds a standard #[test] SRV lookup test using Sandbox + MockDns. |
| tests/test_simple_lookup_srv_multiple.rs | Adds a standard #[test] SRV lookup test using Sandbox + MockDns. |
| tests/sandbox/mod.rs | Adds the core Sandbox runner that respawns the test binary inside bubblewrap. |
| tests/sandbox/components.rs | Adds the SandboxComponent trait and wiring for the DNS component requirements/startup. |
| tests/sandbox/components/dns.rs | Refactors mock DNS server into MockDns and moves MockSrv alongside it. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Build and execute a bubblewrap command | ||
| let output = Command::new("bwrap") | ||
| .args(["--unshare-all"]) // Unshare every namespace | ||
| .args(["--dev-bind", "/", "/"]) // TODO: this can be narrowed |
There was a problem hiding this comment.
bwrap is currently invoked with --dev-bind / /, which makes the host filesystem writable inside the sandbox (subject to the calling user’s permissions). This undermines the isolation goals and can allow integration tests to mutate the developer/CI environment (e.g., repo checkout, $HOME, /tmp). Consider switching to a read-only base (e.g., --ro-bind / / or a narrower set of --ro-bind mounts) and explicitly adding writable tmpfs/bind mounts only where needed (like a private /tmp).
| .args(["--dev-bind", "/", "/"]) // TODO: this can be narrowed | |
| .args(["--ro-bind", "/", "/"]) // Use a read-only view of the host filesystem | |
| .args(["--tmpfs", "/tmp"]) // Provide a private writable /tmp inside the sandbox |
There was a problem hiding this comment.
If I am not mistaken the main goal of the sandbox is to ensure a true DNS server can be used for tests. The Glibc resolver used by srv-rs hardcodes port 53 and the location of configuration files in /etc. We use bwrap to allow non-root users to use port 53 and temporary files in /etc during tests. Ensuring the tests have a reduced access to some of the host paths is not really a goal here. Tests traditionally run with the permissions of the developer running them. Restricting access is a worthy goal, but it is not clear that it should be pursued in the sandbox created in this repository for integration testing.
| tokio = { version = "0.2", features = ["rt-threaded", "macros"] } | ||
| tokio = { version = "1.49.0", features = ["rt-multi-thread", "macros"] } | ||
| tempfile = "3.24.0" | ||
| owo-colors = "4.2.3" |
There was a problem hiding this comment.
owo-colors and rayon were added as dependencies as part of the harness introduced in #22 that is being removed in this PR.
| // Build and execute a bubblewrap command | ||
| let output = Command::new("bwrap") | ||
| .args(["--unshare-all"]) // Unshare every namespace | ||
| .args(["--dev-bind", "/", "/"]) // TODO: this can be narrowed |
There was a problem hiding this comment.
If I am not mistaken the main goal of the sandbox is to ensure a true DNS server can be used for tests. The Glibc resolver used by srv-rs hardcodes port 53 and the location of configuration files in /etc. We use bwrap to allow non-root users to use port 53 and temporary files in /etc during tests. Ensuring the tests have a reduced access to some of the host paths is not really a goal here. Tests traditionally run with the permissions of the developer running them. Restricting access is a worthy goal, but it is not clear that it should be pursued in the sandbox created in this repository for integration testing.
| tokio = { version = "0.2", features = ["rt-threaded", "macros"] } | ||
| tokio = { version = "1.49.0", features = ["rt-multi-thread", "macros"] } | ||
| tempfile = "3.24.0" | ||
| owo-colors = "4.2.3" |
There was a problem hiding this comment.
owo-colors and rayon were introduced in #22 for the harness that is being replaced in this PR. It makes sense to remove these dependencies.
| fn start(&self) -> Box<dyn std::any::Any>; | ||
| } | ||
|
|
||
| impl SandboxComponent for MockDns { |
There was a problem hiding this comment.
I would have expected this to live in the components/dns.rs file. This would ensure that adding or modifying a component is self contained and does not need to touch a "central registry" file.
The integration tests introduced in #22 function correctly, but require sacrificing Rust's default test harness and simulating the output.
This PR introduces a way to achieve the same sandboxing without sacrificing the test harness. It also implements the sandboxing with more thoughtful modules and a cleaner builder interface.
It also accidentally/naturally satisfies part 2 of #29 (Replace Trust-DNS resolver with Hickory):
The diff here is somewhat large, but it's essentially a no-op refactor: the tests themselves have not substantially changed.
Least importantly, but still worth mentioning: I also took this opportunity to upgrade our ancient version of tokio to the latest version. This required two minor code changes in the benchmarking code and nowhere else.