Still ready to merge. The only change since my last review is in the two docs callouts, which were rewritten after phinze's comments. The code is the same as what I approved.
distributed-runners.md. The upgrade note now matches how upgrades actually run. It says managed upgrades do the coordinator first and then roll runners one at a time, and that each runner starts pulling again once its own upgrade restarts it. That fits the code: the runner reads coordinator_internal_ip at startup, so the pull gap on each runner ends when it restarts, and nobody has to step in. For manual upgrades, the runners-first order still holds up, because a new runner talking to an old coordinator uses the API-address fallback in prepareNetworkDeps. This fixes the problem I missed when I first signed off on the runners-first wording.
firewall.md. The callout now says 5000/tcp never needs to be opened publicly, and warns anyone still on an older release, which listens on all interfaces, to block it themselves. That's the change in emphasis phinze asked for.
phinze suggested naming the actual versions ("when upgrading from vX to vY"). Both callouts still say "the release that routes registry pulls over WireGuard". I assume the release number isn't settled yet. If it is, putting it in would help anyone scanning the docs to see whether this applies to them. That's phinze's thread to settle, and I won't hold the merge for it.
My earlier non-blocking caveat still applies: runners read the coordinator's bridge gateway only at startup, so if the coordinator comes back on a new flannel subnet, they keep pulling from the old address until they restart.
Verdict: ready
{
"owner": "mirendev",
"repo": "runtime",
"number": 1264,
"verdict": "ready",
"event": "approve",
"summary": "Still ready to merge. The only change since my last review is in the two docs callouts, which were rewritten after phinze's comments. The code is the same as what I approved.\n\n**distributed-runners.md.** The upgrade note now matches how upgrades actually run. It says managed upgrades do the coordinator first and then roll runners one at a time, and that each runner starts pulling again once its own upgrade restarts it. That fits the code: the runner reads `coordinator_internal_ip` at startup, so the pull gap on each runner ends when it restarts, and nobody has to step in. For manual upgrades, the runners-first order still holds up, because a new runner talking to an old coordinator uses the API-address fallback in `prepareNetworkDeps`. This fixes the problem I missed when I first signed off on the runners-first wording.\n\n**firewall.md.** The callout now says 5000/tcp never needs to be opened publicly, and warns anyone still on an older release, which listens on all interfaces, to block it themselves. That's the change in emphasis phinze asked for.\n\nphinze suggested naming the actual versions (\"when upgrading from vX to vY\"). Both callouts still say \"the release that routes registry pulls over WireGuard\". I assume the release number isn't settled yet. If it is, putting it in would help anyone scanning the docs to see whether this applies to them. That's phinze's thread to settle, and I won't hold the merge for it.\n\nMy earlier non-blocking caveat still applies: runners read the coordinator's bridge gateway only at startup, so if the coordinator comes back on a new flannel subnet, they keep pulling from the old address until they restart.",
"posted_to_pr": true,
"head_sha": "253fe64ff1e80480e613e4f19c5a4b100b7ac7a7"
}Both of my blocking concerns are fixed in the code, and I'm comfortable with this merging.
Startup coupling. ClusterAccess.Start now returns the error from setupRemoteCoordinatorInfo instead of logging a Warn. So when WorkloadIssuerInfo fails three times, the runner stops with "querying workload issuer info after 3 attempts: …", which is the real cause, and the deferred rs.Close() still cleans up on that path. An invalid IP now fails startup instead of quietly turning identity off. The r.deps.WorkloadIssuer != nil early return moved below the IP parse, so the address is always read.
Mixed versions. When coordinator_internal_ip is absent, the runner falls back to the API address and logs a Warn. That fixes the new-runner/old-coordinator case, and TestLegacyCoordinatorRegistryUsesAPIAddress covers it. The distributed-runners doc now gives an upgrade order that works: runners first, then the coordinator, then restart the runners. It also says plainly that pulls fail on runners that haven't restarted. That settles my earlier point.
Tests. TestAllowRegistryFromWireGuardRules now checks the part that actually protects the registry: each of the lo/rt0/flannel-wg ACCEPT rules comes before the DROP, and the DROP comes before the older -i rt0 accept. It needs root, so it only runs in a privileged environment. I'd still like a blackbox check that a runner can pull over the overlay, but I won't hold the merge for it.
One thing to be aware of, not a blocker. Runners read the coordinator's bridge gateway only once, at startup. That gateway is subnet.Router() from the coordinator's flannel lease (boot_network.go). Leases do rotate after a long enough outage; the comment on ReconcileBridgeAddresses in network/bridge.go describes that happening. If the coordinator comes back on a new subnet, runners keep pulling from the old gateway until they restart, and nothing logs why. The RPC doc already calls the value "current", so if that ever happens in practice, a periodic re-query or a clear pull-failure log would be a good follow-up.
Posted to the PR's comment threads when you submit.
setupRemoteCoordinatorInfo now returns its error, and ClusterAccess.Start stops with setting up coordinator registry and workload identity: …, so the RPC failure is the reason the boot stops. An invalid IP also fails startup instead of quietly turning identity off. Thanks for fixing it.coordinator_internal_ip, the runner falls back to the API address and logs a Warn, and TestLegacyCoordinatorRegistryUsesAPIAddress tests that. The old-runner/new-coordinator case now has a documented upgrade order (runners first, then the coordinator, then restart the runners), with a warning about the window where pulls fail. That's what I was asking for.Verdict: ready
I don't think this is ready to merge yet. The coordinator side is sound. The registry binds to the bridge gateway, and the new INPUT rules sit ahead of the existing -i rt0 --dport 5000 accept. Together they block packets addressed to the gateway that arrive on a public interface. The rules still let through the paths that need access: lo (host containerd), rt0 (buildkit and sandboxes) and flannel-wg (runners). Ordering also holds: the bridge gets its address in SandboxController.Init, and the registry now DependsOn(sandboxHost). The problems are in how a distributed runner learns the address.
A coordinator blip while a runner starts now stops the runner from booting, and the error points at the wrong cause. The internal IP is read inside setupRemoteWorkloadIssuer. That function gives up after 3 attempts, and ClusterAccess.Start only logs its error as a Warn (runner.go:478). Before this PR, that failure just meant no workload tokens. Now prepareNetworkDeps fails the boot with "coordinator internal WireGuard address is unavailable", and nothing points the operator to the RPC failure a few seconds earlier. The coupling breaks the other way too: a malformed IP returns before the issuer is wired, so identity gets turned off because of a registry-address problem. Sandbox-host now requires this value, so fetching it should be its own step that fails startup with the real error.
Mixed versions break in both directions, and neither the docs nor the code say so.
cluster.local to the coordinator's public IP. Nothing listens on :5000 there anymore, so image pulls fail.coordinator_internal_ip (the field is missing, not empty), so the runner refuses to start.So there's no upgrade order that keeps pulls working. That may be fine if runners and coordinator always upgrade together. If so, please say it in the PR and the upgrade notes. If not, consider a transitional fallback: when the field is absent, keep the old public mapping and log a loud warning.
Tests: The registration tests and the prepareNetworkDeps tests cover the new plumbing well. TestRegistryOnlyListensOnChosenInterface mostly checks how net.Listen behaves, not anything this PR adds. It also needs 127.0.0.2 to be usable, which it isn't by default on macOS, and the file has no build tag. The iptables rules are what actually protect the registry, and nothing tests them. A blackbox check that a runner can pull over the overlay would give me much more confidence here.
queryWorkloadIssuerInfo fails 3 times, we return before this block. Start only logs a Warn, and then prepareNetworkDeps fails with "internal WireGuard address is unavailable", so the real RPC error never reaches the boot failure. The other way round, an invalid IP returns an error here before the issuer is wired, so identity gets turned off over a registry problem. I'd move this into its own required step in ClusterAccess.Start (or have Start return the error when the address is missing). Then startup fails with the real cause, and identity setup and registry addressing can't take each other down.coordinator_internal_ip, this fails the whole runner boot. If the runner is older and the coordinator is newer, the runner keeps mapping cluster.local to the public IP, where the registry no longer listens. Either document that runners and coordinator have to be upgraded together, or add a transitional fallback here when the field is absent (the old public-IP mapping plus a warning).Verdict: not_ready