kacerrandClaude Opus 5
bbe9b751a9
A host name is pasted into the providerID hosts/<name>/domains/<vm>, which parseProviderID splits on "/", so a name containing a slash produced an ID that never parsed again: Get, Delete and GC all failed permanently and the VM leaked with the Proxy's finalizer stuck. The image map key has the same defect and lands in a file name rather than an ID: base-<key>.<format> in the host's storage pool. Both now have to match a plain identifier pattern. Machine-type keys are left alone; they are only ever compared, never embedded. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
78 lines
4.0 KiB
Markdown
78 lines
4.0 KiB
Markdown
# Execution: fix the five code-review findings on `feat/kvm-provider`
|
|
Plan: [docs/plans/2026-09-10-2222-kvm-review-fixes.md](../plans/2026-09-10-2222-kvm-review-fixes.md)
|
|
|
|
- [x] Step 0 — Save plan into the repo
|
|
- [x] Step 1 — Unwedge deletion (finding 1)
|
|
- [x] Step 2 — Validate names that become identifiers (finding 2)
|
|
- [ ] Step 3 — Read the SSH material lazily (finding 3)
|
|
- [ ] Step 4 — Guest agent via vendor-data script (findings 4 and 5)
|
|
- [ ] Step 5 — Validate, record, push
|
|
|
|
## Step 0 — plan committed
|
|
|
|
Copied out of `~/.claude/plans/` and committed alone before touching code, as
|
|
the repo's plan rules require.
|
|
|
|
## Step 1 — Unwedge deletion
|
|
|
|
Three changes, in the order the bug unfolds.
|
|
|
|
`reconcileDelete` calls `Get` first and only drops the finalizer when it
|
|
answers not-found, so any permanent error there retried forever. It now
|
|
switches on the class: a permanent `Get` still calls `Delete`, which is
|
|
idempotent, and releases the object when that succeeds. Polling a `Get`
|
|
that cannot converge was the wedge; `Delete` is the only call that can
|
|
still make progress.
|
|
|
|
`crawl.example.com/skip-cleanup: "true"` abandons the instance and removes
|
|
the finalizer. It is checked **before** the provider lookup, which also
|
|
frees a Proxy whose provider was deleted from the config — a second wedge
|
|
sitting three lines above the reported one, with the same shape. No
|
|
`EventRecorder` exists on the reconciler and none was added, so the
|
|
abandonment is an Info log; `docs/architecture.md` already records that
|
|
decision for the whole controller.
|
|
|
|
In the KVM provider, `Delete` for a providerID naming an unconfigured host
|
|
now logs and returns success. The precedent is two functions further down:
|
|
`removeVolumes` treats a missing pool the same way, with a comment saying
|
|
outright that refusing "would pin the Proxy's finalizer".
|
|
|
|
**`Get` deliberately still fails permanently for that case.** Making it
|
|
answer not-found would have been a one-line fix for the wedge, but the live
|
|
reconcile path reads not-found as "the VM is gone" and rebuilds it
|
|
elsewhere. A typo in `hosts` would then quietly duplicate every Proxy on
|
|
that host, with the originals still running where GC cannot see them, since
|
|
`ListByTag` only sweeps configured hosts. The asymmetry is now pinned by a
|
|
test that asserts both halves at once.
|
|
|
|
## Step 2 — Validate names that become identifiers
|
|
|
|
One regexp, `^[A-Za-z0-9][A-Za-z0-9_.-]*$`, applied to `hosts[].name` and to
|
|
the `images` map keys. Dots stay legal because a host name like
|
|
`nas1.home.lan` is ordinary and harmless; the character that actually
|
|
breaks things is `/`.
|
|
|
|
The review only reported the host name. The image key has the same defect
|
|
and is arguably worse: it is not merely embedded in an ID but pasted into a
|
|
file name, `base-<key>.<format>`, so `../etc/x` would be a path traversal
|
|
into the storage pool rather than a parse failure. Both are validated in the
|
|
same pass. Machine-type keys are left alone deliberately — they are only
|
|
ever compared against `placement.machineType`, never embedded anywhere.
|
|
|
|
Worth noting: this is validation of the operator's own config file, which
|
|
only a cluster admin can write, so the traversal was never a privilege
|
|
boundary. It is still cheaper to reject at load than to explain a stuck
|
|
Proxy later. Both new table cases assert the message text, so the wording
|
|
that names the consequence is now pinned by tests.
|
|
|
|
Worth noting: the review filed this as a KVM finding, but the fix is mostly
|
|
in the controller, because KVM is the only provider that resolves a
|
|
providerID segment against operator config at all. GCP and Kubernetes hand
|
|
the lookup to the backend, which answers 404 for a retired zone or a deleted
|
|
namespace, so they reach not-found and release on their own. Anything else
|
|
that classifies permanently on the delete path — revoked RBAC on the
|
|
Kubernetes provider is the realistic one — was equally stuck before this
|
|
step and is what the annotation is really for. The KVM table case
|
|
`"host not configured"` moved out of `TestDelete_errors` into its own named
|
|
test, since it is no longer an error.
|