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)