Fix: dns short name by KerneyJ · Pull Request #156 · NetSys/intermesh

KerneyJ · 2026-06-08T21:44:46Z

Addresses issue: IM-201. In short when the host's /etc/resolv.conf
has a search directive and ndots > 0, libc rewrites short mesh queries
(e.g. alice → alice.example.com) before they hit the loopback port-53
redirect, so the mesh resolver never sees the original short name and
resolution silently fails. Docker receives its /etc/resolv.conf file from the host.
This lead to test_short_name_dns in test/e2e.rs to fail. On Berkeley's
campus network /etc/resolv.conf unmodified contains:

search berkeley.edu
nameserver ...
nameserver ...
nameserver ...

The default value of ndots is 1, which leads to the above described
scenario and test_short_name_dns failing. for more details read IM-201.

This PR does a couple of things that I'll discuss in order of importance.

The second commit in this PR adds a set of failing test cases that isolate the problem.
These test cases assume a particular solution(found in third commit) that is up for
discussion. So any feedback on what these test cases ought to expect is appreciated.
Also feedback on scenarios that this PR does not address(e.g. VM test with Tailscale) or
tests that are not useful is welcomed.
The third commit contains a solution that is heavily based off of Tailscale's approach .
Tailscale takes ownership of /etc/resolv.conf, backs up the original, and monitors
the file for changes(overwrites changes when they appear). This PR takes ownership
of /etc/resolv.conf but does not overwrite changes(to keep it simple).
The first commit in this PR remove --ephemeral from launch_instance .
This caused the test_basic_vm_smoke to fail on my machine, that change
could be easily pulled out of this PR.

ejj · 2026-06-09T01:01:22Z

                &image,
                &instance,
                "--vm",
-                "--ephemeral",


https://linuxcontainers.org/incus/docs/main/reference/instance_properties/

so all ephemeral does is ensure that the vm is deleted when its stopped.

we do clean things up but its still a nice safety measure.

so i guess the question is, why does it break for you? whats the error? not necessarily opposed to removing it but i would want to understand more

When --ephemeral is marked the VM's are deleted before the test start.
The exact error is

thread 'test_basic_vm_smoke' (1091115) panicked at tests/vm.rs:20:42:
assertion failed: guest agent is not ready: im-4614fad7fd090555-1091114-vm1: Error: Failed to fetch instance "im-4614fad7fd090555-1091114-vm1" in project "default": Instance not found

what version of incus are you running?

ejj-agent

AI-generated review draft. This has not been reviewed by a human. Any comments made are non-binding; feel free to ignore them by resolving. This uses the same AI-review process Ethan uses to review his own code.

Thanks for digging into the libc search/ndots path; the direction makes sense, but I found a few issues worth fixing before this lands.

ejj-agent · 2026-06-09T13:48:01Z

+    section.next("Wait for gossip convergence");
+    let root_imid = root.imid().assert();
+    let db_imid = db.imid().assert();
+    let expected_names = HashMap::from([


AI-generated, human-unreviewed, non-binding: These new tests build their convergence expectations with HashMap/HashSet, but tests/e2e.rs imports BTreeMap/BTreeSet and wait_for_convergence takes &BTreeMap<Name, BTreeSet<Imid>> / &BTreeMap<Imid, BTreeSet<IpAddr>>. As written the integration test crate does not compile; switching these new maps/sets to BTreeMap::from and BTreeSet::from fixes the type and matches the rest of the file.

I just rebased onto main, compiled, built, and ran the test without changing to BTreeMaps/BTreeSets and the tests build and pass. Is the change to BTreeMap/BTreeSet on main yet?

ejj-agent · 2026-06-09T13:48:01Z

+        // Read upstream nameservers from /etc/resolv.conf *before*
+        // resolv_conf.install() overwrites the file. Otherwise we'd read back
+        // our own `nameserver 127.0.0.1` and forward queries to ourselves.
        let resolver = create_resolver();


AI-generated, human-unreviewed, non-binding: The normal-path ordering avoids reading the managed nameserver 127.0.0.1, but crash recovery still has that failure mode. If a prior run crashed after installing the managed file, /etc/resolv.conf still has the sentinel and loopback nameserver; on restart this create_resolver() runs before install() restores the snapshot, so the upstream resolver can be configured to forward back to the proxy itself. Consider splitting “recover prior managed file” out so it runs before resolver construction, then install the managed file after the real upstream config has been captured.

ejj-agent · 2026-06-09T13:48:01Z

+        // Defer order matters: scopeguard fires LIFO. Register the resolv.conf
+        // restore first so nftables_clean (registered second) fires first on
+        // shutdown — global system rules drop ASAP, local file restore after.
+        resolv_conf


AI-generated, human-unreviewed, non-binding: The restore guard is only registered after install() returns successfully. If install() snapshots the original and then fails while/after mutating /etc/resolv.conf (for example fs::write succeeds but the later chmod fails), Handle::run returns an error without restoring the global resolver file. Since this touches host-global state, install() should either roll back internally on post-snapshot failures or the guard should be active before the mutation begins.

ejj-agent · 2026-06-09T13:48:01Z

-        // Clean up any stale rules from a previous run.
+        // Clean up any stale state from a previous run.
        nftables_clean();
+        let resolv_conf = make_resolv_conf(&self.state)?;


AI-generated, human-unreviewed, non-binding: make_resolv_conf(&self.state)? now runs before the !self.state.intercept early-return path, so a non-intercepting proxy can fail due to resolv.conf snapshot path setup even though it will never manage /etc/resolv.conf. Moving this below the intercept check keeps the non-intercept mode behavior unchanged.

AI-generated -- a later change on this branch added resolv_conf.recover() immediately
after make_resolv_conf and before the intercept gate, so it's no longer dead weight
recover() restores a managed /etc/resolv.conf left by a crashed prior run and intentionally runs
cross-mode: if a previous intercept run crashed (leaving nameserver 127.0.0.1) and the operator
restarts without intercept, moving this below the gate would strand the host's DNS on a dead
127.0.0.1 until a future intercept run. The cited failure only occurs if state_file has no
parent (path /), which would break the daemon everywhere else anyway.

Removes ephemeral from the VM tests 1. Causes test_basic_vm_smoke to fail(at least on my machine) 2. VMHarness cleans up instances after the tests

Adds four #[ignore]'d tests pinning resolv.conf handling when libc's search-expansion would rewrite short mesh names before nftables can intercept them. Three docker e2e tests cover a pre-seeded `search` + `ndots:1` resolv.conf, the same via `docker run --dns-search`, and SIGTERM restoring the original. One incus vm test covers the systemd-resolved symlink layout, where the proxy must skip management and leave the symlink intact. All four fail against current main; the proxy fix lands in a follow-up commit. There are probably tests that I'm not thinking of(e.g. maybe a vm tests with Tailscale). Would love suggestions for more tests.

Snapshots the host's resolv.conf at startup and installs a managed file with ndots:0, so libc no longer rewrites short queries via the search directive before the mesh resolver sees them.

Two fixes from review of the short-name resolution change: - Extract `recover()` and run it before `create_resolver()` (and before the intercept gate, so a stale managed file is cleaned up even when restarting without intercept). Previously recovery happened inside `install()`, after the resolver had already read our managed `nameserver 127.0.0.1` and would forward queries back to the proxy. - Make `install()` atomic with respect to host-global state: if writing the managed file fails partway (e.g. content lands but chmod fails), roll back to the snapshot before returning, since the caller registers the restore guard only after `install()` returns Ok.

ejj-agent

AI-generated review draft. This has not been reviewed by a human. Any comments made are non-binding; feel free to ignore them by resolving. This uses the same AI-review process Ethan uses to review his own code.

The design is sound and well-executed. The sentinel + snapshot state machine is the right shape, and both orderings that make or break it are handled and documented: recover() runs before create_resolver() reads the file, and the defer! LIFO ordering drops nftables rules before restoring the file. The symlink skip-and-warn for systemd-resolved is the right call, and the VM test pinning it will keep a future "fix" from clobbering resolved.

Verified empirically on head 2fe175a8 (real docker + incus 7.0): clippy clean, all 7 new unit tests pass, all three new docker e2e tests pass, and test_systemd_resolved_active passes. This also resolves the open question on the earlier tests/e2e.rs review thread: the branch builds and the new tests pass as-is with the HashMap/HashSet imports.

On the PR description's open questions: the test expectations look right — managed-file content, snapshot capture, restoration, and end-to-end short-name resolution through the real libc path are the things worth pinning. The scenario missing from the matrix is crash-restart (see the inline comment on the unused restart() helper).

Remaining points not tied to a single line:

Suggestion: nothing guards the managed file mid-run, so NetworkManager/dhclient will clobber it on the first DHCP renew and short names silently break until restart. The PR description owns this tradeoff vs. Tailscale's watch-and-rewrite, and it's fine for Docker-first targets, but it deserves a tracking issue — on a typical laptop the fix lasts about one lease.
Question: should the new module get context/interfaces/src/proxy/resolv_conf.md per the mechanical src/ -> context/interfaces/ mapping? Precedent is inconsistent (intercept.md exists, dns.md doesn't), so flagging rather than asserting.
Suggestion: the two new search-domain tests add the third and fourth copies of the ~50-line init/join/convergence block; a small two-node bootstrap helper would pay for itself now.

Trajectory: the core design and orderings look settled; what's left is the rollback edge case in install(), a decision on the crash-recovery e2e vs. the unused helper, and landing the --ephemeral change with its root cause (already under discussion on the existing thread).

ejj-agent · 2026-06-12T02:22:19Z

+        if let Err(e) = write_resolv(&self.path, managed_content().as_bytes()) {
+            if let Err(rollback) = self.restore() {
+                error!("rollback after failed resolv.conf install failed: {rollback:#}");
+            }
+            return Err(e);
+        }


AI-generated, human-unreviewed, non-binding: Important — this rollback routes through restore(), whose sentinel gate has a hole for torn writes: if fs::write truncates the file and then fails (e.g. ENOSPC), the file no longer starts with the sentinel, so restore() takes the "no longer ours" branch — leaving the corrupted file in place and deleting the snapshot, the only copy of the original. Since install() owns this transition, consider having the rollback write the snapshot content back unconditionally instead of going through the sentinel check.

ejj-agent · 2026-06-12T02:22:19Z

+                "{} no longer has the intermesh sentinel; leaving it alone",
+                self.path.display()
+            );
+            let _ = fs::remove_file(&self.snapshot);


AI-generated, human-unreviewed, non-binding: Suggestion — keeping the snapshot here is free and preserves operator recourse: a lingering snapshot is inert because recover() only fires on the sentinel. Deleting it makes the pre-proxy original unrecoverable the moment an external writer claims the file.

ejj-agent · 2026-06-12T02:22:19Z

+    /// Restart the container. Docker re-runs the original entrypoint and
+    /// rebuilds `/etc/resolv.conf` / `/etc/hosts` / `/etc/hostname` fresh;
+    /// the writable layer (state files, snapshots) persists.
+    pub(crate) fn restart(&self) -> Result<()> {


AI-generated, human-unreviewed, non-binding: Important — restart() has no callers; it only compiles silently because tests/helpers/mod.rs has #![allow(dead_code)]. Was this intended for a crash-recovery e2e (kill -9 with the managed file installed, restart, assert the original is restored and upstream DNS still works)? That's the one scenario the recovery commit exists for that has no e2e coverage — either add that test or drop the helper.

ejj-agent · 2026-06-12T02:22:19Z

                &instance,
                "--vm",
-                "--ephemeral",
                "-c",


AI-generated, human-unreviewed, non-binding: one datapoint for this thread — the harness prefix embeds the PID (tests/helpers/vm.rs:126), so the pre-launch cleanup_instances can never match leftovers from a previous killed run; my test host has six leaked im-* VMs from two old PIDs. If --ephemeral stays out, consider dropping the PID from the prefix or glob-cleaning stale im-<worktree-hash>-* instances.

ejj-agent · 2026-06-12T02:22:19Z

+/// production hosts on a corp VPN or campus network.
+#[test]
+#[ignore = "e2e test requiring docker"]
+fn test_docker_dns_search_flag_is_handled() {


AI-generated, human-unreviewed, non-binding: do both search-domain tests earn their place? By the time install() runs, the printf-seeded file and the --dns-search-seeded file are indistinguishable to the daemon — same content, same bind mount. If the goal is pinning Docker's own header-writing behavior, the --dns-search variant alone seems to cover it.

ejj-agent · 2026-06-12T02:22:19Z

+#[cfg(test)]
+mod tests {
+    use super::*;
+    use std::os::unix::fs as unix_fs;


AI-generated, human-unreviewed, non-binding: Nit — repo import rules avoid as renaming; since symlink is the only thing used, use std::os::unix::fs::symlink; avoids the alias.

ejj-agent · 2026-06-12T02:22:19Z

 [[package]]
 name = "bitflags"
-version = "2.10.0"
+version = "2.12.1"


AI-generated, human-unreviewed, non-binding: Nit — the bitflags 2.10.0 → 2.12.1 bump looks like unrelated lockfile churn; reverting keeps the diff to one concern.

ejj reviewed Jun 9, 2026

View reviewed changes

ejj-agent reviewed Jun 9, 2026

View reviewed changes

KerneyJ added 4 commits June 11, 2026 12:47

Removed ephemeral

0fcd63d

Removes ephemeral from the VM tests 1. Causes test_basic_vm_smoke to fail(at least on my machine) 2. VMHarness cleans up instances after the tests

proxy: resolve short names by managing /etc/resolv.conf

013cc33

Snapshots the host's resolv.conf at startup and installs a managed file with ndots:0, so libc no longer rewrites short queries via the search directive before the mesh resolver sees them.

KerneyJ force-pushed the fix-short-name branch from 18432e8 to 2fe175a Compare June 11, 2026 20:37

ejj-agent reviewed Jun 12, 2026

View reviewed changes

Uh oh!

Conversation

KerneyJ commented Jun 8, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ejj-agent left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ejj-agent left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants