Merge pull request #43368 from TBBle/generalised-Windows-device-syntax

Introduce `://` syntax for Windows Devices in DeviceMapping.PathOnHost

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)
+	if err != nil {
+		return err
 	}
 
+	s.Windows.Devices = append(s.Windows.Devices, devices...)
+
 	return nil
 }
 
@@ -465,3 +451,31 @@ func readCredentialSpecFile(id, root, location string) (string, error) {
 	}
 	return string(bcontents[:]), nil
 }
+
+func setupWindowsDevices(devices []containertypes.DeviceMapping) (specDevices []specs.WindowsDevice, err error) {
+	if len(devices) == 0 {
+		return
+	}
+
+	for _, deviceMapping := range devices {
+		devicePath := deviceMapping.PathOnHost
+		if strings.HasPrefix(devicePath, "class/") {
+			devicePath = strings.Replace(devicePath, "class/", "class://", 1)
+		}
+
+		srcParts := strings.SplitN(devicePath, "://", 2)
+		if len(srcParts) != 2 {
+			return nil, errors.Errorf("invalid device assignment path: '%s', must be 'class/ID' or 'IDType://ID'", deviceMapping.PathOnHost)
+		}
+		if srcParts[0] == "" {
+			return nil, errors.Errorf("invalid device assignment path: '%s', IDType cannot be empty", deviceMapping.PathOnHost)
+		}
+		wd := specs.WindowsDevice{
+			ID:     srcParts[1],
+			IDType: srcParts[0],
+		}
+		specDevices = append(specDevices, wd)
+	}
+
+	return
+}

daemon/oci_windows_test.go

@@ -311,3 +311,83 @@ 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", func(t *testing.T) {
+		devices, err := setupWindowsDevices(nil)
+		assert.NilError(t, err)
+		assert.Equal(t, len(devices), 0)
+	})
+
+	t.Run("it fails if any devices are blank", func(t *testing.T) {
+		devices, err := setupWindowsDevices([]containertypes.DeviceMapping{{PathOnHost: "class/anything"}, {PathOnHost: ""}})
+		assert.ErrorContains(t, err, "invalid device assignment path")
+		assert.ErrorContains(t, err, "''")
+		assert.Equal(t, len(devices), 0)
+	})
+
+	t.Run("it fails if all devices do not contain '/' or '://'", func(t *testing.T) {
+		devices, err := setupWindowsDevices([]containertypes.DeviceMapping{{PathOnHost: "anything"}, {PathOnHost: "goes"}})
+		assert.ErrorContains(t, err, "invalid device assignment path")
+		assert.ErrorContains(t, err, "'anything'")
+		assert.Equal(t, len(devices), 0)
+	})
+
+	t.Run("it fails if any devices do not contain '/' or '://'", func(t *testing.T) {
+		devices, err := setupWindowsDevices([]containertypes.DeviceMapping{{PathOnHost: "class/anything"}, {PathOnHost: "goes"}})
+		assert.ErrorContains(t, err, "invalid device assignment path")
+		assert.ErrorContains(t, err, "'goes'")
+		assert.Equal(t, len(devices), 0)
+	})
+
+	t.Run("it fails if all '/'-separated devices do not have IDType 'class'", func(t *testing.T) {
+		devices, err := setupWindowsDevices([]containertypes.DeviceMapping{{PathOnHost: "klass/anything"}, {PathOnHost: "klass/goes"}})
+		assert.ErrorContains(t, err, "invalid device assignment path")
+		assert.ErrorContains(t, err, "'klass/anything'")
+		assert.Equal(t, len(devices), 0)
+	})
+
+	t.Run("it fails if any '/'-separated devices do not have IDType 'class'", func(t *testing.T) {
+		devices, err := setupWindowsDevices([]containertypes.DeviceMapping{{PathOnHost: "class/anything"}, {PathOnHost: "klass/goes"}})
+		assert.ErrorContains(t, err, "invalid device assignment path")
+		assert.ErrorContains(t, err, "'klass/goes'")
+		assert.Equal(t, len(devices), 0)
+	})
+
+	t.Run("it fails if any '://'-separated devices have IDType ''", func(t *testing.T) {
+		devices, err := setupWindowsDevices([]containertypes.DeviceMapping{{PathOnHost: "class/anything"}, {PathOnHost: "://goes"}})
+		assert.ErrorContains(t, err, "invalid device assignment path")
+		assert.ErrorContains(t, err, "'://goes'")
+		assert.Equal(t, len(devices), 0)
+	})
+
+	t.Run("it creates devices if all '/'-separated devices have IDType 'class'", func(t *testing.T) {
+		devices, err := setupWindowsDevices([]containertypes.DeviceMapping{{PathOnHost: "class/anything"}, {PathOnHost: "class/goes"}})
+		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])
+		}
+	})
+
+	t.Run("it creates devices if all '://'-separated devices have non-blank IDType", func(t *testing.T) {
+		devices, err := setupWindowsDevices([]containertypes.DeviceMapping{{PathOnHost: "class://anything"}, {PathOnHost: "klass://goes"}})
+		expectedDevices := []specs.WindowsDevice{{IDType: "class", ID: "anything"}, {IDType: "klass", ID: "goes"}}
+		assert.NilError(t, err)
+		assert.Equal(t, len(devices), len(expectedDevices))
+		for i := range expectedDevices {
+			assert.Equal(t, devices[i], expectedDevices[i])
+		}
+	})
+
+	t.Run("it creates devices when given a mix of '/'-separated and '://'-separated devices", func(t *testing.T) {
+		devices, err := setupWindowsDevices([]containertypes.DeviceMapping{{PathOnHost: "class/anything"}, {PathOnHost: "klass://goes"}})
+		expectedDevices := []specs.WindowsDevice{{IDType: "class", ID: "anything"}, {IDType: "klass", ID: "goes"}}
+		assert.NilError(t, err)
+		assert.Equal(t, len(devices), len(expectedDevices))
+		for i := range expectedDevices {
+			assert.Equal(t, devices[i], expectedDevices[i])
+		}
+	})
+}

docs/api/version-history.md

@@ -65,6 +65,11 @@ keywords: "API, Docker, rcli, REST, documentation"
     - "locked"
     - "active/worker"
     - "active/manager"
+* `POST /containers/create` for Windows containers now accepts a new syntax in
+  `HostConfig.Resources.Devices.PathOnHost`. As well as the existing `class/<GUID>`
+  syntax, `<IDType>://<ID>` is now recognised. Support for specific `<IDType>` values
+  depends on the underlying implementation and Windows version. This change is not
+  versioned, and affects all API versions if the daemon has this patch.
 
 ## v1.41 API changes
 

integration-cli/docker_cli_ps_test.go

@@ -540,7 +540,7 @@ func (s *DockerSuite) TestPsListContainersFilterCreated(c *testing.T) {
 	// filter containers by 'create' - note, no -a needed
 	out, _ = dockerCmd(c, "ps", "-q", "-f", "status=created")
 	containerOut := strings.TrimSpace(out)
-	assert.Assert(c, strings.HasPrefix(cID, containerOut))
+	assert.Assert(c, strings.Contains(containerOut, shortCID), "Should have seen '%s' in ps output:\n%s", shortCID, out)
 }
 
 // Test for GitHub issue #12595

integration/container/devices_windows_test.go

@@ -0,0 +1,128 @@
+package container // import "github.com/docker/docker/integration/container"
+
+import (
+	"context"
+	"strings"
+	"testing"
+	"time"
+
+	"github.com/docker/docker/api/types"
+	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
+		isolation                   containertypes.Isolation
+		expectedStartFailure        bool
+		expectedStartFailureMessage string
+		expectedExitCode            int
+		expectedStdout              string
+		expectedStderr              string
+	}{
+		{
+			doc:              "process/no device mounted",
+			isolation:        containertypes.IsolationProcess,
+			expectedExitCode: 1,
+		},
+		{
+			doc:            "process/class/5B45201D-F2F2-4F3B-85BB-30FF1F953599 mounted",
+			devices:        []string{"class/5B45201D-F2F2-4F3B-85BB-30FF1F953599"},
+			isolation:      containertypes.IsolationProcess,
+			expectedStdout: "/Windows/System32/HostDriverStore/FileRepository",
+		},
+		{
+			doc:            "process/class://5B45201D-F2F2-4F3B-85BB-30FF1F953599 mounted",
+			devices:        []string{"class://5B45201D-F2F2-4F3B-85BB-30FF1F953599"},
+			isolation:      containertypes.IsolationProcess,
+			expectedStdout: "/Windows/System32/HostDriverStore/FileRepository",
+		},
+		{
+			doc:            "process/vpci-class-guid://5B45201D-F2F2-4F3B-85BB-30FF1F953599 mounted",
+			devices:        []string{"vpci-class-guid://5B45201D-F2F2-4F3B-85BB-30FF1F953599"},
+			isolation:      containertypes.IsolationProcess,
+			expectedStdout: "/Windows/System32/HostDriverStore/FileRepository",
+		},
+		{
+			doc:              "hyperv/no device mounted",
+			isolation:        containertypes.IsolationHyperV,
+			expectedExitCode: 1,
+		},
+		{
+			doc:                         "hyperv/class/5B45201D-F2F2-4F3B-85BB-30FF1F953599 mounted",
+			devices:                     []string{"class/5B45201D-F2F2-4F3B-85BB-30FF1F953599"},
+			isolation:                   containertypes.IsolationHyperV,
+			expectedStartFailure:        !testEnv.RuntimeIsWindowsContainerd(),
+			expectedStartFailureMessage: "device assignment is not supported for HyperV containers",
+			expectedStdout:              "/Windows/System32/HostDriverStore/FileRepository",
+		},
+		{
+			doc:                         "hyperv/class://5B45201D-F2F2-4F3B-85BB-30FF1F953599 mounted",
+			devices:                     []string{"class://5B45201D-F2F2-4F3B-85BB-30FF1F953599"},
+			isolation:                   containertypes.IsolationHyperV,
+			expectedStartFailure:        !testEnv.RuntimeIsWindowsContainerd(),
+			expectedStartFailureMessage: "device assignment is not supported for HyperV containers",
+			expectedStdout:              "/Windows/System32/HostDriverStore/FileRepository",
+		},
+		{
+			doc:                         "hyperv/vpci-class-guid://5B45201D-F2F2-4F3B-85BB-30FF1F953599 mounted",
+			devices:                     []string{"vpci-class-guid://5B45201D-F2F2-4F3B-85BB-30FF1F953599"},
+			isolation:                   containertypes.IsolationHyperV,
+			expectedStartFailure:        !testEnv.RuntimeIsWindowsContainerd(),
+			expectedStartFailureMessage: "device assignment is not supported for HyperV containers",
+			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(d.isolation)}
+			for _, deviceName := range d.devices {
+				deviceOptions = append(deviceOptions, container.WithWindowsDevice(deviceName))
+			}
+
+			id := container.Create(ctx, t, client, deviceOptions...)
+
+			// Hyper-V isolation is failing even with no actual devices added.
+			// TODO: Once https://github.com/moby/moby/issues/43395 is resolved,
+			// remove this skip.If and validate the expected behaviour under Hyper-V.
+			skip.If(t, d.isolation == containertypes.IsolationHyperV && !d.expectedStartFailure, "FIXME. HyperV isolation setup is probably incorrect in the test")
+
+			err := client.ContainerStart(ctx, id, types.ContainerStartOptions{})
+			if d.expectedStartFailure {
+				assert.ErrorContains(t, err, d.expectedStartFailureMessage)
+				return
+			}
+
+			assert.NilError(t, err)
+
+			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
+	}
+}

libcontainerd/local/local_windows.go

@@ -312,6 +312,11 @@ func (c *client) createWindows(id string, spec *specs.Spec, runtimeOptions inter
 			return errors.New("device assignment is not supported for HyperV containers")
 		}
 		for _, d := range spec.Windows.Devices {
+			// Per https://github.com/microsoft/hcsshim/blob/v0.9.2/internal/uvm/virtual_device.go#L17-L18,
+			// these represent an Interface Class GUID.
+			if d.IDType != "class" && d.IDType != "vpci-class-guid" {
+				return errors.Errorf("device assignment of type '%s' is not supported", d.IDType)
+			}
 			configuration.AssignedDevices = append(configuration.AssignedDevices, hcsshim.AssignedDevice{InterfaceClassGUID: d.ID})
 		}
 	}