From 3322eb4d740f88cd29fd55204904696df2ef8c6b Mon Sep 17 00:00:00 2001 From: Hannu Valtonen Date: Wed, 2 Sep 2026 11:53:42 +0000 Subject: [PATCH] storage: add runtime USB disk hotplug Add an explicit XHCI controller device for macOS 15 and newer, then expose runtime storage operations over vfkit's Unix-domain REST endpoint. - Attach NBD exports and local raw images as USB mass-storage devices - Make attach and detach idempotent, with conflicts for reused device IDs - Keep backend URIs and paths out of responses and restrict socket modes - Reject TCP storage control and validate IDs, paths, sizes, and NBD input - Retain Virtualization.framework objects until detach completes - Add unit, race, configuration, REST, and integration coverage - Verify discovery, I/O, and detach in a Fedora ARM64 guest Raw-image DELETE requests only detach the guest device. They never remove or truncate the backing file. NBD detach leaves the remote export untouched. Requires macOS 15 or newer for the XHCI controller and runtime USB APIs. --- cmd/vfkit/main.go | 6 +- doc/usage.md | 96 +++++++ pkg/config/config.go | 4 + pkg/config/config_test.go | 9 + pkg/config/json.go | 16 ++ pkg/config/json_test.go | 11 +- pkg/config/virtio.go | 103 ++++++- pkg/config/virtio_test.go | 57 ++++ pkg/rest/rest.go | 72 ++++- pkg/rest/rest_test.go | 101 +++++++ pkg/rest/vf/storage.go | 211 +++++++++++++++ pkg/rest/vf/storage_test.go | 350 ++++++++++++++++++++++++ pkg/rest/vf/vm_config.go | 10 +- pkg/vf/storage_hotplug.go | 476 +++++++++++++++++++++++++++++++++ pkg/vf/storage_hotplug_test.go | 300 +++++++++++++++++++++ pkg/vf/virtio.go | 39 +-- pkg/vf/vm.go | 6 +- test/vm_helpers.go | 10 + test/vm_test.go | 124 ++++++++- 19 files changed, 1959 insertions(+), 42 deletions(-) create mode 100644 pkg/rest/vf/storage.go create mode 100644 pkg/rest/vf/storage_test.go create mode 100644 pkg/vf/storage_hotplug.go create mode 100644 pkg/vf/storage_hotplug_test.go diff --git a/cmd/vfkit/main.go b/cmd/vfkit/main.go index 397996b4..a2142159 100644 --- a/cmd/vfkit/main.go +++ b/cmd/vfkit/main.go @@ -190,7 +190,11 @@ func runVFKit(vmConfig *config.VirtualMachine, opts *cmdline.Options) error { // Do not enable the rests server if user sets scheme to None if opts.RestfulURI != cmdline.DefaultRestfulURI { restVM := restvf.NewVzVirtualMachine(vfVM) - srv, err := rest.NewServer(restVM, restVM, opts.RestfulURI) + var storageHandler rest.VirtualMachineStorageHandler + if len(vmConfig.USBXHCIControllerDevices()) > 0 { + storageHandler = restVM + } + srv, err := rest.NewServer(restVM, restVM, opts.RestfulURI, storageHandler) if err != nil { return err } diff --git a/doc/usage.md b/doc/usage.md index 1ddea1ad..9c09dd12 100644 --- a/doc/usage.md +++ b/doc/usage.md @@ -290,6 +290,21 @@ This allows to connect to the export of the remote NBD server: --device nbd,uri=nbd://192.168.64.4:11111/export,deviceId=nbd1,timeout=3000 ``` +### Runtime USB storage hotplug + +The `--device usb-xhci` option adds the XHCI controller required for runtime +USB hotplug. This option requires macOS 15 or newer. It does not attach a USB +device by itself. + +Runtime storage control also requires the REST service to use a Unix-domain +socket. TCP REST endpoints intentionally do not expose storage operations: with +a TCP `--restful-uri`, vfkit starts normally and logs a warning instead of +serving the storage endpoints. + +```bash +--device usb-xhci --restful-uri unix:///tmp/vfkit.sock +``` + ### Networking @@ -526,6 +541,10 @@ None To interact with the RESTful API, append a valid scheme to your base command: `--restful-uri tcp://localhost:8081`. +Unix-domain sockets (`--restful-uri unix:///path/to/vfkit.sock`) are created +with owner-only (`0600`) permissions, so only the user running vfkit can use +the API. + ### Get the virtual machine's state Obtain the state of the virtual machine that is being run by vfkit. @@ -564,6 +583,83 @@ GET /vm/inspect Response: `{ "cpus": uint, "memory": uint64, "devices": []config.VirtIODevice }` +The response describes the configuration the virtual machine was started with. +Storage devices attached at runtime are listed by `GET /vm/storage` instead. + +### Manage runtime storage + +Runtime storage endpoints are available only when vfkit starts with both +`--device usb-xhci` and a Unix-domain `--restful-uri`. The device identifier +must start with a letter or digit, may otherwise contain only letters, digits, +`.`, `_`, or `-`, and can be at most 255 characters long. + +Attach an NBD-backed USB mass-storage device: + +```HTTP +PUT /vm/storage/cache +Content-Type: application/json + +{ + "backend": "nbd", + "uri": "nbd://192.168.64.4:10809/export", + "timeoutMilliseconds": 15000, + "synchronizationMode": "full", + "readOnly": false +} +``` + +The NBD URI must use the `nbd`, `nbds`, `nbd+unix`, or `nbds+unix` scheme, +include a host (and, if given, a port between 1 and 65535) for `nbd` and +`nbds`, and contain only RFC 3986 URI characters; percent-encode anything else. +`timeoutMilliseconds` defaults to 15000 and must be between 1 and 600000; +`synchronizationMode` defaults to `full`. + +Attach a USB mass-storage device backed by a local raw disk image: + +```HTTP +PUT /vm/storage/scratch +Content-Type: application/json + +{ + "backend": "raw", + "path": "/var/tmp/scratch.raw", + "readOnly": false +} +``` + +Raw disk paths must be absolute. The target must be a regular file whose size +is a positive multiple of 512 bytes. + +The first successful request returns `HTTP 201`. Repeating the same request is +an idempotent no-op and returns `HTTP 200`. Requests with unknown fields, +malformed NBD URIs, or raw disk images that do not satisfy the rules above +return `HTTP 400`. Reusing an identifier with a different configuration, or +sending a request while an attach or detach for the same identifier is still +in progress, returns `HTTP 409`. + +List hotplugged devices without exposing their NBD URIs or raw disk paths: + +```HTTP +GET /vm/storage +``` + +Response: `{ "devices": [ { "id": string, "backend": "nbd"|"raw", "readOnly": bool, "state": string } ] }` + +`state` is `attached` for raw-backed devices. NBD-backed devices report +`connecting` until the NBD client connects, then `connected` or `disconnected` +as the connection changes; the guest sees I/O errors while disconnected and +the client reconnects automatically. + +Detach a device: + +```HTTP +DELETE /vm/storage/cache +``` + +Detach is idempotent and returns `HTTP 204` whether or not the identifier is +currently attached. A device that the virtualization framework no longer +reports as attached is dropped from the list and also returns `HTTP 204`. + ## Enabling a Graphical User Interface ### Add a virtio-gpu device diff --git a/pkg/config/config.go b/pkg/config/config.go index f96c3149..d5e034af 100644 --- a/pkg/config/config.go +++ b/pkg/config/config.go @@ -188,6 +188,10 @@ func (vm *VirtualMachine) VirtioNetDevices() []*VirtioNet { return FilterDevices[*VirtioNet](vm) } +func (vm *VirtualMachine) USBXHCIControllerDevices() []*USBXHCIController { + return FilterDevices[*USBXHCIController](vm) +} + func (vm *VirtualMachine) NetworkBlockDevice(deviceID string) *NetworkBlockDevice { for _, dev := range vm.Devices { if nbdDev, isNbdDev := dev.(*NetworkBlockDevice); isNbdDev && nbdDev.DeviceIdentifier == deviceID { diff --git a/pkg/config/config_test.go b/pkg/config/config_test.go index e97474c6..2cf90f72 100644 --- a/pkg/config/config_test.go +++ b/pkg/config/config_test.go @@ -47,6 +47,15 @@ func TestNetworkBlockDevice_NoDevice(t *testing.T) { require.Nil(t, nbdItem) } +func TestUSBXHCIControllerDevices(t *testing.T) { + vm := &VirtualMachine{} + xhci, err := USBXHCIControllerNew() + require.NoError(t, err) + require.NoError(t, vm.AddDevice(xhci)) + + assert.Equal(t, []*USBXHCIController{{}}, vm.USBXHCIControllerDevices()) +} + func TestVirtualMachine_ValidateBlockDevices(t *testing.T) { vm := &VirtualMachine{} diff --git a/pkg/config/json.go b/pkg/config/json.go index 5c54cc03..6135f227 100644 --- a/pkg/config/json.go +++ b/pkg/config/json.go @@ -27,6 +27,7 @@ const ( vfGpu vmComponentKind = "virtiogpu" vfInput vmComponentKind = "virtioinput" usbMassStorage vmComponentKind = "usbmassstorage" + usbXHCI vmComponentKind = "usbxhci" nvme vmComponentKind = "nvme" rosetta vmComponentKind = "rosetta" ignition vmComponentKind = "ignition" @@ -187,6 +188,10 @@ func unmarshalDevice(rawMsg json.RawMessage) (VirtioDevice, error) { var newDevice USBMassStorage err = json.Unmarshal(rawMsg, &newDevice) dev = &newDevice + case usbXHCI: + var newDevice USBXHCIController + err = json.Unmarshal(rawMsg, &newDevice) + dev = &newDevice case vfNbd: var newDevice NetworkBlockDevice err = json.Unmarshal(rawMsg, &newDevice) @@ -413,6 +418,17 @@ func (dev *USBMassStorage) MarshalJSON() ([]byte, error) { }) } +func (dev *USBXHCIController) MarshalJSON() ([]byte, error) { + type devWithKind struct { + jsonKind + USBXHCIController + } + return json.Marshal(devWithKind{ + jsonKind: kind(usbXHCI), + USBXHCIController: *dev, + }) +} + func (ign *Ignition) MarshalJSON() ([]byte, error) { type devWithKind struct { jsonKind diff --git a/pkg/config/json_test.go b/pkg/config/json_test.go index fa7b9cca..344fb338 100644 --- a/pkg/config/json_test.go +++ b/pkg/config/json_test.go @@ -170,18 +170,21 @@ var jsonTests = map[string]jsonTest{ usb, err := USBMassStorageNew("/usbmassstorage") require.NoError(t, err) usb.SetReadOnly(true) + // XHCI controller for runtime USB hotplug + xhci, err := USBXHCIControllerNew() + require.NoError(t, err) // rosetta rosetta, err := RosettaShareNew("vz-rosetta") require.NoError(t, err) // NBD nbd, err := NetworkBlockDeviceNew("uri", 1, SynchronizationFullMode) require.NoError(t, err) - err = vm.AddDevices(fs, usb, rosetta, nbd) + err = vm.AddDevices(fs, usb, xhci, rosetta, nbd) require.NoError(t, err) return vm }, - expectedJSON: `{"vcpus":3,"memoryBytes":4194304000,"bootloader":{"kind":"linuxBootloader","vmlinuzPath":"/vmlinuz","initrdPath":"/initrd","kernelCmdLine":"console=hvc0"},"devices":[{"kind":"virtioserial","logFile":"/virtioserial"},{"kind":"virtioinput","inputType":"keyboard"},{"kind":"virtiogpu","usesGUI":false,"width":800,"height":600},{"kind":"virtionet","nat":true,"macAddress":"00:11:22:33:44:55"},{"kind":"virtiorng"},{"kind":"virtioblk","devName":"virtio-blk","imagePath":"/virtioblk"},{"kind":"virtiosock","port":1234,"socketURL":"/virtiovsock"},{"kind":"virtiofs","mountTag":"tag","sharedDir":"/virtiofs"},{"kind":"usbmassstorage","devName":"usb-mass-storage","imagePath":"/usbmassstorage","readOnly":true},{"kind":"rosetta","mountTag":"vz-rosetta","installRosetta":false,"ignoreIfMissing":false},{"kind":"nbd", "devName":"nbd", "uri":"uri", "DeviceIdentifier":"", "SynchronizationMode":"full","Timeout":1000000}]}`, + expectedJSON: `{"vcpus":3,"memoryBytes":4194304000,"bootloader":{"kind":"linuxBootloader","vmlinuzPath":"/vmlinuz","initrdPath":"/initrd","kernelCmdLine":"console=hvc0"},"devices":[{"kind":"virtioserial","logFile":"/virtioserial"},{"kind":"virtioinput","inputType":"keyboard"},{"kind":"virtiogpu","usesGUI":false,"width":800,"height":600},{"kind":"virtionet","nat":true,"macAddress":"00:11:22:33:44:55"},{"kind":"virtiorng"},{"kind":"virtioblk","devName":"virtio-blk","imagePath":"/virtioblk"},{"kind":"virtiosock","port":1234,"socketURL":"/virtiovsock"},{"kind":"virtiofs","mountTag":"tag","sharedDir":"/virtiofs"},{"kind":"usbmassstorage","devName":"usb-mass-storage","imagePath":"/usbmassstorage","readOnly":true},{"kind":"usbxhci"},{"kind":"rosetta","mountTag":"vz-rosetta","installRosetta":false,"ignoreIfMissing":false},{"kind":"nbd", "devName":"nbd", "uri":"uri", "DeviceIdentifier":"", "SynchronizationMode":"full","Timeout":1000000}]}`, }, } @@ -294,6 +297,10 @@ var jsonStabilityTests = map[string]jsonStabilityTest{ skipFields: []string{"DevName", "URI", "Type"}, expectedJSON: `{"kind":"usbmassstorage","devName":"usb-mass-storage","imagePath":"ImagePath","readOnly":true,"type":"image"}`, }, + "USBXHCIController": { + obj: &USBXHCIController{}, + expectedJSON: `{"kind":"usbxhci"}`, + }, "NVMExpressController": { newObjectFunc: func(t *testing.T) any { nvme, err := NVMExpressControllerNew("") diff --git a/pkg/config/virtio.go b/pkg/config/virtio.go index ebdcd11d..51853954 100644 --- a/pkg/config/virtio.go +++ b/pkg/config/virtio.go @@ -5,6 +5,7 @@ import ( "fmt" "math" "net" + "net/url" "os" "slices" "strconv" @@ -124,6 +125,72 @@ const ( SynchronizationNoneMode NBDSynchronizationMode = "none" ) +// DefaultNBDTimeout is the NBD connection timeout used when none is specified. +const DefaultNBDTimeout = 15 * time.Second + +// ParseNBDSynchronizationMode converts the user-facing "full"/"none" value into +// an NBDSynchronizationMode. +func ParseNBDSynchronizationMode(value string) (NBDSynchronizationMode, error) { + switch NBDSynchronizationMode(value) { + case SynchronizationFullMode: + return SynchronizationFullMode, nil + case SynchronizationNoneMode: + return SynchronizationNoneMode, nil + default: + return "", fmt.Errorf("invalid sync mode: %s, must be 'full' or 'none'", value) + } +} + +// ValidateNBDURI checks that uri is a well-formed NBD URI, as specified by +// https://github.com/NetworkBlockDevice/nbd/blob/master/doc/uri.md, that the +// Virtualization framework can use. Characters outside the RFC 3986 URI +// character set are rejected up front because the framework raises an +// uncatchable exception instead of returning an error for such URIs. +func ValidateNBDURI(uri string) error { + if uri == "" { + return fmt.Errorf("'uri' must be specified") + } + for i := 0; i < len(uri); i++ { + if !isURIByte(uri[i]) { + return fmt.Errorf("invalid character %q in 'uri', percent-encode it", uri[i]) + } + } + + parsed, err := url.Parse(uri) + if err != nil { + return fmt.Errorf("error: %w", err) + } + + switch parsed.Scheme { + case "nbd", "nbds": + if parsed.Opaque != "" || parsed.Hostname() == "" { + return fmt.Errorf("'uri' must specify a host: %s://[:]/", parsed.Scheme) + } + if port := parsed.Port(); port != "" { + if n, err := strconv.Atoi(port); err != nil || n < 1 || n > 65535 { + return fmt.Errorf("invalid port in 'uri': %s", port) + } + } + case "nbd+unix", "nbds+unix": + if parsed.Opaque != "" || parsed.Query().Get("socket") == "" { + return fmt.Errorf("'uri' must be of the form %s:///?socket=", parsed.Scheme) + } + default: + return fmt.Errorf("invalid scheme: %s. Expected one of: 'nbd', 'nbds', 'nbd+unix', or 'nbds+unix'", parsed.Scheme) + } + + return nil +} + +// isURIByte reports whether c may appear unescaped in an RFC 3986 URI. +func isURIByte(c byte) bool { + switch { + case c >= 'a' && c <= 'z', c >= 'A' && c <= 'Z', c >= '0' && c <= '9': + return true + } + return strings.IndexByte("-._~:/?#[]@!$&'()*+,;=%", c) >= 0 +} + type NetworkBlockDevice struct { NetworkBlockStorageConfig DeviceIdentifier string @@ -133,6 +200,25 @@ type NetworkBlockDevice struct { type VirtioBalloon struct{} +// USBXHCIController configures an XHCI controller for runtime USB hotplug. +// The controller is available on macOS 15 and newer. +type USBXHCIController struct{} + +func USBXHCIControllerNew() (VirtioDevice, error) { + return &USBXHCIController{}, nil +} + +func (dev *USBXHCIController) FromOptions(options []option) error { + if len(options) != 0 { + return fmt.Errorf("unknown options for usb-xhci devices: %s", options) + } + return nil +} + +func (dev *USBXHCIController) ToCmdLine() ([]string, error) { + return []string{"--device", "usb-xhci"}, nil +} + func VirtioBalloonNew() (VirtioDevice, error) { return &VirtioBalloon{}, nil } @@ -203,6 +289,8 @@ func deviceFromCmdLine(deviceOpts string) (VirtioDevice, error) { dev = &VirtioVsock{} case "usb-mass-storage": dev = usbMassStorageNewEmpty() + case "usb-xhci": + dev = &USBXHCIController{} case "virtio-input": dev = &VirtioInput{} case "virtio-gpu": @@ -808,8 +896,8 @@ func networkBlockDeviceNewEmpty() *NetworkBlockDevice { }, }, DeviceIdentifier: "", - Timeout: time.Duration(15000 * time.Millisecond), // set a default timeout to 15s - SynchronizationMode: SynchronizationFullMode, // default mode to full + Timeout: DefaultNBDTimeout, + SynchronizationMode: SynchronizationFullMode, // default mode to full } } @@ -862,14 +950,11 @@ func (nbd *NetworkBlockDevice) FromOptions(options []option) error { } nbd.Timeout = time.Duration(timeoutMS) * time.Millisecond case "sync": - switch option.value { - case string(SynchronizationFullMode): - nbd.SynchronizationMode = SynchronizationFullMode - case string(SynchronizationNoneMode): - nbd.SynchronizationMode = SynchronizationNoneMode - default: - return fmt.Errorf("invalid sync mode: %s, must be 'full' or 'none'", option.value) + mode, err := ParseNBDSynchronizationMode(option.value) + if err != nil { + return err } + nbd.SynchronizationMode = mode default: unhandledOpts = append(unhandledOpts, option) } diff --git a/pkg/config/virtio_test.go b/pkg/config/virtio_test.go index 08dee811..45cc49bf 100644 --- a/pkg/config/virtio_test.go +++ b/pkg/config/virtio_test.go @@ -69,6 +69,11 @@ func testErrorVirtioDev(t *testing.T, test *virtioDevTest) { func TestVirtioDevices(t *testing.T) { testImagePath := filepath.Join(t.TempDir(), "test.img") var virtioDevTests = map[string]virtioDevTest{ + "NewUSBXHCIController": { + newDev: USBXHCIControllerNew, + expectedDev: &USBXHCIController{}, + expectedCmdLine: []string{"--device", "usb-xhci"}, + }, "NewVirtioBlk": { newDev: func() (VirtioDevice, error) { return getTestVirtioBlkDevice(testImagePath) @@ -482,3 +487,55 @@ func TestVirtioDevices(t *testing.T) { } }) } + +func TestValidateNBDURI(t *testing.T) { + valid := []string{ + "nbd://192.168.64.4:10809/export", + "nbds://nbd.example.com/export", + "nbd://[::1]:10809/export", + "nbd://host/path%20with%20spaces", + "nbd+unix:///export?socket=/var/run/nbd.sock", + "nbds+unix:///export?socket=/var/run/nbd.sock", + } + for _, uri := range valid { + assert.NoError(t, ValidateNBDURI(uri), uri) + } + + invalid := map[string]string{ + "": "'uri' must be specified", + "https://example.com/x": "invalid scheme", + "nbd://": "must specify a host", + "nbd:relative": "must specify a host", + "nbd://host:0/x": "invalid port", + "nbd://host:65536/x": "invalid port", + "nbd://ho\"st/x": "invalid character", + "nbd://ho/x": "invalid character", + "nbd://host/a b": "invalid character", + "nbd://hôst/x": "invalid character", + "nbd+unix:relative": "must be of the form", + "nbd+unix:///": "must be of the form", + "nbd+unix:///?socket=": "must be of the form", + "nbds+unix:///": "must be of the form", + "nbds+unix:///?socket=": "must be of the form", + } + for uri, message := range invalid { + err := ValidateNBDURI(uri) + require.Error(t, err, uri) + assert.Contains(t, err.Error(), message, uri) + } +} + +func TestParseNBDSynchronizationMode(t *testing.T) { + mode, err := ParseNBDSynchronizationMode("full") + require.NoError(t, err) + assert.Equal(t, SynchronizationFullMode, mode) + + mode, err = ParseNBDSynchronizationMode("none") + require.NoError(t, err) + assert.Equal(t, SynchronizationNoneMode, mode) + + _, err = ParseNBDSynchronizationMode("") + require.Error(t, err) + _, err = ParseNBDSynchronizationMode("Full") + require.Error(t, err) +} diff --git a/pkg/rest/rest.go b/pkg/rest/rest.go index d9803e86..6b563939 100644 --- a/pkg/rest/rest.go +++ b/pkg/rest/rest.go @@ -3,9 +3,12 @@ package rest import ( "errors" "fmt" + "net" + "net/http" "net/url" "os" "syscall" + "time" "github.com/crc-org/vfkit/pkg/util" "github.com/gin-gonic/gin" @@ -67,21 +70,67 @@ type VFKitService struct { // Start initiates the already configured gin service func (v *VFKitService) Start() { + var listener net.Listener + if v.Scheme == Unix { + // Bind synchronously, before the VM starts creating files, because + // the umask change in newUnixListener is process-wide. + unixListener, err := newUnixListener(v.Path) + if err != nil { + logrus.Fatal(err) + } + listener = unixListener + util.RegisterExitHandler(func() { os.Remove(v.Path) }) + } go func() { var err error switch v.Scheme { case TCP: err = v.router.Run(v.Host) case Unix: - util.RegisterExitHandler(func() { os.Remove(v.Path) }) - err = v.router.RunUnix(v.Path) + err = v.serveUnix(listener) } logrus.Fatal(err) }() } -// NewServer creates a new restful service -func NewServer(inspector VirtualMachineInspector, stateHandler VirtualMachineStateHandler, endpoint string) (*VFKitService, error) { +// newUnixListener binds the REST socket with owner-only permissions. The +// umask is narrowed while the socket is bound so that it is never reachable +// with wider permissions; the explicit chmod covers filesystems that do not +// apply the umask to sockets. +func newUnixListener(path string) (net.Listener, error) { + oldMask := syscall.Umask(0077) + listener, err := net.Listen("unix", path) + syscall.Umask(oldMask) + if err != nil { + return nil, err + } + if err := os.Chmod(path, 0600); err != nil { + _ = listener.Close() + _ = os.Remove(path) + return nil, fmt.Errorf("could not restrict unix socket permissions: %w", err) + } + return listener, nil +} + +func (v *VFKitService) serveUnix(listener net.Listener) error { + defer listener.Close() + defer os.Remove(v.Path) + server := &http.Server{ + Handler: v.router, + ReadHeaderTimeout: 5 * time.Second, + } + return server.Serve(listener) +} + +// NewServer creates a new restful service. storageHandler may be nil; storage +// endpoints are only served on unix endpoints, so it is ignored with a warning +// for TCP endpoints. +func NewServer( + inspector VirtualMachineInspector, + stateHandler VirtualMachineStateHandler, + endpoint string, + storageHandler VirtualMachineStorageHandler, +) (*VFKitService, error) { gin.SetMode(gin.ReleaseMode) r := gin.Default() ep, err := NewEndpoint(endpoint) @@ -101,6 +150,15 @@ func NewServer(inspector VirtualMachineInspector, stateHandler VirtualMachineSta r.GET("/vm/state", stateHandler.GetVMState) r.POST("/vm/state", stateHandler.SetVMState) r.GET("/vm/inspect", inspector.Inspect) + if storageHandler != nil { + if ep.Scheme == Unix { + r.GET("/vm/storage", storageHandler.ListStorageDevices) + r.PUT("/vm/storage/:id", storageHandler.AttachStorageDevice) + r.DELETE("/vm/storage/:id", storageHandler.DetachStorageDevice) + } else { + logrus.Warn("storage hotplug endpoints are only served on unix REST endpoints, not registering /vm/storage") + } + } return &s, nil } @@ -113,6 +171,12 @@ type VirtualMachineStateHandler interface { SetVMState(c *gin.Context) } +type VirtualMachineStorageHandler interface { + ListStorageDevices(c *gin.Context) + AttachStorageDevice(c *gin.Context) + DetachStorageDevice(c *gin.Context) +} + // parseRestfulURI validates the input URI and returns an URL object func parseRestfulURI(inputURI string) (*url.URL, error) { restURI, err := url.ParseRequestURI(inputURI) diff --git a/pkg/rest/rest_test.go b/pkg/rest/rest_test.go index fd6bffed..6711e8fd 100644 --- a/pkg/rest/rest_test.go +++ b/pkg/rest/rest_test.go @@ -2,12 +2,113 @@ package rest import ( "fmt" + "net/http" + "net/http/httptest" "net/url" + "os" "testing" + "github.com/gin-gonic/gin" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) +type recordingVMHandler struct { + storageCalls []string +} + +func (h *recordingVMHandler) Inspect(c *gin.Context) { + c.Status(http.StatusOK) +} + +func (h *recordingVMHandler) GetVMState(c *gin.Context) { + c.Status(http.StatusOK) +} + +func (h *recordingVMHandler) SetVMState(c *gin.Context) { + c.Status(http.StatusAccepted) +} + +func (h *recordingVMHandler) ListStorageDevices(c *gin.Context) { + h.storageCalls = append(h.storageCalls, "list") + c.Status(http.StatusOK) +} + +func (h *recordingVMHandler) AttachStorageDevice(c *gin.Context) { + h.storageCalls = append(h.storageCalls, "attach:"+c.Param("id")) + c.Status(http.StatusCreated) +} + +func (h *recordingVMHandler) DetachStorageDevice(c *gin.Context) { + h.storageCalls = append(h.storageCalls, "detach:"+c.Param("id")) + c.Status(http.StatusNoContent) +} + +func TestNewServerRegistersStorageRoutes(t *testing.T) { + handler := &recordingVMHandler{} + server, err := NewServer(handler, handler, "unix:///tmp/vfkit-test.sock", handler) + require.NoError(t, err) + + tests := []struct { + method string + path string + status int + }{ + {method: http.MethodGet, path: "/vm/storage", status: http.StatusOK}, + {method: http.MethodPut, path: "/vm/storage/cache", status: http.StatusCreated}, + {method: http.MethodDelete, path: "/vm/storage/cache", status: http.StatusNoContent}, + } + + for _, tt := range tests { + recorder := httptest.NewRecorder() + request := httptest.NewRequest(tt.method, tt.path, nil) + server.router.ServeHTTP(recorder, request) + assert.Equal(t, tt.status, recorder.Code) + } + assert.Equal(t, []string{"list", "attach:cache", "detach:cache"}, handler.storageCalls) +} + +func TestNewServerSkipsStorageRoutesOnTCP(t *testing.T) { + handler := &recordingVMHandler{} + server, err := NewServer(handler, handler, "tcp://127.0.0.1:8080", handler) + require.NoError(t, err) + + recorder := httptest.NewRecorder() + server.router.ServeHTTP(recorder, httptest.NewRequest(http.MethodGet, "/vm/storage", nil)) + assert.Equal(t, http.StatusNotFound, recorder.Code) + assert.Empty(t, handler.storageCalls) +} + +func TestNewServerWithoutStorageHandler(t *testing.T) { + handler := &recordingVMHandler{} + server, err := NewServer(handler, handler, "unix:///tmp/vfkit-test.sock", nil) + require.NoError(t, err) + + recorder := httptest.NewRecorder() + server.router.ServeHTTP(recorder, httptest.NewRequest(http.MethodPut, "/vm/storage/cache", nil)) + assert.Equal(t, http.StatusNotFound, recorder.Code) + assert.Empty(t, handler.storageCalls) +} + +func TestNewUnixListenerIsOwnerOnly(t *testing.T) { + socketFile, err := os.CreateTemp("/tmp", "vfkit-rest-") + require.NoError(t, err) + socketPath := socketFile.Name() + require.NoError(t, socketFile.Close()) + require.NoError(t, os.Remove(socketPath)) + t.Cleanup(func() { _ = os.Remove(socketPath) }) + + listener, err := newUnixListener(socketPath) + require.NoError(t, err) + t.Cleanup(func() { + require.NoError(t, listener.Close()) + }) + + info, err := os.Stat(socketPath) + require.NoError(t, err) + assert.Equal(t, os.FileMode(0600), info.Mode().Perm()) +} + func TestParseRestfulURI(t *testing.T) { type args struct { inputURI string diff --git a/pkg/rest/vf/storage.go b/pkg/rest/vf/storage.go new file mode 100644 index 00000000..256c1e1f --- /dev/null +++ b/pkg/rest/vf/storage.go @@ -0,0 +1,211 @@ +package rest + +import ( + "encoding/json" + "errors" + "io" + "net/http" + "path/filepath" + "regexp" + "time" + + "github.com/crc-org/vfkit/pkg/config" + corevf "github.com/crc-org/vfkit/pkg/vf" + "github.com/gin-gonic/gin" + log "github.com/sirupsen/logrus" +) + +const ( + maximumStorageTimeoutMilliseconds int64 = 600_000 + maximumStorageLocationLength = 4096 + maximumStorageRequestBytes int64 = 64 << 10 +) + +// storageDeviceIDPattern matches the identifiers documented in doc/usage.md: a +// leading letter or digit followed by letters, digits, '.', '_' or '-'. +var storageDeviceIDPattern = regexp.MustCompile(`^[A-Za-z0-9][A-Za-z0-9._-]{0,254}$`) + +type storageDeviceRequest struct { + Backend string `json:"backend"` + URI string `json:"uri"` + Path string `json:"path"` + TimeoutMilliseconds int64 `json:"timeoutMilliseconds"` + SynchronizationMode string `json:"synchronizationMode"` + ReadOnly bool `json:"readOnly"` +} + +func (vm *VzVirtualMachine) ListStorageDevices(c *gin.Context) { + c.JSON(http.StatusOK, gin.H{"devices": vm.storageController.HotpluggedStorageDevices()}) +} + +func (vm *VzVirtualMachine) AttachStorageDevice(c *gin.Context) { + id := c.Param("id") + if !storageDeviceIDPattern.MatchString(id) { + c.JSON(http.StatusBadRequest, gin.H{"error": "invalid storage device ID"}) + return + } + + request, err := decodeStorageDeviceRequest(c) + if err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": "invalid storage device request: " + err.Error()}) + return + } + created, err := vm.attachStorageDevice(id, request) + if err != nil { + location := request.sensitiveLocation() + status, message := storageErrorResponse(err, "failed to attach storage device", location) + if status == http.StatusInternalServerError { + log.Errorf("Failed to attach storage device %s: %s", id, corevf.RedactLocation(err.Error(), location)) + } + c.JSON(status, gin.H{"error": message}) + return + } + status := http.StatusOK + if created { + status = http.StatusCreated + } + c.JSON(status, gin.H{"id": id}) +} + +// decodeStorageDeviceRequest reads the JSON body strictly: the body is capped, +// unknown fields are rejected so that misspelled options cannot silently fall +// back to defaults, and trailing content is refused. +func decodeStorageDeviceRequest(c *gin.Context) (storageDeviceRequest, error) { + var request storageDeviceRequest + decoder := json.NewDecoder(http.MaxBytesReader(c.Writer, c.Request.Body, maximumStorageRequestBytes)) + decoder.DisallowUnknownFields() + if err := decoder.Decode(&request); err != nil { + return request, err + } + if err := decoder.Decode(&struct{}{}); !errors.Is(err, io.EOF) { + return request, errors.New("unexpected content after the JSON object") + } + return request, nil +} + +type storageRequestError struct { + message string +} + +func (err *storageRequestError) Error() string { + return err.message +} + +func invalidStorageRequest(message string) error { + return &storageRequestError{message: message} +} + +// storageErrorResponse maps errors from the storage controller to an HTTP +// status and a client-safe message; location is redacted from messages. +func storageErrorResponse(err error, fallback string, location string) (int, string) { + var requestError *storageRequestError + switch { + case errors.As(err, &requestError): + return http.StatusBadRequest, requestError.Error() + case errors.Is(err, corevf.ErrInvalidStorageDevice): + return http.StatusBadRequest, corevf.RedactLocation(err.Error(), location) + case errors.Is(err, corevf.ErrStorageDeviceConflict), errors.Is(err, corevf.ErrStorageDeviceBusy): + return http.StatusConflict, err.Error() + case errors.Is(err, corevf.ErrUSBControllerUnavailable): + return http.StatusServiceUnavailable, err.Error() + default: + return http.StatusInternalServerError, fallback + } +} + +func (vm *VzVirtualMachine) attachStorageDevice(id string, request storageDeviceRequest) (bool, error) { + switch request.Backend { + case "nbd": + spec, err := request.nbdStorageDevice() + if err != nil { + return false, err + } + return vm.storageController.HotplugNBDStorageDevice(id, spec) + case "raw": + spec, err := request.rawStorageDevice() + if err != nil { + return false, err + } + return vm.storageController.HotplugRawStorageDevice(id, spec) + default: + return false, invalidStorageRequest("storage backend must be nbd or raw") + } +} + +func (vm *VzVirtualMachine) DetachStorageDevice(c *gin.Context) { + id := c.Param("id") + if !storageDeviceIDPattern.MatchString(id) { + c.JSON(http.StatusBadRequest, gin.H{"error": "invalid storage device ID"}) + return + } + if _, err := vm.storageController.DetachHotpluggedStorageDevice(id); err != nil { + status, message := storageErrorResponse(err, "failed to detach storage device", "") + if status == http.StatusInternalServerError { + log.Errorf("Failed to detach storage device %s: %v", id, err) + } + c.JSON(status, gin.H{"error": message}) + return + } + c.Status(http.StatusNoContent) +} + +func (request storageDeviceRequest) nbdStorageDevice() (corevf.NBDStorageDevice, error) { + if request.Path != "" { + return corevf.NBDStorageDevice{}, invalidStorageRequest("path is not valid for the nbd backend") + } + if request.URI == "" { + return corevf.NBDStorageDevice{}, invalidStorageRequest("storage URI is required") + } + if len(request.URI) > maximumStorageLocationLength { + return corevf.NBDStorageDevice{}, invalidStorageRequest("storage URI is too long") + } + if err := config.ValidateNBDURI(request.URI); err != nil { + return corevf.NBDStorageDevice{}, invalidStorageRequest("invalid storage URI: " + err.Error()) + } + timeout := request.TimeoutMilliseconds + if timeout == 0 { + timeout = config.DefaultNBDTimeout.Milliseconds() + } + if timeout < 1 || timeout > maximumStorageTimeoutMilliseconds { + return corevf.NBDStorageDevice{}, invalidStorageRequest("timeoutMilliseconds must be between 1 and 600000") + } + synchronizationMode := config.SynchronizationFullMode + if request.SynchronizationMode != "" { + mode, err := config.ParseNBDSynchronizationMode(request.SynchronizationMode) + if err != nil { + return corevf.NBDStorageDevice{}, invalidStorageRequest("synchronizationMode must be full or none") + } + synchronizationMode = mode + } + return corevf.NBDStorageDevice{ + URI: request.URI, + Timeout: time.Duration(timeout) * time.Millisecond, + SynchronizationMode: synchronizationMode, + ReadOnly: request.ReadOnly, + }, nil +} + +func (request storageDeviceRequest) rawStorageDevice() (corevf.RawStorageDevice, error) { + if request.URI != "" || request.TimeoutMilliseconds != 0 || request.SynchronizationMode != "" { + return corevf.RawStorageDevice{}, invalidStorageRequest( + "uri, timeoutMilliseconds, and synchronizationMode are not valid for the raw backend", + ) + } + if request.Path == "" { + return corevf.RawStorageDevice{}, invalidStorageRequest("storage path is required") + } + if len(request.Path) > maximumStorageLocationLength { + return corevf.RawStorageDevice{}, invalidStorageRequest("storage path is too long") + } + if !filepath.IsAbs(request.Path) { + return corevf.RawStorageDevice{}, invalidStorageRequest("storage path must be absolute") + } + return corevf.RawStorageDevice{Path: request.Path, ReadOnly: request.ReadOnly}, nil +} + +func (request storageDeviceRequest) sensitiveLocation() string { + if request.Backend == "raw" { + return request.Path + } + return request.URI +} diff --git a/pkg/rest/vf/storage_test.go b/pkg/rest/vf/storage_test.go new file mode 100644 index 00000000..27134553 --- /dev/null +++ b/pkg/rest/vf/storage_test.go @@ -0,0 +1,350 @@ +package rest + +import ( + "bytes" + "encoding/json" + "errors" + "fmt" + "net/http" + "net/http/httptest" + "strings" + "testing" + "time" + + "github.com/crc-org/vfkit/pkg/config" + corevf "github.com/crc-org/vfkit/pkg/vf" + "github.com/gin-gonic/gin" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type fakeStorageController struct { + devices []corevf.HotpluggedStorageDevice + attachCreated bool + attachErr error + detachErr error + attachedID string + attachedSpec corevf.NBDStorageDevice + attachedRaw corevf.RawStorageDevice + detachedID string +} + +func (f *fakeStorageController) HotpluggedStorageDevices() []corevf.HotpluggedStorageDevice { + return f.devices +} + +func (f *fakeStorageController) HotplugNBDStorageDevice( + id string, + spec corevf.NBDStorageDevice, +) (bool, error) { + f.attachedID = id + f.attachedSpec = spec + return f.attachCreated, f.attachErr +} + +func (f *fakeStorageController) HotplugRawStorageDevice( + id string, + spec corevf.RawStorageDevice, +) (bool, error) { + f.attachedID = id + f.attachedRaw = spec + return f.attachCreated, f.attachErr +} + +func (f *fakeStorageController) DetachHotpluggedStorageDevice(id string) (bool, error) { + f.detachedID = id + return true, f.detachErr +} + +// storageTestContext builds a gin context for method/path. body may be nil, a +// raw string, or a value that is JSON-encoded. +func storageTestContext(t *testing.T, method string, path string, body any) (*gin.Context, *httptest.ResponseRecorder) { + t.Helper() + gin.SetMode(gin.TestMode) + recorder := httptest.NewRecorder() + context, _ := gin.CreateTestContext(recorder) + var requestBody bytes.Buffer + switch b := body.(type) { + case nil: + case string: + requestBody.WriteString(b) + default: + require.NoError(t, json.NewEncoder(&requestBody).Encode(body)) + } + context.Request = httptest.NewRequest(method, path, &requestBody) + return context, recorder +} + +// attachContext builds a PUT context whose :id parameter is id. The request +// path itself stays fixed so that ids which are not valid URL paths can be +// exercised. +func attachContext(t *testing.T, id string, body any) (*gin.Context, *httptest.ResponseRecorder) { + t.Helper() + context, recorder := storageTestContext(t, http.MethodPut, "/vm/storage/device", body) + context.Params = gin.Params{{Key: "id", Value: id}} + return context, recorder +} + +func TestAttachStorageDevice(t *testing.T) { + controller := &fakeStorageController{attachCreated: true} + vm := &VzVirtualMachine{storageController: controller} + context, recorder := attachContext(t, "avrea-persistent", map[string]any{ + "backend": "nbd", + "uri": "nbd://192.0.2.10:10809/export-token", + "timeoutMilliseconds": 60_000, + "synchronizationMode": "full", + "readOnly": false, + }) + + vm.AttachStorageDevice(context) + + assert.Equal(t, http.StatusCreated, recorder.Code) + assert.Equal(t, "avrea-persistent", controller.attachedID) + assert.Equal(t, "nbd://192.0.2.10:10809/export-token", controller.attachedSpec.URI) + assert.Equal(t, 60*time.Second, controller.attachedSpec.Timeout) + assert.Equal(t, config.SynchronizationFullMode, controller.attachedSpec.SynchronizationMode) + assert.False(t, controller.attachedSpec.ReadOnly) +} + +func TestAttachStorageDeviceAppliesDefaults(t *testing.T) { + controller := &fakeStorageController{attachCreated: true} + vm := &VzVirtualMachine{storageController: controller} + context, recorder := attachContext(t, "cache", map[string]any{ + "backend": "nbd", + "uri": "nbd://192.0.2.10:10809/export", + }) + + vm.AttachStorageDevice(context) + + assert.Equal(t, http.StatusCreated, recorder.Code) + assert.Equal(t, config.DefaultNBDTimeout, controller.attachedSpec.Timeout) + assert.Equal(t, config.SynchronizationFullMode, controller.attachedSpec.SynchronizationMode) +} + +func TestAttachRawStorageDevice(t *testing.T) { + controller := &fakeStorageController{attachCreated: true} + vm := &VzVirtualMachine{storageController: controller} + context, recorder := attachContext(t, "scratch", map[string]any{ + "backend": "raw", + "path": "/var/tmp/scratch.raw", + "readOnly": true, + }) + + vm.AttachStorageDevice(context) + + assert.Equal(t, http.StatusCreated, recorder.Code) + assert.Equal(t, "scratch", controller.attachedID) + assert.Equal(t, corevf.RawStorageDevice{ + Path: "/var/tmp/scratch.raw", + ReadOnly: true, + }, controller.attachedRaw) +} + +func TestAttachRawStorageDeviceRejectsRelativePath(t *testing.T) { + controller := &fakeStorageController{} + vm := &VzVirtualMachine{storageController: controller} + context, recorder := attachContext(t, "scratch", map[string]any{ + "backend": "raw", + "path": "scratch.raw", + }) + + vm.AttachStorageDevice(context) + + assert.Equal(t, http.StatusBadRequest, recorder.Code) + assert.Empty(t, controller.attachedID) +} + +func TestAttachRawStorageDeviceDoesNotEchoPath(t *testing.T) { + opaquePath := "/var/tmp/opaque-tenant-path/scratch.raw" + controller := &fakeStorageController{attachErr: errors.New("could not open " + opaquePath)} + vm := &VzVirtualMachine{storageController: controller} + context, recorder := attachContext(t, "scratch", map[string]any{ + "backend": "raw", + "path": opaquePath, + }) + + vm.AttachStorageDevice(context) + + assert.Equal(t, http.StatusInternalServerError, recorder.Code) + assert.NotContains(t, recorder.Body.String(), opaquePath) +} + +func TestAttachRawStorageDeviceReportsInvalidImageAsBadRequest(t *testing.T) { + opaquePath := "/var/tmp/opaque-tenant-path/scratch.raw" + controller := &fakeStorageController{attachErr: fmt.Errorf( + "%w: could not inspect raw disk image: stat %s: no such file or directory", + corevf.ErrInvalidStorageDevice, opaquePath, + )} + vm := &VzVirtualMachine{storageController: controller} + context, recorder := attachContext(t, "scratch", map[string]any{ + "backend": "raw", + "path": opaquePath, + }) + + vm.AttachStorageDevice(context) + + assert.Equal(t, http.StatusBadRequest, recorder.Code) + assert.Contains(t, recorder.Body.String(), "could not inspect raw disk image") + assert.NotContains(t, recorder.Body.String(), opaquePath) +} + +func TestAttachStorageDeviceDoesNotEchoURI(t *testing.T) { + controller := &fakeStorageController{attachErr: errors.New("attach failed")} + vm := &VzVirtualMachine{storageController: controller} + opaqueURI := "nbd://192.0.2.10:10809/opaque-export" + context, recorder := attachContext(t, "cache", map[string]any{ + "backend": "nbd", + "uri": opaqueURI, + }) + + vm.AttachStorageDevice(context) + + assert.Equal(t, http.StatusInternalServerError, recorder.Code) + assert.NotContains(t, recorder.Body.String(), opaqueURI) +} + +func TestAttachStorageDeviceRejectsInvalidID(t *testing.T) { + for _, id := range []string{"bad id", "_cache", ".scratch", "-x", ""} { + controller := &fakeStorageController{} + vm := &VzVirtualMachine{storageController: controller} + context, recorder := attachContext(t, id, map[string]any{ + "backend": "nbd", + "uri": "nbd://192.0.2.10:10809/export-token", + }) + + vm.AttachStorageDevice(context) + + assert.Equal(t, http.StatusBadRequest, recorder.Code, id) + assert.Empty(t, controller.attachedID, id) + } +} + +func TestAttachStorageDeviceRejectsInvalidNBDURI(t *testing.T) { + for _, uri := range []string{ + "https://example.com/secret-token", + "nbd://", + "nbd:relative", + "nbd://host:0/x", + "nbd://ho\"st/x", + "nbd://ho/x", + } { + controller := &fakeStorageController{} + vm := &VzVirtualMachine{storageController: controller} + context, recorder := attachContext(t, "cache", map[string]any{ + "backend": "nbd", + "uri": uri, + }) + + vm.AttachStorageDevice(context) + + assert.Equal(t, http.StatusBadRequest, recorder.Code, uri) + assert.Empty(t, controller.attachedID, uri) + } +} + +func TestAttachStorageDeviceRejectsUnknownFields(t *testing.T) { + controller := &fakeStorageController{} + vm := &VzVirtualMachine{storageController: controller} + context, recorder := attachContext(t, "scratch", map[string]any{ + "backend": "raw", + "path": "/var/tmp/scratch.raw", + "read_only": true, + }) + + vm.AttachStorageDevice(context) + + assert.Equal(t, http.StatusBadRequest, recorder.Code) + assert.Contains(t, recorder.Body.String(), "read_only") + assert.Empty(t, controller.attachedID) +} + +func TestAttachStorageDeviceRejectsTrailingContent(t *testing.T) { + controller := &fakeStorageController{} + vm := &VzVirtualMachine{storageController: controller} + context, recorder := attachContext(t, "scratch", + `{"backend":"raw","path":"/var/tmp/scratch.raw"}{"backend":"raw","path":"/var/tmp/other.raw"}`) + + vm.AttachStorageDevice(context) + + assert.Equal(t, http.StatusBadRequest, recorder.Code) + assert.Empty(t, controller.attachedID) +} + +func TestAttachStorageDeviceRejectsOversizedBody(t *testing.T) { + controller := &fakeStorageController{} + vm := &VzVirtualMachine{storageController: controller} + context, recorder := attachContext(t, "scratch", map[string]any{ + "backend": "raw", + "path": "/" + strings.Repeat("a", int(maximumStorageRequestBytes)), + }) + + vm.AttachStorageDevice(context) + + assert.Equal(t, http.StatusBadRequest, recorder.Code) + assert.Empty(t, controller.attachedID) +} + +func TestAttachStorageDeviceConflict(t *testing.T) { + for _, attachErr := range []error{corevf.ErrStorageDeviceConflict, corevf.ErrStorageDeviceBusy} { + controller := &fakeStorageController{attachErr: attachErr} + vm := &VzVirtualMachine{storageController: controller} + context, recorder := attachContext(t, "cache", map[string]any{ + "backend": "nbd", + "uri": "nbd://192.0.2.10:10809/export-token", + }) + + vm.AttachStorageDevice(context) + + assert.Equal(t, http.StatusConflict, recorder.Code, attachErr) + } +} + +func TestAttachStorageDeviceWithoutController(t *testing.T) { + controller := &fakeStorageController{attachErr: corevf.ErrUSBControllerUnavailable} + vm := &VzVirtualMachine{storageController: controller} + context, recorder := attachContext(t, "cache", map[string]any{ + "backend": "nbd", + "uri": "nbd://192.0.2.10:10809/export-token", + }) + + vm.AttachStorageDevice(context) + + assert.Equal(t, http.StatusServiceUnavailable, recorder.Code) +} + +func TestListStorageDevicesOmitsURI(t *testing.T) { + controller := &fakeStorageController{devices: []corevf.HotpluggedStorageDevice{ + {ID: "cache", Backend: "nbd", ReadOnly: false, State: corevf.StorageDeviceStateConnected}, + }} + vm := &VzVirtualMachine{storageController: controller} + context, recorder := storageTestContext(t, http.MethodGet, "/vm/storage", nil) + + vm.ListStorageDevices(context) + + assert.Equal(t, http.StatusOK, recorder.Code) + assert.JSONEq(t, `{"devices":[{"id":"cache","backend":"nbd","readOnly":false,"state":"connected"}]}`, recorder.Body.String()) + assert.NotContains(t, recorder.Body.String(), "uri") +} + +func TestDetachStorageDeviceIsIdempotent(t *testing.T) { + controller := &fakeStorageController{} + vm := &VzVirtualMachine{storageController: controller} + context, _ := storageTestContext(t, http.MethodDelete, "/vm/storage/cache", nil) + context.Params = gin.Params{{Key: "id", Value: "cache"}} + + vm.DetachStorageDevice(context) + + assert.Equal(t, http.StatusNoContent, context.Writer.Status()) + assert.Equal(t, "cache", controller.detachedID) +} + +func TestDetachStorageDeviceBusy(t *testing.T) { + controller := &fakeStorageController{detachErr: corevf.ErrStorageDeviceBusy} + vm := &VzVirtualMachine{storageController: controller} + context, recorder := storageTestContext(t, http.MethodDelete, "/vm/storage/cache", nil) + context.Params = gin.Params{{Key: "id", Value: "cache"}} + + vm.DetachStorageDevice(context) + + assert.Equal(t, http.StatusConflict, recorder.Code) +} diff --git a/pkg/rest/vf/vm_config.go b/pkg/rest/vf/vm_config.go index add379bd..b4039b53 100644 --- a/pkg/rest/vf/vm_config.go +++ b/pkg/rest/vf/vm_config.go @@ -11,10 +11,18 @@ import ( type VzVirtualMachine struct { *vf.VirtualMachine + storageController storageController } func NewVzVirtualMachine(vm *vf.VirtualMachine) *VzVirtualMachine { - return &VzVirtualMachine{vm} + return &VzVirtualMachine{VirtualMachine: vm, storageController: vm} +} + +type storageController interface { + HotpluggedStorageDevices() []vf.HotpluggedStorageDevice + HotplugNBDStorageDevice(id string, spec vf.NBDStorageDevice) (bool, error) + HotplugRawStorageDevice(id string, spec vf.RawStorageDevice) (bool, error) + DetachHotpluggedStorageDevice(id string) (bool, error) } // Inspect returns information about the virtual machine like hw resources diff --git a/pkg/vf/storage_hotplug.go b/pkg/vf/storage_hotplug.go new file mode 100644 index 00000000..556ec3ac --- /dev/null +++ b/pkg/vf/storage_hotplug.go @@ -0,0 +1,476 @@ +package vf + +import ( + "errors" + "fmt" + "os" + "sort" + "strings" + "sync" + "time" + + "github.com/Code-Hex/vz/v3" + "github.com/crc-org/vfkit/pkg/config" + log "github.com/sirupsen/logrus" +) + +var ( + // ErrStorageDeviceConflict is returned when a device ID is already used by + // a device with a different configuration. + ErrStorageDeviceConflict = errors.New("storage device ID is already in use") + // ErrStorageDeviceBusy is returned when an attach or detach for the same + // device ID is still in progress. + ErrStorageDeviceBusy = errors.New("storage device operation is already in progress") + // ErrInvalidStorageDevice wraps errors caused by an unusable device + // specification (malformed NBD URI, unsuitable raw disk image, ...). + ErrInvalidStorageDevice = errors.New("invalid storage device") + // ErrUSBControllerUnavailable is returned when the virtual machine has no + // usb-xhci controller to attach devices to. + ErrUSBControllerUnavailable = errors.New("usb-xhci controller is not configured") + + errStorageDeviceNotAttached = errors.New("storage device is not attached") +) + +// States reported by HotpluggedStorageDevice.State. +const ( + // StorageDeviceStateAttached is the state of a raw-backed device. + StorageDeviceStateAttached = "attached" + // StorageDeviceStateConnecting is the state of an NBD-backed device until + // the NBD client reports a connection. + StorageDeviceStateConnecting = "connecting" + // StorageDeviceStateConnected means the NBD client is connected. + StorageDeviceStateConnected = "connected" + // StorageDeviceStateDisconnected means the NBD client reported an error + // and is trying to reconnect. + StorageDeviceStateDisconnected = "disconnected" +) + +type NBDStorageDevice struct { + URI string + Timeout time.Duration + SynchronizationMode config.NBDSynchronizationMode + ReadOnly bool +} + +type RawStorageDevice struct { + Path string + ReadOnly bool +} + +type HotpluggedStorageDevice struct { + ID string `json:"id"` + Backend string `json:"backend"` + ReadOnly bool `json:"readOnly"` + State string `json:"state"` +} + +// RedactLocation replaces location (an NBD URI or a raw disk path) in detail +// with a placeholder so the text can be logged or returned to clients. +func RedactLocation(detail, location string) string { + if location == "" { + return detail + } + return strings.ReplaceAll(detail, location, "") +} + +type storageHotplugHandle interface { + Detach() error + Connected() <-chan struct{} + DidEncounterError() <-chan error +} + +type storageHotplugBackend interface { + AttachNBDStorageDevice(spec NBDStorageDevice) (storageHotplugHandle, error) + AttachRawStorageDevice(spec RawStorageDevice) (storageHotplugHandle, error) +} + +type storageDeviceSpec struct { + backend string + location string + timeout time.Duration + synchronizationMode config.NBDSynchronizationMode + readOnly bool +} + +type hotpluggedStorageDevice struct { + spec storageDeviceSpec + handle storageHotplugHandle + state string + // pending is set while an attach or detach is in flight. The manager lock + // is released during those calls, which block until the virtualization + // framework has finished. + pending bool + stopMonitor chan struct{} +} + +type storageHotplugManager struct { + mu sync.Mutex + backend storageHotplugBackend + devices map[string]*hotpluggedStorageDevice +} + +func newStorageHotplugManager(backend storageHotplugBackend) *storageHotplugManager { + return &storageHotplugManager{ + backend: backend, + devices: make(map[string]*hotpluggedStorageDevice), + } +} + +func (vm *VirtualMachine) HotpluggedStorageDevices() []HotpluggedStorageDevice { + if vm.storageHotplug == nil { + return []HotpluggedStorageDevice{} + } + return vm.storageHotplug.List() +} + +func (vm *VirtualMachine) HotplugNBDStorageDevice(id string, spec NBDStorageDevice) (bool, error) { + if vm.storageHotplug == nil { + return false, ErrUSBControllerUnavailable + } + return vm.storageHotplug.Attach(id, spec) +} + +func (vm *VirtualMachine) HotplugRawStorageDevice(id string, spec RawStorageDevice) (bool, error) { + if vm.storageHotplug == nil { + return false, ErrUSBControllerUnavailable + } + return vm.storageHotplug.AttachRaw(id, spec) +} + +func (vm *VirtualMachine) DetachHotpluggedStorageDevice(id string) (bool, error) { + if vm.storageHotplug == nil { + return false, nil + } + return vm.storageHotplug.Detach(id) +} + +func (manager *storageHotplugManager) List() []HotpluggedStorageDevice { + manager.mu.Lock() + defer manager.mu.Unlock() + + devices := make([]HotpluggedStorageDevice, 0, len(manager.devices)) + for id, device := range manager.devices { + if device.handle == nil { + // attach still in progress + continue + } + devices = append(devices, HotpluggedStorageDevice{ + ID: id, + Backend: device.spec.backend, + ReadOnly: device.spec.readOnly, + State: device.state, + }) + } + sort.Slice(devices, func(i, j int) bool { + return devices[i].ID < devices[j].ID + }) + return devices +} + +func (manager *storageHotplugManager) Attach(id string, spec NBDStorageDevice) (bool, error) { + deviceSpec := storageDeviceSpec{ + backend: "nbd", + location: spec.URI, + timeout: spec.Timeout, + synchronizationMode: spec.SynchronizationMode, + readOnly: spec.ReadOnly, + } + return manager.attach(id, deviceSpec, func() (storageHotplugHandle, error) { + return manager.backend.AttachNBDStorageDevice(spec) + }, true) +} + +func (manager *storageHotplugManager) AttachRaw(id string, spec RawStorageDevice) (bool, error) { + deviceSpec := storageDeviceSpec{ + backend: "raw", + location: spec.Path, + readOnly: spec.ReadOnly, + } + return manager.attach(id, deviceSpec, func() (storageHotplugHandle, error) { + return manager.backend.AttachRawStorageDevice(spec) + }, false) +} + +// attach reserves id, runs the backend attach without holding the manager lock +// and then records the result, so that listing and operations on other +// devices are not blocked behind a slow attach. +func (manager *storageHotplugManager) attach( + id string, + spec storageDeviceSpec, + attach func() (storageHotplugHandle, error), + monitor bool, +) (bool, error) { + record, err := manager.reserve(id, spec) + if err != nil || record == nil { + return false, err + } + + handle, err := attach() + + manager.mu.Lock() + defer manager.mu.Unlock() + if err != nil { + delete(manager.devices, id) + return false, err + } + record.handle = handle + record.pending = false + record.state = StorageDeviceStateAttached + if monitor { + record.state = StorageDeviceStateConnecting + record.stopMonitor = make(chan struct{}) + go manager.monitorNBDStorageDevice(id, record, record.stopMonitor) + } + log.Infof("Attached %s-backed USB storage device %s", spec.backend, id) + return true, nil +} + +// reserve claims id for an attach in progress. It returns a nil record and a +// nil error when an identical device is already attached. +func (manager *storageHotplugManager) reserve(id string, spec storageDeviceSpec) (*hotpluggedStorageDevice, error) { + manager.mu.Lock() + defer manager.mu.Unlock() + + if current, ok := manager.devices[id]; ok { + if current.pending { + return nil, ErrStorageDeviceBusy + } + if current.spec == spec { + return nil, nil + } + return nil, ErrStorageDeviceConflict + } + record := &hotpluggedStorageDevice{spec: spec, pending: true} + manager.devices[id] = record + return record, nil +} + +func (manager *storageHotplugManager) Detach(id string) (bool, error) { + manager.mu.Lock() + record, ok := manager.devices[id] + if !ok { + manager.mu.Unlock() + return false, nil + } + if record.pending { + manager.mu.Unlock() + return false, ErrStorageDeviceBusy + } + record.pending = true + manager.mu.Unlock() + + err := record.handle.Detach() + + manager.mu.Lock() + defer manager.mu.Unlock() + if errors.Is(err, errStorageDeviceNotAttached) { + log.Warnf("USB storage device %s is no longer attached, dropping it", id) + err = nil + } + if err != nil { + record.pending = false + return false, err + } + delete(manager.devices, id) + if record.stopMonitor != nil { + close(record.stopMonitor) + } + log.Infof("Detached %s-backed USB storage device %s", record.spec.backend, id) + return true, nil +} + +func (manager *storageHotplugManager) setState(record *hotpluggedStorageDevice, state string) { + manager.mu.Lock() + defer manager.mu.Unlock() + record.state = state +} + +func (manager *storageHotplugManager) monitorNBDStorageDevice( + id string, + record *hotpluggedStorageDevice, + stop <-chan struct{}, +) { + // handle and spec are set before the goroutine is started and never change. + handle := record.handle + uri := record.spec.location + for { + select { + case err, ok := <-handle.DidEncounterError(): + if !ok { + return + } + detail := "unknown error" + if err != nil { + detail = RedactLocation(err.Error(), uri) + } + manager.setState(record, StorageDeviceStateDisconnected) + log.Warnf("NBD-backed USB storage device %s disconnected: %s", id, detail) + case _, ok := <-handle.Connected(): + if !ok { + return + } + manager.setState(record, StorageDeviceStateConnected) + log.Infof("NBD-backed USB storage device %s connected", id) + case <-stop: + return + } + } +} + +type vzStorageHotplugBackend struct { + vm *vz.VirtualMachine +} + +func (backend *vzStorageHotplugBackend) usbController() (*vz.USBController, error) { + controllers := backend.vm.USBControllers() + if len(controllers) != 1 { + return nil, ErrUSBControllerUnavailable + } + return controllers[0], nil +} + +func (backend *vzStorageHotplugBackend) AttachNBDStorageDevice( + spec NBDStorageDevice, +) (storageHotplugHandle, error) { + controller, err := backend.usbController() + if err != nil { + return nil, err + } + if err := config.ValidateNBDURI(spec.URI); err != nil { + return nil, fmt.Errorf("%w: %w", ErrInvalidStorageDevice, err) + } + attachment, err := vz.NewNetworkBlockDeviceStorageDeviceAttachment( + spec.URI, + spec.Timeout, + spec.ReadOnly, + nbdSynchronizationModeVZ(spec.SynchronizationMode), + ) + if err != nil { + return nil, wrapAttachmentError("could not create NBD attachment", err) + } + handle, err := attachUSBStorageDevice(controller, attachment) + if err != nil { + return nil, err + } + handle.connected = attachment.Connected() + handle.didEncounterError = attachment.DidEncounterError() + return handle, nil +} + +func (backend *vzStorageHotplugBackend) AttachRawStorageDevice( + spec RawStorageDevice, +) (storageHotplugHandle, error) { + controller, err := backend.usbController() + if err != nil { + return nil, err + } + if err := validateRawStorageDevice(spec.Path); err != nil { + return nil, err + } + storageConfig := DiskStorageConfig{ + StorageConfig: config.StorageConfig{DevName: "usb-mass-storage", ReadOnly: spec.ReadOnly}, + ImagePath: spec.Path, + } + attachment, err := storageConfig.toVz() + if err != nil { + return nil, wrapAttachmentError("could not create raw disk attachment", err) + } + handle, err := attachUSBStorageDevice(controller, attachment) + if err != nil { + return nil, err + } + return handle, nil +} + +func validateRawStorageDevice(path string) error { + info, err := os.Stat(path) + if err != nil { + return fmt.Errorf("%w: could not inspect raw disk image: %w", ErrInvalidStorageDevice, err) + } + if !info.Mode().IsRegular() { + return fmt.Errorf("%w: raw disk image is not a regular file", ErrInvalidStorageDevice) + } + if info.Size() == 0 || info.Size()%512 != 0 { + return fmt.Errorf("%w: raw disk image size must be a positive multiple of 512 bytes", ErrInvalidStorageDevice) + } + return nil +} + +// wrapAttachmentError marks framework errors caused by an unusable device +// specification as ErrInvalidStorageDevice so callers can report them as +// client errors rather than internal failures. +func wrapAttachmentError(message string, err error) error { + if isVZError(err, vz.ErrorInvalidVirtualMachineConfiguration) || isVZError(err, vz.ErrorInvalidDiskImage) { + return fmt.Errorf("%w: %s: %w", ErrInvalidStorageDevice, message, err) + } + return fmt.Errorf("%s: %w", message, err) +} + +func isVZError(err error, code vz.ErrorCode) bool { + var nsErr *vz.NSError + return errors.As(err, &nsErr) && nsErr.Domain == "VZErrorDomain" && nsErr.Code == int(code) +} + +// attachUSBStorageDevice wraps attachment in a USB mass-storage device and +// attaches it to controller. +// +// vz v3.7.1 never releases VZUSBMassStorageDevice objects and the framework +// refuses to re-attach a detached device, so every attach/detach cycle retains +// one device object (with the configuration and attachment it references) +// until vfkit exits. The controller lookup and specification validation run +// before any framework object is created to keep that set to devices the +// framework actually attached. +func attachUSBStorageDevice( + controller *vz.USBController, + attachment vz.StorageDeviceAttachment, +) (*vzStorageHotplugHandle, error) { + configuration, err := vz.NewUSBMassStorageDeviceConfiguration(attachment) + if err != nil { + return nil, fmt.Errorf("could not create USB mass-storage configuration: %w", err) + } + device, err := vz.NewUSBMassStorageDevice(configuration) + if err != nil { + return nil, fmt.Errorf("could not create USB mass-storage device: %w", err) + } + handle := &vzStorageHotplugHandle{ + controller: controller, + device: device, + configuration: configuration, + attachment: attachment, + } + if err := controller.Attach(device); err != nil { + return nil, fmt.Errorf("could not attach USB mass-storage device: %w", err) + } + return handle, nil +} + +type vzStorageHotplugHandle struct { + controller *vz.USBController + device vz.USBDevice + // configuration and attachment are never read: they keep the Go wrappers, + // and through their finalizers the Objective-C objects, alive for as long + // as the device is attached. + configuration *vz.USBMassStorageDeviceConfiguration + attachment vz.StorageDeviceAttachment + connected <-chan struct{} + didEncounterError <-chan error +} + +func (handle *vzStorageHotplugHandle) Detach() error { + err := handle.controller.Detach(handle.device) + if err == nil { + return nil + } + if isVZError(err, vz.ErrorDeviceNotFound) { + return fmt.Errorf("%w: %w", errStorageDeviceNotAttached, err) + } + return fmt.Errorf("could not detach USB mass-storage device: %w", err) +} + +func (handle *vzStorageHotplugHandle) Connected() <-chan struct{} { + return handle.connected +} + +func (handle *vzStorageHotplugHandle) DidEncounterError() <-chan error { + return handle.didEncounterError +} diff --git a/pkg/vf/storage_hotplug_test.go b/pkg/vf/storage_hotplug_test.go new file mode 100644 index 00000000..30d712a4 --- /dev/null +++ b/pkg/vf/storage_hotplug_test.go @@ -0,0 +1,300 @@ +package vf + +import ( + "errors" + "fmt" + "os" + "testing" + "time" + + "github.com/crc-org/vfkit/pkg/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +type fakeStorageHotplugHandle struct { + connected chan struct{} + errCh chan error + detachErr error + detaches int +} + +func newFakeStorageHotplugHandle() *fakeStorageHotplugHandle { + return &fakeStorageHotplugHandle{ + connected: make(chan struct{}), + errCh: make(chan error), + } +} + +func (handle *fakeStorageHotplugHandle) Detach() error { + handle.detaches++ + return handle.detachErr +} + +func (handle *fakeStorageHotplugHandle) Connected() <-chan struct{} { + return handle.connected +} + +func (handle *fakeStorageHotplugHandle) DidEncounterError() <-chan error { + return handle.errCh +} + +type fakeStorageHotplugBackend struct { + handles []*fakeStorageHotplugHandle + attachErr error + attachCalls []NBDStorageDevice + rawCalls []RawStorageDevice + // attachStarted, when set, receives a value when an attach begins; + // attachBlock, when set, must be closed before the attach completes. + attachStarted chan struct{} + attachBlock chan struct{} +} + +func (backend *fakeStorageHotplugBackend) attach() (storageHotplugHandle, error) { + if backend.attachStarted != nil { + backend.attachStarted <- struct{}{} + } + if backend.attachBlock != nil { + <-backend.attachBlock + } + if backend.attachErr != nil { + return nil, backend.attachErr + } + handle := newFakeStorageHotplugHandle() + backend.handles = append(backend.handles, handle) + return handle, nil +} + +func (backend *fakeStorageHotplugBackend) AttachNBDStorageDevice( + spec NBDStorageDevice, +) (storageHotplugHandle, error) { + backend.attachCalls = append(backend.attachCalls, spec) + return backend.attach() +} + +func (backend *fakeStorageHotplugBackend) AttachRawStorageDevice( + spec RawStorageDevice, +) (storageHotplugHandle, error) { + backend.rawCalls = append(backend.rawCalls, spec) + return backend.attach() +} + +func testNBDStorageDevice(uri string) NBDStorageDevice { + return NBDStorageDevice{ + URI: uri, + Timeout: 60 * time.Second, + SynchronizationMode: config.SynchronizationFullMode, + } +} + +func TestStorageHotplugManagerAttachIsIdempotent(t *testing.T) { + backend := &fakeStorageHotplugBackend{} + manager := newStorageHotplugManager(backend) + spec := testNBDStorageDevice("nbd://192.0.2.10:10809/export") + + created, err := manager.Attach("cache", spec) + require.NoError(t, err) + assert.True(t, created) + created, err = manager.Attach("cache", spec) + require.NoError(t, err) + assert.False(t, created) + assert.Len(t, backend.attachCalls, 1) + + _, err = manager.Detach("cache") + require.NoError(t, err) +} + +func TestStorageHotplugManagerRejectsConflictingID(t *testing.T) { + backend := &fakeStorageHotplugBackend{} + manager := newStorageHotplugManager(backend) + _, err := manager.Attach("cache", testNBDStorageDevice("nbd://192.0.2.10:10809/first")) + require.NoError(t, err) + + created, err := manager.Attach("cache", testNBDStorageDevice("nbd://192.0.2.10:10809/second")) + assert.False(t, created) + assert.ErrorIs(t, err, ErrStorageDeviceConflict) + assert.Len(t, backend.attachCalls, 1) + + _, err = manager.Detach("cache") + require.NoError(t, err) +} + +func TestStorageHotplugManagerForgetsFailedAttach(t *testing.T) { + backend := &fakeStorageHotplugBackend{attachErr: errors.New("attach failed")} + manager := newStorageHotplugManager(backend) + + created, err := manager.Attach("cache", testNBDStorageDevice("nbd://192.0.2.10:10809/export")) + assert.False(t, created) + assert.EqualError(t, err, "attach failed") + assert.Empty(t, manager.List()) + + backend.attachErr = nil + created, err = manager.Attach("cache", testNBDStorageDevice("nbd://192.0.2.10:10809/export")) + require.NoError(t, err) + assert.True(t, created) + _, err = manager.Detach("cache") + require.NoError(t, err) +} + +func TestStorageHotplugManagerRetainsDeviceAfterDetachFailure(t *testing.T) { + backend := &fakeStorageHotplugBackend{} + manager := newStorageHotplugManager(backend) + _, err := manager.Attach("cache", testNBDStorageDevice("nbd://192.0.2.10:10809/export")) + require.NoError(t, err) + backend.handles[0].detachErr = errors.New("detach failed") + + detached, err := manager.Detach("cache") + assert.False(t, detached) + assert.EqualError(t, err, "detach failed") + assert.Equal(t, []HotpluggedStorageDevice{ + {ID: "cache", Backend: "nbd", State: StorageDeviceStateConnecting}, + }, manager.List()) + + backend.handles[0].detachErr = nil + _, err = manager.Detach("cache") + require.NoError(t, err) + assert.Empty(t, manager.List()) +} + +func TestStorageHotplugManagerDropsDeviceThatIsNoLongerAttached(t *testing.T) { + backend := &fakeStorageHotplugBackend{} + manager := newStorageHotplugManager(backend) + _, err := manager.Attach("cache", testNBDStorageDevice("nbd://192.0.2.10:10809/export")) + require.NoError(t, err) + backend.handles[0].detachErr = fmt.Errorf("device gone: %w", errStorageDeviceNotAttached) + + detached, err := manager.Detach("cache") + require.NoError(t, err) + assert.True(t, detached) + assert.Empty(t, manager.List()) +} + +func TestStorageHotplugManagerDoesNotBlockWhileAttaching(t *testing.T) { + backend := &fakeStorageHotplugBackend{ + attachStarted: make(chan struct{}), + attachBlock: make(chan struct{}), + } + manager := newStorageHotplugManager(backend) + spec := testNBDStorageDevice("nbd://192.0.2.10:10809/export") + + done := make(chan error, 1) + go func() { + _, err := manager.Attach("cache", spec) + done <- err + }() + <-backend.attachStarted + + assert.Empty(t, manager.List()) + _, err := manager.Attach("cache", spec) + assert.ErrorIs(t, err, ErrStorageDeviceBusy) + _, err = manager.Detach("cache") + assert.ErrorIs(t, err, ErrStorageDeviceBusy) + + close(backend.attachBlock) + require.NoError(t, <-done) + assert.Equal(t, []HotpluggedStorageDevice{ + {ID: "cache", Backend: "nbd", State: StorageDeviceStateConnecting}, + }, manager.List()) + + _, err = manager.Detach("cache") + require.NoError(t, err) +} + +func TestStorageHotplugManagerTracksNBDConnectionState(t *testing.T) { + backend := &fakeStorageHotplugBackend{} + manager := newStorageHotplugManager(backend) + _, err := manager.Attach("cache", testNBDStorageDevice("nbd://192.0.2.10:10809/export")) + require.NoError(t, err) + handle := backend.handles[0] + stateOf := func() string { return manager.List()[0].State } + + assert.Equal(t, StorageDeviceStateConnecting, stateOf()) + handle.connected <- struct{}{} + require.Eventually(t, func() bool { return stateOf() == StorageDeviceStateConnected }, time.Second, 10*time.Millisecond) + handle.errCh <- errors.New("nbd://192.0.2.10:10809/export went away") + require.Eventually(t, func() bool { return stateOf() == StorageDeviceStateDisconnected }, time.Second, 10*time.Millisecond) + + _, err = manager.Detach("cache") + require.NoError(t, err) +} + +func TestStorageHotplugManagerListIsSortedAndDoesNotExposeURI(t *testing.T) { + backend := &fakeStorageHotplugBackend{} + manager := newStorageHotplugManager(backend) + _, err := manager.Attach("zeta", testNBDStorageDevice("nbd://192.0.2.10:10809/secret-z")) + require.NoError(t, err) + readOnly := testNBDStorageDevice("nbd://192.0.2.10:10809/secret-a") + readOnly.ReadOnly = true + _, err = manager.Attach("alpha", readOnly) + require.NoError(t, err) + + assert.Equal(t, []HotpluggedStorageDevice{ + {ID: "alpha", Backend: "nbd", ReadOnly: true, State: StorageDeviceStateConnecting}, + {ID: "zeta", Backend: "nbd", ReadOnly: false, State: StorageDeviceStateConnecting}, + }, manager.List()) + + _, err = manager.Detach("alpha") + require.NoError(t, err) + _, err = manager.Detach("zeta") + require.NoError(t, err) +} + +func TestStorageHotplugManagerDetachMissingIsIdempotent(t *testing.T) { + manager := newStorageHotplugManager(&fakeStorageHotplugBackend{}) + + detached, err := manager.Detach("missing") + require.NoError(t, err) + assert.False(t, detached) +} + +func TestStorageHotplugManagerAttachesRawImage(t *testing.T) { + backend := &fakeStorageHotplugBackend{} + manager := newStorageHotplugManager(backend) + spec := RawStorageDevice{Path: "/var/tmp/scratch.raw", ReadOnly: true} + + created, err := manager.AttachRaw("scratch", spec) + require.NoError(t, err) + assert.True(t, created) + created, err = manager.AttachRaw("scratch", spec) + require.NoError(t, err) + assert.False(t, created) + assert.Equal(t, []RawStorageDevice{spec}, backend.rawCalls) + assert.Equal(t, []HotpluggedStorageDevice{{ + ID: "scratch", + Backend: "raw", + ReadOnly: true, + State: StorageDeviceStateAttached, + }}, manager.List()) + + _, err = manager.Detach("scratch") + require.NoError(t, err) +} + +func TestValidateRawStorageDevice(t *testing.T) { + dir := t.TempDir() + + err := validateRawStorageDevice(dir + "/missing.raw") + assert.ErrorIs(t, err, ErrInvalidStorageDevice) + err = validateRawStorageDevice(dir) + assert.ErrorIs(t, err, ErrInvalidStorageDevice) + assert.Contains(t, err.Error(), "not a regular file") + + odd := dir + "/odd.raw" + require.NoError(t, writeFileOfSize(odd, 1000)) + err = validateRawStorageDevice(odd) + assert.ErrorIs(t, err, ErrInvalidStorageDevice) + assert.Contains(t, err.Error(), "multiple of 512") + + good := dir + "/good.raw" + require.NoError(t, writeFileOfSize(good, 4096)) + assert.NoError(t, validateRawStorageDevice(good)) +} + +func TestRedactLocation(t *testing.T) { + assert.Equal(t, "open : denied", RedactLocation("open /var/tmp/x.raw: denied", "/var/tmp/x.raw")) + assert.Equal(t, "unchanged", RedactLocation("unchanged", "")) +} + +func writeFileOfSize(path string, size int) error { + return os.WriteFile(path, make([]byte, size), 0600) +} diff --git a/pkg/vf/virtio.go b/pkg/vf/virtio.go index f24679cb..f9aa2edd 100644 --- a/pkg/vf/virtio.go +++ b/pkg/vf/virtio.go @@ -2,7 +2,6 @@ package vf import ( "fmt" - "net/url" "os" "path/filepath" "strings" @@ -386,21 +385,7 @@ func (dev *NetworkBlockDevice) toVz() (vz.StorageDeviceConfiguration, error) { } func (dev *NetworkBlockDevice) validateNbdURI(uri string) error { - if uri == "" { - return fmt.Errorf("'uri' must be specified") - } - - parsed, err := url.Parse(uri) - if err != nil { - return fmt.Errorf("error: %w", err) - } - - // The format specified by https://github.com/NetworkBlockDevice/nbd/blob/master/doc/uri.md - if parsed.Scheme != "nbd" && parsed.Scheme != "nbds" && parsed.Scheme != "nbd+unix" && parsed.Scheme != "nbds+unix" { - return fmt.Errorf("invalid scheme: %s. Expected one of: 'nbd', 'nbds', 'nbd+unix', or 'nbds+unix'", parsed.Scheme) - } - - return nil + return config.ValidateNBDURI(uri) } func (dev *NetworkBlockDevice) validateNbdDeviceIdentifier(deviceID string) error { @@ -420,7 +405,11 @@ func (dev *NetworkBlockDevice) validateNbdDeviceIdentifier(deviceID string) erro } func (dev *NetworkBlockDevice) SynchronizationModeVZ() vz.DiskSynchronizationMode { - if dev.SynchronizationMode == config.SynchronizationNoneMode { + return nbdSynchronizationModeVZ(dev.SynchronizationMode) +} + +func nbdSynchronizationModeVZ(mode config.NBDSynchronizationMode) vz.DiskSynchronizationMode { + if mode == config.SynchronizationNoneMode { return vz.DiskSynchronizationModeNone } return vz.DiskSynchronizationModeFull @@ -463,6 +452,8 @@ func ListenNetworkBlockDevices(vm *VirtualMachine) error { func AddToVirtualMachineConfig(vmConfig *VirtualMachineConfiguration, dev config.VirtioDevice) error { switch d := dev.(type) { + case *config.USBXHCIController: + return (*USBXHCIController)(d).AddToVirtualMachineConfig(vmConfig) case *config.USBMassStorage: return (*USBMassStorage)(d).AddToVirtualMachineConfig(vmConfig) case *config.VirtioBlk: @@ -568,3 +559,17 @@ func (dev *USBMassStorage) AddToVirtualMachineConfig(vmConfig *VirtualMachineCon type DiskStorageConfig config.DiskStorageConfig type USBMassStorage config.USBMassStorage + +type USBXHCIController config.USBXHCIController + +func (dev *USBXHCIController) AddToVirtualMachineConfig(vmConfig *VirtualMachineConfiguration) error { + if len(vmConfig.usbControllersConfiguration) != 0 { + return fmt.Errorf("only one usb-xhci controller can be configured") + } + controller, err := vz.NewXHCIControllerConfiguration() + if err != nil { + return fmt.Errorf("could not create usb-xhci controller: %w", err) + } + vmConfig.usbControllersConfiguration = append(vmConfig.usbControllersConfiguration, controller) + return nil +} diff --git a/pkg/vf/vm.go b/pkg/vf/vm.go index 480903ba..866dc6d6 100644 --- a/pkg/vf/vm.go +++ b/pkg/vf/vm.go @@ -16,7 +16,8 @@ import ( type VirtualMachine struct { *vz.VirtualMachine - vfConfig *VirtualMachineConfiguration + vfConfig *VirtualMachineConfiguration + storageHotplug *storageHotplugManager } var PlatformType string @@ -57,6 +58,7 @@ func NewVirtualMachine(vmConfig config.VirtualMachine) (*VirtualMachine, error) if err := vm.toVz(); err != nil { return nil, err } + vm.storageHotplug = newStorageHotplugManager(&vzStorageHotplugBackend{vm: vm.VirtualMachine}) return vm, nil } @@ -91,6 +93,7 @@ type VirtualMachineConfiguration struct { serialPortsConfiguration []*vz.VirtioConsoleDeviceSerialPortConfiguration socketDevicesConfiguration []vz.SocketDeviceConfiguration consolePortsConfiguration []*vz.VirtioConsolePortConfiguration + usbControllersConfiguration []vz.USBControllerConfiguration } func NewVirtualMachineConfiguration(vmConfig *config.VirtualMachine) (*VirtualMachineConfiguration, error) { @@ -167,6 +170,7 @@ func (cfg *VirtualMachineConfiguration) toVz() (*vz.VirtualMachineConfiguration, cfg.SetNetworkDevicesVirtualMachineConfiguration(cfg.networkDevicesConfiguration) cfg.SetEntropyDevicesVirtualMachineConfiguration(cfg.entropyDevicesConfiguration) cfg.SetSerialPortsVirtualMachineConfiguration(cfg.serialPortsConfiguration) + cfg.SetUSBControllersVirtualMachineConfiguration(cfg.usbControllersConfiguration) if len(cfg.consolePortsConfiguration) > 0 { consoleDeviceConfiguration, err := vz.NewVirtioConsoleDeviceConfiguration() diff --git a/test/vm_helpers.go b/test/vm_helpers.go index d126dbb6..8c830ae7 100644 --- a/test/vm_helpers.go +++ b/test/vm_helpers.go @@ -3,6 +3,7 @@ package test import ( "fmt" "net" + "net/http" "os" "os/exec" "path/filepath" @@ -20,6 +21,15 @@ import ( "golang.org/x/crypto/ssh" ) +func unixHTTPClient(socketPath string) *http.Client { + transport := &http.Transport{ + Dial: func(_, _ string) (net.Conn, error) { + return net.Dial("unix", socketPath) + }, + } + return &http.Client{Transport: transport} +} + func retryIPFromMAC(errCh chan error, macAddress string) (string, error) { var ( err error diff --git a/test/vm_test.go b/test/vm_test.go index 03e53e64..a846cba6 100644 --- a/test/vm_test.go +++ b/test/vm_test.go @@ -1,6 +1,7 @@ package test import ( + "bytes" "encoding/json" "fmt" "io" @@ -281,12 +282,7 @@ var pciidVersionedTests = map[int]map[string]pciidTest{ } func restInspect(t *testing.T, vm *testVM) *config.VirtualMachine { - tr := &http.Transport{ - Dial: func(_, _ string) (conn net.Conn, err error) { - return net.Dial("unix", vm.restSocketPath) - }, - } - client := &http.Client{Transport: tr} + client := unixHTTPClient(vm.restSocketPath) resp, err := client.Get("http://vfkit/vm/inspect") require.NoError(t, err) defer resp.Body.Close() @@ -385,6 +381,108 @@ func checkPCIDevice(t *testing.T, vm *testVM, vendorID, deviceID int) { require.Regexp(t, re, string(lspci)) } +func testHotplugRawUSBStorage(t *testing.T, vm *testVM) { + const diskSize = 16 * 1024 * 1024 + diskPath := filepath.Join(t.TempDir(), "hotplug.raw") + disk, err := os.OpenFile(diskPath, os.O_CREATE|os.O_EXCL|os.O_RDWR, 0600) + require.NoError(t, err) + require.NoError(t, disk.Truncate(diskSize)) + _, err = disk.WriteAt([]byte("host-created-disk"), 0) + require.NoError(t, err) + require.NoError(t, disk.Close()) + + client := unixHTTPClient(vm.restSocketPath) + t.Cleanup(client.CloseIdleConnections) + attached := false + t.Cleanup(func() { + if !attached { + return + } + request, cleanupErr := http.NewRequest(http.MethodDelete, "http://vfkit/vm/storage/scratch", nil) + if !assert.NoError(t, cleanupErr) { + return + } + response, cleanupErr := client.Do(request) + if !assert.NoError(t, cleanupErr) { + return + } + response.Body.Close() + assert.Equal(t, http.StatusNoContent, response.StatusCode) + }) + requestBody := bytes.NewBufferString(fmt.Sprintf( + `{"backend":"raw","path":%q,"readOnly":false}`, + diskPath, + )) + request, err := http.NewRequest(http.MethodPut, "http://vfkit/vm/storage/scratch", requestBody) + require.NoError(t, err) + request.Header.Set("Content-Type", "application/json") + response, err := client.Do(request) + require.NoError(t, err) + response.Body.Close() + require.Equal(t, http.StatusCreated, response.StatusCode) + attached = true + + deviceOutput, err := vm.SSHCombinedOutput(t, ` +for attempt in $(seq 1 100); do + for size in /sys/block/sd*/size; do + [ -e "$size" ] || continue + if [ "$(cat "$size")" = "32768" ]; then + basename "$(dirname "$size")" + exit 0 + fi + done + sleep 0.1 + done + exit 1`) + if err != nil { + diagnostics, diagnosticsErr := vm.SSHCombinedOutput(t, ` +echo '--- kernel ---' +uname -a +echo '--- block devices ---' +ls -la /sys/block +echo '--- usb devices ---' +find /sys/bus/usb/devices -maxdepth 2 -type f -name product -exec sh -c 'echo "$1: $(cat "$1")"' _ {} \; +echo '--- modules ---' +cat /proc/modules +echo '--- dmesg ---' +dmesg | tail -100`) + require.NoError(t, err, "%s\n%s\ndiagnostics error: %v", string(deviceOutput), string(diagnostics), diagnosticsErr) + } + deviceName := string(bytes.TrimSpace(deviceOutput)) + require.Regexp(t, `^sd[a-z]+$`, deviceName) + + contents, err := vm.SSHCombinedOutput(t, fmt.Sprintf( + "sudo dd if=/dev/%s bs=1 count=17 2>/dev/null", + deviceName, + )) + require.NoError(t, err) + require.Equal(t, "host-created-disk", string(contents)) + + _, err = vm.SSHCombinedOutput(t, fmt.Sprintf( + "printf guest-wrote-data | sudo dd of=/dev/%s bs=1 seek=4096 conv=notrunc 2>/dev/null && sync", + deviceName, + )) + require.NoError(t, err) + + request, err = http.NewRequest(http.MethodDelete, "http://vfkit/vm/storage/scratch", nil) + require.NoError(t, err) + response, err = client.Do(request) + require.NoError(t, err) + response.Body.Close() + require.Equal(t, http.StatusNoContent, response.StatusCode) + attached = false + + _, err = vm.SSHCombinedOutput(t, fmt.Sprintf( + "for attempt in $(seq 1 100); do [ ! -e /sys/block/%s ] && exit 0; sleep 0.1; done; exit 1", + deviceName, + )) + require.NoError(t, err) + + diskContents, err := os.ReadFile(diskPath) + require.NoError(t, err) + require.Equal(t, "guest-wrote-data", string(diskContents[4096:4096+len("guest-wrote-data")])) +} + func TestCloudInit(t *testing.T) { if err := macOSAvailable(13); err != nil { t.Log("Skipping TestCloudInit test") @@ -397,7 +495,7 @@ func TestCloudInit(t *testing.T) { require.NoError(t, err) // set efi bootloader - fedoraProvider.efiVariableStorePath = "efi-variable-store" + fedoraProvider.efiVariableStorePath = filepath.Join(tempDir, "efi-variable-store") fedoraProvider.createVariableStore = true vm := NewTestVM(t, fedoraProvider) @@ -433,6 +531,13 @@ func TestCloudInit(t *testing.T) { vm.AddDevice(t, dev) log.Infof("shared disk: %s - cloud-init", dev.DevName) + hotplugSupported := macOSAvailable(15) == nil + if hotplugSupported { + xhci, xhciErr := config.USBXHCIControllerNew() + require.NoError(t, xhciErr) + vm.AddDevice(t, xhci) + } + vm.Start(t) vm.WaitForSSH(t) @@ -440,6 +545,11 @@ func TestCloudInit(t *testing.T) { require.NoError(t, err) log.Infof("executed whoami - output: %s", string(data)) require.Equal(t, "vfkituser\n", string(data)) + if hotplugSupported { + t.Run("hotplug-raw-usb-storage", func(t *testing.T) { + testHotplugRawUSBStorage(t, vm) + }) + } log.Info("stopping vm") vm.Stop(t)