# 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) - [x] Step 3 — Read the SSH material lazily (finding 3) - [x] Step 4 — Guest agent via vendor-data script (findings 4 and 5) - [x] 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-.`, 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. ## Step 4 — Guest agent via vendor-data script The review filed two findings against `withGuestAgent` — detection too narrow, and YAML re-typing on the round-trip. Reading cloud-init 26.1's sources while planning turned both into one finding: the operator should not be editing the user's document at all. Vendor-data gets its own handler chain (`stages.py:_default_vendordata_handlers`), with separate script and cloud-config paths. A vendor-data **script** is therefore written to the vendor script directory and run by `cc_scripts_vendor`, never touching the config merger that drops colliding keys. On the final-stage module list it lands after `package_update_upgrade_install` and before the user's `runcmd`, so the agent now comes up *earlier* than the old `runcmd` append did. That also closed the door the previous approach had left ajar. The merge cannot be rescued from inside either document: `merge_how` in user-data governs only the first source, and `merge_how` in vendor-data is popped by `dict_extract_mergers` and discarded before the document is written. There was no fix that kept the merge. `withGuestAgent`, `isCloudConfig` and `appendUnique` are gone, along with seed.go's `sigs.k8s.io/yaml` dependency. `installGuestAgent` (default true) turns the script off. When `vendorData` is also configured the two ship as a MIME multipart, with the user's document declared `text/plain` — a type cloud-init re-sniffs from the content's own header line, so this code never classifies what the user wrote. Before deleting the old path, its damage was captured rather than taken on trust. Round-tripping a plausible cloud-config through it: ```sh # temporary test in the package, deleted afterwards go test -run TestZZProbeRetyping -v ./internal/provider/kvm/ # in: version: 1.10 # out: version: 1.1 ``` `1.10` arriving as `1.1` is worse than the reviewer's `passwd` example, since nothing about it looks wrong in the seed. Comments were dropped and keys reordered on the same pass. The generated vendor-data was then checked against the cloud-init actually installed on this machine, rather than against the docs: ```sh python3 -c 'from cloudinit import user_data, handlers; ...' # vendor-script: multipart=False sniffed=text/x-shellscript # vendor-multipart: multipart=True part 1 text/plain -> text/cloud-config # part 2 text/x-shellscript ``` and the script itself through `sh -n`. Worth noting: the script has to choose a package manager where cloud-init's `packages:` key was distro-agnostic. It covers apt, dnf, yum, zypper and apk, and both systemd and OpenRC for the enable step, but an image outside that set now needs `installGuestAgent: false` and the agent baked in. The seed's user-data is byte-identical to the Proxy's `cloudInit` from here on, which is what the new `TestBuildSeedISO_userDataIsNeverRewritten` pins across seven formats. One consequence worth remembering: a user-data that sets `vendor_data: {enabled: false}` disables the agent, and cloud-init documents that as intended behaviour. ## Step 5 — Validation Unit suite, `go vet` and `gofmt` clean across the repo. Then the two real runs. **Integration against beelink**, unchanged environment from the KVM plan's log, passed in 22 s on a warm cache, with `virDomainInterfaceAddresses source=DomainInterfaceAddressesSrcAgent interfaces=2` — the agent installed by the new vendor script answering, on a stock Debian 13 image. **In-cluster**, both Proxies deleted and recreated against the rebuilt image: Ready with an address 2 min 3 s after `kubectl apply`, 204 through squid from the dev box for both. Inside the debug VM: ```sh ssh novakj@192.168.0.94 'systemctl is-active qemu-guest-agent; cloud-init status; \ sudo head -4 /var/lib/cloud/instance/user-data.txt; sudo ls /var/lib/cloud/instance/scripts/vendor/' # active # status: done # #cloud-config # # Login for debugging: user "novakj", password "kvm-dev", passwordless sudo. # ... # part-001 ``` Two things worth having seen with your own eyes there: the user's **comment lines survived** into the guest, which the old merge stripped, and the script really did land in the vendor directory rather than being merged into the user's config. **Negative check for the Secret**, done by pointing the deployment's volume at a name that does not exist rather than deleting the real Secret: ```sh kubectl -n egress-proxies-operator-system patch deploy egress-proxies-operator-controller-manager \ --type=json -p '[{"op":"replace","path":"/spec/template/spec/volumes/1/secret/secretName","value":"kvm-ssh-absent"}]' ``` The manager came up and stayed up, 0 restarts, logging `WARNING: kvm ssh material is not usable ... open /etc/proxy-operator/kvm/id_ed25519: no such file or directory`. The Kubernetes Proxies kept reconciling; the KVM ones failed per reconcile and retried. Before this branch the same config exited the process before any controller started. Patched back afterwards, everything Ready. Worth noting: the KVM Proxies kept reporting `Ready` during that window rather than degrading visibly. That is existing behaviour, not a regression — a transient provider error rides the workqueue and does not clear a healthy status — but it does mean the only signal of a missing Secret is the log and the reconcile errors. Also, the ListByTag sweep on beelink still reports `managed=2` where one domain is an orphan from an older failed run; unrelated to this work, and the same leftover noted in the KVM plan's Run 6. 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.