Record the review-fix validation: integration, in-cluster and the missing-Secret check

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

CHANGELOG.md

@@ -1,5 +1,37 @@
 # Changelog
 
+## 2026-09-10 22:58 UTC — Code-review fixes on the KVM branch: deletion, validation, lazy SSH, guest agent
+
+Five findings from a review of `feat/kvm-provider`, all fixed on the branch.
+
+- **A Proxy can always finish deleting.** The delete path used to drop the finalizer
+  only when `Get` answered not-found, so a permanent error there retried forever —
+  a KVM host removed from the config was enough. It now calls `Delete` anyway, since
+  that is idempotent and can finish where `Get` cannot, and
+  `crawl.example.com/skip-cleanup: "true"` abandons the instance for whatever stays
+  stuck. In the KVM provider, deleting over an unconfigured host is a no-op success;
+  `Get` still fails permanently there so a typo cannot look like a missing VM.
+- **The guest agent ships as a vendor-data script**, superseding the cloud-config
+  merge in the entry below. Merging could never be made safe: cloud-init drops
+  vendor-data keys the user-data defines, and the strategy cannot be overridden from
+  either document. The Proxy's cloud-init now reaches the seed byte for byte in any
+  format — Jinja, MIME, gzip, scripts — where the round-trip previously re-typed
+  unquoted scalars (`version: 1.10` arrived as `1.1`), dropped comments and reordered
+  keys. New `installGuestAgent` setting, default on.
+- **A missing `kvm-ssh` Secret no longer crash-loops the manager.** The SSH material
+  is read on first use and cached, so only KVM Proxies fail and they recover when the
+  Secret appears. A missing file is transient, an unparseable key permanent, and
+  startup still logs a warning.
+- **Host names and image keys are validated.** Either can end up in a providerID or a
+  volume file name, where a slash made the VM unaddressable for good.
+- Verified end to end: unit suite, the integration test against a real hypervisor
+  (22 s, agent reporting the IP), both in-cluster Proxies Ready in 2 min with 204
+  through squid, the guest showing `qemu-guest-agent` active with its cloud-config
+  comments intact, and the manager staying up with the Secret pointed away.
+- Key files: `internal/provider/kvm/{seed.go,conn.go,kvm.go}`,
+  `internal/controller/proxy_controller.go`, `internal/provider/config_kvm.go`,
+  `api/v1alpha1/proxy_types.go`.
+
 ## 2026-09-10 21:04 UTC — KVM provider passes in-cluster; guest agent merged into cloud-config
 
 - Two KVM Proxies with real cloud-configs reach `Ready` from inside the kind cluster

README.md

@@ -164,6 +164,12 @@ Cloud-init from a Secret: the Secret **must** carry the label
 labelled Secrets, so an unlabelled one is invisible (the Proxy reports
 `CloudInitError`). Rotating the Secret's content triggers VM replacement.
 
+Stuck deletion: a Proxy stays `Terminating` while the operator keeps failing
+to clean its instance up — credentials revoked, a provider or a KVM host
+dropped from the config. Annotate it with
+`crawl.example.com/skip-cleanup: "true"` to abandon the instance and release
+the object. Whatever is still running then has to be removed by hand.
+
 ## Providers
 
 `Managed` proxies are provisioned by a provider — a small compute backend

api/v1alpha1/proxy_types.go

@@ -62,6 +62,15 @@ const (
 	// must be replaced.
 	AnnotationSpecHash = "crawl.example.com/spec-hash"
 
+	// AnnotationSkipCleanup abandons the provider-side instance: set it to
+	// "true" and deletion removes the finalizer without asking the provider
+	// to clean anything up. It is the escape hatch for a cleanup that cannot
+	// succeed — a provider dropped from the config, credentials revoked, a
+	// backend that answers permanently — where the object would otherwise
+	// stay in Terminating forever. The instance is left running and becomes
+	// the administrator's to remove.
+	AnnotationSkipCleanup = "crawl.example.com/skip-cleanup"
+
 	// LabelCloudInit must be set (to "true") on every Secret referenced by
 	// spec.cloudInit.secretRef: the manager's cache only holds Secrets
 	// carrying this label, so an unlabelled Secret is invisible to the

config/samples/providers-config.yaml

@@ -30,8 +30,9 @@ providers:
       knownHostsFile: /etc/proxy-operator/kvm/known_hosts
       # insecureIgnoreHostKey: true   # skip host-key checks (not recommended)
       # diskSizeGb: 10                # per-VM overlay; cloud-init grows the root fs into it
-      # vendorData: |                 # extra vendor-data; the qemu-guest-agent bootstrap is merged
+      # installGuestAgent: false      # skip the vendor-data script that installs qemu-guest-agent
-      #   #cloud-config               # into each Proxy's cloud-config, not configured here
+      # vendorData: |                 # extra vendor-data, carried beside the agent script as a
+      #   #cloud-config               # MIME multipart. The Proxy's own cloud-init is never rewritten.
       #   ...
       images:
         # placement.image names a key here (or is an https:// URL directly).

docs/architecture.md

@@ -489,6 +489,15 @@ they read as choices, not accidents. Chronological by build step.
   cluster-wide Secret read RBAC — an unrestricted cache would hold every
   Secret in scope). Unlabelled referenced Secrets are invisible by
   construction, surfacing as `CloudInitError`.
+- **Deletion never gives up on its own, but can be released by hand:** a
+  permanent cleanup failure keeps retrying with the finalizer on, because
+  dropping it automatically would leak a running instance on any provider
+  that answers "permission denied". Where `Get` fails permanently the
+  reconciler still calls `Delete`, since it is idempotent and may finish
+  where `Get` cannot converge. `crawl.example.com/skip-cleanup: "true"`
+  abandons the instance and releases the object; it is checked before the
+  provider lookup, so it also frees a Proxy whose provider is gone from the
+  config.
 - **Events RBAC from the plan is omitted:** nothing wires an
   EventRecorder in the prototype, and granting verbs nothing uses would
   be RBAC lint noise. Add the marker together with the recorder if events

docs/kvm-provider.md

@@ -27,13 +27,12 @@ Per host, once:
    `edk2-ovmf` on Fedora/RHEL. VMs boot UEFI by default (libvirt picks
    the OVMF binary itself); a domain start fails with "no firmware
    found" without it. Only `firmware: bios` images skip this.
-6. Optionally, **outbound internet from the VMs**: the operator adds
+6. Optionally, **outbound internet from the VMs**: a vendor-data script
-   `qemu-guest-agent` to every Proxy's cloud-config (`packages` plus a
+   installs `qemu-guest-agent` from the distro mirrors on first boot.
-   `runcmd` that enables it), so it is installed from the distro mirrors
+   Without it the VM still works — the operator falls back to the host's
-   on first boot. Without it the VM still works — the operator falls back
+   ARP table to find the IP, which fills in once the guest sends traffic —
-   to the host's ARP table to find the IP, which fills in once the guest
+   but the ARP table only sees traffic to the host itself, so on a plain
-   sends traffic — but note the ARP table only sees traffic to the host
+   bridge the agent is in practice required.
-   itself, so on a plain bridge the agent is in practice required.
 
 Check from the operator's point of view before wiring it in:
 
@@ -78,9 +77,10 @@ ssh virt@nas1.home.lan ip link show br0
    `/etc/proxy-operator/kvm` as two files, `id_ed25519` and
    `known_hosts`, which is what `sshKeyFile` and `knownHostsFile` in the
    provider config point at. The deployment starts fine without the
-   Secret as long as no `kvm` provider is configured. Rotating the key:
+   Secret, even with a `kvm` provider configured: only that provider's
-   re-run the command (or the make target) and restart the manager; it
+   proxies fail, and they recover once the Secret exists. Rotating the
-   reads both files once at startup.
+   key: re-run the command (or the make target) and restart the manager;
+   both files are read once, on the first connection, and cached.
 
 2. Add a provider entry to the providers config
    (`config/manager/providers_config.yaml` in-cluster; full sample in
@@ -104,9 +104,13 @@ ssh virt@nas1.home.lan ip link show br0
            bridge: br0
    ```
 
-   The operator reads the key and known_hosts at startup and refuses to
+   The operator checks both files at startup and logs a `WARNING:` line
-   start if either is missing — a bad path fails fast, not on the first
+   when either is unreadable, but it does not refuse to start: the
-   Proxy.
+   provider set is built all-or-nothing, so exiting here would crash-loop
+   the manager and take the `kubernetes` and `gcp` providers with it.
+   The files are read for real on the first connection. A missing file is
+   a transient failure, so proxies heal by themselves once the Secret
+   lands; a key that does not parse is permanent and needs a fix.
 
 3. Apply a Proxy (`config/samples/proxy_kvm.yaml`) and watch it:
 
@@ -131,13 +135,15 @@ ssh virt@nas1.home.lan ip link show br0
 | `knownHostsFile` | unless `insecureIgnoreHostKey` | | OpenSSH `known_hosts` to verify host keys against |
 | `insecureIgnoreHostKey` | | `false` | skip host-key verification entirely |
 | `diskSizeGb` | | `10` | capacity of each VM's overlay disk; cloud-init's growpart expands the root fs into it |
-| `vendorData` | | | extra cloud-init vendor-data placed on every seed next to the Proxy's user-data; cloud-init drops any top-level key here that the user-data also defines. The guest-agent bootstrap is not configured here, see *What lands on the host* |
+| `vendorData` | | | extra cloud-init vendor-data placed on every seed next to the Proxy's user-data. Beware that cloud-init drops any top-level key of a vendor-data **cloud-config** that the user-data also defines. Shipped as one part of a MIME multipart alongside the agent script when `installGuestAgent` is on |
+| `installGuestAgent` | | `true` | add the vendor-data script that installs and starts `qemu-guest-agent`. Turn off for images that ship it already or cannot reach a package mirror; IP discovery then depends on the host's ARP table |
+| `images.<key>` | | | the key names the image in `placement.image`. Letters, digits, dots, dashes and underscores only — it becomes the cached volume name `base-<key>.<format>` |
 | `images.<key>.url` | yes per entry | | http(s) URL of a cloud image (qcow2 or raw) |
 | `images.<key>.sha256` | | | expected digest; verified after download, a change triggers a re-download |
 | `images.<key>.format` | | `qcow2` | `qcow2` or `raw` |
 | `images.<key>.firmware` | | `uefi` | `uefi` (OVMF, secure boot off) or `bios` (SeaBIOS) for images without an EFI system partition; Debian 13 hangs in GRUB under BIOS on a headless VM, see *Known gaps* |
 | `machineTypes.<key>` | ≥ 1 | | `{vcpus, memoryMiB}` size profile |
-| `hosts[].name` | yes | | the `placement.zone` value that selects this host; unique |
+| `hosts[].name` | yes | | the `placement.zone` value that selects this host; unique. Letters, digits, dots, dashes and underscores only — it becomes part of the providerID `hosts/<name>/domains/<vm>` |
 | `hosts[].address` | yes | | SSH endpoint, `host:port` |
 | `hosts[].user` | yes | | SSH user, member of the `libvirt` group |
 | `hosts[].socket` | | `/var/run/libvirt/libvirt-sock` | libvirt socket path on the host |
@@ -176,20 +182,27 @@ console, the guest-agent channel, a virtio RNG and no graphics. Its
 creation time) — that is how orphan GC recognises its own VMs, since
 libvirt has no labels.
 
-The seed's `user-data` is the Proxy's `cloudInit` with the guest-agent
+The seed's `user-data` is the Proxy's `cloudInit`, **byte for byte**. Any
-bootstrap **merged into the document**: `qemu-guest-agent` appended to
+format cloud-init accepts works, including a Jinja template, a MIME
-`packages` and `[systemctl, enable, --now, qemu-guest-agent]` to `runcmd`
+archive, a shell script or a gzipped payload; the operator never parses or
-(neither duplicated if already there; other keys untouched, though the
+re-renders it.
-YAML is re-rendered). It is not shipped as vendor-data because cloud-init
+
-merges vendor-data under user-data with its default mergers, which keep
+The guest agent rides along in `vendor-data`, as a **shell script** rather
-the user-data's value for any key both define and never append lists — a
+than a cloud-config. That is the only shape cloud-init cannot drop:
-cloud-config with its own `packages:` would silently lose the agent and
+vendor-data cloud-config is merged under the user's own document with
-the Proxy would sit in `Provisioning` forever. Only when the user-data is
+mergers that keep the user-data's value for every key both define and never
-not a cloud-config document (a shell script, `#include`, MIME archive)
+append lists, so a Proxy carrying its own `packages:` silently lost the
-does the bootstrap go to `vendor-data`, and then only if `vendorData` is
+agent and sat in `Provisioning` forever. The strategy cannot be overridden
-not configured. A cloud-config that does not parse as a YAML mapping, or
+from either document — `merge_how` in user-data governs only the first
-whose `packages`/`runcmd` is not a list, fails the Proxy permanently
+source, and `merge_how` in vendor-data is consumed and discarded before the
-rather than booting a VM without its config.
+document is stored. A vendor-data script skips all of it: cloud-init gives
+vendor-data its own handler chain, writes scripts to a separate directory
+and runs them from `cc_scripts_vendor`, after the user's packages are
+installed and before the user's `runcmd`. The script is idempotent, picks
+whichever of `apt-get`, `dnf`, `yum`, `zypper` or `apk` the image has, and
+logs into `/var/log/cloud-init-output.log`. Set `installGuestAgent: false`
+to leave it out. When `vendorData` is also configured, the two travel as
+separate parts of a MIME multipart vendor-data.
 
 ## The image cache
 
@@ -264,6 +277,14 @@ The provider logs through controller-runtime's logger, named `kvm`, with
   cleaned when the same Proxy reconciles again (Delete removes volumes by
   name even with no domain), but a Proxy CR that is gone for good leaves
   them for `virsh vol-list` to find.
+- **A host removed from the config takes its VMs with it.** `Delete` for a
+  providerID naming an unconfigured host is a no-op success, so the Proxy
+  finishes deleting instead of pinning its finalizer, but the domain and its
+  volumes stay on the hypervisor and orphan GC cannot see them either (it
+  only sweeps configured hosts). Delete the Proxies before retiring a host,
+  or clean up afterwards with `virsh undefine --nvram` plus `virsh vol-delete`.
+  `Get` still fails permanently for such a providerID on purpose: a typo in
+  `hosts` must not look like a missing VM and trigger a rebuild elsewhere.
 - **The first image fetch is synchronous.** It holds a reconcile worker
   for the download (LAN speed, a few hundred MB, once per image per
   host). Other Proxies keep reconciling on the remaining workers.
@@ -273,8 +294,10 @@ The provider logs through controller-runtime's logger, named `kvm`, with
   bridge with no DHCP never gets an address and stays `Provisioning`
   forever; check `virsh console`. The same symptom with a booted, addressed
   VM means the guest agent never came up: check `systemctl status
-  qemu-guest-agent` and `cloud-init status --long` in the guest (a VM
+  qemu-guest-agent`, `cloud-init status --long` and the vendor script's
-  without outbound internet cannot install it).
+  output in `/var/log/cloud-init-output.log` (a VM without outbound
+  internet cannot install it). A user-data that disables vendor-data
+  (`vendor_data: {enabled: false}`) also removes the agent, by design.
 - **Debian 13 cloud images hang in GRUB under BIOS on a headless VM**
   ([Debian bug #1111240](https://bugs.debian.org/1111240), open, in
   `grub2`): trixie's image ships `terminal_output gfxterm serial` in its

docs/plans-executions/2026-09-04-1404-kvm-provider.md

@@ -631,6 +631,26 @@ and the `beelink` host entries in `config/manager/providers_config.yaml`
 and `config/samples/providers-config.yaml`, which are this machine's dev
 setup rather than the repo's defaults.
 
+### Run 8: review fixes
+
+A `/code-review high` pass over the branch found five defects, fixed in
+[plan 2026-09-10-2222](../plans/2026-09-10-2222-kvm-review-fixes.md) with
+its own execution log. Two of them land on Run 7's conclusions above and
+supersede them:
+
+- The guest-agent **merge described in Run 7 is gone**. It could not be made
+  safe for user-data that is not a plain cloud-config, and its YAML
+  round-trip re-typed unquoted scalars. The bootstrap now ships as a
+  vendor-data shell script, which cloud-init never merges, and the Proxy's
+  cloud-init reaches the seed byte for byte.
+- The read-only-filesystem fix from Run 7 still stands, but the eager SSH
+  key load it sat next to does not: a missing `kvm-ssh` Secret aborted the
+  whole provider set and crash-looped the manager. The material is read on
+  first use now.
+
+Re-validated the same way: integration test against beelink, both Proxies
+recreated in kind, 204 through squid.
+
 ### MR opened
 
 [PR #5](https://gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/pulls/5),

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

@@ -0,0 +1,229 @@
+# 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-<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.
+
+## 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.

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

@@ -0,0 +1,195 @@
+# Plan: Fix the five code-review findings on `feat/kvm-provider`
+**Created:** 2026-09-10 22:22 UTC
+
+## Context
+
+PR #5 (`feat/kvm-provider` → `main`, 19 commits) is open and awaiting merge. A
+`/code-review high` pass over the branch returned five confirmed findings: two that can
+pin a Proxy's finalizer forever, one startup crash-loop, and two in the guest-agent merge
+committed earlier today. All five are fixed on the same branch so the PR updates in place,
+then re-validated against the real hypervisor before merging.
+
+Four decisions, made by the user during planning:
+
+1. **Wedged deletes** — attempt `Delete` even when `Get` fails permanently, make an
+   unconfigured KVM host a no-op delete, and add an annotation that abandons cleanup for
+   whatever still cannot finish.
+2. **Missing `kvm-ssh` Secret** — read the SSH material lazily, so only KVM Proxies fail.
+3. **Guest agent** — stop rewriting the user's cloud-config. Ship the bootstrap as a
+   vendor-data shell script instead.
+4. Validation is unit tests, the integration test against beelink, and the kind e2e. The
+   uncommitted dev-only files stay exactly as they are and out of the PR.
+
+## Why the guest-agent fix is a rewrite, not a patch
+
+Both cloud-init findings come from the same root cause: this operator edits the user's
+document. Reading cloud-init 26.1's sources settles that it never has to.
+
+Vendor-data is consumed by its **own** handler chain with separate script and config paths
+(`stages.py:_default_vendordata_handlers`). A vendor-data **shell script** is therefore
+written to the vendor script directory and run by `cc_scripts_vendor`, never passing
+through the config merger that drops colliding keys. On the final-stage module list it
+runs after `package_update_upgrade_install` and before the user's own `runcmd`, so the
+agent comes up early regardless of what the user's config does.
+
+That also closes the door the previous approach left open. The merge cannot be rescued
+from inside the documents: `merge_how` in the user-data governs only the first source, and
+`merge_how` in vendor-data is popped and discarded by `dict_extract_mergers` before the
+document is ever written. There is no supported way to make vendor-data cloud-config keys
+survive a user-data that defines the same key.
+
+Consequences: user-data reaches the seed byte for byte in any format, so Jinja templates,
+MIME archives, gzip payloads and shell scripts stop being special cases (finding 4), and
+nothing re-serialises YAML, so unquoted scalars keep their type (finding 5). The cost is
+that the script picks the package manager itself where cloud-init's `packages` key was
+distro-agnostic.
+
+## Why the delete fix is shaped this way
+
+KVM is the only provider that resolves a providerID segment against operator config. GCP
+and Kubernetes let the backend answer "not found", so retiring a location yields
+`ErrNotFound` and the finalizer drops. `Provider.host()`
+([kvm.go:118-139](internal/provider/kvm/kvm.go#L118-L139)) returns `ErrPermanent`, and
+`reconcileDelete` ([proxy_controller.go:281-291](internal/controller/proxy_controller.go#L281-L291))
+drops the finalizer only on `ErrNotFound`. `deletionFailure`
+([:327-336](internal/controller/proxy_controller.go#L327-L336)) says in its own comment
+that it retries "until an operator intervenes", and no intervention mechanism exists. The
+annotation is that mechanism, and it also rescues the unconfigured-provider wedge at
+[:276-279](internal/controller/proxy_controller.go#L276-L279).
+
+`Get` keeps returning `ErrPermanent` for an unknown host deliberately. Turning it into
+`ErrNotFound` would make a typo in the host list look like a missing VM on the live path,
+and the reconciler would recreate every affected Proxy elsewhere while the originals keep
+running where garbage collection cannot see them.
+
+## Steps
+
+One commit per step on `feat/kvm-provider`, each ending with
+`Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>`.
+
+### Step 0 — Save this plan into the repo
+Copy to `docs/plans/2026-09-10-2222-kvm-review-fixes.md` with the same header timestamp and
+commit it alone, before any code change. Start
+`docs/plans-executions/2026-09-10-2222-kvm-review-fixes.md` with the status checklist when
+Step 1 lands.
+
+### Step 1 — Unwedge deletion (finding 1)
+- `api/v1alpha1/proxy_types.go`: add `AnnotationSkipCleanup = "crawl.example.com/skip-cleanup"`
+  beside `AnnotationSpecHash` ([:59-63](api/v1alpha1/proxy_types.go#L59-L63)), documented as
+  abandoning the backend instance.
+- `internal/controller/proxy_controller.go`, `reconcileDelete`: honour the annotation after
+  the empty-providerID branch and **before** the provider lookup, so it also covers a
+  provider that is no longer configured. Log the abandoned providerID at Info.
+  `ProxyReconciler` has no `EventRecorder`; do not add one for this.
+- Same function: replace the two `if`s after `Get` with a switch. On
+  `provider.Class(err) == provider.ErrPermanent`, call `Delete` anyway, since it is
+  idempotent, and remove the finalizer when it succeeds rather than requeueing into a `Get`
+  that can never converge. On failure, `deletionFailure(errors.Join(getErr, delErr))`.
+  Guard on `err != nil`, since `Class(nil)` is nil.
+- `internal/provider/kvm/kvm.go`, `Delete`: an unconfigured host segment means there is
+  nothing this operator can clean, so log it and return success, mirroring `removeVolumes`
+  ([kvm.go:506-514](internal/provider/kvm/kvm.go#L506-L514)) for a missing pool. `Get` is
+  unchanged.
+- Tests: extend the deletion table in `internal/controller/reconcile_test.go`
+  ([:357-393](internal/controller/reconcile_test.go#L357-L393)) with permanent-Get plus
+  succeeding Delete, permanent-Get plus failing Delete, the annotation with the provider
+  never called, and the annotation on an unconfigured provider. In `kvm_test.go`, the
+  `"host not configured"` case at [:737](internal/provider/kvm/kvm_test.go#L737) flips from
+  `ErrPermanent` to success; add a named test beside `TestDelete_missingPoolIsNotAnError`
+  ([:715](internal/provider/kvm/kvm_test.go#L715)).
+- Docs: the annotation in `README.md` (~line 163) and `docs/architecture.md` (~line 487);
+  a *Known gaps* entry in `docs/kvm-provider.md` saying a domain on a host removed from
+  config can no longer be cleaned up by the operator, the Proxy still finishes deleting,
+  and the VM must be removed by hand because garbage collection only sweeps configured
+  hosts.
+
+### Step 2 — Validate names that become identifiers (finding 2)
+- `internal/provider/config_kvm.go`: add a name regexp beside `sha256Hex`
+  ([:6](internal/provider/config_kvm.go#L6)) allowing letters, digits, dot, dash and
+  underscore. Apply it to `hosts[].name` after the existing empty check
+  ([:129-131](internal/provider/config_kvm.go#L129-L131)) and to each `images` map key,
+  which becomes the cached volume name `base-<key>.<format>`
+  ([image.go:49-55](internal/provider/kvm/image.go#L49-L55)) and half of the image-cache
+  lock key ([image.go:96](internal/provider/kvm/image.go#L96)). `machineTypes` keys are only
+  compared, never embedded, so leave them.
+- Keep the file's first-error-wins style and `fmt.Sprintf` field paths. The host message
+  should name the consequence: the value becomes part of `hosts/<name>/domains/<vm>`, which
+  `parseProviderID` splits on `/` ([id.go:11-21](internal/provider/kvm/id.go#L11-L21)).
+- Tests: two entries in the `TestLoadConfig_kvmInvalid` table
+  ([config_kvm_test.go:76](internal/provider/config_kvm_test.go#L76)), existing
+  name/yaml/wantErrSub shape.
+
+### Step 3 — Read the SSH material lazily (finding 3)
+- `internal/provider/kvm/conn.go`: keep `newSSHAuth`, and add a lazy dialer that builds it
+  on first dial and caches it once it succeeds, so a `kvm-ssh` Secret created after the
+  manager starts heals on the next reconcile with no restart.
+- `internal/provider/kvm/kvm.go`, `New`: stop calling `newSSHAuth`; pass the lazy dialer.
+  Use the context parameter it currently ignores to log a warning when either file is
+  unreadable at startup, keeping the diagnostic without the exit.
+- Error classes: a missing or unreadable file stays unclassified, so `Class` defaults it to
+  transient and the Proxy retries until the Secret appears; an unparseable key or malformed
+  known_hosts wraps `ErrPermanent`. Distinguish with `errors.Is(err, fs.ErrNotExist)`.
+- Fix the two doc comments promising startup failure
+  ([kvm.go:79-81](internal/provider/kvm/kvm.go#L79-L81),
+  [conn.go:182-185](internal/provider/kvm/conn.go#L182-L185)) and the matching claim in
+  `docs/kvm-provider.md` (~lines 107-109). `DISCOVERY_TOKEN` already sets the house
+  convention for an optional mount: degrade with a loud warning.
+- Tests in `conn_test.go`, which already writes key files: `New` succeeds with no key
+  present, the first dial fails transiently, a dial after the file appears succeeds, and a
+  corrupt key is permanent.
+
+### Step 4 — Guest agent via vendor-data script (findings 4 and 5)
+- `internal/provider/kvm/seed.go`: delete `withGuestAgent`, `isCloudConfig`, `appendUnique`
+  and the `sigs.k8s.io/yaml` import. User-data goes onto the seed exactly as the Proxy
+  supplied it.
+- Add the bootstrap as a `#!/bin/sh` vendor-data script: skip when the agent is already
+  present, otherwise install `qemu-guest-agent` through whichever of `apt-get`, `dnf`,
+  `yum`, `zypper` or `apk` exists, then `systemctl enable --now`. Run `apt-get update`
+  before installing, since nothing else guarantees a fresh index when the user's config has
+  no `packages` key. Keep it idempotent and let it log to stdout, which lands in
+  `cloud-init-output.log`.
+- When the provider config also sets `vendorData`, wrap both into a MIME multipart
+  vendor-data: the user's document as one part, the script as another. Cloud-init detects
+  multipart by finding `mime-version:` in the first 4096 bytes, so the header must lead.
+- `internal/provider/config_kvm.go`: add `installGuestAgent`, defaulting to on. Off means
+  the seed carries only the user's own vendor-data and IP discovery falls back to the host
+  ARP table. Nothing fails permanently any more, so the error path in `writeSeed`
+  ([kvm.go:252-255](internal/provider/kvm/kvm.go#L252-L255)) narrows to MIME assembly.
+- Tests in `seed_test.go`: user-data byte-identical for cloud-config, Jinja, MIME, gzip and
+  script inputs; the script present in vendor-data; the opt-out; the multipart case
+  including the `mime-version:` position; keep the existing ISO tests.
+- Docs: rewrite the *What lands on the host* passage added earlier today, the `vendorData`
+  row and a new `installGuestAgent` row in the config table, and the ARP note in
+  *Known gaps*. Cite the cloud-init behaviour that forced the change.
+
+### Step 5 — Validate, record, push
+- `gofmt -l internal/`, `go vet ./...`, `go test -short ./...`.
+- `make test-integration-kvm` against beelink, environment as recorded in the KVM execution
+  log.
+- Rebuild the image, `kind load`, restart the manager, delete and recreate both Proxies,
+  curl a test URL through each.
+- `CHANGELOG.md` entry at the top with a real `date` timestamp, noting it supersedes the
+  merge approach described in today's earlier entry. Add a Run 8 section to
+  `docs/plans-executions/2026-09-04-1404-kvm-provider.md` plus this plan's own execution
+  file. Push with the token helper; PR #5 updates in place.
+
+## Verification
+- Unit: the controller cases prove a permanent `Get` no longer pins the finalizer and that
+  the annotation short-circuits before the provider is touched. `TestDelete_*` proves an
+  unconfigured host is a clean delete while `TestGet_*` stays permanent, which is what keeps
+  the live path from recreating VMs.
+- Seed: assert the uploaded user-data equals the Proxy's `cloudInit` byte for byte, which is
+  the property the old merge could not offer.
+- Integration on beelink: Create, IP via the guest agent, ListByTag, Delete. In the guest,
+  `cloud-init status --long` clean and `systemctl is-active qemu-guest-agent` active.
+- In-cluster: both Proxies reach Ready with an IP and return 204 through squid. Their
+  cloud-config defines its own `packages`, which is the exact case that failed in Run 7.
+- Negative check in kind: rename the `kvm-ssh` Secret away and restart. The pod must stay
+  up, the Kubernetes Proxies must stay Ready, and only the KVM Proxies degrade. Restore the
+  Secret and confirm they recover without a restart.
+
+## Out of scope
+- The two items the review noted but did not report: deleting a cached base image while
+  running VMs back onto it, and an ad-hoc image URL always being treated as qcow2. Both are
+  documented known gaps and neither is a regression from this branch.
+- The uncommitted dev-only files, by the user's decision.

internal/controller/proxy_controller.go

@@ -273,17 +273,38 @@ func (r *ProxyReconciler) reconcileDelete(ctx context.Context, p *crawlv1alpha1.
 		controllerutil.RemoveFinalizer(p, crawlv1alpha1.FinalizerName)
 		return ctrl.Result{}, r.Update(ctx, p)
 	}
+	if p.Annotations[crawlv1alpha1.AnnotationSkipCleanup] == "true" {
+		// Checked before the provider lookup so it also releases a Proxy whose
+		// provider is gone from the config. The instance is left running.
+		logf.FromContext(ctx).Info("skip-cleanup annotation set; abandoning the instance",
+			"providerID", p.Status.ProviderID, "provider", p.Spec.Provider)
+		controllerutil.RemoveFinalizer(p, crawlv1alpha1.FinalizerName)
+		return ctrl.Result{}, r.Update(ctx, p)
+	}
 	prov, ok := r.Providers[p.Spec.Provider]
 	if !ok {
 		return ctrl.Result{}, fmt.Errorf(
 			"provider %q is not configured; cannot clean up instance %s", p.Spec.Provider, p.Status.ProviderID)
 	}
 	_, err := prov.Get(ctx, p.Status.ProviderID)
-	if provider.Class(err) == provider.ErrNotFound {
+	switch {
+	case err == nil:
+	case provider.Class(err) == provider.ErrNotFound:
 		controllerutil.RemoveFinalizer(p, crawlv1alpha1.FinalizerName)
 		return ctrl.Result{}, r.Update(ctx, p)
-	}
+	case provider.Class(err) == provider.ErrPermanent:
-	if err != nil {
+		// Get will never answer for this instance — a host retired from the
+		// config, a providerID the provider can no longer resolve. Delete is
+		// idempotent and may still finish, so try it instead of polling a Get
+		// that cannot converge, and release the object when it does.
+		if delErr := prov.Delete(ctx, p.Status.ProviderID); delErr != nil {
+			return r.deletionFailure(errors.Join(err, delErr))
+		}
+		logf.FromContext(ctx).Info("instance unreachable but cleanup succeeded; removing the finalizer",
+			"providerID", p.Status.ProviderID, "error", err.Error())
+		controllerutil.RemoveFinalizer(p, crawlv1alpha1.FinalizerName)
+		return ctrl.Result{}, r.Update(ctx, p)
+	default:
 		return r.deletionFailure(err)
 	}
 	if err := prov.Delete(ctx, p.Status.ProviderID); err != nil {

internal/controller/reconcile_test.go

@@ -2,6 +2,7 @@ package controller
 
 import (
 	"context"
+	"slices"
 	"testing"
 	"time"
 
@@ -78,6 +79,10 @@ func notFoundErr() error {
 	return provider.Wrap(provider.ErrNotFound, "get", "stub", "some-id", nil)
 }
 
+func permanentErr() error {
+	return provider.Wrap(provider.ErrPermanent, "get", "stub", "some-id", nil)
+}
+
 func testScheme(t *testing.T) *runtime.Scheme {
 	t.Helper()
 	s := runtime.NewScheme()
@@ -125,6 +130,15 @@ func withSpecHashAnnotation(hash string) func(*crawlv1alpha1.Proxy) {
 	}
 }
 
+func withSkipCleanup() func(*crawlv1alpha1.Proxy) {
+	return func(p *crawlv1alpha1.Proxy) {
+		if p.Annotations == nil {
+			p.Annotations = map[string]string{}
+		}
+		p.Annotations[crawlv1alpha1.AnnotationSkipCleanup] = "true"
+	}
+}
+
 func deleting() func(*crawlv1alpha1.Proxy) {
 	return func(p *crawlv1alpha1.Proxy) {
 		now := metav1.Now()
@@ -375,6 +389,59 @@ func TestReconcile_actionTable(t *testing.T) {
 				assertProxyGone(t, r)
 			},
 		},
+		{
+			// A permanent Get (a retired KVM host, a providerID the provider
+			// cannot resolve) used to retry forever with the finalizer on.
+			name:       "deletion with an unreachable instance still deletes and releases",
+			proxy:      managedProxy(deleting(), withProviderID("stub-id-1")),
+			stub:       &stubProvider{getErr: permanentErr()},
+			wantResult: ctrl.Result{},
+			verify: func(t *testing.T, r *ProxyReconciler, stub *stubProvider) {
+				assertProxyGone(t, r)
+				if stub.deleteCalls != 1 {
+					t.Errorf("deleteCalls = %d, want 1 — Delete must be tried when Get cannot answer", stub.deleteCalls)
+				}
+			},
+		},
+		{
+			name:  "deletion keeps the finalizer when cleanup itself fails",
+			proxy: managedProxy(deleting(), withProviderID("stub-id-1")),
+			stub: &stubProvider{
+				getErr:    permanentErr(),
+				deleteErr: provider.Wrap(provider.ErrPermanent, "delete", "stub", "stub-id-1", nil),
+			},
+			wantResult: ctrl.Result{},
+			wantErr:    true,
+			verify: func(t *testing.T, r *ProxyReconciler, _ *stubProvider) {
+				// getProxy fails the test if the object is gone, which is what
+				// finalizer removal would have caused.
+				if p := getProxy(t, r); !slices.Contains(p.Finalizers, crawlv1alpha1.FinalizerName) {
+					t.Error("finalizer removed although cleanup failed; the instance would leak silently")
+				}
+			},
+		},
+		{
+			name:       "skip-cleanup annotation abandons the instance",
+			proxy:      managedProxy(deleting(), withProviderID("stub-id-1"), withSkipCleanup()),
+			stub:       &stubProvider{getInst: &provider.Instance{ID: "stub-id-1", State: provider.StateRunning}},
+			wantResult: ctrl.Result{},
+			verify: func(t *testing.T, r *ProxyReconciler, stub *stubProvider) {
+				assertProxyGone(t, r)
+				if stub.getCalls != 0 || stub.deleteCalls != 0 {
+					t.Errorf("getCalls/deleteCalls = %d/%d, want 0/0 — the provider must not be asked", stub.getCalls, stub.deleteCalls)
+				}
+			},
+		},
+		{
+			name: "skip-cleanup annotation releases a proxy whose provider is gone",
+			proxy: managedProxy(deleting(), withProviderID("stub-id-1"), withSkipCleanup(),
+				func(p *crawlv1alpha1.Proxy) { p.Spec.Provider = "retired" }),
+			stub:       &stubProvider{},
+			wantResult: ctrl.Result{},
+			verify: func(t *testing.T, r *ProxyReconciler, _ *stubProvider) {
+				assertProxyGone(t, r)
+			},
+		},
 		{
 			name:  "deletion deletes the instance and polls",
 			proxy: managedProxy(deleting(), withProviderID("stub-id-1")),

internal/provider/config_kvm.go

@@ -23,10 +23,17 @@ type KVMConfig struct {
 	// guest's cloud-init growpart expands the root filesystem into it.
 	DiskSizeGB int64 `json:"diskSizeGb,omitempty"`
 	// VendorData is optional cloud-init vendor-data placed on every seed next
-	// to the Proxy's own user-data. cloud-init drops any top-level key here
+	// to the Proxy's own user-data. Note that cloud-init drops any top-level
-	// that the user-data also defines. The qemu-guest-agent bootstrap does
+	// key of a vendor-data cloud-config that the user-data also defines.
-	// not live here: the provider merges it into each Proxy's cloud-config.
+	// When InstallGuestAgent is on, this document and the agent script are
+	// carried as separate parts of a MIME multipart vendor-data.
 	VendorData string `json:"vendorData,omitempty"`
+	// InstallGuestAgent adds a vendor-data script that installs and starts
+	// qemu-guest-agent, which is how the provider learns a VM's address
+	// before the host's ARP table has seen traffic from it. Default true.
+	// Turn it off for images that already ship the agent, or that cannot
+	// reach a package mirror; discovery then depends on ARP alone.
+	InstallGuestAgent *bool `json:"installGuestAgent,omitempty"`
 	// Images are named base images; placement.image names a key here (or is
 	// an http(s) URL directly). The provider downloads each image into a
 	// host's storage pool on first use and caches it there.
@@ -83,6 +90,14 @@ type KVMHost struct {
 
 var sha256Hex = regexp.MustCompile(`^[0-9a-fA-F]{64}$`)
 
+// kvmIdentifier constrains the config names that are pasted into
+// identifiers elsewhere: a host name goes into the providerID
+// hosts/<name>/domains/<vm>, which is split on "/" to parse, and an image
+// key becomes the file name of the cached base volume. A slash in either
+// produces an ID or a path that never resolves again, so the VM leaks with
+// nothing able to address it.
+var kvmIdentifier = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9_.-]*$`)
+
 func validateKVM(prefix string, c *KVMConfig) error {
 	if c == nil {
 		return fmt.Errorf("%s: kvm block is required", prefix)
@@ -98,6 +113,10 @@ func validateKVM(prefix string, c *KVMConfig) error {
 	}
 	for name, img := range c.Images {
 		p := fmt.Sprintf("%s: kvm.images[%q]", prefix, name)
+		if !kvmIdentifier.MatchString(name) {
+			return fmt.Errorf("%s: key must be letters, digits, dots, dashes or underscores"+
+				" — it becomes the cached volume name base-<key>.<format>", p)
+		}
 		u, err := url.Parse(img.URL)
 		if err != nil || (u.Scheme != "http" && u.Scheme != "https") || u.Host == "" {
 			return fmt.Errorf("%s.url must be an http(s) URL (got %q)", p, img.URL)
@@ -129,6 +148,10 @@ func validateKVM(prefix string, c *KVMConfig) error {
 		if h.Name == "" {
 			return fmt.Errorf("%s: name is required", p)
 		}
+		if !kvmIdentifier.MatchString(h.Name) {
+			return fmt.Errorf("%s: name %q must be letters, digits, dots, dashes or underscores"+
+				" — it becomes part of the providerID hosts/<name>/domains/<vm>", p, h.Name)
+		}
 		if seen[h.Name] {
 			return fmt.Errorf("%s: duplicate host name %q", p, h.Name)
 		}

internal/provider/config_kvm_test.go

@@ -129,6 +129,19 @@ hosts:
 			yaml:       minimal("sshKeyFile: /key\nknownHostsFile: /kh\nmachineTypes:\n  small: {vcpus: 1, memoryMiB: 1024}\nhosts:\n  - {name: nas1, address: a:22, user: virt, bridge: br0}\n  - {name: nas1, address: b:22, user: virt, bridge: br0}\n"),
 			wantErrSub: `duplicate host name "nas1"`,
 		},
+		{
+			// A slash would produce hosts/dc1/rack2/domains/<vm>, which
+			// parseProviderID can never split back, so the VM would be
+			// unreachable for Get, Delete and GC alike.
+			name:       "host name with a slash",
+			yaml:       minimal("sshKeyFile: /key\nknownHostsFile: /kh\nmachineTypes:\n  small: {vcpus: 1, memoryMiB: 1024}\nhosts:\n  - {name: dc1/rack2, address: a:22, user: virt, bridge: br0}\n"),
+			wantErrSub: `name "dc1/rack2" must be letters, digits, dots, dashes or underscores`,
+		},
+		{
+			name:       "image key with a slash",
+			yaml:       minimal("sshKeyFile: /key\nknownHostsFile: /kh\nimages:\n  ../etc/d: {url: https://x/y.qcow2}\n" + hostsAndTypes),
+			wantErrSub: `images["../etc/d"]: key must be letters, digits, dots, dashes or underscores`,
+		},
 		{
 			name:       "host missing address",
 			yaml:       minimal("sshKeyFile: /key\nknownHostsFile: /kh\nmachineTypes:\n  small: {vcpus: 1, memoryMiB: 1024}\nhosts:\n  - {name: nas1, user: virt, bridge: br0}\n"),

internal/provider/kvm/conn.go

@@ -5,6 +5,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"io/fs"
 	"net"
 	"os"
 	"sync"
@@ -180,14 +181,17 @@ func awaitOrCancel(ctx context.Context, fn func() error, abort func()) error {
 }
 
 // sshAuth holds the pieces of the SSH client config shared by every host:
-// the private key and the host-key policy, both loaded once at startup so a
+// the private key and the host-key policy. Both are loaded on the first
-// missing or unreadable file fails the operator fast rather than on the
+// connection rather than at startup — see lazyDialer.
-// first Create.
 type sshAuth struct {
 	signer  ssh.Signer
 	hostKey ssh.HostKeyCallback
 }
 
+// newSSHAuth loads the key material. A file that is not there yet stays
+// unclassified, so the provider treats it as transient and heals when the
+// Secret appears; content that will never parse is permanent, since
+// retrying cannot fix it.
 func newSSHAuth(cfg provider.KVMConfig) (*sshAuth, error) {
 	key, err := os.ReadFile(cfg.SSHKeyFile)
 	if err != nil {
@@ -195,18 +199,57 @@ func newSSHAuth(cfg provider.KVMConfig) (*sshAuth, error) {
 	}
 	signer, err := ssh.ParsePrivateKey(key)
 	if err != nil {
-		return nil, fmt.Errorf("parsing kvm.sshKeyFile: %w", err)
+		return nil, fmt.Errorf("%w: parsing kvm.sshKeyFile: %v", provider.ErrPermanent, err)
 	}
 	hostKey := ssh.InsecureIgnoreHostKey() //nolint:gosec // opt-in via config, documented as insecure
 	if !cfg.InsecureIgnoreHostKey {
 		hostKey, err = knownhosts.New(cfg.KnownHostsFile)
-		if err != nil {
+		if err != nil && errors.Is(err, fs.ErrNotExist) {
 			return nil, fmt.Errorf("loading kvm.knownHostsFile: %w", err)
 		}
+		if err != nil {
+			return nil, fmt.Errorf("%w: loading kvm.knownHostsFile: %v", provider.ErrPermanent, err)
+		}
 	}
 	return &sshAuth{signer: signer, hostKey: hostKey}, nil
 }
 
+// lazyAuth defers loading the SSH material to the first connection and
+// caches it once it works. The kvm-ssh Secret is an optional mount: the
+// manager has to start without it and keep every other provider running,
+// and a Secret created afterwards has to heal the KVM proxies on the next
+// reconcile rather than needing a restart.
+func lazyAuth(cfg provider.KVMConfig) func() (*sshAuth, error) {
+	var (
+		mu     sync.Mutex
+		cached *sshAuth
+	)
+	return func() (*sshAuth, error) {
+		mu.Lock()
+		defer mu.Unlock()
+		if cached != nil {
+			return cached, nil
+		}
+		auth, err := newSSHAuth(cfg)
+		if err != nil {
+			return nil, err
+		}
+		cached = auth
+		return auth, nil
+	}
+}
+
+func lazyDialer(cfg provider.KVMConfig) hostDialer {
+	auth := lazyAuth(cfg)
+	return func(ctx context.Context, host provider.KVMHost) (*hostClient, error) {
+		a, err := auth()
+		if err != nil {
+			return nil, err
+		}
+		return a.dialer()(ctx, host)
+	}
+}
+
 // dialer returns the production hostDialer: SSH to the host, forward its
 // libvirt unix socket through the connection, and open a libvirt session
 // over that.

internal/provider/kvm/conn_test.go

@@ -525,6 +525,97 @@ func TestNewSSHAuth_missingFiles(t *testing.T) {
 	}
 }
 
+// The kvm-ssh Secret is an optional mount, so the provider has to be
+// constructible without it and has to start working once it appears — the
+// alternative was a constructor error that crash-looped the whole manager.
+func TestLazyAuth_healsWhenTheKeyAppears(t *testing.T) {
+	t.Parallel()
+	f := newSSHFixture(t, true)
+	keyPEM, err := os.ReadFile(f.keyFile)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if err := os.Remove(f.keyFile); err != nil {
+		t.Fatal(err)
+	}
+	auth := lazyAuth(f.cfg)
+
+	if _, err := auth(); err == nil {
+		t.Fatal("loading with no key file succeeded, want an error")
+	} else if class := classify(err); class != provider.ErrTransient {
+		t.Errorf("class = %v, want ErrTransient so the proxy retries once the Secret lands", class)
+	}
+	if err := os.WriteFile(f.keyFile, keyPEM, 0o600); err != nil {
+		t.Fatal(err)
+	}
+	loaded, err := auth()
+	if err != nil {
+		t.Fatalf("loading after the key appeared: %v", err)
+	}
+	// Cached from here on: the file may vanish again (a Secret rotated out)
+	// without breaking connections that already work.
+	if err := os.Remove(f.keyFile); err != nil {
+		t.Fatal(err)
+	}
+	if again, err := auth(); err != nil || again != loaded {
+		t.Errorf("auth() = %v, %v; want the cached value reused", again, err)
+	}
+}
+
+func TestLazyAuth_unparseableKeyIsPermanent(t *testing.T) {
+	t.Parallel()
+	f := newSSHFixture(t, true)
+	if err := os.WriteFile(f.keyFile, []byte("not a key"), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	_, err := lazyAuth(f.cfg)()
+	if classify(err) != provider.ErrPermanent {
+		t.Errorf("class = %v, want ErrPermanent: retrying cannot fix a key that does not parse", classify(err))
+	}
+}
+
+// The dialer must authenticate with the material the lazy loader produced,
+// not a stale or empty one. The echo socket is not a libvirt server, so the
+// call gets as far as the libvirt handshake and no further; what matters is
+// that it is past SSH by then.
+func TestLazyDialer_authenticatesOverSSH(t *testing.T) {
+	t.Parallel()
+	f := newSSHFixture(t, true)
+	ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
+	defer cancel()
+
+	_, err := lazyDialer(f.cfg)(ctx, f.host)
+	if err == nil {
+		t.Fatal("dial to an echo socket succeeded, want a libvirt handshake error")
+	}
+	if strings.Contains(err.Error(), "sshKeyFile") || strings.Contains(err.Error(), "ssh handshake") {
+		t.Errorf("dial failed before libvirt: %v", err)
+	}
+	if paths := f.server.requestedPaths(); len(paths) != 1 || paths[0] != f.host.Socket {
+		t.Errorf("server saw forward requests %v, want [%s]", paths, f.host.Socket)
+	}
+}
+
+// New must not fail on missing key material, or one misconfigured provider
+// aborts registry.Build and the manager exits before any controller starts.
+func TestNew_missingSSHMaterialStillBuilds(t *testing.T) {
+	t.Parallel()
+	p, err := New(context.Background(), provider.ProviderConfig{
+		Name: "kvm-home", Type: "kvm",
+		KVM: &provider.KVMConfig{
+			SSHKeyFile:     filepath.Join(t.TempDir(), "absent"),
+			KnownHostsFile: filepath.Join(t.TempDir(), "absent"),
+			Hosts:          []provider.KVMHost{{Name: "nas1", Address: "nas1:22", User: "virt", Bridge: "br0"}},
+		},
+	})
+	if err != nil || p == nil {
+		t.Fatalf("New() = %v, %v; want a usable provider", p, err)
+	}
+	if _, err := p.Get(context.Background(), "hosts/nas1/domains/proxy-x"); provider.Class(err) != provider.ErrTransient {
+		t.Errorf("Get class = %v, want ErrTransient", provider.Class(err))
+	}
+}
+
 func TestKeepalive_closesClientWhenServerStalls(t *testing.T) {
 	t.Parallel()
 	f := newSSHFixture(t, false)

internal/provider/kvm/kvm.go

@@ -77,18 +77,22 @@ type Provider struct {
 }
 
 // New builds a Provider that reaches its hosts over SSH. The key and
-// known_hosts files are read here so a bad path fails at startup rather
+// known_hosts files are read on the first connection, not here: they come
-// than on the first Create. Satisfies registry.Constructor.
+// from an optional Secret mount, and a constructor that failed without it
-func New(_ context.Context, pc provider.ProviderConfig) (provider.Provider, error) {
+// would abort the whole provider set and crash-loop the manager, taking
+// the kubernetes and gcp providers down with it. Unreadable files are
+// still reported at startup, as a warning. Satisfies registry.Constructor.
+func New(ctx context.Context, pc provider.ProviderConfig) (provider.Provider, error) {
 	cfg := provider.KVMConfig{}
 	if pc.KVM != nil {
 		cfg = *pc.KVM
 	}
-	auth, err := newSSHAuth(cfg)
+	if _, err := newSSHAuth(cfg); err != nil {
-	if err != nil {
+		logf.FromContext(ctx).WithName("kvm").WithValues("provider", pc.Name).Info(
-		return nil, fmt.Errorf("kvm provider %q: %w", pc.Name, err)
+			"WARNING: kvm ssh material is not usable; proxies on this provider will fail until it is",
+			"error", err.Error())
 	}
-	return newWithDialer(pc, auth.dialer(), nil), nil
+	return newWithDialer(pc, lazyDialer(cfg), nil), nil
 }
 
 // newWithDialer is the test seam: the dialer decides what libvirtAPI each
@@ -246,7 +250,7 @@ func (p *Provider) writeSeed(log logr.Logger, api libvirtAPI, pool libvirt.Stora
 	if err := deleteIfExists(api, pool, seedName); err != nil {
 		return libvirt.StorageVol{}, err
 	}
-	userData, vendorData, err := withGuestAgent(userData, p.cfg.VendorData)
+	vendorData, err := buildVendorData(p.cfg.VendorData, p.cfg.InstallGuestAgent == nil || *p.cfg.InstallGuestAgent)
 	if err != nil {
 		return libvirt.StorageVol{}, fmt.Errorf("%w: %v", provider.ErrPermanent, err)
 	}
@@ -451,7 +455,8 @@ func mapState(s libvirt.DomainState, ip string) provider.InstanceState {
 // domain is not destroyed again, a missing domain is not undefined, and
 // the per-VM volumes are removed by name even when the domain is already
 // gone, so a crash between volume creation and define is cleaned up on the
-// next reconcile.
+// next reconcile. A providerID naming a host that is no longer configured
+// is a no-op success rather than an error.
 func (p *Provider) Delete(ctx context.Context, providerID string) error {
 	hostName, name, err := parseProviderID(providerID)
 	if err != nil {
@@ -459,7 +464,14 @@ func (p *Provider) Delete(ctx context.Context, providerID string) error {
 	}
 	host, err := p.host(hostName)
 	if err != nil {
-		return p.wrapErr("delete", providerID, err)
+		// The host is no longer configured, so this operator has no way to
+		// reach the domain and nothing left to clean. Same reasoning as
+		// removeVolumes over a missing pool: refusing would pin the Proxy's
+		// finalizer. Whatever is still running there is the admin's to remove,
+		// and orphan GC cannot see it either — see docs/kvm-provider.md.
+		p.logger(ctx).Info("host is not configured; delete has nothing to clean up",
+			"host", hostName, "providerID", providerID)
+		return nil
 	}
 	log := p.logger(ctx).WithValues("host", host.Name, "name", name)
 	err = p.hosts.call(ctx, log, host, func(api libvirtAPI) error {

internal/provider/kvm/kvm_test.go

@@ -188,8 +188,11 @@ func assertCreated(t *testing.T, api *fakeAPI) {
 		t.Errorf("seed = format %s, %d bytes, capacity %d; want raw with capacity == uploaded size", seed.format, len(seed.data), seed.capacity)
 	}
 	_, files := readSeed(t, seed.data)
-	if ud := files["user-data"]; !strings.Contains(ud, "squid") || !strings.Contains(ud, guestAgentPackage) {
+	if files["user-data"] != testCloudInit {
-		t.Errorf("seed user-data = %q, want the Proxy's cloud-config with the guest agent merged in", ud)
+		t.Errorf("seed user-data = %q, want the Proxy's cloud-init verbatim", files["user-data"])
+	}
+	if !strings.Contains(files["vendor-data"], "qemu-guest-agent") {
+		t.Errorf("seed vendor-data = %q, want the guest-agent script", files["vendor-data"])
 	}
 }
 
@@ -726,6 +729,29 @@ func TestDelete_missingPoolIsNotAnError(t *testing.T) {
 	}
 }
 
+// A host dropped from the config leaves the operator no way to reach the
+// domain, so Delete has nothing left to clean. Failing instead would pin
+// the Proxy's finalizer forever, since the reconciler's delete path cannot
+// make progress without a Delete that can succeed.
+func TestDelete_unconfiguredHostIsNotAnError(t *testing.T) {
+	t.Parallel()
+	api := readyHost()
+	managedDomain(t, api, testName, testUID, true, nil)
+	p := newTestProvider(hostFakes{"nas1": api}, "nas1")
+
+	if err := p.Delete(context.Background(), "hosts/nas9/domains/"+testName); err != nil {
+		t.Fatalf("Delete over an unconfigured host: %v", err)
+	}
+	if api.domain(testName) == nil {
+		t.Error("domain on the configured host was touched; only the named host's state may change")
+	}
+	// Get still refuses, so the live path cannot mistake a config typo for a
+	// missing VM and recreate it somewhere else.
+	if _, err := p.Get(context.Background(), "hosts/nas9/domains/"+testName); provider.Class(err) != provider.ErrPermanent {
+		t.Errorf("Get class = %v, want ErrPermanent", provider.Class(err))
+	}
+}
+
 func TestDelete_errors(t *testing.T) {
 	t.Parallel()
 	cases := []struct {
@@ -735,7 +761,6 @@ func TestDelete_errors(t *testing.T) {
 		class error
 	}{
 		{"malformed id", "nope", func(*testing.T) hostFakes { return hostFakes{"nas1": readyHost()} }, provider.ErrPermanent},
-		{"host not configured", "hosts/nas9/domains/x", func(*testing.T) hostFakes { return hostFakes{"nas1": readyHost()} }, provider.ErrPermanent},
 		{"host unreachable", "hosts/nas1/domains/x", func(*testing.T) hostFakes { return hostFakes{} }, provider.ErrTransient},
 		{
 			"undefine fails", "hosts/nas1/domains/" + testName,
@@ -884,12 +909,20 @@ func TestMapState(t *testing.T) {
 	}
 }
 
-func TestNew_badKeyPathFailsFast(t *testing.T) {
+// A bad key path no longer fails the constructor: registry.Build aborts the
+// whole provider set on a constructor error, so that took the manager down
+// with every other provider. It degrades per-proxy instead — see
+// TestNew_missingSSHMaterialStillBuilds and TestLazyAuth_healsWhenTheKeyAppears.
+func TestNew_badKeyPathDefersToFirstUse(t *testing.T) {
 	t.Parallel()
 	cfg := testConfig("nas1")
 	cfg.KVM.SSHKeyFile = t.TempDir() + "/missing"
-	if _, err := New(context.Background(), cfg); err == nil {
+	p, err := New(context.Background(), cfg)
-		t.Fatal("New succeeded with a missing key file")
+	if err != nil {
+		t.Fatalf("New() error = %v, want a provider that fails only when used", err)
+	}
+	if err := p.Delete(context.Background(), "hosts/nas1/domains/proxy-x"); provider.Class(err) != provider.ErrTransient {
+		t.Errorf("Delete class = %v, want ErrTransient", provider.Class(err))
 	}
 }
 

internal/provider/kvm/seed.go

@@ -3,110 +3,104 @@ package kvm
 import (
 	"bytes"
 	"fmt"
-	"reflect"
+	"io"
+	"mime/multipart"
+	"net/textproto"
 	"strings"
 
 	"github.com/kdomanski/iso9660"
-	"sigs.k8s.io/yaml"
 )
 
 // seedLabel is the volume label cloud-init's NoCloud datasource looks for.
 const seedLabel = "cidata"
 
-const (
+// guestAgentScript installs and starts the QEMU guest agent, which is how
-	cloudConfigHeader = "#cloud-config"
+// the provider learns a VM's address before the host's ARP table has seen
-	guestAgentPackage = "qemu-guest-agent"
+// any traffic from it.
-)
+//
-
+// It ships as vendor-data, and as a *script* rather than a cloud-config,
-// guestAgentEnable is the runcmd entry that starts the agent once it is
+// because that is the only shape cloud-init cannot drop. Vendor-data
-// installed, so the provider can read the VM's IP before ARP sees traffic.
+// cloud-config is merged under the Proxy's own user-data with mergers that
-var guestAgentEnable = []any{"systemctl", "enable", "--now", guestAgentPackage}
+// keep the user-data's value for every key both documents define and never
-
+// append lists, so a Proxy carrying its own packages: silently lost the
-// guestAgentVendorData is the same bootstrap as a standalone cloud-config.
+// agent. The strategy cannot be overridden from either document: merge_how
-// It is only used as vendor-data when the Proxy's user-data is not a
+// in user-data governs only the first source, and merge_how in vendor-data
-// cloud-config document (a shell script, a MIME archive, an #include), i.e.
+// is consumed and discarded before the document is stored. A vendor-data
-// when there are no top-level keys for it to collide with.
+// script bypasses all of it — cloud-init gives vendor-data its own handler
-const guestAgentVendorData = cloudConfigHeader + `
+// chain, writes scripts to a separate directory and runs them from
-packages:
+// cc_scripts_vendor, after the user's packages are installed and before the
-  - qemu-guest-agent
+// user's runcmd.
-runcmd:
+const guestAgentScript = `#!/bin/sh
-  - [systemctl, enable, --now, qemu-guest-agent]
+# Added by egress-proxies-operator so the hypervisor can report this VM's
+# address. Set installGuestAgent: false on the provider to leave it out.
+set -e
+if ! command -v qemu-ga >/dev/null 2>&1 && [ ! -x /usr/sbin/qemu-ga ]; then
+	if command -v apt-get >/dev/null 2>&1; then
+		apt-get update
+		DEBIAN_FRONTEND=noninteractive apt-get install -y qemu-guest-agent
+	elif command -v dnf >/dev/null 2>&1; then
+		dnf install -y qemu-guest-agent
+	elif command -v yum >/dev/null 2>&1; then
+		yum install -y qemu-guest-agent
+	elif command -v zypper >/dev/null 2>&1; then
+		zypper --non-interactive install qemu-guest-agent
+	elif command -v apk >/dev/null 2>&1; then
+		apk add qemu-guest-agent
+	else
+		echo "no known package manager; install qemu-guest-agent in the image" >&2
+		exit 1
+	fi
+fi
+if command -v systemctl >/dev/null 2>&1; then
+	systemctl enable --now qemu-guest-agent
+elif command -v rc-update >/dev/null 2>&1; then
+	rc-update add qemu-guest-agent default
+	rc-service qemu-guest-agent start
+fi
 `
 
-// withGuestAgent decides where the guest-agent bootstrap goes and returns
+// buildVendorData renders the seed's vendor-data. With nothing configured
-// the user-data and vendor-data to put on the seed.
+// it is the guest-agent script alone; with both, a MIME multipart carrying
-//
+// the configured document and the script as separate parts, since
-// It cannot simply ride along as vendor-data: cloud-init merges vendor-data
+// vendor-data is a single file and the two must not be concatenated.
-// under user-data with its default mergers, which keep the user-data's
+func buildVendorData(configured string, installAgent bool) (string, error) {
-// value for any key both define and never append lists. A Proxy whose
+	switch {
-// cloud-config has its own `packages:` (most do) would silently lose the
+	case !installAgent:
-// agent and sit in Provisioning forever. So for a cloud-config user-data
+		return configured, nil
-// the bootstrap is merged into the document itself: `qemu-guest-agent`
+	case strings.TrimSpace(configured) == "":
-// appended to `packages`, the enable command to `runcmd`, both skipped
+		return guestAgentScript, nil
-// when already present. Anything the operator cannot parse as a
-// cloud-config mapping is an error — cloud-init would reject it too, and a
-// VM that boots without its config is harder to diagnose than a Proxy that
-// fails fast.
-func withGuestAgent(userData, vendorData string) (string, string, error) {
-	if !isCloudConfig(userData) {
-		if vendorData == "" {
-			vendorData = guestAgentVendorData
-		}
-		return userData, vendorData, nil
-	}
-	var cfg map[string]any
-	if err := yaml.Unmarshal([]byte(userData), &cfg); err != nil {
-		return "", "", fmt.Errorf("parsing cloud-config user-data: %w", err)
-	}
-	if cfg == nil {
-		cfg = map[string]any{}
-	}
-	pkgs, err := appendUnique(cfg, "packages", guestAgentPackage)
-	if err != nil {
-		return "", "", err
-	}
-	cfg["packages"] = pkgs
-	cmds, err := appendUnique(cfg, "runcmd", guestAgentEnable)
-	if err != nil {
-		return "", "", err
-	}
-	cfg["runcmd"] = cmds
-	out, err := yaml.Marshal(cfg)
-	if err != nil {
-		return "", "", fmt.Errorf("rendering cloud-config user-data: %w", err)
-	}
-	return cloudConfigHeader + "\n" + string(out), vendorData, nil
-}
-
-// isCloudConfig reports whether userData is a cloud-config document by
-// cloud-init's own rule: the first line is the #cloud-config header. Empty
-// user-data counts, so a Proxy without cloud-init still gets the agent.
-func isCloudConfig(userData string) bool {
-	if strings.TrimSpace(userData) == "" {
-		return true
-	}
-	first, _, _ := strings.Cut(userData, "\n")
-	return strings.TrimSpace(first) == cloudConfigHeader
-}
-
-// appendUnique returns cfg[key] with item appended unless an equal entry
-// is already there. A missing key is an empty list; anything but a list is
-// an error, since cloud-init expects one there as well.
-func appendUnique(cfg map[string]any, key string, item any) ([]any, error) {
-	var items []any
-	switch v := cfg[key].(type) {
-	case nil:
-	case []any:
-		items = v
 	default:
-		return nil, fmt.Errorf("cloud-config %s: want a list, got %T", key, v)
+		return multipartVendorData(configured, guestAgentScript)
 	}
-	for _, it := range items {
+}
-		if reflect.DeepEqual(it, item) {
+
-			return items, nil
+// multipartVendorData packs several documents into one vendor-data blob.
+// cloud-init decides a payload is multipart by finding "mime-version:"
+// within its first 4096 bytes, so those headers lead. The configured
+// document goes in as text/plain on purpose: cloud-init re-sniffs that type
+// from the content's own header line, which keeps this code out of the
+// business of classifying what the user wrote.
+func multipartVendorData(configured, script string) (string, error) {
+	var body bytes.Buffer
+	w := multipart.NewWriter(&body)
+	parts := []struct{ contentType, content string }{
+		{"text/plain", configured},
+		{"text/x-shellscript", script},
+	}
+	for _, p := range parts {
+		part, err := w.CreatePart(textproto.MIMEHeader{"Content-Type": {p.contentType}})
+		if err != nil {
+			return "", fmt.Errorf("building vendor-data part: %w", err)
+		}
+		if _, err := io.WriteString(part, p.content); err != nil {
+			return "", fmt.Errorf("writing vendor-data part: %w", err)
 		}
 	}
-	return append(items, item), nil
+	if err := w.Close(); err != nil {
+		return "", fmt.Errorf("closing vendor-data: %w", err)
+	}
+	return fmt.Sprintf("MIME-Version: 1.0\r\nContent-Type: multipart/mixed; boundary=%q\r\n\r\n%s",
+		w.Boundary(), body.String()), nil
 }
 
 // buildSeedISO renders a NoCloud seed: user-data and vendor-data verbatim,

internal/provider/kvm/seed_test.go

@@ -4,12 +4,12 @@ import (
 	"bytes"
 	"io"
 	"maps"
-	"reflect"
+	"mime"
+	"mime/multipart"
 	"strings"
 	"testing"
 
 	"github.com/kdomanski/iso9660"
-	"sigs.k8s.io/yaml"
 )
 
 func readSeed(t *testing.T, iso []byte) (label string, files map[string]string) {
@@ -44,7 +44,7 @@ func readSeed(t *testing.T, iso []byte) (label string, files map[string]string)
 func TestBuildSeedISO_contents(t *testing.T) {
 	t.Parallel()
 	userData := "#cloud-config\npackages: [squid]\n"
-	iso, err := buildSeedISO("proxy-abc123def456ghij", userData, guestAgentVendorData)
+	iso, err := buildSeedISO("proxy-abc123def456ghij", userData, guestAgentScript)
 	if err != nil {
 		t.Fatalf("buildSeedISO() error = %v", err)
 	}
@@ -58,7 +58,7 @@ func TestBuildSeedISO_contents(t *testing.T) {
 	if files["user-data"] != userData {
 		t.Errorf("user-data = %q, want verbatim cloud-init", files["user-data"])
 	}
-	if files["vendor-data"] != guestAgentVendorData {
+	if files["vendor-data"] != guestAgentScript {
 		t.Errorf("vendor-data = %q", files["vendor-data"])
 	}
 	if got, want := files["meta-data"], "instance-id: proxy-abc123def456ghij\nlocal-hostname: proxy-abc123def456ghij\n"; got != want {
@@ -91,7 +91,7 @@ func TestBuildSeedISO_deterministicContent(t *testing.T) {
 
 func TestBuildSeedISO_emptyUserData(t *testing.T) {
 	t.Parallel()
-	iso, err := buildSeedISO("proxy-x", "", guestAgentVendorData)
+	iso, err := buildSeedISO("proxy-x", "", guestAgentScript)
 	if err != nil {
 		t.Fatalf("buildSeedISO() error = %v", err)
 	}
@@ -101,149 +101,133 @@ func TestBuildSeedISO_emptyUserData(t *testing.T) {
 	}
 }
 
-// parseCloudConfig is what cloud-init sees: the document under the header.
+// The whole point of shipping the agent as vendor-data: whatever the Proxy
-func parseCloudConfig(t *testing.T, doc string) map[string]any {
+// wrote reaches the guest untouched, in any of cloud-init's formats. The
-	t.Helper()
+// previous implementation parsed and re-rendered cloud-config user-data,
-	if first, _, _ := strings.Cut(doc, "\n"); first != cloudConfigHeader {
+// which dropped comments, reordered keys and re-typed unquoted scalars
-		t.Fatalf("first line = %q, want %q", first, cloudConfigHeader)
+// (version: 1.10 arrived as 1.1).
-	}
+func TestBuildSeedISO_userDataIsNeverRewritten(t *testing.T) {
-	var cfg map[string]any
-	if err := yaml.Unmarshal([]byte(doc), &cfg); err != nil {
-		t.Fatalf("merged user-data is not YAML: %v\n%s", err, doc)
-	}
-	return cfg
-}
-
-func TestWithGuestAgent_mergesIntoCloudConfig(t *testing.T) {
-	t.Parallel()
-	agentCmd := []any{"systemctl", "enable", "--now", "qemu-guest-agent"}
-	tests := []struct {
-		name         string
-		userData     string
-		wantPackages []any
-		wantRuncmd   []any
-	}{
-		{
-			name:         "empty document",
-			userData:     "#cloud-config\n",
-			wantPackages: []any{"qemu-guest-agent"},
-			wantRuncmd:   []any{agentCmd},
-		},
-		{
-			name:         "no user-data at all",
-			userData:     "",
-			wantPackages: []any{"qemu-guest-agent"},
-			wantRuncmd:   []any{agentCmd},
-		},
-		{
-			// The case that motivated this: cloud-init's default merger keeps
-			// the user-data's lists and drops vendor-data's, so the agent must
-			// be appended to the user's own packages and runcmd.
-			name: "user packages and runcmd are kept and extended",
-			userData: "#cloud-config\npackages:\n  - squid\nruncmd:\n  - systemctl restart squid\n" +
-				"write_files:\n  - path: /etc/squid/conf.d/x.conf\n    content: |\n      http_port 3128\n      via off\n",
-			wantPackages: []any{"squid", "qemu-guest-agent"},
-			wantRuncmd:   []any{"systemctl restart squid", agentCmd},
-		},
-		{
-			name:         "agent already present is not duplicated",
-			userData:     "#cloud-config\npackages: [qemu-guest-agent]\nruncmd:\n  - [systemctl, enable, --now, qemu-guest-agent]\n",
-			wantPackages: []any{"qemu-guest-agent"},
-			wantRuncmd:   []any{agentCmd},
-		},
-		{
-			name:         "header with trailing whitespace",
-			userData:     "#cloud-config   \r\npackages: [curl]\n",
-			wantPackages: []any{"curl", "qemu-guest-agent"},
-			wantRuncmd:   []any{agentCmd},
-		},
-	}
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			t.Parallel()
-			user, vendor, err := withGuestAgent(tc.userData, "")
-			if err != nil {
-				t.Fatalf("withGuestAgent() error = %v", err)
-			}
-			if vendor != "" {
-				t.Errorf("vendor-data = %q, want none when the agent is merged into the user-data", vendor)
-			}
-			cfg := parseCloudConfig(t, user)
-			if !reflect.DeepEqual(cfg["packages"], tc.wantPackages) {
-				t.Errorf("packages = %#v, want %#v", cfg["packages"], tc.wantPackages)
-			}
-			if !reflect.DeepEqual(cfg["runcmd"], tc.wantRuncmd) {
-				t.Errorf("runcmd = %#v, want %#v", cfg["runcmd"], tc.wantRuncmd)
-			}
-		})
-	}
-}
-
-func TestWithGuestAgent_keepsOtherKeysAndVendorData(t *testing.T) {
-	t.Parallel()
-	userData := "#cloud-config\nhostname: px\nwrite_files:\n  - path: /etc/x\n    content: |\n      line one\n      line two\n    permissions: '0644'\nbootcmd: [echo hi]\n"
-	user, vendor, err := withGuestAgent(userData, "#cloud-config\nntp: {enabled: true}\n")
-	if err != nil {
-		t.Fatalf("withGuestAgent() error = %v", err)
-	}
-	if vendor != "#cloud-config\nntp: {enabled: true}\n" {
-		t.Errorf("vendor-data = %q, want the configured one verbatim", vendor)
-	}
-	got := parseCloudConfig(t, user)
-	delete(got, "packages")
-	delete(got, "runcmd")
-	want := parseCloudConfig(t, userData)
-	if !reflect.DeepEqual(got, want) {
-		t.Errorf("merged user-data changed the user's keys:\n got %#v\nwant %#v", got, want)
-	}
-}
-
-func TestWithGuestAgent_nonCloudConfigFallsBackToVendorData(t *testing.T) {
-	t.Parallel()
-	tests := []struct {
-		name       string
-		userData   string
-		vendorData string
-		wantVendor string
-	}{
-		{"shell script, no vendor-data", "#!/bin/sh\napt-get install -y squid\n", "", guestAgentVendorData},
-		{"include, no vendor-data", "#include\nhttps://example/x\n", "", guestAgentVendorData},
-		{"shell script, own vendor-data wins", "#!/bin/sh\n", "#cloud-config\nntp: {}\n", "#cloud-config\nntp: {}\n"},
-	}
-	for _, tc := range tests {
-		t.Run(tc.name, func(t *testing.T) {
-			t.Parallel()
-			user, vendor, err := withGuestAgent(tc.userData, tc.vendorData)
-			if err != nil {
-				t.Fatalf("withGuestAgent() error = %v", err)
-			}
-			if user != tc.userData {
-				t.Errorf("user-data = %q, want it untouched", user)
-			}
-			if vendor != tc.wantVendor {
-				t.Errorf("vendor-data = %q, want %q", vendor, tc.wantVendor)
-			}
-		})
-	}
-}
-
-func TestWithGuestAgent_rejectsBrokenCloudConfig(t *testing.T) {
 	t.Parallel()
 	tests := []struct {
 		name     string
 		userData string
 	}{
-		{"not yaml", "#cloud-config\npackages: [squid\n"},
+		{"cloud-config with its own packages", "#cloud-config\n# keep me\npackages:\n  - squid\nversion: 1.10\nchpasswd: {list: 'u:0123456789'}\n"},
-		{"not a mapping", "#cloud-config\n- squid\n"},
+		{"jinja template", "## template: jinja\n#cloud-config\nhostname: {{ v1.local_hostname }}\n"},
-		{"packages is not a list", "#cloud-config\npackages: squid\n"},
+		{"shell script", "#!/bin/sh\napt-get install -y squid\n"},
-		{"runcmd is not a list", "#cloud-config\nruncmd:\n  a: b\n"},
+		{"include", "#include\nhttps://example.invalid/x\n"},
+		{"mime multipart", "MIME-Version: 1.0\r\nContent-Type: multipart/mixed; boundary=\"x\"\r\n\r\n--x\r\nContent-Type: text/cloud-config\r\n\r\n#cloud-config\npackages: [squid]\n\r\n--x--\r\n"},
+		{"gzip payload", "\x1f\x8b\x08\x00\x00\x00\x00\x00\x00\x03binary\x00bytes"},
+		{"leading blank line", "\n\n#cloud-config\npackages: [squid]\n"},
 	}
 	for _, tc := range tests {
 		t.Run(tc.name, func(t *testing.T) {
 			t.Parallel()
-			if _, _, err := withGuestAgent(tc.userData, ""); err == nil {
+			vendor, err := buildVendorData("", true)
-				t.Errorf("withGuestAgent() = nil error, want one for %q", tc.userData)
+			if err != nil {
+				t.Fatalf("buildVendorData() error = %v", err)
+			}
+			iso, err := buildSeedISO("proxy-x", tc.userData, vendor)
+			if err != nil {
+				t.Fatalf("buildSeedISO() error = %v", err)
+			}
+			_, files := readSeed(t, iso)
+			if files["user-data"] != tc.userData {
+				t.Errorf("user-data was rewritten:\n got %q\nwant %q", files["user-data"], tc.userData)
+			}
+			if !strings.Contains(files["vendor-data"], "qemu-guest-agent") {
+				t.Errorf("vendor-data = %q, want the agent script", files["vendor-data"])
 			}
 		})
 	}
 }
+
+func TestBuildVendorData_agentScriptAlone(t *testing.T) {
+	t.Parallel()
+	got, err := buildVendorData("", true)
+	if err != nil {
+		t.Fatalf("buildVendorData() error = %v", err)
+	}
+	if got != guestAgentScript {
+		t.Errorf("vendor-data = %q, want the bare script", got)
+	}
+	if !strings.HasPrefix(got, "#!") {
+		t.Error("vendor-data must start with a shebang or cloud-init treats it as something other than a script")
+	}
+}
+
+func TestBuildVendorData_optOutKeepsConfiguredOnly(t *testing.T) {
+	t.Parallel()
+	configured := "#cloud-config\nntp: {enabled: true}\n"
+	tests := []struct {
+		name       string
+		configured string
+		want       string
+	}{
+		{"nothing configured", "", ""},
+		{"configured document passes through", configured, configured},
+	}
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+			got, err := buildVendorData(tc.configured, false)
+			if err != nil {
+				t.Fatalf("buildVendorData() error = %v", err)
+			}
+			if got != tc.want {
+				t.Errorf("vendor-data = %q, want %q", got, tc.want)
+			}
+			if strings.Contains(got, "qemu-guest-agent") {
+				t.Error("agent script present although installGuestAgent is off")
+			}
+		})
+	}
+}
+
+// Both documents have to survive in one file, and cloud-init only looks for
+// the multipart marker in the first 4096 bytes.
+func TestBuildVendorData_multipartCarriesBoth(t *testing.T) {
+	t.Parallel()
+	configured := "#cloud-config\nntp:\n  enabled: true\n"
+	got, err := buildVendorData(configured, true)
+	if err != nil {
+		t.Fatalf("buildVendorData() error = %v", err)
+	}
+	head := got
+	if len(head) > 4096 {
+		head = head[:4096]
+	}
+	if !strings.Contains(strings.ToLower(head), "mime-version:") {
+		t.Fatalf("no mime-version: in the first 4096 bytes; cloud-init would treat this as a single document:\n%s", head)
+	}
+	_, params, err := mime.ParseMediaType(strings.SplitN(got, "\r\n\r\n", 2)[0][strings.Index(got, "Content-Type:")+len("Content-Type: "):])
+	if err != nil {
+		t.Fatalf("parsing the multipart content type: %v", err)
+	}
+	body := strings.SplitN(got, "\r\n\r\n", 2)[1]
+	r := multipart.NewReader(strings.NewReader(body), params["boundary"])
+	var bodies, types []string
+	for {
+		part, err := r.NextPart()
+		if err == io.EOF {
+			break
+		}
+		if err != nil {
+			t.Fatalf("reading part: %v", err)
+		}
+		content, err := io.ReadAll(part)
+		if err != nil {
+			t.Fatalf("reading part body: %v", err)
+		}
+		bodies = append(bodies, string(content))
+		types = append(types, part.Header.Get("Content-Type"))
+	}
+	if len(bodies) != 2 || bodies[0] != configured || bodies[1] != guestAgentScript {
+		t.Fatalf("parts = %q, want the configured document then the script", bodies)
+	}
+	// text/plain makes cloud-init re-sniff the user's document from its own
+	// header line, so this code never has to classify it.
+	if types[0] != "text/plain" || types[1] != "text/x-shellscript" {
+		t.Errorf("part types = %v, want [text/plain text/x-shellscript]", types)
+	}
+}