Uh oh!
There was an error while loading. Please reload this page.
Add TOML configuration for default DNS nameservers, search, options - #1614
Add TOML configuration for default DNS nameservers, search, options#16140xMH wants to merge 3 commits into
Conversation
| } | ||
| final public class DNSConfig: Codable, Sendable { | ||
| public static let defaultNameservers: [String] = [] |
There was a problem hiding this comment.
Today there is a subtle difference between what the default DNS settings are for container aka the apiserver itself, which would determine the hostname resolution (aka the A record) for a given container on the host, and the DNS settings for setting up the resolv.conf in a container. I think we should maintain this distinction. There could be scenarios where a user does not want the default DNS domain on the host to necessarily match the default DNS domain that the container application uses in the container itself.
I think we should add a new field on the ContainerConfig type that has the default DNS settings for running a container.
Essentially we'd end up with something like this in the TOML:
[dns] <-------- default DNS settings for the APIServer
domain = "test"
[container.dns] <----------- default DNS settings for containers server = "8.8.8.8"
domain = "foo"
search = ["foo", "test"]
options = ["haha"]
There was a problem hiding this comment.
Thanks, good insight. I removed that extra builder startup call.
6af7693 to
b010238CompareCode Coverage
|
0xMH
commented
Jun 3, 2026
Thanks for the review, @katiewasnothere. Happy to iterate further if anything else needs addressing. |
katiewasnothere
commented
Jun 3, 2026
Hi @0xMH thank you for all the work you've done on this change! We are planning to make a new release of the container package some time in the next week and we want to hold off on merging this change until then. |
am-saksham
commented
Jul 5, 2026
Hi @0xMH and @katiewasnothere! First, thanks for the great work figuring out the core logic for this. I noticed this PR has been stalled since the June release and has picked up some merge conflicts. I also saw that a couple of newer PRs for #1449 were opened recently, but they seem to have missed the @0xMH, if you are currently swamped with other things, would you mind if I pull your branch, resolve the conflicts against Let me know if that sounds good! |
Thanks for the offer! but no need since I'm waiting for the green flag to continue and rebase and prepare for the merge. |
am-saksham
commented
Jul 5, 2026
Awesome, sounds good! Looking forward to seeing it merged. Let me know if you end up needing a hand later on! |
katiewasnothere
commented
Aug 4, 2026
@0xMH Really sorry for the delay here, we've been swamped. I'd love to get this in for this sprint (which ends at the end of August). Could you rebase your PR? I will take another pass through the code. Thank you for your work here! |
| - `192.168.*.*` | ||
| - `172.16.*.*` through `172.31.*.*` | ||
| - The host ends with the machine's default container DNS domain (as defined in `DNSConfig.defaultDomain`, located [here](../Sources/ContainerPersistence/ContainerSystemConfig.swift)) | ||
| - The host ends with the machine's configured internal DNS domain from `[dns].domain` |
There was a problem hiding this comment.
Could we add a link to the reference doc here instead?
| public static func dnsConfiguration( | ||
| from flags: Flags.DNS, | ||
| defaults: ContainerDNSConfig, | ||
| hostDomainFallback: String? = nil |
| public let nameservers: [String] | ||
| public let searchDomains: [String] | ||
| public let options: [String] |
There was a problem hiding this comment.
What do you think about making these optionals instead of empty arrays?
| public let cpus: Int | ||
| public let memory: MemorySize | ||
| public let dns: ContainerDNSConfig |
There was a problem hiding this comment.
What do you think about making this an optional as well?
Type of Change
Motivation and Context
Closes#1449. Adds default values for
--dns,--dns-search,--dns-option, and--dns-domainto the[dns]section of~/.config/container/config.toml, so users hitting macOSmDNSResponderconflicts can set the workaround once instead of repeating it on every invocation. Depends on the merged TOML configuration introduced by #1425.Defaults are read by
container run,container build, andcontainer builder startvia a sharedUtility.dnsConfiguration(from:defaults:)helper. CLI flags take precedence;--no-dnsstill disables DNS. Two pre-existing bugs in the build path were fixed to make the feature work end-to-end:BuildCommandwas forwarding onlydnsNameserversto the builder (now forwards all four DNS fields), andBuilderStart'sdnsChangedcheck only compared the first non-empty field (now compares all four).Testing