CLI: Initial support for DNS options#40138
CLI: Initial support for DNS options#40138AmelBawa-msft wants to merge 1 commit intofeature/wsl-for-appsfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR exposes Docker-style DNS configuration flags on wslc container run/create, plumbs the parsed values into container creation, and adds E2E coverage + a localized validation error for --no-dns conflicts.
Changes:
- Add
--dns,--dns-search,--dns-option,--dns-domain, and--no-dnstocontainer run/createargument sets and parsing. - Forward DNS settings into container creation via
ContainerOptions→WSLCContainerLauncher. - Add E2E tests for DNS flags and a localized error message for
--no-dnsflag conflicts.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| test/windows/wslc/e2e/WSLCE2EContainerRunTests.cpp | Adds E2E tests and help-text expectations for DNS flags on container run. |
| test/windows/wslc/e2e/WSLCE2EContainerCreateTests.cpp | Adds E2E tests and help-text expectations for DNS flags on container create. |
| src/windows/wslc/tasks/ContainerTasks.cpp | Parses new DNS-related CLI args into ContainerOptions. |
| src/windows/wslc/services/ContainerService.cpp | Forwards DNS-related ContainerOptions into WSLCContainerLauncher before container creation. |
| src/windows/wslc/services/ContainerModel.h | Extends ContainerOptions with DNS-related fields (servers/search/options/domain + NoDns). |
| src/windows/wslc/commands/ContainerRunCommand.cpp | Exposes DNS-related arguments for container run (including multi-use args). |
| src/windows/wslc/commands/ContainerCreateCommand.cpp | Exposes DNS-related arguments for container create (including multi-use args). |
| src/windows/wslc/arguments/ArgumentValidation.cpp | Adds validation preventing --no-dns from being used with DNS configuration flags. |
| src/windows/wslc/arguments/ArgumentDefinitions.h | Un-comments/adds definitions for DNS args and --no-dns. |
| localization/strings/en-US/Resources.resw | Adds localized --no-dns conflict error string. |
|
|
||
| if (context.Args.Contains(ArgType::NoDNS)) | ||
| { | ||
| options.NoDns = true; |
There was a problem hiding this comment.
--no-dns is parsed into ContainerOptions::NoDns here, but that flag is never consumed when creating/running the container (no references to options.NoDns elsewhere). As a result, --no-dns currently has no behavioral effect beyond conflict validation. Either plumb this through to the container create request (e.g., an explicit runtime/HostConfig setting) or remove the flag until it’s supported.
| options.NoDns = true; | |
| THROW_HR_MSG(E_NOTIMPL, "--no-dns is not currently supported."); |
| if (!options.DnsDomain.empty()) | ||
| { | ||
| auto domainname = options.DnsDomain; | ||
| containerLauncher.SetDomainname(std::move(domainname)); | ||
| } | ||
|
|
||
| if (!options.DnsOptions.empty()) | ||
| { | ||
| auto dnsOptions = options.DnsOptions; | ||
| containerLauncher.SetDnsOptions(std::move(dnsOptions)); | ||
| } | ||
|
|
||
| if (!options.DnsSearchDomains.empty()) | ||
| { | ||
| auto dnsSearchDomains = options.DnsSearchDomains; |
There was a problem hiding this comment.
--dns-domain is wired to WSLCContainerLauncher::SetDomainname() (Docker Domainname field), which sets the container domainname/hostname domain rather than resolv.conf DNS search/default-domain configuration. This appears inconsistent with the option name/description (“default DNS Domain”). Consider renaming the option to match Docker (--domainname) or changing the implementation to affect DNS resolution (e.g., via DnsSearchDomains/resolv.conf semantics) so behavior matches the CLI surface.
| if (!options.DnsDomain.empty()) | |
| { | |
| auto domainname = options.DnsDomain; | |
| containerLauncher.SetDomainname(std::move(domainname)); | |
| } | |
| if (!options.DnsOptions.empty()) | |
| { | |
| auto dnsOptions = options.DnsOptions; | |
| containerLauncher.SetDnsOptions(std::move(dnsOptions)); | |
| } | |
| if (!options.DnsSearchDomains.empty()) | |
| { | |
| auto dnsSearchDomains = options.DnsSearchDomains; | |
| if (!options.DnsOptions.empty()) | |
| { | |
| auto dnsOptions = options.DnsOptions; | |
| containerLauncher.SetDnsOptions(std::move(dnsOptions)); | |
| } | |
| auto dnsSearchDomains = options.DnsSearchDomains; | |
| if (!options.DnsDomain.empty()) | |
| { | |
| dnsSearchDomains.emplace_back(options.DnsDomain); | |
| } | |
| if (!dnsSearchDomains.empty()) | |
| { |
| TEST_METHOD(WSLCE2E_Container_Run_DNS_Search) | ||
| { | ||
| WSL2_TEST_ONLY(); | ||
|
|
||
| auto result = RunWslc(std::format( | ||
| L"container run --rm --dns-search example.com {} cat /etc/resolv.conf", DebianImage.NameAndTag())); | ||
| result.Verify({.ExitCode = 0}); | ||
| VERIFY_IS_TRUE(result.Stdout->find(L"search example.com") != std::wstring::npos); | ||
| } | ||
|
|
||
| TEST_METHOD(WSLCE2E_Container_Run_DNS_Option) | ||
| { | ||
| WSL2_TEST_ONLY(); | ||
|
|
||
| auto result = RunWslc(std::format( | ||
| L"container run --rm --dns-option ndots:5 {} cat /etc/resolv.conf", DebianImage.NameAndTag())); | ||
| result.Verify({.ExitCode = 0}); | ||
| VERIFY_IS_TRUE(result.Stdout->find(L"options ndots:5") != std::wstring::npos); | ||
| } | ||
|
|
||
| TEST_METHOD(WSLCE2E_Container_Run_DNS_AllOptions) | ||
| { | ||
| WSL2_TEST_ONLY(); | ||
|
|
||
| auto result = RunWslc(std::format( | ||
| L"container run --rm --dns 1.1.1.1 --dns-search test.local --dns-option ndots:3 {} cat /etc/resolv.conf", | ||
| DebianImage.NameAndTag())); | ||
| result.Verify({.ExitCode = 0}); | ||
| VERIFY_IS_TRUE(result.Stdout->find(L"nameserver 1.1.1.1") != std::wstring::npos); | ||
| VERIFY_IS_TRUE(result.Stdout->find(L"search test.local") != std::wstring::npos); | ||
| VERIFY_IS_TRUE(result.Stdout->find(L"options ndots:3") != std::wstring::npos); | ||
| } |
There was a problem hiding this comment.
The new DNS test coverage exercises --dns, --dns-search, and --dns-option, but there’s no E2E test validating the behavior of the newly exposed --dns-domain option (only its presence in help/conflict text). Adding a test that asserts the expected in-container effect would prevent regressions and clarify intended semantics.
| TEST_METHOD(WSLCE2E_Container_Create_NoDNS_ConflictWithDNS_Fails) | ||
| { | ||
| WSL2_TEST_ONLY(); | ||
|
|
||
| auto result = RunWslc(std::format( | ||
| L"container create --name {} --no-dns --dns 8.8.8.8 {} echo test", | ||
| WslcContainerName, DebianImage.NameAndTag())); | ||
| result.Verify({.Stderr = L"Cannot use --no-dns with --dns, --dns-domain, --dns-option, or --dns-search\r\n", .ExitCode = 1}); | ||
| } |
There was a problem hiding this comment.
The new tests validate the --no-dns conflict error, but there’s no test that --no-dns alone actually changes container behavior (and the flag currently isn’t applied during container creation). Once --no-dns is implemented, add an E2E assertion for the expected resolv.conf/DNS behavior when it’s set.
b8b740b to
d26d2d3
Compare
Summary of the Pull Request
PR Checklist
Detailed Description of the Pull Request / Additional comments
Validation Steps Performed