Data

{
  "owner": "mirendev",
  "repo": "runtime",
  "number": 1104,
  "verdict": "caveats",
  "event": "comment",
  "summary": "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`.\n\n**One concrete issue worth resolving before merge: the empty-`dnsServer` case produces a broken TOML entry.**\n\n`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:\n\n```toml\n[dns]\n  nameservers = [ \"\" ]\n```\n\nAn 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:\n\n```go\nif dnsServer != \"\" {\n    // emit [dns] block\n}\n```\n\nThis 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.\n\n**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.",
  "comments": [
    {
      "path": "components/buildkit/buildkit.go",
      "line": 326,
      "side": "RIGHT",
      "body": "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`.",
      "ai_prompt": "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)."
    }
  ],
  "posted_to_pr": true
}