Fix CA ordering so fresh installs boot with etcd mTLS - #964
Conversation
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 33 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds 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: ✅ ready to merge — auto-review, non-blocking
This is a well-targeted fix for a real bootstrap ordering problem. On a fresh install with DistributedRunners enabled, SetupEtcdTLS was called before Coordinator.Start had a chance to run LoadCA — meaning there was no CA on disk yet, causing a "CA certificate not found" crash-loop. The fix is clean: extract the create-if-missing CA logic into a package-level EnsureCA, then call it in server.go immediately before SetupEtcdTLS. Coordinator.LoadCA now delegates to EnsureCA instead of duplicating the logic, which is a genuine improvement.
The logic is correct. EnsureCA is idempotent (reads existing file first, only generates+writes when absent), so calling it twice — once early in Server(), once again when Coordinator.Start → LoadCA → EnsureCA runs — is harmless. The both-CA-and-key-on-disk cases are handled correctly, and file permissions are preserved (0644 for certs, 0600 for keys).
The tests are solid for what they cover. TestEtcdTLSColdBoot faithfully reproduces MIR-1464: it fails SetupEtcdTLS with no CA, calls EnsureCA, then verifies the issued certs chain properly. TestEnsureCAIdempotent pins that the second call doesn't regenerate. That's the right set of tests for this change.
One stale comment remains: SetupEtcdTLS's doc says "The CA must already exist (created by the coordinator's LoadCA)" at line 179, but that's no longer true — the callsite in server.go now ensures it via EnsureCA. The comment should be updated to reflect the new invariant (callers must call EnsureCA or LoadCA first). It's not a correctness issue, but it will confuse the next person to read that function.
🍪 full review note · comment /biscuit review to run biscuit again.
Inline comments
components/coordinate/coordinate.go:179
This doc comment is now stale. The CA is no longer guaranteed by Coordinator.LoadCA; it's guaranteed by EnsureCA, which server.go calls before this function. Consider updating to something like: "The CA must already exist on disk; call EnsureCA (or LoadCA) before SetupEtcdTLS."
🤖 Prompt for AI Agents
In components/coordinate/coordinate.go at line
179, the doc comment on SetupEtcdTLS says 'The CA
must already exist (created by the coordinator's
LoadCA).' This is stale — the caller in server.go
now uses EnsureCA directly. Update the comment to
say the CA must already exist on disk, and that
callers should call EnsureCA or Coordinator.LoadCA
before invoking this function.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/coordinate/coordinate.go`:
- Around line 472-514: Update the CA loading and generation flow around the
existing certificate/key reads so both files are inspected before deciding to
create a CA: load the pair only when both exist, create only when both return
os.ErrNotExist, and propagate any other read error. Publish the generated
certificate and key using a recoverable atomic pair strategy that prevents a
successful cert write with a failed key write from being treated as a valid or
permanently broken CA; add failure-injection coverage for partial publication.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b5ec191b-e4ac-437a-8efa-8a00b6378f4b
📒 Files selected for processing (3)
cli/commands/server.gocomponents/coordinate/coordinate.gocomponents/coordinate/etcd_tls_test.go
| if data, err := os.ReadFile(cert); err == nil { | ||
| c.Log.Info("loading existing CA", "path", cert) | ||
| log.Info("loading existing CA", "path", cert) | ||
|
|
||
| key, err := os.ReadFile(keyPath) | ||
| if err != nil { | ||
| return fmt.Errorf("missing key for CA: %w", err) | ||
| return nil, fmt.Errorf("missing key for CA: %w", err) | ||
| } | ||
|
|
||
| ca, err := caauth.LoadFromPEM(data, key) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to load CA: %w", err) | ||
| return nil, fmt.Errorf("failed to load CA: %w", err) | ||
| } | ||
|
|
||
| c.authority = ca | ||
| } else { | ||
| c.Log.Info("generating new CA", "path", cert) | ||
| return ca, nil | ||
| } | ||
|
|
||
| ca, err := caauth.New(caauth.Options{ | ||
| CommonName: "miren-server", | ||
| Organization: "miren", | ||
| ValidFor: 10 * year, | ||
| }) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to generate CA: %w", err) | ||
| } | ||
| log.Info("generating new CA", "path", cert) | ||
|
|
||
| err = os.MkdirAll(filepath.Dir(cert), 0755) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to create CA directory: %w", err) | ||
| } | ||
| ca, err := caauth.New(caauth.Options{ | ||
| CommonName: "miren-server", | ||
| Organization: "miren", | ||
| ValidFor: 10 * year, | ||
| }) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to generate CA: %w", err) | ||
| } | ||
|
|
||
| cd, kd, err := ca.ExportPEM() | ||
| if err != nil { | ||
| return fmt.Errorf("failed to export CA: %w", err) | ||
| } | ||
| if err := os.MkdirAll(filepath.Dir(cert), 0755); err != nil { | ||
| return nil, fmt.Errorf("failed to create CA directory: %w", err) | ||
| } | ||
|
|
||
| err = os.WriteFile(cert, cd, 0644) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to write CA cert: %w", err) | ||
| } | ||
| cd, kd, err := ca.ExportPEM() | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to export CA: %w", err) | ||
| } | ||
|
|
||
| err = os.WriteFile(keyPath, kd, 0600) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to write CA key: %w", err) | ||
| } | ||
| if err := os.WriteFile(cert, cd, 0644); err != nil { | ||
| return nil, fmt.Errorf("failed to write CA cert: %w", err) | ||
| } | ||
|
|
||
| c.authority = ca | ||
| if err := os.WriteFile(keyPath, kd, 0600); err != nil { | ||
| return nil, fmt.Errorf("failed to write CA key: %w", err) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve CA-pair integrity on failed or interrupted creation.
A successful ca.crt write followed by a failed ca.key write leaves later boots permanently failing at the “missing key” path. Also, any non-NotExist certificate read error is treated as absence and can start CA replacement. Read both files first, create only when both are definitively absent, and use a recoverable atomic publish strategy for the pair. Add failure-injection coverage for partial writes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/coordinate/coordinate.go` around lines 472 - 514, Update the CA
loading and generation flow around the existing certificate/key reads so both
files are inspected before deciding to create a CA: load the pair only when both
exist, create only when both return os.ErrNotExist, and propagate any other read
error. Publish the generated certificate and key using a recoverable atomic pair
strategy that prevents a successful cert write with a failed key write from
being treated as a valid or permanently broken CA; add failure-injection
coverage for partial publication.
There was a problem hiding this comment.
Good catch on the read-error case. Tightened EnsureCA to only regenerate when the cert is genuinely absent (os.ErrNotExist); any other read error now propagates instead of silently overwriting a valid CA, with a test that pins it.
Punting the atomic cert/key publish + partial-write failure injection as out of scope here: it's pre-existing behavior lifted verbatim from LoadCA, and it fails loudly ("missing key for CA") rather than silently. Worth a follow-up if we want the belt-and-suspenders.
There was a problem hiding this comment.
@phinze, thanks—the read-error fix addresses that portion of the finding. The partial CA-pair publication risk remains deferred as noted, which is reasonable for this narrowly scoped bootstrap fix.
Would you like me to open a follow-up GitHub issue for recoverable/atomic CA cert-and-key publication plus failure-injection coverage?
SetupEtcdTLS reads the server CA from disk and hard-errors if it's missing, but the only create-if-missing path (Coordinator.LoadCA) runs much later inside Coordinator.Start. With distributed runners enabled, a fresh `server install` starts the service before anything has generated a CA, so the first boot fails "CA certificate not found" and crash-loops until the install's later auth generate lands. A bare `miren server` on an empty data dir never recovers at all. Extract the CA create-if-missing logic out of LoadCA into a standalone EnsureCA, and call it right before SetupEtcdTLS so the binary bootstraps its own CA. Scoped to the distributed-runners branch, so every other boot path is unchanged. Also tighten the read path while it's being extracted: distinguish a missing CA from an unreadable one, so a transient read error can't be mistaken for absence and silently regenerate over a valid CA.
7d41b25 to
dfa58c6
Compare
2 participants
When we turned on etcd mTLS for distributed runners, we introduced a chicken-and-egg problem that only bites on a truly fresh machine.
SetupEtcdTLSreads the server CA off disk and gives up with "CA certificate not found" if it isn't there. Trouble is, the only code that creates the CA when it's missing isCoordinator.LoadCA, and that doesn't run untilCoordinator.Start, several hundred lines and a bunch of component starts later.Existing clusters never noticed, because a prior boot had already written the CA, and
make devsidesteps it by runningauth generatefirst. But a productionserver installstarts the systemd service before it generates the CA. Registration runs first and creates a service-account keypair for cloud auth, but nothing writes the server CA until the very end, whenconfigureLocalClientshells out toauth generate. So with distributed runners on by default the first boot fails, crash-loops onRestart=always, and only self-heals once thatauth generatewrites the CA. The install's readiness check reports failure the whole time. Worse, a baremiren serveron an empty data dir with no out-of-band bootstrap fails permanently: the etcd TLS block always errors beforeLoadCAis ever reached.The fix is to make the binary self-sufficient. We pulled the create-if-missing logic out of
LoadCAinto a standaloneEnsureCAand call it right before the etcd TLS setup, so the server materializes its own CA on cold boot. It's scoped to the distributed-runners branch, so every other boot path stays byte-for-byte the same. There's a cold-boot test that pins the ordering invariant: a fresh data dir makesSetupEtcdTLSfail, andEnsureCAis what makes it succeed, with the issued etcd certs verified to chain back to the CA.While extracting the logic, we also tightened the read path so a CA cert that exists but can't be read (permissions, transient IO) is treated as an error rather than absence, instead of silently regenerating over a valid CA and invalidating every cert it issued.
Closes MIR-1464