Skip to content

Comments

libnet: setupDNS: don't overwrite user-modified resolv.conf#51507

Merged
robmry merged 3 commits intomoby:masterfrom
zhangguanzhang:fix-pause-restart
Nov 25, 2025
Merged

libnet: setupDNS: don't overwrite user-modified resolv.conf#51507
robmry merged 3 commits intomoby:masterfrom
zhangguanzhang:fix-pause-restart

Conversation

@zhangguanzhang
Copy link
Contributor

@zhangguanzhang zhangguanzhang commented Nov 13, 2025 •
edited by robmry
Loading

- What I did

When restarting containers with non-host networks, sandbox.setupDNS() now calls resolvconf.UserModified() to check if resolv.conf was manually modified, preventing unintended overwrites.

- Human readable description for the release notes

Do not overwrite user-modified `/etc/resolv.conf` across container restarts.

- A picture of a cute animal (not mandatory but encouraged)
7318e7f0b7850db8a698a4159692717f_720

@github-actions github-actions bot added the area/daemon Core Engine label Nov 13, 2025
@zhangguanzhang zhangguanzhang changed the title daemon: sandbox.setupDNS()should call resolvconf.UserModified for checks. daemon: sandbox.setupDNS() should call resolvconf.UserModified for checks. Nov 13, 2025
@zhangguanzhang zhangguanzhang force-pushed the fix-pause-restart branch 5 times, most recently from a5dc7fc to ce131f7 Compare November 13, 2025 06:50
@zhangguanzhang zhangguanzhang changed the title daemon: sandbox.setupDNS() should call resolvconf.UserModified for checks. daemon: sandbox.setupDNS() should call resolvconf.UserModified for restart /pause Nov 13, 2025
@zhangguanzhang zhangguanzhang changed the title daemon: sandbox.setupDNS() should call resolvconf.UserModified for restart /pause daemon: sandbox.setupDNS() should check user-modified resolv.conf on container restart for non-host networks Nov 14, 2025
@github-actions github-actions bot added the area/networking Networking label Nov 14, 2025
@zhangguanzhang
Copy link
Contributor Author

zhangguanzhang commented Nov 16, 2025

Can someone help me take a look?

akerouanton
akerouanton reviewed Nov 24, 2025
View reviewed changes
@akerouanton akerouanton added status/2-code-review kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. impact/changelog area/networking/dns Networking and removed area/daemon Core Engine labels Nov 24, 2025
@github-actions github-actions bot added the area/daemon Core Engine label Nov 24, 2025
@zhangguanzhang
Copy link
Contributor Author

zhangguanzhang commented Nov 24, 2025

@akerouanton @thaJeztah
test failed:

=== FAIL: daemon/libnetwork TestDNSOptions (0.03s)
    sandbox_dns_unix_test.go:60: assertion failed: ndots:5 (string) != ndots:0 (dnsOptionsList[0] string)
    sandbox_dns_unix_test.go:66: assertion failed: ndots:5 (string) != ndots:0 (dnsOptionsList[0] string)

moby/daemon/libnetwork/sandbox_dns_unix_test.go

Lines 31 to 60 in 8cd48c7

func TestDNSOptions(t *testing.T) {
c, err := New(context.Background(), config.OptionDataDir(t.TempDir()))
assert.NilError(t, err)
sb, err := c.NewSandbox(context.Background(), "cnt1", nil)
assert.NilError(t, err)
cleanup := func(s *Sandbox) {
if err := s.Delete(context.Background()); err != nil {
t.Error(err)
}
}
defer cleanup(sb)
sb.startResolver(false)
err = sb.setupDNS()
assert.NilError(t, err)
err = sb.rebuildDNS()
assert.NilError(t, err)
dnsOptionsList := getResolvConfOptions(t, sb.config.resolvConfPath)
assert.Check(t, is.Len(dnsOptionsList, 1))
assert.Check(t, is.Equal("ndots:0", dnsOptionsList[0]))
sb.config.dnsOptionsList = []string{"ndots:5"}
err = sb.setupDNS()
assert.NilError(t, err)
dnsOptionsList = getResolvConfOptions(t, sb.config.resolvConfPath)
assert.Check(t, is.Len(dnsOptionsList, 1))
assert.Check(t, is.Equal("ndots:5", dnsOptionsList[0]))

sb.startResolver(false) will call sb.rebuildDNS()

moby/daemon/libnetwork/sandbox_dns_unix.go

Line 77 in 8cd48c7

err = sb.rebuildDNS()

It seems we still need some kind of flag to determine whether sandbox was created after a restart. 🤔

@akerouanton
Copy link
Member

akerouanton commented Nov 24, 2025

Looks like you're hitting this:

moby/daemon/libnetwork/sandbox_dns_unix.go

Lines 332 to 340 in 6e52828

// 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)

Since the 2nd argument is an empty string, rc.WriteFile() won't update the hash file and subsequent calls to sb.setupDNS() won't update the resolv.conf (due to the change I requested).

I'll discuss that TODO with @robmry to see if we can fix it. This would avoid the need to add a sandbox option to tell libnetwork whether it should consider the hash file.

@zhangguanzhang
Copy link
Contributor Author

zhangguanzhang commented Nov 24, 2025

Looks like you're hitting this:

moby/daemon/libnetwork/sandbox_dns_unix.go

Lines 332 to 340 in 6e52828

// 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)

Since the 2nd argument is an empty string, rc.WriteFile() won't update the hash file and subsequent calls to sb.setupDNS() won't update the resolv.conf (due to the change I requested).

I'll discuss that TODO with @robmry to see if we can fix it. This would avoid the need to add a sandbox option to tell libnetwork whether it should consider the hash file.

Thank you for the detailed feedback.
If there are any additional concerns or edge cases you’d like me to look into, please feel free to point them out. I’ll update the PR to reflect your suggestions.

@akerouanton
libnet: populateNetworkResourcesOS: updateDNS only if !needResolver
937246a 
When ep.needResolver() is true, sb.startResolver() calls sb.rebuildDNS()
which doesn't update the resolv.conf hash file.

Subsequent calls to sb.updateDNS() (which is only called by
populateNetworkResourcesOS) won't have any effect since it'll compare
the hash file and consider that the file was manually modified.

Make this explicit by gating the call to updateDNS() on !needResolver().

Signed-off-by: Albin Kerouanton <[email protected]>
@akerouanton
Copy link
Member

akerouanton commented Nov 25, 2025

@zhangguanzhang I went ahead and force-pushed your branch. It contains two new commits that address the TODO in sb.rebuildDNS(). I also touched your last commit to make the comment in sb.setupDNS() more useful and improved the commit message.

CI should be green this time.

@akerouanton akerouanton changed the title daemon: sandbox.setupDNS() should check user-modified resolv.conf on container restart for non-host networks libnet: setupDNS: don't overwrite user-modified resolv.conf Nov 25, 2025
akerouanton
akerouanton approved these changes Nov 25, 2025
View reviewed changes
Copy link
Member

@akerouanton akerouanton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Last commit LGTM.

@akerouanton akerouanton added this to the 29.1.0 milestone Nov 25, 2025
robmry
robmry reviewed Nov 25, 2025
View reviewed changes
Copy link
Contributor

@robmry robmry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you - just a couple of comments on the tests ...

@zhangguanzhang zhangguanzhang force-pushed the fix-pause-restart branch 3 times, most recently from 1218915 to 00246ea Compare November 25, 2025 12:29
@zhangguanzhang
libnet: setupDNS: don't overwrite user-modified resolv.conf
7639e19 
Call resolvconf.UserModified() in sandbox.setupDNS() to check if
resolv.conf was manually modified before regenerating it during
container restart for non-host network modes.

Signed-off-by: zhangguanzhang <[email protected]>
Signed-off-by: Albin Kerouanton <[email protected]>
@robmry robmry merged commit 56dda25 into moby:master Nov 25, 2025
182 checks passed
@robmry
Copy link
Contributor

robmry commented Nov 25, 2025

Merged - thank you @zhangguanzhang.

@zhangguanzhang zhangguanzhang deleted the fix-pause-restart branch November 25, 2025 15:12
This was referenced Nov 28, 2025
Closed
Closed
@akerouanton akerouanton mentioned this pull request Nov 28, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@akerouanton akerouanton akerouanton approved these changes

@robmry robmry robmry approved these changes

@thaJeztah thaJeztah Awaiting requested review from thaJeztah

Assignees

No one assigned

Labels

area/daemon Core Engine area/networking/dns Networking area/networking Networking impact/changelog kind/enhancement Enhancements are not bugs or new features but can improve usability or performance. status/2-code-review

Projects

None yet

Milestone

29.1.0

Development

Successfully merging this pull request may close these issues.

non-host container restart will change ths resolv

3 participants

@zhangguanzhang @akerouanton @robmry