Merge pull request moby#51615 from akerouanton/revert-51507

Revert "libnet: setupDNS: don't overwrite user-modified resolv.conf"

Author: thaJeztah

daemon/libnetwork/sandbox_dns_unix.go

@@ -264,17 +264,8 @@ func (sb *Sandbox) loadResolvConf(path string) (*resolvconf.ResolvConf, error) {
 // be a copy of the host's file, with overrides for nameservers, options and search
 // domains applied.
 func (sb *Sandbox) setupDNS() error {
-	sb.restoreResolvConfPath()
-
-	// When the container is restarted, a new Sandbox is created but the same resolv.conf is re-used. If it was
-	// user-modified, do not attempt to overwrite it.
-	if !sb.config.useDefaultSandBox {
-		if mod, err := resolvconf.UserModified(sb.config.resolvConfPath, sb.config.resolvConfHashFile); err != nil || mod {
-			return err
-		}
-	}
-
 	// Make sure the directory exists.
+	sb.restoreResolvConfPath()
 	dir, _ := filepath.Split(sb.config.resolvConfPath)
 	if err := createBasePath(dir); err != nil {
 		return err
@@ -338,7 +329,15 @@ func (sb *Sandbox) rebuildDNS() error {
 	// upstream nameservers.
 	sb.setExternalResolvers(extNameServers)
 
-	return rc.WriteFile(sb.config.resolvConfPath, sb.config.resolvConfHashFile, filePerm)
+	// Write the file for the container - preserving old behaviour, not updating the
+	// hash file (so, no further updates will be made).
+	// TODO(robmry) - I think that's probably accidental, I can't find a reason for it,
+	//  and the old resolvconf.Build() function wrote the file but not the hash, which
+	//  is surprising. But, before fixing it, a guard/flag needs to be added to
+	//  sb.updateDNS() to make sure that when an endpoint joins a sandbox that already
+	//  has an internal resolver, the container's resolv.conf is still (re)configured
+	//  for an internal resolver.
+	return rc.WriteFile(sb.config.resolvConfPath, "", filePerm)
 }
 
 func createBasePath(dir string) error {

daemon/libnetwork/sandbox_dns_unix_test.go

@@ -14,17 +14,12 @@ import (
 	is "gotest.tools/v3/assert/cmp"
 )
 
-func getResolvConf(t *testing.T, rcPath string) resolvconf.ResolvConf {
+func getResolvConfOptions(t *testing.T, rcPath string) []string {
 	t.Helper()
 	resolv, err := os.ReadFile(rcPath)
 	assert.NilError(t, err)
 	rc, err := resolvconf.Parse(bytes.NewBuffer(resolv), "")
 	assert.NilError(t, err)
-	return rc
-}
-
-func getResolvConfOptions(t *testing.T, rcPath string) []string {
-	rc := getResolvConf(t, rcPath)
 	return rc.Options()
 }
 
@@ -95,60 +90,3 @@ func TestDNSOptions(t *testing.T) {
 	dnsOptionsList = getResolvConfOptions(t, sb2.config.resolvConfPath)
 	assert.Check(t, is.DeepEqual([]string{"ndots:0"}, dnsOptionsList))
 }
-
-func TestNonHostNetDNSRestart(t *testing.T) {
-	c, err := New(context.Background(), config.OptionDataDir(t.TempDir()))
-	assert.NilError(t, err)
-
-	// Step 1: Create initial sandbox (simulating first container start)
-	sb, err := c.NewSandbox(context.Background(), "cnt1")
-	assert.NilError(t, err)
-
-	defer func() {
-		_ = sb.Delete(context.Background())
-	}()
-
-	sb.startResolver(false)
-
-	err = sb.setupDNS()
-	assert.NilError(t, err)
-	err = sb.rebuildDNS()
-	assert.NilError(t, err)
-
-	// Step 2: Simulate user manually overwriting the container's resolv.conf
-	resolvConfPath := sb.config.resolvConfPath
-	modifiedContent := []byte(`nameserver 1.1.1.1`)
-	err = os.WriteFile(resolvConfPath, modifiedContent, 0644)
-	assert.NilError(t, err)
-
-	// Step 3: Delete the sandbox (simulating container stop)
-	err = sb.Delete(context.Background())
-	assert.NilError(t, err)
-
-	// Step 4: Create a new sandbox (simulating container restart)
-	sbRestart, err := c.NewSandbox(context.Background(), "cnt1",
-		OptionResolvConfPath(resolvConfPath),
-	)
-	assert.NilError(t, err)
-	defer func() {
-		if err := sbRestart.Delete(context.Background()); err != nil {
-			t.Error(err)
-		}
-	}()
-
-	sbRestart.startResolver(false)
-
-	// Step 5: Call setupDNS on restart - should preserve user modifications
-	err = sbRestart.setupDNS()
-	assert.NilError(t, err)
-
-	rc := getResolvConf(t, sbRestart.config.resolvConfPath)
-	assert.Check(t, is.Equal("1.1.1.1", rc.NameServers()[0].String()))
-
-	// Step 6: Call rebuildDNS on restart - should preserve user modifications
-	err = sbRestart.rebuildDNS()
-	assert.NilError(t, err)
-
-	rc = getResolvConf(t, sbRestart.config.resolvConfPath)
-	assert.Check(t, is.Equal("1.1.1.1", rc.NameServers()[0].String()))
-}

daemon/libnetwork/sandbox_linux.go

@@ -366,11 +366,6 @@ func (sb *Sandbox) populateNetworkResourcesOS(ctx context.Context, ep *Endpoint)
 
 	if ep.needResolver() {
 		sb.startResolver(false)
-	} else {
-		// Make sure /etc/resolv.conf is set up.
-		if err := sb.updateDNS(ep.getNetwork().enableIPv6); err != nil {
-			return err
-		}
 	}
 
 	if i != nil && i.srcName != "" {
@@ -453,6 +448,10 @@ func (sb *Sandbox) populateNetworkResourcesOS(ctx context.Context, ep *Endpoint)
 	}
 
 	sb.addHostsEntries(ctx, ep.getEtcHostsAddrs())
+	// Make sure /etc/resolv.conf is set up.
+	if err := sb.updateDNS(ep.getNetwork().enableIPv6); err != nil {
+		return err
+	}
 
 	// Populate load balancer only after updating all the other
 	// information including gateway and other routes so that

integration/networking/resolvconf_test.go

@@ -212,41 +212,3 @@ func TestNslookupWindows(t *testing.T) {
 	// can only be changed in daemon.json using feature flag "windows-dns-proxy".
 	assert.Check(t, is.Contains(res.Stdout.String(), "Addresses:"))
 }
-
-// TestResolvConfPreservedOnRestart verifies that external modifications to
-// /etc/resolv.conf are preserved when a non-host network container is restarted.
-// Regression test for https://github.com/moby/moby/issues/51490
-func TestResolvConfPreservedOnRestart(t *testing.T) {
-	skip.If(t, testEnv.DaemonInfo.OSType == "windows", "No /etc/resolv.conf on Windows")
-
-	ctx := setupTest(t)
-
-	d := daemon.New(t, daemon.WithResolvConf(network.GenResolvConf("8.8.8.8")))
-	d.StartWithBusybox(ctx, t)
-	defer d.Stop(t)
-
-	c := d.NewClientT(t)
-	defer c.Close()
-
-	const ctrName = "test-resolvconf-preserved-on-restart"
-	id := container.Run(ctx, t, c, container.WithName(ctrName))
-	defer c.ContainerRemove(ctx, id, client.ContainerRemoveOptions{Force: true})
-
-	appendContent := `# hello`
-	res, err := container.Exec(ctx, c, ctrName, []string{
-		"sh", "-c",
-		"echo '" + appendContent + "' >> /etc/resolv.conf",
-	})
-	assert.NilError(t, err)
-	assert.Check(t, is.Equal(res.ExitCode, 0))
-
-	// Restart the container.
-	_, err = c.ContainerRestart(ctx, ctrName, client.ContainerRestartOptions{})
-	assert.Assert(t, is.Nil(err))
-
-	// Verify the modification was preserved
-	res, err = container.Exec(ctx, c, ctrName, []string{"tail", "-n", "1", "/etc/resolv.conf"})
-	assert.NilError(t, err)
-	assert.Check(t, is.Equal(res.ExitCode, 0))
-	assert.Check(t, is.Contains(res.Stdout(), appendContent))
-}