-
Notifications
You must be signed in to change notification settings - Fork 723
fix: allow localhost DNS servers when using host network #4653
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
df19f68 to
71836a8
Compare
| } | ||
|
|
||
| func TestFilterResolvDnsWithLocalhostOption(t *testing.T) { | ||
| // Test 1: allowLocalhostDNS=false should strip localhost (original behavior) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should be subtests? (t.Run)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we have some integration tests too?
nerdctl/cmd/nerdctl/container/container_run_network_linux_test.go
Lines 935 to 978 in 20a3eeb
| func TestHostNetworkHostName(t *testing.T) { | |
| nerdtest.Setup() | |
| testCase := &test.Case{ | |
| Require: require.Not(require.Windows), | |
| Setup: func(data test.Data, helpers test.Helpers) { | |
| helpers.Custom("cat", "/etc/hostname").Run(&test.Expected{ | |
| Output: func(stdout string, t tig.T) { | |
| data.Labels().Set("hostHostname", stdout) | |
| }, | |
| }) | |
| }, | |
| Command: func(data test.Data, helpers test.Helpers) test.TestableCommand { | |
| return helpers.Command("run", "--rm", | |
| "--network", "host", | |
| testutil.AlpineImage, "cat", "/etc/hostname") | |
| }, | |
| Expected: func(data test.Data, helpers test.Helpers) *test.Expected { | |
| return &test.Expected{ | |
| Output: expect.Equals(data.Labels().Get("hostHostname")), | |
| } | |
| }, | |
| } | |
| testCase.Run(t) | |
| } | |
| func TestNoneNetworkDnsConfigs(t *testing.T) { | |
| nerdtest.Setup() | |
| testCase := &test.Case{ | |
| Require: require.Not(require.Windows), | |
| Command: func(data test.Data, helpers test.Helpers) test.TestableCommand { | |
| return helpers.Command("run", "--rm", | |
| "--network", "none", | |
| "--dns", "0.1.2.3", "--dns-search", "example.com", "--dns-option", "timeout:3", "--dns-option", "attempts:5", | |
| testutil.CommonImage, "cat", "/etc/resolv.conf") | |
| }, | |
| Expected: test.Expects(0, nil, expect.Contains( | |
| "0.1.2.3", | |
| "example.com", | |
| "attempts:5", | |
| "timeout:3", | |
| )), | |
| } | |
| testCase.Run(t) | |
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have implemented some simple integration tests. I don't think we should modify host /etc/resolv.conf content on-the-fly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm investigating CI failure in rootless setup.
5e3e478 to
9c02946
Compare
This commit addresses the issue where nerdctl was unconditionally stripping localhost DNS servers from /etc/resolv.conf when container is using host network. Fixes: containerd#4651 Signed-off-by: Youfu Zhang <[email protected]>
(This change is vibed by Claude Haiku 4.5, reviewed by human.)
This commit addresses the issue where nerdctl was unconditionally stripping localhost DNS servers from /etc/resolv.conf when containers used --network=host.
Changes made:
resolvconf.FilterResolvDNSWithLocalhostOption(), likeresolvconf.FilterResolvDNS()but allows controlling whether localhost nameservers are preserved.The fix ensures:
Fixes: #4651