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

# Execution: KVM/libvirt provider

Plan: [2026-09-04-1404-kvm-provider.md](../plans/2026-09-04-1404-kvm-provider.md)

- [x] Step 0 — Save plan + branch `feat/kvm-provider`
- [x] Step 1 — Config (`KVMConfig` & friends, `validate()` case, tests)
- [x] Step 2 — Pure builders (`domainxml.go`, `volume.go`, `seed.go`, `id.go`, `metadata.go`)
- [x] Step 3 — Transport + errors (`conn.go`, `errors.go`)
- [x] Step 3b — Image cache (`image.go`)
- [x] Step 4 — Provider methods (`kvm.go`) over a fake API
- [x] Step 5 — Wiring, manifests, samples, docs, CHANGELOG
- [x] Step 6 — Integration test against a real hypervisor (written and compiled; first real run is part of Step 7)
- [ ] Step 7 — Manual validation + MR

## Step 0 — Save plan + branch

Plan committed alone on `feat/proxy-operator`, then branched:

```bash
git checkout -b feat/kvm-provider
```

## Step 1 — Config

The new structs (`KVMConfig`, `KVMHost`, `KVMMachineType`, `KVMImage`) and
`validateKVM` went into their own file, `internal/provider/config_kvm.go`,
rather than being appended to `config.go` as the plan said: the KVM block has
far more fields and rules than the other two providers, and keeping it
separate leaves `config.go` readable. Tests likewise live in
`config_kvm_test.go` instead of extending the table in `config_test.go`; the
invalid-config table covers every required field, duplicate host names, image
URL scheme, sha256 shape, and the disk format enum.

While adding the `kvm` case to `validate()`, the pairwise "type is X but a Y
block is set" checks (which would have needed six branches with three types)
were replaced by a `setBlocks()` helper that lists the non-nil blocks and
rejects any that don't match `type`. The error strings are unchanged, so the
existing GCP/kubernetes tests still pass.

Dependencies were fetched up front so the API signatures in the plan could be
checked with `go doc`; they stay `// indirect` in `go.mod` until Step 2 imports
them:

```bash
go get github.com/digitalocean/go-libvirt@latest github.com/kdomanski/iso9660@v0.4.0
# go-libvirt resolved to v0.0.0-20260814190004-1a83157e1858 (no tagged release)
# side effect: golang.org/x/crypto 0.54.0 → 0.56.0, x/mod, x/text, x/tools bumped
```

Worth noting: `go doc` showed that go-libvirt's `New(net.Conn)` is deprecated
in favour of `NewWithDialer(socket.Dialer)`, where `Dialer` is a one-method
`Dial() (net.Conn, error)` interface. Step 3 should wrap the SSH client's unix
forward in that interface instead of using `New`; the plan's transport design
is otherwise unaffected.

## Step 2 — Pure builders

`internal/provider/kvm/` now has the five builders from the plan plus
`kvm.go` holding the package doc and `withDefaults` (disk size 10 GiB, the
default vendor-data, `/var/run/libvirt/libvirt-sock`, pool `default`, image
format `qcow2`). Everything is a pure function over `encoding/xml` structs or
strings, unit-tested by unmarshalling the output back and checking fields.

Two design choices differ from the plan text:

- **Domain disks reference pool volumes, not host paths.** The plan had
  `buildDomainXML` take the overlay and seed paths (needing two
  `StorageVolGetPath` round-trips per Create). The domain XML now uses
  `<disk type="volume"><source pool=... volume=.../>`, so only the *base*
  image path is ever looked up (the overlay's `<backingStore>` needs a real
  path). Fewer RPCs and the builder's inputs are all names.
  **Reverted in Step 7**: volume-referenced disks cannot start on an
  AppArmor host, see there. The domain XML is back to `type="file"` with
  the two `StorageVolGetPath` calls the plan had.
- **Capacities are always `unit="bytes"`.** libvirt's `G` means GiB but
  `GB` means 10^9; spelling the byte count out removes the ambiguity from
  both the overlay and the seed/base volumes.

The metadata element is written with a default namespace
(`<instance xmlns="…/kvm">`) because `encoding/xml` cannot emit prefixes on
marshal. libvirt may hand it back prefixed; `parseInstanceMetadata` is
tested against both spellings since `encoding/xml` matches on the namespace
URI. Whether libvirt accepts the default-namespace form on `DomainDefineXML`
is confirmed in Step 6 against a real host, not here. (Step 7: it does,
but `DomainGetMetadata` returns the element with no namespace at all; the
parser now matches on local name only.)

Checked before relying on `github.com/kdomanski/iso9660` for the seed: its
filename mangling lowercases and keeps `-`, so `user-data`/`meta-data`/
`vendor-data` come out as `user-data;1` etc. in the directory record, which
the Linux isofs driver (and therefore cloud-init's NoCloud mount) presents as
`user-data`. The seed test opens the ISO with the same library's reader and
asserts label `cidata`, all three files, and byte-identical output for
identical inputs. `NewWriter()` stages files in a temp directory, so
`Cleanup()` is deferred.

`go mod tidy` ran as part of this step; it promoted `kdomanski/iso9660` to a
direct dependency and dropped `go-libvirt` again because nothing imports it
yet — Step 3 brings it back.

Worth noting: `withDefaults` copies the `Images` map and `Hosts` slice
instead of mutating the config in place, so the registry's shared
`ProviderConfig` stays untouched. The default vendor-data installs
`qemu-guest-agent` via `packages:` + `runcmd`, which means the first boot
needs network access to the distro mirrors; a Proxy on an isolated bridge
would still work (ARP discovery), just without the agent.

## Step 3 — Transport + errors

`conn.go` holds the `libvirtAPI` seam (17 methods, signatures as in the
plan), a per-host connection pool, and the SSH dialer; `errors.go` holds
`classify`, `isLibvirtError`, `hasCode` and `logAPIError`. Both are
unit-tested; the whole package still runs in about a second under `-race`.

The one thing the plan did not anticipate: **go-libvirt RPCs take no
context and never time out**. A hung hypervisor would pin a reconcile worker
forever. So the pool exposes `call(ctx, host, fn)` rather than handing out
raw clients: `fn` runs in a goroutine, and if `ctx` ends first the pool
closes the SSH connection out from under it (which makes the blocked RPC
return) and reports `ctx.Err()`. The same helper (`awaitOrCancel`) bounds
`lv.Connect()` during dial. Two more liveness measures for the same reason:
the SSH handshake gets a deadline (ctx's, or 30 s), and each connection runs
an OpenSSH `keepalive@openssh.com` loop every 15 s that closes the client
when a request errors or goes unanswered for one interval.

Connection-drop policy in `call`: any error that is *not* a `libvirt.Error`
(so io.EOF, "connection closed", SSH failures) drops the cached connection
and the next call redials; a daemon-reported `libvirt.Error` keeps it. Dials
are serialised per host (concurrent Creates on one host share a dial) but
hosts don't block each other.

As Step 1 noted, the dialer uses `libvirt.NewWithDialer` with a
`socket.Dialer` that wraps `ssh.Client.Dial("unix", socket)` rather than the
deprecated `New(net.Conn)`.

Tests: the pool is exercised through a fake dialer (reuse, redial after
`Disconnected`, drop on transport error, keep on libvirt error, cancel
releases a blocked call, concurrent callers share one dial, hosts
independent). The SSH layer is tested against an in-process
`x/crypto/ssh` server that serves `direct-streamlocal@openssh.com` channels
by dialing the requested unix path (an echo socket standing in for
`libvirt-sock`), with keys generated per test and a real `known_hosts` file
written via `knownhosts.Line`. Covered: forward round-trip and the socket
path the server saw, unknown host key rejected with `knownhosts.KeyError`,
`insecureIgnoreHostKey`, wrong client key, cancelled ctx, missing key or
known_hosts file, and keepalive both closing a stalled client and leaving a
healthy one alone. The libvirt handshake (`Connect()`) itself is *not*
covered here — faking libvirt's XDR RPC just for that is not worth it; the
Step 6 integration test covers it against a real host.

`go mod tidy` promoted `go-libvirt` and `golang.org/x/crypto` to direct
dependencies.

Worth noting: `classify` maps `ErrOperationInvalid` (e.g. destroying a
domain that is already off) to Transient; Delete in Step 4 must check that
code itself via `hasCode` and treat it as success rather than relying on
the classifier. Nothing maps to `ErrQuotaExceeded` — libvirt has no quota
concept; an out-of-space pool surfaces as a Transient `ErrInternalError`.

## Step 3b — Image cache

`image.go` follows the plan closely: `resolveImage` maps `placement.image`
to a configured key or a direct http(s) URL (else Permanent);
`imageCache.ensure` checks base volume + marker (+ marker content when a
checksum is configured), tears down and re-fetches when anything is off,
streams the HTTP body through `StorageVolUpload` with a `sha256` tee,
refreshes the pool, then writes the marker last. A per-`(host, volume)`
mutex serialises concurrent callers; the pool itself is the only cache
state.

Errors from this file are pre-tagged with the taxonomy sentinel via
`fmt.Errorf("%w: …", provider.ErrPermanent, …)` and `classify` now honours
such tags before looking at libvirt codes. That keeps one `wrapErr` path in
the provider instead of special-casing image errors.

`fake_test.go` landed with this step rather than Step 4: an in-memory
`libvirtAPI` with pools, volumes (content, format, capacity, backing path),
domains (state, metadata, per-source interface addresses), per-method error
injection (`fail` always / `failOnce`) and an `uploadHook` to break a stream
mid-way. It models the libvirt error codes the provider branches on
(`ErrStorageVolExist`, `ErrNoStorageVol`, `ErrNoDomain`,
`ErrOperationInvalid`, `ErrNoDomainMetadata`, `ErrAgentUnresponsive`).

Tests cover every case in the plan plus: no checksum configured (marker is
still written with the computed digest; second call needs no marker read),
403/500 alongside 404/503, missing pool → Permanent, cancelled ctx →
Transient with nothing created, and different hosts fetching independently.

One test-harness lesson worth keeping: `httptest`'s server drops
`Content-Length` and switches to chunked encoding for bodies larger than
its 2 KiB write buffer, so the fake mirror sets the header explicitly.
Real distro mirrors send it; a server that doesn't is reported as
Permanent with a message naming the header.

Worth noting: the base volume is created with `capacity = Content-Length`
and the *image's* format (`qcow2`), and the raw bytes are uploaded over it.
That relies on libvirt's upload writing the file verbatim (which is how
`virsh vol-upload` populates volumes); the following `StoragePoolRefresh`
makes libvirt re-read the real virtual size from the qcow2 header. Step 6
confirms this on a real host. Also, the whole download runs inside one
`pool.call`, so cancellation aborts the HTTP body (ctx-bound) and the
upload stream fails; the half-written base volume is deleted on the spot
and, failing that, on the next attempt via the missing marker.

## Step 4 — Provider methods

`kvm.go` now holds `Provider`, `New` (loads the SSH key and known_hosts
up front via `newSSHAuth`, so a bad path fails at startup), the
`newWithDialer` seam, and `Create`/`Get`/`Delete`/`ListByTag`. Each
method resolves its host from config, then runs the whole libvirt
conversation inside one `hostPool.call`, so a cancelled reconcile drops
the connection and the blocked RPC returns. Errors pass through `classify`
in one `wrapErr`, as the Step 3b note intended.

Behaviour that goes beyond the plan text, or that a future reader should
know before touching this file:

- **Create looks up the domain first.** If it exists, only the running
  check runs and the volumes are never touched. If not, nothing can be
  using the volumes yet: the overlay is reused when it exists (its
  creation is atomic; `ErrStorageVolExist` → reuse) but the **seed is
  always deleted and rebuilt**. The plan said "exists → skip upload"; the
  reviewer pointed out that a reconcile cancelled mid-upload leaves a
  truncated seed that the compensating delete cannot remove, because
  `hostPool.call` has already closed the connection the delete would go
  over. Skipping such a seed would boot the VM without cloud-init and no
  retry would ever repair it. Rebuilding costs one ~40 KiB upload per
  attempt before the domain exists, nothing afterwards. The table test
  breaks Create after each step and re-runs it, and a separate test
  starts from a leftover truncated seed.
- **Get swallows address-source errors.** `DomainInterfaceAddresses` from
  ARP returns nothing until the guest has sent traffic and the agent
  source errors with `ErrAgentUnresponsive` until `qemu-guest-agent` is
  up, so both are logged at V(2) and mean "no IP yet" → `Provisioning`.
  Loopback, link-local and unspecified addresses are skipped when
  choosing the first IPv4 (the agent reports `lo` too).
- **State mapping:** `running`/`blocked` are active; `nostate` →
  Provisioning (the reconciler answers Stopped with delete-and-recreate,
  too drastic for a state libvirt could not determine); everything else
  (paused, pmsuspended, shutdown, shutoff, crashed) → Stopped.
- **Delete treats a missing pool as "nothing to clean"** rather than
  Permanent: failing there would pin the Proxy's finalizer on a config
  edit that renamed the pool. Domain removal is destroy (`ErrOperationInvalid`
  on a stopped domain is fine) → undefine with managed-save, snapshot
  metadata and nvram flags → overlay and seed deleted by name.
- **ListByTag** reads each domain's metadata once (passed into the same
  `inspect` that Get uses, so no second `DomainGetMetadata` round-trip)
  and skips domains without the operator's element. A domain that cannot
  be read at all — vanished mid-sweep, corrupt metadata, state call
  failing — is *skipped*, not fatal: GC never deletes what it does not
  see, so leaving one out is safe, whereas one bad domain blocking GC for
  every other one (the first version's behaviour, caught in review) is
  not. An unreachable host still fails the whole sweep.
  `ConnectListAllDomains` is called with flags 0 (active and inactive).
- A host named in a providerID but no longer in `kvm.hosts` is Permanent
  for Get and Delete: the operator cannot reach it, and retrying forever
  would hide the config problem.

Tests run the provider over a `hostDialer` that maps host names to
`fakeAPI` instances (an unmapped host is "unreachable"). The image is
pre-seeded in the pool for most tests; one test runs the full first-use
download through `httptest` and confirms a second Proxy on the same host
does not download again.

Found while running the package: `TestBuildSeedISO_deterministic` from
Step 2 was wrong. `iso9660.ImageWriter` stamps `time.Now()` into the
volume descriptor, so two builds in different seconds differ by six
timestamp bytes; the test had only passed because both builds landed in
the same second. Byte identity is nothing the provider depends on (an
existing seed is never re-uploaded), so the test now compares label,
files and size, and the comment on `buildSeedISO` no longer claims
identical output.

The `go-operator-reviewer` agent ran on the first version of this step
and found the two bugs above (truncated seed, wedged GC sweep) plus the
double metadata read and the `nostate` mapping; all were fixed before
committing. One finding it raised is deliberately **not** fixed here
because it lives in Step 3b's `image.go` and changes the volume naming
the plan chose: re-fetching a base image (checksum edited in config, or
upstream content changed) deletes `base-<key>.<fmt>` while existing
overlays still back onto it, breaking those VMs' disks. Content-addressed
base names would avoid it. Flagged for the user; until decided, Step 5's
docs must warn that an image's `sha256`/`url` must not change while
Proxies using it exist.

Worth noting: `go test -cover` reports 88.2 % for the package; the
uncovered lines are the real SSH/libvirt dial path and log-only
branches. `go vet` and `-race` (three runs) are clean. Wiring into
`cmd/main.go` is Step 5, so `New` is not yet reachable from a running
operator.

## Step 6 — Integration test

Done before Step 5, at the user's request, after a discussion of what the
unit tests can and cannot prove: they run over an in-memory fake of the
libvirt calls, so every assumption about libvirt's real behaviour (domain
XML accepted, raw qcow2 upload + pool refresh usable as a backing image,
guest boots and reports an address) needed a test against a real host.
The wiring in Step 5 is not a prerequisite — the test drives the provider
directly.

`internal/provider/kvm/kvm_integration_test.go` is behind
`//go:build integration` and skips itself under `-short` or when any of
`KVM_TEST_HOST`, `KVM_TEST_USER`, `KVM_TEST_SSH_KEY`, `KVM_TEST_BRIDGE`,
`KVM_TEST_IMAGE_URL` is unset. Optional knobs beyond the plan's list:
`KVM_TEST_KNOWN_HOSTS` (without it the run disables host-key checking
and logs that), `KVM_TEST_IMAGE_SHA256`, `KVM_TEST_POOL`,
`KVM_TEST_SOCKET`, `KVM_TEST_TIMEOUT` (default 20 m, must cover a cold
image download) and `KVM_TEST_KEEP` to leave a failed run's VM behind for
`virsh console`.

One sequential test, `TestKVM_lifecycle`, not subtests: each stage
depends on hypervisor state the previous one left. Create (timed, so the
log shows the image fetch), re-Create returns the same ID, poll Get to
Running with an IPv4 (Stopped fails fast with a hint to use the console),
UID and CreatedAt survive the metadata round-trip, ListByTag includes it,
Delete, Get → NotFound, second Delete is a no-op, ListByTag excludes it,
and the pool is inspected through the provider's own connection pool
(`p.hosts.call`, reachable because the test is in package `kvm`): overlay
and seed gone, base image and marker kept. A TCP connect to the guest's
port 22 is attempted but only logged, since the test machine may not
route to the bridge network. Cleanup deletes the VM on failure with its
own two-minute budget, because the test context may already be spent.

Makefile target, so the invocation is not something to remember:

```bash
make test-integration-kvm
# = go test -race -tags=integration -count=1 -v -timeout 40m -run TestKVM ./internal/provider/kvm/
```

`docs/testing.md` gained the kvm package rows in the suite table, a
"live libvirt" bullet under the deliberately-not-covered list, and a
section on running this test, with the deviation from the testcontainers
convention called out as the plan asked.

Worth noting: **the test has not been executed yet.** This session had
no hypervisor credentials; it was verified with
`go vet -tags=integration ./...` and by running it without the
environment (self-skip). Its first real run is the opening act of
Step 7's manual validation, and it is the point at which the Step 2 and
Step 3b "confirm on a real host" notes get answered. The test's config
is built directly rather than through the provider package's validation
(unexported), so a typo in an env var surfaces from `New` or from
libvirt, not from a validation message.

## Step 5 — Wiring, manifests, samples, docs, CHANGELOG

Done after Step 6 (see that entry for why the order flipped). Everything
the plan listed, plus a few things it did not:

- `cmd/main.go`: `"kvm": kvm.New` in the builtins map. No RBAC change —
  the provider talks to hypervisors, not the API server.
- `config/manager/manager.yaml`: `kvm-ssh` Secret mounted read-only at
  `/etc/proxy-operator/kvm` with `optional: true`, so deployments without
  a kvm provider are unaffected. It nests under the providers-config
  mount at `/etc/proxy-operator`; kustomize renders it (`kubectl
  kustomize config/default` checked). The container runs as uid 65532
  and Secret files default to 0644, so the key is readable without an
  `fsGroup`; the `kubectl create secret` recipe is in a comment next to
  the mount, in `docs/kvm-provider.md` and in the README.
- `config/samples/providers-config.yaml`: `kvm-home` block with Debian 13
  and Ubuntu 24.04 official image URLs, two machine types, one host, and
  every optional field shown commented out. Parsed through
  `provider.LoadConfigFile` in a throwaway test to make sure the sample
  validates. `config/samples/proxy_kvm.yaml` uses the GCP sample's squid
  cloud-init and shows `ssh_authorized_keys` commented out rather than
  set — a sample with a placeholder key would either fail cloud-init or
  invite pasting a real key into a committed file. Added to the samples
  kustomization; `config/manager/providers_config.yaml`'s comment now
  mentions kvm.
- `docs/kvm-provider.md`: prerequisites with a three-line preflight
  (`virsh list`, `pool-info`, `ip link` over ssh), cluster setup, the full
  config reference table, placement table, a table of what lands on the
  host and its lifetime, how the image cache works and how to evict, the
  **warning not to change or evict an image while Proxies use it** (the
  reviewer's finding from Step 4, carried as a documented rule since the
  content-addressed-naming alternative was not chosen), how to log in,
  and the known gaps.
- README: providers bullet and a "KVM setup" section mirroring the GCP
  one. `docs/architecture.md`: components row, a §5 block for the kvm
  call flow in the same style as the other two, and five decision-log
  entries (SSH transport, operator-fetched images, seed always rebuilt,
  `nostate` → Provisioning, skip-vs-fail in the GC sweep).
- `CHANGELOG.md` entry at the top, stating explicitly that the provider
  is not yet validated on a real host.

Verification: `go build ./...`, `go vet ./...`, `go test -short` on
`cmd` and `internal/provider/...`, and both kustomizations rendered.
`hack/providers-dev.yaml` untouched, as planned.

Worth noting: the CRD has no rule requiring all three placement fields
(that is GCP's own check), so the single-host "zone may be omitted"
behaviour works without an API change. And with this step the branch is
feature-complete but **unproven**: Step 7 runs the integration test and
the end-to-end checklist against a real hypervisor before the MR.

## Step 7 — Manual validation + MR (in progress)

Host: `beelink` (Ubuntu, libvirt 11.6.0, QEMU 10.1.0, pool `default` at
`/srv/vms/images` on local ext4, bridge `br0`), reached over Tailscale as
`beelink.tail8c69c.ts.net`, SSH user `novakj`. Not the `nas1.home.lan`
the plan and samples name; that host does not resolve from the dev box.

```bash
export KVM_TEST_HOST=beelink.tail8c69c.ts.net:22
export KVM_TEST_USER=novakj
export KVM_TEST_SSH_KEY=$HOME/.ssh/proxy-operator_ed25519
export KVM_TEST_KNOWN_HOSTS=$HOME/.ssh/kvm_known_hosts
export KVM_TEST_BRIDGE=br0
export KVM_TEST_IMAGE_URL=https://cloud.debian.org/images/cloud/trixie/latest/debian-13-generic-amd64.qcow2
make test-integration-kvm
```

### Run 1 and 2: `Could not open '/srv/vms/images/base-test-image.qcow2': Permission denied`

Everything up to `DomainCreate` worked on the first try against a real
host, which answers the open notes from Steps 2 and 3b: the SSH-forwarded
libvirt handshake, the raw qcow2 upload plus pool refresh as a backing
image, the default-namespace metadata element, and `DomainDefineXML` for
the generated XML were all accepted by libvirt 11.6. QEMU then exited at
start with `Permission denied` on the base image.

Ownership was ruled out the slow way: the file was `root:root 0600` (the
dir-pool default; libvirt's DAC driver chowns the chain at start and
restores it after a failed start, so that observation meant nothing),
`qemu.conf` was all defaults (`libvirt-qemu`, `dynamic_ownership=1`), and
after a manual `chown libvirt-qemu:kvm` the second run failed identically.

The actual cause is the Step 2 choice of `<disk type="volume">`.
libvirt's AppArmor helper builds each domain's profile from the domain XML
without consulting the storage driver, and its `add_file_path` only emits a
rule when the source has a `path`; a volume-referenced disk has pool and
volume names instead, so neither the overlay nor its backing image gets a
rule and QEMU is denied on the first one it opens. Every stock Debian and
Ubuntu hypervisor confines QEMU this way. The unit tests could not have
caught it: the fake accepts either form.

Fix, in this step: `buildDomainXML` takes `DiskPath`/`SeedPath` and emits
`type="file"` disks; `ensureOverlay` and `writeSeed` return the volume
handles (the exists-already branch now does a `StorageVolLookupByName`),
and Create resolves both paths with `StorageVolGetPath` before defining
the domain. Two extra cheap RPCs per Create. `TestBuildDomainXML_fieldByField`
now fails on any `type="volume"` or `pool=` attribute so the design cannot
drift back, and `TestCreate_freshHost` asserts the disk sources equal the
paths the fake reports.

Worth noting: the first two runs left the base image and its marker in
the pool as designed, so later runs skip the 435 MB download.

### Run 3: `parsing domain metadata: expected element <instance> in name space … but have no name space`

With file-backed disks, Create returned in 1 s (image cached) and the
domain started. Get then failed to parse the metadata: libvirt 11.6 finds
the element by our URI but returns it as a bare `<instance>` with no
namespace declaration at all. Step 2 had tested the default-namespace and
prefixed spellings and left "does libvirt accept the default-namespace
form" for the real host; it does, it just does not give it back that way.

Fix: `parseInstanceMetadata` matches on the local name only, since the
lookup already selected the element by URI. The fake now strips the
declaration when storing metadata, so the provider's Get and ListByTag
tests parse the shape libvirt really returns, and a new
`TestParseInstanceMetadata_libvirtBareForm` pins it.