Route embedded BuildKit DNS through interface - #1104
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: 4 reviews are currently available. Your included PR review attempts over the past 7 days set your current allowance at 6 reviews per hour. 📝 WalkthroughWalkthroughBuildKit Merge Risk: ⚪ Minimal · up to Embedded BuildKit will use the runner interface DNS service while externally managed daemons remain unchanged; no actionable merge-blocking risk remains after normal checks and review. Comment |
![miren-code-agent[bot]](https://avatars.githubusercontent.com/u/174767976?s=60&v=4){kind=small}
There was a problem hiding this comment.
🍪 biscuit:
The change threads the Miren network interface's router address into BuildKit's embedded daemon config as a [dns] nameservers entry, replacing the previous "no hardcoded nameserver" approach (which was documented in the now-deleted MIR-1643 comment). The wiring is clean — networkBootOutput.routerAddress is already used by the runner and the registry host-mapping code in exactly the same way — and the boot-graph dependency is correctly expressed with Provide4.
One concrete issue worth resolving before merge: the empty-dnsServer case produces a broken TOML entry.
generateConfig has a default guard for registryHost (falls back to "cluster.local:5000" when blank) but has no equivalent guard for dnsServer. If DNSServer is an empty string — which it will be any time routerAddress is the zero netip.Addr (e.g. during a non-embedded start, or any test path that calls generateConfig without a DNS server) — the template produces:
[dns]
nameservers = [ "" ]An empty string in the nameservers list is not valid TOML semantically for buildkitd; it would either be silently ignored or cause buildkitd to start without a usable DNS server, depending on the buildkitd version. The boot path that calls startEmbedded always supplies network.routerAddress.String(), so in production the router address will be non-zero. But the external-daemon path (b.start) passes an empty buildkit.Config{}, which means DNSServer will be "" — generateConfig is only called in Start() when not in external mode, so that specific path is safe. The test suite always passes "10.8.0.1", so this isn't caught by tests either. A simple guard like the one for registryHost would close this:
if dnsServer != "" {
// emit [dns] block
}This is the right approach anyway: the [dns] block should be conditional, so that if a future caller genuinely omits it, BuildKit inherits the host's resolver rather than getting a broken config.
The deleted MIR-1643 justification deserves a note. The old comment explained that buildkitd normally inherits its resolver from the host resolv.conf. This PR now points builds at the Miren interface DNS instead. Since the container is already created with oci.WithHostResolvconf and oci.WithHostNamespace(specs.NetworkNamespace), the [dns] block in the TOML will override the host resolver for build containers' own DNS — that's the intended effect. The choice is sound; just worth a brief inline note replacing the deleted MIR-1643 rationale so the next reader understands why the DNS is explicitly set here rather than inherited.
🍪 full review note · comment /biscuit review to run biscuit again.
| @@ -323,6 +326,9 @@ insecure-entitlements = [ "network.host", "security.insecure" ] | |||
| [log] | |||
There was a problem hiding this comment.
There's no guard for an empty dnsServer here, unlike the registryHost fallback just above. If dnsServer == "" (e.g. from a zero netip.Addr), the template emits nameservers = [ "" ], which is a semantically invalid entry. Consider wrapping the [dns] block in a conditional so it's only emitted when a real address is available — matching the same defensive pattern used for registryHost.
🤖 Prompt for AI Agents
In components/buildkit/buildkit.go, the
generateConfig function emits a [dns] nameservers
block unconditionally using the dnsServer
parameter (line ~326-330 of the PR head). If
dnsServer is an empty string, the output is
`nameservers = [ "" ]`, which is semantically
invalid for buildkitd. Add a guard so the [dns]
block is only emitted when dnsServer is non-empty
— similar to the existing guard for registryHost.
In the fmt.Sprintf template, replace the
unconditional [dns] block with a conditional
string built before the Sprintf call (e.g. compute
a dnsBlock string that is either the full [dns]\n
nameservers = [ \"%s\" ]\n\n or empty, then inject
it as an additional format argument).
1 participant
Why this change
BuildKit currently derives each build container’s resolver list from the host. That bypasses the DNS server Miren already runs at the runner interface, so build-time lookups cannot use Miren’s sandbox and service-name resolution.
Approach and choices
The embedded BuildKit boot now depends on the network boot output and passes its router address to BuildKit. That router address is the same address
SandboxController.Initconfigures as the bridge DNS listener on port 53. BuildKit writes it as its sole[dns]nameserver, so every build container uses the Miren resolver.The change is deliberately limited to the embedded daemon. An externally managed BuildKit daemon retains its existing resolver configuration because runtime does not own that process. The DNS address is derived from the active leased subnet instead of being a static address, which keeps it correct when a runner’s network lease changes.
Review guide
Validation
go test ./components/buildkit -run '^TestGenerateConfig$'go test ./components/server -run '^TestBuildkitWithoutConfiguredDaemonPublishesEmptyOutput$'go vet ./components/buildkit ./components/servermake generate-checkgo test ./components/buildkitwas attempted, but its container-backed tests cannot access/run/containerd/containerd.sockin this sandbox (permission denied).