Break out `setupWindowsDevices` and add tests

Since this function is about to get more complicated, and change
behaviour, this establishes tests for the existing implementation.

Signed-off-by: Paul "TBBle" Hampson <Paul.Hampson@Pobox.com>

daemon/oci_windows.go

@@ -254,27 +254,13 @@ func (daemon *Daemon) createSpecWindowsFields(c *container.Container, s *specs.S
 		return err
 	}
 
-	// Do we have any assigned devices?
-	if len(c.HostConfig.Devices) > 0 {
-		if isHyperV {
-			return errors.New("device assignment is not supported for HyperV containers")
-		}
-		for _, deviceMapping := range c.HostConfig.Devices {
-			srcParts := strings.SplitN(deviceMapping.PathOnHost, "/", 2)
-			if len(srcParts) != 2 {
-				return errors.New("invalid device assignment path")
-			}
-			if srcParts[0] != "class" {
-				return errors.Errorf("invalid device assignment type: '%s' should be 'class'", srcParts[0])
-			}
-			wd := specs.WindowsDevice{
-				ID:     srcParts[1],
-				IDType: srcParts[0],
-			}
-			s.Windows.Devices = append(s.Windows.Devices, wd)
-		}
+	devices, err := setupWindowsDevices(c.HostConfig.Devices, isHyperV)
+	if err != nil {
+		return err
 	}
 
+	s.Windows.Devices = append(s.Windows.Devices, devices...)
+
 	return nil
 }
 
@@ -465,3 +451,30 @@ func readCredentialSpecFile(id, root, location string) (string, error) {
 	}
 	return string(bcontents[:]), nil
 }
+
+func setupWindowsDevices(devices []containertypes.DeviceMapping, isHyperV bool) (specDevices []specs.WindowsDevice, err error) {
+	if len(devices) == 0 {
+		return
+	}
+
+	if isHyperV {
+		return nil, errors.New("device assignment is not supported for HyperV containers")
+	}
+
+	for _, deviceMapping := range devices {
+		srcParts := strings.SplitN(deviceMapping.PathOnHost, "/", 2)
+		if len(srcParts) != 2 {
+			return nil, errors.New("invalid device assignment path")
+		}
+		if srcParts[0] != "class" {
+			return nil, errors.Errorf("invalid device assignment type: '%s' should be 'class'", srcParts[0])
+		}
+		wd := specs.WindowsDevice{
+			ID:     srcParts[1],
+			IDType: srcParts[0],
+		}
+		specDevices = append(specDevices, wd)
+	}
+
+	return
+}

daemon/oci_windows_test.go

@@ -311,3 +311,63 @@ func setRegistryOpenKeyFunc(t *testing.T, key *dummyRegistryKey, err ...error) f
 		registryOpenKeyFunc = previousRegistryOpenKeyFunc
 	}
 }
+
+func TestSetupWindowsDevices(t *testing.T) {
+	t.Run("it does nothing if there are no devices and HyperV is disabled", func(t *testing.T) {
+		devices, err := setupWindowsDevices(nil, false)
+		assert.NilError(t, err)
+		assert.Equal(t, len(devices), 0)
+	})
+
+	t.Run("it does nothing if there are no devices and HyperV is enabled", func(t *testing.T) {
+		devices, err := setupWindowsDevices(nil, true)
+		assert.NilError(t, err)
+		assert.Equal(t, len(devices), 0)
+	})
+
+	t.Run("it fails if there are devices and HyperV is enabled", func(t *testing.T) {
+		devices, err := setupWindowsDevices([]containertypes.DeviceMapping{{PathOnHost: "anything"}}, true)
+		assert.ErrorContains(t, err, "device assignment is not supported for HyperV containers")
+		assert.Equal(t, len(devices), 0)
+	})
+
+	t.Run("it fails if any devices are blank and HyperV is disabled", func(t *testing.T) {
+		devices, err := setupWindowsDevices([]containertypes.DeviceMapping{{PathOnHost: "class/anything"}, {PathOnHost: ""}}, false)
+		assert.ErrorContains(t, err, "invalid device assignment path")
+		assert.Equal(t, len(devices), 0)
+	})
+
+	t.Run("it fails if all devices do not contain '/' and HyperV is disabled", func(t *testing.T) {
+		devices, err := setupWindowsDevices([]containertypes.DeviceMapping{{PathOnHost: "anything"}, {PathOnHost: "goes"}}, false)
+		assert.ErrorContains(t, err, "invalid device assignment path")
+		assert.Equal(t, len(devices), 0)
+	})
+
+	t.Run("it fails if any devices do not contain '/' and HyperV is disabled", func(t *testing.T) {
+		devices, err := setupWindowsDevices([]containertypes.DeviceMapping{{PathOnHost: "class/anything"}, {PathOnHost: "goes"}}, false)
+		assert.ErrorContains(t, err, "invalid device assignment path")
+		assert.Equal(t, len(devices), 0)
+	})
+
+	t.Run("it fails if all devices do not have IDType 'class' and HyperV is disabled", func(t *testing.T) {
+		devices, err := setupWindowsDevices([]containertypes.DeviceMapping{{PathOnHost: "klass/anything"}, {PathOnHost: "klass/goes"}}, false)
+		assert.ErrorContains(t, err, "invalid device assignment type: 'klass' should be 'class'")
+		assert.Equal(t, len(devices), 0)
+	})
+
+	t.Run("it fails if any devices do not have IDType 'class' and HyperV is disabled", func(t *testing.T) {
+		devices, err := setupWindowsDevices([]containertypes.DeviceMapping{{PathOnHost: "class/anything"}, {PathOnHost: "klass/goes"}}, false)
+		assert.ErrorContains(t, err, "invalid device assignment type: 'klass' should be 'class'")
+		assert.Equal(t, len(devices), 0)
+	})
+
+	t.Run("it creates devices if all devices have IDType 'class' and HyperV is disabled", func(t *testing.T) {
+		devices, err := setupWindowsDevices([]containertypes.DeviceMapping{{PathOnHost: "class/anything"}, {PathOnHost: "class/goes"}}, false)
+		expectedDevices := []specs.WindowsDevice{{IDType: "class", ID: "anything"}, {IDType: "class", ID: "goes"}}
+		assert.NilError(t, err)
+		assert.Equal(t, len(devices), len(expectedDevices))
+		for i := range expectedDevices {
+			assert.Equal(t, devices[i], expectedDevices[i])
+		}
+	})
+}

integration/container/devices_windows_test.go

@@ -0,0 +1,67 @@
+package container // import "github.com/docker/docker/integration/container"
+
+import (
+	"context"
+	"strings"
+	"testing"
+	"time"
+
+	containertypes "github.com/docker/docker/api/types/container"
+	"github.com/docker/docker/integration/internal/container"
+	"gotest.tools/v3/assert"
+	"gotest.tools/v3/poll"
+	"gotest.tools/v3/skip"
+)
+
+// TestWindowsDevices that Windows Devices are correctly propagated
+// via HostConfig.Devices through to the implementation in hcsshim.
+func TestWindowsDevices(t *testing.T) {
+	skip.If(t, testEnv.DaemonInfo.OSType != "windows")
+	defer setupTest(t)()
+	client := testEnv.APIClient()
+	ctx := context.Background()
+
+	testData := []struct {
+		doc              string
+		devices          []string
+		expectedExitCode int
+		expectedStdout   string
+		expectedStderr   string
+	}{
+		{
+			doc:              "no device mounted",
+			expectedExitCode: 1,
+		},
+		{
+			doc:            "class/5B45201D-F2F2-4F3B-85BB-30FF1F953599 mounted",
+			devices:        []string{"class/5B45201D-F2F2-4F3B-85BB-30FF1F953599"},
+			expectedStdout: "/Windows/System32/HostDriverStore/FileRepository",
+		},
+	}
+
+	for _, d := range testData {
+		d := d
+		t.Run(d.doc, func(t *testing.T) {
+			t.Parallel()
+			deviceOptions := []func(*container.TestContainerConfig){container.WithIsolation(containertypes.IsolationProcess)}
+			for _, deviceName := range d.devices {
+				deviceOptions = append(deviceOptions, container.WithWindowsDevice(deviceName))
+			}
+			id := container.Run(ctx, t, client, deviceOptions...)
+
+			poll.WaitOn(t, container.IsInState(ctx, client, id, "running"), poll.WithDelay(100*time.Millisecond))
+
+			// /Windows/System32/HostDriverStore is mounted from the host when class GUID 5B45201D-F2F2-4F3B-85BB-30FF1F953599
+			// is mounted. See `C:\windows\System32\containers\devices.def` on a Windows host for (slightly more) details.
+			res, err := container.Exec(ctx, client, id, []string{"sh", "-c",
+				"ls -d /Windows/System32/HostDriverStore/* | grep /Windows/System32/HostDriverStore/FileRepository"})
+			assert.NilError(t, err)
+			assert.Equal(t, d.expectedExitCode, res.ExitCode)
+			if d.expectedExitCode == 0 {
+				assert.Equal(t, d.expectedStdout, strings.TrimSpace(res.Stdout()))
+				assert.Equal(t, d.expectedStderr, strings.TrimSpace(res.Stderr()))
+			}
+
+		})
+	}
+}

integration/internal/container/ops.go

@@ -213,3 +213,17 @@ func WithPlatform(p *specs.Platform) func(*TestContainerConfig) {
 		c.Platform = p
 	}
 }
+
+// WithWindowsDevice specifies a Windows Device, ala `--device` on the CLI
+func WithWindowsDevice(device string) func(*TestContainerConfig) {
+	return func(c *TestContainerConfig) {
+		c.HostConfig.Devices = append(c.HostConfig.Devices, containertypes.DeviceMapping{PathOnHost: device})
+	}
+}
+
+// WithIsolation specifies the isolation technology to apply to the container
+func WithIsolation(isolation containertypes.Isolation) func(*TestContainerConfig) {
+	return func(c *TestContainerConfig) {
+		c.HostConfig.Isolation = isolation
+	}
+}