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](../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)
- [ ] 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.

## 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.

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.