tstest/natlab/vmtest: cover openresolv with a second snippet registered (#21003)

TestOpenresolvDNS pins the case where Tailscale owns the only resolvconf
snippet, so on its own it would also pass if tailscaled never read an OS
base config at all. Register a snippet the way a DHCP client would, then
toggle accept-dns to force a reapply, and assert its nameserver becomes
quad-100's upstream.

SetAcceptDNS is the toggle: tailscaled has no reason to re-read the OS
config on its own, and this avoids rebooting the guest to change what the
OS resolver looks like mid-test.

Updates #20825
Updates tailscale/corp#44793

Signed-off-by: Brendan Creane <bcreane@gmail.com>

tstest/natlab/vmtest/dns_openresolv_test.go

@@ -38,6 +38,12 @@
 	// The signature line openresolv writes at the top of resolv.conf.
 	orSignature = "# Generated by resolvconf"
 
+	// A name that no route covers and that only vnet's default DNS server
+	// answers. Resolving it proves the query reached the base config's
+	// nameservers instead of being answered by quad-100.
+	orUpstreamOnlyName = "dualstack-web.example.com"
+	orUpstreamOnlyIP   = "5.0.0.100"
+
 	// Tailscale's own resolver. tailscaled points resolv.conf at it once it has
 	// configured DNS.
 	orQuad100 = "100.100.100.100"
@@ -114,6 +120,65 @@ func TestOpenresolvDNS(t *testing.T) {
 	}
 }
 
+// TestOpenresolvDNSOtherSnippet is the counterweight to TestOpenresolvDNS.
+// When another snippet is present, its nameservers must still become quad-100's
+// default upstream. Without this test, the fix for #20825 could return early on
+// every host and still look correct.
+func TestOpenresolvDNSOtherSnippet(t *testing.T) {
+	env, node := newOpenresolvEnv(t)
+
+	// Register a second snippet, the way a DHCP client would. It points at
+	// vnet's default DNS server, the only thing that answers
+	// orUpstreamOnlyName.
+	cmd := fmt.Sprintf("printf 'nameserver %s\\n' | resolvconf -a eth0.inet", vnet.FakeDNSIPv4())
+	if out, err := env.SSHExec(node, cmd); err != nil {
+		t.Fatalf("%s: %v (%s)", cmd, err, strings.TrimSpace(out))
+	}
+
+	// tailscaled has no reason to re-read the OS config on its own, so force a
+	// full reapply. This is also how the reporter of #20825 reproduced the bug.
+	env.SetAcceptDNS(node, false)
+	env.SetAcceptDNS(node, true)
+
+	// Both snippets are registered.
+	if out, err := env.SSHExec(node, "resolvconf -i"); err != nil {
+		t.Errorf("resolvconf -i: %v (%s)", err, strings.TrimSpace(out))
+	} else {
+		// Match whole fields, so "tailscale" does not match a longer snippet
+		// name.
+		got := strings.Fields(out)
+		for _, want := range []string{"eth0.inet", "tailscale"} {
+			if !slices.Contains(got, want) {
+				t.Errorf("resolvconf -i = %q, want it to list %q", strings.TrimSpace(out), want)
+			}
+		}
+	}
+
+	// Tailscale still owns resolv.conf, so the OS asks quad-100 rather than the
+	// other snippet directly. Check that before the lookup at the end of the
+	// test. Otherwise a successful lookup might only mean libc went straight to
+	// the other snippet's nameserver during the toggle above.
+	assertOpenresolvResolvConf(t, env, node,
+		[]string{orSignature, orQuad100},
+		[]string{vnet.FakeDNSIPv4().String()})
+
+	assertNoDNSReadWarning(t, env, node)
+
+	// tailscaled must read back the other snippet's nameserver and nothing else.
+	// An empty config would mean the #20825 early return fired when it should
+	// not have, and our own snippet would mean the quad-100 filter did not
+	// (tailscale/tailscale#7816).
+	if base := openresolvBaseConfig(t, env, node); base != nil {
+		if want := vnet.FakeDNSIPv4().String(); !slices.Equal(base.Nameservers, []string{want}) {
+			t.Errorf("OS base config nameservers = %q, want just %s", base.Nameservers, want)
+		}
+	}
+
+	// An answer here can only come from quad-100 forwarding to that base config,
+	// since nothing else on the guest answers this name.
+	assertResolves(t, env, node, orUpstreamOnlyName, orUpstreamOnlyIP)
+}
+
 // assertOpenresolvResolvConf waits for the guest's /etc/resolv.conf to contain
 // every string in want and none in notWant.
 func assertOpenresolvResolvConf(t *testing.T, env *vmtest.Env, n *vmtest.Node, want, notWant []string) {

tstest/natlab/vmtest/vmtest.go

@@ -1127,6 +1127,27 @@ func (e *Env) SetAcceptRoutes(n *Node, on bool) {
 	e.t.Logf("[%s] accept-routes=%v", n.name, on)
 }
 
+// SetAcceptDNS toggles the node's CorpDNS preference (the --accept-dns flag),
+// controlling whether it applies the DNS configuration control sends it.
+//
+// Toggling it off and back on makes tailscaled tear down and reapply its whole
+// DNS configuration, including re-reading the OS's own config, without
+// rebooting the guest. A test can therefore change the OS resolver state
+// mid-run and be sure tailscaled re-reads it.
+func (e *Env) SetAcceptDNS(n *Node, on bool) {
+	e.t.Helper()
+	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+	defer cancel()
+
+	if _, err := n.agent.EditPrefs(ctx, &ipn.MaskedPrefs{
+		Prefs:      ipn.Prefs{CorpDNS: on},
+		CorpDNSSet: true,
+	}); err != nil {
+		e.t.Fatalf("SetAcceptDNS(%s, %v): %v", n.name, on, err)
+	}
+	e.t.Logf("[%s] accept-dns=%v", n.name, on)
+}
+
 // ApproveRoutes tells the test control server to approve subnet routes
 // for the given node. The routes should be CIDR strings.
 func (e *Env) ApproveRoutes(n *Node, routes ...string) {