Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (16)
Included review availability: 1 review is currently available. Your included PR review attempts over the past 7 days set your current allowance at 6 reviews per hour. 📝 WalkthroughWalkthroughThe coordinator now binds its OCI registry to the mapped internal address and installs firewall rules for local and WireGuard traffic. Runner registration reports that address, and runner startup validates and uses it to resolve the registry host. The changes also add tests and document the required upgrade and restart sequence. Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to The coordinator's image registry now listens only on the internal bridge address. Firewall rules limit it to local and WireGuard traffic, and runners pull images over WireGuard. The upgrade path keeps older-coordinator compatibility through the API-address fallback, and the required runner restart after a coordinator upgrade is documented. No outstanding merge-blocking issues remain. Comment |
![miren-code-agent[bot]](https://avatars.githubusercontent.com/u/174767976?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
phinze
left a comment
phinze
left a comment
There was a problem hiding this comment.
🤖 Functionally this looks good to go. Binding the registry to the bridge gateway and reaching it over WireGuard is a nice answer to the question MIR-1477 left open, and it's a real security win beyond reachability: since #991, runners have been sending bearer tokens and pulling app images over plain HTTP across the public network, and now all of that rides the encrypted overlay. The per-runner pull gap during an upgrade roll seems acceptable to us.
The two docs callouts could use a cleanup pass, though. Notes inline, neither blocking.
| :::danger[Keep the registry off the internet] | ||
| TCP port 5000 is cluster-internal. Do not expose it externally, including on bare-metal hosts without a cloud security group. The OCI registry listens on the coordinator's Miren bridge gateway, not on its public interfaces. | ||
| ::: |
There was a problem hiding this comment.
🤖 nit: this reads a little oddly now. It's a danger telling operators not to expose 5000, and then the last sentence says the registry doesn't listen on public interfaces anyway, so there's nothing left for them to do. I think this PR moved the right guidance from "block it" to "you never need to open it." That could be a plain note saying 5000 is cluster-internal and never needs opening, plus the version where this changed so anyone still on an older release (which listens on all interfaces) knows to block it themselves.
There was a problem hiding this comment.
🤖 The callout now says public TCP 5000 never needs opening on the internal-only release, and tells operators of older releases to restrict access to trusted runners. I left the release number unset until it is assigned. Docs lint/build and a rendered-page check passed.
| :::warning[Upgrade runners before changing the registry address] | ||
| Upgrade runner binaries first, while the coordinator still serves the registry on its old address. Then upgrade the coordinator and restart the runners so they learn its internal WireGuard address. Image pulls on runners that have not restarted after the coordinator upgrade will fail; plan a maintenance window for this transition. | ||
| ::: |
There was a problem hiding this comment.
🤖 The framing here is a bit off. "Before changing the registry address" makes it sound like something an operator does, but nobody changes that address by hand. What moves it is upgrading to this release. I think this wants to read as a one-time note for the version where it lands: "when upgrading from vX to vY, ..."
It's also worth squaring with how upgrades actually run. Managed upgrades (miren-cloud/upgrades.md) do the server first and then roll runners one at a time, which is the opposite of the order here. That still works out: each runner can't pull images from the moment the server upgrades until its own upgrade restarts it. Saying that directly would keep a managed-upgrade user from thinking they need to intervene, and the runners-first order can stay as the advice for anyone upgrading by hand.
There was a problem hiding this comment.
I agree, and I missed this. When I signed off on the runners-first order, I didn't check it against miren-cloud/upgrades.md. That doc says the server upgrades itself first and then rolls runners "one at a time", so managed users will never do what this warning tells them to. As you say, the managed order still ends up fine. Each upgraded runner restarts on a binary that reads coordinator_internal_ip, so pulls only fail on a given runner between the server upgrade and that runner's own restart. Writing it as a one-time "upgrading from vX to vY" note that says that window closes on its own for managed upgrades, and keeping runners-first as the advice for manual upgrades, would make it both accurate and less alarming.
There was a problem hiding this comment.
🤖 The one-time upgrade note now describes managed coordinator-first upgrades and the per-runner pull gap, alongside runner-first advice for manual upgrades. I did not hard-code a release number before it is assigned. Docs lint/build and a rendered-page check passed.
There was a problem hiding this comment.
I checked the rewritten note against the code, and it's accurate. It says managed upgrades go coordinator-first with a pull gap on each runner that ends when that runner restarts. That matches the runner reading coordinator_internal_ip at startup. Runners-first still holds for manual upgrades, because of the API-address fallback in prepareNetworkDeps. Holding off on the version number until it's assigned makes sense to me, but please fill in the "upgrading from vX to vY" wording once the release is cut, since that's how readers will tell whether the note applies to them. phinze, it's your thread, so I'll leave closing it to you.
There was a problem hiding this comment.
🍪 biscuit: ✅ ready to merge — auto-review, non-blocking
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.
🍪 full review note · reviewed at 253fe64 · comment /biscuit review to run biscuit again.
2 participants
What changed
rt0bridge gateway (10.8.x.1:5000), and restrict inbound traffic to local/bridge and WireGuard interfaces.coordinator_internal_ip; mapcluster.localto it for image pulls while leaving coordinator API access on its configured address. Existingcluster.local:5000image references and bearer-token checks remain unchanged.Verification
make lint: 0 issues.iso run go test -p 1 ./network ./components/coordinate ./components/runner ./components/distributedrunner ./components/ocireg ./servers/runner ./components/server -skip '^TestRunnerCoordinatorIntegration$' -count=1: all seven packages passed. A focused ISO test verified the firewall rules precede an old bridge allow rule; runner tests cover internal routing and legacy coordinator fallback.cd docs && bun run lint && bun run build: passed. Both changed callouts were rendered and visually inspected.10.8.99.1:5000, runner routed to it throughflannel-wg, unauthenticated/v2/returned 401, public-facing port 5000 refused connections, and a BuildKit push/runner pull ofcluster.local:5000/...deployed a healthy app.TestRunnerCoordinatorIntegrationbecause the test container could not connect to/run/containerd/containerd.sock(timeout); it passed with that environment-dependent test excluded.Rollout / tradeoffs
This one-time transition can interrupt image pulls on a runner between the coordinator cutover and that runner’s restart. Miren Cloud-managed upgrades update the coordinator first and then restart runners one at a time, closing each runner’s gap automatically. For manual upgrades, upgrade runner binaries first while the old coordinator serves the registry, then upgrade the coordinator and restart the runners again to learn its WireGuard gateway. Runners cache the gateway at startup and need restarting if the coordinator subnet lease changes. External BuildKit clients need their own private route and name mapping. HTTP registry requests still require bearer tokens; WireGuard encrypts the inter-host transport.