From cd51886139de4eb0140c6a7fb312f2ff031f0551 Mon Sep 17 00:00:00 2001 From: kacerr Date: Thu, 10 Sep 2026 07:37:51 +0000 Subject: [PATCH] Reference KVM domain disks by host path so AppArmor-confined QEMU can open them The first run against a real hypervisor (beelink, Ubuntu, libvirt 11.6) failed at DomainCreate with "Could not open base image: Permission denied" even with correct DAC ownership. libvirt's virt-aa-helper only grants QEMU access to disk sources that carry a path; the form chosen in Step 2 gives it pool and volume names instead, so the overlay and its backing image get no AppArmor rule on any stock Debian/Ubuntu host. buildDomainXML now takes DiskPath/SeedPath and emits type="file" disks. ensureOverlay and writeSeed return the volume handles and Create resolves both paths with StorageVolGetPath before defining the domain. The domain XML test rejects any volume/pool reference so this cannot drift back; the Create test asserts the sources equal the paths libvirt reported. Execution log: Step 2 note marked reverted, Step 7 entry opened with the two failed runs and the diagnosis. Co-Authored-By: Claude Fable 5.1 --- .../2026-09-04-1404-kvm-provider.md | 56 +++++++++++++++++++ internal/provider/kvm/domainxml.go | 32 ++++++----- internal/provider/kvm/domainxml_test.go | 16 +++--- internal/provider/kvm/kvm.go | 54 +++++++++++------- internal/provider/kvm/kvm_test.go | 7 +++ 5 files changed, 125 insertions(+), 40 deletions(-) diff --git a/docs/plans-executions/2026-09-04-1404-kvm-provider.md b/docs/plans-executions/2026-09-04-1404-kvm-provider.md index a256fa4..e9dc2bb 100644 --- a/docs/plans-executions/2026-09-04-1404-kvm-provider.md +++ b/docs/plans-executions/2026-09-04-1404-kvm-provider.md @@ -69,6 +69,9 @@ Two design choices differ from the plan text: ``, 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. @@ -389,3 +392,56 @@ Worth noting: the CRD has no rule requiring all three placement fields 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. diff --git a/internal/provider/kvm/domainxml.go b/internal/provider/kvm/domainxml.go index 32f0732..bfdcfe4 100644 --- a/internal/provider/kvm/domainxml.go +++ b/internal/provider/kvm/domainxml.go @@ -9,24 +9,29 @@ import ( const guestAgentChannel = "org.qemu.guest_agent.0" // domainParams is everything buildDomainXML needs, already resolved from the -// config and placement so the builder stays a pure function. +// config and placement so the builder stays a pure function. DiskPath and +// SeedPath are the volumes' host paths as reported by libvirt. type domainParams struct { Name string UID string CreatedAt time.Time VCPUs int32 MemoryMiB int64 - Pool string Bridge string - DiskVol string - SeedVol string + DiskPath string + SeedPath string } // buildDomainXML renders a q35 KVM guest: host-passthrough CPU, a virtio -// overlay disk and a SATA cdrom for the seed (both referenced as pool -// volumes so no host paths are needed), one virtio NIC on the host bridge, -// a serial console, the guest-agent channel, a virtio RNG, no graphics, and -// the operator's metadata element. +// overlay disk and a SATA cdrom for the seed, one virtio NIC on the host +// bridge, a serial console, the guest-agent channel, a virtio RNG, no +// graphics, and the operator's metadata element. +// +// Disks are type="file" with host paths, not type="volume" with pool/volume +// names: libvirt's AppArmor helper (virt-aa-helper) only grants QEMU access +// to sources that carry a path, so a volume-referenced disk fails to start +// with "Permission denied" on any AppArmor-confined host (stock Debian and +// Ubuntu). The paths cost one StorageVolGetPath round-trip each per Create. func buildDomainXML(p domainParams) (string, error) { d := domainXML{ Type: "kvm", @@ -47,17 +52,17 @@ func buildDomainXML(p domainParams) (string, error) { Devices: devicesXML{ Disks: []diskXML{ { - Type: "volume", + Type: "file", Device: "disk", Driver: driverXML{Name: "qemu", Type: formatQcow2}, - Source: diskSourceXML{Pool: p.Pool, Volume: p.DiskVol}, + Source: diskSourceXML{File: p.DiskPath}, Target: targetXML{Dev: "vda", Bus: "virtio"}, }, { - Type: "volume", + Type: "file", Device: "cdrom", Driver: driverXML{Name: "qemu", Type: formatRaw}, - Source: diskSourceXML{Pool: p.Pool, Volume: p.SeedVol}, + Source: diskSourceXML{File: p.SeedPath}, Target: targetXML{Dev: "sda", Bus: "sata"}, ReadOnly: &struct{}{}, }, @@ -161,8 +166,7 @@ type driverXML struct { } type diskSourceXML struct { - Pool string `xml:"pool,attr"` - Volume string `xml:"volume,attr"` + File string `xml:"file,attr"` } type targetXML struct { diff --git a/internal/provider/kvm/domainxml_test.go b/internal/provider/kvm/domainxml_test.go index d52474c..b401670 100644 --- a/internal/provider/kvm/domainxml_test.go +++ b/internal/provider/kvm/domainxml_test.go @@ -14,10 +14,9 @@ func testDomainParams() domainParams { CreatedAt: time.Date(2026, 9, 4, 12, 0, 0, 0, time.UTC), VCPUs: 2, MemoryMiB: 2048, - Pool: "default", Bridge: "br0", - DiskVol: "proxy-abc123def456ghij.qcow2", - SeedVol: "proxy-abc123def456ghij-seed.iso", + DiskPath: "/var/lib/libvirt/images/proxy-abc123def456ghij.qcow2", + SeedPath: "/var/lib/libvirt/images/proxy-abc123def456ghij-seed.iso", } } @@ -63,16 +62,19 @@ func TestBuildDomainXML_fieldByField(t *testing.T) { t.Fatalf("disks = %d, want 2", len(d.Devices.Disks)) } disk, seed := d.Devices.Disks[0], d.Devices.Disks[1] - if disk.Type != "volume" || disk.Device != "disk" || disk.Driver.Type != "qcow2" || - disk.Source.Pool != "default" || disk.Source.Volume != "proxy-abc123def456ghij.qcow2" || + if disk.Type != "file" || disk.Device != "disk" || disk.Driver.Type != "qcow2" || + disk.Source.File != "/var/lib/libvirt/images/proxy-abc123def456ghij.qcow2" || disk.Target.Dev != "vda" || disk.Target.Bus != "virtio" || disk.ReadOnly != nil { t.Errorf("overlay disk = %+v", disk) } - if seed.Type != "volume" || seed.Device != "cdrom" || seed.Driver.Type != "raw" || - seed.Source.Pool != "default" || seed.Source.Volume != "proxy-abc123def456ghij-seed.iso" || + if seed.Type != "file" || seed.Device != "cdrom" || seed.Driver.Type != "raw" || + seed.Source.File != "/var/lib/libvirt/images/proxy-abc123def456ghij-seed.iso" || seed.Target.Bus != "sata" || seed.ReadOnly == nil { t.Errorf("seed cdrom = %+v", seed) } + if strings.Contains(raw, `type="volume"`) || strings.Contains(raw, " pool=") { + t.Errorf("disks reference pool volumes; virt-aa-helper cannot grant QEMU access to those:\n%s", raw) + } if len(d.Devices.Interfaces) != 1 { t.Fatalf("interfaces = %d, want 1", len(d.Devices.Interfaces)) diff --git a/internal/provider/kvm/kvm.go b/internal/provider/kvm/kvm.go index e510d08..6d94dc4 100644 --- a/internal/provider/kvm/kvm.go +++ b/internal/provider/kvm/kvm.go @@ -183,13 +183,23 @@ func (p *Provider) Create(ctx context.Context, req provider.CreateRequest) (stri if err != nil { return err } - if err := p.ensureOverlay(log, api, pool, req.Name, basePath, src.Format); err != nil { + overlay, err := p.ensureOverlay(log, api, pool, req.Name, basePath, src.Format) + if err != nil { return err } - if err := p.writeSeed(log, api, pool, req.Name, req.CloudInit); err != nil { + seed, err := p.writeSeed(log, api, pool, req.Name, req.CloudInit) + if err != nil { return err } - dom, err = p.defineDomain(log, api, host, req, mt) + diskPath, err := api.StorageVolGetPath(overlay) + if err != nil { + return fmt.Errorf("resolving overlay path: %w", err) + } + seedPath, err := api.StorageVolGetPath(seed) + if err != nil { + return fmt.Errorf("resolving seed path: %w", err) + } + dom, err = p.defineDomain(log, api, host, req, mt, diskPath, seedPath) if err != nil { return err } @@ -203,60 +213,66 @@ func (p *Provider) Create(ctx context.Context, req provider.CreateRequest) (stri return id, nil } -func (p *Provider) ensureOverlay(log logr.Logger, api libvirtAPI, pool libvirt.StoragePool, name, basePath, baseFormat string) error { +// ensureOverlay creates the per-VM overlay volume, or returns the existing +// one: creation is atomic, so an overlay that exists is complete. +func (p *Provider) ensureOverlay(log logr.Logger, api libvirtAPI, pool libvirt.StoragePool, name, basePath, baseFormat string) (libvirt.StorageVol, error) { xml, err := buildVolumeXML(overlayVolume(name, p.cfg.DiskSizeGB, basePath, baseFormat)) if err != nil { - return err + return libvirt.StorageVol{}, err } - _, err = api.StorageVolCreateXML(pool, xml, 0) + vol, err := api.StorageVolCreateXML(pool, xml, 0) switch { case err == nil: log.V(1).Info("overlay volume created", "volume", overlayVolumeName(name), "base", basePath) + return vol, nil case hasCode(err, libvirt.ErrStorageVolExist): log.V(1).Info("overlay volume already exists", "volume", overlayVolumeName(name)) + vol, err := api.StorageVolLookupByName(pool, overlayVolumeName(name)) + if err != nil { + return libvirt.StorageVol{}, fmt.Errorf("looking up existing overlay volume: %w", err) + } + return vol, nil default: - return fmt.Errorf("creating overlay volume: %w", err) + return libvirt.StorageVol{}, fmt.Errorf("creating overlay volume: %w", err) } - return nil } // writeSeed replaces the seed volume with a freshly built NoCloud ISO. // Only called while no domain references it. -func (p *Provider) writeSeed(log logr.Logger, api libvirtAPI, pool libvirt.StoragePool, name, userData string) error { +func (p *Provider) writeSeed(log logr.Logger, api libvirtAPI, pool libvirt.StoragePool, name, userData string) (libvirt.StorageVol, error) { seedName := seedVolumeName(name) if err := deleteIfExists(api, pool, seedName); err != nil { - return err + return libvirt.StorageVol{}, err } iso, err := buildSeedISO(name, userData, p.cfg.VendorData) if err != nil { - return fmt.Errorf("%w: %v", provider.ErrPermanent, err) + return libvirt.StorageVol{}, fmt.Errorf("%w: %v", provider.ErrPermanent, err) } xml, err := buildVolumeXML(rawVolume(seedName, int64(len(iso)))) if err != nil { - return err + return libvirt.StorageVol{}, err } vol, err := api.StorageVolCreateXML(pool, xml, 0) if err != nil { - return fmt.Errorf("creating seed volume: %w", err) + return libvirt.StorageVol{}, fmt.Errorf("creating seed volume: %w", err) } if err := api.StorageVolUpload(vol, bytes.NewReader(iso), 0, 0, 0); err != nil { - return fmt.Errorf("uploading seed volume: %w", err) + return libvirt.StorageVol{}, fmt.Errorf("uploading seed volume: %w", err) } log.V(1).Info("seed volume uploaded", "volume", seedName, "bytes", len(iso)) - return nil + return vol, nil } -func (p *Provider) defineDomain(log logr.Logger, api libvirtAPI, host provider.KVMHost, req provider.CreateRequest, mt provider.KVMMachineType) (libvirt.Domain, error) { +func (p *Provider) defineDomain(log logr.Logger, api libvirtAPI, host provider.KVMHost, req provider.CreateRequest, mt provider.KVMMachineType, diskPath, seedPath string) (libvirt.Domain, error) { xml, err := buildDomainXML(domainParams{ Name: req.Name, UID: req.UID, CreatedAt: p.now(), VCPUs: mt.VCPUs, MemoryMiB: mt.MemoryMiB, - Pool: host.Pool, Bridge: host.Bridge, - DiskVol: overlayVolumeName(req.Name), - SeedVol: seedVolumeName(req.Name), + DiskPath: diskPath, + SeedPath: seedPath, }) if err != nil { return libvirt.Domain{}, err diff --git a/internal/provider/kvm/kvm_test.go b/internal/provider/kvm/kvm_test.go index c627c34..087c371 100644 --- a/internal/provider/kvm/kvm_test.go +++ b/internal/provider/kvm/kvm_test.go @@ -210,6 +210,13 @@ func TestCreate_freshHost(t *testing.T) { if d.Devices.Interfaces[0].Source.Bridge != "br0" { t.Errorf("bridge = %q, want br0", d.Devices.Interfaces[0].Source.Bridge) } + wantDisk, wantSeed := api.poolPath+"/"+overlayVolumeName(testName), api.poolPath+"/"+seedVolumeName(testName) + if got := d.Devices.Disks[0].Source.File; got != wantDisk { + t.Errorf("overlay disk source = %q, want the path libvirt reported, %q", got, wantDisk) + } + if got := d.Devices.Disks[1].Source.File; got != wantSeed { + t.Errorf("seed cdrom source = %q, want the path libvirt reported, %q", got, wantSeed) + } if m := d.Metadata.Instance; m.UID != testUID || !m.managed() || !m.createdAt().Equal(testNow) { t.Errorf("metadata = %+v, want managed, uid %s, createdAt %s", m, testUID, testNow) }