diff --git a/.claude/settings.json b/.claude/settings.json index ec2dbda..e95a782 100644 --- a/.claude/settings.json +++ b/.claude/settings.json @@ -93,7 +93,13 @@ "Bash(git -C /Users/jan.novak/srv/go/egress-proxies-operator tag 0.01)", "Bash(git -C /Users/jan.novak/srv/go/egress-proxies-operator push origin 0.01)", "Bash(chmod +x docs/demo/run-demo.sh)", - "Bash(bash -n docs/demo/run-demo.sh)" + "Bash(bash -n docs/demo/run-demo.sh)", + "Bash(git add *)", + "Bash(git commit -q -m 'Add plan: KVM/libvirt provider *)", + "Bash(go get *)", + "Bash(go build *)", + "Bash(go test *)", + "Bash(git commit *)" ], "additionalDirectories": [ "/Users/jan.novak/srv/go/egress-proxies-operator/.claude", diff --git a/CHANGELOG.md b/CHANGELOG.md index 4fe773f..a8248f9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,91 @@ # 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 + (2 min from apply, IP via the guest agent, 204 through squid from the LAN). +- The `qemu-guest-agent` bootstrap is now merged into each Proxy's cloud-config + (`packages` + `runcmd`) instead of shipped as vendor-data: cloud-init's default + merger drops vendor-data keys the user-data also defines, so any Proxy with its own + `packages:` never got the agent and stayed `Provisioning` forever. `vendorData` is + now plain extra vendor-data with no default; non-cloud-config user-data (scripts, + `#include`) still gets the bootstrap as vendor-data. A cloud-config that is not a + YAML mapping fails the Proxy permanently instead of booting without its config. +- The manager pod mounts a 64 MiB `emptyDir` on `/tmp`: the seed ISO writer stages + files on disk and the root filesystem is read-only. +- Errors already classified with a provider sentinel no longer drop the cached + libvirt SSH connection; only libvirt and transport errors do. +- Key files: `internal/provider/kvm/{seed.go,conn.go,errors.go}`, + `config/manager/manager.yaml`, `docs/kvm-provider.md`. + +## 2026-09-10 10:07 UTC — KVM provider validated on a real hypervisor; UEFI default; libvirt call tracing + +- `make test-integration-kvm` passes end to end against a libvirt 11.6 / QEMU 10.1 + host (Debian 13 image, UEFI, guest agent reporting the IP after ~20 s). Four + defects found only on the real host, all fixed on the branch: + - domain disks are now `type="file"` with resolved paths: pool-volume references + get no AppArmor rule from `virt-aa-helper`, so QEMU was denied the disks; + - domain metadata is parsed by local name: libvirt returns the element without + its namespace declaration; + - guests boot **UEFI by default** (`images..firmware: uefi|bios`, OVMF now a + host prerequisite): Debian 13's image hangs in GRUB under SeaBIOS on a headless + VM with a serial console (Debian bug #1111240, open); bookworm is unaffected; + - the image cache's raw-upload path is confirmed to produce a byte-identical file. +- Every libvirt RPC is logged at `--zap-log-level=debug` as one `libvirt call` line + (`rpc=virDomainCreate`, identifiers, duration, `libvirtError` on failure); level 2 + adds XML, metadata and addresses; cloud-init is never logged. The integration test + shows the trace in `-v` output (`KVM_TEST_LOG_LEVEL`, default 1). +- Key files: `internal/provider/kvm/{domainxml.go,metadata.go,trace.go,conn.go}`, + `internal/provider/config_kvm.go`, `docs/kvm-provider.md`. + +## 2026-09-08 11:00 UTC — KVM/libvirt provider + +- New `kvm` provider type: Managed proxies as VMs on your own libvirt/KVM hosts. Each + host is reached over SSH with the libvirt unix socket forwarded through the + connection (pure Go, nothing to expose on the hypervisors beyond sshd); one provider + instance can span several hosts, selected by `placement.zone`. +- Base images (`images:` map or a direct URL in `placement.image`) are downloaded by the + operator into each host's storage pool on first use, verified by sha256 when configured, + and cached there. Per-VM disks are qcow2 overlays; cloud-init arrives on a NoCloud seed + ISO built in-process. GC identity lives in the domain's `` element. +- Wiring: `"kvm": kvm.New` in `cmd/main.go`; optional `kvm-ssh` Secret mounted at + `/etc/proxy-operator/kvm`; samples `config/samples/proxy_kvm.yaml` and a `kvm-home` + block in `providers-config.yaml`; `docs/kvm-provider.md`; opt-in integration test + (`make test-integration-kvm`) against a real hypervisor. +- Not validated on a real host at the time of this entry; see 2026-09-10 above. + ## 2026-08-10 09:34 CEST — kind e2e verified; fix Squid OOM in containers; quickstart goes in-cluster - Full end-to-end pass on a throwaway kind cluster: Squid pod Ready with a real CONNECT diff --git a/Makefile b/Makefile index d348f5d..b7ab1ce 100644 --- a/Makefile +++ b/Makefile @@ -66,6 +66,10 @@ vet: ## Run go vet against code. test: manifests generate fmt vet setup-envtest ## Run tests. KUBEBUILDER_ASSETS="$(shell "$(ENVTEST)" use $(ENVTEST_K8S_VERSION) --bin-dir "$(LOCALBIN)" -p path)" go test -race $$(go list ./... | grep -v /e2e) -coverprofile cover.out +.PHONY: test-integration-kvm +test-integration-kvm: ## Run the KVM provider test against a real hypervisor. Needs KVM_TEST_HOST/USER/SSH_KEY/BRIDGE/IMAGE_URL; see docs/testing.md. + go test -race -tags=integration -count=1 -v -timeout 40m -run TestKVM ./internal/provider/kvm/ + # TODO(user): To use a different vendor for e2e tests, modify the setup under 'tests/e2e'. # The default setup assumes Kind is pre-installed and builds/loads the Manager Docker image locally. # kubectl kuberc is disabled by default for test isolation; enable with: @@ -174,6 +178,20 @@ uninstall: manifests kustomize ## Uninstall CRDs from the K8s cluster specified @out="$$( "$(KUSTOMIZE)" build config/crd 2>/dev/null || true )"; \ if [ -n "$$out" ]; then echo "$$out" | "$(KUBECTL)" delete --ignore-not-found=$(ignore-not-found) -f -; else echo "No CRDs to delete; skipping."; fi +KVM_SSH_KEY ?= $(HOME)/.ssh/proxy-operator_ed25519 +KVM_KNOWN_HOSTS ?= $(HOME)/.ssh/kvm_known_hosts +KVM_SSH_NAMESPACE ?= egress-proxies-operator-system + +.PHONY: kvm-ssh-secret +kvm-ssh-secret: ## Create or update the kvm-ssh Secret from KVM_SSH_KEY (private key) and KVM_KNOWN_HOSTS. + @test -f "$(KVM_SSH_KEY)" || { echo "KVM_SSH_KEY=$(KVM_SSH_KEY) not found"; exit 1; } + @test -f "$(KVM_KNOWN_HOSTS)" || { echo "KVM_KNOWN_HOSTS=$(KVM_KNOWN_HOSTS) not found"; exit 1; } + @grep -q 'PRIVATE KEY' "$(KVM_SSH_KEY)" || { echo "KVM_SSH_KEY=$(KVM_SSH_KEY) is not a private key (did you pass the .pub?)"; exit 1; } + "$(KUBECTL)" -n $(KVM_SSH_NAMESPACE) create secret generic kvm-ssh \ + --from-file=id_ed25519="$(KVM_SSH_KEY)" \ + --from-file=known_hosts="$(KVM_KNOWN_HOSTS)" \ + --dry-run=client -o yaml | "$(KUBECTL)" apply -f - + .PHONY: deploy deploy: manifests kustomize ## Deploy controller to the K8s cluster specified in ~/.kube/config. cd config/manager && "$(KUSTOMIZE)" edit set image controller=${IMG} diff --git a/README.md b/README.md index a3a80dd..df3df65 100644 --- a/README.md +++ b/README.md @@ -15,9 +15,10 @@ itself**, and hands out to crawler clients via an HTTP list/lease API. cattle**: any meaningful spec change (placement, cloud-init, port) deletes and recreates the VM — never in-place mutation. - **Providers** behind one minimal interface: `kubernetes` (a real Squid - pod in this cluster — local dev/CI) and `gcp` (Compute Engine VMs with - ephemeral external IPs — the real egress fleet). Config is a YAML file - (`--providers-config`) with named instances (`gcp-eu`, `gcp-us`, ...). + pod in this cluster — local dev/CI), `gcp` (Compute Engine VMs with + ephemeral external IPs — the real egress fleet) and `kvm` (VMs on your + own libvirt hypervisors, reached over SSH). Config is a YAML file + (`--providers-config`) with named instances (`gcp-eu`, `kvm-home`, ...). - **Health engine** probes every proxy by fetching a URL *through* it (a real CONNECT tunnel — a proxy that accepts TCP but can't egress goes Unhealthy), with threshold logic and transition-only status writes. @@ -131,11 +132,44 @@ make deploy IMG=/egress-proxies-operator:dev which also installs Squid via cloud-init). A missing field fails the Proxy with a message naming it. +## KVM setup + +Managed proxies as VMs on libvirt/KVM hosts you own. Full guide: +[docs/kvm-provider.md](docs/kvm-provider.md). + +1. Each host needs `libvirtd`, an SSH user in the `libvirt` group, a + bridge to a network with DHCP, and a storage pool. +2. Create the `kvm-ssh` Secret from the operator's **private** key and a + `known_hosts` with each host's key; the manager mounts it at + `/etc/proxy-operator/kvm`: + + ```sh + ssh-keyscan -H >> ~/.ssh/kvm_known_hosts + kubectl -n egress-proxies-operator-system create secret generic kvm-ssh \ + --from-file=id_ed25519=$HOME/.ssh/proxy-operator_ed25519 \ + --from-file=known_hosts=$HOME/.ssh/kvm_known_hosts + # or: make kvm-ssh-secret (same paths by default; KVM_SSH_KEY / KVM_KNOWN_HOSTS to override) + ``` + +3. Add a `kvm` entry to the providers config with `images`, + `machineTypes` and `hosts` (see + [config/samples/providers-config.yaml](config/samples/providers-config.yaml)). + Base images are downloaded into each host's pool on first use. +4. A Proxy names a host (`placement.zone`, optional with one host), a + size (`placement.machineType`) and an image (`placement.image`) — see + [config/samples/proxy_kvm.yaml](config/samples/proxy_kvm.yaml). + Cloud-init from a Secret: the Secret **must** carry the label `crawl.example.com/cloud-init: "true"` — the operator's cache only holds 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 diff --git a/api/v1alpha1/proxy_types.go b/api/v1alpha1/proxy_types.go index 865fad0..bd17201 100644 --- a/api/v1alpha1/proxy_types.go +++ b/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 diff --git a/cmd/main.go b/cmd/main.go index 2b85b59..18d76cf 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -58,6 +58,7 @@ import ( "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider" "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider/gcp" "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider/kubernetes" + "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider/kvm" "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/provider/registry" "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/tracing" "gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/internal/version" @@ -187,6 +188,7 @@ func main() { "gcp": func(ctx context.Context, pc provider.ProviderConfig) (provider.Provider, error) { return gcp.NewWithWireOptions(ctx, pc, gcp.WireLogOptions{FullPayloads: gcpWireFullPayloads}) }, + "kvm": kvm.New, }) if err != nil { setupLog.Error(err, "Failed to build providers") diff --git a/config/manager/kustomization.yaml b/config/manager/kustomization.yaml index 08f98ea..e268bef 100644 --- a/config/manager/kustomization.yaml +++ b/config/manager/kustomization.yaml @@ -5,5 +5,5 @@ apiVersion: kustomize.config.k8s.io/v1beta1 kind: Kustomization images: - name: controller - newName: example.com/egress-proxies-operator - newTag: v0.0.1 + newName: egress-proxies-operator + newTag: kvm-dev diff --git a/config/manager/manager.yaml b/config/manager/manager.yaml index 5676abb..0f9a83a 100644 --- a/config/manager/manager.yaml +++ b/config/manager/manager.yaml @@ -145,9 +145,30 @@ spec: - name: providers-config mountPath: /etc/proxy-operator readOnly: true + # SSH material for kvm providers (sshKeyFile / knownHostsFile in the + # providers config point here). Optional: the mount is empty when the + # Secret does not exist, and only kvm entries need it. Create it with: + # kubectl -n egress-proxies-operator-system create secret generic kvm-ssh \ + # --from-file=id_ed25519= \ + # --from-file=known_hosts= + - name: kvm-ssh + mountPath: /etc/proxy-operator/kvm + readOnly: true + # Scratch space: the root filesystem is read-only, and the kvm + # provider builds each VM's cloud-init seed ISO through a temp + # directory (a few hundred KB, deleted right after upload). + - name: tmp + mountPath: /tmp volumes: - name: providers-config configMap: name: providers-config + - name: kvm-ssh + secret: + secretName: kvm-ssh + optional: true + - name: tmp + emptyDir: + sizeLimit: 64Mi serviceAccountName: controller-manager terminationGracePeriodSeconds: 10 diff --git a/config/manager/providers_config.yaml b/config/manager/providers_config.yaml index 6c383c0..311a96d 100644 --- a/config/manager/providers_config.yaml +++ b/config/manager/providers_config.yaml @@ -9,9 +9,46 @@ metadata: data: # Mounted at /etc/proxy-operator/providers.yaml (--providers-config). # The default ships only the kubernetes-pod provider so the operator runs - # out of the box; add gcp entries (type: gcp, gcp.project: ...) for real - # egress fleets — see config/samples/providers-config.yaml. + # out of the box; add gcp entries (type: gcp, gcp.project: ...) or kvm + # entries (type: kvm, hosts reached over SSH) for real egress fleets — + # see config/samples/providers-config.yaml. providers.yaml: | providers: - name: kubernetes type: kubernetes + - name: kvm-home + type: kvm + kvm: + # SSH to each host with the libvirt unix socket forwarded through the + # connection; nothing to open on the hypervisors beyond sshd. Both + # files come from the optional kvm-ssh Secret mounted by + # config/manager/manager.yaml — see docs/kvm-provider.md. + sshKeyFile: /etc/proxy-operator/kvm/id_ed25519 + 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 + # #cloud-config # into each Proxy's cloud-config, not configured here + # ... + images: + # placement.image names a key here (or is an https:// URL directly). + # Fetched into each host's pool on first use and cached there as + # base-.qcow2; do not change an entry while Proxies use it. + debian-13: + url: https://cloud.debian.org/images/cloud/trixie/latest/debian-13-generic-amd64.qcow2 + # sha256: "<64 hex>" # optional; verified after download + # firmware: uefi # default; "bios" only for images with no EFI partition + # # (Debian 13 hangs in GRUB under BIOS headless: Debian bug #1111240) + ubuntu-24.04: + url: https://cloud-images.ubuntu.com/noble/current/noble-server-cloudimg-amd64.img + machineTypes: # placement.machineType names a key here + small: {vcpus: 1, memoryMiB: 1024} + medium: {vcpus: 2, memoryMiB: 2048} + hosts: # placement.zone names a host; optional with one host + - name: beelink + address: beelink.tail8c69c.ts.net:22 + user: novakj # must be in the libvirt group + bridge: br0 # to a LAN with DHCP; the VM's IP comes from there + # socket: /var/run/libvirt/libvirt-sock + # pool: default + diff --git a/config/samples/kustomization.yaml b/config/samples/kustomization.yaml index d1c025f..4545440 100644 --- a/config/samples/kustomization.yaml +++ b/config/samples/kustomization.yaml @@ -4,5 +4,6 @@ resources: - proxy_kubernetes.yaml - proxy_gcp.yaml +- proxy_kvm.yaml - proxy_external.yaml # +kubebuilder:scaffold:manifestskustomizesamples diff --git a/config/samples/providers-config.yaml b/config/samples/providers-config.yaml index 34220d8..e7d57b9 100644 --- a/config/samples/providers-config.yaml +++ b/config/samples/providers-config.yaml @@ -19,3 +19,39 @@ providers: # diskSizeGb: 10 # auth: Application Default Credentials (workload identity # in-cluster, gcloud ADC locally). No key-file plumbing. + - name: kvm-home + type: kvm + kvm: + # SSH to each host with the libvirt unix socket forwarded through the + # connection; nothing to open on the hypervisors beyond sshd. Both + # files come from the optional kvm-ssh Secret mounted by + # config/manager/manager.yaml — see docs/kvm-provider.md. + sshKeyFile: /etc/proxy-operator/kvm/id_ed25519 + 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 + # installGuestAgent: false # skip the vendor-data script that installs qemu-guest-agent + # 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). + # Fetched into each host's pool on first use and cached there as + # base-.qcow2; do not change an entry while Proxies use it. + debian-13: + url: https://cloud.debian.org/images/cloud/trixie/latest/debian-13-generic-amd64.qcow2 + # sha256: "<64 hex>" # optional; verified after download + # firmware: uefi # default; "bios" only for images with no EFI partition + # # (Debian 13 hangs in GRUB under BIOS headless: Debian bug #1111240) + ubuntu-24.04: + url: https://cloud-images.ubuntu.com/noble/current/noble-server-cloudimg-amd64.img + machineTypes: # placement.machineType names a key here + small: {vcpus: 1, memoryMiB: 1024} + medium: {vcpus: 2, memoryMiB: 2048} + hosts: # placement.zone names a host; optional with one host + - name: beelink + address: beelink:22 + user: novakj # must be in the libvirt group + bridge: br0 # to a LAN with DHCP; the VM's IP comes from there + # socket: /var/run/libvirt/libvirt-sock + # pool: default diff --git a/config/samples/proxy_kvm.yaml b/config/samples/proxy_kvm.yaml new file mode 100644 index 0000000..e84e4d6 --- /dev/null +++ b/config/samples/proxy_kvm.yaml @@ -0,0 +1,43 @@ +# A Managed proxy backed by a libvirt/KVM hypervisor on the home network: +# the operator boots a stock Debian cloud image as a VM on the named host, +# with a NIC on the host's bridge (IP from the LAN's DHCP), and installs +# Squid via cloud-init. Requires a providers-config entry named "kvm-home" +# (see providers-config.yaml) and the kvm-ssh Secret (docs/kvm-provider.md). +# +# placement.zone is the host name from kvm.hosts (optional when only one +# host is configured); machineType and image are keys in the provider's +# machineTypes and images maps. The first Proxy for an image on a host +# waits for the image download; later ones reuse the cached copy. +apiVersion: crawl.example.com/v1alpha1 +kind: Proxy +metadata: + name: proxy-kvm-sample +spec: + mode: Managed + provider: kvm-home + placement: + zone: nas1 + machineType: small + image: debian-13 + port: 3128 + cloudInit: + inline: | + #cloud-config + packages: + - squid + write_files: + - path: /etc/squid/conf.d/proxy-operator.conf + content: | + http_port 3128 + http_access allow all + via off + forwarded_for off + runcmd: + - systemctl restart squid + # Optional: log in with `ssh debian@` (the Debian image's default + # user) to look around; `virsh console ` on the host works too. + # ssh_authorized_keys: + # - ssh-ed25519 AAAA... you@laptop + attributes: + geo: home + purpose: crawl diff --git a/docs/architecture.md b/docs/architecture.md index 5c1053b..3f7daee 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -9,6 +9,7 @@ | Provider contract | `internal/provider` | library | — | `Provider` interface, error taxonomy, deterministic naming, config, metrics decorator | | kubernetes provider | `internal/provider/kubernetes` | library | — | real Squid pods in this cluster (local dev/CI) | | gcp provider | `internal/provider/gcp` | library | — | Compute Engine VMs, four API calls, fire-and-forget ops | +| kvm provider | `internal/provider/kvm` | library | — | VMs on libvirt hosts over SSH-forwarded sockets; image cache, overlay disks, NoCloud seeds | | Health engine | `internal/health` | Runnable | yes | through-the-proxy probes, thresholds, transition events | | Lease store | `internal/lease` | Runnable (expiry sweep) | no | in-memory leases + cooldowns, single mutex | | Discovery API | `internal/discovery` | Runnable | no | HTTP list/lease/release/report on `:8090` | @@ -147,6 +148,25 @@ prov.ListByTag ─► AggregatedList(label filter, ─┘ prov ReturnPartialSuccess: true) providerID = zones//instances/ — zone-qualified, so Get/Delete stay correct even mid-replacement after a zone edit + +kvm provider (internal/provider/kvm/) — libvirt RPC over an SSH-forwarded unix socket, per host +prov.Create ──► DomainLookupByName: exists → start if needed, done ─┐ every step tolerates + else: ensure base image in pool (download on first use) │ the previous attempt + overlay volume (exists = reuse) │ having got that far; + seed ISO (always rebuilt — a cut-off upload must │ the seed is the one + never be trusted) │ thing redone + DomainDefineXML + DomainCreate ─┘ +prov.Get ──► DomainGetState + DomainGetMetadata + DomainInterfaceAddresses + (ARP, then guest agent; first routable IPv4) + running without IP = Provisioning; nostate = Provisioning +prov.Delete ──► destroy (not running = fine) → undefine → delete overlay + seed by name +prov.ListByTag ─► ConnectListAllDomains on every host, keep domains whose + carries managed=true; one host down fails the sweep, + one unreadable domain is skipped +providerID = hosts//domains/; GC identity lives in the domain's + element (libvirt has no labels). All RPCs run under a context- +bounded call: go-libvirt never times out on its own, so a cancelled reconcile +closes the SSH connection to unblock it. ``` The reconciler never watches provider-side resources (Pods or GCP VMs). @@ -415,6 +435,41 @@ they read as choices, not accidents. Chronological by build step. - **Unknown GCP instance statuses map to `Stopped`:** the reconciler's answer to Stopped is delete-and-recreate, the always-safe move for cattle when the API grows a new state. +- **KVM hosts are reached over SSH with the libvirt socket forwarded,** + not libvirtd's TCP/TLS listener: nothing to configure or expose on the + hypervisors beyond sshd, pure Go on the operator side (no cgo, no + libvirt libraries in the image), and the hosts list can grow by a + config edit because the providerID carries the host name. +- **KVM base images are fetched by the operator into the host's pool** + through libvirt's upload stream, rather than expected to be pre-placed: + the operator then needs no filesystem access on the host and a new + host is usable with zero manual staging. The cost is a synchronous + first-time download inside Create. +- **The KVM seed ISO is rebuilt on every Create attempt** before the + domain exists, instead of "exists → skip": a reconcile cancelled mid + upload leaves a truncated seed, and the compensating delete cannot run + because the connection was already closed to unblock the RPC. Skipping + it would boot a VM with no cloud-init and no retry would ever repair it. +- **KVM `nostate` maps to `Provisioning`, not `Stopped`:** unlike GCP's + unknown statuses, `nostate` means libvirt could not determine the + state at all; answering that with delete-and-recreate is too drastic, + so the next poll decides. +- **KVM guests boot UEFI by default, secure boot off.** Found on the + first real host: Debian 13's generic image hangs in GRUB under SeaBIOS + on a headless VM (Debian bug #1111240: `terminal_output gfxterm + serial` with a serial console and no video device) regardless of CPU + model or seed, and boots under OVMF with the serial console working. + The alternatives were a virtio console (loses the kernel's serial + output) or a video device the VMs have no use for. Every + current distro cloud image supports UEFI, so it is the default and + `firmware: bios` is the per-image escape; libvirt's firmware + autoselection (``) keeps the OVMF path out of the + operator's config. Secure boot is off so unsigned images boot too. +- **A KVM domain GC cannot read is skipped, a host it cannot reach fails + the sweep.** GC never deletes what it does not see, so leaving one + domain out is safe; one corrupt domain blocking GC for every other one + is not. A whole host missing from the view would hide many, so that + still aborts. - **Kubernetes 403s classify as `ErrPermanent`** even though quota exhaustion also surfaces as 403 (indistinguishable from RBAC denial in `apierrors`): not hammering an API server that may never allow the @@ -434,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 diff --git a/docs/kvm-provider.md b/docs/kvm-provider.md new file mode 100644 index 0000000..f58e4e5 --- /dev/null +++ b/docs/kvm-provider.md @@ -0,0 +1,318 @@ +# The KVM provider + +Runs Managed proxies as VMs on your own libvirt/KVM hypervisors. The +operator reaches each host over SSH, forwards the libvirt unix socket +through that connection, and talks libvirt's own RPC over it — so the +hosts need nothing beyond `sshd` and `libvirtd`, and the operator image +carries no libvirt libraries. VMs boot a stock distro cloud image that the +operator downloads into the host's storage pool on first use, with the +Proxy's cloud-init delivered on a NoCloud seed ISO built in-process. + +## Hypervisor prerequisites + +Per host, once: + +1. **libvirt + KVM** (`libvirtd` running, `/dev/kvm` present). +2. **An SSH user in the `libvirt` group**, so it may open + `/var/run/libvirt/libvirt-sock`. The operator authenticates with a + private key; nothing else on the host is touched. +3. **A bridge to a network with DHCP** (e.g. `br0` bridged onto the LAN). + Each VM gets one virtio NIC on it and takes its IP from that network's + DHCP; the cluster must be able to reach those IPs for health probes + and for crawlers to use the proxy. +4. **A storage pool** (default `default`, usually `/var/lib/libvirt/images`) + with room for one base image per distro (a few hundred MB) plus the + per-VM overlays (`diskSizeGb` each, thin-provisioned). +5. **UEFI firmware for guests**: the `ovmf` package on Debian/Ubuntu, + `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**: a vendor-data script + installs `qemu-guest-agent` from the distro mirrors on first boot. + Without it the VM still works — the operator falls back to the host's + ARP table to find the IP, which fills in once the guest sends traffic — + but the ARP table only sees traffic to the host itself, so on a plain + bridge the agent is in practice required. + +Check from the operator's point of view before wiring it in: + +```sh +ssh virt@nas1.home.lan virsh -c qemu:///system list --all # user can drive libvirt +ssh virt@nas1.home.lan virsh -c qemu:///system pool-info default +ssh virt@nas1.home.lan ip link show br0 +``` + +## Cluster-side setup + +1. Create the SSH Secret from a key pair dedicated to the operator and a + `known_hosts` file holding every host's key. The worked example below + uses `~/.ssh/proxy-operator_ed25519` and `~/.ssh/kvm_known_hosts`, + the same paths the integration test and `make kvm-ssh-secret` default + to. + + ```sh + # once: a key pair for the operator; the .pub goes on every host + ssh-keygen -t ed25519 -f ~/.ssh/proxy-operator_ed25519 -N '' -C proxy-operator + ssh-copy-id -i ~/.ssh/proxy-operator_ed25519.pub virt@nas1.home.lan + + # once per host: its host key, under the address the *operator* will + # dial (hosts[].address), which may differ from what your laptop uses + ssh-keyscan -H nas1.home.lan >> ~/.ssh/kvm_known_hosts + + # the Secret: the PRIVATE key (not the .pub) plus known_hosts + kubectl -n egress-proxies-operator-system create secret generic kvm-ssh \ + --from-file=id_ed25519=$HOME/.ssh/proxy-operator_ed25519 \ + --from-file=known_hosts=$HOME/.ssh/kvm_known_hosts + ``` + + `make kvm-ssh-secret` runs that last command as an apply, so it also + updates an existing Secret; override the paths with + `KVM_SSH_KEY=... KVM_KNOWN_HOSTS=...`. Putting the `.pub` in the + Secret is the classic mistake: the operator then fails at startup + with a "not a private key" error. Without `known_hosts`, set + `insecureIgnoreHostKey: true` and accept that a MITM could hand you a + fake hypervisor. + + `config/manager/manager.yaml` mounts the Secret (optional) at + `/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, even with a `kvm` provider configured: only that provider's + proxies fail, and they recover once the Secret exists. Rotating the + 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 + `config/samples/providers-config.yaml`): + + ```yaml + - name: kvm-home + type: kvm + kvm: + sshKeyFile: /etc/proxy-operator/kvm/id_ed25519 + knownHostsFile: /etc/proxy-operator/kvm/known_hosts + images: + debian-13: + url: https://cloud.debian.org/images/cloud/trixie/latest/debian-13-generic-amd64.qcow2 + machineTypes: + small: {vcpus: 1, memoryMiB: 1024} + hosts: + - name: nas1 + address: nas1.home.lan:22 + user: virt + bridge: br0 + ``` + + The operator checks both files at startup and logs a `WARNING:` line + when either is unreadable, but it does not refuse to start: the + 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: + + ```sh + kubectl get px -w + # NAME MODE PROVIDER PHASE IP HEALTHY + # proxy-kvm-sample Managed kvm-home Provisioning + # proxy-kvm-sample Managed kvm-home Running 192.168.1.73 + # proxy-kvm-sample Managed kvm-home Ready 192.168.1.73 True + ``` + + The first Proxy for a given image on a given host sits in + `Provisioning` for the length of the image download; the operator log + shows `fetching base image into host pool` and `base image cached` + with the size and duration. + +## Config reference + +| Field | Required | Default | Meaning | +|---|---|---|---| +| `sshKeyFile` | yes | | private key used for every host | +| `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. 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.` | | | the key names the image in `placement.image`. Letters, digits, dots, dashes and underscores only — it becomes the cached volume name `base-.` | +| `images..url` | yes per entry | | http(s) URL of a cloud image (qcow2 or raw) | +| `images..sha256` | | | expected digest; verified after download, a change triggers a re-download | +| `images..format` | | `qcow2` | `qcow2` or `raw` | +| `images..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.` | ≥ 1 | | `{vcpus, memoryMiB}` size profile | +| `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//domains/` | +| `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 | +| `hosts[].pool` | | `default` | storage pool for base images, overlays and seeds | +| `hosts[].bridge` | yes | | host bridge for the VM's NIC | + +## Placement + +| Proxy field | Meaning for kvm | +|---|---| +| `placement.zone` | host name from `hosts`. May be omitted when exactly one host is configured; otherwise required. Unknown → `Failed` | +| `placement.machineType` | key in `machineTypes`. Unknown → `Failed` | +| `placement.image` | key in `images`, or an `http(s)://` URL used directly. Anything else → `Failed` | +| `placement.region` | ignored | + +`Failed` here means the reconciler stops retrying and the message names +the offending value; fix the spec (which replaces the VM) or the config. + +## What lands on the host + +For a Proxy whose deterministic name is `proxy-abc…`, on the chosen +host's pool: + +| Object | Name | Lifetime | +|---|---|---| +| base image | `base-.qcow2` (or `base-.qcow2` for a URL) | cached; survives Proxy deletion | +| image marker | `base-.qcow2.sha256` | with the base image | +| overlay disk | `proxy-abc….qcow2`, qcow2 backed by the base | with the Proxy | +| seed ISO | `proxy-abc…-seed.iso` | with the Proxy | +| domain | `proxy-abc…` | with the Proxy | + +The domain is a q35 machine with host-passthrough CPU, the overlay on +virtio, the seed on a SATA cdrom, one virtio NIC on the bridge, a serial +console, the guest-agent channel, a virtio RNG and no graphics. Its +`` carries the operator's element (managed flag, owning UID, +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`, **byte for byte**. Any +format cloud-init accepts works, including a Jinja template, a MIME +archive, a shell script or a gzipped payload; the operator never parses or +re-renders it. + +The guest agent rides along in `vendor-data`, as a **shell script** rather +than a cloud-config. That is the only shape cloud-init cannot drop: +vendor-data cloud-config is merged under the user's own document with +mergers that keep the user-data's value for every key both define and never +append lists, so a Proxy carrying its own `packages:` silently lost the +agent and sat in `Provisioning` forever. The strategy cannot be overridden +from either document — `merge_how` in user-data governs only the first +source, and `merge_how` in vendor-data is consumed and discarded before the +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 + +The base image is downloaded **by the operator, through libvirt**, into +the host's pool the first time a Proxy asks for it on that host — nothing +is pre-placed on the hypervisor, and the operator needs no filesystem +access there. The bytes stream from the URL through the operator into +the pool with a running sha256; nothing is buffered. The marker volume is +written last and holds the digest, so a crash mid-download leaves a base +volume without a marker, which the next attempt deletes and redoes. + +A re-download happens when: the marker is missing, or `sha256` is +configured and differs from the marker's content. Otherwise the cached +copy is used for as long as it exists, even if the URL's content changed +upstream (a `latest` URL is pinned at the moment of first fetch, per +host). + +To evict manually: + +```sh +ssh virt@nas1 virsh -c qemu:///system vol-delete --pool default base-debian-13.qcow2.sha256 +ssh virt@nas1 virsh -c qemu:///system vol-delete --pool default base-debian-13.qcow2 +``` + +⚠️ **Do not change an image entry's `url` or `sha256`, and do not evict +its base volume, while Proxies using that image exist on the host.** The +overlays back onto the base file; deleting or replacing it corrupts every +VM that uses it. Delete those Proxies first, or add the new image under a +new key and move Proxies over by changing their `placement.image` (which +replaces the VM). + +## Logging into a VM + +- Put your key in the Proxy's cloud-init (`ssh_authorized_keys`) and + `ssh debian@` (Debian) / `ssh ubuntu@` (Ubuntu). +- Or on the host: `virsh console proxy-abc…` attaches to the serial + console; cloud-init's output scrolls by there on first boot. +- `virsh domifaddr proxy-abc… --source arp` / `--source agent` shows what + the operator sees when it looks for the IP. + +## Logging + +The provider logs through controller-runtime's logger, named `kvm`, with +`provider`, `host` and `name` (the domain) on every line. + +- `--zap-log-level=debug` (logr V(1)): one line per provider operation + (`domain defined`, `KVM domain ensured running`, ...) **and one + `libvirt call` line per libvirt RPC**, named `kvm.libvirt`, with + `rpc` (the libvirt API function, e.g. `virDomainLookupByName`), the + identifiers it was given (`domain`, `pool`, `volume`), curated results + (`state`, `path`, `interfaces`), `duration`, and on failure `error` + plus `libvirtError` (the code name, e.g. `ErrNoDomain`) and + `libvirtCode`. Connection events (`libvirt host connected`, dial + failures, `libvirt connection dropped` with a reason) log at the same + level. A Create is about ten RPCs; a Get is four. +- `--zap-log-level=2`: the same lines also carry the domain and volume + `xml`, the `metadata` element, the interface `addresses` and the + domain `names` from a listing. +- Cloud-init is never logged at any level: the seed ISO upload reports + only `length` and `duration`. There is no raw wire tier like the GCP + provider's V(5); go-libvirt exposes no hook for it. + +## Known gaps + +- **No quota or capacity detection.** libvirt reports an out-of-space + pool or an over-committed host as an ordinary error; the reconciler + treats it as transient and retries with backoff rather than the slower + quota backoff. +- **Orphan volumes without a domain are invisible to GC.** A crash + between creating the volumes and defining the domain leaves an overlay + and seed that `ListByTag` cannot see (it lists domains). They are + 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. +- **Replacing an image in use breaks running VMs** — see the warning + under *The image cache*. +- **IP discovery needs either guest traffic or the agent.** A VM on a + 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`, `cloud-init status --long` and the vendor script's + 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 + GRUB config, and with a serial console but no video device (exactly + the domain this provider defines) GRUB prints "Booting" and never + hands over to the kernel. Not host-specific; reproduced by the Debian + cloud team. The same image boots under UEFI with the serial console + intact, which is why `firmware` defaults to `uefi`. Debian 12 + (bookworm) images are unaffected and boot either way. Images that + need `firmware: bios` are the exception, not the rule. + +## Testing + +Unit tests run over an in-memory fake of the libvirt API. The optional +integration test against a real host is described in +[testing.md](testing.md#the-kvm-integration-test-real-hypervisor); its +`-v` output includes the `libvirt call` trace above, at the verbosity +`KVM_TEST_LOG_LEVEL` selects (default 1). diff --git a/docs/plans-executions/2026-09-04-1404-kvm-provider.md b/docs/plans-executions/2026-09-04-1404-kvm-provider.md new file mode 100644 index 0000000..3f961cf --- /dev/null +++ b/docs/plans-executions/2026-09-04-1404-kvm-provider.md @@ -0,0 +1,673 @@ +# 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) +- [x] 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 + ``, so only the *base* + image path is ever looked up (the overlay's `` 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 +(``) 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-.` 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 ``. +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 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 `` 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. + +### Run 4: guest hangs at GRUB's "Booting" line + +Create and start passed; Get polled `Provisioning` with no IP while the +VM sat at 100 % CPU after GRUB printed "Booting `Debian GNU/Linux`" and +before the kernel wrote anything to the serial console. The domain XML +matches a working VM on the same host (q35, BIOS, host-passthrough, +virtio disk, SATA cdrom, br0), so the difference is the disk content: +the cached image was downloaded without a checksum and never verified. +Diagnosis was handed to the user (sha512 against Debian's published +sums, `qemu-img check`, and a `virt-install --import` of the same image +outside the operator). Because the provider's own logs were invisible in +the test output, this run also produced a side plan: +[2026-09-10-0805-kvm-libvirt-tracing](2026-09-10-0805-kvm-libvirt-tracing.md), +which traces every libvirt RPC at V(1) and routes it into `-v` output. + +### Run 5: the guest hang is Debian 13's BIOS boot path; UEFI fixes it + +The cached image's sha512 matched Debian's published sum, and +`qemu-img info` showed a sound 3 GiB qcow2, which also confirms that +the raw upload over a libvirt-created volume plus pool refresh yields a +byte-identical file (the Step 3b note is answered). Bisected by hand on +beelink with `virt-install --import` on a fresh overlay of the same +image: + +| variant | seed | CPU | firmware | result | +|---|---|---|---|---| +| operator's domain | ours | host-passthrough | SeaBIOS | hangs after GRUB "Booting", 100 % CPU | +| bare | none | host-passthrough | SeaBIOS | same hang | +| bare | none | host-model | SeaBIOS | same hang | +| bare | none | host-passthrough | **OVMF (`--boot uefi`)** | **boots to login** | + +So neither the seed ISO, the domain XML nor the CPU model was the +problem: the image's legacy GRUB → kernel handoff does not work on this +host, while its EFI stub path does. Not diagnosed further; it is +Debian's chain, not ours, and every current cloud image boots UEFI. + +Change: `images..firmware` (`uefi` default, `bios` escape), +threaded through `withDefaults` → `imageSource` → `domainParams`, and +`buildDomainXML` emits `` so libvirt selects the +OVMF binary and creates the NVRAM file; the `nvram` undefine flag Delete +already passes removes it (seen on the host: `virsh undefine` without +`--nvram` refuses such a domain). Direct-URL images default to UEFI +too. Docs: OVMF is a new hypervisor prerequisite, config table row, +sample comment, known-gaps entry, decision-log entry. + +Worth noting: the first image download went through with no checksum +configured, and the cache reuses a base image by key, so switching +`KVM_TEST_IMAGE_URL` requires deleting `base-test-image.qcow2` and its +marker by hand first. That is the reviewer's Step 4 finding showing up +in practice; still documented rather than fixed. + +Follow-up, same day: the cause is known upstream. [Debian bug +#1111240](https://bugs.debian.org/1111240) (filed 2025-08-16 against +the cloud image, reassigned to `grub2`, still open as of 2026-09-10; +trixie ships grub2 2.12-9+deb13u2) reports exactly this with a domain +XML almost identical to ours: BIOS, virtio disk, SATA cdrom seed, bridge +NIC, ``, no ``/``}) + p := newTestProvider(hostFakes{"nas1": nas1, "nas2": nas2}, "nas1", "nas2") + + got, err := p.ListByTag(context.Background()) + if err != nil { + t.Fatalf("ListByTag: %v", err) + } + want := []provider.Instance{ + {ID: "hosts/nas1/domains/proxy-one", IP: "192.168.1.10", State: provider.StateRunning, UID: "uid-1", CreatedAt: testNow}, + {ID: "hosts/nas2/domains/proxy-two", State: provider.StateStopped, UID: "uid-2", CreatedAt: testNow}, + } + if len(got) != len(want) { + t.Fatalf("got %d instances %+v, want %d", len(got), got, len(want)) + } + for i := range want { + if got[i] != want[i] { + t.Errorf("instance[%d] = %+v, want %+v", i, got[i], want[i]) + } + } +} + +func TestListByTag_oneHostDownFailsSweep(t *testing.T) { + t.Parallel() + nas1 := readyHost() + managedDomain(t, nas1, "proxy-one", "uid-1", true, nil) + p := newTestProvider(hostFakes{"nas1": nas1}, "nas1", "nas2") + + got, err := p.ListByTag(context.Background()) + wantClass(t, err, provider.ErrTransient) + if got != nil { + t.Errorf("got %+v alongside the error, want nil (no partial view)", got) + } +} + +// A domain that cannot be read is left out of the sweep rather than +// failing it: GC never deletes what it does not see, so skipping is the +// safe direction, while one bad domain blocking GC for all others is not. +func TestListByTag_skipsUnreadableDomain(t *testing.T) { + t.Parallel() + cases := []struct { + name string + setup func(t *testing.T, api *fakeAPI) + }{ + { + "deleted mid-sweep", + func(t *testing.T, api *fakeAPI) { + managedDomain(t, api, "proxy-one", "uid-1", true, nil) + api.failOnce["DomainGetMetadata"] = lvError(libvirt.ErrNoDomain, "gone") + }, + }, + { + "corrupt metadata", + func(_ *testing.T, api *fakeAPI) { + api.putDomain(&fakeDomain{name: "proxy-one", metadata: ". libvirt has no labels or tags, but it stores any namespaced XML +// there and hands it back through DomainGetMetadata, which is how the GC +// contract (managed flag + owning UID) and the creation time survive across +// operator restarts. +const metadataURI = "https://gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/kvm" + +type instanceMetadata struct { + XMLName xml.Name `xml:"https://gitea.home.hrajfrisbee.cz/kacerr/egress-proxies-operator/kvm instance"` + Managed string `xml:"managed"` + UID string `xml:"uid"` + CreatedAt string `xml:"createdAt"` +} + +func newInstanceMetadata(uid string, createdAt time.Time) instanceMetadata { + return instanceMetadata{ + Managed: provider.LabelManagedYes, + UID: uid, + CreatedAt: createdAt.UTC().Format(time.RFC3339), + } +} + +// parseInstanceMetadata decodes the element as DomainGetMetadata returns it. +// libvirt selects the element by namespace URI but serialises it without the +// declaration (observed on libvirt 11.6: a bare ), and other +// versions may keep a default or prefixed form. The URI is already checked +// by the lookup, so only the local name is matched here. +func parseInstanceMetadata(s string) (instanceMetadata, error) { + var raw struct { + XMLName xml.Name `xml:"instance"` + instanceMetadata + } + if err := xml.Unmarshal([]byte(s), &raw); err != nil { + return instanceMetadata{}, fmt.Errorf("parsing domain metadata: %w", err) + } + return raw.instanceMetadata, nil +} + +func (m instanceMetadata) managed() bool { + return m.Managed == provider.LabelManagedYes +} + +// createdAt returns the zero time on a malformed timestamp: orphan GC treats +// zero as "old", so a corrupt value can never protect a domain forever. +func (m instanceMetadata) createdAt() time.Time { + t, err := time.Parse(time.RFC3339, m.CreatedAt) + if err != nil { + return time.Time{} + } + return t +} diff --git a/internal/provider/kvm/metadata_test.go b/internal/provider/kvm/metadata_test.go new file mode 100644 index 0000000..3f40920 --- /dev/null +++ b/internal/provider/kvm/metadata_test.go @@ -0,0 +1,84 @@ +package kvm + +import ( + "encoding/xml" + "strings" + "testing" + "time" +) + +func TestInstanceMetadata_roundTrip(t *testing.T) { + t.Parallel() + created := time.Date(2026, 9, 4, 12, 30, 0, 0, time.FixedZone("CEST", 2*3600)) + raw, err := xml.Marshal(newInstanceMetadata("uid-1", created)) + if err != nil { + t.Fatalf("Marshal() error = %v", err) + } + if !strings.Contains(string(raw), `xmlns="`+metadataURI+`"`) { + t.Errorf("marshalled metadata lacks the namespace declaration: %s", raw) + } + m, err := parseInstanceMetadata(string(raw)) + if err != nil { + t.Fatalf("parseInstanceMetadata() error = %v", err) + } + if !m.managed() || m.UID != "uid-1" { + t.Errorf("parsed = %+v, want managed with uid-1", m) + } + if got := m.createdAt(); !got.Equal(created) || got.Location() != time.UTC { + t.Errorf("createdAt() = %v, want %v in UTC", got, created) + } +} + +func TestParseInstanceMetadata(t *testing.T) { + t.Parallel() + tests := []struct { + name string + in string + wantErr bool + wantManaged bool + wantUID string + wantCreated time.Time + }{ + { + // libvirt re-serialises the stored element with whatever prefix it + // chooses; only the namespace URI is contractual. + name: "libvirt prefixed form", + in: `` + + `trueuid-2` + + `2026-09-04T10:00:00Z`, + wantManaged: true, wantUID: "uid-2", wantCreated: time.Date(2026, 9, 4, 10, 0, 0, 0, time.UTC), + }, + { + // What libvirt 11.6 actually returns from DomainGetMetadata: the + // element selected by our URI, serialised without any namespace + // declaration. + name: "libvirt bare form", + in: `trueuid-3` + + `2026-09-10T07:30:00Z`, + wantManaged: true, wantUID: "uid-3", wantCreated: time.Date(2026, 9, 10, 7, 30, 0, 0, time.UTC), + }, + {name: "managed false", in: `falseu`, wantUID: "u"}, + {name: "foreign element", in: `true`, wantErr: true}, + {name: "garbage", in: `not xml`, wantErr: true}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + m, err := parseInstanceMetadata(tc.in) + if (err != nil) != tc.wantErr { + t.Fatalf("error = %v, wantErr %v", err, tc.wantErr) + } + if m.managed() != tc.wantManaged || m.UID != tc.wantUID || !m.createdAt().Equal(tc.wantCreated) { + t.Errorf("parsed = %+v, want managed=%v uid=%q createdAt=%v", m, tc.wantManaged, tc.wantUID, tc.wantCreated) + } + }) + } +} + +func TestInstanceMetadata_createdAtMalformedIsZero(t *testing.T) { + t.Parallel() + m := instanceMetadata{CreatedAt: "yesterday"} + if !m.createdAt().IsZero() { + t.Errorf("createdAt() = %v, want zero time", m.createdAt()) + } +} diff --git a/internal/provider/kvm/seed.go b/internal/provider/kvm/seed.go new file mode 100644 index 0000000..7ba7e13 --- /dev/null +++ b/internal/provider/kvm/seed.go @@ -0,0 +1,133 @@ +package kvm + +import ( + "bytes" + "fmt" + "io" + "mime/multipart" + "net/textproto" + "strings" + + "github.com/kdomanski/iso9660" +) + +// seedLabel is the volume label cloud-init's NoCloud datasource looks for. +const seedLabel = "cidata" + +// guestAgentScript installs and starts the QEMU guest agent, which is how +// the provider learns a VM's address before the host's ARP table has seen +// any traffic from it. +// +// It ships as vendor-data, and as a *script* rather than a cloud-config, +// because that is the only shape cloud-init cannot drop. Vendor-data +// cloud-config is merged under the Proxy's own user-data with mergers that +// 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 +// agent. The strategy cannot be overridden from either document: merge_how +// in user-data governs only the first source, and merge_how in vendor-data +// is consumed and discarded before the document is stored. A vendor-data +// script bypasses 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. +const guestAgentScript = `#!/bin/sh +# 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 +` + +// buildVendorData renders the seed's vendor-data. With nothing configured +// it is the guest-agent script alone; with both, a MIME multipart carrying +// the configured document and the script as separate parts, since +// vendor-data is a single file and the two must not be concatenated. +func buildVendorData(configured string, installAgent bool) (string, error) { + switch { + case !installAgent: + return configured, nil + case strings.TrimSpace(configured) == "": + return guestAgentScript, nil + default: + return multipartVendorData(configured, guestAgentScript) + } +} + +// 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) + } + } + 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, +// and a meta-data whose instance-id is the deterministic VM name, so +// cloud-init never mistakes a reboot for a new instance. The ISO's volume +// descriptor carries the build time, so two builds differ in a few +// timestamp bytes; the files and label are identical. +func buildSeedISO(name, userData, vendorData string) ([]byte, error) { + w, err := iso9660.NewWriter() + if err != nil { + return nil, fmt.Errorf("creating iso writer: %w", err) + } + defer w.Cleanup() //nolint:errcheck // temp-dir cleanup; nothing to do on failure + + files := []struct{ name, body string }{ + {"user-data", userData}, + {"meta-data", fmt.Sprintf("instance-id: %s\nlocal-hostname: %s\n", name, name)}, + {"vendor-data", vendorData}, + } + for _, f := range files { + if err := w.AddFile(strings.NewReader(f.body), f.name); err != nil { + return nil, fmt.Errorf("adding %s to seed iso: %w", f.name, err) + } + } + var buf bytes.Buffer + if err := w.WriteTo(&buf, seedLabel); err != nil { + return nil, fmt.Errorf("writing seed iso: %w", err) + } + return buf.Bytes(), nil +} diff --git a/internal/provider/kvm/seed_test.go b/internal/provider/kvm/seed_test.go new file mode 100644 index 0000000..66caa50 --- /dev/null +++ b/internal/provider/kvm/seed_test.go @@ -0,0 +1,233 @@ +package kvm + +import ( + "bytes" + "io" + "maps" + "mime" + "mime/multipart" + "strings" + "testing" + + "github.com/kdomanski/iso9660" +) + +func readSeed(t *testing.T, iso []byte) (label string, files map[string]string) { + t.Helper() + img, err := iso9660.OpenImage(bytes.NewReader(iso)) + if err != nil { + t.Fatalf("OpenImage() error = %v", err) + } + label, err = img.Label() + if err != nil { + t.Fatalf("Label() error = %v", err) + } + root, err := img.RootDir() + if err != nil { + t.Fatalf("RootDir() error = %v", err) + } + children, err := root.GetChildren() + if err != nil { + t.Fatalf("GetChildren() error = %v", err) + } + files = make(map[string]string, len(children)) + for _, c := range children { + body, err := io.ReadAll(c.Reader()) + if err != nil { + t.Fatalf("reading %s: %v", c.Name(), err) + } + files[c.Name()] = string(body) + } + return label, files +} + +func TestBuildSeedISO_contents(t *testing.T) { + t.Parallel() + userData := "#cloud-config\npackages: [squid]\n" + iso, err := buildSeedISO("proxy-abc123def456ghij", userData, guestAgentScript) + if err != nil { + t.Fatalf("buildSeedISO() error = %v", err) + } + label, files := readSeed(t, iso) + if !strings.EqualFold(label, seedLabel) { + t.Errorf("label = %q, want %q", label, seedLabel) + } + if len(files) != 3 { + t.Errorf("files = %v, want exactly user-data, meta-data, vendor-data", files) + } + if files["user-data"] != userData { + t.Errorf("user-data = %q, want verbatim cloud-init", files["user-data"]) + } + 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 { + t.Errorf("meta-data = %q, want %q", got, want) + } +} + +// The writer stamps the build time into the volume descriptor, so two +// builds are not byte-identical; what must not change between them is +// what cloud-init reads: the label and the three files. +func TestBuildSeedISO_deterministicContent(t *testing.T) { + t.Parallel() + a, err := buildSeedISO("proxy-x", "#cloud-config\n", "") + if err != nil { + t.Fatalf("first buildSeedISO() error = %v", err) + } + b, err := buildSeedISO("proxy-x", "#cloud-config\n", "") + if err != nil { + t.Fatalf("second buildSeedISO() error = %v", err) + } + labelA, filesA := readSeed(t, a) + labelB, filesB := readSeed(t, b) + if labelA != labelB || !maps.Equal(filesA, filesB) { + t.Errorf("two builds for the same inputs differ: %q %v vs %q %v", labelA, filesA, labelB, filesB) + } + if len(a) != len(b) { + t.Errorf("sizes differ: %d vs %d; the seed volume is created with the ISO's exact size", len(a), len(b)) + } +} + +func TestBuildSeedISO_emptyUserData(t *testing.T) { + t.Parallel() + iso, err := buildSeedISO("proxy-x", "", guestAgentScript) + if err != nil { + t.Fatalf("buildSeedISO() error = %v", err) + } + _, files := readSeed(t, iso) + if body, ok := files["user-data"]; !ok || body != "" { + t.Errorf("user-data = %q (present=%v), want an empty file to still exist", body, ok) + } +} + +// The whole point of shipping the agent as vendor-data: whatever the Proxy +// wrote reaches the guest untouched, in any of cloud-init's formats. The +// previous implementation parsed and re-rendered cloud-config user-data, +// which dropped comments, reordered keys and re-typed unquoted scalars +// (version: 1.10 arrived as 1.1). +func TestBuildSeedISO_userDataIsNeverRewritten(t *testing.T) { + t.Parallel() + tests := []struct { + name string + userData string + }{ + {"cloud-config with its own packages", "#cloud-config\n# keep me\npackages:\n - squid\nversion: 1.10\nchpasswd: {list: 'u:0123456789'}\n"}, + {"jinja template", "## template: jinja\n#cloud-config\nhostname: {{ v1.local_hostname }}\n"}, + {"shell script", "#!/bin/sh\napt-get install -y squid\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() + vendor, err := buildVendorData("", true) + 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) + } +} diff --git a/internal/provider/kvm/trace.go b/internal/provider/kvm/trace.go new file mode 100644 index 0000000..a19a38d --- /dev/null +++ b/internal/provider/kvm/trace.go @@ -0,0 +1,211 @@ +package kvm + +import ( + "fmt" + "io" + "time" + + libvirt "github.com/digitalocean/go-libvirt" + "github.com/go-logr/logr" +) + +// tracingAPI wraps a libvirtAPI and logs one V(1) line per call after it +// returns: the libvirt API function (rpc=virDomainLookupByName, ...), the +// identifiers it was given, curated results, duration, and on failure the +// error with libvirt's code name. At V(2) the same line also carries XML +// bodies, metadata and address lists. Upload and download bodies are never +// logged: the seed ISO carries cloud-init. +type tracingAPI struct { + inner libvirtAPI + log logr.Logger +} + +type tracedCall struct { + t *tracingAPI + method string + start time.Time + kv []any +} + +// begin names the call by its libvirt API function: go-libvirt's method +// names are the remote procedure names, which are the public vir* API +// names without the prefix. +func (t *tracingAPI) begin(method string, kv ...any) *tracedCall { + return &tracedCall{t: t, method: "vir" + method, start: time.Now(), kv: kv} +} + +// detail adds key/values that are only worth the bytes at V(2). +func (c *tracedCall) detail(kv ...any) *tracedCall { + if c.t.log.V(2).Enabled() { + c.kv = append(c.kv, kv...) + } + return c +} + +func (c *tracedCall) done(err error, kv ...any) { + fields := make([]any, 0, 2+len(c.kv)+len(kv)+10) + fields = append(fields, "rpc", c.method) + fields = append(fields, c.kv...) + fields = append(fields, kv...) + fields = append(fields, "duration", time.Since(c.start).Round(time.Microsecond)) + if err != nil { + fields = append(fields, "error", err.Error()) + fields = append(fields, libvirtErrorFields(err)...) + } + c.t.log.V(1).Info("libvirt call", fields...) +} + +// flagKV renders a flags value only when something is set; go-libvirt's +// generated stringers do not know bitmask combinations. +func flagKV(v int64) []any { + if v == 0 { + return nil + } + return []any{"flags", v} +} + +func optString(s libvirt.OptString) string { + if len(s) == 0 { + return "" + } + return s[0] +} + +func (t *tracingAPI) DomainLookupByName(name string) (libvirt.Domain, error) { + c := t.begin("DomainLookupByName", "domain", name) + d, err := t.inner.DomainLookupByName(name) + c.done(err) + return d, err +} + +func (t *tracingAPI) DomainDefineXML(xml string) (libvirt.Domain, error) { + c := t.begin("DomainDefineXML", "xmlBytes", len(xml)).detail("xml", xml) + d, err := t.inner.DomainDefineXML(xml) + c.done(err, "domain", d.Name) + return d, err +} + +func (t *tracingAPI) DomainCreate(d libvirt.Domain) error { + c := t.begin("DomainCreate", "domain", d.Name) + err := t.inner.DomainCreate(d) + c.done(err) + return err +} + +func (t *tracingAPI) DomainDestroy(d libvirt.Domain) error { + c := t.begin("DomainDestroy", "domain", d.Name) + err := t.inner.DomainDestroy(d) + c.done(err) + return err +} + +func (t *tracingAPI) DomainUndefineFlags(d libvirt.Domain, flags libvirt.DomainUndefineFlagsValues) error { + c := t.begin("DomainUndefineFlags", append([]any{"domain", d.Name}, flagKV(int64(flags))...)...) + err := t.inner.DomainUndefineFlags(d, flags) + c.done(err) + return err +} + +func (t *tracingAPI) DomainGetState(d libvirt.Domain, flags uint32) (int32, int32, error) { + c := t.begin("DomainGetState", "domain", d.Name) + state, reason, err := t.inner.DomainGetState(d, flags) + c.done(err, "state", libvirt.DomainState(state), "reason", reason) + return state, reason, err +} + +func (t *tracingAPI) DomainGetMetadata(d libvirt.Domain, typ int32, uri libvirt.OptString, flags libvirt.DomainModificationImpact) (string, error) { + c := t.begin("DomainGetMetadata", "domain", d.Name, "type", typ, "uri", optString(uri)) + s, err := t.inner.DomainGetMetadata(d, typ, uri, flags) + c.detail("metadata", s).done(err, "metadataBytes", len(s)) + return s, err +} + +func (t *tracingAPI) DomainInterfaceAddresses(d libvirt.Domain, source uint32, flags uint32) ([]libvirt.DomainInterface, error) { + c := t.begin("DomainInterfaceAddresses", "domain", d.Name, "source", libvirt.DomainInterfaceAddressesSource(source)) + ifaces, err := t.inner.DomainInterfaceAddresses(d, source, flags) + if t.log.V(2).Enabled() { + c.detail("addresses", formatAddresses(ifaces)) + } + c.done(err, "interfaces", len(ifaces)) + return ifaces, err +} + +func formatAddresses(ifaces []libvirt.DomainInterface) []string { + var out []string + for _, iface := range ifaces { + for _, a := range iface.Addrs { + out = append(out, fmt.Sprintf("%s=%s/%d", iface.Name, a.Addr, a.Prefix)) + } + } + return out +} + +func (t *tracingAPI) ConnectListAllDomains(needResults int32, flags libvirt.ConnectListAllDomainsFlags) ([]libvirt.Domain, uint32, error) { + c := t.begin("ConnectListAllDomains", flagKV(int64(flags))...) + doms, n, err := t.inner.ConnectListAllDomains(needResults, flags) + if t.log.V(2).Enabled() { + names := make([]string, 0, len(doms)) + for _, d := range doms { + names = append(names, d.Name) + } + c.detail("names", names) + } + c.done(err, "count", len(doms)) + return doms, n, err +} + +func (t *tracingAPI) StoragePoolLookupByName(name string) (libvirt.StoragePool, error) { + c := t.begin("StoragePoolLookupByName", "pool", name) + pool, err := t.inner.StoragePoolLookupByName(name) + c.done(err) + return pool, err +} + +func (t *tracingAPI) StoragePoolRefresh(pool libvirt.StoragePool, flags uint32) error { + c := t.begin("StoragePoolRefresh", "pool", pool.Name) + err := t.inner.StoragePoolRefresh(pool, flags) + c.done(err) + return err +} + +func (t *tracingAPI) StorageVolLookupByName(pool libvirt.StoragePool, name string) (libvirt.StorageVol, error) { + c := t.begin("StorageVolLookupByName", "pool", pool.Name, "volume", name) + vol, err := t.inner.StorageVolLookupByName(pool, name) + c.done(err) + return vol, err +} + +func (t *tracingAPI) StorageVolGetPath(vol libvirt.StorageVol) (string, error) { + c := t.begin("StorageVolGetPath", "pool", vol.Pool, "volume", vol.Name) + path, err := t.inner.StorageVolGetPath(vol) + c.done(err, "path", path) + return path, err +} + +func (t *tracingAPI) StorageVolCreateXML(pool libvirt.StoragePool, xml string, flags libvirt.StorageVolCreateFlags) (libvirt.StorageVol, error) { + c := t.begin("StorageVolCreateXML", append([]any{"pool", pool.Name, "xmlBytes", len(xml)}, flagKV(int64(flags))...)...).detail("xml", xml) + vol, err := t.inner.StorageVolCreateXML(pool, xml, flags) + c.done(err, "volume", vol.Name) + return vol, err +} + +func (t *tracingAPI) StorageVolUpload(vol libvirt.StorageVol, r io.Reader, offset, length uint64, flags libvirt.StorageVolUploadFlags) error { + c := t.begin("StorageVolUpload", append([]any{"pool", vol.Pool, "volume", vol.Name, "offset", offset, "length", length}, flagKV(int64(flags))...)...) + err := t.inner.StorageVolUpload(vol, r, offset, length, flags) + c.done(err) + return err +} + +func (t *tracingAPI) StorageVolDownload(vol libvirt.StorageVol, w io.Writer, offset, length uint64, flags libvirt.StorageVolDownloadFlags) error { + c := t.begin("StorageVolDownload", append([]any{"pool", vol.Pool, "volume", vol.Name, "offset", offset, "length", length}, flagKV(int64(flags))...)...) + err := t.inner.StorageVolDownload(vol, w, offset, length, flags) + c.done(err) + return err +} + +func (t *tracingAPI) StorageVolDelete(vol libvirt.StorageVol, flags libvirt.StorageVolDeleteFlags) error { + c := t.begin("StorageVolDelete", append([]any{"pool", vol.Pool, "volume", vol.Name}, flagKV(int64(flags))...)...) + err := t.inner.StorageVolDelete(vol, flags) + c.done(err) + return err +} diff --git a/internal/provider/kvm/trace_test.go b/internal/provider/kvm/trace_test.go new file mode 100644 index 0000000..0a11a30 --- /dev/null +++ b/internal/provider/kvm/trace_test.go @@ -0,0 +1,300 @@ +package kvm + +import ( + "bytes" + "context" + "errors" + "io" + "reflect" + "strings" + "sync" + "testing" + + libvirt "github.com/digitalocean/go-libvirt" + "github.com/go-logr/logr" + "github.com/go-logr/logr/funcr" +) + +// captureLogger returns a funcr logger capped at verbosity (the test +// stand-in for --zap-log-level=) and a snapshot function for the +// lines it recorded. The sink is mutex-guarded: a cancelled call logs from +// the abandoned goroutine. +func captureLogger(verbosity int) (logr.Logger, func() []string) { + var mu sync.Mutex + var lines []string + log := funcr.New(func(prefix, args string) { + mu.Lock() + defer mu.Unlock() + lines = append(lines, prefix+" "+args) + }, funcr.Options{Verbosity: verbosity}) + return log, func() []string { + mu.Lock() + defer mu.Unlock() + return append([]string(nil), lines...) + } +} + +func captureContext(verbosity int) (context.Context, func() []string) { + log, snapshot := captureLogger(verbosity) + return logr.NewContext(context.Background(), log), snapshot +} + +const stoppedName = "proxy-stopped" + +// tracedHost is a fake with one managed running domain and its volumes, so +// every method of the seam has something to act on. +func tracedHost(t *testing.T) *fakeAPI { + t.Helper() + api := readyHost() + managedDomain(t, api, testName, testUID, true, map[uint32][]libvirt.DomainInterface{ + uint32(libvirt.DomainInterfaceAddressesSrcArp): {{Name: "vnet0", Addrs: []libvirt.DomainIPAddr{ipv4("192.168.1.50")}}}, + }) + api.putDomain(&fakeDomain{name: stoppedName}) + return api +} + +func TestTracingAPI_logsEveryMethod(t *testing.T) { + t.Parallel() + pool := libvirt.StoragePool{Name: "default"} + dom := libvirt.Domain{Name: testName} + overlay := libvirt.StorageVol{Pool: "default", Name: overlayVolumeName(testName)} + cases := []struct { + method string + call func(api libvirtAPI) error + }{ + {"DomainLookupByName", func(api libvirtAPI) error { _, err := api.DomainLookupByName(testName); return err }}, + {"DomainDefineXML", func(api libvirtAPI) error { + xml, err := buildDomainXML(testDomainParams()) + if err != nil { + return err + } + _, err = api.DomainDefineXML(xml) + return err + }}, + {"DomainCreate", func(api libvirtAPI) error { return api.DomainCreate(libvirt.Domain{Name: stoppedName}) }}, + {"DomainDestroy", func(api libvirtAPI) error { return api.DomainDestroy(dom) }}, + {"DomainUndefineFlags", func(api libvirtAPI) error { return api.DomainUndefineFlags(dom, undefineFlags) }}, + {"DomainGetState", func(api libvirtAPI) error { _, _, err := api.DomainGetState(dom, 0); return err }}, + {"DomainGetMetadata", func(api libvirtAPI) error { + _, err := api.DomainGetMetadata(dom, int32(libvirt.DomainMetadataElement), libvirt.OptString{metadataURI}, 0) + return err + }}, + {"DomainInterfaceAddresses", func(api libvirtAPI) error { + _, err := api.DomainInterfaceAddresses(dom, uint32(libvirt.DomainInterfaceAddressesSrcArp), 0) + return err + }}, + {"ConnectListAllDomains", func(api libvirtAPI) error { _, _, err := api.ConnectListAllDomains(1, 0); return err }}, + {"StoragePoolLookupByName", func(api libvirtAPI) error { _, err := api.StoragePoolLookupByName("default"); return err }}, + {"StoragePoolRefresh", func(api libvirtAPI) error { return api.StoragePoolRefresh(pool, 0) }}, + {"StorageVolLookupByName", func(api libvirtAPI) error { _, err := api.StorageVolLookupByName(pool, overlay.Name); return err }}, + {"StorageVolGetPath", func(api libvirtAPI) error { _, err := api.StorageVolGetPath(overlay); return err }}, + {"StorageVolCreateXML", func(api libvirtAPI) error { + xml, err := buildVolumeXML(rawVolume("new.iso", 3)) + if err != nil { + return err + } + _, err = api.StorageVolCreateXML(pool, xml, 0) + return err + }}, + {"StorageVolUpload", func(api libvirtAPI) error { + return api.StorageVolUpload(overlay, strings.NewReader("abc"), 0, 3, 0) + }}, + {"StorageVolDownload", func(api libvirtAPI) error { + return api.StorageVolDownload(overlay, io.Discard, 0, 0, 0) + }}, + {"StorageVolDelete", func(api libvirtAPI) error { return api.StorageVolDelete(overlay, 0) }}, + } + if n := reflect.TypeOf((*libvirtAPI)(nil)).Elem().NumMethod(); n != len(cases) { + t.Fatalf("libvirtAPI has %d methods, this test covers %d; trace the new one", n, len(cases)) + } + for _, tc := range cases { + t.Run(tc.method, func(t *testing.T) { + t.Parallel() + log, snapshot := captureLogger(1) + api := &tracingAPI{inner: tracedHost(t), log: log} + if err := tc.call(api); err != nil { + t.Fatalf("%s: %v", tc.method, err) + } + lines := snapshot() + if len(lines) != 1 { + t.Fatalf("logged %d lines, want exactly 1:\n%s", len(lines), strings.Join(lines, "\n")) + } + for _, want := range []string{`"msg"="libvirt call"`, `"rpc"="vir` + tc.method + `"`, `"duration"=`} { + if !strings.Contains(lines[0], want) { + t.Errorf("line missing %s:\n%s", want, lines[0]) + } + } + if strings.Contains(lines[0], `"error"=`) { + t.Errorf("successful call logged an error:\n%s", lines[0]) + } + }) + } +} + +func TestTracingAPI_passesResultsThrough(t *testing.T) { + t.Parallel() + inner := tracedHost(t) + log, _ := captureLogger(1) + traced := &tracingAPI{inner: inner, log: log} + dom := libvirt.Domain{Name: testName} + + wantState, wantReason, err := inner.DomainGetState(dom, 0) + if err != nil { + t.Fatal(err) + } + state, reason, err := traced.DomainGetState(dom, 0) + if err != nil || state != wantState || reason != wantReason { + t.Errorf("DomainGetState = %d/%d/%v, want %d/%d/nil", state, reason, err, wantState, wantReason) + } + + overlay := libvirt.StorageVol{Pool: "default", Name: overlayVolumeName(testName)} + wantPath, _ := inner.StorageVolGetPath(overlay) + if path, err := traced.StorageVolGetPath(overlay); err != nil || path != wantPath { + t.Errorf("StorageVolGetPath = %q/%v, want %q", path, err, wantPath) + } + + wantDoms, _, _ := inner.ConnectListAllDomains(1, 0) + doms, _, err := traced.ConnectListAllDomains(1, 0) + if err != nil || !reflect.DeepEqual(doms, wantDoms) { + t.Errorf("ConnectListAllDomains = %v/%v, want %v", doms, err, wantDoms) + } + + inner.fail["DomainCreate"] = lvError(libvirt.ErrOperationInvalid, "already running") + if err := traced.DomainCreate(dom); !hasCode(err, libvirt.ErrOperationInvalid) { + t.Errorf("DomainCreate error = %v, want the injected libvirt error unchanged", err) + } +} + +func TestTracingAPI_errorFields(t *testing.T) { + t.Parallel() + tests := []struct { + name string + inject error + want []string + notWant []string + }{ + { + name: "libvirt error carries code, name and message", + inject: lvError(libvirt.ErrNoDomain, "Domain not found: no domain with matching name"), + want: []string{`"error"="Domain not found`, `"libvirtError"="ErrNoDomain"`, `"libvirtCode"=42`}, + }, + { + name: "transport error has no libvirt fields", + inject: io.EOF, + want: []string{`"error"="EOF"`}, + notWant: []string{`"libvirtError"`, `"libvirtCode"`}, + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + inner := tracedHost(t) + inner.fail["DomainLookupByName"] = tc.inject + log, snapshot := captureLogger(1) + api := &tracingAPI{inner: inner, log: log} + if _, err := api.DomainLookupByName(testName); !errors.Is(err, tc.inject) { + t.Fatalf("error = %v, want the injected one", err) + } + joined := strings.Join(snapshot(), "\n") + for _, want := range tc.want { + if !strings.Contains(joined, want) { + t.Errorf("output missing %s:\n%s", want, joined) + } + } + for _, absent := range tc.notWant { + if strings.Contains(joined, absent) { + t.Errorf("output unexpectedly contains %s:\n%s", absent, joined) + } + } + }) + } +} + +func TestTracingAPI_detailOnlyAtV2(t *testing.T) { + t.Parallel() + dom := libvirt.Domain{Name: testName} + calls := []struct { + method string + call func(api libvirtAPI) error + always string + detail string + }{ + {"DomainDefineXML", func(api libvirtAPI) error { + xml, err := buildDomainXML(testDomainParams()) + if err != nil { + return err + } + _, err = api.DomainDefineXML(xml) + return err + }, `"xmlBytes"=`, `"xml"="= 2) { + t.Errorf("v%d output contains %s = %v, want %v:\n%s", verbosity, tc.detail, has, verbosity >= 2, joined) + } + }) + } + } +} + +func TestTracingAPI_uploadLogsSizeNotContent(t *testing.T) { + t.Parallel() + const sentinel = "SENTINEL-seed-content-must-never-be-logged" + log, snapshot := captureLogger(2) + api := &tracingAPI{inner: tracedHost(t), log: log} + vol := libvirt.StorageVol{Pool: "default", Name: seedVolumeName(testName)} + if err := api.StorageVolUpload(vol, bytes.NewReader([]byte(sentinel)), 0, uint64(len(sentinel)), 0); err != nil { + t.Fatal(err) + } + joined := strings.Join(snapshot(), "\n") + if !strings.Contains(joined, `"length"=42`) { + t.Errorf("upload line missing the length:\n%s", joined) + } + if strings.Contains(joined, sentinel) { + t.Errorf("upload content leaked into logs:\n%s", joined) + } +} + +func TestTracingAPI_silentBelowV1(t *testing.T) { + t.Parallel() + log, snapshot := captureLogger(0) + api := &tracingAPI{inner: tracedHost(t), log: log} + if _, err := api.DomainLookupByName(testName); err != nil { + t.Fatal(err) + } + if lines := snapshot(); len(lines) != 0 { + t.Errorf("verbosity 0 logged %d lines:\n%s", len(lines), strings.Join(lines, "\n")) + } +} diff --git a/internal/provider/kvm/volume.go b/internal/provider/kvm/volume.go new file mode 100644 index 0000000..da0814d --- /dev/null +++ b/internal/provider/kvm/volume.go @@ -0,0 +1,78 @@ +package kvm + +import ( + "encoding/xml" + "fmt" +) + +const bytesPerGiB = 1 << 30 + +func overlayVolumeName(name string) string { return name + ".qcow2" } + +func seedVolumeName(name string) string { return name + "-seed.iso" } + +// volumeSpec describes a storage volume to create. A non-empty BackingPath +// makes it a copy-on-write overlay of that file. +type volumeSpec struct { + Name string + SizeBytes int64 + Format string + BackingPath string + BackingFormat string +} + +func overlayVolume(name string, sizeGB int64, basePath, baseFormat string) volumeSpec { + return volumeSpec{ + Name: overlayVolumeName(name), + SizeBytes: sizeGB * bytesPerGiB, + Format: formatQcow2, + BackingPath: basePath, + BackingFormat: baseFormat, + } +} + +func rawVolume(name string, sizeBytes int64) volumeSpec { + return volumeSpec{Name: name, SizeBytes: sizeBytes, Format: formatRaw} +} + +// buildVolumeXML renders the volume for StorageVolCreateXML. Capacity is +// always given in bytes so no libvirt unit-suffix ambiguity (G vs GiB) can +// creep in. +func buildVolumeXML(v volumeSpec) (string, error) { + x := volumeXML{ + Type: "file", + Name: v.Name, + Capacity: sizeXML{Unit: "bytes", Value: v.SizeBytes}, + Target: volumeTargetXML{Format: formatXML{Type: v.Format}}, + } + if v.BackingPath != "" { + x.BackingStore = &backingStoreXML{Path: v.BackingPath, Format: formatXML{Type: v.BackingFormat}} + } + out, err := xml.Marshal(x) + if err != nil { + return "", fmt.Errorf("marshalling volume xml: %w", err) + } + return string(out), nil +} + +type volumeXML struct { + XMLName xml.Name `xml:"volume"` + Type string `xml:"type,attr"` + Name string `xml:"name"` + Capacity sizeXML `xml:"capacity"` + Target volumeTargetXML `xml:"target"` + BackingStore *backingStoreXML `xml:"backingStore"` +} + +type volumeTargetXML struct { + Format formatXML `xml:"format"` +} + +type formatXML struct { + Type string `xml:"type,attr"` +} + +type backingStoreXML struct { + Path string `xml:"path"` + Format formatXML `xml:"format"` +} diff --git a/internal/provider/kvm/volume_test.go b/internal/provider/kvm/volume_test.go new file mode 100644 index 0000000..c9b68a8 --- /dev/null +++ b/internal/provider/kvm/volume_test.go @@ -0,0 +1,62 @@ +package kvm + +import ( + "encoding/xml" + "strings" + "testing" +) + +func TestVolumeNames(t *testing.T) { + t.Parallel() + if got := overlayVolumeName("proxy-x"); got != "proxy-x.qcow2" { + t.Errorf("overlayVolumeName = %q", got) + } + if got := seedVolumeName("proxy-x"); got != "proxy-x-seed.iso" { + t.Errorf("seedVolumeName = %q", got) + } +} + +func TestBuildVolumeXML_overlay(t *testing.T) { + t.Parallel() + raw, err := buildVolumeXML(overlayVolume("proxy-x", 10, "/var/lib/libvirt/images/base-debian-13.qcow2", "qcow2")) + if err != nil { + t.Fatalf("buildVolumeXML() error = %v", err) + } + var v volumeXML + if err := xml.Unmarshal([]byte(raw), &v); err != nil { + t.Fatalf("output is not well-formed XML: %v", err) + } + if v.Type != "file" || v.Name != "proxy-x.qcow2" { + t.Errorf("type/name = %s/%s", v.Type, v.Name) + } + if v.Capacity.Unit != "bytes" || v.Capacity.Value != 10*bytesPerGiB { + t.Errorf("capacity = %d %s, want %d bytes", v.Capacity.Value, v.Capacity.Unit, 10*bytesPerGiB) + } + if v.Target.Format.Type != "qcow2" { + t.Errorf("format = %q", v.Target.Format.Type) + } + if v.BackingStore == nil { + t.Fatalf("backingStore missing:\n%s", raw) + } + if v.BackingStore.Path != "/var/lib/libvirt/images/base-debian-13.qcow2" || v.BackingStore.Format.Type != "qcow2" { + t.Errorf("backingStore = %+v", v.BackingStore) + } +} + +func TestBuildVolumeXML_rawHasNoBackingStore(t *testing.T) { + t.Parallel() + raw, err := buildVolumeXML(rawVolume("proxy-x-seed.iso", 372736)) + if err != nil { + t.Fatalf("buildVolumeXML() error = %v", err) + } + if strings.Contains(raw, "backingStore") { + t.Errorf("raw volume must not declare a backing store:\n%s", raw) + } + var v volumeXML + if err := xml.Unmarshal([]byte(raw), &v); err != nil { + t.Fatalf("output is not well-formed XML: %v", err) + } + if v.Name != "proxy-x-seed.iso" || v.Capacity.Value != 372736 || v.Target.Format.Type != "raw" { + t.Errorf("volume = %+v", v) + } +}