egress-proxies-operator/docs/plans-executions/2026-09-10-2222-kvm-review-fixes.md

Execution: fix the five code-review findings on feat/kvm-provider

Plan: docs/plans/2026-09-10-2222-kvm-review-fixes.md

• Step 0 — Save plan into the repo
• Step 1 — Unwedge deletion (finding 1)
• 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.

Step 3 — Read the SSH material lazily

kvm.New no longer loads the key; lazyAuth does, on the first connection, caching the result once it works. registry.Build is all-or-nothing by design, so the old constructor error did not merely fail the KVM provider, it exited the manager before any controller started.

The plan said "lazy dialer", and the first attempt was exactly that — one closure wrapping both the loading and the dial. It could not be tested: the fixture's SSH server is real, but the socket behind it is an echo server, so every successful dial dies in the libvirt handshake and the assertion could not tell "the key worked" from "the key did not". Splitting the seam in two, lazyAuth for the material and lazyDialer for the wiring, made both testable: the healing path against lazyAuth, and a separate test that the dialer really authenticates, asserted by the error being past SSH and by the server having seen the forward request.

Error classes carry the intent. A missing file stays unclassified, so provider.Class defaults it to transient and a Proxy heals on its own when the Secret appears — the common case, since the Secret is usually created after the manager. A key that does not parse wraps ErrPermanent, because retrying cannot fix it. newSSHAuth distinguishes the two with errors.Is(err, fs.ErrNotExist).

Worth noting: the startup check is kept, just non-fatally, logged with the WARNING: prefix the repo already uses where logr has no warn level. That preserves the diagnostic the eager version was written for. The caching test deliberately deletes the key file after a successful load and asserts the cached value is reused, which pins the rotation behaviour the docs promise: a rotated Secret needs a manager restart. TestNew_badKeyPathFailsFast was inverted rather than deleted, so the old contract stays visible in the diff next to the new one.

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.